feat(Provenance): Add `DirectoryProvenance` as a `LocalProvenance` #9872

pepper-jk · 2025-01-30T15:55:50Z

In contrast to the previously added RemoteProvenance stands the LocalProvenance, which has no remote source, but instead references a local input of some kind.
The DirectoryProvenance references a local project directory, which is lacking supported (remote) version control. It is defined by its local directory path only.

Since ORT needs further refactoring until DirectoryProvenance can be fully utilized, it the new class can not be used right now. However the presence of a new KnownProvenance class, results in when conditional cases not being exhaustive anymore.

To circumvent this issue, the following changes were made:

Whereever possible RemoteProvenance is used as parameter type instead of KnownProvenance.
When necessary KnownProvenances are cast to RemoteProvenance.
If Provenance is expected, DirectoryProvenance is handled like UnknownProvenance.

The excaption being hash and storageKey, which both default to an empty string. However, since the rest of the code does not handle DirectoryProvenance, this should not become an issue.

For more context on the new Provenance Hierarchy, see: #8803 (comment)

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

model/src/main/kotlin/Provenance.kt

codecov · 2025-02-03T12:49:56Z

Codecov Report

Attention: Patch coverage is 33.33333% with 8 lines in your changes missing coverage. Please review.

Project coverage is 69.60%. Comparing base (f1de6fc) to head (19e6d80).
Report is 5 commits behind head on main.

Files with missing lines	Patch %	Lines
...main/kotlin/storages/ProvenanceBasedFileStorage.kt	33.33%	1 Missing and 1 partial ⚠️
.../kotlin/storages/ProvenanceBasedPostgresStorage.kt	33.33%	1 Missing and 1 partial ⚠️
...del/src/main/kotlin/config/PackageConfiguration.kt	0.00%	1 Missing ⚠️
...src/main/kotlin/utils/FileProvenanceFileStorage.kt	0.00%	1 Missing ⚠️
...main/kotlin/utils/PostgresProvenanceFileStorage.kt	0.00%	1 Missing ⚠️
model/src/main/kotlin/utils/PurlExtensions.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #9872      +/-   ##
============================================
- Coverage     69.66%   69.60%   -0.07%     
  Complexity     1464     1464              
============================================
  Files           270      270              
  Lines          9682     9689       +7     
  Branches       1028     1033       +5     
============================================
- Hits           6745     6744       -1     
- Misses         2487     2492       +5     
- Partials        450      453       +3

Flag	Coverage Δ
funTest-non-docker	`33.25% <25.00%> (-0.06%)`	⬇️
test-ubuntu-24.04	`39.23% <16.66%> (-0.05%)`	⬇️
test-windows-2022	`39.21% <16.66%> (-0.05%)`	⬇️

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.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pepper-jk · 2025-02-05T12:36:06Z

@sschuberth failing tests seem unrelated.
Could you have a look and maybe restart them?
A review would be awesome as well.

sschuberth · 2025-02-11T11:05:35Z

@sschuberth failing tests seem unrelated.

The "Unable to create proxy for sealed class" failures in org.ossreviewtoolkit.scanner.ScannerTest are real. I guess the problem is that nothing currently implements the LocalProvenance interface. So you probably have to add a DirectoryProvenance at the same time.

pepper-jk · 2025-02-11T13:17:18Z

Thanks @sschuberth, I missed that. I'm already working on the DirectoryProvenance over at provenance-directory.
So it would be easy enough to append it here.

However, it is quite a large branch, since a lot of whens require a DirectoryProvenance case.
Most of which are for KnownProvenance input, so I can not default to UnknownProvenance behavior.

I started with reasonable implementation for the DirectoryProvenance behavior, but the scope of required does not seem to end. So I was considering switching to RemoteProvenance inputs to avoid the usage of DirectoryProvenance for now.

Any thoughts or other ideas how to keep the PR small?

sschuberth · 2025-02-12T10:05:57Z

Most of which are for KnownProvenance input, so I can not default to UnknownProvenance behavior.

Can't you "cheat" a bit by first limiting some / all KnownProvenance cases to RemoteProvenance (which semantically is no difference compared to before), and only later bit-by-bit extend respective code paths to also handle LocalProvenance?

pepper-jk · 2025-02-12T11:40:22Z

So I was considering switching to RemoteProvenance inputs to avoid the usage of DirectoryProvenance for now.

Can't you "cheat" a bit by first limiting some / all KnownProvenance cases to RemoteProvenance (which semantically is no difference compared to before), and only later bit-by-bit extend respective code paths to also handle LocalProvenance?

