From ec77f732bb18f4acec8ee863eccdcfe96f5ec1e6 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Sat, 25 Jul 2026 01:43:56 +0200 Subject: [PATCH 1/2] Fix #2013: reuse DependencyManager instance when no new management data is collected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AbstractDependencyManager.deriveChildManager() now returns `this` when no new management data (versions, scopes, optionals, local paths, exclusions) was collected at the current depth AND management is already being applied (depth >= applyFrom). This avoids creating distinct-but-semantically-equal DependencyManager instances that defeat the BfDependencyCollector pool cache — the pool key includes the manager, so unique managers cause pool misses, which lets the skipper prune subtrees that should have been served from the cache. The isApplied() guard is necessary: at depth < applyFrom, returning `this` would freeze the depth counter and prevent management rules from ever being applied to descendants. Co-Authored-By: Claude Opus 4.6 --- .../BfWithSkipperDependencyCollectorTest.java | 72 +++++++++++++++++++ .../pool-cache-transparency/gid_b-alt_1.0.ini | 2 + .../pool-cache-transparency/gid_b_1.0.ini | 2 + .../pool-cache-transparency/gid_c_1.0.ini | 2 + .../pool-cache-transparency/gid_d_1.0.ini | 1 + .../pool-cache-transparency/gid_root_1.0.ini | 3 + .../manager/AbstractDependencyManager.java | 17 +++++ .../graph/manager/DependencyManagerTest.java | 32 +++++++++ 8 files changed, 131 insertions(+) create mode 100644 maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini create mode 100644 maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini create mode 100644 maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini create mode 100644 maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini create mode 100644 maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini diff --git a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java index cf9edd8794..4876e40d59 100644 --- a/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java +++ b/maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java @@ -25,6 +25,7 @@ import org.eclipse.aether.collection.CollectResult; import org.eclipse.aether.collection.DependencyCollectionException; import org.eclipse.aether.graph.Dependency; +import org.eclipse.aether.graph.DependencyNode; import org.eclipse.aether.graph.Exclusion; import org.eclipse.aether.impl.ArtifactDescriptorReader; import org.eclipse.aether.internal.impl.StubRemoteRepositoryManager; @@ -37,6 +38,8 @@ import org.junit.jupiter.api.Test; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; /** * UT for {@link BfDependencyCollector}. @@ -68,6 +71,75 @@ private Dependency newDep(String coords, String scope, Collection exc return d.setExclusions(exclusions); } + /** + * Verifies that the pool cache is transparent w.r.t. the DependencyManager: the graph + * structure must not depend on whether the pool hits or misses. + *

