Make levelOrder the default visitor (#1593)

Apply same strategy to classpath collection as we apply to dependency collection ("nearest wins").

Co-authored-by: Guillaume Nodet <gnodet@gmail.com>

Author: cstamas

maven-resolver-api/src/main/java/org/eclipse/aether/ConfigurationProperties.java

@@ -515,16 +515,17 @@ public final class ConfigurationProperties {
     public static final String HTTPS_SECURITY_MODE_INSECURE = "insecure";
 
     /**
-     * A flag indicating which visitor should be used to "flatten" the dependency graph into list. Default is
-     * same as in older resolver versions "preOrder", while it can accept values like "postOrder" and "levelOrder".
+     * A flag indicating which visitor should be used to "flatten" the dependency graph into list. In Maven 4
+     * the default is new "levelOrder", while Maven 3 used "preOrder". This property accepts values
+     * "preOrder", "postOrder" and "levelOrder".
      *
      * @see #REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_PREORDER
      * @see #REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_POSTORDER
      * @see #REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_LEVELORDER
      * @since 2.0.0
      * @configurationSource {@link RepositorySystemSession#getConfigProperties()}
      * @configurationType {@link java.lang.String}
-     * @configurationDefaultValue {@link #REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_PREORDER}
+     * @configurationDefaultValue {@link #DEFAULT_REPOSITORY_SYSTEM_DEPENDENCY_VISITOR}
      * @configurationRepoIdSuffix No
      */
     public static final String REPOSITORY_SYSTEM_DEPENDENCY_VISITOR = PREFIX_SYSTEM + "dependencyVisitor";
@@ -551,6 +552,14 @@ public final class ConfigurationProperties {
      */
     public static final String REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_LEVELORDER = "levelOrder";
 
+    /**
+     * The default visitor strategy.
+     *
+     * @since 2.0.12
+     */
+    public static final String DEFAULT_REPOSITORY_SYSTEM_DEPENDENCY_VISITOR =
+            REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_LEVELORDER;
+
     /**
      * A flag indicating whether version scheme cache statistics should be printed on JVM shutdown.
      * This is useful for analyzing cache performance and effectiveness in development and testing scenarios.

maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/DefaultRepositorySystem.java

@@ -92,7 +92,6 @@
 import org.eclipse.aether.spi.artifact.decorator.ArtifactDecoratorFactory;
 import org.eclipse.aether.spi.synccontext.SyncContextFactory;
 import org.eclipse.aether.util.ConfigUtils;
-import org.eclipse.aether.util.graph.visitor.FilteringDependencyVisitor;
 import org.eclipse.aether.util.graph.visitor.LevelOrderDependencyNodeConsumerVisitor;
 import org.eclipse.aether.util.graph.visitor.PostorderDependencyNodeConsumerVisitor;
 import org.eclipse.aether.util.graph.visitor.PreorderDependencyNodeConsumerVisitor;
@@ -328,27 +327,24 @@ private List<DependencyNode> doFlattenDependencyNodes(
             RepositorySystemSession session, DependencyNode root, DependencyFilter dependencyFilter) {
         final ArrayList<DependencyNode> dependencyNodes = new ArrayList<>();
         if (root != null) {
-            DependencyVisitor builder = getDependencyVisitor(session, dependencyNodes::add);
-            DependencyVisitor visitor =
-                    (dependencyFilter != null) ? new FilteringDependencyVisitor(builder, dependencyFilter) : builder;
-            root.accept(visitor);
+            root.accept(getDependencyVisitor(session, dependencyNodes::add, dependencyFilter));
         }
         return dependencyNodes;
     }
 
     private DependencyVisitor getDependencyVisitor(
-            RepositorySystemSession session, Consumer<DependencyNode> nodeConsumer) {
+            RepositorySystemSession session, Consumer<DependencyNode> nodeConsumer, DependencyFilter dependencyFilter) {
         String strategy = ConfigUtils.getString(
                 session,
-                ConfigurationProperties.REPOSITORY_SYSTEM_DEPENDENCY_VISITOR_PREORDER,
+                ConfigurationProperties.DEFAULT_REPOSITORY_SYSTEM_DEPENDENCY_VISITOR,
                 ConfigurationProperties.REPOSITORY_SYSTEM_DEPENDENCY_VISITOR);
         switch (strategy) {
             case PreorderDependencyNodeConsumerVisitor.NAME:
-                return new PreorderDependencyNodeConsumerVisitor(nodeConsumer);
+                return new PreorderDependencyNodeConsumerVisitor(nodeConsumer, dependencyFilter);
             case PostorderDependencyNodeConsumerVisitor.NAME:
-                return new PostorderDependencyNodeConsumerVisitor(nodeConsumer);
+                return new PostorderDependencyNodeConsumerVisitor(nodeConsumer, dependencyFilter);
             case LevelOrderDependencyNodeConsumerVisitor.NAME:
-                return new LevelOrderDependencyNodeConsumerVisitor(nodeConsumer);
+                return new LevelOrderDependencyNodeConsumerVisitor(nodeConsumer, dependencyFilter);
             default:
                 throw new IllegalArgumentException("Invalid dependency visitor strategy: " + strategy);
         }

maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/AbstractDependencyNodeConsumerVisitor.java

@@ -22,23 +22,35 @@
 import java.util.Map;
 import java.util.function.Consumer;
 
+import org.eclipse.aether.graph.DependencyFilter;
 import org.eclipse.aether.graph.DependencyNode;
 import org.eclipse.aether.graph.DependencyVisitor;
 
 import static java.util.Objects.requireNonNull;
 
 /**
  * Abstract base class for dependency tree traverses that feed {@link Consumer<DependencyNode>}.
+ * <p>
+ * <strong>Implementations derived from this class cannot be embedded into {@link FilteringDependencyVisitor}</strong>,
+ * this is why these classes accept {@link DependencyFilter} in constructor instead.
  *
  * @since 2.0.0
  */
 abstract class AbstractDependencyNodeConsumerVisitor implements DependencyVisitor {
-    protected final Consumer<DependencyNode> nodeConsumer;
+    private static final DependencyFilter ACCEPT_ALL = (d, p) -> true;
+
+    private final Consumer<DependencyNode> nodeConsumer;
+
+    private final DependencyFilter filter;
+
+    private final Stack<DependencyNode> path;
 
     private final Map<DependencyNode, Object> visitedNodes;
 
-    protected AbstractDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer) {
+    protected AbstractDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer, DependencyFilter filter) {
         this.nodeConsumer = requireNonNull(nodeConsumer);
+        this.filter = filter == null ? ACCEPT_ALL : filter;
+        this.path = new Stack<>();
         this.visitedNodes = new IdentityHashMap<>(512);
     }
 
@@ -53,8 +65,26 @@ protected boolean setVisited(DependencyNode node) {
     }
 
     @Override
-    public abstract boolean visitEnter(DependencyNode node);
+    public final boolean visitEnter(DependencyNode node) {
+        path.push(node);
+        return doVisitEnter(node);
+    }
+
+    protected abstract boolean doVisitEnter(DependencyNode node);
 
     @Override
-    public abstract boolean visitLeave(DependencyNode node);
+    public final boolean visitLeave(DependencyNode node) {
+        path.pop();
+        return doVisitLeave(node);
+    }
+
+    protected abstract boolean doVisitLeave(DependencyNode node);
+
+    protected boolean acceptNode(DependencyNode node) {
+        return filter.accept(node, path);
+    }
+
+    protected void consumeNode(DependencyNode node) {
+        nodeConsumer.accept(node);
+    }
 }

maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/LevelOrderDependencyNodeConsumerVisitor.java

@@ -20,15 +20,20 @@
 
 import java.util.ArrayList;
 import java.util.HashMap;
+import java.util.List;
 import java.util.function.Consumer;
 
 import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.graph.DependencyFilter;
 import org.eclipse.aether.graph.DependencyNode;
 
 /**
  * Processes dependency graph by traversing the graph in level order. This visitor visits each node exactly once
  * regardless how many paths within the dependency graph lead to the node such that the resulting node sequence is
  * free of duplicates.
+ * <p>
+ * <strong>Instances of this class cannot be embedded into {@link FilteringDependencyVisitor}</strong>, pass in the
+ * filter {@link DependencyFilter} into  constructor instead.
  *
  * @see NodeListGenerator
  * @since 2.0.0
@@ -45,30 +50,42 @@ public final class LevelOrderDependencyNodeConsumerVisitor extends AbstractDepen
      * Creates a new level order list generator.
      */
     public LevelOrderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer) {
-        super(nodeConsumer);
+        this(nodeConsumer, null);
+    }
+
+    /**
+     * Creates a new level order list generator with filter.
+     *
+     * @since 2.0.12
+     */
+    public LevelOrderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer, DependencyFilter filter) {
+        super(nodeConsumer, filter);
         nodesPerLevel = new HashMap<>(16);
         visits = new Stack<>();
     }
 
     @Override
