gnodet commented on code in PR #1904:
URL: https://github.com/apache/maven-resolver/pull/1904#discussion_r3372069555
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -343,7 +319,8 @@ private State(
* sorted conflictIds can serve as a starting to point to walk the
graph.
*/
private Path build(DependencyNode node) throws RepositoryException {
- Path root = new Path(this, node, null, false);
+ String nodeConflictId = this.conflictIds.get(node);
+ Path root = new Path(this, node, nodeConflictId,
Collections.singletonList(nodeConflictId), null);
Review Comment:
Worth noting: for a virtual root node (one with `getDependency() == null`),
`conflictIds.get(node)` returns `null` because `ConflictMarker` only puts nodes
with non-null dependencies into the map. This means `conflictIdPath` starts as
`[null]`.
This is safe in practice — `newPath.contains(childConflictId)` in
`addChildren()` won't match any real (non-null) child conflict ID against
`null`, so no false cycle detection. And adding `null` to `partitions` is the
same behavior as the old constructor (`this.conflictId =
state.conflictIds.get(dn)`). So no regression.
Still, if you wanted to be defensive, you could guard against it:
```suggestion
String nodeConflictId = this.conflictIds.get(node);
Path root = new Path(this, node, nodeConflictId,
nodeConflictId != null ?
Collections.singletonList(nodeConflictId) : Collections.emptyList(),
null);
```
This avoids polluting `conflictIdPath` with `null` for virtual roots.
Non-blocking — the current code works correctly either way.
##########
maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/ResolveTransitiveDependencies.java:
##########
@@ -50,7 +50,10 @@ public static void main(String[] args) throws Exception {
try (RepositorySystem system =
Booter.newRepositorySystem(Booter.selectFactory(args));
CloseableSession session =
Booter.newRepositorySystemSession(system, Booter.selectFs(args))
.build()) {
- Artifact artifact = new
DefaultArtifact("org.apache.maven.resolver:maven-resolver-impl:1.3.3");
+ // An "interesting" artifact that depends on itself (different
version). All Maven versions
+ // released so far contained dom4j:dom4j:1.5.2 on classpath for
dom4j:dom4j:1.6.1
+ // In Resolver 2.0.19 this is fixed.
Review Comment:
Minor: the comment says "In Resolver 2.0.19 this is fixed" — should this
reference the version that will actually include this fix? If 2.0.19 hasn't
been released yet, it might be worth saying "In Resolver 2.0.19+" or just
leaving it as a general statement. Also, is this the right version number, or
should it track whatever the next release cut will be?
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java:
##########
@@ -633,19 +613,23 @@ private void moveOutOfScope() {
/**
* Adds node children: this method should be "batch" used, as all
(potential) children should be added at once.
- * Method will return really added ones, as this class avoids cycles.
+ * Method will return really added ones, as this class avoids cycles.
Those forming a cycle are added to {@link Path}
+ * structure but are not recursed (returned in list), keeping {@link
Path} cycle free.
+ * This method also maintains "conflictIdPath", that is a list of
"conflict partition IDs" leading from root
+ * toward given instance. This implies that this conflict resolver, by
its nature "redoes" the
+ * {@link TransformationContextKeys#CYCLIC_CONFLICT_IDS} calculated by
{@link ConflictIdSorter}.
*/
private List<Path> addChildren(List<DependencyNode> children) throws
RepositoryException {
ArrayList<Path> added = new ArrayList<>(children.size());
for (DependencyNode child : children) {
- boolean cycle = this.state
- .cyclicPredecessors
- .getOrDefault(this.state.conflictIds.get(child),
Collections.emptySet())
- .contains(this.conflictId);
- Path c = new Path(this.state, child, this, cycle);
+ String childConflictId = this.state.conflictIds.get(child);
+ List<String> newPath = new ArrayList<>(this.conflictIdPath);
+ boolean cycle = newPath.contains(childConflictId);
Review Comment:
This is the key improvement over 5e97fd15. `contains()` on an
`ArrayList<String>` is O(depth) per child, making the total cost O(N·D) where N
is node count and D is average depth. For typical Maven dependency trees (depth
< 20), this is negligible.
If you ever wanted to optimize for pathological deep trees, you could pass a
`Set<String>` alongside the list (or instead of it, since order doesn't matter
for cycle detection — only membership). But that's premature optimization for
Maven's use case.
One thought: since `newPath` is a new `ArrayList` copy per child, it becomes
the child's `conflictIdPath` and is never modified after construction. You
could wrap it with `Collections.unmodifiableList(newPath)` in the `Path`
constructor to enforce immutability, but again — not blocking, just a style
preference.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
