Migrate the Opossum reporter to KxS #9484

alexzurbonsen · 2024-11-21T18:16:29Z

[FOR REVIEW]: Best reviewed commit by commit. First commit of this PR contains the actual changes. Second commit just moves code around to improve readability.

Context

Closes #8839

Summary

Migrates the OpossumReporter plugin away from Jackson serialization to use kotlinx serialization.

To get properly typed data structures for serialization the previous untyped Map<*, *>s are substituted by properly typed data classes. This leads to the creation of an OpossumInputCreator class that holds intermediate data structures needed for processing of the Reporter input.

Test Plan

Automated tests:

OpossumReporterTest
OpossumReporterFunTest

Manual test: Compare two opossum files with ORT builds from main and this branch

repeat for each branch:

./gradlew installDist
cli/build/install/ort/bin/ort analyze -i <INPUT_PROJECT> -o ./RESULT -f JSON 
cli/build/install/ort/bin/ort report -i /path/to/analyzer-result.json -o ./RESULT -f Opossum

Unzip report.opossum files and validate the input.json s with this little python script:
https://gist.github.com/alexzurbonsen/69e38586dcffbb6b988b010bbd096759

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

codecov · 2024-11-21T19:13:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.96%. Comparing base (1f76243) to head (df473e8).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #9484   +/-   ##
=========================================
  Coverage     67.96%   67.96%           
  Complexity     1290     1290           
=========================================
  Files           249      249           
  Lines          8810     8810           
  Branches        915      915           
=========================================
  Hits           5988     5988           
  Misses         2433     2433           
  Partials        389      389

Flag	Coverage Δ
funTest-docker	`64.82% <ø> (ø)`
funTest-non-docker	`33.25% <ø> (ø)`

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.

alexzurbonsen · 2024-11-22T18:13:41Z

plugins/reporters/opossum/src/test/kotlin/OpossumReporterTest.kt

-            opossumInput.packageToRoot.values.forAll { levelForPath ->
-                levelForPath.keys.forAll { path ->
-                    fileList shouldContain resolvePath(path)
-                }
-            }


Deleting this because otherwise I would have to write a separate test for OpossumInputCreator (and make the property packageToRoot non private, I guess).

But it seems like this test does not add much value. The same file paths are in resourcesToAttributions, via addSignal and later processing of pathToSignals. And we have a test for paths in resourcesToAttributions against the same list.

alexzurbonsen · 2024-11-22T18:18:19Z

Hi @sschuberth, it is ready for review now. Let me know what you think. Removed the untyped maps.

Serialization of the recursive data structure OpossumResources was a bit tricky and I decided to not support deserialization. I assume there is no use case for it anyways.

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

sschuberth · 2024-11-22T18:48:38Z

plugins/reporters/opossum/build.gradle.kts

@@ -20,6 +20,9 @@
 plugins {
    // Apply precompiled plugins.
    id("ort-plugin-conventions")
+


Commit message nits:

We prefer imperative mood, i.e. "Migrates" should say "Migrate".

We usually start capitalized after ":" in the title.

The commit message body still uses simple present instead of imperative mood.

sschuberth · 2024-11-22T19:06:43Z

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

-        val followUp: Boolean = false,
-        val excludeFromNotice: Boolean = false,
-        val uuid: UUID = UUID.randomUUID()
+        @Transient


Is this a left-over from not having an UUID serializer yet?

No, but it can be solved differently, I realized. I removed the property and collect the signals in a map uuidToSignals now. We could also collect them in a map signalsToUUID but that would require to replace the current matches method by overriding the equals and hashCode methods, I suppose. Would you prefer that? For now I didn't do it in the interest of keeping the diff smaller.

but that would require to replace the current matches method by overriding the equals and hashCode methods, I suppose. Would you prefer that?

I guess not. In Kotlin, I try to avoid overriding equals and hashCode as much as possible in favor of auto-generate code.

But couldn't @Transient then simply have been dropped and the original code be kept? Anyway, go for whatever causes less work for you.

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

sschuberth · 2024-11-22T19:10:27Z

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

+                    preSelected = preSelected,
+                    followUp = OpossumFollowUp.FOLLOW_UP.takeIf { followUp },
+                    excludeFromNotice = excludeFromNotice,
+                    comment = comment,


Nit: Please avoid trailing commas.

Same in other places.

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

plugins/reporters/opossum/src/test/kotlin/OpossumReporterTest.kt

sschuberth · 2024-11-25T10:04:14Z

PS: @alexzurbonsen please also rebase to resolve conflicts.

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt

alexzurbonsen · 2024-11-25T12:41:04Z

Sorry for the detekt noise. Should be fixed now, I hope.

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt


-            addResource(pathPieces)
-        }
+    internal fun createOpossumInput(input: ReporterInput, maxDepth: Int = Int.MAX_VALUE): OpossumInput =


