Remove `ReporterInput.resolutionProvider` #8026

fviernau · 2023-12-12T14:52:43Z

As of the addition of OrtResult.resolvedConfiguration, the ORT result is self-contained WRT to package configurations,
resolutions and curations. So, in theory the OrtResult can be passed to each respective Reporter without
any accompanying provider, e.g. ResolutionProvider, CurationProvider as you name it. This would be less complicated to handle: e.g. make the ReporterCommand first include all the data in OrtResult and then pass around the OrtResult to the reporters.

If you look at the functions OrtResult.getOpenIssues() or OrtResult.getResolutions(), these consider only resolutions from the repository configuration, but not the ones from the resolved configuration. So, the result of these functions can normally only be used by further processing it with some matching against data from a ResolutionProvider. This inconvenience would be removed if just all data was included in OrtResult and actually also used by its member functions.

This PR implements this only for the resolution provider.
An analog PR should be done for package configurations, which would allow to implement filtering issues by affectedPath / path excludes within OrtResult.getIssues().

Part of #7921.

codecov · 2023-12-12T15:09:54Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (86be29e) 67.08% compared to head (ab50952) 67.08%.
Report is 6 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #8026   +/-   ##
=========================================
  Coverage     67.08%   67.08%           
  Complexity     2054     2054           
=========================================
  Files           357      357           
  Lines         17121    17121           
  Branches       2457     2457           
=========================================
  Hits          11486    11486           
  Misses         4613     4613           
  Partials       1022     1022

Flag	Coverage Δ
funTest-non-docker	`34.10% <ø> (ø)`
test	`36.76% <ø> (ø)`

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.

helper-cli/src/main/kotlin/commands/repoconfig/GenerateRuleViolationResolutionsCommand.kt

sschuberth · 2023-12-13T16:03:08Z

helper-cli/src/main/kotlin/commands/repoconfig/RemoveEntriesCommand.kt