+ * Scenario (from #2013): + * 

+     *   root
+     *   ├── b      → c → d
+     *   └── b-alt  → c → d   (c is a shared transitive dependency)
+     *
+ * With {@link TransitiveDependencyManager}, {@code deriveChildManager()} used to always + * create a new instance (unique {@code path} field), making every pool key unique. + * The pool would miss for {@code c} under {@code b-alt}, the skipper would mark it as + * a duplicate, and the node would end up with zero children — even though the same + * {@code c} under {@code b} had children. + *

+ * The fix in {@code AbstractDependencyManager.deriveChildManager()} reuses the same + * manager instance when no new management data is collected, so the pool key matches + * and children are preserved. + */ + @Test + void testPoolCacheTransparencyWithTransitiveDependencyManager() throws DependencyCollectionException { + collector = setupCollector(newReader("pool-cache-transparency/")); + parser = new DependencyGraphParser("artifact-descriptions/pool-cache-transparency/"); + session.setDependencyManager(new TransitiveDependencyManager(null)); + + Dependency root = newDep("gid:root:ext:1.0", "compile"); + CollectRequest request = new CollectRequest(root, Collections.singletonList(repository)); + CollectResult result = collector.collectDependencies(session, request); + + assertEquals(0, result.getExceptions().size()); + + // root has two children: b and b-alt + DependencyNode rootNode = result.getRoot(); + assertEquals(2, rootNode.getChildren().size(), "root should have 2 children (b, b-alt)"); + + // b → c + DependencyNode b = rootNode.getChildren().get(0); + assertEquals("b", b.getArtifact().getArtifactId()); + assertFalse(b.getChildren().isEmpty(), "b should have children"); + + // b → c → d + DependencyNode cUnderB = b.getChildren().get(0); + assertEquals("c", cUnderB.getArtifact().getArtifactId()); + assertFalse(cUnderB.getChildren().isEmpty(), "c under b should have children (d)"); + + // b-alt → c (this is the key assertion: c under b-alt must also have children) + DependencyNode bAlt = rootNode.getChildren().get(1); + assertEquals("b-alt", bAlt.getArtifact().getArtifactId()); + assertFalse(bAlt.getChildren().isEmpty(), "b-alt should have children"); + + DependencyNode cUnderBAlt = bAlt.getChildren().get(0); + assertEquals("c", cUnderBAlt.getArtifact().getArtifactId()); + + // Before the fix, this assertion would fail: c under b-alt had zero children + // because the pool key differed (different DependencyManager instance) and the + // skipper marked it as a duplicate. + assertFalse( + cUnderBAlt.getChildren().isEmpty(), + "c under b-alt should have children (d) — pool cache must be transparent"); + + // Verify that c's child is d in both subtrees + assertEquals("d", cUnderB.getChildren().get(0).getArtifact().getArtifactId()); + assertTrue( + cUnderBAlt.getChildren().stream() + .anyMatch(n -> "d".equals(n.getArtifact().getArtifactId())), + "c under b-alt should have d as a child"); + } + @Test void testSkipperWithDifferentExclusion() throws DependencyCollectionException { collector = setupCollector(newReader("managed/")); diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini new file mode 100644 index 0000000000..a5aecf921b --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b-alt_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:c:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini new file mode 100644 index 0000000000..a5aecf921b --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_b_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:c:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini new file mode 100644 index 0000000000..732de5300a --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_c_1.0.ini @@ -0,0 +1,2 @@ +[dependencies] +gid:d:ext:1.0 diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini new file mode 100644 index 0000000000..61a252c235 --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_d_1.0.ini @@ -0,0 +1 @@ +[dependencies] diff --git a/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini new file mode 100644 index 0000000000..f59e27e009 --- /dev/null +++ b/maven-resolver-impl/src/test/resources/artifact-descriptions/pool-cache-transparency/gid_root_1.0.ini @@ -0,0 +1,3 @@ +[dependencies] +gid:b:ext:1.0 +gid:b-alt:ext:1.0 diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java index 2b2eeab358..c124a018d2 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java @@ -379,6 +379,23 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context) } } + // Optimization: when no new management data was collected at this depth and management + // is already being applied (depth >= applyFrom), reuse this instance. This avoids creating + // unnecessarily distinct DependencyManager instances that would defeat the BF collector's + // pool cache — the pool key includes the manager, so distinct-but-semantically-equal + // managers cause pool misses, which in turn lets the skipper prune subtrees that should + // have been served from the cache. This is the common case for transitive dependencies + // whose POMs do not declare . + // See https://github.com/apache/maven-resolver/issues/2013 + if (managedVersions == null + && managedScopes == null + && managedOptionals == null + && managedLocalPaths == null + && managedExclusions == null + && isApplied()) { + return this; + } + return newInstance( managedVersions != null ? managedVersions.done() : null, managedScopes != null ? managedScopes.done() : null, diff --git a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java index db5c4afe17..9976fe1d02 100644 --- a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java +++ b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java @@ -204,6 +204,38 @@ void testTransitive() { assertNull(mngt); } + /** + * Verifies the instance-reuse optimization in {@link AbstractDependencyManager#deriveChildManager}: + * when no new management data is collected and management is already being applied + * (depth >= applyFrom), deriveChildManager should return the same instance. + * This is critical for BF collector pool cache transparency (issue #2013). + */ + @Test + void testDeriveChildManagerReusesInstanceWhenNoNewManagementData() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // depth=0 → depth=1: root → first level (applyFrom=2, so not yet applied) + DependencyManager depth1 = manager.deriveChildManager(newContext()); + assertNotSame(manager, depth1, "depth 0→1: must return new instance (not yet applied)"); + + // depth=1 → depth=2: no managed deps, now at applyFrom=2 (applied) + DependencyManager depth2 = depth1.deriveChildManager(newContext()); + assertNotSame(depth1, depth2, "depth 1→2: must return new instance (crossing applyFrom boundary)"); + + // depth=2 → depth=3: no managed deps, already applied → should reuse + DependencyManager depth3 = depth2.deriveChildManager(newContext()); + assertSame(depth2, depth3, "depth 2→3 with no managed deps: should reuse instance"); + + // depth=3 → depth=4: still no managed deps → should keep reusing + DependencyManager depth4 = depth3.deriveChildManager(newContext()); + assertSame(depth3, depth4, "depth 3→4 with no managed deps: should reuse instance"); + + // depth=2 → depth=3 with managed deps: should NOT reuse + DependencyManager depth3WithMgmt = + depth2.deriveChildManager(newContext(new Dependency(new DefaultArtifact("new:dep:1.0"), "compile"))); + assertNotSame(depth2, depth3WithMgmt, "depth 2→3 with managed deps: must return new instance"); + } + @Test void testDefault() { DependencyManager manager = new DefaultDependencyManager(null); From 080296079df3df080ce12bb1ca0faa58b721e8d7 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 28 Jul 2026 09:54:31 +0200 Subject: [PATCH 2/2] Address review: add DefaultDependencyManager and nearer-to-root-wins tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add two tests suggested by review on PR #2014: 1. testDeriveChildManagerReusesInstanceWithDefaultManager — verifies the optimization also works with DefaultDependencyManager (applyFrom=0), where instance reuse fires at the very first derivation. 2. testNearerToRootWinsAfterOptimizationReusesInstance — documents that the "nearer-to-root wins" invariant holds after the optimization reuses an instance: root's version management at depth 1 still wins when a deeper POM tries to override the same key, because containsManagedVersion finds it in ancestors and skips collection. Co-Authored-By: Claude Opus 4.6 --- .../graph/manager/DependencyManagerTest.java | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java index 9976fe1d02..9285a35137 100644 --- a/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java +++ b/maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/manager/DependencyManagerTest.java @@ -236,6 +236,69 @@ void testDeriveChildManagerReusesInstanceWhenNoNewManagementData() { assertNotSame(depth2, depth3WithMgmt, "depth 2→3 with managed deps: must return new instance"); } + /** + * Verifies the instance-reuse optimization with {@link DefaultDependencyManager} (applyFrom=0). + * Since management is applied from depth 0, the optimization should fire at the very first + * derivation when no management data is collected. + */ + @Test + void testDeriveChildManagerReusesInstanceWithDefaultManager() { + DependencyManager manager = new DefaultDependencyManager(null); + + // DefaultDependencyManager has applyFrom=0, so isApplied() is true from the start. + // depth=0 → depth=1: no managed deps → should reuse (already applied) + DependencyManager depth1 = manager.deriveChildManager(newContext()); + assertSame(manager, depth1, "depth 0→1 with no managed deps: should reuse (applyFrom=0)"); + + // depth=0 → depth=1 with managed deps: should NOT reuse + DependencyManager depth1WithMgmt = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("mgd:dep:1.0"), "compile"))); + assertNotSame(manager, depth1WithMgmt, "depth 0→1 with managed deps: must return new instance"); + + // depth=1 (with mgmt) → depth=2: no managed deps → should reuse + DependencyManager depth2 = depth1WithMgmt.deriveChildManager(newContext()); + assertSame(depth1WithMgmt, depth2, "depth 1→2 with no managed deps: should reuse"); + } + + /** + * Verifies that the "nearer-to-root wins" invariant holds when the optimization + * reuses instances. Scenario: root contributes version management at depth 1, + * the optimization fires at depth 2→3 (empty context), and a deeper POM tries + * to override the same key — the root's version must still win. + */ + @Test + void testNearerToRootWinsAfterOptimizationReusesInstance() { + DependencyManager manager = new TransitiveDependencyManager(null); + + // depth=0 → depth=1: root contributes version 1.0 for artifact "x" + DependencyManager depth1 = + manager.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:1.0"), "compile"))); + + // depth=1 → depth=2: empty context (new instance because applyFrom boundary) + DependencyManager depth2 = depth1.deriveChildManager(newContext()); + assertNotSame(depth1, depth2, "depth 1→2: new instance (crossing applyFrom boundary)"); + + // depth=2 → depth=3: empty context, optimization fires + DependencyManager depth3 = depth2.deriveChildManager(newContext()); + assertSame(depth2, depth3, "depth 2→3: optimization should reuse instance"); + + // Verify root's version still applies after optimization reuse + DependencyManagement mngt = depth3.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null)); + assertNotNull(mngt, "management should apply at depth >= applyFrom"); + assertEquals("1.0", mngt.getVersion(), "root's version must apply after optimization reuse"); + + // depth=3 → depth=4: context tries to override "x" with 2.0, but containsManagedVersion + // finds it in ancestors → no new data collected → optimization fires again + DependencyManager depth4 = + depth3.deriveChildManager(newContext(new Dependency(new DefaultArtifact("test:x:2.0"), "compile"))); + assertSame(depth3, depth4, "optimization should fire when context only has already-managed keys"); + + // Root's version still wins ("nearer-to-root wins" invariant) + mngt = depth4.manageDependency(new Dependency(new DefaultArtifact("test:x:9.0"), null)); + assertNotNull(mngt, "management should still apply"); + assertEquals("1.0", mngt.getVersion(), "nearer-to-root version must win over deeper override attempt"); + } + @Test void testDefault() { DependencyManager manager = new DefaultDependencyManager(null);