refactor(model): Stop implementing Comparable with DependencyReference #8697

fviernau · 2024-05-24T16:29:46Z

This is a single commit, based on #8705. This PR has to be merged afterwards.

codecov · 2024-05-24T16:43:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.84%. Comparing base (1765a15) to head (5d8f9d9).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8697      +/-   ##
============================================
- Coverage     67.85%   67.84%   -0.01%     
  Complexity     1165     1165              
============================================
  Files           244      244              
  Lines          7736     7735       -1     
  Branches        865      865              
============================================
- Hits           5249     5248       -1     
  Misses         2128     2128              
  Partials        359      359

Flag	Coverage Δ
funTest-docker	`66.84% <ø> (ø)`
funTest-non-docker	`34.17% <0.00%> (+<0.01%)`	⬆️
test	`38.00% <100.00%> (-0.01%)`	⬇️

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.

analyzer/src/test/kotlin/AnalyzerResultBuilderTest.kt

sschuberth · 2024-05-27T09:29:10Z

model/src/main/kotlin/DependencyGraph.kt

@@ -295,7 +295,7 @@ data class RootDependencyIndex(
 * Note: This is by intention no data class. Equality is tested via references and not via the values contained.
 */
 @JsonInclude(JsonInclude.Include.NON_DEFAULT)
-class DependencyReference(
+data class DependencyReference(


Commit message: Could you please add a permalink to the exact test case? I'm having trouble finding it.

I've also re-written the commit message as the thing with the copy() was not exactly right.

sschuberth · 2024-05-27T09:31:56Z

model/src/main/kotlin/DependencyGraph.kt

-            fragment - other.fragment
-        }
-}
+)


I'm not sure what the original motivation for this compareTo() implementation was... like, was the implementation more or less arbitrary, just to be able to use the class in sorted sets, or was there more to it, like and intention to sort by indices in some representation?

Maybe @oheger-bosch can chime in here to confirm that this change (by now, with the preceding changes) is uncritical?

It's been a while... But as far as I remember, the purpose of the compareTo() implementation was to have a more deterministic order in dependency graphs. During tests, it popped up that the order in which items were added to a graph builder had an impact on the resulting graphs. They were isomorphic, but not identical. By defining an ordering, the effect could be reduced, but not fully eliminated.

"They were isomorphic, but not identical"

Do you remember in what way this could be problematic? Was it non-deterministic without implementing compareTo() ?

I am not 100% sure, but I think for a given order in which the graph builder was called, the results were the same.

oheger-bosch · 2024-05-27T15:32:17Z

model/src/main/kotlin/DependencyGraph.kt

