You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2026/06/07 18:16:45 UTC
[PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
cstamas opened a new pull request, #1904:
URL: https://github.com/apache/maven-resolver/pull/1904
Fixes #1903 bug in `PathConflictResolver` (PCR).
Also, the fix in 5e97fd15f8dbc562c481252bc8ea1037992e6407 is wrong, and this PR redoes now all.
Problem was cycle detection. Original (2.0.18) PCR did left "uncovered" nodes, still causing cycles, that should have been removed by PCR (causing SO in client code). The commit above remedied it, but caused another issue how cycles were detected (it opted to reuse existing `CYCLIC_CONFLICT_IDS`), but was again wrong.
Then I realized: PCR by its design is "rebuilding" tree with `conflictIds` on Path instances, so it can easily spot properly when a cycle is about to happen (when a next node is partitioned into same `CONFLICT_ID` that is already contained in the path to root.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1904:
URL: https://github.com/apache/maven-resolver/pull/1904#issuecomment-4648829928
Applied all PR comments in last commit, using set, using null-protection in set, added test, fixed comment (added + to have some ref to release timeline).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #1904:
URL: https://github.com/apache/maven-resolver/pull/1904
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1904:
URL: https://github.com/apache/maven-resolver/pull/1904#issuecomment-4647273566
One additional thought on **test coverage**: the existing `ConflictResolverTest` covers several cycle scenarios (`winnerCycleRemoved`, `subtreeRepeatedSameInstanceWithRealCycle`, `nettyBoringSslExample`), but none of them reproduce the specific topology that triggered the NPE — where a cycle-flagged node has children that are *not* in the cycle, and those children's subtrees contain unique conflictIds not reachable by any other path.
A test like the following would nail down the exact regression from 5e97fd15 and prevent it from recurring:
```java
// dom4j-like topology:
// root -> A -> B (in cycle with A via cyclicPredecessors) -> C (unique, only reachable through B)
// -> A' (same conflictId as A, actual cycle)
// Old code: B flagged as cycle → C never explored → partitions.get(C_id) == null → NPE
// New code: B not a cycle (B_id ∉ [root_id, A_id]) → C explored normally → cycle broken at A'
```
Not blocking the merge, but would be a nice safety net. The existing tests exercise the *correction* path (cycle properly broken) but not the *non-regression* path (non-cycle siblings properly explored).
— gnodet
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Bugfix: Fix NPE in PathConflictResolver [maven-resolver]
Posted by "slawekjaranowski (via GitHub)" <gi...@apache.org>.
slawekjaranowski commented on code in PR #1904:
URL: https://github.com/apache/maven-resolver/pull/1904#discussion_r3370060162
##########
maven-resolver-demos/maven-resolver-demo-snippets/src/main/java/org/apache/maven/resolver/examples/ResolveTransitiveDependencies.java:
##########
@@ -50,6 +50,7 @@ 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("dom4j:dom4j:1.6.1");
Review Comment:
do we need such comment?
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org