Skip to content

Bug: fix new PCR as it in certain cases left duplicates #1896

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

Merged
cstamas merged 1 commit into from
Jun 4, 2026
+199 −14
Merged

Bug: fix new PCR as it in certain cases left duplicates #1896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
50 changes: 38 additions & 12 deletions ...er-util/src/main/java/org/eclipse/aether/util/graph/transformer/PathConflictResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -164,6 +165,14 @@ public DependencyNode transformGraph(DependencyNode node, DependencyGraphTransfo
throw new RepositoryException("conflict groups have not been identified");
}

Map<String, Collection<String>> cyclicPredecessors = new HashMap<>();
for (Collection<String> cycle : conflictIdCycles) {
for (String conflictId : cycle) {
Collection<String> predecessors = cyclicPredecessors.computeIfAbsent(conflictId, k -> new HashSet<>());
predecessors.addAll(cycle);
}
}

State state = new State(
ConflictResolver.getVerbosity(context.getSession()),
ConfigUtils.getBoolean(
Expand All @@ -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) {
Expand Down Expand Up @@ -277,6 +286,11 @@ private static class State {
*/
private final ConflictResolver.OptionalitySelector optionalitySelector;

/**
* The conflict predecessors by conflictId.
*/
private final Map<String, Collection<String>> cyclicPredecessors;

/**
* The node to conflictId mapping from {@link ConflictMarker}.
*/
Expand All @@ -293,6 +307,11 @@ private static class State {
*/
private final Map<String, Path> resolvedIds;

/**
* The root {@link Path}.
*/
private final Path root;

@SuppressWarnings("checkstyle:ParameterNumber")
private State(
ConflictResolver.Verbosity verbosity,
Expand All @@ -301,26 +320,32 @@ private State(
ConflictResolver.ScopeSelector scopeSelector,
ConflictResolver.ScopeDeriver scopeDeriver,
ConflictResolver.OptionalitySelector optionalitySelector,
Map<DependencyNode, String> conflictIds) {
Map<String, Collection<String>> cyclicPredecessors,
Map<DependencyNode, String> 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);
}

/**
* Consumes the dirty graph and builds internal structures out of {@link Path} instances that is always a
* 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;
}

/**
Expand All @@ -332,9 +357,7 @@ private void gatherCRNodes(Path node) throws RepositoryException {
// add children; we will get back those really added (not causing cycles)
List<Path> added = node.addChildren(children);
for (Path child : added) {
if (!child.cycle) {
gatherCRNodes(child);
}
gatherCRNodes(child);
}
}
}
Expand Down Expand Up @@ -615,13 +638,16 @@ private void moveOutOfScope() {
private List<Path> addChildren(List<DependencyNode> children) throws RepositoryException {
ArrayList<Path> 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;
}
Expand Down
163 changes: 161 additions & 2 deletions ...er-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,20 @@
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;
import org.eclipse.aether.artifact.DefaultArtifact;
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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
Expand Down
Loading