feat(scanner): Improve the behavior when storing conflicting results

If ORT is run in parallel, e.g. on different runners on CI, it may
happen that a package which has no corresponding storage entry is
scanned in parallel on mutliple nodes. Once the nodes attempt to store
the scan results, only the writing of the first result succeeds. But the
following nodes attempt to overwrite the result (with the same value),
which results in the scan storage to write the stack trace to the log
as error. Also it (re-)throws the exception which is caught again by the
`Scanner` which again logs the stack trace as a warning.

There is no problem with the functionality, but the logs look as if
there was a serious issue. This is not true, because the attempt to
write the scan results must have been to overwrite a value with the
exact same value. This holds, because the key / unique constraints have
been choosen such that it does.

Adjust the interface and the implementations to not throw anymore on
unique constraint violation, because overwriting data with identical
data has no impact anyway. Still leave a message in the debug log,
but without the stack trace in order to reduce the amount of spamming,
as this behavior is expected. Furthermore, turn the return type of
`write()` into `Boolean`, so that it is still able to examine / assert
all previously implemented test cases.

Note: According to [1] the `insertIgnore()` seems to target this use
      case.

[1]: https://www.jetbrains.com/help/exposed/dsl-crud-operations.html#batch-insert

Signed-off-by: Frank Viernau <[email protected]>

Author: fviernau

scanner/src/funTest/kotlin/storages/AbstractProvenanceBasedScanStorageFunTest.kt

@@ -25,6 +25,7 @@ import io.kotest.core.spec.style.WordSpec
 import io.kotest.matchers.collections.containExactly
 import io.kotest.matchers.collections.containExactlyInAnyOrder
 import io.kotest.matchers.should
+import io.kotest.matchers.shouldBe
 
 import org.ossreviewtoolkit.model.ArtifactProvenance
 import org.ossreviewtoolkit.model.Hash
@@ -61,22 +62,25 @@ abstract class AbstractProvenanceBasedScanStorageFunTest(vararg listeners: TestL
             "succeed for a valid scan result" {
                 val scanResult = createScanResult()
 
-                storage.write(scanResult)
+                val writeResult = storage.write(scanResult)
                 val readResult = storage.read(scanResult.provenance as KnownProvenance)
 
+                writeResult shouldBe true
                 readResult should containExactly(scanResult)
             }
 
-            "fail if provenance information is missing" {
-                val scanResult = createScanResult(provenance = UnknownProvenance)
+            "return false if a result for the same provenance and scanner is already stored" {
+                val scanResult = createScanResult()
+                storage.write(scanResult)
 
-                shouldThrow<ScanStorageException> { storage.write(scanResult) }
+                val writeResult = storage.write(scanResult)
+
+                writeResult shouldBe false
             }
 
-            "fail if a result for the same provenance and scanner is already stored" {
-                val scanResult = createScanResult()
+            "fail if provenance information is missing" {
+                val scanResult = createScanResult(provenance = UnknownProvenance)
 
-                storage.write(scanResult)
                 shouldThrow<ScanStorageException> { storage.write(scanResult) }
             }
 
@@ -97,9 +101,11 @@ abstract class AbstractProvenanceBasedScanStorageFunTest(vararg listeners: TestL
                         vcsInfo = VcsInfo.valid().copy(revision = "another")
                     )
                 )
-
                 storage.write(scanResult1)
-                shouldThrow<ScanStorageException> { storage.write(scanResult2) }
+
+                val writeResult = storage.write(scanResult2)
+
+                writeResult shouldBe false
             }
         }
 

scanner/src/main/kotlin/ScanStorage.kt