@@ -103,7 +103,7 @@ internal class RemoveEntriesCommand : CliktCommand(
        }

        val ruleViolationResolutions = ortResult.getRuleViolations().let { ruleViolations ->
-            ortResult.getResolutions().ruleViolations.filter { resolution ->
+            ortResult.getRepositoryConfigResolutions().ruleViolations.filter { resolution ->


Commit message: Typo in "cofigurations".

but not the ones from the resolved configuration

I might be mistaken, but aren't resolutions from the repository configuration also baked into the resolved configuration?

If so, the quoted sentence should probably say "but not the other ones ...".

And if not, is it a mistake that resolutions from the repository configuration are not part of the resolved configuration?

I might be mistaken, but aren't resolutions from the repository configuration also baked into the resolved configuration?

Yes, that's true.

Commit message: Typo in "cofigurations".

Typo is still there.

model/src/main/kotlin/OrtResult.kt

plugins/commands/reporter/src/main/kotlin/ReporterCommand.kt

plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedModelMapper.kt

model/src/main/kotlin/OrtResult.kt

sschuberth · 2023-12-18T12:20:14Z

model/src/main/kotlin/OrtResult.kt

+     */
+    @JsonIgnore
+    fun getResolutions(): Resolutions =
+        getRepositoryConfigResolutions().merge(resolvedConfiguration.resolutions.orEmpty())


What now confuses me here is: If as discussed here resolutions from the repository config are already part of the resolved configuration, why do they need to be merged here?

I decided for this, as not all stages have the resolutions in the resolved configuration, e.g. like the analyzer.

Does merge deduplicate resolutions?

IIRC only resolutions that actually match anything are added to the resolved configuration. Merging resolutions from the repository config would add those that don't match anything.

I decided for this, as not all stages have the resolutions in the resolved configuration, e.g. like the analyzer.

IMO if resolutions have not been resolved, it's fine to ignore those from the repository configuration. Because using them but ignoring those from the resolutions file is also inconsistent with what later steps that properly resolve resolutions will see.

IMO if resolutions have not been resolved, it's fine to ignore those from the repository configuration.

I've changed it accordingly.

mnonnenmacher · 2023-12-18T14:29:12Z

helper-cli/src/main/kotlin/commands/repoconfig/RemoveEntriesCommand.kt

@@ -103,7 +103,7 @@ internal class RemoveEntriesCommand : CliktCommand(
        }

        val ruleViolationResolutions = ortResult.getRuleViolations().let { ruleViolations ->
-            ortResult.getResolutions().ruleViolations.filter { resolution ->
+            ortResult.getRepositoryConfigResolutions().ruleViolations.filter { resolution ->


Commit message: Typo in "cofigurations".

Typo is still there.

mnonnenmacher · 2023-12-18T14:52:21Z

model/src/main/kotlin/OrtResult.kt

+     */
+    @JsonIgnore
+    fun getResolutions(): Resolutions =
+        getRepositoryConfigResolutions().merge(resolvedConfiguration.resolutions.orEmpty())


Does merge deduplicate resolutions?

IIRC only resolutions that actually match anything are added to the resolved configuration. Merging resolutions from the repository config would add those that don't match anything.

I decided for this, as not all stages have the resolutions in the resolved configuration, e.g. like the analyzer.

IMO if resolutions have not been resolved, it's fine to ignore those from the repository configuration. Because using them but ignoring those from the resolutions file is also inconsistent with what later steps that properly resolve resolutions will see.

mnonnenmacher · 2023-12-18T14:53:49Z

model/src/main/kotlin/OrtResult.kt

@@ -272,7 +272,7 @@ data class OrtResult(
            .mapNotNull { (id, issues) -> issues.takeUnless { isExcluded(id) } }
            .flatten()
            .filter { issue ->
-                issue.severity >= minSeverity && getRepositoryConfigResolutions().issues.none { it.matches(issue) }
+                issue.severity >= minSeverity && getResolutions().issues.none { it.matches(issue) }


Function docs need to be adapted.

mnonnenmacher · 2023-12-18T14:56:51Z

plugins/reporters/evaluated-model/src/main/kotlin/EvaluatedModelMapper.kt

Commit message: Typo in StatisticsCalculator

sschuberth · 2023-12-19T10:17:00Z

reporter/src/main/kotlin/StatisticsCalculator.kt

-        ortResult.getVulnerabilities(omitExcluded = true).values.flatten()
-            .filterNot { resolutionProvider.isResolved(it) }.size
+    private fun getOpenVulnerabilities(ortResult: OrtResult): Int =
+        ortResult.getVulnerabilities(omitExcluded = true).values.flatten().size


This needs to also pass omitResolved = true now in exchange for dropping .filterNot { resolutionProvider.isResolved(it) }.

sschuberth · 2023-12-19T10:21:27Z

reporter/src/main/kotlin/StatisticsCalculator.kt

-    ): IssueStatistics {
-        val openIssues = ortResult.getOpenIssues(Severity.HINT).filterNot { resolutionProvider.isResolved(it) }
+    private fun getOpenIssues(ortResult: OrtResult, ortConfig: OrtConfiguration): IssueStatistics {
+        val openIssues = ortResult.getOpenIssues(Severity.HINT)


Note (to myself as well): The signature of getOpenIssues() is a bit inconsistent with getRuleViolations() and getVulnerabilities() in that it offers no omitResolved parameter, but always filters for unresolved issues.

As such, the .filterNot { resolutionProvider.isResolved(it) } call could have been dropped without a change of functionality independently of this commit / PR. Maybe this should be clarified in the commit message. Or do we even take the chance and align the API as part of this PR?

As such, the .filterNot { resolutionProvider.isResolved(it) } call could have been dropped without a change of functionality independently of this commit / PR.

Can you help understanding why it would not change functionality?

getOpenIssues() (as opposed to getIssues()) is implemented like this:

ort/model/src/main/kotlin/OrtResult.kt

Lines 270 to 276 in 07f9efb

fun getOpenIssues(minSeverity: Severity = Severity.WARNING) =

getIssues()

.mapNotNull { (id, issues) -> issues.takeUnless { isExcluded(id) } }

.flatten()

.filter { issue ->

issue.severity >= minSeverity && getResolutions().issues.none { it.matches(issue) }

}

so the getResolutions().issues.none { it.matches(issue) } part already only keeps issues for which no resolution matches. Which means the .filterNot { resolutionProvider.isResolved(it) } never was necessary here, if I'm not mistaken.

As such, the .filterNot { resolutionProvider.isResolved(it) } call could have been dropped without a change of functionality independently of this commit / PR. Maybe this should be clarified in the commit message. Or do we even take the chance and align the API as part of this PR?

However, this comment assumes that OrtResult contains all resolutions. This wasn't true until my recent changed which changed the reporter command to ensure that. And that change was a preparation for this commit, so to me it makes sense to remove it in this commit. Does this make sense to you now as well ?

plugins/commands/reporter/src/main/kotlin/ReporterCommand.kt

sschuberth · 2023-12-19T10:30:30Z

plugins/reporters/freemarker/src/main/kotlin/FreemarkerTemplateProcessor.kt


        /**
         * Return a list of [RuleViolation]s for which no [RuleViolationResolution] is provided.
         */
        @Suppress("UNUSED") // This function is used in the templates.
        fun filterForUnresolvedRuleViolations(ruleViolation: List<RuleViolation>): List<RuleViolation> =
-            ruleViolation.filterNot { input.resolutionProvider.isResolved(it) }
+            ruleViolation.filterNot { input.ortResult.isResolved(it) }


I wonder whether we should instead simply drop these functions and tell reporter authors to call getRuleViolations(omitResolved = true). Similar below.

This would be a breaking change, as we basically don't know who is making use of these.
I strongly prefer to, if at all, do this out of scope of this PR in a separate dedicated PR later on.

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

The reporter command has been changed to ensure that all resolutions are included in the `OrtResult` before passing it around to the reporters. Reflect that also in this test asset. This was left-over by 10c0362. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

Align all `reporter-test-input.yml` files to again have the same contents. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

Prepare for the removal of `ReporterInput.resolutionProvider`. The change is non-functional because the reporter command ensures that all relevant resolutions are contained in the `OrtResult`. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

The reporter command ensures that all resolutions are contained in the ORT result. So, the resolution provider is not necessary anymore. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

As `OrtResult` meanwhile has all the functions of `ResolutionProvider`, make it implement that interface to document that. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

Comments were addressed.

fviernau requested a review from a team as a code owner December 12, 2023 14:52

fviernau force-pushed the reporter-remove-resolution-provider branch 6 times, most recently from 3e3979a to 967a269 Compare December 13, 2023 09:08

sschuberth reviewed Dec 13, 2023

View reviewed changes

fviernau force-pushed the reporter-remove-resolution-provider branch from 967a269 to ba8739b Compare December 13, 2023 20:12

fviernau requested a review from sschuberth December 13, 2023 20:12

fviernau force-pushed the reporter-remove-resolution-provider branch from ba8739b to e3b751b Compare December 14, 2023 09:37

fviernau mentioned this pull request Dec 14, 2023

Remove ReporterInput.packageConfigurationProvider #8039

Merged

fviernau force-pushed the reporter-remove-resolution-provider branch from e3b751b to 85db0af Compare December 18, 2023 11:52

sschuberth reviewed Dec 18, 2023

View reviewed changes

mnonnenmacher previously requested changes Dec 18, 2023

View reviewed changes

fviernau force-pushed the reporter-remove-resolution-provider branch 5 times, most recently from 0cdf172 to 2ee3109 Compare December 19, 2023 07:59

fviernau requested review from mnonnenmacher and sschuberth December 19, 2023 08:00

sschuberth requested changes Dec 19, 2023

View reviewed changes

fviernau force-pushed the reporter-remove-resolution-provider branch 2 times, most recently from 2023082 to d3d7a18 Compare December 19, 2023 11:39

fviernau requested a review from sschuberth December 19, 2023 11:39

fviernau mentioned this pull request Dec 19, 2023

Preparations for removing ReporterInput.resolutionProvider #8048

Merged

fviernau force-pushed the reporter-remove-resolution-provider branch from d3d7a18 to 5784097 Compare December 20, 2023 11:34

fviernau enabled auto-merge (rebase) December 20, 2023 11:56

fviernau force-pushed the reporter-remove-resolution-provider branch 3 times, most recently from 1299d6c to ee38e1e Compare December 27, 2023 08:46

fviernau force-pushed the reporter-remove-resolution-provider branch from ee38e1e to 5bb980e Compare January 2, 2024 17:49

sschuberth force-pushed the reporter-remove-resolution-provider branch from 5bb980e to 8d4859a Compare January 8, 2024 17:38

sschuberth and others added 5 commits January 8, 2024 18:39

chore(static-html-reporter): Align YAML assets to use unindented lists

c0d2a84

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

test(reporter): Add issue resolutions to all test assets

a58a81a

Align all `reporter-test-input.yml` files to again have the same contents. Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

sschuberth force-pushed the reporter-remove-resolution-provider branch 2 times, most recently from 7390bf5 to 9877fb9 Compare January 8, 2024 20:33

sschuberth previously approved these changes Jan 8, 2024

View reviewed changes

sschuberth disabled auto-merge January 8, 2024 20:35

fviernau and others added 5 commits January 8, 2024 21:42

refactor(reporter)!: Remove ReporterInput.resolutionProvider

9f4c66c

The reporter command ensures that all resolutions are contained in the ORT result. So, the resolution provider is not necessary anymore. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

refactor(model): Make OrtResult implement ResolutionProvider

6fcd6f3

As `OrtResult` meanwhile has all the functions of `ResolutionProvider`, make it implement that interface to document that. Signed-off-by: Frank Viernau <frank_viernau@epam.com>

docs(evaluated-model-reporter): Use imperative mood in function docs

ab50952

Signed-off-by: Sebastian Schuberth <sebastian@doubleopen.org>

sschuberth dismissed their stale review via ab50952 January 8, 2024 20:42

sschuberth force-pushed the reporter-remove-resolution-provider branch from 9877fb9 to ab50952 Compare January 8, 2024 20:42

sschuberth approved these changes Jan 8, 2024

View reviewed changes

sschuberth enabled auto-merge (rebase) January 8, 2024 20:43

sschuberth merged commit e1c0651 into main Jan 9, 2024
22 checks passed

sschuberth deleted the reporter-remove-resolution-provider branch January 9, 2024 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `ReporterInput.resolutionProvider` #8026

Remove `ReporterInput.resolutionProvider` #8026

fviernau commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading

sschuberth Dec 13, 2023

fviernau Dec 13, 2023

mnonnenmacher Dec 18, 2023

sschuberth Dec 18, 2023

fviernau Dec 18, 2023

mnonnenmacher Dec 18, 2023

fviernau Dec 18, 2023

mnonnenmacher Dec 18, 2023

mnonnenmacher Dec 18, 2023

mnonnenmacher Dec 18, 2023

mnonnenmacher Dec 18, 2023

sschuberth Dec 19, 2023

sschuberth Dec 19, 2023

fviernau Dec 19, 2023

sschuberth Dec 20, 2023

fviernau Dec 21, 2023

fviernau Dec 21, 2023

sschuberth Dec 19, 2023

fviernau Dec 19, 2023

	fun getOpenIssues(minSeverity: Severity = Severity.WARNING) =
	getIssues()
	.mapNotNull { (id, issues) -> issues.takeUnless { isExcluded(id) } }
	.flatten()
	.filter { issue ->
	issue.severity >= minSeverity && getResolutions().issues.none { it.matches(issue) }
	}

Remove ReporterInput.resolutionProvider #8026

Remove ReporterInput.resolutionProvider #8026

Conversation

fviernau commented Dec 12, 2023 • edited Loading

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Remove `ReporterInput.resolutionProvider` #8026

Remove `ReporterInput.resolutionProvider` #8026

fviernau commented Dec 12, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading