gnodet commented on issue #2013: URL: https://github.com/apache/maven-resolver/issues/2013#issuecomment-5075559762
### Deeper root cause: `TransitiveDependencyManager` always creates new instances The cache-transparency issue described above is a **symptom**. The deeper root cause is that `TransitiveDependencyManager` always creates a new instance in `deriveChildManager()`, even when it collects no new management data at the current depth. #### Why it happens [`AbstractDependencyManager.deriveChildManager()`](https://github.com/apache/maven-resolver/blob/master/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java) checks `isDerived()` (i.e. `depth < deriveUntil`) to decide whether to call `newInstance()`: - **`ClassicDependencyManager`** sets `deriveUntil = 2` → at depth ≥ 2, `isDerived()` returns `false` → `deriveChildManager()` returns `this` → same instance → pool key matches → no problem. - **`TransitiveDependencyManager`** sets `deriveUntil = Integer.MAX_VALUE` → `isDerived()` **always** returns `true` → `deriveChildManager()` **always** calls `newInstance()` → each new instance gets a unique `path` (ancestor chain, added in #1539) → every pool key is unique → pool never hits → skipper kicks in → duplicate nodes get pruned. This is why Maven 3.10 (`ClassicDependencyManager` by default) and Maven 4 (`TransitiveDependencyManager` by default) produce different graphs with the **same Resolver version** — it's not a Resolver regression, it's that `TransitiveDependencyManager` defeats the pool cache by design. #### Prior art: PR #1489 @cstamas explored this in [PR #1489](https://github.com/apache/maven-resolver/pull/1489) ("Move out depth from DepMgr") — which attempted to remove `depth` from the `DependencyManager` to make managers equal regardless of tree position. It was closed with -1 because it required breaking the `DependencyManager` API (adding `depth` as a parameter to `deriveChildManager()` and `manageDependency()`). #### Suggested non-breaking fix There's a simpler optimization that doesn't break any API. At the end of `AbstractDependencyManager.deriveChildManager()`, just before calling `newInstance(...)`, if **all collected maps are `null`** (no dependency contributed new management data at the current depth), return `this` instead of creating a new instance: ```java // In AbstractDependencyManager.deriveChildManager(): if (managedVersions == null && managedScopes == null && managedOptionals == null && managedLocalPaths == null && managedExclusions == null) { return this; // no new management data → reuse this manager } return newInstance(/* ... */); ``` This would make `TransitiveDependencyManager` return the same instance when a dependency contributes no management rules — which is the **common case** for most transitive dependencies (only a handful of POMs actually declare `<dependencyManagement>`). Pool keys would match in the typical case, the skipper wouldn't be consulted for already-cached subtrees, and the shade plugin scenario (and any similar plugin walking the dependency graph) would work without needing `CONFIG_PROP_VERBOSE`. This fix is purely an optimization — it preserves the current behavior for nodes that DO contribute management data, while avoiding unnecessary new instances for nodes that don't. -- 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]
