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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
