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/10/20 18:44:59 UTC
[PR] fix: prefix files parsing bug [maven-resolver]
cstamas opened a new pull request, #2217:
URL: https://github.com/apache/maven-resolver/pull/2217
Fixes #2191
No matter of order, "shorter path wins". Also, fixing a misnamed field siblings vs children.
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#discussion_r4329845919
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTree.java:
##########
@@ -76,12 +117,24 @@ public boolean acceptedPath(String path) {
final List<String> pathElements = elementsOfPath(path);
PrefixTree currentNode = this;
for (String pathElement : pathElements) {
- currentNode = currentNode.siblings.get(pathElement);
+ currentNode = currentNode.children.get(pathElement);
if (currentNode == null || currentNode.isLeaf()) {
break;
}
}
// path is accepted if its elements lead to an existing node that is a leaf node
return currentNode != null && currentNode.isLeaf();
}
+
+ @Override
+ public String toString() {
+ return name + (leaf ? "*" : "");
+ }
+
+ public void dump(String prefix) {
+ System.out.println(prefix + this);
+ for (PrefixTree node : children.values()) {
+ node.dump(prefix + " ");
+ }
+ }
Review Comment:
Dropped
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#discussion_r4329625800
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTreeTest.java:
##########
@@ -33,10 +34,157 @@ public class PrefixTreeTest {
void smoke() {
PrefixTree prefixTree = new PrefixTree("root");
prefixTree.loadNodes(Stream.of("# comment", "", "/org/apache/maven"));
+ prefixTree.dump("");
+
+ assertFalse(prefixTree.acceptedPath("/com/google"));
+ assertFalse(prefixTree.acceptedPath("/com/google/maven/aaa"));
+
+ assertFalse(prefixTree.acceptedPath("/org/eclipse/maven"));
+ assertFalse(prefixTree.acceptedPath("/org/eclipse/maven/aaa"));
+
assertTrue(prefixTree.acceptedPath("/org/apache/maven"));
assertTrue(prefixTree.acceptedPath("/org/apache/maven/aaa"));
assertFalse(prefixTree.acceptedPath("/org/apache/baz"));
assertFalse(prefixTree.acceptedPath("/org/apache/baz/aaa"));
}
+
+ @Test
+ void overlappingShallow() {
+ PrefixTree prefixTree = new PrefixTree("root");
+ prefixTree.loadNodes(Stream.of("# comment", "/org", "/org/apache"));
+ prefixTree.dump("");
Review Comment:
🧹 Debug leftover: `dump()` prints to stdout during test runs. Remove before merge.
```suggestion
```
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTreeTest.java:
##########
@@ -33,10 +34,157 @@ public class PrefixTreeTest {
void smoke() {
PrefixTree prefixTree = new PrefixTree("root");
prefixTree.loadNodes(Stream.of("# comment", "", "/org/apache/maven"));
+ prefixTree.dump("");
+
+ assertFalse(prefixTree.acceptedPath("/com/google"));
+ assertFalse(prefixTree.acceptedPath("/com/google/maven/aaa"));
+
+ assertFalse(prefixTree.acceptedPath("/org/eclipse/maven"));
+ assertFalse(prefixTree.acceptedPath("/org/eclipse/maven/aaa"));
+
assertTrue(prefixTree.acceptedPath("/org/apache/maven"));
assertTrue(prefixTree.acceptedPath("/org/apache/maven/aaa"));
assertFalse(prefixTree.acceptedPath("/org/apache/baz"));
assertFalse(prefixTree.acceptedPath("/org/apache/baz/aaa"));
}
+
+ @Test
+ void overlappingShallow() {
+ PrefixTree prefixTree = new PrefixTree("root");
+ prefixTree.loadNodes(Stream.of("# comment", "/org", "/org/apache"));
+ prefixTree.dump("");
+
+ assertFalse(prefixTree.acceptedPath("/com/google"));
+ assertFalse(prefixTree.acceptedPath("/com/google/maven/aaa"));
+
+ assertTrue(prefixTree.acceptedPath("/org/eclipse/maven"));
+ assertTrue(prefixTree.acceptedPath("/org/eclipse/maven/aaa"));
+
+ assertTrue(prefixTree.acceptedPath("/org/apache/maven"));
+ assertTrue(prefixTree.acceptedPath("/org/apache/maven/aaa"));
+
+ assertTrue(prefixTree.acceptedPath("/org/apache/baz"));
+ assertTrue(prefixTree.acceptedPath("/org/apache/baz/aaa"));
+ }
+
+ @Test
+ void overlappingDeep() {
+ PrefixTree prefixTree = new PrefixTree("root");
+ prefixTree.loadNodes(Stream.of("# comment", "", "/org/apache", "/org/apache/maven"));
+ prefixTree.dump("");
Review Comment:
🧹 Debug leftover: `dump()` prints to stdout during test runs. Remove before merge.
```suggestion
```
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTreeTest.java:
##########
@@ -33,10 +34,157 @@ public class PrefixTreeTest {
void smoke() {
PrefixTree prefixTree = new PrefixTree("root");
prefixTree.loadNodes(Stream.of("# comment", "", "/org/apache/maven"));
+ prefixTree.dump("");
Review Comment:
🧹 Debug leftover: `dump()` prints to stdout during test runs. Remove before merge.
```suggestion
```
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas merged PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#issuecomment-6299040933
The CI failed with
```
Could not transfer artifact dev.sigstore:sigstore-java:pom:2.3.0 from/to central (https://repo.maven.apache.org/maven2): Connection reset
```
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#discussion_r4329323609
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/ruletree/Node.java:
##########
@@ -18,36 +18,32 @@
*/
package org.eclipse.aether.internal.impl.filter.ruletree;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.HashMap;
/**
* A tree structure with rules.
*/
abstract class Node<N extends Node<?>> {
protected final String name;
- protected final ConcurrentHashMap<String, N> siblings;
+ protected final HashMap<String, N> children;
Review Comment:
⚠️ **Thread-safety regression**: switching from `ConcurrentHashMap` to `HashMap` breaks concurrent access in `GroupTree`. `GroupIdRemoteRepositoryFilterSource.postProcess()` calls `.compute(normalized, ...).loadNode(line)` — the `.loadNode()` runs outside the atomic `compute()`, so multiple threads resolving artifacts from the same repository mutate the same `GroupTree.children` concurrently. `HashMap` is not thread-safe here.
Either keep `ConcurrentHashMap` for this field, or synchronize `GroupTree.loadNode()`.
(`PrefixTree` is unaffected — its tree is always fully built inside `computeIfAbsent` before being published.)
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#issuecomment-6300245225
Removed dump calls in UTs, but left methods in place as they are used in DEBUG output
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#discussion_r4329810505
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTree.java:
##########
@@ -42,13 +43,27 @@
* <li><a href="https://repo.eclipse.org/content/repositories/tycho/.meta/prefixes.txt">Eclipse Tycho</a></li>
* </ul>
*/
-public class PrefixTree extends Node<PrefixTree> {
+public class PrefixTree {
private static List<String> elementsOfPath(final String path) {
return Arrays.stream(path.split("/")).filter(e -> !e.isEmpty()).collect(toList());
}
+ private final String name;
+ private final HashMap<String, PrefixTree> children;
+ private final boolean leaf;
Review Comment:
Yea, is fine.
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#issuecomment-6300032862
Dropped `Node` as it became an impediment instead of "shared" parent.
--
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] fix: prefix files parsing bug [maven-resolver]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #2217:
URL: https://github.com/apache/maven-resolver/pull/2217#discussion_r4329741826
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTree.java:
##########
@@ -42,13 +43,27 @@
* <li><a href="https://repo.eclipse.org/content/repositories/tycho/.meta/prefixes.txt">Eclipse Tycho</a></li>
* </ul>
*/
-public class PrefixTree extends Node<PrefixTree> {
+public class PrefixTree {
private static List<String> elementsOfPath(final String path) {
return Arrays.stream(path.split("/")).filter(e -> !e.isEmpty()).collect(toList());
}
+ private final String name;
+ private final HashMap<String, PrefixTree> children;
+ private final boolean leaf;
Review Comment:
`PrefixTree` now uses `HashMap` for `children`, whereas the prior shared `Node` implementation used `ConcurrentHashMap`. If `PrefixTree` is ever accessed concurrently during/after construction (e.g., load + reads, or publication without strict happens-before), this can introduce data races or visibility issues. Consider using `ConcurrentHashMap` here as well, or make the structure immutable after `loadNodes()` (e.g., build with mutable maps, then freeze) and ensure safe publication.
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/filter/ruletree/PrefixTree.java:
##########
@@ -76,12 +117,24 @@ public boolean acceptedPath(String path) {
final List<String> pathElements = elementsOfPath(path);
PrefixTree currentNode = this;
for (String pathElement : pathElements) {
- currentNode = currentNode.siblings.get(pathElement);
+ currentNode = currentNode.children.get(pathElement);
if (currentNode == null || currentNode.isLeaf()) {
break;
}
}
// path is accepted if its elements lead to an existing node that is a leaf node
return currentNode != null && currentNode.isLeaf();
}
+
+ @Override
+ public String toString() {
+ return name + (leaf ? "*" : "");
+ }
+
+ public void dump(String prefix) {
+ System.out.println(prefix + this);
+ for (PrefixTree node : children.values()) {
+ node.dump(prefix + " ");
+ }
+ }
Review Comment:
This introduces (and exposes publicly) a `dump()` method that writes directly to stdout from production code. Direct `System.out.println` can cause noisy output in embedded/library usage and is hard to control operationally. Prefer either removing `dump()` entirely, making it package-private for debugging only, or routing output through the project’s logging framework at an appropriate level.
--
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