From c5bb8c609e76d12f5e2c9a93ebc739413f6c60f8 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Sun, 31 May 2026 18:01:57 +0200 Subject: [PATCH] Bug: fix new PCR as it in certain cases left duplicates The was spotted by Quarkus plugin that was building own "app graph" and bellied up because of discovered duplicated in graph. --- .../transformer/PathConflictResolver.java | 50 ++++-- .../transformer/ConflictResolverTest.java | 163 +++++++++++++++++- 2 files changed, 199 insertions(+), 14 deletions(-) diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java index 985106a0b0..2c6fa43af6 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java @@ -22,6 +22,7 @@ import java.util.Collection; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -164,6 +165,14 @@ public DependencyNode transformGraph(DependencyNode node, DependencyGraphTransfo throw new RepositoryException("conflict groups have not been identified"); } + Map> cyclicPredecessors = new HashMap<>(); + for (Collection cycle : conflictIdCycles) { + for (String conflictId : cycle) { + Collection predecessors = cyclicPredecessors.computeIfAbsent(conflictId, k -> new HashSet<>()); + predecessors.addAll(cycle); + } + } + State state = new State( ConflictResolver.getVerbosity(context.getSession()), ConfigUtils.getBoolean( @@ -174,9 +183,9 @@ public DependencyNode transformGraph(DependencyNode node, DependencyGraphTransfo scopeSelector.getInstance(node, context), scopeDeriver.getInstance(node, context), optionalitySelector.getInstance(node, context), - conflictIds); - - state.build(node); + cyclicPredecessors, + conflictIds, + node); // loop over topographically sorted conflictIds for (String conflictId : sortedConflictIds) { @@ -277,6 +286,11 @@ private static class State { */ private final ConflictResolver.OptionalitySelector optionalitySelector; + /** + * The conflict predecessors by conflictId. + */ + private final Map> cyclicPredecessors; + /** * The node to conflictId mapping from {@link ConflictMarker}. */ @@ -293,6 +307,11 @@ private static class State { */ private final Map resolvedIds; + /** + * The root {@link Path}. + */ + private final Path root; + @SuppressWarnings("checkstyle:ParameterNumber") private State( ConflictResolver.Verbosity verbosity, @@ -301,16 +320,21 @@ private State( ConflictResolver.ScopeSelector scopeSelector, ConflictResolver.ScopeDeriver scopeDeriver, ConflictResolver.OptionalitySelector optionalitySelector, - Map conflictIds) { + Map> cyclicPredecessors, + Map conflictIds, + DependencyNode node) + throws RepositoryException { this.verbosity = verbosity; this.showCyclesInStandardVerbosity = showCyclesInStandardVerbosity; this.versionSelector = versionSelector; this.scopeSelector = scopeSelector; this.scopeDeriver = scopeDeriver; this.optionalitySelector = optionalitySelector; + this.cyclicPredecessors = cyclicPredecessors; this.conflictIds = conflictIds; this.partitions = new HashMap<>(); this.resolvedIds = new HashMap<>(); + this.root = build(node); } /** @@ -318,9 +342,10 @@ private State( * tree. As a side effect, {@link #partitions} are being filled up as well, that combined with topo * sorted conflictIds can serve as a starting to point to walk the graph. */ - private void build(DependencyNode node) throws RepositoryException { + private Path build(DependencyNode node) throws RepositoryException { Path root = new Path(this, node, null, false); gatherCRNodes(root); + return root; } /** @@ -332,9 +357,7 @@ private void gatherCRNodes(Path node) throws RepositoryException { // add children; we will get back those really added (not causing cycles) List added = node.addChildren(children); for (Path child : added) { - if (!child.cycle) { - gatherCRNodes(child); - } + gatherCRNodes(child); } } } @@ -615,13 +638,16 @@ private void moveOutOfScope() { private List addChildren(List children) throws RepositoryException { ArrayList added = new ArrayList<>(children.size()); for (DependencyNode child : children) { - String childConflictId = this.state.conflictIds.get(child); - boolean cycle = this.state.partitions.getOrDefault(childConflictId, Collections.emptyList()).stream() - .anyMatch(p -> p.dn.equals(child)); + 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); this.children.add(c); c.derive(0, false); - added.add(c); + if (c.parent != null && !c.parent.cycle) { + added.add(c); + } } return added; } diff --git a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java index 9d8fca505f..2f1b801303 100644 --- a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java +++ b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java @@ -21,6 +21,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.concurrent.atomic.AtomicInteger; import java.util.stream.Stream; import org.eclipse.aether.RepositoryException; @@ -28,10 +29,12 @@ import org.eclipse.aether.graph.DefaultDependencyNode; import org.eclipse.aether.graph.Dependency; import org.eclipse.aether.graph.DependencyNode; +import org.eclipse.aether.graph.DependencyVisitor; import org.eclipse.aether.internal.test.util.DependencyGraphParser; import org.eclipse.aether.internal.test.util.TestVersion; import org.eclipse.aether.internal.test.util.TestVersionConstraint; import org.eclipse.aether.util.graph.visitor.DependencyGraphDumper; +import org.eclipse.aether.util.graph.visitor.TreeDependencyVisitor; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.Arguments; import org.junit.jupiter.params.provider.MethodSource; @@ -483,6 +486,8 @@ void winnerCycleRemoved(ConflictResolver conflictResolver) throws RepositoryExce System.out.println("CR=" + conflictResolver.getClass().getSimpleName() + "; verbosity=" + verbosity.name()); if (!cyclesLeftInPlace) { transformed.accept(DUMPER_SOUT); + } else { + transformed.accept(new TreeDependencyVisitor(DUMPER_SOUT)); } assertSame(transformed, root); @@ -522,13 +527,167 @@ void winnerCycleRemoved(ConflictResolver conflictResolver) throws RepositoryExce } } + @ParameterizedTest + @MethodSource("conflictResolverSource") + void subtreeRepeatedSameInstance(ConflictResolver conflictResolver) throws RepositoryException { + // root -> d1 ----> st1* -> st2 -> st3 + // |---> st1* --> st2 --> st3 + // |-----> st3 + // Note: st1* are TWO DIFFERENT instances, but st2->st3 are same instances on different paths + DependencyNode root = makeDependencyNode("some-group", "root", "1.0"); + DependencyNode d1 = makeDependencyNode("some-group", "d1", "1.0"); + DependencyNode st1a = makeDependencyNode("some-group", "st1", "1.0"); + DependencyNode st1b = makeDependencyNode("some-group", "st1", "1.0"); + DependencyNode st2 = makeDependencyNode("some-group", "st2", "1.0"); + DependencyNode st3 = makeDependencyNode("some-group", "st3", "1.0"); + + root.setChildren(mutableList(d1, st1b)); + d1.setChildren(mutableList(st1a)); + st1a.setChildren(mutableList(st2)); + st2.setChildren(mutableList(st3)); + st1b.setChildren(mutableList(st2, st3)); + + System.out.println(); + System.out.println("Input node:"); + root.accept(DUMPER_SOUT); + + DependencyNode transformedRoot = transform(conflictResolver, root); + + System.out.println(); + System.out.println("Transformed node:"); + transformedRoot.accept(DUMPER_SOUT); + + assertSame(transformedRoot, root); + AtomicInteger occurrenceOfSt3 = new AtomicInteger(0); + root.accept(new DependencyVisitor() { + @Override + public boolean visitEnter(DependencyNode node) { + if (node.getArtifact() != null + && node.getArtifact().getArtifactId().equals("st3")) { + occurrenceOfSt3.incrementAndGet(); + } + return true; + } + + @Override + public boolean visitLeave(DependencyNode node) { + return true; + } + }); + assertEquals(1, occurrenceOfSt3.get()); // st3 must be present only once in tree + } + + @ParameterizedTest + @MethodSource("conflictResolverSource") + void subtreeRepeatedSameInstanceWithRealCycle(ConflictResolver conflictResolver) throws RepositoryException { + // root -> d1 ----> st1* -> st2 -> st3 + // |---> st1* --> st2 --> st3* -> st2 + // |-----> st3 + // Note: st1* are TWO DIFFERENT instances, but st2->st3 are same instances on different paths + // Note: st3* are TWO DIFFERENT instances + DependencyNode root = makeDependencyNode("some-group", "root", "1.0"); + DependencyNode d1 = makeDependencyNode("some-group", "d1", "1.0"); + DependencyNode st1a = makeDependencyNode("some-group", "st1", "1.0"); + DependencyNode st1b = makeDependencyNode("some-group", "st1", "1.0"); + DependencyNode st2 = makeDependencyNode("some-group", "st2", "1.0"); + DependencyNode st3 = makeDependencyNode("some-group", "st3", "1.0"); + DependencyNode st3a = makeDependencyNode("some-group", "st3", "1.0"); + + root.setChildren(mutableList(d1, st1b)); + d1.setChildren(mutableList(st1a)); + st1a.setChildren(mutableList(st2)); + st2.setChildren(mutableList(st3a)); + st1b.setChildren(mutableList(st2, st3)); + st3a.setChildren(mutableList(st2)); + + System.out.println(); + System.out.println("Input node:"); + root.accept(new TreeDependencyVisitor(DUMPER_SOUT)); // we have a cycle, so must wrap it + + DependencyNode transformedRoot = transform(conflictResolver, root); + + System.out.println(); + System.out.println("Transformed node:"); + // transformedRoot.accept(DUMPER_SOUT); + + assertSame(transformedRoot, root); + AtomicInteger occurrenceOfSt3 = new AtomicInteger(0); + root.accept(new DependencyVisitor() { + @Override + public boolean visitEnter(DependencyNode node) { + if (node.getArtifact() != null + && node.getArtifact().getArtifactId().equals("st3")) { + occurrenceOfSt3.incrementAndGet(); + } + return true; + } + + @Override + public boolean visitLeave(DependencyNode node) { + return true; + } + }); + assertEquals(1, occurrenceOfSt3.get()); // st3 must be present only once in tree + } + + @ParameterizedTest + @MethodSource("conflictResolverSource") + void nettyBoringSslExample(ConflictResolver conflictResolver) throws RepositoryException { + // root -> nbc1 ---> nbc2 + // |---> nbc2 ---> nbc1 + DependencyNode root = makeDependencyNode("some-group", "root", "1.0"); + DependencyNode nbc1 = makeDependencyNode("some-group", "nbc", "1.0", "arch1", "compile"); + DependencyNode nbc2 = makeDependencyNode("some-group", "nbc", "1.0", "arch2", "compile"); + + root.setChildren(mutableList(nbc1, nbc2)); + nbc1.setChildren(mutableList(nbc2)); + nbc2.setChildren(mutableList(nbc1)); + + System.out.println(); + System.out.println("Input node:"); + root.accept(new TreeDependencyVisitor(DUMPER_SOUT)); // we have a cycle, so must wrap it + + DependencyNode transformedRoot = transform(conflictResolver, root); + + System.out.println(); + System.out.println("Transformed node:"); + transformedRoot.accept(DUMPER_SOUT); + + assertSame(transformedRoot, root); + AtomicInteger occurrence = new AtomicInteger(0); + root.accept(new DependencyVisitor() { + @Override + public boolean visitEnter(DependencyNode node) { + if (node.getArtifact() != null + && node.getArtifact().getArtifactId().equals("nbc")) { + occurrence.incrementAndGet(); + } + return true; + } + + @Override + public boolean visitLeave(DependencyNode node) { + return true; + } + }); + assertEquals(2, occurrence.get()); // st3 must be present only once in tree + } + private static DependencyNode makeDependencyNode(String groupId, String artifactId, String version) { return makeDependencyNode(groupId, artifactId, version, "compile"); } private static DependencyNode makeDependencyNode(String groupId, String artifactId, String version, String scope) { - DefaultDependencyNode node = new DefaultDependencyNode( - new Dependency(new DefaultArtifact(groupId + ':' + artifactId + ':' + version), scope)); + return makeDependencyNode(groupId, artifactId, version, null, "compile"); + } + + private static DependencyNode makeDependencyNode( + String groupId, String artifactId, String version, String classifier, String scope) { + DefaultDependencyNode node = (classifier != null && !classifier.isEmpty()) + ? new DefaultDependencyNode(new Dependency( + new DefaultArtifact(groupId + ':' + artifactId + ":jar:" + classifier + ":" + version), scope)) + : new DefaultDependencyNode( + new Dependency(new DefaultArtifact(groupId + ':' + artifactId + ':' + version), scope)); node.setVersion(new TestVersion(version)); node.setVersionConstraint(new TestVersionConstraint(node.getVersion())); return node;