feat(Repository): Use KnownProvenance instead of VcsInfo #8764

pepper-jk · 2024-06-17T09:29:04Z

In order to allow source artifacts as well as local source code to be scanned, we require a more generallized Repository, which allows any type of KnownProvenance. While we still set the signature of Repository to allow KnownProvenances, this commit focuses on accomidateing RepositoryProvenance as then main input for now.

Therefore we wrap VcsInfo with RepositoryProvenance, whenever it is used as input, and unwrap it, when it is produced as output. This gives us a quick update of the now depreacted code, without the necessity to support all KnownProvenance types from the get go.

We also update any realted tests, mostly the expected OrtResults, but also some Repository definitions and calls.

To avoid having vcs and vcsProcessed appear in the OrtResult output, we change them to be a method (fun) instead of a variable (val). This is still a soft refactor, to keep the widely used vcsProcessed available for now. We might still remove it later.

Signed-off-by: Jens Keim jens.keim@forvia.com

Part of: #8803.

reporter/src/testFixtures/kotlin/TestData.kt

plugins/reporters/freemarker/src/test/kotlin/FreeMarkerTemplateProcessorTest.kt

codecov · 2024-06-20T16:33:33Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.79%. Comparing base (37f4aa5) to head (ba2a2a2).
Report is 813 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8764   +/-   ##
=========================================
  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% <100.00%> (ø)`
test	`38.12% <0.00%> (ø)`

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.

model/src/main/kotlin/Provenance.kt

sschuberth · 2024-06-26T18:09:24Z

model/src/main/kotlin/VcsInfo.kt

@@ -83,6 +83,11 @@ data class VcsInfo(
     * Return a [VcsInfoCurationData] with the properties from this [VcsInfo].
     */
    fun toCuration() = VcsInfoCurationData(type, url, revision, path)
+


Commit message nits:

Duplicate "method" in first sentence.

Please use passive voice, i.e. no "we", "I", "us" etc.

~~I overhauled the commit message after the changes to the commit.~~

EDIT: had to force push the old version, in order to see your reviews on the correct commit.
I will go through everything first before pushing any changes.

Pushed new commits with code changes only.
I will squash them once they are reviewed and accepted.
Afterwards I will update the commit messages and force push.

Just pushed the squashed commits with the overhauled messages.

sschuberth · 2024-06-26T18:09:51Z

model/src/main/kotlin/VcsInfo.kt

@@ -83,6 +83,11 @@ data class VcsInfo(
     * Return a [VcsInfoCurationData] with the properties from this [VcsInfo].
     */
    fun toCuration() = VcsInfoCurationData(type, url, revision, path)
+
+    /**
+     * Return true if this vcs information matches the other vcs information.


The docs for this function require some special attention: The matching disregards the path property (otherwise just vcsInfoA == vcsInfoB could have been used for matching). So in a way this is a bit similar to a PackageCuration's isApplicableDisregardingVersion() function, and I'd name it as equalsDisregardingPath().

You are right, I missed that. I'll change the name and refactored any related changes accordingly.

When I change the name and the doc, should the same doc changes be applied to the matches method in RespositoryProvenance in the next commit:
f43b5d9#diff-db17c034876de657175ab28d452f2812913baba034a764c609990260ce101751

Not necessarily. The current docs already make clear the only the normalized VCS info is compared, so I think we're good.

sschuberth · 2024-06-26T18:18:01Z

model/src/main/kotlin/Provenance.kt

+     * provenance's normalized [VcsInfo][vcsInfo].
+     */
+    override fun matches(other: Provenance): Boolean =
+        other is RepositoryProvenance && other.vcsInfo.normalize().matches(vcsInfo.normalize())


Why is this disregarding the resolvedRevision? And if that's a mistake, why not simply use provenanveA == provenanceB for matching Provenances?

It was not done on purpose.

why not simply use provenanveA == provenanceB for matching Provenances?

Because Repository used the equalsDisregardingPath style matches before, and I wanted to ensure the same behavior as before. IIRC some tests would also fail with just == between VcsInfo, which provenanveA == provenanceB would imply, correct?

which provenanveA == provenanceB would imply, correct?

Yes, == on data classes is a "recursive equals()".

Added a comparison of resolvedRevision.

sschuberth · 2024-06-26T18:19:46Z

model/src/main/kotlin/Provenance.kt

@@ -80,7 +80,9 @@ data class RepositoryProvenance(
    val resolvedRevision: String
 ) : KnownProvenance {
    init {
-        require(resolvedRevision.isNotBlank()) { "The resolved revision must not be blank." }
+        require(vcsInfo == VcsInfo.EMPTY || resolvedRevision.isNotBlank()) {


Commit message nits:

s/vcs/VCS/

s/git/Git/

Odd indentation before "Both"

Again use passive voice

Logic-wise, I'd prefer to have the clause order swapped. Usually we require resolvedRevision.isNotBlank() but if it is blank, the whole VcsInfo must be empty: resolvedRevision.isNotBlank() || vcsInfo == VcsInfo.EMPTY.

s/vcs/VCS/

vcs is part of a test case name string. so changing the capitalization would erode the reference.
Maybe I should change the format to "fail if no vcs matches" to make this more clear and also explicitly call them test cases?

Swapped the logic around.

sschuberth · 2024-06-26T18:32:31Z

analyzer/src/main/kotlin/Analyzer.kt

@@ -42,6 +42,7 @@ import org.ossreviewtoolkit.model.AnalyzerResult
 import org.ossreviewtoolkit.model.AnalyzerRun
 import org.ossreviewtoolkit.model.OrtResult
 import org.ossreviewtoolkit.model.Repository
+import org.ossreviewtoolkit.model.RepositoryProvenance


Commit message:

Again use passive voice

Typo in "generallized" -> "generalized"

"Repository" -> "Repository class" (use backticks so it's clear you refer to the class name)

"accomidateing" -> "accommodating"

"then main" -> "the main"

"depreacted" -> "deprecated"

"realted" -> "related"

sschuberth · 2024-06-26T18:45:45Z

model/src/main/kotlin/Repository.kt


    /**
     * The configuration of the repository, parsed from [ORT_REPO_CONFIG_FILENAME].
     */
    val config: RepositoryConfiguration = RepositoryConfiguration()
 ) {
    companion object {
+        private const val DEFAULT_REVISION = ""


This is more "empty" than "default". And I'd simply inline it.

sschuberth · 2024-06-26T18:47:36Z

model/src/main/kotlin/Repository.kt

            nestedRepositories = emptyMap(),
            config = RepositoryConfiguration()
        )
    }

+    fun vcs(): VcsInfo {


Alternatively, this could be a getter-property annotated with JsonIgnore. Same below.

Done. Let's see if the tests like me.

fviernau · 2024-06-27T06:01:29Z

model/src/main/kotlin/Provenance.kt

@@ -80,7 +80,9 @@ data class RepositoryProvenance(
    val resolvedRevision: String
 ) : KnownProvenance {
    init {


Why haven't you decided for keeping the require as-is, and make the tests construct instances with non-empty resolved revision?

I'm asking to me it seems that if repository provenance is sued, the VCS info should not be empty, because otherwise it would be an Unkown provenance, wouldn't it?

You are probably correct. Changing the tests¹ to use "main" as revision instead of blank, like I did for model/src/test/kotlin/OrtResultTest.kt, might be the better way to solve the issue with the tests, than allowing blank resolvedRevisions in RepositoryProvenance for VcsInfo.Empty.

However, the Analyzer also allows VcsInfo.Empty to be passed from workingTree to the Repository. And with VcsInfo.Empty workingTree will very likely also return an empty revision. So in order to not change that behavior of the Analyzer and Repository in this case, I added this exception to RepositoryProvenance.

I have not yet noticed any issues regarding non-Repository uses of the RepositoryProvenance.

Still you are bringing up some good points, maybe the Analyzer behavior could change in this case? Then we could revert the exception to RepositoryProvenance. We could dial this in once we handle other Provenances in the Analyzer.

¹ evaluator/src/test/kotlin/TestData.kt
model/src/test/kotlin/licenses/TestData.kt
reporter/src/testFixtures/kotlin/TestData.kt

fviernau · 2024-06-27T06:06:22Z

model/src/main/kotlin/Repository.kt


    /**
     * A map of nested repositories, for example Git submodules or Git-Repo modules. The key is the path to the
     * nested repository relative to the root of the main repository.
     */
    @JsonInclude(JsonInclude.Include.NON_EMPTY)
-    val nestedRepositories: Map<String, VcsInfo> = emptyMap(),
+    val nestedRepositories: Map<String, KnownProvenance> = emptyMap(),


Nested repositories can remain VcsInfo or alternatively repository provenance, because by definition this can only be a repository.

So Provenances have no hierarchy? The information required to create nestedRepositories is only available when using Git as a VCS?

Correct, Git and GitRepo are the only implementors of WorkingTree.getNested().

I have now reverted the nestedRepositories changes (ac06be1) and will squash them into the prior commit (665ab20) once all checks pass.

Rebasing on current main was also no issue, so I guess I will be done soon.

Just pushed the rebased and squashed version.

fviernau · 2024-06-27T06:09:29Z

Can we have a GitHub issue which describes exactly the goal of this effort? In particular, mention explicitly in which cases we want to support local file provenance. (e.g. analyzer, scanner, scan storages, ...). I'm still worried about the added complexity of this refactoring, compared to the value it will provide, but having the goal clearly statet would at least ensure we are reviewing against the same outlined goal.

sschuberth · 2024-06-27T09:06:13Z

Can we have a GitHub issue which describes exactly the goal of this effort?

I have started such an issue at #8803 for people to comment on.

As was pointed out during the code review [1], there is now need to change `nestedRepositories` to support `Provenance`s, as only `git` is supported for `nestedRepositories`, which always contains `vcsInfo`. In order to reduce the amount of changes for this PR, any changes regarding `nestedRepositories` are now reverted. [1] oss-review-toolkit#8764 (comment) Signed-off-by: Jens Keim <jens.keim@forvia.com>

…ath` Up until now the method `VcsInfo.matches(VcsInfo)` was only defined and used inside the `Repository` class. With shifting from `VcsInfo` to `RepositoryProvenance`, the matcher needs to be available inside of `RepositoryProvenance`. For that purpose, it gets moved into `VcsInfo` proper, in order to be available anywhere `VcsInfo` is used. It further gets a new name `equalsDisregardingPath`, since it disregards the path, when matching `VcsInfo`s. [1] [1] oss-review-toolkit@9d271b5#r1655333611 Signed-off-by: Jens Keim <jens.keim@forvia.com>

