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..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 @@ -204,6 +204,101 @@ 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"); + } + + /** + * 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);