gnodet-bot commented on code in PR #13570:
URL: https://github.com/apache/maven/pull/13570#discussion_r4322074167
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -89,15 +92,100 @@ public abstract static class DefaultExtensibleEnumRegistry<
implements ExtensibleEnumRegistry<T> {
protected final Map<String, T> values;
+ private final Lookup lookup;
+ private final Class<P> providerType;
+ @SafeVarargs
public DefaultExtensibleEnumRegistry(List<P> providers, T...
builtinValues) {
+ this(null, null, providers, builtinValues);
+ }
+
+ @SafeVarargs
+ public DefaultExtensibleEnumRegistry(
+ Lookup lookup, Class<P> providerType, List<P> providers, T...
builtinValues) {
+ this.lookup = lookup;
+ this.providerType = providerType;
values = Stream.concat(Stream.of(builtinValues),
providers.stream().flatMap(p -> p.provides().stream()))
.collect(Collectors.toUnmodifiableMap(t ->
t.id().toLowerCase(Locale.ROOT), t -> t));
}
+ /**
+ * Looks up an enum value by id, checking dynamic providers before the
static registry.
+ *
+ * <p><b>Cardinality assumption:</b> when dynamic lookup is enabled,
this method iterates
+ * all registered providers and all their values on every call —
O(n·m) where {@code n} is
+ * the number of providers and {@code m} is the average number of
values per provider.
+ * Both are expected to be O(tens) in practice (a handful of SPI
providers, each contributing
+ * a small set of values). This method is <em>not</em> designed for
cardinalities in the
+ * thousands; callers that need to look up the same value repeatedly
in a tight loop should
+ * cache the result themselves.</p>
+ *
+ * <p><b>Classloader visibility:</b> providers contributed by project
extensions are only
+ * visible when the current thread's context classloader can load the
provider's class.
+ * This prevents extension-scoped providers from leaking across
reactor modules during
+ * parallel builds.</p>
+ *
+ * @param id the enum value id, must not be {@code null}
+ * @return the matching value, or {@code Optional.empty()} if not found
+ */
@Override
public Optional<T> lookup(String id) {
- return Optional.ofNullable(values.get(requireNonNull(id,
"id").toLowerCase(Locale.ROOT)));
+ String key = requireNonNull(id, "id").toLowerCase(Locale.ROOT);
+ if (lookup != null && providerType != null) {
+ ClassLoader tccl =
Thread.currentThread().getContextClassLoader();
+ Optional<T> result = lookup.lookupList(providerType).stream()
+ .filter(p -> isVisibleFrom(p, tccl))
+ .flatMap(p -> p.provides().stream())
+ .filter(t ->
key.equals(t.id().toLowerCase(Locale.ROOT)))
+ .findFirst();
+ if (result.isPresent()) {
+ return result;
+ }
+ }
+ return Optional.ofNullable(values.get(key));
+ }
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
+ * (realms) that are only imported into the project realm that
declared the extension.
+ */
+ private boolean isVisibleFrom(P provider, ClassLoader classLoader) {
+ if (classLoader == null) {
+ return true;
+ }
+ Class<?> providerClass = provider.getClass();
+ ClassLoader providerCl = providerClass.getClassLoader();
+ if (providerCl == null) {
+ return true;
+ }
+ // Check loadClass — works for standard classloaders and
ClassRealm with package-level exports
+ try {
+ if (classLoader.loadClass(providerClass.getName()) ==
providerClass) {
+ return true;
+ }
+ } catch (ClassNotFoundException e) {
Review Comment:
⚠️ **Still open (prior finding):** `ClassLoader.loadClass()` can throw
`NoClassDefFoundError` (a `LinkageError`) when the class was previously loaded
but its transitive dependencies failed to link. Only `ClassNotFoundException`
is caught — a `LinkageError` escapes unchecked and aborts the entire `lookup()`
call with a stack trace instead of a clean not-found result.
```suggestion
} catch (ClassNotFoundException | LinkageError e) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -89,15 +92,100 @@ public abstract static class DefaultExtensibleEnumRegistry<
implements ExtensibleEnumRegistry<T> {
protected final Map<String, T> values;
+ private final Lookup lookup;
+ private final Class<P> providerType;
+ @SafeVarargs
public DefaultExtensibleEnumRegistry(List<P> providers, T...
builtinValues) {
+ this(null, null, providers, builtinValues);
+ }
+
+ @SafeVarargs
+ public DefaultExtensibleEnumRegistry(
+ Lookup lookup, Class<P> providerType, List<P> providers, T...
builtinValues) {
+ this.lookup = lookup;
+ this.providerType = providerType;
values = Stream.concat(Stream.of(builtinValues),
providers.stream().flatMap(p -> p.provides().stream()))
.collect(Collectors.toUnmodifiableMap(t ->
t.id().toLowerCase(Locale.ROOT), t -> t));
}
+ /**
+ * Looks up an enum value by id, checking dynamic providers before the
static registry.
+ *
+ * <p><b>Cardinality assumption:</b> when dynamic lookup is enabled,
this method iterates
+ * all registered providers and all their values on every call —
O(n·m) where {@code n} is
+ * the number of providers and {@code m} is the average number of
values per provider.
+ * Both are expected to be O(tens) in practice (a handful of SPI
providers, each contributing
+ * a small set of values). This method is <em>not</em> designed for
cardinalities in the
+ * thousands; callers that need to look up the same value repeatedly
in a tight loop should
+ * cache the result themselves.</p>
+ *
+ * <p><b>Classloader visibility:</b> providers contributed by project
extensions are only
+ * visible when the current thread's context classloader can load the
provider's class.
+ * This prevents extension-scoped providers from leaking across
reactor modules during
+ * parallel builds.</p>
+ *
+ * @param id the enum value id, must not be {@code null}
+ * @return the matching value, or {@code Optional.empty()} if not found
+ */
@Override
public Optional<T> lookup(String id) {
- return Optional.ofNullable(values.get(requireNonNull(id,
"id").toLowerCase(Locale.ROOT)));
+ String key = requireNonNull(id, "id").toLowerCase(Locale.ROOT);
+ if (lookup != null && providerType != null) {
+ ClassLoader tccl =
Thread.currentThread().getContextClassLoader();
+ Optional<T> result = lookup.lookupList(providerType).stream()
+ .filter(p -> isVisibleFrom(p, tccl))
+ .flatMap(p -> p.provides().stream())
+ .filter(t ->
key.equals(t.id().toLowerCase(Locale.ROOT)))
+ .findFirst();
+ if (result.isPresent()) {
+ return result;
+ }
+ }
+ return Optional.ofNullable(values.get(key));
+ }
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
+ * (realms) that are only imported into the project realm that
declared the extension.
+ */
+ private boolean isVisibleFrom(P provider, ClassLoader classLoader) {
+ if (classLoader == null) {
+ return true;
+ }
+ Class<?> providerClass = provider.getClass();
+ ClassLoader providerCl = providerClass.getClassLoader();
+ if (providerCl == null) {
+ return true;
+ }
+ // Check loadClass — works for standard classloaders and
ClassRealm with package-level exports
+ try {
+ if (classLoader.loadClass(providerClass.getName()) ==
providerClass) {
+ return true;
+ }
+ } catch (ClassNotFoundException e) {
+ // Not loadable by name (hidden class or realm-ID import) —
check further
+ }
+ // Walk the Java classloader parent chain
+ for (ClassLoader cl = classLoader; cl != null; cl =
cl.getParent()) {
+ if (cl == providerCl) {
+ return true;
+ }
+ }
+ // Check ClassRealm foreign imports via reflection — handles the
common case
+ // where Maven imports extension realms using the realm ID rather
than package names
+ return isImportedRealm(classLoader, providerCl);
+ }
+
+ private static boolean isImportedRealm(ClassLoader classLoader,
ClassLoader providerCl) {
+ try {
+ Method method =
classLoader.getClass().getMethod("getImportRealms");
+ Collection<?> importedRealms = (Collection<?>)
method.invoke(classLoader);
+ return importedRealms.contains(providerCl);
+ } catch (ReflectiveOperationException | ClassCastException e) {
Review Comment:
⚠️ **New issue:** `getMethod()` on line 183 can throw `SecurityException` (a
`RuntimeException`, not a `ReflectiveOperationException`). The catch block only
covers `ReflectiveOperationException | ClassCastException` — a
`SecurityException` from a SecurityManager denying reflective access escapes
unchecked through the stream `filter()` lambda in `lookup()`, aborting the
entire provider resolution rather than safely returning `false`.
SecurityManager is deprecated in JDK 17+ but not yet removed; Maven 4 still
runs in CI-hardened and enterprise environments where it may be active.
```suggestion
} catch (ReflectiveOperationException | ClassCastException |
SecurityException e) {
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/ExtensibleEnumRegistries.java:
##########
@@ -89,15 +92,100 @@ public abstract static class DefaultExtensibleEnumRegistry<
implements ExtensibleEnumRegistry<T> {
protected final Map<String, T> values;
+ private final Lookup lookup;
+ private final Class<P> providerType;
+ @SafeVarargs
public DefaultExtensibleEnumRegistry(List<P> providers, T...
builtinValues) {
+ this(null, null, providers, builtinValues);
+ }
+
+ @SafeVarargs
+ public DefaultExtensibleEnumRegistry(
+ Lookup lookup, Class<P> providerType, List<P> providers, T...
builtinValues) {
+ this.lookup = lookup;
+ this.providerType = providerType;
values = Stream.concat(Stream.of(builtinValues),
providers.stream().flatMap(p -> p.provides().stream()))
.collect(Collectors.toUnmodifiableMap(t ->
t.id().toLowerCase(Locale.ROOT), t -> t));
}
+ /**
+ * Looks up an enum value by id, checking dynamic providers before the
static registry.
+ *
+ * <p><b>Cardinality assumption:</b> when dynamic lookup is enabled,
this method iterates
+ * all registered providers and all their values on every call —
O(n·m) where {@code n} is
+ * the number of providers and {@code m} is the average number of
values per provider.
+ * Both are expected to be O(tens) in practice (a handful of SPI
providers, each contributing
+ * a small set of values). This method is <em>not</em> designed for
cardinalities in the
+ * thousands; callers that need to look up the same value repeatedly
in a tight loop should
+ * cache the result themselves.</p>
+ *
+ * <p><b>Classloader visibility:</b> providers contributed by project
extensions are only
+ * visible when the current thread's context classloader can load the
provider's class.
+ * This prevents extension-scoped providers from leaking across
reactor modules during
+ * parallel builds.</p>
+ *
+ * @param id the enum value id, must not be {@code null}
+ * @return the matching value, or {@code Optional.empty()} if not found
+ */
@Override
public Optional<T> lookup(String id) {
- return Optional.ofNullable(values.get(requireNonNull(id,
"id").toLowerCase(Locale.ROOT)));
+ String key = requireNonNull(id, "id").toLowerCase(Locale.ROOT);
+ if (lookup != null && providerType != null) {
+ ClassLoader tccl =
Thread.currentThread().getContextClassLoader();
+ Optional<T> result = lookup.lookupList(providerType).stream()
+ .filter(p -> isVisibleFrom(p, tccl))
+ .flatMap(p -> p.provides().stream())
+ .filter(t ->
key.equals(t.id().toLowerCase(Locale.ROOT)))
+ .findFirst();
+ if (result.isPresent()) {
+ return result;
+ }
+ }
+ return Optional.ofNullable(values.get(key));
+ }
+
+ /**
+ * Checks whether a provider is visible from the given classloader.
+ * Providers from project extensions are loaded by extension-specific
classloaders
+ * (realms) that are only imported into the project realm that
declared the extension.
+ */
+ private boolean isVisibleFrom(P provider, ClassLoader classLoader) {
+ if (classLoader == null) {
+ return true;
+ }
+ Class<?> providerClass = provider.getClass();
+ ClassLoader providerCl = providerClass.getClassLoader();
+ if (providerCl == null) {
+ return true;
+ }
+ // Check loadClass — works for standard classloaders and
ClassRealm with package-level exports
+ try {
+ if (classLoader.loadClass(providerClass.getName()) ==
providerClass) {
+ return true;
+ }
+ } catch (ClassNotFoundException e) {
+ // Not loadable by name (hidden class or realm-ID import) —
check further
+ }
+ // Walk the Java classloader parent chain
+ for (ClassLoader cl = classLoader; cl != null; cl =
cl.getParent()) {
+ if (cl == providerCl) {
+ return true;
+ }
+ }
+ // Check ClassRealm foreign imports via reflection — handles the
common case
+ // where Maven imports extension realms using the realm ID rather
than package names
+ return isImportedRealm(classLoader, providerCl);
+ }
+
+ private static boolean isImportedRealm(ClassLoader classLoader,
ClassLoader providerCl) {
+ try {
+ Method method =
classLoader.getClass().getMethod("getImportRealms");
Review Comment:
💡 **Still open (prior finding):**
`classLoader.getClass().getMethod("getImportRealms")` is resolved via
reflection on every `isVisibleFrom()` invocation — once per SPI provider per
`lookup()` call. Reflective method lookup is significantly more expensive than
a direct call. Cache the resolved `Method` per classloader class using
`ClassValue<Optional<Method>>`:
```java
private static final ClassValue<Optional<Method>> GET_IMPORT_REALMS = new
ClassValue<>() {
@Override
protected Optional<Method> computeValue(Class<?> type) {
try {
return Optional.of(type.getMethod("getImportRealms"));
} catch (NoSuchMethodException | SecurityException e) {
return Optional.empty();
}
}
};
private static boolean isImportedRealm(ClassLoader classLoader, ClassLoader
providerCl) {
return GET_IMPORT_REALMS.get(classLoader.getClass())
.map(m -> {
try {
Collection<?> realms = (Collection<?>)
m.invoke(classLoader);
return realms.contains(providerCl);
} catch (ReflectiveOperationException | ClassCastException
e) {
return false;
}
})
.orElse(false);
}
```
--
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]