@@ -302,7 +295,7 @@ data class RootDependencyIndex(
 * Note: This is by intention no data class. Equality is tested via references and not via the values contained.
 */
 @JsonInclude(JsonInclude.Include.NON_DEFAULT)
-class DependencyReference(
+data class DependencyReference(


This change makes me worry. The class comment says "This is by intention no data class. Equality is tested via references and not via the values contained." If you now have a generated equals() implementation that checks all properties, you can get very expensive traversals over whole graphs.

Hm, the tests which started failing when stopping to implement Comparable, could it be that they asserted a bit too much? (e.g. is there maybe a fix for this possible in test code?)

Another alternative could be to hold these dependency references always in lists, not sets, to avoid the equals() calls in the first place.

equals() is called when constructing the dependency graph to match subtrees. This was the reason why this class was not a data class. I am pretty sure that this change will blow up for bigger graphs. Did you test this with a larger project?

fviernau · 2024-05-27T16:59:41Z

@oheger-bosch @sschuberth given above comments, do you have any preference how the top commit should be changed to move this forward?

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

Improve readability, and while at it make use of the default value for scope roots in two of the calls. Signed-off-by: Frank Viernau <[email protected]>

Only sort on serialization for human readability and reproducibility. Also, remove the now unused comparator, which was used only by tests before. Signed-off-by: Frank Viernau <[email protected]>

fviernau · 2024-05-27T17:09:23Z

I've split-out the non-controversial commits to #8705.

The first two test cases of `AnalyzerResultBuilder` [1] compare instances of `DependencyReference`s in their assertion. Without the `Comparable` interface implemented, these assertions fail because the equality is changed to compare whether the instances (memory locations) are the same. So, turn the `DependencyReference` into a data class to fix that. [1]: https://github.com/oss-review-toolkit/ort/blob/41b450b11c5930179016ba1a54d77e03f0c70b4c/analyzer/src/test/kotlin/AnalyzerResultBuilderTest.kt#L128-L154 Signed-off-by: Frank Viernau <[email protected]>

fviernau · 2024-05-27T17:53:15Z

@oheger-bosch another question which just came up to me: Do you recall why it is necessary to have tests for de-serialization and serialization? It seems that no custom serializers are at play, so normally we do no test serialization in that scenario IIUC. So, could it be an option to just drop these serialization tests from AnalyzerResultBuilderTest ?

oheger-bosch · 2024-05-28T06:59:13Z

@oheger-bosch another question which just came up to me: Do you recall why it is necessary to have tests for de-serialization and serialization? It seems that no custom serializers are at play, so normally we do no test serialization in that scenario IIUC. So, could it be an option to just drop these serialization tests from AnalyzerResultBuilderTest ?

I don't recally exactly any more. But I think these tests have been added in the progress of migrating from the old format for dependency trees to the dependency graph format. Since this was a major rewrite, it was useful to have tests in place.

fviernau force-pushed the eliminate-dep-graph-sorted-set-part-2 branch from 8d060bd to 4569367 Compare May 27, 2024 08:48

fviernau marked this pull request as ready for review May 27, 2024 09:12

fviernau requested a review from a team as a code owner May 27, 2024 09:12

fviernau enabled auto-merge (rebase) May 27, 2024 09:12

sschuberth requested changes May 27, 2024

View reviewed changes

fviernau force-pushed the eliminate-dep-graph-sorted-set-part-2 branch from 4569367 to 7fbfc48 Compare May 27, 2024 09:43

fviernau requested a review from sschuberth May 27, 2024 09:45

oheger-bosch reviewed May 27, 2024

View reviewed changes

fviernau added 3 commits May 27, 2024 19:05

test(model): Make formatting of fragments construction consistent

3f2a0eb

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

refactor(model): Use named arguments in a couple of constructor calls

87fd707

Improve readability, and while at it make use of the default value for scope roots in two of the calls. Signed-off-by: Frank Viernau <[email protected]>

refactor(model): Stop using SortedSet for scopeRoots

9ece42c

Only sort on serialization for human readability and reproducibility. Also, remove the now unused comparator, which was used only by tests before. Signed-off-by: Frank Viernau <[email protected]>

fviernau force-pushed the eliminate-dep-graph-sorted-set-part-2 branch from 7fbfc48 to 9ece42c Compare May 27, 2024 17:06

fviernau changed the title ~~model: Remove the remaining uses of SortedSet / SortedMap from classes which get serialized~~ refactor(model): Stop implementing Comparable with DependencyReference May 27, 2024

fviernau marked this pull request as draft May 28, 2024 07:49

auto-merge was automatically disabled May 28, 2024 07:49
Pull request was converted to draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(model): Stop implementing Comparable with DependencyReference #8697

refactor(model): Stop implementing Comparable with DependencyReference #8697

fviernau commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading

sschuberth May 27, 2024

fviernau May 27, 2024

sschuberth May 27, 2024

oheger-bosch May 27, 2024

fviernau May 27, 2024

oheger-bosch May 27, 2024

oheger-bosch May 27, 2024

fviernau May 27, 2024

fviernau May 27, 2024

oheger-bosch May 28, 2024

fviernau commented May 27, 2024

fviernau commented May 27, 2024

fviernau commented May 27, 2024

oheger-bosch commented May 28, 2024

refactor(model): Stop implementing Comparable with DependencyReference #8697

Are you sure you want to change the base?

refactor(model): Stop implementing Comparable with DependencyReference #8697

Conversation

fviernau commented May 24, 2024 • edited Loading

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fviernau commented May 27, 2024

fviernau commented May 27, 2024

fviernau commented May 27, 2024

oheger-bosch commented May 28, 2024

fviernau commented May 24, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading