refactor(PackageCurationProvider): Query packages instead of ids

Strictly speaking, a package id is not enough to query curations as
different servers might host different artifacts under the same id. A
typical example is an internal fork of an upstream artifact that is
internally hosted under the same id.

Currently, ClearlyDefined is the only curation provider that allows to
take this into account via its "provider" concept, see [1] and [2] for
related discussions. So, as a preparation for ORT to replace the
hard-coded providers [3] with real ones determined based on URLs, change
the curation provider API to take whole packages instead of only their
ids, so that implementations have access to artifacts and VCS URLs.

[1]: #155
[2]: package-url/purl-spec#33
[3]: https://github.com/oss-review-toolkit/ort/blob/33531c7/model/src/main/kotlin/utils/Extensions.kt#L38-L57

Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>

analyzer/src/funTest/kotlin/curation/ClearlyDefinedPackageCurationProviderFunTest.kt

@@ -28,35 +28,36 @@ import io.kotest.matchers.shouldNot
 
 import org.ossreviewtoolkit.clients.clearlydefined.ClearlyDefinedService.Server
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.utils.spdx.toSpdx
 
 class ClearlyDefinedPackageCurationProviderFunTest : WordSpec({
     "The production server" should {
         val provider = ClearlyDefinedPackageCurationProvider()
 
         "return an existing curation for the javax.servlet-api Maven package" {
-            val identifier = Identifier("Maven:javax.servlet:javax.servlet-api:3.1.0")
+            val packages = createPackagesFromIds("Maven:javax.servlet:javax.servlet-api:3.1.0")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should haveSize(1)
             curations.values.flatten().first().data.concludedLicense shouldBe
                     "CDDL-1.0 OR GPL-2.0-only WITH Classpath-exception-2.0".toSpdx()
         }
 
         "return an existing curation for the slf4j-log4j12 Maven package" {
-            val identifier = Identifier("Maven:org.slf4j:slf4j-log4j12:1.7.30")
+            val packages = createPackagesFromIds("Maven:org.slf4j:slf4j-log4j12:1.7.30")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should haveSize(1)
             curations.values.flatten().first().data.vcs?.revision shouldBe "0b97c416e42a184ff9728877b461c616187c58f7"
         }
 
         "return no curation for a non-existing dummy NPM package" {
-            val identifier = Identifier("NPM:@scope:name:1.2.3")
+            val packages = createPackagesFromIds("NPM:@scope:name:1.2.3")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should beEmpty()
         }
@@ -66,18 +67,18 @@ class ClearlyDefinedPackageCurationProviderFunTest : WordSpec({
         val provider = ClearlyDefinedPackageCurationProvider(Server.DEVELOPMENT)
 
         "return an existing curation for the platform-express NPM package" {
-            val identifier = Identifier("NPM:@nestjs:platform-express:6.2.3")
+            val packages = createPackagesFromIds("NPM:@nestjs:platform-express:6.2.3")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should haveSize(1)
             curations.values.flatten().first().data.concludedLicense shouldBe "Apache-1.0".toSpdx()
         }
 
         "return no curation for a non-existing dummy Maven package" {
-            val identifier = Identifier("Maven:group:name:1.2.3")
+            val packages = createPackagesFromIds("Maven:group:name:1.2.3")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should beEmpty()
         }
@@ -92,20 +93,23 @@ class ClearlyDefinedPackageCurationProviderFunTest : WordSpec({
             val provider = ClearlyDefinedPackageCurationProvider(config)
 
             // Use an id which is known to have non-empty results from an earlier test.
-            val identifier = Identifier("Maven:org.slf4j:slf4j-log4j12:1.7.30")
+            val packages = createPackagesFromIds("Maven:org.slf4j:slf4j-log4j12:1.7.30")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations should beEmpty()
         }
 
         "be retrieved for packages without a namespace" {
             val provider = ClearlyDefinedPackageCurationProvider()
-            val identifier = Identifier("NPM::acorn:0.6.0")
+            val packages = createPackagesFromIds("NPM::acorn:0.6.0")
 
-            val curations = provider.getCurationsFor(listOf(identifier))
+            val curations = provider.getCurationsFor(packages)
 
             curations shouldNot beEmpty()
         }
     }
 })
+
+private fun createPackagesFromIds(vararg ids: String) =
+    ids.map { Package.EMPTY.copy(id = Identifier(it)) }

analyzer/src/funTest/kotlin/curation/OrtConfigPackageCurationProviderFunTest.kt

@@ -27,24 +27,29 @@ import io.kotest.matchers.should
 import io.kotest.matchers.shouldNot
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 
 class OrtConfigPackageCurationProviderFunTest : StringSpec({
     "provider can load curations from the ort-config repository" {
         val azureCore = Identifier("NuGet::Azure.Core:1.22.0")
         val azureCoreAmqp = Identifier("NuGet::Azure.Core.Amqp:1.2.0")
+        val packages = createPackagesFromIds(azureCore, azureCoreAmqp)
 
-        val curations = OrtConfigPackageCurationProvider().getCurationsFor(listOf(azureCore, azureCoreAmqp))
+        val curations = OrtConfigPackageCurationProvider().getCurationsFor(packages)
 
         curations.shouldContainKeys(azureCore, azureCoreAmqp)
         curations.getValue(azureCore) shouldNot beEmpty()
         curations.getValue(azureCoreAmqp) shouldNot beEmpty()
     }
 
     "provider does not fail for packages which have no curations" {
-        val id = Identifier("Some:Bogus:Package:Id")
+        val packages = createPackagesFromIds(Identifier("Some:Bogus:Package:Id"))
 
-        val curations = OrtConfigPackageCurationProvider().getCurationsFor(listOf(id))
+        val curations = OrtConfigPackageCurationProvider().getCurationsFor(packages)
 
         curations should beEmptyMap()
     }
 })
+
+private fun createPackagesFromIds(vararg ids: Identifier) =
+    ids.map { Package.EMPTY.copy(id = it) }

analyzer/src/main/kotlin/Analyzer.kt

@@ -341,12 +341,11 @@ private fun resolveConfiguration(
     analyzerResult: AnalyzerResult,
     curationProviders: List<Pair<String, PackageCurationProvider>>
 ): ResolvedConfiguration {
-    val packageIds = analyzerResult.packages.mapTo(mutableSetOf()) { it.id }
     val packageCurations = mutableListOf<PackageCuration>()
 
     val duration = measureTime {
         curationProviders.forEach { (_, curationProvider) ->
-            packageCurations += curationProvider.getCurationsFor(packageIds).values.flatten()
+            packageCurations += curationProvider.getCurationsFor(analyzerResult.packages).values.flatten()
         }
     }
 

analyzer/src/main/kotlin/PackageCurationProvider.kt

@@ -20,6 +20,7 @@
 package org.ossreviewtoolkit.analyzer
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.config.PackageCurationProviderConfiguration
 import org.ossreviewtoolkit.utils.common.ConfigurablePluginFactory
@@ -83,10 +84,10 @@ interface PackageCurationProviderFactory<CONFIG> : ConfigurablePluginFactory<Pac
  */
 fun interface PackageCurationProvider {
     /**
-     * Return all available [PackageCuration]s for the provided [pkgIds], associated by the package's [Identifier]. Each
-     * list of curations must be non-empty; if no curation is available for a package, the returned map must not contain
-     * a key for that package's identifier at all.
+     * Return all available [PackageCuration]s for the provided [packages], associated by the package's [Identifier].
+     * Each list of curations must be non-empty; if no curation is available for a package, the returned map must not
+     * contain a key for that package's identifier at all.
      */
     // TODO: Maybe make this a suspend function, then all implementing classes could deal with coroutines more easily.
-    fun getCurationsFor(pkgIds: Collection<Identifier>): Map<Identifier, List<PackageCuration>>
+    fun getCurationsFor(packages: Collection<Package>): Map<Identifier, List<PackageCuration>>
 }

analyzer/src/main/kotlin/curation/ClearlyDefinedPackageCurationProvider.kt

@@ -38,6 +38,7 @@ import org.ossreviewtoolkit.clients.clearlydefined.getCurationsChunked
 import org.ossreviewtoolkit.clients.clearlydefined.getDefinitionsChunked
 import org.ossreviewtoolkit.model.Hash
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.PackageCurationData
 import org.ossreviewtoolkit.model.RemoteArtifact
@@ -97,11 +98,11 @@ class ClearlyDefinedPackageCurationProvider(
         ClearlyDefinedService.create(config.serverUrl, client ?: OkHttpClientHelper.buildClient())
     }
 
-    override fun getCurationsFor(pkgIds: Collection<Identifier>): Map<Identifier, List<PackageCuration>> {
-        val coordinatesToIds = pkgIds.mapNotNull { pkgId ->
-            pkgId.toClearlyDefinedTypeAndProvider()?.let { (type, provider) ->
-                val namespace = pkgId.namespace.takeUnless { it.isEmpty() }
-                Coordinates(type, provider, namespace, pkgId.name, pkgId.version) to pkgId
+    override fun getCurationsFor(packages: Collection<Package>): Map<Identifier, List<PackageCuration>> {
+        val coordinatesToIds = packages.mapNotNull { pkg ->
+            pkg.toClearlyDefinedTypeAndProvider()?.let { (type, provider) ->
+                val namespace = pkg.id.namespace.takeUnless { it.isEmpty() }
+                Coordinates(type, provider, namespace, pkg.id.name, pkg.id.version) to pkg.id
             }
         }.toMap()
 

analyzer/src/main/kotlin/curation/OrtConfigPackageCurationProvider.kt

@@ -28,6 +28,7 @@ import org.ossreviewtoolkit.analyzer.PackageCurationProvider
 import org.ossreviewtoolkit.analyzer.PackageCurationProviderFactory
 import org.ossreviewtoolkit.downloader.vcs.Git
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.VcsInfo
 import org.ossreviewtoolkit.model.VcsType
@@ -60,9 +61,9 @@ open class OrtConfigPackageCurationProvider : PackageCurationProvider {
         }
     }
 
-    override fun getCurationsFor(pkgIds: Collection<Identifier>) =
-        pkgIds.mapNotNull { pkgId ->
-            getCurationsFor(pkgId).takeUnless { it.isEmpty() }?.let { pkgId to it }
+    override fun getCurationsFor(packages: Collection<Package>) =
+        packages.mapNotNull { pkg ->
+            getCurationsFor(pkg.id).takeUnless { it.isEmpty() }?.let { pkg.id to it }
         }.toMap()
 
     private fun getCurationsFor(pkgId: Identifier): List<PackageCuration> {

analyzer/src/main/kotlin/curation/SimplePackageCurationProvider.kt

@@ -20,15 +20,15 @@
 package org.ossreviewtoolkit.analyzer.curation
 
 import org.ossreviewtoolkit.analyzer.PackageCurationProvider
-import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 
 /**
  * A [PackageCurationProvider] that provides the specified [packageCurations].
  */
 open class SimplePackageCurationProvider(val packageCurations: List<PackageCuration>) : PackageCurationProvider {
-    override fun getCurationsFor(pkgIds: Collection<Identifier>) =
-        pkgIds.mapNotNull { pkgId ->
-            packageCurations.filter { it.isApplicable(pkgId) }.takeUnless { it.isEmpty() }?.let { pkgId to it }
+    override fun getCurationsFor(packages: Collection<Package>) =
+        packages.mapNotNull { pkg ->
+            packageCurations.filter { it.isApplicable(pkg.id) }.takeUnless { it.isEmpty() }?.let { pkg.id to it }
         }.toMap()
 }

analyzer/src/main/kotlin/curation/Sw360PackageCurationProvider.kt

@@ -37,6 +37,7 @@ import org.ossreviewtoolkit.analyzer.PackageCurationProviderFactory
 import org.ossreviewtoolkit.model.Hash
 import org.ossreviewtoolkit.model.HashAlgorithm
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.PackageCurationData
 import org.ossreviewtoolkit.model.RemoteArtifact
@@ -97,9 +98,9 @@ class Sw360PackageCurationProvider(config: Sw360StorageConfiguration) : PackageC
     private val connectionFactory = createConnection(config)
     private val releaseClient = connectionFactory.releaseAdapter
 
-    override fun getCurationsFor(pkgIds: Collection<Identifier>) =
-        pkgIds.mapNotNull { pkgId ->
-            getCurationsFor(pkgId).takeUnless { it.isEmpty() }?.let { pkgId to it }
+    override fun getCurationsFor(packages: Collection<Package>) =
+        packages.mapNotNull { pkg ->
+            getCurationsFor(pkg.id).takeUnless { it.isEmpty() }?.let { pkg.id to it }
         }.toMap()
 
     private fun getCurationsFor(pkgId: Identifier): List<PackageCuration> {

analyzer/src/test/kotlin/curation/ClearlyDefinedPackageCurationProviderTest.kt

@@ -32,6 +32,7 @@ import io.kotest.matchers.should
 import java.time.Duration
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.utils.ort.OkHttpClientHelper
 
 /**
@@ -68,9 +69,10 @@ class ClearlyDefinedPackageCurationProviderTest : WordSpec({
             }
 
             val provider = ClearlyDefinedPackageCurationProvider("http://localhost:${server.port()}", client)
-            val ids = listOf(Identifier("Maven:some-ns:some-component:1.2.3"))
+            val id = Identifier("Maven:some-ns:some-component:1.2.3")
+            val packages = listOf(Package.EMPTY.copy(id = id))
 
-            provider.getCurationsFor(ids) should beEmpty()
+            provider.getCurationsFor(packages) should beEmpty()
         }
     }
 })

analyzer/src/test/kotlin/curation/FilePackageCurationProviderTest.kt

@@ -21,14 +21,17 @@ package org.ossreviewtoolkit.analyzer.curation
 
 import io.kotest.assertions.throwables.shouldThrow
 import io.kotest.core.spec.style.StringSpec
+import io.kotest.inspectors.forAll
 import io.kotest.matchers.collections.beEmpty
 import io.kotest.matchers.collections.haveSize
+import io.kotest.matchers.collections.shouldContainExactlyInAnyOrder
 import io.kotest.matchers.should
 import io.kotest.matchers.shouldBe
 
 import java.io.File
 
 import org.ossreviewtoolkit.model.Identifier
+import org.ossreviewtoolkit.model.Package
 
 class FilePackageCurationProviderTest : StringSpec() {
     private val curationsDir = File("src/test/assets/package-curations-dir")
@@ -47,11 +50,13 @@ class FilePackageCurationProviderTest : StringSpec() {
                 Identifier("maven", "org.ossreviewtoolkit", "example", "1.0"),
                 Identifier("maven", "org.foo", "bar", "0.42")
             )
+            val packages = idsWithExistingCurations.map { Package.EMPTY.copy(id = it) }
 
-            idsWithExistingCurations.forEach {
-                val curations = provider.getCurationsFor(listOf(it)).values.flatten()
+            val curations = provider.getCurationsFor(packages)
 
-                curations should haveSize(1)
+            curations.keys shouldContainExactlyInAnyOrder idsWithExistingCurations
+            curations.values.forAll {
+                it should haveSize(1)
             }
         }
 
@@ -65,19 +70,22 @@ class FilePackageCurationProviderTest : StringSpec() {
                 Identifier("maven", "org.ossreviewtoolkit", "example", "1.0"),
                 Identifier("maven", "org.foo", "bar", "0.42")
             )
+            val packages = idsWithExistingCurations.map { Package.EMPTY.copy(id = it) }
 
-            idsWithExistingCurations.forEach {
-                val curations = provider.getCurationsFor(listOf(it)).values.flatten()
+            val curations = provider.getCurationsFor(packages)
 
-                curations should haveSize(1)
+            curations.keys shouldContainExactlyInAnyOrder idsWithExistingCurations
+            curations.values.forAll {
+                it should haveSize(1)
             }
         }
 
         "Provider returns only matching curations for a fixed version" {
             val provider = FilePackageCurationProvider(curationsFile)
 
             val identifier = Identifier("maven", "org.hamcrest", "hamcrest-core", "1.3")
-            val curations = provider.getCurationsFor(listOf(identifier)).values.flatten()
+            val packages = listOf(Package.EMPTY.copy(id = identifier))
+            val curations = provider.getCurationsFor(packages).values.flatten()
 
             curations should haveSize(4)
             curations.forEach {
@@ -95,9 +103,9 @@ class FilePackageCurationProviderTest : StringSpec() {
             val idMaxVersion = Identifier("npm", "", "ramda", "0.25.0")
             val idOutVersion = Identifier("npm", "", "ramda", "0.26.0")
 
-            val curationsMinVersion = provider.getCurationsFor(listOf(idMinVersion)).values.flatten()
-            val curationsMaxVersion = provider.getCurationsFor(listOf(idMaxVersion)).values.flatten()
-            val curationsOutVersion = provider.getCurationsFor(listOf(idOutVersion)).values.flatten()
+            val curationsMinVersion = provider.getCurationsFor(createPackagesFromIds(idMinVersion)).values.flatten()
+            val curationsMaxVersion = provider.getCurationsFor(createPackagesFromIds(idMaxVersion)).values.flatten()
+            val curationsOutVersion = provider.getCurationsFor(createPackagesFromIds(idOutVersion)).values.flatten()
 
             curationsMinVersion should haveSize(1)
             (provider.packageCurations - curationsMinVersion).forEach {
@@ -129,3 +137,6 @@ class FilePackageCurationProviderTest : StringSpec() {
         }
     }
 }
+
+private fun createPackagesFromIds(vararg ids: Identifier) =
+    ids.map { Package.EMPTY.copy(id = it) }

cli/src/main/kotlin/commands/UploadCurationsCommand.kt

@@ -44,6 +44,7 @@ import org.ossreviewtoolkit.clients.clearlydefined.HarvestStatus
 import org.ossreviewtoolkit.clients.clearlydefined.Patch
 import org.ossreviewtoolkit.clients.clearlydefined.callBlocking
 import org.ossreviewtoolkit.clients.clearlydefined.getDefinitionsChunked
+import org.ossreviewtoolkit.model.Package
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.PackageCurationData
 import org.ossreviewtoolkit.model.readValueOrDefault
@@ -84,9 +85,8 @@ class UploadCurationsCommand : OrtCommand(
         }.values
 
         val curationsToCoordinates = curations.mapNotNull { curation ->
-            curation.id.toClearlyDefinedCoordinates()?.let { coordinates ->
-                curation to coordinates
-            }
+            val pkg = Package.EMPTY.copy(id = curation.id)
+            pkg.toClearlyDefinedCoordinates()?.let { curation to it }
         }.toMap()
 
         val definitions = service.getDefinitionsChunked(curationsToCoordinates.values)
@@ -146,7 +146,8 @@ class UploadCurationsCommand : OrtCommand(
 }
 
 private fun PackageCuration.toContributionPatch(): ContributionPatch? {
-    val coordinates = id.toClearlyDefinedCoordinates() ?: return null
+    val pkg = Package.EMPTY.copy(id = id)
+    val coordinates = pkg.toClearlyDefinedCoordinates() ?: return null
 
     val info = ContributionInfo(
         // The exact values to use here are unclear; use what is mostly used at
@@ -165,7 +166,7 @@ private fun PackageCuration.toContributionPatch(): ContributionPatch? {
 
     val described = CurationDescribed(
         projectWebsite = data.homepageUrl?.let { URI(it) },
-        sourceLocation = id.toClearlyDefinedSourceLocation(data.vcs, data.sourceArtifact)
+        sourceLocation = pkg.toClearlyDefinedSourceLocation(data.vcs, data.sourceArtifact)
     )
 
     val curation = Curation(