@@ -97,15 +97,15 @@ interface ProvenanceBasedScanStorageWriter : ScanStorageWriter {
     /**
      * Write the provided [scanResult] to the storage. The [scanResult] must have a [KnownProvenance], and if it has a
      * [RepositoryProvenance] the VCS path must be empty, because only scan results for complete repositories are
-     * stored.
+     * stored. If the storage already contains a result for the same storage key, the write operation is skipped
+     * and false is returned.
      *
      * A [ScanStorageException] is thrown if:
      * * An error occurs while writing to the storage.
-     * * The storage already contains a result for the same provenance and scanner.
      * * The provenance of the [scanResult] is [unknown][UnknownProvenance].
      * * The provenance of the [scanResult] is a [RepositoryProvenance] with a non-empty VCS path.
      */
-    fun write(scanResult: ScanResult)
+    fun write(scanResult: ScanResult): Boolean
 }
 
 /**

scanner/src/main/kotlin/storages/ProvenanceBasedFileStorage.kt

@@ -75,7 +75,7 @@ class ProvenanceBasedFileStorage(private val backend: FileStorage) : ProvenanceB
         }
     }
 
-    override fun write(scanResult: ScanResult) {
+    override fun write(scanResult: ScanResult): Boolean {
         val provenance = scanResult.provenance
 
         requireEmptyVcsPath(provenance)
@@ -87,9 +87,12 @@ class ProvenanceBasedFileStorage(private val backend: FileStorage) : ProvenanceB
         val existingScanResults = read(provenance)
 
         if (existingScanResults.any { it.matches(scanResult) }) {
-            throw ScanStorageException(
-                "Storage already contains a result for ${scanResult.provenance} and ${scanResult.scanner}."
-            )
+            logger.debug {
+                "Not writing scan result because storage already contains a result for ${scanResult.provenance} and " +
+                    "${scanResult.scanner}."
+            }
+
+            return false
         }
 
         val scanResults = existingScanResults + scanResult
@@ -101,6 +104,7 @@ class ProvenanceBasedFileStorage(private val backend: FileStorage) : ProvenanceB
         runCatching {
             backend.write(path, input)
             logger.debug { "Stored scan result for '$provenance' at path '$path'." }
+            return true
         }.onFailure {
             when (it) {
                 is IllegalArgumentException, is IOException -> {
@@ -114,6 +118,8 @@ class ProvenanceBasedFileStorage(private val backend: FileStorage) : ProvenanceB
                 else -> throw it
             }
         }
+
+        return false
     }
 }
 

scanner/src/main/kotlin/storages/ProvenanceBasedPostgresStorage.kt

@@ -25,13 +25,14 @@ import javax.sql.DataSource
 
 import org.apache.logging.log4j.kotlin.logger
 
+import org.jetbrains.exposed.dao.id.EntityID
 import org.jetbrains.exposed.dao.id.IntIdTable
 import org.jetbrains.exposed.sql.Database
 import org.jetbrains.exposed.sql.SchemaUtils
 import org.jetbrains.exposed.sql.SchemaUtils.withDataBaseLock
 import org.jetbrains.exposed.sql.and
 import org.jetbrains.exposed.sql.andWhere
-import org.jetbrains.exposed.sql.insert
+import org.jetbrains.exposed.sql.insertIgnoreAndGetId
 import org.jetbrains.exposed.sql.selectAll
 
 import org.ossreviewtoolkit.model.ArtifactProvenance
@@ -132,7 +133,7 @@ class ProvenanceBasedPostgresStorage(
     // TODO: Override read(provenance, scannerMatcher) to make it more efficient by matching the scanner details in the
     //       query.
 
-    override fun write(scanResult: ScanResult) {
+    override fun write(scanResult: ScanResult): Boolean {
         val provenance = scanResult.provenance
 
         requireEmptyVcsPath(provenance)
@@ -141,9 +142,11 @@ class ProvenanceBasedPostgresStorage(
             throw ScanStorageException("Scan result must have a known provenance, but it is $provenance.")
         }
 
+        var rowId: EntityID<Int>? = null
+
         try {
             database.transaction {
-                table.insert {
+                rowId = table.insertIgnoreAndGetId {
                     when (provenance) {
                         is ArtifactProvenance -> {
                             it[artifactUrl] = provenance.sourceArtifact.url
@@ -170,6 +173,15 @@ class ProvenanceBasedPostgresStorage(
 
             throw ScanStorageException(e)
         }
+
+        if (rowId == null) {
+            logger.debug {
+                "Not writing scan result because storage already contains a result for ${scanResult.provenance} and " +
+                    "${scanResult.scanner}."
+            }
+        }
+
+        return rowId != null
     }
 }
 

scanner/src/test/kotlin/ScannerTest.kt

@@ -196,8 +196,9 @@ class ScannerTest : WordSpec({
 
             val storedScanResults = mutableListOf<ScanResult>()
             val writer = object : ProvenanceBasedScanStorageWriter {
-                override fun write(scanResult: ScanResult) {
+                override fun write(scanResult: ScanResult): Boolean {
                     storedScanResults.add(scanResult)
+                    return true
                 }
             }
 
@@ -1038,7 +1039,7 @@ private class FakePackageBasedStorageWriter : PackageBasedScanStorageWriter {
 }
 
 private class FakeProvenanceBasedStorageWriter : ProvenanceBasedScanStorageWriter {
-    override fun write(scanResult: ScanResult) = Unit
+    override fun write(scanResult: ScanResult) = true
 }
 
 private fun createContext(labels: Map<String, String> = emptyMap(), type: PackageType = PackageType.PACKAGE) =