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.

Sign up for GitHub

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

Jump to bottom

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f2a0eb
test(model): Make formatting of `fragments` construction consistent
fviernau May 24, 2024
87fd707
refactor(model): Use named arguments in a couple of constructor calls
fviernau May 24, 2024
9ece42c
refactor(model): Stop using `SortedSet` for `scopeRoots`
fviernau May 24, 2024
5d8f9d9
refactor(model): Stop implementing Comparable with DependencyReference
fviernau May 24, 2024
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
23 changes: 11 additions & 12 deletions analyzer/src/test/kotlin/AnalyzerResultBuilderTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,18 +103,17 @@ class AnalyzerResultBuilderTest : WordSpec() {
DependencyGraph.qualifyScope(project2, "scope-3") to listOf(RootDependencyIndex(0))
)

private val graph1 =
DependencyGraph(
dependencies1,
sortedSetOf(DependencyGraph.DEPENDENCY_REFERENCE_COMPARATOR, depRef1, depRef2),
scopeMapping1
)
private val graph2 =
DependencyGraph(
dependencies2,
sortedSetOf(DependencyGraph.DEPENDENCY_REFERENCE_COMPARATOR, depRef3),
scopeMapping2
)
private val graph1 = DependencyGraph(
packages = dependencies1,
scopeRoots = setOf(depRef1, depRef2),
sschuberth marked this conversation as resolved.
Show resolved Hide resolved
scopes = scopeMapping1
)

private val graph2 = DependencyGraph(
packages = dependencies2,
scopeRoots = setOf(depRef3),
scopes = scopeMapping2
)

private val analyzerResult1 = ProjectAnalyzerResult(
project1, setOf(package1), listOf(issue3, issue4)
Expand Down
25 changes: 4 additions & 21 deletions model/src/main/kotlin/DependencyGraph.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,6 @@ import com.fasterxml.jackson.annotation.JsonIgnore
import com.fasterxml.jackson.annotation.JsonInclude
import com.fasterxml.jackson.databind.annotation.JsonSerialize

import java.util.SortedSet

import org.ossreviewtoolkit.model.utils.DependencyGraphEdgeSortedSetConverter
import org.ossreviewtoolkit.model.utils.DependencyReferenceSortedSetConverter
import org.ossreviewtoolkit.model.utils.PackageLinkageValueFilter
Expand Down Expand Up @@ -86,7 +84,8 @@ data class DependencyGraph(
* declared by scopes that cannot be reached via other paths in the dependency graph. Note that this property
* exists for backwards compatibility only; it is replaced by the lists of nodes and edges.
*/
val scopeRoots: SortedSet<DependencyReference> = sortedSetOf(),
@JsonSerialize(converter = DependencyReferenceSortedSetConverter::class)
val scopeRoots: Set<DependencyReference> = emptySet(),

/**
* A mapping from scope names to the direct dependencies of the scopes. Based on this information, the set of
Expand All @@ -109,12 +108,6 @@ data class DependencyGraph(
val edges: Set<DependencyGraphEdge>? = null
) {
companion object {
/**
* A comparator for [DependencyReference] objects. Note that the concrete order does not really matter, it
* just has to be well-defined.
*/
val DEPENDENCY_REFERENCE_COMPARATOR = compareBy<DependencyReference>({ it.pkg }, { it.fragment })

/**
* Return a name for the given [scope][scopeName] that is qualified with parts of the identifier of the given
* [project]. This is used to ensure that the scope names are unique when constructing a dependency graph from
Expand Down Expand Up @@ -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(
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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?)

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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?

/**
* Stores the numeric index of the package dependency referenced by this object. The package behind this index can
* be resolved by evaluating the list of identifiers stored in [DependencyGraph] at this index.
Expand Down Expand Up @@ -333,17 +326,7 @@ class DependencyReference(
* A list of [Issue]s that occurred handling this dependency.
*/
val issues: List<Issue> = emptyList()
) : Comparable<DependencyReference> {
/**
* Define an order on [DependencyReference] instances. Instances are ordered by their indices and fragment indices.
*/
override fun compareTo(other: DependencyReference): Int =
if (pkg != other.pkg) {
pkg - other.pkg
} else {
fragment - other.fragment
}
}
)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

"They were isomorphic, but not identical"

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

Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* A data class representing a node in the dependency graph.
Expand Down
9 changes: 4 additions & 5 deletions model/src/main/kotlin/utils/DependencyGraphBuilder.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,11 +171,10 @@ class DependencyGraphBuilder<D>(
val (nodes, edges) = references.toGraph(indexMapping)

return DependencyGraph(
sortedDependencyIds,
sortedSetOf(),
constructSortedScopeMappings(scopeMapping, indexMapping),
nodes,
edges.removeCycles()
packages = sortedDependencyIds,
scopes = constructSortedScopeMappings(scopeMapping, indexMapping),
nodes = nodes,
edges = edges.removeCycles()
)
}

