feat(Provenance): Add `RemoteProvenance` sub interface #9476

pepper-jk · 2024-11-20T15:20:12Z

In preparation to split KnownProvenance hierarchy between the upcoming LocalProvances and the existing RemoteProvenances, both RepositoryProvenance and ArtifactProvance now inherit from the RemoteProvenance interface instead of KnownProvenance.

Conditional types, casts, and conditional cases were adjusted as to fit the new interface hierarchy.

Signed-off-by: Jens Keim [email protected]

pepper-jk · 2024-11-26T13:08:52Z

Hi @sschuberth, do you have any feedback for me yet?

model/src/main/kotlin/Provenance.kt

sschuberth · 2024-11-22T08:54:56Z

helper-cli/src/main/kotlin/utils/Extensions.kt

@@ -38,6 +38,7 @@ import org.ossreviewtoolkit.model.OrtResult
 import org.ossreviewtoolkit.model.PackageCuration
 import org.ossreviewtoolkit.model.Project
 import org.ossreviewtoolkit.model.Provenance
+import org.ossreviewtoolkit.model.RemoteProvenance


The intention of the commit is good. I'm just curious how you ensured to capture all occurrences. Can you describe your approach for that in the commit message?

I utilized IntelliJ's Find Usage feature to find all occurrences of KnownProvenance by categories, such as is conditionals, cast type, function parameter, return type, etc.

For this commit (78e4be6) I went through the is conditional findings and investigated their context by checking the surrounding and when necessary the code calling the relevant function.

For 2d70747 I checked the cast types and for 7a6f396 I checked the function parameters for KnownProvenance as well as the is conditionals for RepositoryProvenance and ArtifactProvenance.

Would you like me to add a note to all three commits in regards to the process? (I could always refer to "as described in the previous commit" ofc)

I just added the descriptions to the 3 commits. Let me know if this describes the approach clear enough.

sschuberth · 2024-11-22T08:56:17Z

helper-cli/src/main/kotlin/utils/Extensions.kt

@@ -169,7 +170,7 @@ internal fun OrtResult.getLicenseFindingsById(
            packageConfigurationProvider.getPackageConfigurations(id, provenance).flatMap { it.licenseFindingCurations }
        }