alexzurbonsen · 2024-11-25T14:11:17Z

Fixed the failing workflow. PR is ready for review again.

plugins/reporters/opossum/src/funTest/kotlin/OpossumReporterFunTest.kt

sschuberth · 2024-11-25T14:38:37Z

Fixed the failing workflow. PR is ready for review again.

Thanks a lot @alexzurbonsen! There are a bunch of more things I'd like to address, e.g. regarding the Git history as we do not allow catch-all "Add review feedback" commit but want the review comments address in the commit the review comments was made on.

But I'll address these myself and take over your branch. Thanks again for all your work!

alexzurbonsen · 2024-11-25T18:24:29Z

@sschuberth Okay, thank you! If I can be of any help, let me know.

I just saw that now detekt is failing on the functional test. May be it didn't get there before. But the fix is easy at least.

Signed-off-by: Sebastian Schuberth <[email protected]>

Migrate the OpossumReporter plugin away from Jackson serialization to use kotlinx-serialization. To get properly typed data structures for serialization the previous untyped `Map<*, *>` are substituted by properly typed data classes. This leads to the creation of an `OpossumInputCreator` class that holds intermediate data structures needed for processing of the Reporter input. Signed-off-by: alexzurbonsen <[email protected]> Signed-off-by: Sebastian Schuberth <[email protected]>

sschuberth · 2024-11-25T14:27:53Z

plugins/reporters/opossum/build.gradle.kts

