feat(model)!: Stop silently ignoring invalid declared license mappings

[fviernau]

Previously, PackageCuration.apply() silently ignored declared license mapping entries with invalid SPDX expressions. For example, if one accidentally omits the LicenseRef- prefix, the mapping is just silently ignored.

Add a check that all values in the Map are valid SPDX expression into the constructor, to fail as early as possible. When used via a FilePackageCurationProvider, ORT now fails with the error message pointing to the problematic curation file path.

Fixes: #7828.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.79%. Comparing base (951bbc4) to head (8ce0d9a).

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8691   +/-   ##
=========================================
  Coverage     67.79%   67.79%           
  Complexity     1164     1164           
=========================================
  Files           243      243           
  Lines          7711     7711           
  Branches        861      861           
=========================================
  Hits           5228     5228           
  Misses         2127     2127           
  Partials        356      356           

Flag	Coverage Δ
funTest-non-docker	33.96% <ø> (ø)
test	38.12% <ø> (ø)

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[fviernau]

@oss-review-toolkit/core-devs gentle ask for review

[sschuberth]

I wonder whether this is the best place to implement this kind of check. My guess is that the "swallowing" of the mapping happens due to isValid() failing at

ort/utils/ort/src/main/kotlin/DeclaredLicenseProcessor.kt

Line 97 in 0efd110

return mappedLicense?.normalize()?.takeIf { it.isValid() || it.toString() == SpdxConstants.NONE }

I'd rather see a check being implemented in that function, as it then would also apply to built-in declared license mappings, to guards against typos etc. in declared-license-mapping.yml.

As I agree that having a mapping configured that maps to some value that we'd discard later on anyway dos not make any sense, I was playing around with a proposal that now ended up in another / additional PR, see #8730.

[fviernau]

I'd rather see a check being implemented in that function, as it then would also apply to built-in declared license mappings, to guards against typos etc. in declared-license-mapping.yml.

I deliberately put it there for the reason mentioned in the commit message [1], as I believe knowing the file path of the incorrect entry must be in the error.

[1] ... ORT now fails with the error message pointing to the problematic curation file path.

[sschuberth]

I deliberately put it there for the reason mentioned in the commit message [1], as I believe knowing the file path of the incorrect entry must be in the error.

While I agree that having the file path in the error message would be beneficial, I'm still unsure if this is the right place to add this kind of check. I'd rather include this as another PR check in the package curation repository then. But I'm happy to also hear @mnonnenmacher's opinion here.

Another reason why I'm a bit reluctant to add more of these init-based checks to serialized data classes is related to #8052 where we do something similar for SpdxFile: By now, I believe that serialized data classes should be just that, a "dumb" representation of the data model, without any built-in validation. The consumers of those classes should then, depending on their use-case of the data model, decide what kind of validation, if any, should be implemented.

[sschuberth]

IMO "within" sounds a bit strange here. How about:

"The declared license '$key' is configured to map to '$value' which is not a valid SPDX expression."

(So I would also include the key into the exception message for clarity.)

[sschuberth]

The text looks unchanged. Did you forget to push?