In preparation of replacing `VcsInfo` in `Repository` with `KnownProvenance`, a method to match `Provenance` against each other was added. This inherited method, allows any `Provenace` to be matched against any other. Since the type is verified to be equal before matching any attributes, it can even match against `UnknownProvenance`, no need to limit it to `KnownProvenance`. Signed-off-by: Jens Keim <jens.keim@forvia.com>

This will allow tests using `Repository` objects with `VcsInfo.EMPTY` to continue to function, even though `RepositoryProvenance` does not usually allow blank revisions. By restricting the exception to `VcsInfo.EMPTY` any other behavior of `RepositoryProvenance` should be retained. Some unrelated `OrtResultTest`s had to be given non-blank revisions: Both test cases `"fail if no vcs matches"` and `"use the correct vcs"` don't hinge on the given blank revision, so to assure they do not fail for unrelated reasons, the default Git revision "main" was added to the example repos. Signed-off-by: Jens Keim <jens.keim@forvia.com>

In order to allow source artifacts as well as local source code to be scanned, a more generalized `Repository` class is required, which allows any type of `KnownProvenance`. While the signature of the `Repository` class is set to allow any `KnownProvenance` object, this commit focuses on accommodating `RepositoryProvenance` as the main input for now. Therefore `VcsInfo` is wrapped within `RepositoryProvenance`, whenever it is used as input, and unwraped, when it is produced as output. This results in a faster update of the now deprecated code, without the necessity to support all `KnownProvenance` types from the get go. Any related tests are also updated, mostly the expected `OrtResults`, but also some `Repository` definitions and calls. To avoid having `vcs` and `vcsProcessed` appear in the `OrtResult` output, getter properties were created and marked as `@JsonIgnore`. Signed-off-by: Jens Keim <jens.keim@forvia.com>

