Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change the curation provider API to take packages instead of ids #6387

Merged
merged 1 commit into from
Feb 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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
sschuberth marked this conversation as resolved.
Show resolved Hide resolved
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")
fviernau marked this conversation as resolved.
Show resolved Hide resolved

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()
}
Expand All @@ -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()
}
Expand All @@ -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)) }
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
3 changes: 1 addition & 2 deletions analyzer/src/main/kotlin/Analyzer.kt
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,11 @@ private fun resolveConfiguration(
analyzerResult: AnalyzerResult,
curationProviders: List<Pair<String, PackageCurationProvider>>
): ResolvedConfiguration {
val packageIds = analyzerResult.packages.mapTo(mutableSetOf()) { it.id }
val packageCurations = mutableListOf<PackageCuration>()

curationProviders.forEach { (id, curationProvider) ->
val (curations, duration) = measureTimedValue {
curationProvider.getCurationsFor(packageIds).values.flatten().distinct()
curationProvider.getCurationsFor(analyzerResult.packages).values.flatten().distinct()
}

packageCurations += curations
Expand Down
9 changes: 5 additions & 4 deletions analyzer/src/main/kotlin/PackageCurationProvider.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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>>
mnonnenmacher marked this conversation as resolved.
Show resolved Hide resolved
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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> {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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

/**
Expand Down Expand Up @@ -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()
}
}
})
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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)
}
}

Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -129,3 +137,6 @@ class FilePackageCurationProviderTest : StringSpec() {
}
}
}

private fun createPackagesFromIds(vararg ids: Identifier) =
ids.map { Package.EMPTY.copy(id = it) }
11 changes: 6 additions & 5 deletions cli/src/main/kotlin/commands/UploadCurationsCommand.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand All @@ -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(
Expand Down
Loading