From 88eb3241d7d195e350b2a21403cee10583e96297 Mon Sep 17 00:00:00 2001 From: Tamas Cservenak Date: Tue, 29 Oct 2024 10:00:46 +0100 Subject: [PATCH] [MRESOLVER-614] Fix transitive dependency management (#595) Undo original "clic-clac" solution introduced in first PR, instead, add more fine-grained control of what and when is applied by tracking collected rule depth. --- https://issues.apache.org/jira/browse/MRESOLVER-614 --- .../manager/AbstractDependencyManager.java | 169 +++++++++++------- .../manager/ClassicDependencyManager.java | 40 ++--- .../manager/DefaultDependencyManager.java | 31 ++-- .../manager/TransitiveDependencyManager.java | 31 ++-- .../graph/manager/DependencyManagerTest.java | 3 +- 5 files changed, 149 insertions(+), 125 deletions(-) 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 77d2aca37..0b9c1cf77 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 @@ -18,6 +18,7 @@ */ package org.eclipse.aether.util.graph.manager; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; @@ -38,6 +39,20 @@ /** * A dependency manager support class. + *

+ * This implementation is Maven specific, as it works hand-in-hand along with Maven ModelBuilder. While model builder + * handles dependency management in the context of single POM (inheritance, imports, etc.), this implementation carries + * in-lineage modifications based on previously recorded dependency management rules sourced from ascendants while + * building the dependency graph. Root sourced management rules are special, in a way they are always applied, while + * en-route collected ones are carefully applied to proper descendants only, to not override work done by model + * builder already. + *

+ * Details: Model builder handles version, scope from own dependency management (think "effective POM"). On the other + * hand it does not handle optional for example. System paths are aligned across whole graph, making sure there is + * same system path used by same dependency. Finally, exclusions (exclusions are additional information not effective + * or applied in same POM) are always applied. This implementation makes sure, that version and scope are not applied + * onto same node that actually provided the rules, to no override work that ModelBuilder did. It achieves this goal + * by tracking "depth" for each collected rule and ignoring rules coming from same depth as processed dependency node is. * * @since 2.0.0 */ @@ -49,20 +64,18 @@ public abstract class AbstractDependencyManager implements DependencyManager { protected final int applyFrom; - protected final Map managedVersions; + protected final Map> managedVersions; - protected final Map managedScopes; + protected final Map> managedScopes; - protected final Map managedOptionals; + protected final Map> managedOptionals; - protected final Map managedLocalPaths; + protected final Map> managedLocalPaths; - protected final Map> managedExclusions; + protected final Map>>> managedExclusions; protected final SystemDependencyScope systemDependencyScope; - protected final DependencyCollectionContext currentContext; - private final int hashCode; protected AbstractDependencyManager(int deriveUntil, int applyFrom, ScopeManager scopeManager) { @@ -77,8 +90,7 @@ protected AbstractDependencyManager(int deriveUntil, int applyFrom, ScopeManager Collections.emptyMap(), scopeManager != null ? scopeManager.getSystemDependencyScope().orElse(null) - : SystemDependencyScope.LEGACY, - null); + : SystemDependencyScope.LEGACY); } @SuppressWarnings("checkstyle:ParameterNumber") @@ -86,13 +98,12 @@ protected AbstractDependencyManager( int depth, int deriveUntil, int applyFrom, - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - SystemDependencyScope systemDependencyScope, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions, + SystemDependencyScope systemDependencyScope) { this.depth = depth; this.deriveUntil = deriveUntil; this.applyFrom = applyFrom; @@ -103,8 +114,6 @@ protected AbstractDependencyManager( this.managedExclusions = requireNonNull(managedExclusions); // nullable: if using scope manager, but there is no system scope defined this.systemDependencyScope = systemDependencyScope; - // nullable until applicable, then "lock step" below - this.currentContext = currentContext; this.hashCode = Objects.hash( depth, @@ -118,12 +127,11 @@ protected AbstractDependencyManager( } protected abstract DependencyManager newInstance( - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - DependencyCollectionContext currentContext); + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions); @Override public DependencyManager deriveChildManager(DependencyCollectionContext context) { @@ -132,22 +140,13 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context) return this; } - if (!isApplied() || currentContext == null) { - return derive(context, context); // original behaviour - } else { - return derive(currentContext, context); // clic-clac: defer application to children; not onto own deps - } - } + Map> managedVersions = this.managedVersions; + Map> managedScopes = this.managedScopes; + Map> managedOptionals = this.managedOptionals; + Map> managedLocalPaths = this.managedLocalPaths; + Map>>> managedExclusions = this.managedExclusions; - protected DependencyManager derive( - DependencyCollectionContext currentContext, DependencyCollectionContext nextContext) { - Map managedVersions = this.managedVersions; - Map managedScopes = this.managedScopes; - Map managedOptionals = this.managedOptionals; - Map managedLocalPaths = this.managedLocalPaths; - Map> managedExclusions = this.managedExclusions; - - for (Dependency managedDependency : currentContext.getManagedDependencies()) { + for (Dependency managedDependency : context.getManagedDependencies()) { Artifact artifact = managedDependency.getArtifact(); Object key = new Key(artifact); @@ -156,7 +155,7 @@ protected DependencyManager derive( if (managedVersions == this.managedVersions) { managedVersions = new HashMap<>(this.managedVersions); } - managedVersions.put(key, version); + managedVersions.put(key, new Holder<>(depth, version)); } String scope = managedDependency.getScope(); @@ -164,7 +163,7 @@ protected DependencyManager derive( if (managedScopes == this.managedScopes) { managedScopes = new HashMap<>(this.managedScopes); } - managedScopes.put(key, scope); + managedScopes.put(key, new Holder<>(depth, scope)); } Boolean optional = managedDependency.getOptional(); @@ -172,7 +171,7 @@ protected DependencyManager derive( if (managedOptionals == this.managedOptionals) { managedOptionals = new HashMap<>(this.managedOptionals); } - managedOptionals.put(key, optional); + managedOptionals.put(key, new Holder<>(depth, optional)); } String localPath = systemDependencyScope == null @@ -182,7 +181,7 @@ protected DependencyManager derive( if (managedLocalPaths == this.managedLocalPaths) { managedLocalPaths = new HashMap<>(this.managedLocalPaths); } - managedLocalPaths.put(key, localPath); + managedLocalPaths.put(key, new Holder<>(depth, localPath)); } Collection exclusions = managedDependency.getExclusions(); @@ -190,13 +189,13 @@ protected DependencyManager derive( if (managedExclusions == this.managedExclusions) { managedExclusions = new HashMap<>(this.managedExclusions); } - Collection managed = managedExclusions.computeIfAbsent(key, k -> new LinkedHashSet<>()); - managed.addAll(exclusions); + Collection>> managed = + managedExclusions.computeIfAbsent(key, k -> new ArrayList<>()); + managed.add(new Holder<>(depth, exclusions)); } } - return newInstance( - managedVersions, managedScopes, managedOptionals, managedLocalPaths, managedExclusions, nextContext); + return newInstance(managedVersions, managedScopes, managedOptionals, managedLocalPaths, managedExclusions); } @Override @@ -206,21 +205,25 @@ public DependencyManagement manageDependency(Dependency dependency) { Object key = new Key(dependency.getArtifact()); if (isApplied()) { - String version = managedVersions.get(key); - if (version != null) { + Holder version = managedVersions.get(key); + // is managed locally by model builder + // apply only rules coming from "higher" levels + if (version != null && isApplicable(version)) { management = new DependencyManagement(); - management.setVersion(version); + management.setVersion(version.getValue()); } - String scope = managedScopes.get(key); - if (scope != null) { + Holder scope = managedScopes.get(key); + // is managed locally by model builder + // apply only rules coming from "higher" levels + if (scope != null && isApplicable(scope)) { if (management == null) { management = new DependencyManagement(); } - management.setScope(scope); + management.setScope(scope.getValue()); if (systemDependencyScope != null - && !systemDependencyScope.is(scope) + && !systemDependencyScope.is(scope.getValue()) && systemDependencyScope.getSystemPath(dependency.getArtifact()) != null) { Map properties = new HashMap<>(dependency.getArtifact().getProperties()); @@ -229,37 +232,48 @@ public DependencyManagement manageDependency(Dependency dependency) { } } + // system scope paths always applied to have them aligned + // (same artifact == same path) in whole graph if (systemDependencyScope != null - && (systemDependencyScope.is(scope) + && (scope != null && systemDependencyScope.is(scope.getValue()) || (scope == null && systemDependencyScope.is(dependency.getScope())))) { - String localPath = managedLocalPaths.get(key); + Holder localPath = managedLocalPaths.get(key); if (localPath != null) { if (management == null) { management = new DependencyManagement(); } Map properties = new HashMap<>(dependency.getArtifact().getProperties()); - systemDependencyScope.setSystemPath(properties, localPath); + systemDependencyScope.setSystemPath(properties, localPath.getValue()); management.setProperties(properties); } } - Boolean optional = managedOptionals.get(key); - if (optional != null) { + // optional is not managed by model builder + // apply only rules coming from "higher" levels + Holder optional = managedOptionals.get(key); + if (optional != null && isApplicable(optional)) { if (management == null) { management = new DependencyManagement(); } - management.setOptional(optional); + management.setOptional(optional.getValue()); } } - Collection exclusions = managedExclusions.get(key); + // exclusions affect only downstream + // this will not "exclude" own dependency, + // is just added as additional information + // ModelBuilder does not merge exclusions (only applies if dependency does not have exclusion) + // so we merge it here even from same level + Collection>> exclusions = managedExclusions.get(key); if (exclusions != null) { if (management == null) { management = new DependencyManagement(); } Collection result = new LinkedHashSet<>(dependency.getExclusions()); - result.addAll(exclusions); + for (Holder> exclusion : exclusions) { + result.addAll(exclusion.getValue()); + } management.setExclusions(result); } @@ -280,6 +294,17 @@ protected boolean isApplied() { return depth >= applyFrom; } + /** + * Returns {@code true} if rule in holder is applicable at current depth. + */ + protected boolean isApplicable(Holder holder) { + // explanation: derive collects rules (at given depth) and then last + // call newInstance does depth++. This means that distance 1 is still "same node". + // Hence, rules from depth - 2 or above should be applied. + // root is special: is always applied. + return holder.getDepth() == 0 || depth > holder.getDepth() + 1; + } + @Override public boolean equals(Object obj) { if (this == obj) { @@ -304,9 +329,7 @@ public int hashCode() { } protected static class Key { - private final Artifact artifact; - private final int hashCode; Key(Artifact artifact) { @@ -338,4 +361,22 @@ public String toString() { return String.valueOf(artifact); } } + + protected static class Holder { + private final int depth; + private final T value; + + Holder(int depth, T value) { + this.depth = depth; + this.value = requireNonNull(value); + } + + public int getDepth() { + return depth; + } + + public T getValue() { + return value; + } + } } diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java index 4157c5fcb..cdf82db5b 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/ClassicDependencyManager.java @@ -31,6 +31,8 @@ * A dependency manager that mimics the way Maven 2.x works. This manager was used throughout all Maven 3.x versions. *

* This manager has {@code deriveUntil=2} and {@code applyFrom=2}. + *

+ * Note regarding transitivity: it is broken, and should not be used. */ public final class ClassicDependencyManager extends AbstractDependencyManager { /** @@ -65,13 +67,12 @@ private ClassicDependencyManager( int depth, int deriveUntil, int applyFrom, - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - SystemDependencyScope systemDependencyScope, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions, + SystemDependencyScope systemDependencyScope) { super( depth, deriveUntil, @@ -81,8 +82,7 @@ private ClassicDependencyManager( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } @Override @@ -91,25 +91,18 @@ public DependencyManager deriveChildManager(DependencyCollectionContext context) // Removing this IF makes one IT fail here (read comment above): // https://github.com/apache/maven-integration-testing/blob/b4e8fd52b99a058336f9c7c5ec44fdbc1427759c/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng4720DependencyManagementExclusionMergeTest.java#L67 if (depth == 1) { - return newInstance( - managedVersions, - managedScopes, - managedOptionals, - managedLocalPaths, - managedExclusions, - currentContext); + return newInstance(managedVersions, managedScopes, managedOptionals, managedLocalPaths, managedExclusions); } return super.deriveChildManager(context); } @Override protected DependencyManager newInstance( - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions) { return new ClassicDependencyManager( depth + 1, deriveUntil, @@ -119,7 +112,6 @@ protected DependencyManager newInstance( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } } diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java index 1f31713d9..9c1f8bb2d 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/DefaultDependencyManager.java @@ -21,7 +21,6 @@ import java.util.Collection; import java.util.Map; -import org.eclipse.aether.collection.DependencyCollectionContext; import org.eclipse.aether.collection.DependencyManager; import org.eclipse.aether.graph.Exclusion; import org.eclipse.aether.scope.ScopeManager; @@ -59,13 +58,12 @@ private DefaultDependencyManager( int depth, int deriveUntil, int applyFrom, - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - SystemDependencyScope systemDependencyScope, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions, + SystemDependencyScope systemDependencyScope) { super( depth, deriveUntil, @@ -75,18 +73,16 @@ private DefaultDependencyManager( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } @Override protected DependencyManager newInstance( - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions) { return new DefaultDependencyManager( depth + 1, deriveUntil, @@ -96,7 +92,6 @@ protected DependencyManager newInstance( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } } diff --git a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java index 3bc86a45b..e9c87c210 100644 --- a/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java +++ b/maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java @@ -21,7 +21,6 @@ import java.util.Collection; import java.util.Map; -import org.eclipse.aether.collection.DependencyCollectionContext; import org.eclipse.aether.collection.DependencyManager; import org.eclipse.aether.graph.Exclusion; import org.eclipse.aether.scope.ScopeManager; @@ -56,13 +55,12 @@ private TransitiveDependencyManager( int depth, int deriveUntil, int applyFrom, - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - SystemDependencyScope systemDependencyScope, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions, + SystemDependencyScope systemDependencyScope) { super( depth, deriveUntil, @@ -72,18 +70,16 @@ private TransitiveDependencyManager( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } @Override protected DependencyManager newInstance( - Map managedVersions, - Map managedScopes, - Map managedOptionals, - Map managedLocalPaths, - Map> managedExclusions, - DependencyCollectionContext currentContext) { + Map> managedVersions, + Map> managedScopes, + Map> managedOptionals, + Map> managedLocalPaths, + Map>>> managedExclusions) { return new TransitiveDependencyManager( depth + 1, deriveUntil, @@ -93,7 +89,6 @@ protected DependencyManager newInstance( managedOptionals, managedLocalPaths, managedExclusions, - systemDependencyScope, - currentContext); + systemDependencyScope); } } 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 fa3074c8f..41f1df8cd 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 @@ -222,7 +222,8 @@ void testTransitive() { mngt = manager.manageDependency(new Dependency(B1, null)); assertNotNull(mngt); assertEquals(Boolean.TRUE, mngt.getOptional()); - assertEquals(B2.getVersion(), mngt.getVersion()); + // DO NOT APPLY ONTO ITSELF + // assertEquals(B2.getVersion(), mngt.getVersion()); mngt = manager.manageDependency(new Dependency(C1, null)); assertNotNull(mngt); assertEquals(mngt.getScope(), "newscope");