Copilot commented on code in PR #2014:
URL: https://github.com/apache/maven-resolver/pull/2014#discussion_r3673657633
##########
maven-resolver-impl/src/test/java/org/eclipse/aether/internal/impl/collect/bf/BfWithSkipperDependencyCollectorTest.java:
##########
@@ -68,6 +71,75 @@ private Dependency newDep(String coords, String scope,
Collection<Exclusion> 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.
+ * <p>
+ * Scenario (from <a
href="https://github.com/apache/maven-resolver/issues/2013";>#2013</a>):
+ * <pre>
+ * root
+ * ├── b → c → d
+ * └── b-alt → c → d (c is a shared transitive dependency)
+ * </pre>
+ * 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.
+ * <p>
+ * 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());
Review Comment:
This test assumes a deterministic ordering of `rootNode.getChildren()` (b at
index 0, b-alt at index 1). If the collector or underlying data structures
don’t guarantee ordering, the test can become flaky. Prefer selecting
`b`/`b-alt` by matching `artifactId` from `rootNode.getChildren()`, and
likewise select `c` by matching rather than `get(0)`.
##########
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 <dependencyManagement>.
+ // See https://github.com/apache/maven-resolver/issues/2013
+ if (managedVersions == null
+ && managedScopes == null
+ && managedOptionals == null
+ && managedLocalPaths == null
+ && managedExclusions == null
+ && isApplied()) {
+ return this;
+ }
Review Comment:
The reuse condition hard-codes every managed-* accumulator. This is easy to
miss when new managed dimensions are added (e.g., introducing another managed
field later would require updating this condition). Consider centralizing the
‘collected anything’ decision (e.g., a single boolean set when any management
is recorded, or a small helper like `hasNewManagementData(...)`) so future
changes can’t accidentally bypass/reduce reuse.
--
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]
