From 74e3b996ffa7ca77bcedf6fe7828eb0331fb8d5c Mon Sep 17 00:00:00 2001 From: Sebastian Schuberth Date: Tue, 24 Jan 2023 10:45:58 +0100 Subject: [PATCH] refactor(ClearlyDefinedStorage)!: Simplify exception handling 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 --- .../kotlin/storages/ClearlyDefinedStorage.kt | 97 +++++++------------ .../storages/ClearlyDefinedStorageTest.kt | 16 ++- 2 files changed, 40 insertions(+), 73 deletions(-) diff --git a/scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt b/scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt index bcf87bed89541..4cfab1045cae8 100644 --- a/scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt +++ b/scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt @@ -19,8 +19,6 @@ package org.ossreviewtoolkit.scanner.storages -import java.io.IOException - import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.runBlocking @@ -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.") } /** @@ -112,43 +108,25 @@ class ClearlyDefinedStorage( } override fun readInternal(id: Identifier): Result> = - readPackageFromClearlyDefined(id, null, null) + runBlocking(Dispatchers.IO) { readFromClearlyDefined(Package.EMPTY.copy(id = id)) } override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result> = - readPackageFromClearlyDefined(pkg.id, pkg.vcs, pkg.sourceArtifact.takeIf { it.url.isNotEmpty() }) + runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) } override fun addInternal(id: Identifier, scanResult: ScanResult): Result = 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> = - try { - runBlocking(Dispatchers.IO) { readFromClearlyDefined(id, packageCoordinates(id, vcs, sourceArtifact)) } - } catch (e: IllegalArgumentException) { - e.showStackTrace() + private suspend fun readFromClearlyDefined(pkg: Package): Result> = + 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> { - logger.info { "Looking up results for '${id.toCoordinates()}'." } - - return try { val tools = service.harvestTools( coordinates.type, coordinates.provider, @@ -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> { - 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> { + private suspend fun loadScanCodeResults(coordinates: Coordinates, version: String): List { val toolResponse = service.harvestToolData( coordinates.type, coordinates.provider, @@ -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() } } } diff --git a/scanner/src/test/kotlin/storages/ClearlyDefinedStorageTest.kt b/scanner/src/test/kotlin/storages/ClearlyDefinedStorageTest.kt index b07eb908a5d6d..8c3ea1095d130 100644 --- a/scanner/src/test/kotlin/storages/ClearlyDefinedStorageTest.kt +++ b/scanner/src/test/kotlin/storages/ClearlyDefinedStorageTest.kt @@ -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 @@ -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() } - "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( @@ -387,9 +387,7 @@ class ClearlyDefinedStorageTest : WordSpec({ val result = storage.read(TEST_IDENTIFIER) - result.shouldBeSuccess { - it should beEmpty() - } + result.shouldBeFailure() } "return an empty result if a harvest tool file request returns an unexpected result" {