Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove ReporterInput.resolutionProvider #8026

Merged
merged 10 commits into from
Jan 9, 2024
36 changes: 35 additions & 1 deletion model/src/main/kotlin/OrtResult.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,10 +28,14 @@ import org.apache.logging.log4j.kotlin.loggerOf

import org.ossreviewtoolkit.model.ResolvedPackageCurations.Companion.REPOSITORY_CONFIGURATION_PROVIDER_ID
import org.ossreviewtoolkit.model.config.Excludes
import org.ossreviewtoolkit.model.config.IssueResolution
import org.ossreviewtoolkit.model.config.LicenseFindingCuration
import org.ossreviewtoolkit.model.config.RepositoryConfiguration
import org.ossreviewtoolkit.model.config.Resolutions
import org.ossreviewtoolkit.model.config.RuleViolationResolution
import org.ossreviewtoolkit.model.config.VulnerabilityResolution
import org.ossreviewtoolkit.model.config.orEmpty
import org.ossreviewtoolkit.model.utils.ResolutionProvider
import org.ossreviewtoolkit.model.vulnerabilities.Vulnerability
import org.ossreviewtoolkit.utils.common.zipWithCollections
import org.ossreviewtoolkit.utils.spdx.model.SpdxLicenseChoice
Expand Down Expand Up @@ -83,7 +87,7 @@ data class OrtResult(
*/
@JsonInclude(JsonInclude.Include.NON_EMPTY)
val labels: Map<String, String> = emptyMap()
) {
) : ResolutionProvider {
companion object {
/**
* A constant for an [OrtResult] with an empty repository and all other properties `null`.
Expand Down Expand Up @@ -387,6 +391,36 @@ data class OrtResult(
@JsonIgnore
fun getResolutions(): Resolutions = resolvedConfiguration.resolutions.orEmpty()

/**
* Return true if and only if [violation] is resolved in this [OrtResult].
*/
override fun isResolved(violation: RuleViolation): Boolean =
getResolutions().ruleViolations.any { it.matches(violation) }

/**
* Return true if and only if [vulnerability] is resolved in this [OrtResult].
*/
override fun isResolved(vulnerability: Vulnerability): Boolean =
getResolutions().vulnerabilities.any { it.matches(vulnerability) }

/**
* Return the resolutions matching [issue].
*/
override fun getResolutionsFor(issue: Issue): List<IssueResolution> =
getResolutions().issues.filter { it.matches(issue) }

/**
* Return the resolutions matching [violation].
*/
override fun getResolutionsFor(violation: RuleViolation): List<RuleViolationResolution> =
getResolutions().ruleViolations.filter { it.matches(violation) }

/**
* Return the resolutions matching [vulnerability].
*/
override fun getResolutionsFor(vulnerability: Vulnerability): List<VulnerabilityResolution> =
getResolutions().vulnerabilities.filter { it.matches(vulnerability) }

sschuberth marked this conversation as resolved.
Show resolved Hide resolved
/**
* Return all [RuleViolation]s contained in this [OrtResult]. Optionally exclude resolved violations with
* [omitResolved] and remove violations below the [minSeverity].
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -257,7 +257,6 @@ class ReporterCommand : OrtCommand(
ortResult,
ortConfig,
packageConfigurationProvider,
DefaultResolutionProvider(ortResult.getResolutions()),
DefaultLicenseTextProvider(licenseTextDirectories),
copyrightGarbage,
licenseInfoResolver,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import io.kotest.matchers.shouldBe
import org.ossreviewtoolkit.model.FileFormat
import org.ossreviewtoolkit.model.OrtResult
import org.ossreviewtoolkit.model.config.PluginConfiguration
import org.ossreviewtoolkit.model.utils.DefaultResolutionProvider
import org.ossreviewtoolkit.reporter.ReporterInput
import org.ossreviewtoolkit.utils.common.normalizeLineBreaks
import org.ossreviewtoolkit.utils.test.getAssetAsString
Expand Down Expand Up @@ -66,7 +65,6 @@ class EvaluatedModelReporterFunTest : WordSpec({
private fun TestConfiguration.generateReport(ortResult: OrtResult, options: Map<String, String> = emptyMap()): String {
val input = ReporterInput(
ortResult = ortResult,
resolutionProvider = DefaultResolutionProvider.create(ortResult),
howToFixTextProvider = { "Some how to fix text." }
)

Expand Down
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit message: Typo in StatisticsCalculator

Original file line number Diff line number Diff line change
Expand Up @@ -29,13 +29,17 @@ import org.ossreviewtoolkit.model.PackageLinkage
import org.ossreviewtoolkit.model.Project
import org.ossreviewtoolkit.model.Provenance
import org.ossreviewtoolkit.model.RemoteArtifact
import org.ossreviewtoolkit.model.Repository
import org.ossreviewtoolkit.model.ResolvedConfiguration
import org.ossreviewtoolkit.model.RuleViolation
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.TextLocation
import org.ossreviewtoolkit.model.VcsInfo
import org.ossreviewtoolkit.model.config.IssueResolution
import org.ossreviewtoolkit.model.config.LicenseFindingCuration
import org.ossreviewtoolkit.model.config.PathExclude
import org.ossreviewtoolkit.model.config.RepositoryConfiguration
import org.ossreviewtoolkit.model.config.Resolutions
import org.ossreviewtoolkit.model.config.RuleViolationResolution
import org.ossreviewtoolkit.model.config.ScopeExclude
import org.ossreviewtoolkit.model.config.VulnerabilityResolution
Expand Down Expand Up @@ -142,8 +146,8 @@ internal class EvaluatedModelMapper(private val input: ReporterInput) {
ruleViolations = ruleViolations,
vulnerabilitiesResolutions = vulnerabilitiesResolutions,
vulnerabilities = vulnerabilities,
statistics = with(input) { getStatistics(ortResult, resolutionProvider, licenseInfoResolver, ortConfig) },
repository = input.ortResult.repository,
statistics = with(input) { getStatistics(ortResult, licenseInfoResolver, ortConfig) },
sschuberth marked this conversation as resolved.
Show resolved Hide resolved
repository = input.ortResult.repository.deduplicateResolutions(),
severeIssueThreshold = input.ortConfig.severeIssueThreshold,
severeRuleViolationThreshold = input.ortConfig.severeRuleViolationThreshold,
repositoryConfiguration = input.ortResult.repository.config.toYaml(),
Expand Down Expand Up @@ -604,19 +608,19 @@ internal class EvaluatedModelMapper(private val input: ReporterInput) {
}

private fun addResolutions(issue: Issue): List<IssueResolution> {
val matchingResolutions = input.resolutionProvider.getResolutionsFor(issue)
val matchingResolutions = input.ortResult.getResolutionsFor(issue)

return issueResolutions.addIfRequired(matchingResolutions)
}

private fun addResolutions(ruleViolation: RuleViolation): List<RuleViolationResolution> {
val matchingResolutions = input.resolutionProvider.getResolutionsFor(ruleViolation)
val matchingResolutions = input.ortResult.getResolutionsFor(ruleViolation)

return ruleViolationResolutions.addIfRequired(matchingResolutions)
}

private fun addResolutions(vulnerability: Vulnerability): List<VulnerabilityResolution> {
val matchingResolutions = input.resolutionProvider.getResolutionsFor(vulnerability)
val matchingResolutions = input.ortResult.getResolutionsFor(vulnerability)

return vulnerabilitiesResolutions.addIfRequired(matchingResolutions)
}
Expand Down Expand Up @@ -707,9 +711,9 @@ internal class EvaluatedModelMapper(private val input: ReporterInput) {
)

/**
* Adds the [value] to this list if the list does not already contain an equal item. Returns the item that is
* contained in the list. This is important to make sure that there is only one instance of equal items used in the
* model, because when Jackson generates IDs each instance gets a new ID, no matter if they are equal or not.
* Add the [value] to this list if the list does not already contain an equal item. Return the item contained in the
* list. This is important to make sure that there is only one instance of equal items used in the model, because
* when Jackson generates IDs each instance gets a new ID, no matter if they are equal or not.
*/
private fun <T> MutableList<T>.addIfRequired(value: T): T = find { it == value } ?: value.also { add(it) }

Expand All @@ -718,4 +722,25 @@ internal class EvaluatedModelMapper(private val input: ReporterInput) {
*/
private fun <T> MutableList<T>.addIfRequired(values: Collection<T>): List<T> =
values.map { addIfRequired(it) }.distinct()

/**
* Return a copy of the [RepositoryConfiguration] with [Resolutions] that refer to the same instances as the
* [ResolvedConfiguration] for equal [Resolutions]. This is required for the [EvaluatedModel] to contained indexed
* references instead of duplicate [Resolutions].
*/
private fun Repository.deduplicateResolutions(): Repository {
val resolutions = with(config.resolutions) {
copy(
issues = issues.map { resolution -> issueResolutions.find { resolution == it } ?: resolution },
ruleViolations = ruleViolations.map { resolution ->
ruleViolationResolutions.find { resolution == it } ?: resolution
},
vulnerabilities = vulnerabilities.map { resolution ->
vulnerabilitiesResolutions.find { resolution == it } ?: resolution
}
)
}

return copy(config = config.copy(resolutions = resolutions))
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -275,14 +275,7 @@ class FreemarkerTemplateProcessor(
@JvmOverloads
@Suppress("UNUSED") // This function is used in the templates.
fun hasUnresolvedIssues(threshold: Severity = input.ortConfig.severeIssueThreshold) =
input.ortResult.getIssues().any { (identifier, issues) ->
issues.any { issue ->
val isResolved = input.resolutionProvider.isResolved(issue)
val isExcluded = input.ortResult.isExcluded(identifier)

issue.severity >= threshold && !isResolved && !isExcluded
}
}
input.ortResult.getOpenIssues(minSeverity = threshold).isNotEmpty()

/**
* Return `true` if there are any unresolved and non-excluded [RuleViolation]s whose severity is equal to or
Expand All @@ -291,26 +284,23 @@ class FreemarkerTemplateProcessor(
@JvmOverloads
@Suppress("UNUSED") // This function is used in the templates.
fun hasUnresolvedRuleViolations(threshold: Severity = input.ortConfig.severeRuleViolationThreshold) =
input.ortResult.evaluator?.violations?.any { violation ->
val isResolved = input.resolutionProvider.isResolved(violation)
val isExcluded = violation.pkg?.let { input.ortResult.isExcluded(it) } ?: false

violation.severity >= threshold && !isResolved && !isExcluded
} ?: false
input.ortResult.getRuleViolations(omitResolved = true, minSeverity = threshold).any { violation ->
violation.pkg?.let { input.ortResult.isExcluded(it) } ?: false
}

/**
* 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) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

/**
* Return a list of [SnippetFinding]s grouped by the source file being matched by those snippets.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ repository:
reason: "TEST_DEPENDENCY_OF"
comment: "The scope only contains test dependencies."
resolutions:
issues:
- message: "A test issue\\."
reason: "CANT_FIX_ISSUE"
comment: "A comment explaining why the issue can be ignored."
rule_violations:
- message: "Apache-2.0 hint"
reason: "CANT_FIX_EXCEPTION"
Expand Down Expand Up @@ -758,6 +762,15 @@ resolved_configuration:
curations:
comment: "H2 database offers a license choice"
concluded_license: "MPL-2.0 OR EPL-1.0"
resolutions:
issues:
- message: "A test issue\\."
reason: "CANT_FIX_ISSUE"
comment: "A comment explaining why the issue can be ignored."
rule_violations:
- message: "Apache-2.0 hint"
reason: "CANT_FIX_EXCEPTION"
comment: "Apache-2 is not an issue."
labels:
job_parameters.JOB_PARAM_1: "label job param 1"
job_parameters.JOB_PARAM_2: "label job param 2"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import io.kotest.matchers.shouldBe
import io.kotest.matchers.string.shouldContain

import org.ossreviewtoolkit.model.OrtResult
import org.ossreviewtoolkit.model.utils.DefaultResolutionProvider
import org.ossreviewtoolkit.reporter.ReporterInput
import org.ossreviewtoolkit.utils.common.normalizeLineBreaks
import org.ossreviewtoolkit.utils.common.unpackZip
Expand Down Expand Up @@ -59,12 +58,9 @@ class OpossumReporterFunTest : WordSpec({
})

private fun TestConfiguration.generateReport(ortResult: OrtResult): String {
val input = ReporterInput(
ortResult = ortResult,
resolutionProvider = DefaultResolutionProvider(ortResult.getRepositoryConfigResolutions())
)

val input = ReporterInput(ortResult)
val outputDir = tempdir()

OpossumReporter().generateReport(input, outputDir).single().unpackZip(outputDir)

return outputDir.resolve("input.json").readText()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,10 @@ repository:
reason: "TEST_DEPENDENCY_OF"
comment: "The scope only contains test dependencies."
resolutions:
issues:
- message: "A test issue\\."
reason: "CANT_FIX_ISSUE"
comment: "A comment explaining why the issue can be ignored."
rule_violations:
- message: "Apache-2.0 hint"
reason: "CANT_FIX_EXCEPTION"
Expand Down Expand Up @@ -592,23 +596,23 @@ scanner:
end_time: "1970-01-01T00:00:00Z"
scanners:
"Gradle:org.ossreviewtoolkit:nested-fake-project:1.0.0":
- "FakeScanner"
- "FakeScanner"
"Gradle:org.ossreviewtoolkit.gradle.example:lib:1.0.0":
- "FakeScanner"
- "FakeScanner"
"Ant:junit:junit:4.12":
- "Dummy"
- "Dummy"
"Maven:com.foobar:foobar:1.0":
- "Dummy"
- "Dummy"
"Maven:com.h2database:h2:1.4.200":
- "Dummy"
- "Dummy"
"Maven:org.apache.commons:commons-lang3:3.5":
- "Dummy"
- "Dummy"
"Maven:org.apache.commons:commons-text:1.1":
- "Dummy"
- "Dummy"
"Maven:org.example.test:example-component:1.11":
- "Dummy"
- "Dummy"
"Maven:org.hamcrest:hamcrest-core:1.3":
- "Dummy"
- "Dummy"
files:
- provenance:
vcs_info:
Expand Down Expand Up @@ -758,6 +762,15 @@ resolved_configuration:
curations:
comment: "H2 database offers a license choice"
concluded_license: "MPL-2.0 OR EPL-1.0"
resolutions:
issues:
- message: "A test issue\\."
reason: "CANT_FIX_ISSUE"
comment: "A comment explaining why the issue can be ignored."
rule_violations:
- message: "Apache-2.0 hint"
reason: "CANT_FIX_EXCEPTION"
comment: "Apache-2 is not an issue."
labels:
job_parameters.JOB_PARAM_1: "label job param 1"
job_parameters.JOB_PARAM_2: "label job param 2"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -918,6 +918,10 @@ <h2 id="repository-configuration">Repository Configuration</h2>
reason: "TEST_DEPENDENCY_OF"
comment: "The scope only contains test dependencies."
resolutions:
issues:
- message: "A test issue\\."
reason: "CANT_FIX_ISSUE"
comment: "A comment explaining why the issue can be ignored."
rule_violations:
- message: "Apache-2.0 hint"
reason: "CANT_FIX_EXCEPTION"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ import io.kotest.matchers.shouldBe
import javax.xml.transform.TransformerFactory

import org.ossreviewtoolkit.model.OrtResult
import org.ossreviewtoolkit.model.utils.DefaultResolutionProvider
import org.ossreviewtoolkit.reporter.HowToFixTextProvider
import org.ossreviewtoolkit.reporter.ReporterInput
import org.ossreviewtoolkit.utils.ort.Environment
Expand Down Expand Up @@ -70,7 +69,6 @@ class StaticHtmlReporterFunTest : WordSpec({
private fun TestConfiguration.generateReport(ortResult: OrtResult): String {
val input = ReporterInput(
ortResult = ortResult,
resolutionProvider = DefaultResolutionProvider.create(ortResult),
howToFixTextProvider = HOW_TO_FIX_TEXT_PROVIDER
)

Expand Down
Loading
Loading