@@ -20,6 +20,9 @@
 plugins {
    // Apply precompiled plugins.
    id("ort-plugin-conventions")
+


The commit message body still uses simple present instead of imperative mood.

sschuberth · 2024-11-25T14:28:36Z

plugins/reporters/opossum/src/funTest/kotlin/OpossumReporterFunTest.kt

 import io.kotest.matchers.shouldBe
 import io.kotest.matchers.string.shouldContain
+import kotlinx.serialization.json.jsonArray


This needs an empty line before to group top-level imports. (Our custom Detekt rule would complain.)

plugins/reporters/opossum/src/main/kotlin/OpossumModel.kt

+    val baseUrlsForSources: Map<String, String>,
+    val externalAttributionSources: Map<String, OpossumExternalAttributionSource>
+) {
+    fun getSignalsForFile(file: String): List<OpossumSignalFlat> =


plugins/reporters/opossum/src/main/kotlin/OpossumModel.kt

+internal data class OpossumResources(
+    val tree: MutableMap<String, OpossumResources> = mutableMapOf()
+) {
+    fun addResource(pathPieces: List<String>) {


plugins/reporters/opossum/src/main/kotlin/OpossumModel.kt

+        return isPathAFile(pathPieces)
+    }
+
+    fun isPathAFile(pathPieces: List<String>): Boolean {


plugins/reporters/opossum/src/main/kotlin/OpossumModel.kt

+        return head !in tree || tree.getValue(head).isPathAFile(tail)
+    }
+
+    fun toFileList(): Set<String> =


plugins/reporters/opossum/src/main/kotlin/OpossumModel.kt

+        return head !in tree || tree.getValue(head).isPathAFile(tail)
+    }
+
+    fun toFileList(): Set<String> =


alexzurbonsen · 2024-11-25T21:57:04Z

Thanks for finishing this. I like the idea of extracting the data model and creating a full functional test.

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch 3 times, most recently from 7b1a948 to b3aefdb Compare November 21, 2024 18:26

github-advanced-security bot found potential problems Nov 21, 2024

View reviewed changes

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt Fixed Show fixed Hide fixed

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch from b3aefdb to 98ee401 Compare November 21, 2024 19:50

alexzurbonsen changed the title ~~chore(opossum reporter): migrate serialization to kotlinx~~ chore(opossum reporter): Migrate serialization to kotlinx Nov 21, 2024

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch 2 times, most recently from 91b1609 to 4bf27e2 Compare November 22, 2024 18:04

alexzurbonsen commented Nov 22, 2024

View reviewed changes

alexzurbonsen marked this pull request as ready for review November 22, 2024 18:16

alexzurbonsen requested a review from a team as a code owner November 22, 2024 18:16

alexzurbonsen commented Nov 22, 2024

View reviewed changes

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt Outdated Show resolved Hide resolved

sschuberth requested changes Nov 22, 2024

View reviewed changes

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch from 4bf27e2 to abcdc42 Compare November 25, 2024 10:26

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

plugins/reporters/opossum/src/main/kotlin/OpossumReporter.kt Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch from 3fe68ab to 503fd35 Compare November 25, 2024 12:39

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

alexzurbonsen force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch from 503fd35 to cba08df Compare November 25, 2024 14:10

alexzurbonsen requested a review from sschuberth November 25, 2024 14:11

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

plugins/reporters/opossum/src/funTest/kotlin/OpossumReporterFunTest.kt Fixed Show fixed Hide fixed

plugins/reporters/opossum/src/funTest/kotlin/OpossumReporterFunTest.kt Fixed Show fixed Hide fixed

test(opossum): Rewrite the funTest to compare against an expected result

2762981

Signed-off-by: Sebastian Schuberth <[email protected]>

sschuberth force-pushed the migrate-opossum-reporter-to-kotlinx-serialization branch from cba08df to df473e8 Compare November 25, 2024 20:04

sschuberth approved these changes Nov 25, 2024

View reviewed changes

sschuberth enabled auto-merge (rebase) November 25, 2024 20:07

sschuberth changed the title ~~chore(opossum reporter): Migrate serialization to kotlinx~~ Migrate the Opossum reporter to KxS Nov 25, 2024

github-advanced-security bot found potential problems Nov 25, 2024

View reviewed changes

sschuberth merged commit 99e611b into oss-review-toolkit:main Nov 25, 2024
21 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate the Opossum reporter to KxS #9484

Migrate the Opossum reporter to KxS #9484

alexzurbonsen commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

alexzurbonsen Nov 22, 2024

alexzurbonsen commented Nov 22, 2024

sschuberth Nov 22, 2024

sschuberth Nov 25, 2024

sschuberth Nov 22, 2024

alexzurbonsen Nov 25, 2024

sschuberth Nov 25, 2024

sschuberth Nov 22, 2024

sschuberth Nov 22, 2024

sschuberth commented Nov 25, 2024

alexzurbonsen commented Nov 25, 2024

alexzurbonsen commented Nov 25, 2024

sschuberth commented Nov 25, 2024 •

edited

Loading

alexzurbonsen commented Nov 25, 2024

sschuberth Nov 25, 2024

sschuberth Nov 25, 2024

alexzurbonsen commented Nov 25, 2024

Migrate the Opossum reporter to KxS #9484

Migrate the Opossum reporter to KxS #9484

Conversation

alexzurbonsen commented Nov 21, 2024 • edited Loading

codecov bot commented Nov 21, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

alexzurbonsen commented Nov 22, 2024

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

sschuberth commented Nov 25, 2024

alexzurbonsen commented Nov 25, 2024

alexzurbonsen commented Nov 25, 2024

sschuberth commented Nov 25, 2024 • edited Loading

alexzurbonsen commented Nov 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexzurbonsen commented Nov 25, 2024

alexzurbonsen commented Nov 21, 2024 •

edited

Loading

codecov bot commented Nov 21, 2024 •

edited

Loading

sschuberth commented Nov 25, 2024 •

edited

Loading