Great to see we are on the same page here. I will do just that.

pepper-jk · 2025-02-13T13:18:42Z

@sschuberth I added the DirectoryProvenance now and "cheated" the when cases.
Let me know what you think, if you have any reservations.

sschuberth · 2025-02-13T14:58:46Z

Commit message:

It is defined by its local directory path only.

We should probably say "absolute" / "canonical" / "real" path, and do the implementation accordingly.

it the new class can not be used right now.

"the new class can not be used right now."

However the presence of a new KnownProvenance class, results

"However, the presence of a new KnownProvenance class results"

Whereever

"Wherever"

excaption

"exception"

Provenance Hierarchy

"provenance hierarchy"

model/src/main/kotlin/Provenance.kt

model/src/main/kotlin/utils/FileProvenanceFileStorage.kt

model/src/main/kotlin/utils/PostgresProvenanceFileStorage.kt

model/src/main/kotlin/utils/PurlExtensions.kt

scanner/src/main/kotlin/provenance/ProvenanceDownloader.kt

model/src/main/kotlin/Provenance.kt

sschuberth · 2025-03-24T09:41:24Z

model/src/main/kotlin/ProvenanceResolutionResult.kt

@@ -37,7 +37,7 @@ data class ProvenanceResolutionResult(
     * The resolved provenance of the package. Can only be null if a [packageProvenanceResolutionIssue] occurred.
     */
    @JsonInclude(JsonInclude.Include.NON_NULL)
-    val packageProvenance: KnownProvenance? = null,
+    val packageProvenance: RemoteProvenance? = null,


More generally than already explained in the commit message, I believe we could say that whenever a provenance can only refer to a package, but not to a project, RemoteProvenance instead of KnownProvenance should be used.

Would you agree? Because actually all of this refactoring has the goal to be able to refer to a DirectoryProvenance from an OrtResult instead of using a Repository, right?

Sounds about right. I can add it to the commit message.

Would have to check if it is these are the only places, where we switch to RemoteProvenance tho before removing the previous explanation.

Added the remark to the first commit message.

sschuberth · 2025-03-24T09:42:53Z

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

@@ -84,6 +85,7 @@ data class PackageConfiguration(
            is UnknownProvenance -> false
            is ArtifactProvenance -> sourceArtifactUrl != null && sourceArtifactUrl == provenance.sourceArtifact.url
            is RepositoryProvenance -> vcs != null && vcs.matches(provenance)
+            is DirectoryProvenance -> false


Could combine this with line 85 to

is DirectoryProvenance, UnknownProvenance -> false

But actually, at least when a PackageConfiguration is used in the context of a RepositoryConfiguration, the Identifier (and Provenance) actually is allowed to refer to a Project. So we should probably implement a proper check for DirectoryProvenance in that case after all.

Yes, I assumed we would need a proper check sooner or later, which is why I added the case as a separate line.

Since this would touch the attributes of the PackageConfiguration, I assumed the best cause of action was to postpone any changes for the sake of a shorter PR and handle DirectoryProvenace like UnknownProvenance for now. I'd recommend to handle this, when we actually start using the DirectoryProvenance.

model/src/main/kotlin/utils/FileProvenanceFileStorage.kt

model/src/main/kotlin/utils/PostgresProvenanceFileStorage.kt

model/src/main/kotlin/utils/PurlExtensions.kt

sschuberth · 2025-03-24T10:13:26Z

scanner/src/main/kotlin/ScanController.kt

@@ -79,7 +80,7 @@ internal class ScanController(
     * A map of package [Identifier]s to their resolved [KnownProvenance]s. These provenances are used to filter the
     * scan results for a package based on the VCS path.
     */
-    private val packageProvenances = mutableMapOf<Identifier, KnownProvenance>()
+    private val packageProvenances = mutableMapOf<Identifier, RemoteProvenance>()


If this is changed, the docs need to be adjusted as well.

This change requires some careful thinking: We actually do want to be able to scan projects with DirectoryProvenance in the end, at least in the case when analyzer and scanner are run on the same machine. So is it really correct to limit us to RemoteProvenance here?

There are probably multiple places, where the limit to RemoteProvenance is not correct in order to handle the DirectoryProvenance, once it is fully implemented.

But right now, while the class is dormant, it is either this or cast the arguments. Otherwise we would have to handle it everywhere from the get go as well.

side note: as this attribute is named packageProvenances, we probably need to address the shift from packages to projects as (primary) input for the scanner at some point, since you repeatedly stated before that a DirectoryProvenace can not be a Package due to lack of a remote.

sschuberth · 2025-03-24T10:17:54Z

scanner/src/main/kotlin/storages/ProvenanceBasedPostgresStorage.kt

@@ -86,6 +87,10 @@ class ProvenanceBasedPostgresStorage(
            return database.transaction {
                val query = table.selectAll()

+                if (provenance !is RemoteProvenance) {
+                    throw ScanStorageException("Scan result must have a known provenance, but it is $provenance.")


The message refers to "known provenance", but the check is against RemoteProvenance, so this does not match.

This also somewhat relates to my comment above, that we actually do want to be able to scan a DirectoryProvenance.

Yes, the code and the exception's text are technically not reflecting the same thing.

However, while the DirectoryProvenance is unused, the RemoteProvenance is practically the same as KnownProvenance. Similar to my comment above, this is a temporary change, which aims to avoid issues due unfinished class handling.

sschuberth · 2025-03-24T10:18:03Z

scanner/src/main/kotlin/storages/ProvenanceBasedPostgresStorage.kt

@@ -137,7 +142,7 @@ class ProvenanceBasedPostgresStorage(

        requireEmptyVcsPath(provenance)

-        if (provenance !is KnownProvenance) {
+        if (provenance !is RemoteProvenance) {


model/src/main/kotlin/utils/PostgresProvenanceFileStorage.kt

In contrast to the previously added `RemoteProvenance` stands the `LocalProvenance`, which has no remote source, but instead references a local input of some kind. The `DirectoryProvenance` references a local project directory, which is lacking supported (remote) version control. It is defined by its canonical path only. Since ORT needs further refactoring until `DirectoryProvenance` can be fully utilized, the new class can not be used right now. However the presence of a new `KnownProvenance` class results in `when` conditional cases not being exhaustive anymore. To circumvent this issue, the following changes were made: 1. Wherever possible `RemoteProvenance` is used as parameter type instead of `KnownProvenance`. 2. When necessary `KnownProvenance`s are cast to `RemoteProvenance`. 3. If `Provenance` is expected, `DirectoryProvenance` is handled like `UnknownProvenance`. For instances of `Package` the new default data structure should be `RemoteProvenance`, as a `Package` by definition requires a remote. The exception being `hash` and `storageKey`, which both required a default value, which was set to the `canonicalPath`. However, since the rest of the code does not handle `DirectoryProvenance`, this should remain unused for now. See [1] for more context on the new provenance hierarchy. [1]: oss-review-toolkit#8803 (comment) Signed-off-by: Jens Keim <[email protected]>

Casts were removed from `FileListResolver`, `ProvenanceBasedFileStorage`, `ProvenanceDownloader`, and `Scanner`. In order to avoid casting `KnownProvenance` to `RemoteProvenance`, a lot of parameters need to be changed to `RemoteProvenance`. With the exception of `ProvenanceBasedFileStorage`, which now uses `UnknownProvenance` exceptions just like `ProvenanceBasedPostgresStorage`. Signed-off-by: Jens Keim <[email protected]>

pepper-jk requested a review from a team as a code owner January 30, 2025 15:55

pepper-jk marked this pull request as draft January 30, 2025 16:03

github-advanced-security bot found potential problems Jan 30, 2025

View reviewed changes

model/src/main/kotlin/Provenance.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the provenance-local branch from a984e1f to 2471efa Compare February 3, 2025 12:26

github-advanced-security bot found potential problems Feb 3, 2025

View reviewed changes

model/src/main/kotlin/Provenance.kt Fixed Show fixed Hide fixed

model/src/main/kotlin/Provenance.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the provenance-local branch from 2471efa to 1e3c8cf Compare February 3, 2025 12:33

pepper-jk force-pushed the provenance-local branch from 1e3c8cf to c2f8552 Compare February 3, 2025 13:00

pepper-jk changed the title ~~feat(Provenance): Add DirectoryProvenance as a LocalProvenance~~ feat(Provenance): Add a LocalProvenance sub-interface Feb 3, 2025

pepper-jk force-pushed the provenance-local branch from c2f8552 to c6cbdbc Compare February 3, 2025 13:03

pepper-jk marked this pull request as ready for review February 3, 2025 13:21

pepper-jk force-pushed the provenance-local branch from c6cbdbc to c0a51c2 Compare February 5, 2025 12:52

pepper-jk marked this pull request as draft February 12, 2025 15:46

pepper-jk force-pushed the provenance-local branch 3 times, most recently from 9322fb4 to 9ffd2c5 Compare February 13, 2025 12:50

pepper-jk changed the title ~~feat(Provenance): Add a LocalProvenance sub-interface~~ feat(Provenance): Add DirectoryProvenance as a LocalProvenance Feb 13, 2025

pepper-jk marked this pull request as ready for review February 13, 2025 12:59

sschuberth reviewed Feb 13, 2025

View reviewed changes

pepper-jk force-pushed the provenance-local branch from 9ffd2c5 to 9fc284d Compare February 13, 2025 17:39

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

model/src/main/kotlin/Provenance.kt Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Feb 13, 2025

View reviewed changes

model/src/main/kotlin/Provenance.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the provenance-local branch from 9fc284d to 4d39003 Compare February 13, 2025 18:01

pepper-jk force-pushed the provenance-local branch from cfdad33 to 4b97905 Compare February 20, 2025 15:04

pepper-jk requested a review from sschuberth February 20, 2025 15:08

pepper-jk force-pushed the provenance-local branch from 4b97905 to cdeeddb Compare February 24, 2025 13:54

sschuberth requested a review from a team March 13, 2025 09:15

sschuberth reviewed Mar 24, 2025

View reviewed changes

pepper-jk force-pushed the provenance-local branch from cdeeddb to 84cf646 Compare March 25, 2025 15:07

github-advanced-security bot found potential problems Mar 25, 2025

View reviewed changes

model/src/main/kotlin/utils/PostgresProvenanceFileStorage.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the provenance-local branch from 84cf646 to 150b157 Compare March 25, 2025 15:19

pepper-jk added 2 commits March 26, 2025 13:40

pepper-jk force-pushed the provenance-local branch from 150b157 to 19e6d80 Compare March 26, 2025 12:40

pepper-jk requested a review from sschuberth March 27, 2025 16:03

sschuberth requested a review from a team March 28, 2025 07:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Provenance): Add `DirectoryProvenance` as a `LocalProvenance` #9872

feat(Provenance): Add `DirectoryProvenance` as a `LocalProvenance` #9872

pepper-jk commented Jan 30, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

pepper-jk commented Feb 5, 2025

sschuberth commented Feb 11, 2025

pepper-jk commented Feb 11, 2025 •

edited

Loading

sschuberth commented Feb 12, 2025

pepper-jk commented Feb 12, 2025

pepper-jk commented Feb 13, 2025

sschuberth commented Feb 13, 2025 •

edited

Loading

sschuberth Mar 24, 2025

pepper-jk Mar 25, 2025

pepper-jk Mar 25, 2025

sschuberth Mar 24, 2025

sschuberth Mar 24, 2025

pepper-jk Mar 25, 2025

sschuberth Mar 24, 2025

sschuberth Mar 24, 2025 •

edited

Loading

pepper-jk Mar 25, 2025 •

edited

Loading

sschuberth Mar 24, 2025

pepper-jk Mar 25, 2025

sschuberth Mar 24, 2025

pepper-jk Mar 25, 2025

feat(Provenance): Add DirectoryProvenance as a LocalProvenance #9872

Are you sure you want to change the base?

feat(Provenance): Add DirectoryProvenance as a LocalProvenance #9872

Conversation

pepper-jk commented Jan 30, 2025 • edited Loading

codecov bot commented Feb 3, 2025 • edited Loading

Codecov Report

pepper-jk commented Feb 5, 2025

sschuberth commented Feb 11, 2025

pepper-jk commented Feb 11, 2025 • edited Loading

sschuberth commented Feb 12, 2025

pepper-jk commented Feb 12, 2025

pepper-jk commented Feb 13, 2025

sschuberth commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sschuberth Mar 24, 2025 • edited Loading

Choose a reason for hiding this comment

pepper-jk Mar 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feat(Provenance): Add `DirectoryProvenance` as a `LocalProvenance` #9872

feat(Provenance): Add `DirectoryProvenance` as a `LocalProvenance` #9872

pepper-jk commented Jan 30, 2025 •

edited

Loading

codecov bot commented Feb 3, 2025 •

edited

Loading

pepper-jk commented Feb 11, 2025 •

edited

Loading

sschuberth commented Feb 13, 2025 •

edited

Loading

sschuberth Mar 24, 2025 •

edited

Loading

pepper-jk Mar 25, 2025 •

edited

Loading