refactor(PackageBasedScanStorageReader)!: Make read() take a Package

A *package* based reader should indeed take a `Package` for all its
`read()` functions. This aligns with the existing `read()` function that
takes a `ScannerCriteria`, and it is a requirement for a proper
implementation of `ClearlyDefinedStorage` which uses coordinates with a
"provider" for lookup that can only be correctly constructed when
provenance information is available. So far the `ClearlyDefinedStorage`
hard-codes some assumptions in this regard, which will be corrected in
upcoming changes.

Signed-off-by: Sebastian Schuberth <[email protected]>

helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt

@@ -29,6 +29,7 @@ import com.github.ajalt.clikt.parameters.types.file
 
 import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.LicenseFinding
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.config.OrtConfiguration
 import org.ossreviewtoolkit.model.utils.FindingCurationMatcher
@@ -73,7 +74,7 @@ internal class GetPackageLicensesCommand : CliktCommand(
         .convert { it.expandTilde() }
 
     override fun run() {
-        val scanResults = getStoredScanResults(packageId)
+        val scanResults = getStoredScanResults(Package.EMPTY.copy(id = packageId))
         val packageConfigurationProvider = DirPackageConfigurationProvider(packageConfigurationsDir)
 
         val result = scanResults.firstOrNull()?.let { scanResult ->
@@ -104,10 +105,10 @@ internal class GetPackageLicensesCommand : CliktCommand(
         println(result.toYaml())
     }
 
-    private fun getStoredScanResults(id: Identifier): List<ScanResult> {
+    private fun getStoredScanResults(pkg: Package): List<ScanResult> {
         val ortConfiguration = OrtConfiguration.load(configArguments, configFile)
         val scanStorages = ScanStorages.createFromConfig(ortConfiguration.scanner)
-        return runCatching { scanStorages.read(id) }.getOrDefault(emptyList())
+        return runCatching { scanStorages.read(pkg) }.getOrDefault(emptyList())
     }
 }
 

helper-cli/src/main/kotlin/commands/ListStoredScanResultsCommand.kt

@@ -30,6 +30,7 @@ import com.github.ajalt.clikt.parameters.types.file
 
 import org.ossreviewtoolkit.helper.utils.logger
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.config.OrtConfiguration
 import org.ossreviewtoolkit.model.toYaml
 import org.ossreviewtoolkit.scanner.ScanStorages
@@ -68,7 +69,7 @@ internal class ListStoredScanResultsCommand : CliktCommand(
             "Searching for scan results of '${packageId.toCoordinates()}' in ${scanStorages.readers.size} storage(s)."
         )
 
-        val scanResults = runCatching { scanStorages.read(packageId) }.getOrElse {
+        val scanResults = runCatching { scanStorages.read(Package.EMPTY.copy(id = packageId)) }.getOrElse {
             logger.error { "Could not read scan results: ${it.message}" }
             throw ProgramResult(1)
         }

helper-cli/src/main/kotlin/commands/packageconfig/CreateCommand.kt

@@ -36,6 +36,7 @@ import org.ossreviewtoolkit.helper.utils.sortPathExcludes
 import org.ossreviewtoolkit.helper.utils.write
 import org.ossreviewtoolkit.model.ArtifactProvenance
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.RepositoryProvenance
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.config.PackageConfiguration
@@ -119,7 +120,7 @@ internal class CreateCommand : CliktCommand(
         outputDir.safeMkdirs()
 
         val scanResultsStorage = FileBasedStorage(LocalFileStorage(scanResultsStorageDir))
-        val scanResults = scanResultsStorage.read(packageId).getOrThrow().run {
+        val scanResults = scanResultsStorage.read(Package.EMPTY.copy(id = packageId)).getOrThrow().run {
             listOfNotNull(
                 find { it.provenance is RepositoryProvenance },
                 find { it.provenance is ArtifactProvenance }

scanner/src/funTest/kotlin/storages/AbstractStorageFunTest.kt

@@ -128,7 +128,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
                 val scanResult = ScanResult(provenanceWithSourceArtifact1, scannerDetails1, scanSummaryWithFiles)
 
                 val addResult = storage.add(id1, scanResult)
-                val readResult = storage.read(id1)
+                val readResult = storage.read(pkg1)
 
                 addResult.shouldBeSuccess()
                 readResult.shouldBeSuccess {
@@ -140,7 +140,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
                 val scanResult = ScanResult(provenanceEmpty, scannerDetails1, scanSummaryWithFiles)
 
                 val addResult = storage.add(id1, scanResult)
-                val readResult = storage.read(id1)
+                val readResult = storage.read(pkg1)
 
                 addResult.shouldBeFailure {
                     it.message shouldBe "Not storing scan result for '${id1.toCoordinates()}' because no provenance " +
@@ -167,7 +167,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
                 addResult1.shouldBeSuccess()
                 addResult2.shouldBeFailure()
 
-                val readResult = storage.read(id1)
+                val readResult = storage.read(pkg1)
                 readResult.shouldBeSuccess {
                     it should containExactly(scanResult1)
                 }
@@ -181,7 +181,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
 
                 storage.add(id1, scanResult1).shouldBeSuccess()
                 storage.add(id1, scanResult2).shouldBeSuccess()
-                val readResult = storage.read(id1)
+                val readResult = storage.read(pkg1)
 
                 readResult.shouldBeSuccess {
                     it should containExactlyInAnyOrder(scanResult1, scanResult2)

scanner/src/funTest/kotlin/storages/ClearlyDefinedStorageFunTest.kt

@@ -29,8 +29,12 @@ import java.time.Instant
 import kotlin.time.Duration.Companion.seconds
 
 import org.ossreviewtoolkit.clients.clearlydefined.ClearlyDefinedService.Server
+import org.ossreviewtoolkit.model.Hash
+import org.ossreviewtoolkit.model.HashAlgorithm
 import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.LicenseFinding
+import org.ossreviewtoolkit.model.Package
+import org.ossreviewtoolkit.model.RemoteArtifact
 import org.ossreviewtoolkit.model.RepositoryProvenance
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.ScanSummary
@@ -44,10 +48,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
     val storage = ClearlyDefinedStorage(ClearlyDefinedStorageConfiguration(Server.PRODUCTION.apiUrl))
 
     "Scan results for 'semver4j' from ScanCode should be read correctly" {
-        val id = Identifier("Maven:com.vdurmont:semver4j:3.1.0")
+        val pkg = Package.EMPTY.copy(
+            id = Identifier("Maven:com.vdurmont:semver4j:3.1.0"),
+            binaryArtifact = RemoteArtifact(
+                url = "https://repo1.maven.org/maven2/com/vdurmont/semver4j/3.1.0/semver4j-3.1.0.jar",
+                hash = Hash(
+                    value = "0de1248f09dfe8df3b021c84e0642ee222cceb13",
+                    algorithm = HashAlgorithm.SHA1
+                )
+            )
+        )
 
         withRetry {
-            val results = storage.read(id)
+            val results = storage.read(pkg)
 
             results.shouldBeSuccess {
                 it shouldContain ScanResult(
@@ -98,10 +111,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
     }
 
     "Scan results for 'hoplite-core' from ScanCode should be read correctly" {
-        val id = Identifier("Maven:com.sksamuel.hoplite:hoplite-core:2.1.3")
+        val pkg = Package.EMPTY.copy(
+            id = Identifier("Maven:com.sksamuel.hoplite:hoplite-core:2.1.3"),
+            binaryArtifact = RemoteArtifact(
+                url = "https://repo1.maven.org/maven2/com/sksamuel/hoplite/hoplite-core/2.1.3/hoplite-core-2.1.3.jar",
+                hash = Hash(
+                    value = "143f3c28ac4987907473fb608bce1fe317663ba8",
+                    algorithm = HashAlgorithm.SHA1
+                )
+            )
+        )
 
         withRetry {
-            val results = storage.read(id)
+            val results = storage.read(pkg)
 
             results.shouldBeSuccess {
                 it shouldContain ScanResult(
@@ -132,10 +154,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
     }
 
     "Scan results for packages without a namespace should be present" {
-        val id = Identifier("NPM::iobroker.eusec:0.9.9")
+        val pkg = Package.EMPTY.copy(
+            id = Identifier("NPM::iobroker.eusec:0.9.9"),
+            binaryArtifact = RemoteArtifact(
+                url = "https://registry.npmjs.org/iobroker.eusec/-/iobroker.eusec-0.9.9.tgz",
+                hash = Hash(
+                    value = "b3cf4aeefd31f64907cab7ea7a419f1054137a09",
+                    algorithm = HashAlgorithm.SHA1
+                )
+            )
+        )
 
         withRetry {
-            val results = storage.read(id)
+            val results = storage.read(pkg)
 
             results.shouldBeSuccess { result ->
                 result.map { it.provenance } shouldContain RepositoryProvenance(

scanner/src/main/kotlin/ScanResultsStorage.kt

@@ -55,15 +55,15 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
     open val name: String = javaClass.simpleName
 
     /**
-     * Return all [ScanResult]s contained in this [ScanResultsStorage] corresponding to the package denoted by the given
-     * [id] wrapped in a [Result].
+     * Return all [ScanResult]s contained in this [ScanResultsStorage] corresponding to the [package][pkg] wrapped in a
+     * [Result].
      */
-    fun read(id: Identifier): Result<List<ScanResult>> {
-        val (result, duration) = measureTimedValue { readInternal(id) }
+    fun read(pkg: Package): Result<List<ScanResult>> {
+        val (result, duration) = measureTimedValue { readInternal(pkg) }
 
         result.onSuccess { results ->
             logger.info {
-                "Read ${results.size} scan result(s) for '${id.toCoordinates()}' from ${javaClass.simpleName} in " +
+                "Read ${results.size} scan result(s) for '${pkg.id.toCoordinates()}' from ${javaClass.simpleName} in " +
                     "$duration."
             }
         }
@@ -143,14 +143,14 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
     /**
      * Internal version of [read].
      */
-    protected abstract fun readInternal(id: Identifier): Result<List<ScanResult>>
+    protected abstract fun readInternal(pkg: Package): Result<List<ScanResult>>
 
     /**
      * Internal version of [read]. Implementations may want to override this function if they can filter for the wanted
      * [scannerCriteria] in a more efficient way.
      */
     protected open fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result<List<ScanResult>> =
-        readInternal(pkg.id).map { results ->
+        readInternal(pkg).map { results ->
             if (results.isEmpty()) {
                 results
             } else {
@@ -210,8 +210,8 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
      */
     protected abstract fun addInternal(id: Identifier, scanResult: ScanResult): Result<Unit>
 
-    override fun read(id: Identifier, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult> =
-        read(id).toNestedProvenanceScanResult(nestedProvenance)
+    override fun read(pkg: Package, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult> =
+        read(pkg).toNestedProvenanceScanResult(nestedProvenance)
 
     override fun read(
         pkg: Package,

scanner/src/main/kotlin/ScanStorage.kt

@@ -20,7 +20,6 @@
 package org.ossreviewtoolkit.scanner
 
 import org.ossreviewtoolkit.model.ArtifactProvenance
-import org.ossreviewtoolkit.model.Identifier
 import org.ossreviewtoolkit.model.KnownProvenance
 import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.Provenance
@@ -40,12 +39,12 @@ sealed interface ScanStorageReader
  */
 interface PackageBasedScanStorageReader : ScanStorageReader {
     /**
-     * Read all [ScanResult]s for the provided [id]. The package scan results are converted to a
+     * Read all [ScanResult]s for the provided [package][pkg]. The package scan results are converted to a
      * [NestedProvenanceScanResult] using the provided [nestedProvenance].
      *
      * Throws a [ScanStorageException] if an error occurs while reading from the storage.
      */
-    fun read(id: Identifier, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult>
+    fun read(pkg: Package, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult>
 
     /**
      * Read all [ScanResult]s for the provided [package][pkg] matching the [provenance][KnownProvenance.matches] and the

scanner/src/main/kotlin/ScanStorages.kt

@@ -19,7 +19,7 @@
 
 package org.ossreviewtoolkit.scanner
 
-import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.config.ClearlyDefinedStorageConfiguration
 import org.ossreviewtoolkit.model.config.FileBasedStorageConfiguration
@@ -80,11 +80,11 @@ class ScanStorages(
     }
 
     /**
-     * Read all [ScanResult]s for the provided [id]. Returns an empty list if no stored scan results or no stored
-     * provenance can be found.
+     * Read all [ScanResult]s for the provided [package][pkg]. Returns an empty list if no stored scan results or no
+     * stored provenance can be found.
      */
-    fun read(id: Identifier): List<ScanResult> {
-        val packageProvenances = packageProvenanceStorage.readProvenances(id)
+    fun read(pkg: Package): List<ScanResult> {
+        val packageProvenances = packageProvenanceStorage.readProvenances(pkg.id)
 
         val nestedProvenances = packageProvenances.mapNotNull { result ->
             when (result) {
@@ -104,7 +104,7 @@ class ScanStorages(
 
         nestedProvenances.forEach { nestedProvenance ->
             results += readers.filterIsInstance<PackageBasedScanStorageReader>().flatMap { reader ->
-                reader.read(id, nestedProvenance).flatMap { it.merge() }
+                reader.read(pkg, nestedProvenance).flatMap { it.merge() }
             }
 
             results += readers.filterIsInstance<ProvenanceBasedScanStorageReader>().mapNotNull { reader ->

scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt

@@ -85,8 +85,8 @@ class ClearlyDefinedStorage(
         ClearlyDefinedService.create(config.serverUrl, client ?: OkHttpClientHelper.buildClient())
     }
 
-    override fun readInternal(id: Identifier): Result<List<ScanResult>> =
-        runBlocking(Dispatchers.IO) { readFromClearlyDefined(Package.EMPTY.copy(id = id)) }
+    override fun readInternal(pkg: Package): Result<List<ScanResult>> =
+        runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) }
 
     override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result<List<ScanResult>> =
         runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) }

scanner/src/main/kotlin/storages/FileBasedStorage.kt

@@ -28,6 +28,7 @@ import java.io.IOException
 import org.apache.logging.log4j.kotlin.Logging
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.yamlMapper
 import org.ossreviewtoolkit.scanner.ScanResultsStorage
@@ -51,8 +52,8 @@ class FileBasedStorage(
 
     override val name = "${javaClass.simpleName} with ${backend.javaClass.simpleName} backend"
 
-    override fun readInternal(id: Identifier): Result<List<ScanResult>> {
-        val path = storagePath(id)
+    override fun readInternal(pkg: Package): Result<List<ScanResult>> {
+        val path = storagePath(pkg.id)
 
         return runCatching {
             backend.read(path).use { input ->
@@ -62,7 +63,7 @@ class FileBasedStorage(
             // If the file cannot be found it means no scan results have been stored, yet.
             if (it is FileNotFoundException) return EMPTY_RESULT
 
-            val message = "Could not read scan results for '${id.toCoordinates()}' from path '$path': " +
+            val message = "Could not read scan results for '${pkg.id.toCoordinates()}' from path '$path': " +
                 it.collectMessages()
 
             logger.info { message }
@@ -72,7 +73,9 @@ class FileBasedStorage(
     }
 
     override fun addInternal(id: Identifier, scanResult: ScanResult): Result<Unit> {
-        val existingScanResults = read(id).getOrDefault(emptyList())
+        // Note: The file-based `read()`-implementation does not require the full `Package` information for reading
+        // existing results, so it is fine to use an empty package here.
+        val existingScanResults = read(Package.EMPTY.copy(id = id)).getOrDefault(emptyList())
 
         if (existingScanResults.any { it.scanner == scanResult.scanner && it.provenance == scanResult.provenance }) {
             val message = "Did not store scan result for '${id.toCoordinates()}' because a scan result for the same " +

scanner/src/main/kotlin/storages/NoStorage.kt

@@ -31,7 +31,7 @@ import org.ossreviewtoolkit.scanner.ScannerCriteria
  * errors.
  */
 class NoStorage : ScanResultsStorage() {
-    override fun readInternal(id: Identifier) = EMPTY_RESULT
+    override fun readInternal(pkg: Package) = EMPTY_RESULT
     override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria) = EMPTY_RESULT
     override fun addInternal(id: Identifier, scanResult: ScanResult) = Result.success(Unit)
 }

scanner/src/main/kotlin/storages/PostgresStorage.kt

@@ -131,16 +131,16 @@ class PostgresStorage(
             """.trimIndent()
         )
 
-    override fun readInternal(id: Identifier): Result<List<ScanResult>> =
+    override fun readInternal(pkg: Package): Result<List<ScanResult>> =
         runCatching {
             database.transaction {
-                ScanResultDao.find { ScanResults.identifier eq id.toCoordinates() }.map { it.scanResult }
+                ScanResultDao.find { ScanResults.identifier eq pkg.id.toCoordinates() }.map { it.scanResult }
             }
         }.onFailure {
             if (it is JsonProcessingException || it is SQLException) {
                 it.showStackTrace()
 
-                val message = "Could not read scan results for '${id.toCoordinates()}' from database: " +
+                val message = "Could not read scan results for '${pkg.id.toCoordinates()}' from database: " +
                     it.collectMessages()
 
                 logger.info { message }

scanner/src/main/kotlin/storages/Sw360Storage.kt

@@ -37,6 +37,7 @@ import org.eclipse.sw360.http.HttpClientFactoryImpl
 import org.eclipse.sw360.http.config.HttpClientConfig
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.ScanResult
 import org.ossreviewtoolkit.model.config.Sw360StorageConfiguration
 import org.ossreviewtoolkit.model.jsonMapper
@@ -85,19 +86,19 @@ class Sw360Storage(
     private val connectionFactory = createConnection(configuration)
     private val releaseClient = connectionFactory.releaseAdapter
 
-    override fun readInternal(id: Identifier): Result<List<ScanResult>> {
-        val tempScanResultFile = createTempFileForUpload(id)
+    override fun readInternal(pkg: Package): Result<List<ScanResult>> {
+        val tempScanResultFile = createTempFileForUpload(pkg.id)
 
         val result = runCatching {
-            releaseClient.getSparseReleaseByNameAndVersion(createReleaseName(id), id.version)
+            releaseClient.getSparseReleaseByNameAndVersion(createReleaseName(pkg.id), pkg.id.version)
                 .flatMap { releaseClient.getReleaseById(it.releaseId) }
                 .map { getScanResultOfRelease(it, tempScanResultFile.toPath()) }
                 .orElse(emptyList())
                 .map { path ->
                     path.toFile().readValue<ScanResult>()
                 }
         }.recoverCatching {
-            val message = "Could not read scan results for '${id.toCoordinates()}' in SW360: ${it.message}"
+            val message = "Could not read scan results for '${pkg.id.toCoordinates()}' in SW360: ${it.message}"
 
             logger.info { message }