Skip to content

Commit

Permalink
refactor(ClearlyDefinedStorage)!: Simplify exception handling
Browse files Browse the repository at this point in the history 
Catch all kinds of exceptions inside `readFromClearlyDefined()` already,
which allows to simplify code and to remove the `handleException()` and
`readPackageFromClearlyDefined()` functions.

This is a breaking change as previously empty results are now partly
failures.

Relates to #6367.

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
  • Loading branch information
@sschuberth
sschuberth committed Jan 26, 2023
1 parent d568697 commit 74e3b99
Show file tree
Hide file tree
Showing 2 changed files with 40 additions and 73 deletions.
97 changes: 33 additions & 64 deletions scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

package org.ossreviewtoolkit.scanner.storages

import java.io.IOException

import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking

Expand Down Expand Up @@ -71,18 +69,16 @@ private fun SourceLocation.toCoordinates(): Coordinates = Coordinates(type, prov
private fun VcsInfo.toVcsInfoCurationData(): VcsInfoCurationData = VcsInfoCurationData(type, url, revision, path)

/**
* Generate the coordinates for ClearlyDefined based on the [id], the [vcs], and a [sourceArtifact].
* If information about a Git repository in GitHub is available, this is used. Otherwise, the coordinates
* Generate the coordinates for ClearlyDefined based on the [package][pkg]'s [Package.vcs] [Package.sourceArtifact]
* properties. If information about a Git repository in GitHub is available, this is used. Otherwise, the coordinates
* are derived from the identifier. Throws [IllegalArgumentException] if generating coordinates is not possible.
*/
private fun packageCoordinates(
id: Identifier,
vcs: VcsInfo?,
sourceArtifact: RemoteArtifact?
): Coordinates {
val sourceLocation = id.toClearlyDefinedSourceLocation(vcs?.toVcsInfoCurationData(), sourceArtifact)
return sourceLocation?.toCoordinates() ?: id.toClearlyDefinedCoordinates()
?: throw IllegalArgumentException("Unable to create ClearlyDefined coordinates for '${id.toCoordinates()}'.")
private fun packageCoordinates(pkg: Package): Coordinates {
val vcs = pkg.vcs.takeUnless { it.url.isEmpty() }
val sourceArtifact = pkg.sourceArtifact.takeUnless { it.url.isEmpty() }
val sourceLocation = pkg.id.toClearlyDefinedSourceLocation(vcs?.toVcsInfoCurationData(), sourceArtifact)
return sourceLocation?.toCoordinates() ?: pkg.id.toClearlyDefinedCoordinates()
?: throw IllegalArgumentException("Unable to create ClearlyDefined coordinates for $pkg.")
}

/**
Expand Down Expand Up @@ -112,43 +108,25 @@ class ClearlyDefinedStorage(
}

override fun readInternal(id: Identifier): Result<List<ScanResult>> =
readPackageFromClearlyDefined(id, null, null)
runBlocking(Dispatchers.IO) { readFromClearlyDefined(Package.EMPTY.copy(id = id)) }

override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result<List<ScanResult>> =
readPackageFromClearlyDefined(pkg.id, pkg.vcs, pkg.sourceArtifact.takeIf { it.url.isNotEmpty() })
runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) }

override fun addInternal(id: Identifier, scanResult: ScanResult): Result<Unit> =
Result.failure(ScanStorageException("Adding scan results directly to ClearlyDefined is not supported."))

/**
* Try to obtain a [ScanResult] produced by ScanCode from ClearlyDefined for the package with the given [id].
* Use the VCS information [vcs] and an optional [sourceArtifact] to determine the coordinates for looking up
* the result.
* Try to obtain a [ScanResult] produced by ScanCode from ClearlyDefined using the given [package][pkg]. Its
* coordinates may not necessarily be equivalent to the identifier, as we try to lookup source code results if a
* GitHub repository is known.
*/
private fun readPackageFromClearlyDefined(
id: Identifier,
vcs: VcsInfo?,
sourceArtifact: RemoteArtifact?
): Result<List<ScanResult>> =
try {
runBlocking(Dispatchers.IO) { readFromClearlyDefined(id, packageCoordinates(id, vcs, sourceArtifact)) }
} catch (e: IllegalArgumentException) {
e.showStackTrace()
private suspend fun readFromClearlyDefined(pkg: Package): Result<List<ScanResult>> =
runCatching {
val coordinates = packageCoordinates(pkg)

logger.warn { "Could not obtain ClearlyDefined coordinates for package '${id.toCoordinates()}'." }
logger.info { "Looking up results for '${pkg.id.toCoordinates()}' with $coordinates." }

EMPTY_RESULT
}

/**
* Try to obtain a [ScanResult] produced by ScanCode from ClearlyDefined for the package with the given [id],
* using the given [coordinates] for looking up the data. These may not necessarily be equivalent to the
* identifier, as we try to lookup source code results if a GitHub repository is known.
*/
private suspend fun readFromClearlyDefined(id: Identifier, coordinates: Coordinates): Result<List<ScanResult>> {
logger.info { "Looking up results for '${id.toCoordinates()}'." }

return try {
val tools = service.harvestTools(
coordinates.type,
coordinates.provider,
Expand All @@ -159,38 +137,29 @@ class ClearlyDefinedStorage(

findScanCodeVersion(tools, coordinates)?.let { version ->
loadScanCodeResults(coordinates, version)
} ?: EMPTY_RESULT
} catch (e: HttpException) {
e.response()?.errorBody()?.string()?.let {
logger.error { "Error response from ClearlyDefined is: $it" }
}

handleException(id, e)
} catch (e: IOException) {
// There are some other exceptions thrown by Retrofit to be handled as well, e.g. ConnectException.
handleException(id, e)
}
}
}.orEmpty()
}.recoverCatching { e ->
e.showStackTrace()

/**
* Log the exception that occurred during a request to ClearlyDefined and construct an error result from it.
*/
private fun handleException(id: Identifier, e: Exception): Result<List<ScanResult>> {
e.showStackTrace()
val message = "Error when reading results for '${pkg.id.toCoordinates()}' from ClearlyDefined: " +
e.collectMessages()

val message = "Error when reading results for package '${id.toCoordinates()}' from ClearlyDefined: " +
e.collectMessages()
logger.error { message }

logger.error { message }
if (e is HttpException) {
e.response()?.errorBody()?.string()?.let {
logger.error { "Error response from ClearlyDefined was: $it" }
}
}

return Result.failure(ScanStorageException(message))
}
throw ScanStorageException(message)
}

/**
* Load the ScanCode results file for the package with the given [coordinates] from ClearlyDefined.
* The results have been produced by ScanCode in the given [version].
*/
private suspend fun loadScanCodeResults(coordinates: Coordinates, version: String): Result<List<ScanResult>> {
private suspend fun loadScanCodeResults(coordinates: Coordinates, version: String): List<ScanResult> {
val toolResponse = service.harvestToolData(
coordinates.type,
coordinates.provider,
Expand Down Expand Up @@ -235,8 +204,8 @@ class ClearlyDefinedStorage(
val summary = generateSummary(SpdxConstants.NONE, result)
val details = generateScannerDetails(result)

Result.success(listOf(ScanResult(provenance, details, summary)))
} ?: EMPTY_RESULT
listOf(ScanResult(provenance, details, summary))
}.orEmpty()
}
}
}
16 changes: 7 additions & 9 deletions scanner/src/test/kotlin/storages/ClearlyDefinedStorageTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.VcsType
import org.ossreviewtoolkit.model.config.ClearlyDefinedStorageConfiguration
import org.ossreviewtoolkit.scanner.ScanStorageException
import org.ossreviewtoolkit.scanner.ScannerCriteria

import org.semver4j.Semver
Expand Down Expand Up @@ -365,17 +366,16 @@ class ClearlyDefinedStorageTest : WordSpec({
storage.read(pkg, SCANNER_CRITERIA).shouldBeValid()
}

"return an empty result if the coordinates are not supported by ClearlyDefined" {
"return a failure if the coordinates are not supported by ClearlyDefined" {
val id = TEST_IDENTIFIER.copy(type = "unknown")

val storage = ClearlyDefinedStorage(storageConfiguration(server))

storage.read(id).shouldBeSuccess {
it should beEmpty()
}
val result = storage.read(id)

result.shouldBeFailure<ScanStorageException>()
}

"return an empty result if a harvest tool request returns an unexpected result" {
"return a failure if a harvest tool request returns an unexpected result" {
server.stubFor(
get(anyUrl())
.willReturn(
Expand All @@ -387,9 +387,7 @@ class ClearlyDefinedStorageTest : WordSpec({

val result = storage.read(TEST_IDENTIFIER)

result.shouldBeSuccess {
it should beEmpty()
}
result.shouldBeFailure<ScanStorageException>()
}

"return an empty result if a harvest tool file request returns an unexpected result" {
Expand Down

0 comments on commit 74e3b99

Please sign in to comment.