You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "cstamas (via GitHub)" <gi...@apache.org> on 2026/03/27 15:06:25 UTC
[PR] Advised and Enforced Management [maven-resolver]
cstamas opened a new pull request, #1815:
URL: https://github.com/apache/maven-resolver/pull/1815
Originally, dependency management marked a "subject" as "managed", it was 0/1 true/false only. This PR now adds ability to mark management as "advised" (may be subjected to further management) or "enforced" (equivalent of previously "managed").
Changes:
* `DependencyManager` applies management as "enforced", if they come from root (ie. project POM), otherwise as "advice" (this latter applies to transitive managers only, as for example classic manager have no data from "deeper levels").
* `ConflictManager` backs out for _enforced_ scope/optionality.
* This makes the "isInheritedDerived" hack in dependency manager not needed, as now management rule tells the modality of it (is enforced or just advised)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r4255297679
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java:
##########
@@ -725,7 +805,7 @@ private static DependencyNode makeDependencyNode(String groupId, String artifact
}
private static DependencyNode makeDependencyNode(String groupId, String artifactId, String version, String scope) {
- return makeDependencyNode(groupId, artifactId, version, null, "compile");
+ return makeDependencyNode(groupId, artifactId, version, null, scope);
Review Comment:
🐛 **Incidental bug fix** — this 4-arg `makeDependencyNode` overload was previously hardcoding `"compile"` as the scope instead of forwarding the `scope` parameter. This fix is correct and appreciated, but consider noting it in the PR description or commit message since it's unrelated to the main advised/enforced change.
##########
maven-resolver-util/src/test/java/org/eclipse/aether/util/graph/transformer/ConflictResolverTest.java:
##########
@@ -630,6 +672,28 @@ public boolean visitLeave(DependencyNode node) {
assertEquals(1, occurrenceOfSt3.get()); // st3 must be present only once in tree
}
+ @ParameterizedTest
+ @MethodSource("conflictResolverSource")
+ void enforcedOptionalNotDerived(ConflictResolver conflictResolver) throws RepositoryException {
Review Comment:
💡 **Minor coverage gap** — `enforcedOptionalNotDerived` verifies that enforced optional is not derived from the parent. Consider adding a symmetric `advisedOptionalIsDerived` test to verify that an _advised_ optional value **is** correctly derived (inherited from a parent). This would make the test coverage parallel between the two management modes.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#issuecomment-4752980001
Applied PR comments except the last one (DependencyGraphParser), as comment says, we can fiddle with it later. Also fixed since tags to point to `2.0.19` (was `2.0.17`)
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002330643
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -461,7 +461,7 @@ public DependencyManagement manageDependency(Dependency dependency) {
}
Collection<Exclusion> result = new LinkedHashSet<>(dependency.getExclusions());
Review Comment:
This change is here only to adopt new method and avoid usage of old (deprecated one). Also, if you check this PR, only the `scope` and `optional` are _checked_ (in conflict resolver) for "is it managed or not" and contain needed change for "is it enforced or not". Basically _right now_, whether exclusion is advised or enforced, does not matter, as it is taken as granted (again, only scope and optional are being altered by conflict resolver).
Later, after this PR is merged, the platform support will be the one that will use more this feature.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002348702
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java:
##########
@@ -145,30 +145,4 @@ protected DependencyManager newInstance(
managedExclusions,
systemDependencyScope);
}
-
- /**
- * Controls inheritance-based property derivation for scope and optional properties.
- * <p>
- * <strong>Why scope and optional are special:</strong> In dependency graphs, these two properties
- * are subject to inheritance during graph transformation (which is outside ModelBuilder's scope).
- * Therefore, scope and optional are derived only from the root to prevent interference with
- * inheritance logic.
- * </p>
- * <p>
- * <strong>The inheritance problem:</strong> If we managed scope/optional from sources below the root,
- * we would mark nodes as "managed" in the dependency graph. The "managed" flag means "do not touch it,
- * it is as it should be", which would prevent proper inheritance application during later graph
- * transformation, causing nodes to end up with incorrect scope or optional states.
- * </p>
- * <p>
- * <strong>Special case:</strong> The "system" scope has special handling due to its unique path requirements.
- * </p>
- *
- * @return true only at depth 0 (root level) to ensure inheritance-based properties are only
- * derived from the root, false otherwise
- */
- @Override
- protected boolean isInheritedDerived() {
- return depth == 0;
- }
}
Review Comment:
Yes, this is intentional. But, those will be now applied as "advices" and not as "enforced changes". In case of "nearest" winner selection this should not produce any change in case of conflict (same behavior remains), while it may produce proper "transitive dependency management" in other cases. Where it gets interesting is the use of "highest" winner selection strategy.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3001590149
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +223,67 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
This is a hard runtime break for any consumer code that calls `setManagedBits()` on `DefaultDependencyNode`. Even though the interface says `@noextend`, consumers do instantiate and configure `DefaultDependencyNode` directly.
Suggestion: map the bits to the new model for backwards compat:
```java
@Deprecated
public void setManagedBits(int managedBits) {
HashMap<DependencyManagement.Subject, Boolean> subjects = new HashMap<>();
if ((managedBits & DependencyNode.MANAGED_VERSION) != 0) {
subjects.put(DependencyManagement.Subject.VERSION, true);
}
if ((managedBits & DependencyNode.MANAGED_SCOPE) != 0) {
subjects.put(DependencyManagement.Subject.SCOPE, true);
}
if ((managedBits & DependencyNode.MANAGED_OPTIONAL) != 0) {
subjects.put(DependencyManagement.Subject.OPTIONAL, true);
}
if ((managedBits & DependencyNode.MANAGED_PROPERTIES) != 0) {
subjects.put(DependencyManagement.Subject.PROPERTIES, true);
}
if ((managedBits & DependencyNode.MANAGED_EXCLUSIONS) != 0) {
subjects.put(DependencyManagement.Subject.EXCLUSIONS, true);
}
setManagedSubjects(subjects.isEmpty() ? null : subjects);
}
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Replacing typed fields with `Map<Subject, Object>` loses compile-time type safety. A mistyped `managedValues.put(Subject.VERSION, 42)` would compile but fail at runtime with a `ClassCastException` here.
Consider using `EnumMap` (better memory for enum keys) and keeping typed fields alongside the enforcement tracking, or introducing a small typed record:
```java
private record ManagedValue<T>(T value, boolean enforced) {}
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
+ }
- private String version;
-
- private String scope;
-
- private Boolean optional;
-
- private Collection<Exclusion> exclusions;
-
- private Map<String, String> properties;
+ private final Map<Subject, Object> managedValues;
+ private final Map<Subject, Boolean> managedEnforced;
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Two `HashMap` instances are allocated per `DependencyManagement`, even when empty. In large dependency graphs this adds up since `manageDependency()` is called for every dependency at every level.
Consider:
1. Lazy initialization (allocate only when a setter is called with non-null)
2. Use `EnumMap<Subject, ...>` instead — it uses a fixed-size array internally, much more compact for small enums
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DependencyNode.java:
##########
@@ -38,40 +39,49 @@
* @noextend This interface is not intended to be extended by clients.
*/
public interface DependencyNode {
-
/**
* A bit flag indicating the dependency version was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_VERSION = 0x01;
/**
* A bit flag indicating the dependency scope was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_SCOPE = 0x02;
/**
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
These new methods have no `default` implementation. While `@noimplement` signals intent, providing defaults would be more resilient:
```java
default boolean isManagedSubject(DependencyManagement.Subject subject) {
// Backwards-compatible default based on deprecated getManagedBits()
return switch (subject) {
case VERSION -> (getManagedBits() & MANAGED_VERSION) != 0;
case SCOPE -> (getManagedBits() & MANAGED_SCOPE) != 0;
case OPTIONAL -> (getManagedBits() & MANAGED_OPTIONAL) != 0;
case PROPERTIES -> (getManagedBits() & MANAGED_PROPERTIES) != 0;
case EXCLUSIONS -> (getManagedBits() & MANAGED_EXCLUSIONS) != 0;
};
}
default boolean isManagedSubjectEnforced(DependencyManagement.Subject subject) {
return isManagedSubject(subject); // conservative default: all managed = enforced
}
```
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002077236
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
Review Comment:
Maven Resolver 2.x is still Java 8 baseline, so no `record` yet. Using EnumMap
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
+ }
- private String version;
-
- private String scope;
-
- private Boolean optional;
-
- private Collection<Exclusion> exclusions;
-
- private Map<String, String> properties;
+ private final Map<Subject, Object> managedValues;
+ private final Map<Subject, Boolean> managedEnforced;
Review Comment:
Using EnumMap
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002635743
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +223,67 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Thanks, the backwards-compatible mapping looks good. Much better than throwing.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Good point about the Java 8 baseline. `EnumMap` is the right choice here. The remaining `Map<Subject, Object>` type erasure concern is minor given that all setters are type-safe and getters are the only cast points. Acceptable trade-off.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
+ }
- private String version;
-
- private String scope;
-
- private Boolean optional;
-
- private Collection<Exclusion> exclusions;
-
- private Map<String, String> properties;
+ private final Map<Subject, Object> managedValues;
+ private final Map<Subject, Boolean> managedEnforced;
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
`EnumMap` is perfect here — fixed-size internal array, much lighter than `HashMap`. Thanks.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DependencyNode.java:
##########
@@ -38,40 +39,49 @@
* @noextend This interface is not intended to be extended by clients.
*/
public interface DependencyNode {
-
/**
* A bit flag indicating the dependency version was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_VERSION = 0x01;
/**
* A bit flag indicating the dependency scope was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_SCOPE = 0x02;
/**
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
The default implementations look correct. The `default` case in the `isManagedSubject` switch throwing `IllegalArgumentException` is good defensive coding for potential future enum additions. LGTM.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002075785
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +223,67 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
Review Comment:
fixed
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
Review Comment:
Maven Resolver 2.x is still Java 8 baseline, so no `record` yet. Using EnumSet
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002082164
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DependencyNode.java:
##########
@@ -38,40 +39,49 @@
* @noextend This interface is not intended to be extended by clients.
*/
public interface DependencyNode {
-
/**
* A bit flag indicating the dependency version was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_VERSION = 0x01;
/**
* A bit flag indicating the dependency scope was subject to dependency management
*
* @see #getManagedBits()
+ * @deprecated Use {@link #isManagedSubject(DependencyManagement.Subject)} and {@link #isManagedSubjectEnforced(DependencyManagement.Subject)} instead.
*/
+ @Deprecated
int MANAGED_SCOPE = 0x02;
/**
Review Comment:
Applied
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3443247177
##########
maven-resolver-impl/src/main/java/org/eclipse/aether/internal/impl/collect/PremanagedDependency.java:
##########
@@ -92,7 +95,8 @@ public static PremanagedDependency create(
boolean premanagedState) {
DependencyManagement depMngt = depManager != null ? depManager.manageDependency(dependency) : null;
- int managedBits = 0;
+ EnumMap<DependencyManagement.Subject, Boolean> managedSubjects =
+ new EnumMap<>(DependencyManagement.Subject.class);
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
**Performance:** This `EnumMap` is allocated for **every** dependency during collection, even when `depMngt` is null (no management applies). Previously this was `int managedBits = 0` — zero allocation. For a typical enterprise project, the majority of dependencies are unmanaged.
```suggestion
EnumMap<DependencyManagement.Subject, Boolean> managedSubjects = null;
```
Then lazily create inside the `if (depMngt != null)` block, e.g.:
```java
if (managedSubjects == null) {
managedSubjects = new EnumMap<>(DependencyManagement.Subject.class);
}
managedSubjects.put(...);
```
`setManagedSubjects(null)` already handles null correctly, so no other changes needed.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -99,12 +102,25 @@ public DefaultDependencyNode(Artifact artifact) {
* @param node The node to copy, must not be {@code null}.
*/
public DefaultDependencyNode(DependencyNode node) {
- dependency = node.getDependency();
- artifact = node.getArtifact();
- children = new ArrayList<>(0);
+ this.dependency = node.getDependency();
+ this.artifact = node.getArtifact();
+ this.children = new ArrayList<>(0);
setAliases(node.getAliases());
setRequestContext(node.getRequestContext());
- setManagedBits(node.getManagedBits());
+
+ EnumMap<DependencyManagement.Subject, Boolean> managedSubjects =
+ new EnumMap<>(DependencyManagement.Subject.class);
+ for (DependencyManagement.Subject subject : DependencyManagement.Subject.values()) {
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
**Performance — two issues:**
1. `Subject.values()` allocates a new array each call (Java spec requires a fresh clone). Cache it:
```java
private static final DependencyManagement.Subject[] SUBJECTS = DependencyManagement.Subject.values();
```
then use `for (DependencyManagement.Subject subject : SUBJECTS)`.
2. The `EnumMap` is allocated even when the source node has no managed subjects. Guard the allocation:
```java
EnumMap<DependencyManagement.Subject, Boolean> managedSubjects = null;
for (DependencyManagement.Subject subject : SUBJECTS) {
if (node.isManagedSubject(subject)) {
if (managedSubjects == null) {
managedSubjects = new EnumMap<>(DependencyManagement.Subject.class);
}
managedSubjects.put(subject, node.isManagedSubjectEnforced(subject));
}
}
setManagedSubjects(managedSubjects);
```
This eliminates both the `isEmpty()` check and the wasted allocation for unmanaged nodes.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +226,83 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
public int getManagedBits() {
- return managedBits;
+ byte res = 0;
+ if (isManagedSubject(DependencyManagement.Subject.VERSION)) {
+ res |= DependencyNode.MANAGED_VERSION;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.SCOPE)) {
+ res |= DependencyNode.MANAGED_SCOPE;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.OPTIONAL)) {
+ res |= DependencyNode.MANAGED_OPTIONAL;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.PROPERTIES)) {
+ res |= DependencyNode.MANAGED_PROPERTIES;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.EXCLUSIONS)) {
+ res |= DependencyNode.MANAGED_EXCLUSIONS;
+ }
+ return res;
}
- /**
- * Sets a bit field indicating which attributes of this node were subject to dependency management.
- *
- * @param managedBits The bit field indicating the managed attributes or {@code 0} if dependency management wasn't
- * applied.
- */
+ @Deprecated
public void setManagedBits(int managedBits) {
- this.managedBits = (byte) (managedBits & 0x1F);
+ EnumMap<DependencyManagement.Subject, Boolean> subjects = new EnumMap<>(DependencyManagement.Subject.class);
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
**Performance:** Short-circuit for `managedBits == 0` to avoid the wasted `EnumMap` allocation:
```java
@Deprecated
public void setManagedBits(int managedBits) {
if (managedBits == 0) {
setManagedSubjects(null);
return;
}
EnumMap<DependencyManagement.Subject, Boolean> subjects = new EnumMap<>(DependencyManagement.Subject.class);
// ... rest unchanged
```
##########
maven-resolver-api/src/main/java/org/eclipse/aether/collection/DependencyManagement.java:
##########
@@ -29,22 +30,46 @@
* @see DependencyManager#manageDependency(org.eclipse.aether.graph.Dependency)
*/
public final class DependencyManagement {
+ /**
+ * Enumeration of manageable attributes, attributes that can be subjected to dependency management.
+ *
+ * @since 2.0.17
+ */
+ public enum Subject {
+ VERSION,
+ SCOPE,
+ OPTIONAL,
+ EXCLUSIONS,
+ PROPERTIES
+ }
- private String version;
-
- private String scope;
-
- private Boolean optional;
-
- private Collection<Exclusion> exclusions;
-
- private Map<String, String> properties;
+ private final EnumMap<Subject, Object> managedValues;
+ private final EnumMap<Subject, Boolean> managedEnforced;
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
**Performance:** `managedEnforced` only ever stores `true` (if a subject isn't enforced, it's either absent or `false`). An `EnumSet<Subject>` would be a better fit — it's backed by a single `long` for enums with ≤64 constants, so essentially zero overhead vs ~80-100 bytes for the `EnumMap<Subject, Boolean>`.
```java
private final EnumSet<Subject> managedEnforced;
```
Then `setVersion(String version, boolean enforced)` becomes:
```java
if (enforced) {
this.managedEnforced.add(Subject.VERSION);
} else {
this.managedEnforced.remove(Subject.VERSION);
}
```
And `isManagedSubjectEnforced` becomes:
```java
return isManagedSubject(subject) && managedEnforced.contains(subject);
```
This halves the per-`DependencyManagement` allocation cost.
##########
maven-resolver-test-util/src/main/java/org/eclipse/aether/internal/test/util/DependencyGraphParser.java:
##########
@@ -291,16 +293,17 @@ private DependencyNode build(DependencyNode parent, LineContext ctx, boolean isR
DefaultArtifact artifact = new DefaultArtifact(def.coords, def.properties);
Dependency dependency = new Dependency(artifact, def.scope, def.optional);
node = new DefaultDependencyNode(dependency);
- int managedBits = 0;
+ EnumMap<DependencyManagement.Subject, Boolean> managedSubjects =
+ new EnumMap<>(DependencyManagement.Subject.class);
if (def.premanagedScope != null) {
- managedBits |= DependencyNode.MANAGED_SCOPE;
+ managedSubjects.put(DependencyManagement.Subject.SCOPE, true);
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Managed subjects from the parser are hardcoded as `enforced=true`. This means graph definition files (`.txt` test resources) cannot express "advised" management — only programmatic test construction can.
Not blocking, since the existing programmatic tests (`enforcedScopeNotDerived`, `advisedScopeIsDerived`, etc.) cover the key scenarios. But if more complex graph-based test scenarios are planned, the parser format could be extended with a syntax like `scope:test!` (enforced) vs `scope:test` (advised).
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3001593009
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -394,58 +393,59 @@ public DependencyManagement manageDependency(Dependency dependency) {
Key key = new Key(dependency.getArtifact());
if (isApplied()) {
- String version = getManagedVersion(key);
+ AbstractDependencyManager versionOwner = getManagedVersion(key);
// is managed locally by model builder
// apply only rules coming from "higher" levels
- if (version != null) {
+ if (versionOwner != null) {
management = new DependencyManagement();
- management.setVersion(version);
+ management.setVersion(versionOwner.managedVersions.get(key), versionOwner.path.isEmpty());
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
The enforcement determination via `versionOwner.path.isEmpty()` is implicit — `path` is empty when the manager represents the root level. This works but is non-obvious. Consider adding an `isRoot()` helper or at least a comment explaining the semantics.
Also, this means the enforcement is determined by *where the management entry was defined* (root vs transitive), not by an explicit flag on the entry. Is that always correct? A parent POM's `<dependencyManagement>` inherited into a child POM would still be at depth 0 and thus enforced — which seems correct. But worth documenting explicitly.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -461,7 +461,7 @@ public DependencyManagement manageDependency(Dependency dependency) {
}
Collection<Exclusion> result = new LinkedHashSet<>(dependency.getExclusions());
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Exclusions are always set with `enforced=false`. Is this intentional? Root-level exclusion management (`<dependencyManagement>` in the project POM) arguably should be enforced the same way as scope/version/optional from root level. If a user explicitly declares exclusions in their project's dependency management, the conflict resolver should arguably respect that.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +223,67 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
public int getManagedBits() {
- return managedBits;
+ byte res = 0;
+ if (isManagedSubject(DependencyManagement.Subject.VERSION)) {
+ res |= DependencyNode.MANAGED_VERSION;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.SCOPE)) {
+ res |= DependencyNode.MANAGED_SCOPE;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.OPTIONAL)) {
+ res |= DependencyNode.MANAGED_OPTIONAL;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.PROPERTIES)) {
+ res |= DependencyNode.MANAGED_PROPERTIES;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.EXCLUSIONS)) {
+ res |= DependencyNode.MANAGED_EXCLUSIONS;
+ }
+ return res;
}
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Subtle inconsistency with `DependencyManagement.isManagedSubjectEnforced()` which first checks `isManagedSubject(subject)`:
```java
// DependencyManagement:
return isManagedSubject(subject) && managedEnforced.getOrDefault(subject, false);
// DefaultDependencyNode:
return managedSubjects.getOrDefault(subject, false);
```
Both return `false` for unmanaged subjects today, but the semantic contract differs. Suggest aligning by adding the `containsKey` guard here too, or documenting the contract explicitly.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java:
##########
@@ -145,30 +145,4 @@ protected DependencyManager newInstance(
managedExclusions,
systemDependencyScope);
}
-
- /**
- * Controls inheritance-based property derivation for scope and optional properties.
- * <p>
- * <strong>Why scope and optional are special:</strong> In dependency graphs, these two properties
- * are subject to inheritance during graph transformation (which is outside ModelBuilder's scope).
- * Therefore, scope and optional are derived only from the root to prevent interference with
- * inheritance logic.
- * </p>
- * <p>
- * <strong>The inheritance problem:</strong> If we managed scope/optional from sources below the root,
- * we would mark nodes as "managed" in the dependency graph. The "managed" flag means "do not touch it,
- * it is as it should be", which would prevent proper inheritance application during later graph
- * transformation, causing nodes to end up with incorrect scope or optional states.
- * </p>
- * <p>
- * <strong>Special case:</strong> The "system" scope has special handling due to its unique path requirements.
- * </p>
- *
- * @return true only at depth 0 (root level) to ensure inheritance-based properties are only
- * derived from the root, false otherwise
- */
- @Override
- protected boolean isInheritedDerived() {
- return depth == 0;
- }
}
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
The removal of `isInheritedDerived()` is the most consequential behavioral change in this PR. Previously, `TransitiveDependencyManager` overrode this to return `depth == 0`, gating scope/optional management to root-only. Now, scope/optional are always collected into `managedScopes`/`managedOptionals` at all levels, but marked as "advised" (not enforced) when coming from non-root.
This changes the semantics: before, transitive scope/optional management entries were **not applied at all** (they were skipped in `deriveChildManager`). Now they **are applied** but marked as non-enforced, so the conflict resolver can still override them.
This is a meaningful behavioral difference — a transitive dependency management entry for scope would now *initially set* the scope (which it previously wouldn't), and only later be potentially overridden by the conflict resolver. Is this the intended behavior? It seems correct in principle but should have test coverage.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "cstamas (via GitHub)" <gi...@apache.org>.
cstamas commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002330643
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -461,7 +461,7 @@ public DependencyManagement manageDependency(Dependency dependency) {
}
Collection<Exclusion> result = new LinkedHashSet<>(dependency.getExclusions());
Review Comment:
This change is here only to adopt new method and avoid usage of old (deprecated one). Also, if you check this PR, only the `scope` and `optional` are _checked_ (in conflict resolver) for "is it managed or not" and contain needed change for "is it enforced or not".
Later, after this PR is merged, the platform support will be the one that will use more this feature.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Advised and Enforced Management [maven-resolver]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1815:
URL: https://github.com/apache/maven-resolver/pull/1815#discussion_r3002637239
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -461,7 +461,7 @@ public DependencyManagement manageDependency(Dependency dependency) {
}
Collection<Exclusion> result = new LinkedHashSet<>(dependency.getExclusions());
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Thanks for the explanation and for switching to `enforced=true`. That makes sense — exclusions are additive and should be respected as-is. Good that the conflict resolver does not alter exclusions currently, but marking them as enforced is the correct default for future-proofing.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/TransitiveDependencyManager.java:
##########
@@ -145,30 +145,4 @@ protected DependencyManager newInstance(
managedExclusions,
systemDependencyScope);
}
-
- /**
- * Controls inheritance-based property derivation for scope and optional properties.
- * <p>
- * <strong>Why scope and optional are special:</strong> In dependency graphs, these two properties
- * are subject to inheritance during graph transformation (which is outside ModelBuilder's scope).
- * Therefore, scope and optional are derived only from the root to prevent interference with
- * inheritance logic.
- * </p>
- * <p>
- * <strong>The inheritance problem:</strong> If we managed scope/optional from sources below the root,
- * we would mark nodes as "managed" in the dependency graph. The "managed" flag means "do not touch it,
- * it is as it should be", which would prevent proper inheritance application during later graph
- * transformation, causing nodes to end up with incorrect scope or optional states.
- * </p>
- * <p>
- * <strong>Special case:</strong> The "system" scope has special handling due to its unique path requirements.
- * </p>
- *
- * @return true only at depth 0 (root level) to ensure inheritance-based properties are only
- * derived from the root, false otherwise
- */
- @Override
- protected boolean isInheritedDerived() {
- return depth == 0;
- }
}
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Thanks for clarifying. So the key behavioral difference is:
- **Before:** Transitive scope/optional management entries were silently skipped (never applied)
- **After:** They are applied as "advice" — the initial scope/optional is set, but the conflict resolver can still override them during graph transformation
This is a meaningful improvement, especially for the "highest" winner selection strategy where transitive management entries should have influence. For the "nearest" strategy the end result should be the same since the conflict resolver will still derive scope/optional normally.
I think this behavioral change warrants at least one dedicated test to document and protect the expected behavior — see my overall comment about missing test coverage.
##########
maven-resolver-util/src/main/java/org/eclipse/aether/util/graph/manager/AbstractDependencyManager.java:
##########
@@ -394,58 +393,59 @@ public DependencyManagement manageDependency(Dependency dependency) {
Key key = new Key(dependency.getArtifact());
if (isApplied()) {
- String version = getManagedVersion(key);
+ AbstractDependencyManager versionOwner = getManagedVersion(key);
// is managed locally by model builder
// apply only rules coming from "higher" levels
- if (version != null) {
+ if (versionOwner != null) {
management = new DependencyManagement();
- management.setVersion(version);
+ management.setVersion(versionOwner.managedVersions.get(key), versionOwner.path.isEmpty());
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
The new `isRootManager()` helper method added in the latest commit is exactly what I had in mind. Much clearer than the raw `path.isEmpty()` check. Thanks.
##########
maven-resolver-api/src/main/java/org/eclipse/aether/graph/DefaultDependencyNode.java:
##########
@@ -201,34 +223,67 @@ public void setVersion(Version version) {
this.version = version;
}
+ @Override
public void setScope(String scope) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setScope(scope);
}
+ @Override
public void setOptional(Boolean optional) {
if (dependency == null) {
throw new IllegalStateException("node does not have a dependency");
}
dependency = dependency.setOptional(optional);
}
+ @Override
public int getManagedBits() {
- return managedBits;
+ byte res = 0;
+ if (isManagedSubject(DependencyManagement.Subject.VERSION)) {
+ res |= DependencyNode.MANAGED_VERSION;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.SCOPE)) {
+ res |= DependencyNode.MANAGED_SCOPE;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.OPTIONAL)) {
+ res |= DependencyNode.MANAGED_OPTIONAL;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.PROPERTIES)) {
+ res |= DependencyNode.MANAGED_PROPERTIES;
+ }
+ if (isManagedSubject(DependencyManagement.Subject.EXCLUSIONS)) {
+ res |= DependencyNode.MANAGED_EXCLUSIONS;
+ }
+ return res;
}
Review Comment:
_Claude Code on behalf of Guillaume Nodet_
Fixed in the "Align semantic" commit — `DefaultDependencyNode.isManagedSubjectEnforced()` now guards with `isManagedSubject(subject) &&` just like `DependencyManagement`. Thanks.
--
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: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org