From f9c7220e7bb9938e92ba6479d42c9249edeb5e3f Mon Sep 17 00:00:00 2001 From: Frank Viernau Date: Mon, 30 Dec 2024 14:08:48 +0100 Subject: [PATCH] refactor(model)!: Simplify the `CuratedPackage` and its creation Historically, serialized ORT results contained entries for all `CuratedPackage`s. Back then, the list of `PackageCurationResult`, in particular the value of the `base` property, had been useful to trace back from which original value any curated package had been derived. Meanwhile, ORT results have been refactored to apply the package curations on-the-fly. This means that the serialized ORT result only contains the original, non-curated packages and the package curations separately, but no more the curated packages. So, in order to debug the derivation one can simply use break points in the function which applies the curation. That is why the `base` property is not needed anymore. So, remove the `base` property, which allows to also remove the `diff()` function as it was only needed to compute the `base`. This simplifies things, because the `diff` is not always straight forward to compute and make sense of. Note: It might make sense to add the `original` package as a separate property to `CuratedPackage`. Signed-off-by: Frank Viernau Signed-off-by: Sebastian Schuberth --- model/src/main/kotlin/CuratedPackage.kt | 4 +- model/src/main/kotlin/Package.kt | 23 ---- model/src/main/kotlin/PackageCurationData.kt | 16 +-- .../src/main/kotlin/PackageCurationResult.kt | 35 ------ .../licenses/DefaultLicenseInfoProvider.kt | 4 +- model/src/main/kotlin/licenses/LicenseInfo.kt | 10 +- model/src/test/kotlin/PackageCurationTest.kt | 33 ++--- model/src/test/kotlin/PackageTest.kt | 119 ------------------ ...orter-test-deduplicate-expected-output.yml | 12 +- ...d-model-reporter-test-expected-output.json | 14 +-- ...ed-model-reporter-test-expected-output.yml | 12 +- .../src/main/kotlin/EvaluatedPackage.kt | 4 +- 12 files changed, 35 insertions(+), 251 deletions(-) delete mode 100644 model/src/main/kotlin/PackageCurationResult.kt delete mode 100644 model/src/test/kotlin/PackageTest.kt diff --git a/model/src/main/kotlin/CuratedPackage.kt b/model/src/main/kotlin/CuratedPackage.kt index 9b92ef91f5206..68f773503e135 100644 --- a/model/src/main/kotlin/CuratedPackage.kt +++ b/model/src/main/kotlin/CuratedPackage.kt @@ -20,7 +20,7 @@ package org.ossreviewtoolkit.model /** - * A [Package] including the [PackageCurationResult]s that were applied to it, in order to be able to trace back how the + * A [Package] including the [PackageCurationData] that was applied to it, in order to be able to trace back how the * original metadata of the package was modified by applying [PackageCuration]s. */ data class CuratedPackage( @@ -32,5 +32,5 @@ data class CuratedPackage( /** * The curations in the order they were applied. */ - val curations: List = emptyList() + val curations: List = emptyList() ) diff --git a/model/src/main/kotlin/Package.kt b/model/src/main/kotlin/Package.kt index 7f57b0254cd38..246abd96f2ec5 100644 --- a/model/src/main/kotlin/Package.kt +++ b/model/src/main/kotlin/Package.kt @@ -164,29 +164,6 @@ data class Package( sourceCodeOrigins?.requireNotEmptyNoDuplicates() } - /** - * Compares this package with [other] and creates a [PackageCurationData] containing the values from this package - * which are different in [other]. All equal values are set to null. Only the fields present in - * [PackageCurationData] are compared. - */ - fun diff(other: Package): PackageCurationData { - require(id == other.id) { - "Cannot diff packages with different ids: '${id.toCoordinates()}' vs. '${other.id.toCoordinates()}'" - } - - return PackageCurationData( - authors = authors.takeIf { it != other.authors }, - description = description.takeIf { it != other.description }, - homepageUrl = homepageUrl.takeIf { it != other.homepageUrl }, - binaryArtifact = binaryArtifact.takeIf { it != other.binaryArtifact }, - sourceArtifact = sourceArtifact.takeIf { it != other.sourceArtifact }, - vcs = vcsProcessed.takeIf { it != other.vcsProcessed }?.toCuration(), - isMetadataOnly = isMetadataOnly.takeIf { it != other.isMetadataOnly }, - isModified = isModified.takeIf { it != other.isModified }, - sourceCodeOrigins = sourceCodeOrigins.takeIf { it != other.sourceCodeOrigins } - ) - } - /** * Create a [CuratedPackage] from this package with an empty list of applied curations. */ diff --git a/model/src/main/kotlin/PackageCurationData.kt b/model/src/main/kotlin/PackageCurationData.kt index f53447da137d9..f0d60c4d3c6f4 100644 --- a/model/src/main/kotlin/PackageCurationData.kt +++ b/model/src/main/kotlin/PackageCurationData.kt @@ -159,19 +159,7 @@ data class PackageCurationData( sourceCodeOrigins = sourceCodeOrigins ?: original.sourceCodeOrigins ) - val declaredLicenseMappingDiff = buildMap { - val previous = targetPackage.getDeclaredLicenseMapping().toList() - val current = declaredLicenseMapping.toList() - - putAll(previous - current) - } - - val curations = targetPackage.curations + PackageCurationResult( - base = original.diff(pkg).copy(declaredLicenseMapping = declaredLicenseMappingDiff), - curation = this - ) - - return CuratedPackage(pkg, curations) + return CuratedPackage(pkg, targetPackage.curations + this) } /** @@ -203,5 +191,5 @@ data class PackageCurationData( private fun CuratedPackage.getDeclaredLicenseMapping(): Map = buildMap { - curations.forEach { putAll(it.curation.declaredLicenseMapping) } + curations.forEach { putAll(it.declaredLicenseMapping) } } diff --git a/model/src/main/kotlin/PackageCurationResult.kt b/model/src/main/kotlin/PackageCurationResult.kt deleted file mode 100644 index e6b7afc24531b..0000000000000 --- a/model/src/main/kotlin/PackageCurationResult.kt +++ /dev/null @@ -1,35 +0,0 @@ -/* - * Copyright (C) 2017 The ORT Project Authors (see ) - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - * SPDX-License-Identifier: Apache-2.0 - * License-Filename: LICENSE - */ - -package org.ossreviewtoolkit.model - -/** - * This class contains information about which values were changed when applying a curation. - */ -data class PackageCurationResult( - /** - * Contains the values from before applying the [curation]. Values which were not changed are null. - */ - val base: PackageCurationData, - - /** - * The curation that was applied. - */ - val curation: PackageCurationData -) diff --git a/model/src/main/kotlin/licenses/DefaultLicenseInfoProvider.kt b/model/src/main/kotlin/licenses/DefaultLicenseInfoProvider.kt index 71a72b9c36aa5..fb33997fa1453 100644 --- a/model/src/main/kotlin/licenses/DefaultLicenseInfoProvider.kt +++ b/model/src/main/kotlin/licenses/DefaultLicenseInfoProvider.kt @@ -50,7 +50,7 @@ class DefaultLicenseInfoProvider(val ortResult: OrtResult) : LicenseInfoProvider ortResult.getPackage(id)?.let { (pkg, curations) -> ConcludedLicenseInfo( concludedLicense = pkg.concludedLicense, - appliedCurations = curations.filter { it.curation.concludedLicense != null } + appliedCurations = curations.filter { it.concludedLicense != null } ) } ?: ConcludedLicenseInfo(concludedLicense = null, appliedCurations = emptyList()) @@ -67,7 +67,7 @@ class DefaultLicenseInfoProvider(val ortResult: OrtResult) : LicenseInfoProvider authors = pkg.authors, licenses = pkg.declaredLicenses, processed = pkg.declaredLicensesProcessed, - appliedCurations = curations.filter { it.curation.declaredLicenseMapping.isNotEmpty() } + appliedCurations = curations.filter { it.declaredLicenseMapping.isNotEmpty() } ) } ?: DeclaredLicenseInfo( authors = emptySet(), diff --git a/model/src/main/kotlin/licenses/LicenseInfo.kt b/model/src/main/kotlin/licenses/LicenseInfo.kt index 5f5b95bc70c5f..8b9510003743a 100644 --- a/model/src/main/kotlin/licenses/LicenseInfo.kt +++ b/model/src/main/kotlin/licenses/LicenseInfo.kt @@ -22,7 +22,7 @@ package org.ossreviewtoolkit.model.licenses import org.ossreviewtoolkit.model.CopyrightFinding import org.ossreviewtoolkit.model.Identifier import org.ossreviewtoolkit.model.LicenseFinding -import org.ossreviewtoolkit.model.PackageCurationResult +import org.ossreviewtoolkit.model.PackageCurationData import org.ossreviewtoolkit.model.Provenance import org.ossreviewtoolkit.model.Repository import org.ossreviewtoolkit.model.config.LicenseFindingCuration @@ -66,9 +66,9 @@ data class ConcludedLicenseInfo( val concludedLicense: SpdxExpression?, /** - * The list of [PackageCurationResult]s that modified the concluded license. + * The list of [PackageCurationData] that modified the concluded license. */ - val appliedCurations: List + val appliedCurations: List ) /** @@ -91,9 +91,9 @@ data class DeclaredLicenseInfo( val processed: ProcessedDeclaredLicense, /** - * The list of [PackageCurationResult]s that modified the declared license. + * The list of [PackageCurationData] that modified the declared license. */ - val appliedCurations: List + val appliedCurations: List ) /** diff --git a/model/src/test/kotlin/PackageCurationTest.kt b/model/src/test/kotlin/PackageCurationTest.kt index 010bc0ba8c6d0..8c5e558e6be94 100644 --- a/model/src/test/kotlin/PackageCurationTest.kt +++ b/model/src/test/kotlin/PackageCurationTest.kt @@ -23,9 +23,8 @@ import io.kotest.assertions.assertSoftly import io.kotest.assertions.throwables.shouldThrow import io.kotest.core.spec.style.WordSpec import io.kotest.matchers.collections.containExactly +import io.kotest.matchers.collections.shouldContainExactly import io.kotest.matchers.collections.shouldHaveSize -import io.kotest.matchers.maps.beEmpty -import io.kotest.matchers.maps.shouldContainExactly import io.kotest.matchers.should import io.kotest.matchers.shouldBe @@ -89,8 +88,7 @@ class PackageCurationTest : WordSpec({ } curatedPkg.curations shouldHaveSize 1 - curatedPkg.curations.first().base shouldBe pkg.diff(curatedPkg.metadata) - curatedPkg.curations.first().curation shouldBe curation.data + curatedPkg.curations.first() shouldBe curation.data } "change only curated fields" { @@ -142,8 +140,7 @@ class PackageCurationTest : WordSpec({ } curatedPkg.curations shouldHaveSize 1 - curatedPkg.curations.first().base shouldBe pkg.diff(curatedPkg.metadata) - curatedPkg.curations.first().curation shouldBe curation.data + curatedPkg.curations.first() shouldBe curation.data } "be able to empty VCS information" { @@ -283,24 +280,18 @@ class PackageCurationTest : WordSpec({ result1.metadata.description shouldBe "description 1" result1.curations shouldHaveSize 1 - result1.curations[0].base shouldBe PackageCurationData(description = "") - result1.curations[0].curation shouldBe curation1.data + result1.curations[0] shouldBe curation1.data result2.metadata.description shouldBe "description 2" result2.curations shouldHaveSize 2 - result2.curations[0].base shouldBe PackageCurationData(description = "") - result2.curations[0].curation shouldBe curation1.data - result2.curations[1].base shouldBe PackageCurationData(description = "description 1") - result2.curations[1].curation shouldBe curation2.data + result2.curations[0] shouldBe curation1.data + result2.curations[1] shouldBe curation2.data result3.metadata.description shouldBe "description 3" result3.curations shouldHaveSize 3 - result3.curations[0].base shouldBe PackageCurationData(description = "") - result3.curations[0].curation shouldBe curation1.data - result3.curations[1].base shouldBe PackageCurationData(description = "description 1") - result3.curations[1].curation shouldBe curation2.data - result3.curations[2].base shouldBe PackageCurationData(description = "description 2") - result3.curations[2].curation shouldBe curation3.data + result3.curations[0] shouldBe curation1.data + result3.curations[1] shouldBe curation2.data + result3.curations[2] shouldBe curation3.data } } @@ -326,11 +317,7 @@ class PackageCurationTest : WordSpec({ result3.metadata.declaredLicensesProcessed.spdxExpression shouldBe "Apache-2.0 AND BSD-3-Clause AND CC-BY-2.0".toSpdx() - result3.curations[0].base.declaredLicenseMapping should beEmpty() - result3.curations[1].base.declaredLicenseMapping should beEmpty() - result3.curations[2].base.declaredLicenseMapping.shouldContainExactly( - mapOf("license c" to "CC-BY-1.0".toSpdx()) - ) + result3.curations shouldContainExactly listOf(curation1, curation2, curation3).map { it.data } } } diff --git a/model/src/test/kotlin/PackageTest.kt b/model/src/test/kotlin/PackageTest.kt deleted file mode 100644 index c28660e8a7c46..0000000000000 --- a/model/src/test/kotlin/PackageTest.kt +++ /dev/null @@ -1,119 +0,0 @@ -/* - * Copyright (C) 2017 The ORT Project Authors (see ) - * - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * https://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * - * SPDX-License-Identifier: Apache-2.0 - * License-Filename: LICENSE - */ - -package org.ossreviewtoolkit.model - -import io.kotest.assertions.throwables.shouldThrow -import io.kotest.core.spec.style.StringSpec -import io.kotest.matchers.nulls.beNull -import io.kotest.matchers.should -import io.kotest.matchers.shouldBe - -class PackageTest : StringSpec({ - "diff throws an exception if the identifiers are not equal" { - val pkg = Package.EMPTY.copy(id = Identifier("type1", "namespace1", "name1", "version1")) - val other = Package.EMPTY.copy(id = Identifier("type2", "namespace2", "name2", "version2")) - - shouldThrow { - pkg.diff(other) - } - } - - "diff result contains all changed values" { - val pkg = Package( - id = Identifier( - type = "type", - namespace = "namespace", - name = "name", - version = "version" - ), - authors = setOf("author"), - declaredLicenses = setOf("declared license"), - description = "description", - homepageUrl = "homepageUrl", - binaryArtifact = RemoteArtifact("url", Hash.create("hash")), - sourceArtifact = RemoteArtifact("url", Hash.create("hash")), - vcs = VcsInfo(VcsType.forName("type"), "url", "revision"), - isMetadataOnly = false, - isModified = false, - sourceCodeOrigins = null - ) - - val other = Package( - id = Identifier( - type = "type", - namespace = "namespace", - name = "name", - version = "version" - ), - authors = setOf("other author"), - declaredLicenses = setOf("other declared license"), - description = "other description", - homepageUrl = "other homepageUrl", - binaryArtifact = RemoteArtifact("other url", Hash.create("other hash")), - sourceArtifact = RemoteArtifact("other url", Hash.create("other hash")), - vcs = VcsInfo(VcsType.forName("other type"), "other url", "other revision"), - isMetadataOnly = true, - isModified = true, - sourceCodeOrigins = listOf(SourceCodeOrigin.VCS) - ) - - val diff = pkg.diff(other) - - diff.binaryArtifact shouldBe pkg.binaryArtifact - diff.comment should beNull() - diff.authors shouldBe pkg.authors - diff.homepageUrl shouldBe pkg.homepageUrl - diff.sourceArtifact shouldBe pkg.sourceArtifact - diff.vcs shouldBe pkg.vcsProcessed.toCuration() - diff.isMetadataOnly shouldBe pkg.isMetadataOnly - diff.isModified shouldBe pkg.isModified - diff.sourceCodeOrigins shouldBe pkg.sourceCodeOrigins - } - - "diff result does not contain unchanged values" { - val pkg = Package( - id = Identifier( - type = "type", - namespace = "namespace", - name = "name", - version = "version" - ), - authors = setOf("author"), - declaredLicenses = setOf("declared license"), - description = "description", - homepageUrl = "homepageUrl", - binaryArtifact = RemoteArtifact("url", Hash.create("hash")), - sourceArtifact = RemoteArtifact("url", Hash.create("hash")), - vcs = VcsInfo(VcsType.forName("type"), "url", "revision"), - sourceCodeOrigins = listOf(SourceCodeOrigin.VCS) - ) - - val diff = pkg.diff(pkg) - - diff.binaryArtifact should beNull() - diff.comment should beNull() - diff.authors should beNull() - diff.homepageUrl should beNull() - diff.sourceArtifact should beNull() - diff.vcs should beNull() - diff.isMetadataOnly should beNull() - diff.sourceCodeOrigins should beNull() - } -}) diff --git a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-deduplicate-expected-output.yml b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-deduplicate-expected-output.yml index 9a054a896a801..9b0d251779a65 100644 --- a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-deduplicate-expected-output.yml +++ b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-deduplicate-expected-output.yml @@ -692,10 +692,8 @@ packages: revision: "" path: "" curations: - - base: {} - curation: - comment: "Foobar is an imaginary dependency and offers a license choice" - concluded_license: "GPL-2.0-only OR MIT" + - comment: "Foobar is an imaginary dependency and offers a license choice" + concluded_license: "GPL-2.0-only OR MIT" paths: - 3 levels: @@ -744,10 +742,8 @@ packages: revision: "" path: "" curations: - - base: {} - curation: - comment: "H2 database offers a license choice" - concluded_license: "MPL-2.0 OR EPL-1.0" + - comment: "H2 database offers a license choice" + concluded_license: "MPL-2.0 OR EPL-1.0" paths: - 4 levels: diff --git a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.json b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.json index d2536107f878b..12e09717f7cde 100644 --- a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.json +++ b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.json @@ -759,11 +759,8 @@ "path" : "" }, "curations" : [ { - "base" : { }, - "curation" : { - "comment" : "Foobar is an imaginary dependency and offers a license choice", - "concluded_license" : "GPL-2.0-only OR MIT" - } + "comment" : "Foobar is an imaginary dependency and offers a license choice", + "concluded_license" : "GPL-2.0-only OR MIT" } ], "paths" : [ 3 ], "levels" : [ 1 ], @@ -813,11 +810,8 @@ "path" : "" }, "curations" : [ { - "base" : { }, - "curation" : { - "comment" : "H2 database offers a license choice", - "concluded_license" : "MPL-2.0 OR EPL-1.0" - } + "comment" : "H2 database offers a license choice", + "concluded_license" : "MPL-2.0 OR EPL-1.0" } ], "paths" : [ 4 ], "levels" : [ 1 ], diff --git a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.yml b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.yml index 9a054a896a801..9b0d251779a65 100644 --- a/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.yml +++ b/plugins/reporters/evaluated-model/src/funTest/assets/evaluated-model-reporter-test-expected-output.yml @@ -692,10 +692,8 @@ packages: revision: "" path: "" curations: - - base: {} - curation: - comment: "Foobar is an imaginary dependency and offers a license choice" - concluded_license: "GPL-2.0-only OR MIT" + - comment: "Foobar is an imaginary dependency and offers a license choice" + concluded_license: "GPL-2.0-only OR MIT" paths: - 3 levels: @@ -744,10 +742,8 @@ packages: revision: "" path: "" curations: - - base: {} - curation: - comment: "H2 database offers a license choice" - concluded_license: "MPL-2.0 OR EPL-1.0" + - comment: "H2 database offers a license choice" + concluded_license: "MPL-2.0 OR EPL-1.0" paths: - 4 levels: diff --git a/plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedPackage.kt b/plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedPackage.kt index 6536dcd33360b..35a11e7c4dc6f 100644 --- a/plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedPackage.kt +++ b/plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedPackage.kt @@ -26,7 +26,7 @@ import java.util.SortedSet import org.ossreviewtoolkit.model.Identifier import org.ossreviewtoolkit.model.Package -import org.ossreviewtoolkit.model.PackageCurationResult +import org.ossreviewtoolkit.model.PackageCurationData import org.ossreviewtoolkit.model.RemoteArtifact import org.ossreviewtoolkit.model.VcsInfo import org.ossreviewtoolkit.model.config.PathExclude @@ -64,7 +64,7 @@ data class EvaluatedPackage( val vcs: VcsInfo, val vcsProcessed: VcsInfo = vcs.normalize(), @JsonInclude(JsonInclude.Include.NON_EMPTY) - val curations: List, + val curations: List, @JsonIdentityReference(alwaysAsId = true) val paths: MutableList, @JsonInclude(JsonInclude.Include.NON_EMPTY)