Skip to content

Commit

Permalink
refactor(fossid): Change functions to return issues
Browse files Browse the repository at this point in the history 
Change several functions to return the created issues instead of
adding them to a provided mutable list to adhere to the principle of
immutability.

Signed-off-by: Martin Nonnenmacher <[email protected]>
  • Loading branch information
@mnonnenmacher
mnonnenmacher committed Jun 20, 2024
1 parent f19e8d9 commit bde6a36
Show file tree
Hide file tree
Showing 4 changed files with 77 additions and 83 deletions.
83 changes: 44 additions & 39 deletions plugins/scanners/fossid/src/main/kotlin/FossId.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,12 +302,10 @@ class FossId internal constructor(
.checkResponse("list scans for project").data
checkNotNull(scans)

val issues = mutableListOf<Issue>()

val (scanCode, scanId) = if (config.deltaScans) {
checkAndCreateDeltaScan(scans, url, revision, projectCode, projectName, context, issues)
val result = if (config.deltaScans) {
checkAndCreateDeltaScan(scans, url, revision, projectCode, projectName, context)
} else {
checkAndCreateScan(scans, url, revision, projectCode, projectName, context, issues)
checkAndCreateScan(scans, url, revision, projectCode, projectName, context)
}

if (config.waitForResult && provenance is RepositoryProvenance) {
Expand All @@ -332,14 +330,12 @@ class FossId internal constructor(
"false positives."
}

val rawResults = getRawResults(scanCode, snippetChoices?.choices.orEmpty())
val rawResults = getRawResults(result.scanCode, snippetChoices?.choices.orEmpty())
createResultSummary(
startTime,
provenance,
rawResults,
scanCode,
scanId,
issues,
result,
context.detectedLicenseMapping,
snippetChoices?.choices.orEmpty()
)
Expand All @@ -356,7 +352,11 @@ class FossId internal constructor(
provenance,
details,
summary,
mapOf(SCAN_CODE_KEY to scanCode, SCAN_ID_KEY to scanId, SERVER_URL_KEY to config.serverUrl)
mapOf(
SCAN_CODE_KEY to result.scanCode,
SCAN_ID_KEY to result.scanId,
SERVER_URL_KEY to config.serverUrl
)
)
}
} catch (e: IllegalStateException) {
Expand Down Expand Up @@ -462,12 +462,11 @@ class FossId internal constructor(
revision: String,
projectCode: String,
projectName: String,
context: ScanContext,
issues: MutableList<Issue>
): Pair<String, String> {
context: ScanContext
): FossIdResult {
val existingScan = scans.recentScansForRepository(url, revision = revision).findLatestPendingOrFinishedScan()

val scanCodeAndId = if (existingScan == null) {
val result = if (existingScan == null) {
logger.info { "No scan found for $url and revision $revision. Creating scan..." }

val scanCode = namingProvider.createScanCode(projectName = projectName, branch = revision)
Expand All @@ -478,22 +477,22 @@ class FossId internal constructor(
service.downloadFromGit(config.user, config.apiKey, scanCode)
.checkResponse("download data from Git", false)

createIgnoreRules(scanCode, context.excludes, issues = issues)
val issues = createIgnoreRules(scanCode, context.excludes)

scanCode to scanId
FossIdResult(scanCode, scanId, issues)
} else {
logger.info { "Scan '${existingScan.code}' found for $url and revision $revision." }

val existingScanCode = requireNotNull(existingScan.code) {
"The code for an existing scan must not be null."
}

existingScanCode to existingScan.id.toString()
FossIdResult(existingScanCode, existingScan.id.toString())
}

if (config.waitForResult) checkScan(scanCodeAndId.first)
if (config.waitForResult) checkScan(result.scanCode)

return scanCodeAndId
return result
}

/**
Expand All @@ -505,9 +504,8 @@ class FossId internal constructor(
revision: String,
projectCode: String,
projectName: String,
context: ScanContext,
issues: MutableList<Issue>
): Pair<String, String> {
context: ScanContext
): FossIdResult {
val projectRevision = context.labels[PROJECT_REVISION_LABEL]

val vcs = requireNotNull(VersionControlSystem.forUrl(url))
Expand Down Expand Up @@ -561,8 +559,10 @@ class FossId internal constructor(
service.downloadFromGit(config.user, config.apiKey, scanCode)
.checkResponse("download data from Git", false)

val issues = mutableListOf<Issue>()

if (existingScan == null) {
createIgnoreRules(scanCode, context.excludes, issues = issues)
issues += createIgnoreRules(scanCode, context.excludes)

if (config.waitForResult) checkScan(scanCode)
} else {
Expand All @@ -585,7 +585,7 @@ class FossId internal constructor(
val exclusions = setOf(".git", "^\\.git")
val filteredRules = rules.filterNot { it.type == RuleType.DIRECTORY && it.value in exclusions }

createIgnoreRules(scanCode, context.excludes, filteredRules, issues)
issues += createIgnoreRules(scanCode, context.excludes, filteredRules)
}

logger.info { "Reusing identifications from scan '$existingScanCode'." }
Expand All @@ -601,7 +601,7 @@ class FossId internal constructor(
enforceDeltaScanLimit(recentScans)
}

return scanCode to scanId
return FossIdResult(scanCode, scanId, issues)
}

/**
Expand Down Expand Up @@ -631,17 +631,16 @@ class FossId internal constructor(
private suspend fun createIgnoreRules(
scanCode: String,
excludes: Excludes?,
existingRules: List<IgnoreRule> = emptyList(),
issues : MutableList<Issue>
) {
val excludesRules = excludes?.let {
convertRules(it, issues).also {
logger.info { "${it.size} rules from ORT excludes have been found." }
existingRules: List<IgnoreRule> = emptyList()
): List<Issue> {
val (excludesRules, excludeRuleIssues) = (excludes ?: Excludes.EMPTY).let {
convertRules(it).also { (rules, _) ->
logger.info { "${rules.size} rules from ORT excludes have been found." }
}
}.orEmpty()
}

// Create an issue for each legacy rule.
val legacyRules = existingRules.filterLegacyRules(excludesRules, issues)
val (legacyRules, legacyRuleIssues) = existingRules.filterLegacyRules(excludesRules)
if (legacyRules.isNotEmpty()) {
logger.warn { "${legacyRules.size} legacy rules have been found." }
}
Expand All @@ -654,6 +653,8 @@ class FossId internal constructor(
"Ignore rule of type '${it.type}' and value '${it.value}' has been created for the new scan."
}
}

return excludeRuleIssues + legacyRuleIssues
}

/**
Expand Down Expand Up @@ -910,9 +911,7 @@ class FossId internal constructor(
startTime: Instant,
provenance: Provenance,
rawResults: RawResults,
scanCode: String,
scanId: String,
additionalIssues: List<Issue>,
result: FossIdResult,
detectedLicenseMapping: Map<String, String>,
snippetChoices: List<SnippetChoice>
): ScanResult {
Expand All @@ -930,7 +929,7 @@ class FossId internal constructor(
snippetLicenseFindings
)
val newlyMarkedFiles = markFilesWithChosenSnippetsAsIdentified(
scanCode,
result.scanCode,
snippetChoices,
snippetFindings,
rawResults.listPendingFiles,
Expand Down Expand Up @@ -960,14 +959,14 @@ class FossId internal constructor(
licenseFindings = licenseFindings + snippetLicenseFindings,
copyrightFindings = copyrightFindings,
snippetFindings = snippetFindings,
issues = issues + additionalIssues
issues = issues + result.issues
)

return ScanResult(
provenance,
details,
summary,
mapOf(SCAN_CODE_KEY to scanCode, SCAN_ID_KEY to scanId, SERVER_URL_KEY to config.serverUrl)
mapOf(SCAN_CODE_KEY to result.scanCode, SCAN_ID_KEY to result.scanId, SERVER_URL_KEY to config.serverUrl)
)
}

Expand Down Expand Up @@ -1079,3 +1078,9 @@ class FossId internal constructor(
return result
}
}

private data class FossIdResult(
val scanCode: String,
val scanId: String,
val issues: List<Issue> = emptyList()
)
31 changes: 18 additions & 13 deletions plugins/scanners/fossid/src/main/kotlin/Utils.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,13 @@ private val EXTENSION_REGEX = "\\*\\.(?<extension>\\w+)".toRegex()
private val FILE_REGEX = "(?<file>[^/]+)".toRegex()

/**
* Convert the ORT [path excludes][Excludes.paths] in [excludes] to FossID [IgnoreRule]s. If an error is encountered
* during the mapping, an issue is added to [issues].
* Return the ORT [path excludes][Excludes.paths] in [excludes] converted to FossID [IgnoreRule]s and any errors that
* occurred during the conversion.
*/
internal fun convertRules(excludes: Excludes, issues: MutableList<Issue>): List<IgnoreRule> {
return excludes.paths.mapNotNull { pathExclude ->
internal fun convertRules(excludes: Excludes): Pair<List<IgnoreRule>, List<Issue>> {
val issues = mutableListOf<Issue>()

val ignoreRules = excludes.paths.mapNotNull { pathExclude ->
pathExclude.mapToRule().alsoIfNull {
issues += Issue(
source = "FossID.convertRules",
Expand All @@ -55,6 +57,8 @@ internal fun convertRules(excludes: Excludes, issues: MutableList<Issue>): List<
}
}
}

return ignoreRules to issues
}

@Suppress("UnsafeCallOnNullableType")
Expand Down Expand Up @@ -85,20 +89,21 @@ private fun PathExclude.mapToRule(): IgnoreRule? {

/**
* Filter [IgnoreRule]s which are not contained in the [referenceRules]. These are legacy rules because they were not
* created from the [Excludes] defined in the repository configuration. Also create an [Issue] for each legacy rule and
* add it to [issues].
* created from the [Excludes] defined in the repository configuration. Also create an [Issue] for each legacy rule.
*/
internal fun List<IgnoreRule>.filterLegacyRules(
referenceRules: List<IgnoreRule>,
issues: MutableList<Issue>
): List<IgnoreRule> =
filterNot { rule ->
internal fun List<IgnoreRule>.filterLegacyRules(referenceRules: List<IgnoreRule>): Pair<List<IgnoreRule>, List<Issue>> {
val legacyRules = filterNot { rule ->
referenceRules.any { it.value == rule.value && it.type == rule.type }
}.onEach {
issues += Issue(
}

val issues = legacyRules.map {
Issue(
source = "FossID.compare",
message = "Rule '${it.value}' with type '${it.type}' is not present in the .ort.yml path excludes. " +
"Add it to the .ort.yml file or remove it from the FossID scan.",
severity = Severity.HINT
)
}

return legacyRules to issues
}
6 changes: 2 additions & 4 deletions plugins/scanners/fossid/src/test/kotlin/LegacyRulesTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,8 @@ class LegacyRulesTest : WordSpec({
val rulesToTest = listOf(
IgnoreRule(-1, RuleType.EXTENSION, ".pdf", -1, "")
)
val issues = mutableListOf<Issue>()

val legacyRules = rulesToTest.filterLegacyRules(referenceRules, issues)
val (legacyRules, issues) = rulesToTest.filterLegacyRules(referenceRules)

issues shouldHaveSize 1
issues.first() shouldNotBeNull {
Expand All @@ -63,9 +62,8 @@ class LegacyRulesTest : WordSpec({
val rulesToTest = listOf(
IgnoreRule(-1, RuleType.DIRECTORY, "directory", -1, "")
)
val issues = mutableListOf<Issue>()

val legacyRules = rulesToTest.filterLegacyRules(referenceRules, issues)
val (legacyRules, issues) = rulesToTest.filterLegacyRules(referenceRules)

issues should beEmpty()
legacyRules should beEmpty()
Expand Down
Loading

0 comments on commit bde6a36

Please sign in to comment.