evaluator: Make PackageRules take a CuratedPackage

[sschuberth]

Please have a look at the individual commit messages for the details.

[mnonnenmacher]

I am not going to comment on all points of the already very long discussion, I just want to add some thoughts about the three commits in this PR:

1. Changing PackageRule to get a CuratedPackage: That makes sense to me, if we have a class that contains both the package metadata and the curations, why not use it and instead pass both separately? This makes even more sense if more properties are going to be added to CuratedPackage, like there are plans to add concluded copyrights as well. But because it's a breaking change I have added this label to the PR.
2. Move concludedLicense to CuratedPackage: That makes sense to me as well, especially in the context of the previously mentioned new properties which might be added to CuratedPackage. A better separation of package metadata and user provided values like legal conclusions is a better design in my opinion.
3. Rename pkg to metadata: Also makes sense to me, since I suggested that renaming ;)

[mnonnenmacher]

However, now that I think about it again, maybe the curations could still be dropped from the rules, as the rules would have access to the concluded license via the ResolvedLicenseInfo. Correct @mnonnenmacher?

That is correct, I'm undecided about the question if package rules need access to the list of applied curations. But if more properties are added to CuratedPackage there will also be more uses cases where a PackageRule might want to have access to them.

[fviernau]

I am not going to comment on all points of the already very long discussion, I just want to add some thoughts about the three commits in this PR:

1. Changing `PackageRule` to get a `CuratedPackage`: That makes sense to me, if we have a class that contains both the package metadata and the curations, why not use it and instead pass both separately? This makes even more sense if more properties are going to be added to `CuratedPackage`, like there are plans to add concluded copyrights as well. But because it's a breaking change I have added this label to the PR.

2. Move `concludedLicense` to `CuratedPackage`: That makes sense to me as well, especially in the context of the previously mentioned new properties which might be added to `CuratedPackage`. A better separation of package metadata and user provided values like legal conclusions is a better design in my opinion.

3. Rename `pkg` to `metadata`: Also makes sense to me, since I suggested that renaming ;)

IIUC the whole above argumentation is based on the underlying assumption(s) that it's good to move concludedLicense to CuratedPackage and that it's good to place concludedCopyrights in CuratedPackage not in Package. This underlying assumption was exactly what I was challenging in above lengthy discussion, and if that underlying assumption would not hold the argumentation would fall apart completely.

So, can we discuss pros and cons of (1) having all properties in Package vs. (2) having some properties (which exactly btw.?) in Package and some in CuratedPackage?

[sschuberth]

So, can we discuss pros and cons of (1) having all properties in Package vs. (2) having some properties (which exactly?) in Package and some in CuratedPackage?

To be honest, I'm a little bit exhausted by this surprisingly lengthy discussion about something that I (and probably also @mnonnenmacher) regard as straight-forward changes.

But sure @fviernau, please go ahead and provide your pros for keeping / putting properties in Package that don't actually come from the package manager, esp. given the fact that we already have the CuratedPackage class. Also, please provide cons for having a clean separation between package metadata as provided by the manager, and data that can only come from curations.

[mnonnenmacher]

IIUC the whole above argumentation is based on the underlying assumption(s) that it's good to move concludedLicense to CuratedPackage and that it's good to place concludedCopyrights in CuratedPackage not in Package. This underlying assumption was exactly what I was challenging in above lengthy discussion, and if that underlying assumption would not hold the argumentation would fall apart completely.

There is no underlying assumption, in (2) I explain that I think it is a good idea to separate package metadata from user provided values like the concluded license, and (1) is mostly independent of the refactoring of the concluded license.

So, can we discuss pros and cons of (1) having all properties in Package vs. (2) having some properties (which exactly btw.?) in Package and some in CuratedPackage?

@sschuberth Maybe you could provide a draft of what the final classes should look like in your opinion if legal conclusions are separated from metadata and also concluded copyrights are added? I think this might help to understand the motivation behind the change.

Just some independent ideas about the general refactoring that we could discuss later:
If this change is implemented we could consider renaming CuratedPackage to Package and Package to PackageMetadata. Then it would probably also be good to move the id to the main Package class for accessibility, and we could also consider to reuse PackageMetadata inside Project to reduce duplication.

[sschuberth]

@sschuberth Maybe you could provide a draft of what the final classes should look like in your opinion if legal conclusions are separated from metadata and also concluded copyrights are added? I think this might help to understand the motivation behind the change.

Not sure if it help, but in the context of #4519 / #5680 I envision the CuratedPackage class to get an additional concludedCopyrights property so that it looks like

