Advised and Enforced Management

[cstamas]

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)

[gnodet]

Claude Code on behalf of Guillaume Nodet

In-depth review of "Advised and Enforced Management"

Thanks for working on this @cstamas. The concept of distinguishing "advised" vs "enforced" management is a valuable improvement — it addresses a real pain point where transitive dependency management would mark nodes as "managed" and prevent proper scope/optional inheritance during graph transformation. Here's a detailed review.

---

Summary

This PR replaces the binary "managed or not" bit field (managedBits) with a richer model that tracks both whether and how (advised vs enforced) each attribute is managed. The key behavioral change is:

• Root-level (project POM) management → enforced (conflict resolver won't override)
• Transitive management → advised (conflict resolver may still derive scope/optional)
• This eliminates the isInheritedDerived() hack in TransitiveDependencyManager

---

Concerns

1. Breaking change: setManagedBits() throws at runtime

DefaultDependencyNode.setManagedBits(int) now throws IllegalArgumentException("bits are not supported"). While @noextend/@noimplement hints exist on the interface, DefaultDependencyNode is a public API class and consumers (Maven plugins, IDE integrations like IntelliJ/Eclipse, test frameworks) may call setManagedBits() on existing node instances. A runtime exception is a hard break. Consider instead:

• Mapping the bits to the new managedSubjects map (all as enforced for backwards compat)
• Or at minimum, deprecating with a no-op + warning log rather than throwing

2. Type safety regression in DependencyManagement

The refactoring from typed fields (String version, String scope, Boolean optional, etc.) to Map<Subject, Object> loses compile-time type safety. The getters now rely on unchecked casts:

return (String) managedValues.get(Subject.VERSION);
return (Collection<Exclusion>) managedValues.get(Subject.EXCLUSIONS);

This means a bug like managedValues.put(Subject.VERSION, 42) would compile but fail at runtime. Consider keeping the typed fields alongside the enforcement map, or using a typed wrapper/record per Subject.

3. No new tests for the core semantic change

The PR introduces the advised/enforced distinction but adds no new test cases exercising this behavior. The existing testDependencyManagement_VerboseMode still tests via the deprecated getManagedBits() API. Tests should verify:

• That transitive management produces "advised" (enforced=false) management
• That root-level management produces "enforced" (enforced=true) management
• That ClassicConflictResolver and PathConflictResolver correctly skip scope/optional derivation for enforced nodes but apply it for advised nodes
• That the TestDependencyManager in tests exercises both paths

4. New interface methods without default implementations

DependencyNode.isManagedSubject() and isManagedSubjectEnforced() are added without default methods. While @noimplement signals intent, adding default implementations (delegating to getManagedBits() for isManagedSubject and returning false for isManagedSubjectEnforced) would be more resilient and maintain backwards compat for any existing (even if unsupported) implementations.

5. Inconsistent semantics of isManagedSubjectEnforced between classes

In DependencyManagement:

public boolean isManagedSubjectEnforced(Subject subject) {
    return isManagedSubject(subject) && managedEnforced.getOrDefault(subject, false);
}

In DefaultDependencyNode:

public boolean isManagedSubjectEnforced(DependencyManagement.Subject subject) {
    return managedSubjects.getOrDefault(subject, false);
}

The DefaultDependencyNode version doesn't first check isManagedSubject(). For an unmanaged subject, both return false, but the semantics diverge: one says "not managed so not enforced", the other says "default to false". This could lead to subtle bugs if the map ever contained null values. Suggest aligning the implementations.

6. Method rename is a binary-breaking change

createDependencyNode(...) (the cycle variant) → createDependencyNodeCycle(...) in DependencyCollectorDelegate. This is a protected method, and while DependencyCollectorDelegate is internal, any custom collector subclass will break at compile time. The non-static promotion of createDependencyNode (removing static) is also a binary-level change.

7. Memory overhead

DependencyManagement() now always allocates two HashMap instances even when empty. In large dependency graphs, manageDependency() is called for every dependency at every level. Most calls will produce a DependencyManagement with 0-2 managed subjects. Consider lazy initialization or using EnumMap (which is optimized for enum keys and uses less memory than HashMap).

8. Missing @param in Javadoc

The new interface methods isManagedSubject and isManagedSubjectEnforced in DependencyNode.java are missing @param subject documentation.

---

Minor observations

• The @Override annotations added to DefaultDependencyNode methods that already existed (like getChildren(), getDependency(), etc.) are a nice cleanup but are unrelated to this PR's purpose — consider splitting them out.
• The copy constructor in DefaultDependencyNode now iterates all Subject.values() to build the managed subjects map. This is fine for 5 enum values but is more allocation-heavy than the previous managedBits = (byte)(...) one-liner. Not a real concern, just noting the trade-off.
• The managedSubjects field in DefaultDependencyNode is initialized to Collections.emptyMap() but setManagedSubjects(null) also sets it to Collections.emptyMap(). This is consistent but the field could just be declared without the initializer since the constructor paths all set it.

---

Questions for discussion

1. Exclusions are always "not enforced" (management.setExclusions(result, false) in AbstractDependencyManager). Is this intentional? If root-level exclusion management should also be enforced, this might need the same path.isEmpty() check.

2. Properties from system path handling are always "not enforced" (management.setProperties(properties, false)). Same question — is this by design?

3. The determination of "enforced" via versionOwner.path.isEmpty() is clever but implicit. Would it be clearer to have an explicit isRoot() method or pass the enforcement flag more directly?

[gnodet]

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:

private record ManagedValue<T>(T value, boolean enforced) {}

[cstamas]

Maven Resolver 2.x is still Java 8 baseline, so no record yet. Using EnumMap

[gnodet]

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.

[gnodet]

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

[cstamas]

Using EnumMap

[gnodet]

Claude Code on behalf of Guillaume Nodet

EnumMap is perfect here — fixed-size internal array, much lighter than HashMap. Thanks.

[gnodet]

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.

[gnodet]

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.

[gnodet]

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.

[cstamas]

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.

[gnodet]

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.

[gnodet]

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.

[cstamas]

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.

[gnodet]

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.

[gnodet]

Claude Code on behalf of Guillaume Nodet

Follow-up review after updates

Thanks @cstamas for the quick and thorough iteration. The updates address most of the original concerns well:

Resolved:

• setManagedBits() now maps bits to the new model instead of throwing — backwards compat preserved
• EnumMap instead of HashMap — better memory footprint (good call on the Java 8 baseline, no records)
• default methods on DependencyNode interface — proper fallback for any existing implementations
• isManagedSubjectEnforced semantics aligned between DependencyManagement and DefaultDependencyNode
• isRootManager() helper method — much clearer than raw path.isEmpty()
• Exclusions changed to enforced=true — correct default
• managedSubjects uses null instead of empty map — good optimization

Still outstanding — test coverage:

The PR still has no new tests for the core advised/enforced distinction. This is the single remaining concern. Specifically, I'd like to see tests verifying:

1. Root-level management produces enforced results: When DependencyManager manages scope/optional from root, isManagedSubjectEnforced() returns true on the resulting DependencyNode.

2. Transitive management produces advised results: When TransitiveDependencyManager manages scope/optional from a non-root level, isManagedSubjectEnforced() returns false.

3. Conflict resolver respects enforcement: ClassicConflictResolver (and PathConflictResolver) should skip scope derivation for enforced nodes but apply it for advised nodes. This is the critical end-to-end behavior change.

4. Backwards compat round-trip: setManagedBits(bits) → isManagedSubject() / isManagedSubjectEnforced() returns correct values (all enforced by default).

Even a single integration test exercising scenario (3) would significantly reduce the risk of regressions. The behavioral change from "skip transitive scope/optional management entirely" to "apply as advice, let conflict resolver override" is subtle enough to warrant explicit test documentation.

Other than the missing tests, LGTM.