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

model: Serialize dependency graph edges in some explicit order #8696

Merged
merged 3 commits into from
May 24, 2024
Merged

model: Serialize dependency graph edges in some explicit order #8696

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f791301
refactor(model): Use sets instead of lists for DependencyGraphEdge
fviernau May 24, 2024
d96de6f
refactor(model): Serialize dependency graph edges in sorted order
fviernau May 24, 2024
e872dcf
test: Re-serialize test assets
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
9 changes: 6 additions & 3 deletions model/src/main/kotlin/DependencyGraph.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,11 @@ package org.ossreviewtoolkit.model

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.PackageLinkageValueFilter

/**
Expand Down Expand Up @@ -99,10 +101,11 @@ data class DependencyGraph(
val nodes: List<DependencyGraphNode>? = null,

/**
* A list with the edges of this dependency graph. By traversing the edges, the dependencies of packages can be
* A set with the edges of this dependency graph. By traversing the edges, the dependencies of packages can be
* determined.
*/
val edges: List<DependencyGraphEdge>? = null
@JsonSerialize(converter = DependencyGraphEdgeSortedSetConverter::class)
val edges: Set<DependencyGraphEdge>? = null
Copy link
Member

Choose a reason for hiding this comment

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

The comment in line 102 should now also say "set" instead of "list". Also, the commit message should probably also mention that order is not important either (even if a set usually maintains insertion order).

) {
companion object {
/**
Expand Down Expand Up @@ -419,7 +422,7 @@ private fun constructNodeDependenciesFromScopeRoots(roots: SortedSet<DependencyR
*/
private fun constructNodeDependenciesFromGraph(
nodes: List<DependencyGraphNode>,
edges: List<DependencyGraphEdge>
edges: Set<DependencyGraphEdge>
): NodeDependencies {
val mapping = mutableMapOf<DependencyGraphNode, MutableList<DependencyGraphNode>>()

Expand Down
8 changes: 4 additions & 4 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 @@ -179,7 +179,7 @@ class DependencyGraphBuilder<D>(
)
}

private fun Collection<DependencyGraphEdge>.removeCycles(): List<DependencyGraphEdge> {
private fun Set<DependencyGraphEdge>.removeCycles(): Set<DependencyGraphEdge> {
Copy link
Member

Choose a reason for hiding this comment

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

In the spirit of "be lenient when consuming, be strict when producing", should the receiver stay a Collection?

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 believe in general it makes sense, in this case I decided against it so that one does not have to even think about what happens when input contains duplicates, because the API is private (small scope), and because there is no use besides the Sets one.

val edges = toMutableSet()
val edgesToKeep = breakCycles(edges)
val edgesToRemove = edges - edgesToKeep
Expand All @@ -188,7 +188,7 @@ class DependencyGraphBuilder<D>(
logger.warn { "Removing edge '${it.from} -> ${it.to}' to break a cycle." }
}

return filter { it in edgesToKeep }
return filterTo(mutableSetOf()) { it in edgesToKeep }
}

private fun checkReferences() {
Expand Down Expand Up @@ -436,9 +436,9 @@ class DependencyGraphBuilder<D>(
*/
private fun Collection<DependencyReference>.toGraph(
indexMapping: IntArray
): Pair<List<DependencyGraphNode>, List<DependencyGraphEdge>> {
): Pair<List<DependencyGraphNode>, Set<DependencyGraphEdge>> {
val nodes = mutableSetOf<DependencyGraphNode>()
val edges = mutableListOf<DependencyGraphEdge>()
val edges = mutableSetOf<DependencyGraphEdge>()
val nodeIndices = mutableMapOf<NodeKey, Int>()

fun getOrAddNodeIndex(ref: DependencyReference): Int =
Expand Down
5 changes: 5 additions & 0 deletions model/src/main/kotlin/utils/SortedSetConverters.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@

import org.ossreviewtoolkit.model.ArtifactProvenance
import org.ossreviewtoolkit.model.CopyrightFinding
import org.ossreviewtoolkit.model.DependencyGraphEdge
import org.ossreviewtoolkit.model.FileList
import org.ossreviewtoolkit.model.Identifier
import org.ossreviewtoolkit.model.LicenseFinding
Expand All @@ -44,6 +45,10 @@
override fun convert(value: Set<CopyrightFinding>) = value.toSortedSet(CopyrightFinding.COMPARATOR)
}

class DependencyGraphEdgeSortedSetConverter : StdConverter<Set<DependencyGraphEdge>, Set<DependencyGraphEdge>>() {
override fun convert(value: Set<DependencyGraphEdge>) = value.toSortedSet(compareBy({ it.from }, { it.to }))

Check warning on line 49 in model/src/main/kotlin/utils/SortedSetConverters.kt

View check run for this annotation

Codecov / codecov/patch

model/src/main/kotlin/utils/SortedSetConverters.kt#L48-L49 

Added lines #L48 - L49 were not covered by tests
}

/** Do not convert to SortedSet in order to not require a comparator consistent with equals */
class FileListSortedSetConverter : StdConverter<Set<FileList>, Set<FileList>>() {
override fun convert(value: Set<FileList>) = value.sortedBy { it.provenance.getSortKey() }.toSet()
Expand Down
16 changes: 8 additions & 8 deletions model/src/test/assets/result-with-issues-graph.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1300,22 +1300,22 @@ analyzer:
to: 1
- from: 0
to: 2
- from: 6
to: 7
- from: 4
to: 5
- from: 4
to: 6
- from: 4
to: 8
- from: 6
to: 7
- from: 9
to: 10
- from: 11
to: 12
- from: 12
to: 13
- from: 12
to: 14
- from: 11
to: 12
- from: 17
to: 18
- from: 19
Expand All @@ -1328,14 +1328,14 @@ analyzer:
to: 24
- from: 25
to: 26
- from: 30
to: 26
- from: 30
to: 31
- from: 28
to: 29
- from: 28
to: 30
- from: 30
to: 26
- from: 30
to: 31
scanner: null
advisor: null
evaluator: null
Expand Down
16 changes: 8 additions & 8 deletions model/src/test/assets/sbt-multi-project-example-graph.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1290,22 +1290,22 @@ analyzer:
to: 1
- from: 0
to: 2
- from: 6
to: 7
- from: 4
to: 5
- from: 4
to: 6
- from: 4
to: 8
- from: 6
to: 7
- from: 9
to: 10
- from: 11
to: 12
- from: 12
to: 13
- from: 12
to: 14
- from: 11
to: 12
- from: 17
to: 18
- from: 19
Expand All @@ -1318,14 +1318,14 @@ analyzer:
to: 24
- from: 25
to: 26
- from: 30
to: 26
- from: 30
to: 31
- from: 28
to: 29
- from: 28
to: 30
- from: 30
to: 26
- from: 30
to: 31
scanner: null
advisor: null
evaluator: null
Expand Down
2 changes: 1 addition & 1 deletion model/src/test/kotlin/DependencyGraphTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class DependencyGraphTest : WordSpec({
val nodeConfig1 = DependencyGraphNode(2)
val nodeConfig2 = DependencyGraphNode(2, fragment = 1)
val nodes = listOf(nodeLogging, nodeLang, nodeCollections1, nodeCollections2, nodeConfig1, nodeConfig2)
val edges = listOf(
val edges = setOf(
DependencyGraphEdge(3, 0),
DependencyGraphEdge(4, 1),
DependencyGraphEdge(4, 2),
Expand Down
Loading
Loading