-    public boolean visitEnter(DependencyNode node) {
+    protected boolean doVisitEnter(DependencyNode node) {
         boolean visited = !setVisited(node);
         visits.push(visited);
         if (!visited) {
-            nodesPerLevel.computeIfAbsent(visits.size(), k -> new ArrayList<>()).add(node);
+            List<DependencyNode> nodesOnLevel = nodesPerLevel.computeIfAbsent(visits.size(), k -> new ArrayList<>());
+            if (acceptNode(node)) {
+                nodesOnLevel.add(node);
+            }
         }
         return !visited;
     }
 
     @Override
-    public boolean visitLeave(DependencyNode node) {
+    protected boolean doVisitLeave(DependencyNode node) {
         Boolean visited = visits.pop();
         if (visited) {
             return true;
         }
         if (visits.isEmpty()) {
             for (int l = 1; nodesPerLevel.containsKey(l); l++) {
-                nodesPerLevel.get(l).forEach(nodeConsumer);
+                nodesPerLevel.get(l).forEach(this::consumeNode);
             }
         }
         return true;

maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/PostorderDependencyNodeConsumerVisitor.java

@@ -21,12 +21,16 @@
 import java.util.function.Consumer;
 
 import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.graph.DependencyFilter;
 import org.eclipse.aether.graph.DependencyNode;
 
 /**
  * Processes dependency graph by traversing the graph in postorder. This visitor visits each node exactly once
  * regardless how many paths within the dependency graph lead to the node such that the resulting node sequence is
  * free of duplicates.
+ * <p>
+ * <strong>Instances of this class cannot be embedded into {@link FilteringDependencyVisitor}</strong>, pass in the
+ * filter {@link DependencyFilter} into  constructor instead.
  *
  * @see NodeListGenerator
  * @since 2.0.0
@@ -41,24 +45,35 @@ public final class PostorderDependencyNodeConsumerVisitor extends AbstractDepend
      * Creates a new postorder list generator.
      */
     public PostorderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer) {
-        super(nodeConsumer);
+        this(nodeConsumer, null);
+    }
+
+    /**
+     * Creates a new postorder list generator.
+     *
+     * @since 2.0.12
+     */
+    public PostorderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer, DependencyFilter filter) {
+        super(nodeConsumer, filter);
         visits = new Stack<>();
     }
 
     @Override
-    public boolean visitEnter(DependencyNode node) {
+    protected boolean doVisitEnter(DependencyNode node) {
         boolean visited = !setVisited(node);
         visits.push(visited);
         return !visited;
     }
 
     @Override
-    public boolean visitLeave(DependencyNode node) {
+    protected boolean doVisitLeave(DependencyNode node) {
         Boolean visited = visits.pop();
         if (visited) {
             return true;
         }
-        nodeConsumer.accept(node);
+        if (acceptNode(node)) {
+            consumeNode(node);
+        }
         return true;
     }
 }

maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/visitor/PreorderDependencyNodeConsumerVisitor.java

@@ -21,12 +21,16 @@
 import java.util.function.Consumer;
 
 import org.eclipse.aether.ConfigurationProperties;
+import org.eclipse.aether.graph.DependencyFilter;
 import org.eclipse.aether.graph.DependencyNode;
 
 /**
  * Processes dependency graph by traversing the graph in preorder. This visitor visits each node exactly once
  * regardless how many paths within the dependency graph lead to the node such that the resulting node sequence is
  * free of duplicates.
+ * <p>
+ * <strong>Instances of this class cannot be embedded into {@link FilteringDependencyVisitor}</strong>, pass in the
+ * filter {@link DependencyFilter} into  constructor instead.
  *
  * @see NodeListGenerator
  * @since 2.0.0
@@ -39,20 +43,31 @@ public final class PreorderDependencyNodeConsumerVisitor extends AbstractDepende
      * Creates a new preorder list generator.
      */
     public PreorderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer) {
-        super(nodeConsumer);
+        this(nodeConsumer, null);
+    }
+
+    /**
+     * Creates a new preorder list generator.
+     *
+     * @since 2.0.12
+     */
+    public PreorderDependencyNodeConsumerVisitor(Consumer<DependencyNode> nodeConsumer, DependencyFilter filter) {
+        super(nodeConsumer, filter);
     }
 
     @Override
-    public boolean visitEnter(DependencyNode node) {
+    protected boolean doVisitEnter(DependencyNode node) {
         if (!setVisited(node)) {
             return false;
         }
-        nodeConsumer.accept(node);
+        if (acceptNode(node)) {
+            consumeNode(node);
+        }
         return true;
     }
 
     @Override
-    public boolean visitLeave(DependencyNode node) {
+    protected boolean doVisitLeave(DependencyNode node) {
         return true;
     }
 }

maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/visitor/NodeListGeneratorTest.java

@@ -218,4 +218,40 @@ public boolean visitLeave(DependencyNode node) {
         assertEquals(3, nodeListGenerator.getFiles().size());
         assertEquals(fileNames, classPathNames);
     }
+
+    @Test
+    void testPreOrderDuplicateSuppressionWithFilteringSimple() throws Exception {
+        DependencyNode root = parse("simple.txt");
+
+        NodeListGenerator nodeListGenerator = new NodeListGenerator();
+        PreorderDependencyNodeConsumerVisitor visitor = new PreorderDependencyNodeConsumerVisitor(
+                nodeListGenerator, (d, p) -> !"a".equals(d.getArtifact().getArtifactId()));
+        root.accept(visitor);
+
+        assertSequence(nodeListGenerator.getNodes(), "b", "c", "d", "e");
+    }
+
+    @Test
+    void testPostOrderDuplicateSuppressionWithFilteringSimple() throws Exception {
+        DependencyNode root = parse("simple.txt");
+
+        NodeListGenerator nodeListGenerator = new NodeListGenerator();
+        PostorderDependencyNodeConsumerVisitor visitor = new PostorderDependencyNodeConsumerVisitor(
+                nodeListGenerator, (d, p) -> !"a".equals(d.getArtifact().getArtifactId()));
+        root.accept(visitor);
+
+        assertSequence(nodeListGenerator.getNodes(), "c", "b", "e", "d");
+    }
+
+    @Test
+    void testLevelOrderDuplicateSuppressionWithFilteringSimple() throws Exception {
+        DependencyNode root = parse("simple.txt");
+
+        NodeListGenerator nodeListGenerator = new NodeListGenerator();
+        LevelOrderDependencyNodeConsumerVisitor visitor = new LevelOrderDependencyNodeConsumerVisitor(
+                nodeListGenerator, (d, p) -> !"a".equals(d.getArtifact().getArtifactId()));
+        root.accept(visitor);
+
+        assertSequence(nodeListGenerator.getNodes(), "b", "d", "c", "e");
+    }
 }

src/site/markdown/configuration.md

@@ -116,7 +116,7 @@ To modify this file, edit the template and regenerate.
 | `"aether.syncContext.named.retry.wait"` | `Long` | The amount of milliseconds to wait between retries on time-out. |  `200l`  | 1.7.0 |  No  | Session Configuration |
 | `"aether.syncContext.named.time"` | `Long` | The maximum of time amount to be blocked to obtain lock. |  `30l`  | 1.7.0 |  No  | Session Configuration |
 | `"aether.syncContext.named.time.unit"` | `String` | The unit of maximum time amount to be blocked to obtain lock. Use TimeUnit enum names. |  `"SECONDS"`  | 1.7.0 |  No  | Session Configuration |
-| `"aether.system.dependencyVisitor"` | `String` | A flag indicating which visitor should be used to "flatten" the dependency graph into list. Default is same as in older resolver versions "preOrder", while it can accept values like "postOrder" and "levelOrder". |  `"preOrder"`  | 2.0.0 |  No  | Session Configuration |
+| `"aether.system.dependencyVisitor"` | `String` | A flag indicating which visitor should be used to "flatten" the dependency graph into list. In Maven 4 the default is new "levelOrder", while Maven 3 used "preOrder". This property accepts values "preOrder", "postOrder" and "levelOrder". |  `"levelOrder"`  | 2.0.0 |  No  | Session Configuration |
 | `"aether.transport.apache.followRedirects"` | `Boolean` | If enabled, Apache HttpClient will follow HTTP redirects. |  `true`  | 2.0.2 |  Yes  | Session Configuration |
 | `"aether.transport.apache.https.cipherSuites"` | `String` | Comma-separated list of <a href="https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#ciphersuites">Cipher Suites</a> which are enabled for HTTPS connections. |  -  | 2.0.0 |  No  | Session Configuration |
 | `"aether.transport.apache.https.protocols"` | `String` | Comma-separated list of <a href="https://docs.oracle.com/javase/7/docs/technotes/guides/security/StandardNames.html#jssenames">Protocols </a> which are enabled for HTTPS connections. |  -  | 2.0.0 |  No  | Session Configuration |