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