pepper-jk · 2024-07-12T08:46:53Z

@sschuberth @fviernau I know, you are still considering if this refactoring is the right way, but if you could find the time to review the current changes that would be great.

Since most changes regard code quality and commit messages, it would be great to know if the changes know meet your standards. Not only for this PR, but also for future work.

github-advanced-security bot found potential problems Jun 17, 2024

View reviewed changes

reporter/src/testFixtures/kotlin/TestData.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the repo-known-provenance branch 8 times, most recently from c5852c3 to dd222f1 Compare June 20, 2024 14:32

github-advanced-security bot found potential problems Jun 20, 2024

View reviewed changes

plugins/reporters/freemarker/src/test/kotlin/FreeMarkerTemplateProcessorTest.kt Fixed Show fixed Hide fixed

plugins/reporters/freemarker/src/test/kotlin/FreeMarkerTemplateProcessorTest.kt Fixed Show fixed Hide fixed

pepper-jk force-pushed the repo-known-provenance branch 4 times, most recently from 3f1306d to fdf3ebb Compare June 20, 2024 16:20

pepper-jk force-pushed the repo-known-provenance branch 13 times, most recently from 26e0a8f to a210a69 Compare June 26, 2024 16:25

github-advanced-security bot found potential problems Jun 26, 2024