-    getScanResultsForId(id).filter { it.provenance is KnownProvenance }.map {
+    getScanResultsForId(id).filter { it.provenance is RemoteProvenance }.map {


Shouldn't that actually have been RepositoryProvenance only to begin with, @fviernau, because the map calls filterByVcsPath()?

Since it uses forEach and the ? operator, I assume any non-RepositoryProvenances would be skipped by default, which was why KnownProvenance was the conditional here, not the stricter RepositoryProvenance.

EDIT: If my conclusion is correct, using KnownProvenance should be perfectly fine though. Let me know what you think and I will remove the change if you agree.

sschuberth · 2024-11-22T08:59:18Z

model/src/main/kotlin/ScannerRun.kt

@@ -108,7 +108,7 @@ data class ScannerRun(

    init {
        scanResults.forEach { scanResult ->
-            require(scanResult.provenance is KnownProvenance) {
+            require(scanResult.provenance is RemoteProvenance) {


I'm not sure whether this has to be / should be changed. If the requirement fails, the log message explicitly talks about "an unknown provenance". And as we said it would be fine for the scanner to have the restriction that local provenances can only be scanned if the scanner is executed on the same machine as the analyzer, I think this can stay at KnownProvenance.

More clearly: We should be able to scan non-remote provenance, but not store it in any scan storage, I believe. What's your view @mnonnenmacher?

I'm not sure whether this has to be / should be changed. If the requirement fails, the log message explicitly talks about "an unknown provenance". [...] I think this can stay at KnownProvenance.

Sounds reasonable. Removed the change.

Keeping the Thread open for @mnonnenmacher 's opinion.

model/src/main/kotlin/licenses/LicenseInfoResolver.kt

helper-cli/src/main/kotlin/utils/Extensions.kt

model/src/main/kotlin/config/PackageConfiguration.kt

sschuberth · 2024-11-26T14:31:47Z

Hi @sschuberth, do you have any feedback for me yet?

I had started a review but wasn't able to finish it. I did so now.

In preparation to split the `KnownProvenance` hierarchy further, for now introduce a `RemoteProvenance` that bundles the `RepositoryProvenance` and `ArtifactProvance`. Later an additional `LocalProvance` will be introduced. See [1] for context. [1]: oss-review-toolkit#8803 (comment) Signed-off-by: Jens Keim <[email protected]>

codecov · 2024-11-26T16:09:45Z

Codecov Report

Attention: Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 67.93%. Comparing base (e841910) to head (e67dac2).
Report is 131 commits behind head on main.

Files with missing lines	Patch %	Lines
...del/src/main/kotlin/config/PackageConfiguration.kt	0.00%	1 Missing ⚠️
...main/kotlin/provenance/NestedProvenanceResolver.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9476      +/-   ##
============================================
- Coverage     68.09%   67.93%   -0.17%     
+ Complexity     1291     1290       -1     
============================================
  Files           250      249       -1     
  Lines          8814     8813       -1     
  Branches        916      975      +59     
============================================
- Hits           6002     5987      -15     
- Misses         2427     2436       +9     
- Partials        385      390       +5

Flag	Coverage Δ
funTest-docker	`64.82% <ø> (-0.25%)`	⬇️
funTest-non-docker	`33.22% <50.00%> (-0.35%)`	⬇️
test	`35.82% <0.00%> (-0.06%)`	⬇️

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.

In some instances, the verification of `KnownProvenance` is a failsafe before expecting a `RepositoryProvenance` or `ArtifactProvenance`. Since these classes are now more strictly `RemoteProvenance`s, the conditionals are now using the new interface. All cases of `is KnownProvenance` conditionals were investigated for this, especially the s, using the _Find Usage_ Feature from IntelliJ, the recommended IDE for ORT. Both context of surrounding and calling code was investigated. Signed-off-by: Jens Keim <[email protected]>

In some instances, the casting to `KnownProvenance` is not explicitly required by receiving functions, here a more strict casting to `RemoteProvenance` is cleaner. All cases of `KnownProvenance` as a cast type were investigated by the same methods as the previous commit. Signed-off-by: Jens Keim <[email protected]>

Functions accepting a `KnownProvenance` often only handle the two explicit cases of `Artifact` or `RepositoryProvenance`. Add `else` cases to conditional `when`s. All conditional cases of `Artifact` and `RepositoryProvenance` following after `KnownProvenance` as a parameter type were investigated by the same methods as the previous commits. Signed-off-by: Jens Keim <[email protected]>

pepper-jk requested a review from a team as a code owner November 20, 2024 15:20

pepper-jk mentioned this pull request Nov 20, 2024

refactor: More explicit Provenance Handling #9421

Closed

sschuberth requested changes Nov 26, 2024

View reviewed changes

pepper-jk force-pushed the provenance-hierarchy branch from 39d170a to 7a6f396 Compare November 26, 2024 15:34

pepper-jk force-pushed the provenance-hierarchy branch from 7a6f396 to b823ee6 Compare November 27, 2024 12:46

keimje1 added 3 commits November 27, 2024 16:15

pepper-jk force-pushed the provenance-hierarchy branch from b823ee6 to e67dac2 Compare November 27, 2024 15:16

pepper-jk requested a review from sschuberth November 27, 2024 15:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Provenance): Add `RemoteProvenance` sub interface #9476

feat(Provenance): Add `RemoteProvenance` sub interface #9476

pepper-jk commented Nov 20, 2024 •

edited

Loading

pepper-jk commented Nov 26, 2024

sschuberth Nov 22, 2024

pepper-jk Nov 26, 2024 •

edited

Loading

pepper-jk Nov 27, 2024

sschuberth Nov 22, 2024

pepper-jk Nov 26, 2024 •

edited

Loading

sschuberth Nov 22, 2024

sschuberth Nov 26, 2024

pepper-jk Nov 27, 2024

sschuberth commented Nov 26, 2024

codecov bot commented Nov 26, 2024 •

edited

Loading

feat(Provenance): Add RemoteProvenance sub interface #9476

Are you sure you want to change the base?

feat(Provenance): Add RemoteProvenance sub interface #9476

Conversation

pepper-jk commented Nov 20, 2024 • edited Loading

pepper-jk commented Nov 26, 2024

sschuberth Nov 22, 2024

Choose a reason for hiding this comment

pepper-jk Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

pepper-jk Nov 27, 2024

Choose a reason for hiding this comment

sschuberth Nov 22, 2024

Choose a reason for hiding this comment

pepper-jk Nov 26, 2024 • edited Loading

Choose a reason for hiding this comment

sschuberth Nov 22, 2024

Choose a reason for hiding this comment

sschuberth Nov 26, 2024

Choose a reason for hiding this comment

pepper-jk Nov 27, 2024

Choose a reason for hiding this comment

sschuberth commented Nov 26, 2024

codecov bot commented Nov 26, 2024 • edited Loading

Codecov Report

feat(Provenance): Add `RemoteProvenance` sub interface #9476

feat(Provenance): Add `RemoteProvenance` sub interface #9476

pepper-jk commented Nov 20, 2024 •

edited

Loading

pepper-jk Nov 26, 2024 •

edited

Loading

pepper-jk Nov 26, 2024 •

edited

Loading

codecov bot commented Nov 26, 2024 •

edited

Loading