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