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 <[email protected]> Signed-off-by: Sebastian Schuberth <[email protected]>
oss-review-toolkit · Jan 2, 2025 · f9c7220 · f9c7220
1 parent e7b8a3a
commit f9c7220
Show file tree

Hide file tree

Showing 12 changed files with 35 additions and 251 deletions.
diff --git 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<PackageCurationResult> = emptyList()
+    val curations: List<PackageCurationData> = emptyList()
 )
diff --git 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
@@ -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<String, SpdxExpression> =
     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
diff --git 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
@@ -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<PackageCurationResult>
+    val appliedCurations: List<PackageCurationData>
 )
 
 /**
@@ -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<PackageCurationResult>
+    val appliedCurations: List<PackageCurationData>
 )
 
 /**

diff --git 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
diff --git a/...ed-model/src/funTest/assets/evaluated-model-reporter-test-deduplicate-expected-output.yml b/...ed-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: