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

refactor(PackageBasedScanStorageReader)!: Align read() to take a Package #7641

Merged
merged 1 commit into from
Oct 6, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@

import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.config.OrtConfiguration
import org.ossreviewtoolkit.model.utils.FindingCurationMatcher
Expand Down Expand Up @@ -73,7 +74,7 @@
.convert { it.expandTilde() }

override fun run() {
val scanResults = getStoredScanResults(packageId)
val scanResults = getStoredScanResults(Package.EMPTY.copy(id = packageId))

Check warning on line 77 in helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt

View check run for this annotation

Codecov / codecov/patch

helper-cli/src/main/kotlin/commands/GetPackageLicensesCommand.kt#L77

Added line #L77 was not covered by tests
val packageConfigurationProvider = DirPackageConfigurationProvider(packageConfigurationsDir)

val result = scanResults.firstOrNull()?.let { scanResult ->
Expand Down Expand Up @@ -104,10 +105,10 @@
println(result.toYaml())
}

private fun getStoredScanResults(id: Identifier): List<ScanResult> {
private fun getStoredScanResults(pkg: Package): List<ScanResult> {
val ortConfiguration = OrtConfiguration.load(configArguments, configFile)
val scanStorages = ScanStorages.createFromConfig(ortConfiguration.scanner)
return runCatching { scanStorages.read(id) }.getOrDefault(emptyList())
return runCatching { scanStorages.read(pkg) }.getOrDefault(emptyList())
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import com.github.ajalt.clikt.parameters.types.file

import org.ossreviewtoolkit.helper.utils.logger
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.config.OrtConfiguration
import org.ossreviewtoolkit.model.toYaml
import org.ossreviewtoolkit.scanner.ScanStorages
Expand Down Expand Up @@ -68,7 +69,7 @@ internal class ListStoredScanResultsCommand : CliktCommand(
"Searching for scan results of '${packageId.toCoordinates()}' in ${scanStorages.readers.size} storage(s)."
)

val scanResults = runCatching { scanStorages.read(packageId) }.getOrElse {
val scanResults = runCatching { scanStorages.read(Package.EMPTY.copy(id = packageId)) }.getOrElse {
logger.error { "Could not read scan results: ${it.message}" }
throw ProgramResult(1)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.ossreviewtoolkit.helper.utils.write
import org.ossreviewtoolkit.model.ArtifactProvenance
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.RepositoryProvenance
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.config.PackageConfiguration
Expand Down Expand Up @@ -119,7 +120,7 @@
outputDir.safeMkdirs()

val scanResultsStorage = FileBasedStorage(LocalFileStorage(scanResultsStorageDir))
val scanResults = scanResultsStorage.read(packageId).getOrThrow().run {
val scanResults = scanResultsStorage.read(Package.EMPTY.copy(id = packageId)).getOrThrow().run {

Check warning on line 123 in helper-cli/src/main/kotlin/commands/packageconfig/CreateCommand.kt

View check run for this annotation

Codecov / codecov/patch

helper-cli/src/main/kotlin/commands/packageconfig/CreateCommand.kt#L123

Added line #L123 was not covered by tests
listOfNotNull(
find { it.provenance is RepositoryProvenance },
find { it.provenance is ArtifactProvenance }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
val scanResult = ScanResult(provenanceWithSourceArtifact1, scannerDetails1, scanSummaryWithFiles)

val addResult = storage.add(id1, scanResult)
val readResult = storage.read(id1)
val readResult = storage.read(pkg1)

addResult.shouldBeSuccess()
readResult.shouldBeSuccess {
Expand All @@ -140,7 +140,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
val scanResult = ScanResult(provenanceEmpty, scannerDetails1, scanSummaryWithFiles)

val addResult = storage.add(id1, scanResult)
val readResult = storage.read(id1)
val readResult = storage.read(pkg1)

addResult.shouldBeFailure {
it.message shouldBe "Not storing scan result for '${id1.toCoordinates()}' because no provenance " +
Expand All @@ -167,7 +167,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec
addResult1.shouldBeSuccess()
addResult2.shouldBeFailure()

val readResult = storage.read(id1)
val readResult = storage.read(pkg1)
readResult.shouldBeSuccess {
it should containExactly(scanResult1)
}
Expand All @@ -181,7 +181,7 @@ abstract class AbstractStorageFunTest(vararg listeners: TestListener) : WordSpec

storage.add(id1, scanResult1).shouldBeSuccess()
storage.add(id1, scanResult2).shouldBeSuccess()
val readResult = storage.read(id1)
val readResult = storage.read(pkg1)

readResult.shouldBeSuccess {
it should containExactlyInAnyOrder(scanResult1, scanResult2)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,12 @@ import java.time.Instant
import kotlin.time.Duration.Companion.seconds

import org.ossreviewtoolkit.clients.clearlydefined.ClearlyDefinedService.Server
import org.ossreviewtoolkit.model.Hash
import org.ossreviewtoolkit.model.HashAlgorithm
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.LicenseFinding
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.RemoteArtifact
import org.ossreviewtoolkit.model.RepositoryProvenance
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.ScanSummary
Expand All @@ -44,10 +48,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
val storage = ClearlyDefinedStorage(ClearlyDefinedStorageConfiguration(Server.PRODUCTION.apiUrl))

"Scan results for 'semver4j' from ScanCode should be read correctly" {
val id = Identifier("Maven:com.vdurmont:semver4j:3.1.0")
val pkg = Package.EMPTY.copy(
id = Identifier("Maven:com.vdurmont:semver4j:3.1.0"),
binaryArtifact = RemoteArtifact(
url = "https://repo1.maven.org/maven2/com/vdurmont/semver4j/3.1.0/semver4j-3.1.0.jar",
hash = Hash(
value = "0de1248f09dfe8df3b021c84e0642ee222cceb13",
algorithm = HashAlgorithm.SHA1
)
)
)
mnonnenmacher marked this conversation as resolved.
Show resolved Hide resolved

withRetry {
val results = storage.read(id)
val results = storage.read(pkg)

results.shouldBeSuccess {
it shouldContain ScanResult(
Expand Down Expand Up @@ -98,10 +111,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
}

"Scan results for 'hoplite-core' from ScanCode should be read correctly" {
val id = Identifier("Maven:com.sksamuel.hoplite:hoplite-core:2.1.3")
val pkg = Package.EMPTY.copy(
id = Identifier("Maven:com.sksamuel.hoplite:hoplite-core:2.1.3"),
binaryArtifact = RemoteArtifact(
url = "https://repo1.maven.org/maven2/com/sksamuel/hoplite/hoplite-core/2.1.3/hoplite-core-2.1.3.jar",
hash = Hash(
value = "143f3c28ac4987907473fb608bce1fe317663ba8",
algorithm = HashAlgorithm.SHA1
)
)
)

withRetry {
val results = storage.read(id)
val results = storage.read(pkg)

results.shouldBeSuccess {
it shouldContain ScanResult(
Expand Down Expand Up @@ -132,10 +154,19 @@ class ClearlyDefinedStorageFunTest : StringSpec({
}

"Scan results for packages without a namespace should be present" {
val id = Identifier("NPM::iobroker.eusec:0.9.9")
val pkg = Package.EMPTY.copy(
id = Identifier("NPM::iobroker.eusec:0.9.9"),
binaryArtifact = RemoteArtifact(
url = "https://registry.npmjs.org/iobroker.eusec/-/iobroker.eusec-0.9.9.tgz",
hash = Hash(
value = "b3cf4aeefd31f64907cab7ea7a419f1054137a09",
algorithm = HashAlgorithm.SHA1
)
)
)

withRetry {
val results = storage.read(id)
val results = storage.read(pkg)

results.shouldBeSuccess { result ->
result.map { it.provenance } shouldContain RepositoryProvenance(
Expand Down
18 changes: 9 additions & 9 deletions scanner/src/main/kotlin/ScanResultsStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -55,15 +55,15 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
open val name: String = javaClass.simpleName

/**
* Return all [ScanResult]s contained in this [ScanResultsStorage] corresponding to the package denoted by the given
* [id] wrapped in a [Result].
* Return all [ScanResult]s contained in this [ScanResultsStorage] corresponding to the [package][pkg] wrapped in a
* [Result].
*/
fun read(id: Identifier): Result<List<ScanResult>> {
val (result, duration) = measureTimedValue { readInternal(id) }
fun read(pkg: Package): Result<List<ScanResult>> {
Copy link
Member

Choose a reason for hiding this comment

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

commit and this read() function: I have difficulties understanding which parts of the package will be used to determine which scan results are being returned and how. Can you help?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's indeed a bit hard to understand as the relevant parts are not implemented yet... because they couldn't, as the ClearlyDefined scan storage provider did not have access to the necessary information before this change.

Basically, the ClearlyDefined implementation would need to have access to the VCS location, source artifact location (and maybe also the binary source artifact location) in order to create a proper Coordinate for querying harvested (scan) results. A Coordinate includes a ComponentType and Provider, and in order to determine these properly, more Package properties than just the Identifier are required.

Copy link
Member

Choose a reason for hiding this comment

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

As Package contains many irrelevant properties I would prefer if the interface would only require the relevant values to have a cleaner API. But I would be fine with looking into it after fixing the ClearlyDefinedStorage.

val (result, duration) = measureTimedValue { readInternal(pkg) }

result.onSuccess { results ->
logger.info {
"Read ${results.size} scan result(s) for '${id.toCoordinates()}' from ${javaClass.simpleName} in " +
"Read ${results.size} scan result(s) for '${pkg.id.toCoordinates()}' from ${javaClass.simpleName} in " +
"$duration."
}
}
Expand Down Expand Up @@ -143,14 +143,14 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
/**
* Internal version of [read].
*/
protected abstract fun readInternal(id: Identifier): Result<List<ScanResult>>
protected abstract fun readInternal(pkg: Package): Result<List<ScanResult>>

/**
* Internal version of [read]. Implementations may want to override this function if they can filter for the wanted
* [scannerCriteria] in a more efficient way.
*/
protected open fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result<List<ScanResult>> =
readInternal(pkg.id).map { results ->
readInternal(pkg).map { results ->
if (results.isEmpty()) {
results
} else {
Expand Down Expand Up @@ -210,8 +210,8 @@ abstract class ScanResultsStorage : PackageBasedScanStorage {
*/
protected abstract fun addInternal(id: Identifier, scanResult: ScanResult): Result<Unit>

override fun read(id: Identifier, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult> =
read(id).toNestedProvenanceScanResult(nestedProvenance)
override fun read(pkg: Package, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult> =
read(pkg).toNestedProvenanceScanResult(nestedProvenance)

override fun read(
pkg: Package,
Expand Down
5 changes: 2 additions & 3 deletions scanner/src/main/kotlin/ScanStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
package org.ossreviewtoolkit.scanner

import org.ossreviewtoolkit.model.ArtifactProvenance
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.KnownProvenance
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.Provenance
Expand All @@ -40,12 +39,12 @@ sealed interface ScanStorageReader
*/
interface PackageBasedScanStorageReader : ScanStorageReader {
/**
* Read all [ScanResult]s for the provided [id]. The package scan results are converted to a
* Read all [ScanResult]s for the provided [package][pkg]. The package scan results are converted to a
* [NestedProvenanceScanResult] using the provided [nestedProvenance].
*
* Throws a [ScanStorageException] if an error occurs while reading from the storage.
*/
fun read(id: Identifier, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult>
fun read(pkg: Package, nestedProvenance: NestedProvenance): List<NestedProvenanceScanResult>

/**
* Read all [ScanResult]s for the provided [package][pkg] matching the [provenance][KnownProvenance.matches] and the
Expand Down
12 changes: 6 additions & 6 deletions scanner/src/main/kotlin/ScanStorages.kt
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@

package org.ossreviewtoolkit.scanner

import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.config.ClearlyDefinedStorageConfiguration
import org.ossreviewtoolkit.model.config.FileBasedStorageConfiguration
Expand Down Expand Up @@ -80,11 +80,11 @@ class ScanStorages(
}

/**
* Read all [ScanResult]s for the provided [id]. Returns an empty list if no stored scan results or no stored
* provenance can be found.
* Read all [ScanResult]s for the provided [package][pkg]. Returns an empty list if no stored scan results or no
* stored provenance can be found.
*/
fun read(id: Identifier): List<ScanResult> {
val packageProvenances = packageProvenanceStorage.readProvenances(id)
fun read(pkg: Package): List<ScanResult> {
val packageProvenances = packageProvenanceStorage.readProvenances(pkg.id)

val nestedProvenances = packageProvenances.mapNotNull { result ->
when (result) {
Expand All @@ -104,7 +104,7 @@ class ScanStorages(

nestedProvenances.forEach { nestedProvenance ->
results += readers.filterIsInstance<PackageBasedScanStorageReader>().flatMap { reader ->
reader.read(id, nestedProvenance).flatMap { it.merge() }
reader.read(pkg, nestedProvenance).flatMap { it.merge() }
}

results += readers.filterIsInstance<ProvenanceBasedScanStorageReader>().mapNotNull { reader ->
Expand Down
4 changes: 2 additions & 2 deletions scanner/src/main/kotlin/storages/ClearlyDefinedStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -85,8 +85,8 @@ class ClearlyDefinedStorage(
ClearlyDefinedService.create(config.serverUrl, client ?: OkHttpClientHelper.buildClient())
}

override fun readInternal(id: Identifier): Result<List<ScanResult>> =
runBlocking(Dispatchers.IO) { readFromClearlyDefined(Package.EMPTY.copy(id = id)) }
override fun readInternal(pkg: Package): Result<List<ScanResult>> =
runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) }

override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria): Result<List<ScanResult>> =
runBlocking(Dispatchers.IO) { readFromClearlyDefined(pkg) }
Expand Down
11 changes: 7 additions & 4 deletions scanner/src/main/kotlin/storages/FileBasedStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
import org.apache.logging.log4j.kotlin.Logging

import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.yamlMapper
import org.ossreviewtoolkit.scanner.ScanResultsStorage
Expand All @@ -51,8 +52,8 @@

override val name = "${javaClass.simpleName} with ${backend.javaClass.simpleName} backend"

override fun readInternal(id: Identifier): Result<List<ScanResult>> {
val path = storagePath(id)
override fun readInternal(pkg: Package): Result<List<ScanResult>> {
val path = storagePath(pkg.id)

return runCatching {
backend.read(path).use { input ->
Expand All @@ -62,7 +63,7 @@
// If the file cannot be found it means no scan results have been stored, yet.
if (it is FileNotFoundException) return EMPTY_RESULT

val message = "Could not read scan results for '${id.toCoordinates()}' from path '$path': " +
val message = "Could not read scan results for '${pkg.id.toCoordinates()}' from path '$path': " +

Check warning on line 66 in scanner/src/main/kotlin/storages/FileBasedStorage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/FileBasedStorage.kt#L66

Added line #L66 was not covered by tests
it.collectMessages()

logger.info { message }
Expand All @@ -72,7 +73,9 @@
}

override fun addInternal(id: Identifier, scanResult: ScanResult): Result<Unit> {
val existingScanResults = read(id).getOrDefault(emptyList())
// Note: The file-based `read()`-implementation does not require the full `Package` information for reading
// existing results, so it is fine to use an empty package here.
val existingScanResults = read(Package.EMPTY.copy(id = id)).getOrDefault(emptyList())

if (existingScanResults.any { it.scanner == scanResult.scanner && it.provenance == scanResult.provenance }) {
val message = "Did not store scan result for '${id.toCoordinates()}' because a scan result for the same " +
Expand Down
2 changes: 1 addition & 1 deletion scanner/src/main/kotlin/storages/NoStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
* errors.
*/
class NoStorage : ScanResultsStorage() {
override fun readInternal(id: Identifier) = EMPTY_RESULT
override fun readInternal(pkg: Package) = EMPTY_RESULT

Check warning on line 34 in scanner/src/main/kotlin/storages/NoStorage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/NoStorage.kt#L34

Added line #L34 was not covered by tests
override fun readInternal(pkg: Package, scannerCriteria: ScannerCriteria) = EMPTY_RESULT
override fun addInternal(id: Identifier, scanResult: ScanResult) = Result.success(Unit)
}
6 changes: 3 additions & 3 deletions scanner/src/main/kotlin/storages/PostgresStorage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -131,16 +131,16 @@
""".trimIndent()
)

override fun readInternal(id: Identifier): Result<List<ScanResult>> =
override fun readInternal(pkg: Package): Result<List<ScanResult>> =
runCatching {
database.transaction {
ScanResultDao.find { ScanResults.identifier eq id.toCoordinates() }.map { it.scanResult }
ScanResultDao.find { ScanResults.identifier eq pkg.id.toCoordinates() }.map { it.scanResult }
}
}.onFailure {
if (it is JsonProcessingException || it is SQLException) {
it.showStackTrace()

val message = "Could not read scan results for '${id.toCoordinates()}' from database: " +
val message = "Could not read scan results for '${pkg.id.toCoordinates()}' from database: " +

Check warning on line 143 in scanner/src/main/kotlin/storages/PostgresStorage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/PostgresStorage.kt#L143

Added line #L143 was not covered by tests
it.collectMessages()

logger.info { message }
Expand Down
9 changes: 5 additions & 4 deletions scanner/src/main/kotlin/storages/Sw360Storage.kt
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.eclipse.sw360.http.config.HttpClientConfig

import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.Package
import org.ossreviewtoolkit.model.ScanResult
import org.ossreviewtoolkit.model.config.Sw360StorageConfiguration
import org.ossreviewtoolkit.model.jsonMapper
Expand Down Expand Up @@ -85,19 +86,19 @@
private val connectionFactory = createConnection(configuration)
private val releaseClient = connectionFactory.releaseAdapter

override fun readInternal(id: Identifier): Result<List<ScanResult>> {
val tempScanResultFile = createTempFileForUpload(id)
override fun readInternal(pkg: Package): Result<List<ScanResult>> {
val tempScanResultFile = createTempFileForUpload(pkg.id)

Check warning on line 90 in scanner/src/main/kotlin/storages/Sw360Storage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/Sw360Storage.kt#L90

Added line #L90 was not covered by tests

val result = runCatching {
releaseClient.getSparseReleaseByNameAndVersion(createReleaseName(id), id.version)
releaseClient.getSparseReleaseByNameAndVersion(createReleaseName(pkg.id), pkg.id.version)

Check warning on line 93 in scanner/src/main/kotlin/storages/Sw360Storage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/Sw360Storage.kt#L93

Added line #L93 was not covered by tests
.flatMap { releaseClient.getReleaseById(it.releaseId) }
.map { getScanResultOfRelease(it, tempScanResultFile.toPath()) }
.orElse(emptyList())
.map { path ->
path.toFile().readValue<ScanResult>()
}
}.recoverCatching {
val message = "Could not read scan results for '${id.toCoordinates()}' in SW360: ${it.message}"
val message = "Could not read scan results for '${pkg.id.toCoordinates()}' in SW360: ${it.message}"

Check warning on line 101 in scanner/src/main/kotlin/storages/Sw360Storage.kt

View check run for this annotation

Codecov / codecov/patch

scanner/src/main/kotlin/storages/Sw360Storage.kt#L101

Added line #L101 was not covered by tests

logger.info { message }

Expand Down
Loading