Expand Down
35 changes: 19 additions & 16 deletions model/src/test/kotlin/DependencyGraphTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,11 @@ class DependencyGraphTest : WordSpec({
id("org.apache.commons", "commons-collections4", "4.4"),
id(group = "org.junit", artifact = "junit", version = "5")
)
val fragments =
sortedSetOf(
DependencyReference(0),
DependencyReference(1),
DependencyReference(2)
)
val fragments = setOf(
DependencyReference(0),
DependencyReference(1),
DependencyReference(2)
)
val scopeMap = mapOf(
"p1:scope1" to listOf(RootDependencyIndex(0), RootDependencyIndex(1)),
"p2:scope2" to listOf(RootDependencyIndex(1), RootDependencyIndex(2))
Expand All @@ -63,12 +62,11 @@ class DependencyGraphTest : WordSpec({
id("org.apache.commons", "commons-collections4", "4.4"),
id("org.junit", "junit", "5")
)
val fragments =
sortedSetOf(
DependencyReference(0),
DependencyReference(1),
DependencyReference(2)
)
val fragments = setOf(
DependencyReference(0),
DependencyReference(1),
DependencyReference(2)
)
val scopeMap = mapOf(
qualifiedScopeName to listOf(RootDependencyIndex(0), RootDependencyIndex(1)),
DependencyGraph.qualifyScope(qualifier, "scope2") to listOf(
Expand All @@ -95,7 +93,7 @@ class DependencyGraphTest : WordSpec({
val refCollections = DependencyReference(1)
val refConfig = DependencyReference(2, dependencies = setOf(refLang, refCollections))
val refCsv = DependencyReference(3, dependencies = setOf(refConfig))
val fragments = sortedSetOf(DependencyGraph.DEPENDENCY_REFERENCE_COMPARATOR, refCsv)
val fragments = setOf(refCsv)
val scopeMap = mapOf("s" to listOf(RootDependencyIndex(3)))
val graph = DependencyGraph(ids, fragments, scopeMap)
val scopes = graph.createScopes()
Expand All @@ -117,7 +115,7 @@ class DependencyGraphTest : WordSpec({
val refConfig1 = DependencyReference(2, dependencies = setOf(refLang, refCollections1))
val refConfig2 =
DependencyReference(2, fragment = 1, dependencies = setOf(refLang, refCollections2))
val fragments = sortedSetOf(refConfig1, refConfig2)
val fragments = setOf(refConfig1, refConfig2)
val scopeMap = mapOf(
"s1" to listOf(RootDependencyIndex(2)),
"s2" to listOf(RootDependencyIndex(2, fragment = 1))
Expand Down Expand Up @@ -156,7 +154,12 @@ class DependencyGraphTest : WordSpec({
"s2" to listOf(RootDependencyIndex(2, fragment = 1))
)

val graph = DependencyGraph(ids, sortedSetOf(), scopeMap, nodes, edges)
val graph = DependencyGraph(
packages = ids,
scopes = scopeMap,
nodes = nodes,
edges = edges
)
val scopes = graph.createScopes()

scopeDependencies(scopes, "s1") shouldBe "${ids[2]}<${ids[1]}${ids[0]}>"
Expand All @@ -171,7 +174,7 @@ class DependencyGraphTest : WordSpec({
val issue = Issue(source = "analyzer", message = "Could not analyze :-(")
val refLang = DependencyReference(0, linkage = PackageLinkage.PROJECT_DYNAMIC)
val refCol = DependencyReference(1, issues = listOf(issue), dependencies = setOf(refLang))
val trees = sortedSetOf(refCol)
val trees = setOf(refCol)
val scopeMap = mapOf("s" to listOf(RootDependencyIndex(1)))

val graph = DependencyGraph(ids, trees, scopeMap)
Expand Down
6 changes: 5 additions & 1 deletion model/src/test/kotlin/ProjectTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,11 @@ private fun createDependencyGraph(qualified: Boolean = false): DependencyGraph {
plainScopeMapping
}

return DependencyGraph(dependencies, sortedSetOf(exampleRef, csvRef), scopeMapping)
return DependencyGraph(
packages = dependencies,
scopeRoots = setOf(exampleRef, csvRef),
scopes = scopeMapping
)
}

class ProjectTest : WordSpec({
Expand Down
Loading