View reviewed changes

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

pepper-jk force-pushed the repo-known-provenance branch from a210a69 to 798ac95 Compare June 26, 2024 16:31

pepper-jk force-pushed the repo-known-provenance branch 2 times, most recently from 729a29d to 2f69343 Compare June 26, 2024 16:55

pepper-jk marked this pull request as ready for review June 26, 2024 17:11

pepper-jk requested review from a team as code owners June 26, 2024 17:11

pepper-jk changed the title ~~Draft: feat(Repository): Use KnownProvenance instead of VcsInfo~~ feat(Repository): Use KnownProvenance instead of VcsInfo Jun 26, 2024

sschuberth reviewed Jun 26, 2024

View reviewed changes

fviernau requested changes Jun 27, 2024

View reviewed changes

sschuberth mentioned this pull request Jun 27, 2024

Allow "non-repository" input to the analyzer #8803

Open

pepper-jk force-pushed the repo-known-provenance branch 3 times, most recently from ca44715 to 665ab20 Compare June 28, 2024 13:03

pepper-jk force-pushed the repo-known-provenance branch from f32bdf5 to ac06be1 Compare July 1, 2024 13:18

pepper-jk added 4 commits July 1, 2024 15:38

pepper-jk force-pushed the repo-known-provenance branch from ac06be1 to ba2a2a2 Compare July 1, 2024 13:39

pepper-jk requested review from fviernau and sschuberth July 1, 2024 14:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

pepper-jk commented Jun 17, 2024 •

edited by fviernau

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024 •

edited

Loading

pepper-jk Jun 27, 2024

pepper-jk Jun 28, 2024

sschuberth Jun 26, 2024

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024 •

edited

Loading

pepper-jk Jun 27, 2024

sschuberth Jun 27, 2024

pepper-jk Jun 27, 2024

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024

sschuberth Jun 27, 2024

pepper-jk Jun 27, 2024

sschuberth Jun 26, 2024

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024

pepper-jk Jun 27, 2024

sschuberth Jun 26, 2024

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024

sschuberth Jun 26, 2024

pepper-jk Jun 27, 2024

fviernau Jun 27, 2024

pepper-jk Jun 27, 2024 •

edited

Loading

fviernau Jun 27, 2024

pepper-jk Jun 27, 2024 •

edited

Loading

sschuberth Jun 27, 2024

pepper-jk Jul 1, 2024

pepper-jk Jul 1, 2024

fviernau commented Jun 27, 2024

sschuberth commented Jun 27, 2024

pepper-jk commented Jul 12, 2024

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

Are you sure you want to change the base?

feat(Repository): Use KnownProvenance instead of VcsInfo #8764

Conversation

pepper-jk commented Jun 17, 2024 • edited by fviernau Loading

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

pepper-jk Jun 27, 2024 • 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

pepper-jk Jun 27, 2024 • 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

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

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

pepper-jk Jun 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepper-jk Jun 27, 2024 • 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

fviernau commented Jun 27, 2024

sschuberth commented Jun 27, 2024

pepper-jk commented Jul 12, 2024

pepper-jk commented Jun 17, 2024 •

edited by fviernau

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

pepper-jk Jun 27, 2024 •

edited

Loading

pepper-jk Jun 27, 2024 •

edited

Loading

pepper-jk Jun 27, 2024 •

edited

Loading

pepper-jk Jun 27, 2024 •

edited

Loading