data class CuratedPackage(
    /**
     * The curated package after applying the [curations].
     */
    @JsonAlias("package")
    val metadata: Package,

    /**
     * The concluded license as an [SpdxExpression]. It can be used to override the [declared][declaredLicenses] /
     * [detected][LicenseFinding.license] licenses of a package.
     */
    @JsonInclude(JsonInclude.Include.NON_NULL)
    val concludedLicense: SpdxExpression? = null,

    /**
     * The set of concluded copyright statements for this package. It can be used to override the
     * [detected copyright statements][CopyrightFinding.statement].
     */
    @JsonInclude(JsonInclude.Include.NON_DEFAULT)
    val concludedCopyrights: SortedSet<String> = sortedSetOf(),

    /**
     * The curations in the order they were applied.
     */
    val curations: List<PackageCurationResult> = emptyList()
)

If this change is implemented we could consider renaming CuratedPackage to Package and Package to PackageMetadata. Then it would probably also be good to move the id to the main Package class for accessibility, and we could also consider to reuse PackageMetadata inside Project to reduce duplication.

I believe these are some good ideas. Esp. extracting common stuff of Package and Project.

[fviernau]

I gave this some further thought and decided to leave this discussion, as I feel on this particular topic were mostly talking past one another. So, I'll go with whatever the community decides.

[codecov]

Codecov Report

Base: 57.97% // Head: 57.97% // No change to project coverage 👍

Coverage data is based on head (559c536) compared to base (ba3d7ab).
Patch coverage: 55.55% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5719   +/-   ##
=========================================
  Coverage     57.97%   57.97%           
  Complexity     2228     2228           
=========================================
  Files           326      326           
  Lines         19088    19088           
  Branches       3735     3749   +14     
=========================================
  Hits          11067    11067           
  Misses         6883     6883           
  Partials       1138     1138           

Flag	Coverage Δ
funTest-analyzer-docker	74.57% <83.33%> (ø)
funTest-non-analyzer	46.21% <53.57%> (ø)
test	28.01% <2.94%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cli/src/main/kotlin/commands/DownloaderCommand.kt	36.02% <0.00%> (ø)
...c/main/kotlin/commands/ImportScanResultsCommand.kt	0.00% <0.00%> (ø)
...n/reporters/evaluatedmodel/StatisticsCalculator.kt	80.00% <0.00%> (ø)
...rc/main/kotlin/experimental/ExperimentalScanner.kt	52.73% <0.00%> (ø)
...orter/src/main/kotlin/reporters/OpossumReporter.kt	87.35% <33.33%> (ø)
analyzer/src/main/kotlin/managers/Pip.kt	49.23% <50.00%> (ø)
...zer/src/main/kotlin/managers/utils/MavenSupport.kt	55.22% <50.00%> (ø)
...eporters/freemarker/FreemarkerTemplateProcessor.kt	70.91% <50.00%> (ø)
...n/kotlin/reporters/spdx/SpdxDocumentModelMapper.kt	76.31% <66.66%> (ø)
.../main/kotlin/managers/utils/NuGetAllPackageData.kt	85.07% <100.00%> (ø)
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[sschuberth]

Please have a look again (esp. @mnonnenmacher) as I've dropped the commit that moves concludedLicense from Package to CuratedPackage for now.

Scope of changes has changed.

[mnonnenmacher]

Please have a look again (esp. @mnonnenmacher) as I've dropped the commit that moves concludedLicense from Package to CuratedPackage for now.

Looks good to me, but I'll wait with the approval until the next workday because of the required adaptations in our rules.

[sschuberth]

Please have a look again (esp. @mnonnenmacher) as I've dropped the commit that moves concludedLicense from Package to CuratedPackage for now.

Looks good to me, but I'll wait with the approval until the next workday because of the required adaptations in our rules.

@bennati here's another heads-up for a PR with breaking changes you probably need to adapt your rules to. Your changes probably would need to look similar to these.

[bennati]

Thanks, please go ahead

[sschuberth]

Thanks, please go ahead

@mnonnenmacher, @fviernau if you're also ok with moving forward with this, please approve. I'd like to merge this before I need to catch up with new changes.

[mnonnenmacher]

@sschuberth I have prepared the changes on our side, but please wait with the merge until tomorrow because I don't have time to do a synchronized merge today.

[sschuberth]

please wait with the merge until tomorrow because I don't have time to do a synchronized merge today.

Are we good to go @mnonnenmacher?

[mnonnenmacher]

please wait with the merge until tomorrow because I don't have time to do a synchronized merge today.

Are we good to go @mnonnenmacher?

Yes, go ahead.