gnodet-bot commented on code in PR #281:
URL: https://github.com/apache/maven/pull/281#discussion_r3987063591
##########
impl/maven-core/src/main/java/org/apache/maven/RepositoryUtils.java:
##########
@@ -65,6 +67,46 @@
*/
public class RepositoryUtils {
+ @FunctionalInterface
+ private interface ArtifactOrderingStrategy {
+ void apply(
+ Collection<org.apache.maven.artifact.Artifact> artifacts,
+ Collection<? extends DependencyNode> nodes,
+ List<String> trail,
+ DependencyFilter filter);
+ }
+
+ public enum ArtifactOrdering {
+ /**
+ * Depth-first traversal of the dependency graph to convert nodes to
artifacts. This is the original behavior.
+ */
+ DFS(RepositoryUtils::toArtifactsDFS),
+ /**
+ * Breadth-first traversal of the dependency graph, using a List of
Lists to store artifacts by depth, and a stack to keep the dependency trail.
This is the new behavior.
+ */
+ BFS(RepositoryUtils::toArtifactsBFS);
+
+ private ArtifactOrderingStrategy strategy;
+
+ ArtifactOrdering(ArtifactOrderingStrategy strategy) {
+ this.strategy = strategy;
+ }
+
+ public ArtifactOrderingStrategy getStrategy() {
+ return strategy;
+ }
+ }
+
+ private static ArtifactOrdering artifactOrdering = ArtifactOrdering.BFS;
Review Comment:
🔴 **Thread-safety: mutable global state without synchronization.**
This is a non-`volatile`, non-synchronized mutable static field with a
public setter. Maven's concurrent reactor (`-T`) runs dependency resolution on
multiple threads simultaneously. Two problems:
1. **Visibility**: Thread A calls `setArtifactOrdering(DFS)` — Thread B may
never see the write (JMM allows caching in registers/L1).
2. **Mid-traversal corruption**: If the ordering changes while
`toArtifactsDFS` is recursing (it calls the public `toArtifacts` dispatcher —
see next finding), the traversal switches strategy mid-tree.
At minimum, the field should be `volatile`. Better: make it an
`AtomicReference<ArtifactOrdering>` or pass the strategy as a parameter rather
than relying on global mutable state.
```suggestion
private static volatile ArtifactOrdering artifactOrdering =
ArtifactOrdering.BFS;
```
##########
impl/maven-core/src/test/java/org/apache/maven/RepositoryUtilsTest.java:
##########
@@ -32,4 +42,133 @@ void
testToArtifactMethodsReturnNullWhenInputParameterIsNull() {
assertNull(RepositoryUtils.toArtifact((Artifact) null));
assertNull(RepositoryUtils.toArtifact((org.apache.maven.artifact.Artifact)
null));
}
+
+ public void
testToArtifactsCollectionOrderedByNodeDepth(RepositoryUtils.ArtifactOrdering
ordering) {
Review Comment:
🔴 **Dead test: missing `@Test` or `@ParameterizedTest` — JUnit will never
discover or execute this method.**
This method:
1. Has no `@Test` annotation
2. Takes a parameter (`ArtifactOrdering ordering`) which JUnit 5 cannot
inject without a `@ParameterizedTest` + `@EnumSource`
3. **Never uses the `ordering` parameter** — it doesn't call
`setArtifactOrdering(ordering)`, so even if it ran, it would always test the
default (BFS)
4. Hardcodes BFS-order expectations, so it would fail if called with `DFS`
As written, this provides zero test coverage. It should be:
```suggestion
@Test
void testToArtifactsCollectionOrderedByNodeDepth() {
```
And either:
- Remove the `ordering` parameter and test only BFS, or
- Use `@ParameterizedTest @EnumSource(ArtifactOrdering.class)` with
`setArtifactOrdering(ordering)` at the start and separate expected values per
ordering.
##########
impl/maven-core/src/main/java/org/apache/maven/RepositoryUtils.java:
##########
@@ -65,6 +67,46 @@
*/
public class RepositoryUtils {
+ @FunctionalInterface
+ private interface ArtifactOrderingStrategy {
+ void apply(
+ Collection<org.apache.maven.artifact.Artifact> artifacts,
+ Collection<? extends DependencyNode> nodes,
+ List<String> trail,
+ DependencyFilter filter);
+ }
+
+ public enum ArtifactOrdering {
+ /**
+ * Depth-first traversal of the dependency graph to convert nodes to
artifacts. This is the original behavior.
+ */
+ DFS(RepositoryUtils::toArtifactsDFS),
+ /**
+ * Breadth-first traversal of the dependency graph, using a List of
Lists to store artifacts by depth, and a stack to keep the dependency trail.
This is the new behavior.
+ */
+ BFS(RepositoryUtils::toArtifactsBFS);
+
+ private ArtifactOrderingStrategy strategy;
Review Comment:
💡 **Enum field should be `final`.**
The `strategy` field is assigned only in the constructor but not declared
`final`. Since enum constants are singletons, this field will never be
reassigned, but omitting `final` is misleading and allows accidental mutation.
```suggestion
private final ArtifactOrderingStrategy strategy;
```
##########
impl/maven-core/src/test/java/org/apache/maven/RepositoryUtilsTest.java:
##########
@@ -32,4 +42,133 @@ void
testToArtifactMethodsReturnNullWhenInputParameterIsNull() {
assertNull(RepositoryUtils.toArtifact((Artifact) null));
assertNull(RepositoryUtils.toArtifact((org.apache.maven.artifact.Artifact)
null));
}
+
+ public void
testToArtifactsCollectionOrderedByNodeDepth(RepositoryUtils.ArtifactOrdering
ordering) {
+ List<org.apache.maven.artifact.Artifact> artifacts = new ArrayList<>();
+
+ DependencyNode root = createDependencyTree();
+
+ List<String> trail = new ArrayList<>();
+ DependencyFilter filter = null;
+
+ RepositoryUtils.toArtifacts(artifacts, root.getChildren(), trail,
filter);
+
+ String expected =
+ "[gid:zlevel1:jar:1:, gid:ylevel1:jar:1:, gid:xlevel1:jar:1:,
gid:alevel2:jar:1:, gid:blevel2:jar:1:, gid:clevel2:jar:1:,
gid:alevel3:jar:1:]";
+ assertEquals(expected, artifacts.toString());
+
+ String[][] expectedTrails = {
+ {"gid:zlevel1:jar:1"},
+ {"gid:ylevel1:jar:1"},
+ {"gid:xlevel1:jar:1"},
+ {"gid:zlevel1:jar:1", "gid:alevel2:jar:1"},
+ {"gid:ylevel1:jar:1", "gid:blevel2:jar:1"},
+ {"gid:xlevel1:jar:1", "gid:clevel2:jar:1"},
+ {"gid:ylevel1:jar:1", "gid:blevel2:jar:1", "gid:alevel3:jar:1"}
+ };
+
+ assertDependencyTrails(artifacts, expectedTrails);
+ }
+
+ public void
testToArtifactsCollectionOrderedByNodeDepthWithFilter(RepositoryUtils.ArtifactOrdering
ordering) {
Review Comment:
🔴 **Same issue as above** — this method is also never executed by JUnit.
Missing `@Test`/`@ParameterizedTest`, unused `ordering` parameter, hardcoded
BFS expectations.
```suggestion
@Test
void testToArtifactsCollectionOrderedByNodeDepthWithFilter() {
```
##########
impl/maven-core/src/main/java/org/apache/maven/RepositoryUtils.java:
##########
@@ -116,6 +158,14 @@ public static void toArtifacts(
Collection<? extends DependencyNode> nodes,
List<String> trail,
DependencyFilter filter) {
+ artifactOrdering.getStrategy().apply(artifacts, nodes, trail, filter);
+ }
+
+ private static void toArtifactsDFS(
+ Collection<org.apache.maven.artifact.Artifact> artifacts,
+ Collection<? extends DependencyNode> nodes,
+ List<String> trail,
+ DependencyFilter filter) {
for (DependencyNode node : nodes) {
org.apache.maven.artifact.Artifact artifact =
toArtifact(node.getDependency());
Review Comment:
🔴 **Bug: `toArtifactsDFS` recurses through the public dispatcher instead of
calling itself.**
The recursive call inside this method (at the end of the `for` loop, line
181 in the new file) is:
```java
toArtifacts(artifacts, node.getChildren(), nodeTrail, filter);
```
This calls the **public** `toArtifacts` method which dispatches through
`artifactOrdering.getStrategy()`. If the global ordering field is `BFS`, the
DFS method recurses into BFS mid-traversal — producing a corrupted mix of
orderings.
The original code was correct because `toArtifacts` WAS the recursive
method. Now that it's a dispatcher, the DFS implementation must call itself:
```java
toArtifactsDFS(artifacts, node.getChildren(), nodeTrail, filter);
```
--
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]
