gnodet-bot commented on code in PR #13494:
URL: https://github.com/apache/maven/pull/13494#discussion_r4283557145
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultMavenPluginManager.java:
##########
@@ -378,22 +379,20 @@ public void setupPluginRealm(
foreignImports,
filter,
project.getRemotePluginRepositories(),
- session.getRepositorySession());
+ session.getRepositorySession(),
+ pluginDescriptor.isModular());
PluginRealmCache.CacheRecord cacheRecord =
pluginRealmCache.get(cacheKey, () -> {
createPluginRealm(pluginDescriptor, session, parent,
foreignImports, filter);
return new PluginRealmCache.CacheRecord(
- pluginDescriptor.getClassRealm(),
- pluginDescriptor.getArtifacts(),
- pluginDescriptor.getDependencyNode());
+ pluginDescriptor.getClassRealm(),
pluginDescriptor.getArtifacts());
Review Comment:
⚠️ **`dependencyNode` dropped from cache record — null returned on cache
hit**
The old 3-arg `CacheRecord` constructor stored the dependency node; the new
2-arg form does not. On cache hit (line ~394),
`pluginDescriptor.setDependencyNode(cacheRecord.getDependencyNode())` was also
removed. Any caller that invokes `pluginDescriptor.getDependencyNode()` after a
cache hit will get `null`, silently breaking dependency-graph consumers.
Either add `dependencyNode` back to the cache record, or document explicitly
that the dependency graph is not available after a cached load and audit all
call sites for that assumption.
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -259,6 +273,101 @@ public ClassRealm createPluginRealm(
return createRealm(getKey(plugin, false), RealmType.Plugin, parent,
parentImports, foreignImports, artifacts);
}
+ @Override
+ public ClassRealm createModularPluginRealm(Plugin plugin, ClassLoader
parent, List<Artifact> artifacts)
+ throws ModuleLayerCreationException {
+ Objects.requireNonNull(plugin, "plugin cannot be null");
+
+ if (parent == null) {
+ parent = PARENT_CLASSLOADER;
+ }
+
+ String realmId = getKey(plugin, false);
+ ClassRealm classRealm = newRealm(realmId);
+ classRealm.setParentClassLoader(parent);
+
+ // Collect all artifact paths for the module finder
+ List<Path> modulePaths = new ArrayList<>();
+ if (artifacts != null) {
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null &&
!isProvidedArtifact(artifact, true)) {
+ modulePaths.add(artifact.getFile().toPath());
+ } else if (logger.isDebugEnabled() && artifact.getFile() !=
null) {
+ logger.debug(" Excluded from module path: {}",
getId(artifact));
+ }
+ }
+ }
+
+ if (modulePaths.isEmpty()) {
+ throw new ModuleLayerCreationException("Plugin " + plugin.getId()
+ + " declares <modular>true</modular> but has no artifacts"
+ " to place on the module path");
+ }
+
+ try {
+ ModuleFinder finder = ModuleFinder.of(modulePaths.toArray(new
Path[0]));
+ Set<String> moduleNames = finder.findAll().stream()
+ .map(ref -> ref.descriptor().name())
+ .collect(Collectors.toSet());
+
+ if (moduleNames.isEmpty()) {
+ throw new ModuleLayerCreationException(
+ "Plugin " + plugin.getId() + " declares
<modular>true</modular> but none of its"
+ + " artifacts contain module descriptors
(module-info.class)."
+ + " Ensure the plugin JAR is compiled with a
module-info.java.");
+ }
+
+ // Parent layer: use the runtime layer from ClassWorld if
available (has Maven API
+ // modules + JLine), otherwise fall back to the boot layer.
+ // Flat hierarchy — all plugin layers are siblings under this
parent.
+ org.codehaus.plexus.classworlds.ClassWorld implWorld =
(org.codehaus.plexus.classworlds.ClassWorld) world;
Review Comment:
⚠️ **Unguarded downcast of `ClassWorld` to
`org.codehaus.plexus.classworlds.ClassWorld`**
```java
org.codehaus.plexus.classworlds.ClassWorld implWorld =
(org.codehaus.plexus.classworlds.ClassWorld) world;
```
`world` is typed as the API interface
`org.apache.maven.api.classworlds.ClassWorld`. This cast will throw
`ClassCastException` at runtime if `world` is not the plexus implementation —
for example, in tests or embedded scenarios that inject a different
implementation.
Use `instanceof` before casting:
```java
if (!(world instanceof org.codehaus.plexus.classworlds.ClassWorld
implWorld)) {
logger.debug("ClassWorld is not plexus implementation, skipping modular
plugin realm creation");
return createPluginRealm(...);
}
```
##########
impl/maven-core/src/main/java/org/apache/maven/plugin/PluginParameterExpressionEvaluator.java:
##########
@@ -163,8 +163,7 @@ public Object evaluate(String expr, Class<?> type) throws
ExpressionEvaluationEx
return expression.replace("$$", "$");
}
- // the evaluator may be created without a mojo execution, see the
single argument constructor
- MojoDescriptor mojoDescriptor = mojoExecution != null ?
mojoExecution.getMojoDescriptor() : null;
+ MojoDescriptor mojoDescriptor = mojoExecution.getMojoDescriptor();
Review Comment:
🚨 **`NullPointerException` when evaluating plugin expressions without a mojo
execution**
The previous code explicitly guarded against `mojoExecution == null`:
```java
// the evaluator may be created without a mojo execution, see the single
argument constructor
MojoDescriptor mojoDescriptor = mojoExecution != null ?
mojoExecution.getMojoDescriptor() : null;
```
The single-arg constructor `PluginParameterExpressionEvaluator(MavenSession
session)` still delegates to `this(session, null)`. After this change, any call
to `evaluate()` via that constructor path will throw `NullPointerException` at
`mojoExecution.getMojoDescriptor()`.
The null guard must be restored, or all `null`-passing call sites updated to
use the two-arg constructor with a real `MojoExecution`.
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/realm/ClassRealm.java:
##########
@@ -0,0 +1,556 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.plexus.classworlds.realm;
+
+/*
+ * Copyright 2001-2006 Codehaus Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+import org.codehaus.plexus.classworlds.ClassWorld;
+import org.codehaus.plexus.classworlds.strategy.Strategy;
+import org.codehaus.plexus.classworlds.strategy.StrategyFactory;
+
+/**
+ * The class loading gateway. Each class realm has access to a base class
loader, imports form zero or more other class
+ * loaders, an optional parent class loader and of course its own class path.
When queried for a class/resource, a class
+ * realm will always query its base class loader first before it delegates to
a pluggable strategy. The strategy in turn
+ * controls the order in which imported class loaders, the parent class loader
and the realm itself are searched. The
+ * base class loader is assumed to be capable of loading of the bootstrap
classes.
+ *
+ * @author <a href="mailto:[email protected]";>bob mcwhirter</a>
+ * @author Jason van Zyl
+ */
+public class ClassRealm extends URLClassLoader implements
org.apache.maven.api.classworlds.ClassRealm {
+
+ private ClassWorld world;
+
+ private String id;
+
+ private SortedSet<Entry> foreignImports;
+
+ private SortedSet<Entry> parentImports;
+
+ private Strategy strategy;
+
+ private ClassLoader parentClassLoader;
+
+ private ModuleLayer moduleLayer;
+
+ private ModuleLayer.Controller moduleLayerController;
+
+ private static final boolean IS_PARALLEL_CAPABLE =
Closeable.class.isAssignableFrom(URLClassLoader.class);
+
+ private final ConcurrentMap<String, Object> lockMap;
+
+ /**
+ * Creates a new class realm.
+ *
+ * @param world The class world this realm belongs to, must not
be <code>null</code>.
+ * @param id The identifier for this realm, must not be
<code>null</code>.
+ * @param baseClassLoader The base class loader for this realm, may be
<code>null</code> to use the bootstrap class
+ * loader.
+ */
+ public ClassRealm(ClassWorld world, String id, ClassLoader
baseClassLoader) {
+ super(new URL[0], baseClassLoader);
+
+ this.world = world;
+
+ this.id = id;
+
+ foreignImports = new TreeSet<>();
+
+ strategy = StrategyFactory.getStrategy(this);
+
+ lockMap = IS_PARALLEL_CAPABLE ? new ConcurrentHashMap<>() : null;
+
+ if (IS_PARALLEL_CAPABLE) {
+ // We must call super.getClassLoadingLock at least once
+ // to avoid NPE in super.loadClass.
+ super.getClassLoadingLock(getClass().getName());
+ }
+ }
+
+ public String getId() {
+ return this.id;
+ }
+
+ public ClassWorld getWorld() {
+ return this.world;
+ }
+
+ /**
+ * Returns the underlying ClassLoader for this realm.
+ * <p>
+ * This method allows access to the actual ClassLoader implementation
+ * while maintaining API abstraction. Since ClassRealm extends
URLClassLoader,
+ * this method returns {@code this}.
+ * </p>
+ *
+ * @return the underlying ClassLoader (this instance)
+ */
+ public ClassLoader getClassLoader() {
+ return this;
+ }
+
+ public void importFromParent(String packageName) {
+ if (parentImports == null) {
+ parentImports = new TreeSet<>();
+ }
+
+ parentImports.add(new Entry(null, packageName));
+ }
+
+ boolean isImportedFromParent(String name) {
+ if (parentImports != null && !parentImports.isEmpty()) {
+ for (Entry entry : parentImports) {
+ if (entry.matches(name)) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ return true;
+ }
+
+ public void importFrom(String realmId, String packageName) throws
NoSuchRealmException {
+ importFrom(getWorld().getRealm(realmId), packageName);
+ }
+
+ public void importFrom(ClassLoader classLoader, String packageName) {
+ foreignImports.add(new Entry(classLoader, packageName));
+ }
+
+ public ClassLoader getImportClassLoader(String name) {
+ for (Entry entry : foreignImports) {
+ if (entry.matches(name)) {
+ return entry.getClassLoader();
+ }
+ }
+
+ return null;
+ }
+
+ public Collection<ClassRealm> getImportRealms() {
+ Collection<ClassRealm> importRealms = new HashSet<>();
+
+ for (Entry entry : foreignImports) {
+ if (entry.getClassLoader() instanceof ClassRealm) {
+ importRealms.add((ClassRealm) entry.getClassLoader());
+ }
+ }
+
+ return importRealms;
+ }
+
+ public Strategy getStrategy() {
+ return strategy;
+ }
+
+ public void setParentClassLoader(ClassLoader parentClassLoader) {
+ this.parentClassLoader = parentClassLoader;
+ }
+
+ public ClassLoader getParentClassLoader() {
+ return parentClassLoader;
+ }
+
+ public void setParentRealm(ClassRealm realm) {
+ this.parentClassLoader = realm;
+ }
+
+ public ClassRealm getParentRealm() {
+ return (parentClassLoader instanceof ClassRealm) ? (ClassRealm)
parentClassLoader : null;
+ }
+
+ // Implementation of the original method signature for backward
compatibility
+ public ClassRealm createChildRealm(String id) throws
DuplicateRealmException {
+ ClassRealm childRealm = getWorld().newRealm(id, (ClassLoader) null);
+ childRealm.setParentRealm(this);
+ return childRealm;
+ }
+
+ public void addURL(URL url) {
+ String urlStr = url.toExternalForm();
+
+ if (urlStr.startsWith("jar:") && urlStr.endsWith("!/")) {
+ urlStr = urlStr.substring(4, urlStr.length() - 2);
Review Comment:
💡 **`addExports`/`addOpens`/`addReads` silently discard directives for
boot-layer modules**
When `isBootLayerModule(source)` is true, the method returns without logging
anything. For the caller, it looks like the directive was applied; it was
silently swallowed. Boot-layer modules are common (e.g. `java.base`), and a
user who writes a `module-access` descriptor targeting them will get no
feedback.
Add a `logger.debug()` or `logger.warn()` explaining that boot-layer modules
require `--add-exports`/`--add-opens` in the launcher script instead:
```java
if (source == null || moduleLayerController == null ||
isBootLayerModule(source)) {
logger.debug("Skipping add-exports/add-opens for '{}': boot-layer
modules cannot be modified at runtime; use --add-exports in the JVM launcher",
moduleName);
return;
}
```
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/realm/ClassRealm.java:
##########
@@ -0,0 +1,556 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.plexus.classworlds.realm;
+
+/*
+ * Copyright 2001-2006 Codehaus Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.io.PrintStream;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.Enumeration;
+import java.util.HashSet;
+import java.util.LinkedHashSet;
+import java.util.SortedSet;
+import java.util.TreeSet;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+
+import org.codehaus.plexus.classworlds.ClassWorld;
+import org.codehaus.plexus.classworlds.strategy.Strategy;
+import org.codehaus.plexus.classworlds.strategy.StrategyFactory;
+
+/**
+ * The class loading gateway. Each class realm has access to a base class
loader, imports form zero or more other class
+ * loaders, an optional parent class loader and of course its own class path.
When queried for a class/resource, a class
+ * realm will always query its base class loader first before it delegates to
a pluggable strategy. The strategy in turn
+ * controls the order in which imported class loaders, the parent class loader
and the realm itself are searched. The
+ * base class loader is assumed to be capable of loading of the bootstrap
classes.
+ *
+ * @author <a href="mailto:[email protected]";>bob mcwhirter</a>
+ * @author Jason van Zyl
+ */
+public class ClassRealm extends URLClassLoader implements
org.apache.maven.api.classworlds.ClassRealm {
+
+ private ClassWorld world;
+
+ private String id;
+
+ private SortedSet<Entry> foreignImports;
+
+ private SortedSet<Entry> parentImports;
+
+ private Strategy strategy;
+
+ private ClassLoader parentClassLoader;
+
+ private ModuleLayer moduleLayer;
+
+ private ModuleLayer.Controller moduleLayerController;
+
+ private static final boolean IS_PARALLEL_CAPABLE =
Closeable.class.isAssignableFrom(URLClassLoader.class);
+
+ private final ConcurrentMap<String, Object> lockMap;
+
+ /**
+ * Creates a new class realm.
+ *
+ * @param world The class world this realm belongs to, must not
be <code>null</code>.
+ * @param id The identifier for this realm, must not be
<code>null</code>.
+ * @param baseClassLoader The base class loader for this realm, may be
<code>null</code> to use the bootstrap class
+ * loader.
+ */
+ public ClassRealm(ClassWorld world, String id, ClassLoader
baseClassLoader) {
+ super(new URL[0], baseClassLoader);
+
+ this.world = world;
+
+ this.id = id;
+
+ foreignImports = new TreeSet<>();
+
+ strategy = StrategyFactory.getStrategy(this);
+
+ lockMap = IS_PARALLEL_CAPABLE ? new ConcurrentHashMap<>() : null;
+
+ if (IS_PARALLEL_CAPABLE) {
+ // We must call super.getClassLoadingLock at least once
+ // to avoid NPE in super.loadClass.
+ super.getClassLoadingLock(getClass().getName());
+ }
+ }
+
+ public String getId() {
+ return this.id;
+ }
+
+ public ClassWorld getWorld() {
+ return this.world;
+ }
+
+ /**
+ * Returns the underlying ClassLoader for this realm.
+ * <p>
+ * This method allows access to the actual ClassLoader implementation
+ * while maintaining API abstraction. Since ClassRealm extends
URLClassLoader,
+ * this method returns {@code this}.
+ * </p>
+ *
+ * @return the underlying ClassLoader (this instance)
+ */
+ public ClassLoader getClassLoader() {
+ return this;
+ }
+
+ public void importFromParent(String packageName) {
+ if (parentImports == null) {
+ parentImports = new TreeSet<>();
+ }
+
+ parentImports.add(new Entry(null, packageName));
+ }
+
+ boolean isImportedFromParent(String name) {
+ if (parentImports != null && !parentImports.isEmpty()) {
+ for (Entry entry : parentImports) {
+ if (entry.matches(name)) {
+ return true;
+ }
+ }
+
+ return false;
+ }
+
+ return true;
+ }
+
+ public void importFrom(String realmId, String packageName) throws
NoSuchRealmException {
+ importFrom(getWorld().getRealm(realmId), packageName);
+ }
+
+ public void importFrom(ClassLoader classLoader, String packageName) {
+ foreignImports.add(new Entry(classLoader, packageName));
+ }
+
+ public ClassLoader getImportClassLoader(String name) {
+ for (Entry entry : foreignImports) {
+ if (entry.matches(name)) {
+ return entry.getClassLoader();
+ }
+ }
+
+ return null;
+ }
+
+ public Collection<ClassRealm> getImportRealms() {
+ Collection<ClassRealm> importRealms = new HashSet<>();
Review Comment:
💡 **`getImportRealms()` returns a mutable `HashSet` — breaks encapsulation**
The returned `HashSet` is directly mutable by callers. Wrap before returning:
```java
return Collections.unmodifiableCollection(importRealms);
```
Or if the method contract should be a `Set`, change the return type to
`Set<ClassRealm>` and wrap with `Collections.unmodifiableSet(importRealms)`.
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -259,6 +273,101 @@ public ClassRealm createPluginRealm(
return createRealm(getKey(plugin, false), RealmType.Plugin, parent,
parentImports, foreignImports, artifacts);
}
+ @Override
+ public ClassRealm createModularPluginRealm(Plugin plugin, ClassLoader
parent, List<Artifact> artifacts)
+ throws ModuleLayerCreationException {
+ Objects.requireNonNull(plugin, "plugin cannot be null");
+
+ if (parent == null) {
+ parent = PARENT_CLASSLOADER;
+ }
+
+ String realmId = getKey(plugin, false);
+ ClassRealm classRealm = newRealm(realmId);
+ classRealm.setParentClassLoader(parent);
+
+ // Collect all artifact paths for the module finder
+ List<Path> modulePaths = new ArrayList<>();
+ if (artifacts != null) {
+ for (Artifact artifact : artifacts) {
+ if (artifact.getFile() != null &&
!isProvidedArtifact(artifact, true)) {
+ modulePaths.add(artifact.getFile().toPath());
+ } else if (logger.isDebugEnabled() && artifact.getFile() !=
null) {
+ logger.debug(" Excluded from module path: {}",
getId(artifact));
+ }
+ }
+ }
+
+ if (modulePaths.isEmpty()) {
+ throw new ModuleLayerCreationException("Plugin " + plugin.getId()
+ + " declares <modular>true</modular> but has no artifacts"
+ " to place on the module path");
+ }
+
+ try {
+ ModuleFinder finder = ModuleFinder.of(modulePaths.toArray(new
Path[0]));
+ Set<String> moduleNames = finder.findAll().stream()
+ .map(ref -> ref.descriptor().name())
+ .collect(Collectors.toSet());
+
+ if (moduleNames.isEmpty()) {
+ throw new ModuleLayerCreationException(
+ "Plugin " + plugin.getId() + " declares
<modular>true</modular> but none of its"
+ + " artifacts contain module descriptors
(module-info.class)."
+ + " Ensure the plugin JAR is compiled with a
module-info.java.");
+ }
+
+ // Parent layer: use the runtime layer from ClassWorld if
available (has Maven API
+ // modules + JLine), otherwise fall back to the boot layer.
+ // Flat hierarchy — all plugin layers are siblings under this
parent.
+ org.codehaus.plexus.classworlds.ClassWorld implWorld =
(org.codehaus.plexus.classworlds.ClassWorld) world;
+ ModuleLayer parentLayer =
+ implWorld.getModuleLayer() != null ?
implWorld.getModuleLayer() : ModuleLayer.boot();
+
+ // resolveAndBind: resolves modules and binds service providers,
+ // enabling ServiceLoader discovery within the plugin layer
+ Configuration cfg =
parentLayer.configuration().resolveAndBind(ModuleFinder.of(), finder,
moduleNames);
+
+ // One loader per plugin — isolation between plugins
+ ModuleLayer.Controller controller =
+ ModuleLayer.defineModulesWithOneLoader(cfg,
List.of(parentLayer), parent);
+
+ ModuleLayer pluginLayer = controller.layer();
+
+ // Store per-realm, not per-world
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
classRealm;
+ implRealm.setModuleLayer(pluginLayer, controller);
+
+ // Also populate the realm's classpath with the same artifacts
+ // so that ClassRealm.getURLs() returns meaningful results
+ // and resource loading works through the realm
+ populateRealm(
+ classRealm,
+ artifacts == null
+ ? List.of()
+ : artifacts.stream()
+ .filter(a -> a.getFile() != null &&
!isProvidedArtifact(a, true))
+ .map(ArtifactClassRealmConstituent::new)
+ .collect(Collectors.toList()));
+
+ // Apply META-INF/maven/module-access descriptors for access to
+ // boot-layer modules (e.g. java.base internals)
+ applyModuleAccessDescriptors(classRealm);
+
+ logger.debug(
+ "Created modular plugin realm {} with ModuleLayer
containing modules: {}", realmId, moduleNames);
+
+ return classRealm;
+
+ } catch (FindException | ResolutionException e) {
Review Comment:
⚠️ **`LayerInstantiationException` not caught — unchecked exception from
`defineModulesWithOneLoader` escapes**
`ModuleLayer.defineModulesWithOneLoader()` (called at line 332) throws
`LayerInstantiationException` (an unchecked `RuntimeException`) if the layer
cannot be instantiated. The catch block only handles `FindException |
ResolutionException`, leaving `LayerInstantiationException` to propagate
unwrapped — bypassing the `ModuleLayerCreationException` wrapper and
potentially surfacing as an opaque stack trace to the user.
Add `LayerInstantiationException` to the catch clause:
```java
} catch (FindException | ResolutionException | LayerInstantiationException
e) {
throw new ModuleLayerCreationException(
"Failed to create module layer for plugin " + plugin.getId() +
"...", e);
}
```
##########
impl/maven-classworlds/src/main/java/org/codehaus/plexus/classworlds/ClassWorld.java:
##########
@@ -0,0 +1,303 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.codehaus.plexus.classworlds;
+
+/*
+ * Copyright 2001-2006 Codehaus Foundation.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import java.io.Closeable;
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.Collections;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.function.Predicate;
+
+import org.codehaus.plexus.classworlds.realm.ClassRealm;
+import org.codehaus.plexus.classworlds.realm.DuplicateRealmException;
+import org.codehaus.plexus.classworlds.realm.FilteredClassRealm;
+import org.codehaus.plexus.classworlds.realm.NoSuchRealmException;
+
+/**
+ * A collection of <code>ClassRealm</code>s, indexed by id.
+ *
+ * @author <a href="mailto:[email protected]";>bob mcwhirter</a>
+ */
+public class ClassWorld implements
org.apache.maven.api.classworlds.ClassWorld, Closeable {
+ private Map<String, ClassRealm> realms;
+
+ private final List<ClassWorldListener> listeners = new ArrayList<>();
+
+ private ModuleLayer moduleLayer;
+
+ private ModuleLayer.Controller moduleLayerController;
+
+ public ClassWorld(String realmId, ClassLoader classLoader) {
+ this();
+
+ try {
+ newRealm(realmId, classLoader);
+ } catch (DuplicateRealmException e) {
+ // Will never happen as we are just creating the world.
+ }
+ }
+
+ public ClassWorld() {
+ this.realms = new LinkedHashMap<>();
+ }
+
+ public ClassRealm newRealm(String id) throws DuplicateRealmException {
+ return newRealm(id, getClass().getClassLoader());
+ }
+
+ public ClassRealm newRealm(String id, ClassLoader classLoader) throws
DuplicateRealmException {
+ return newRealm(id, classLoader, null);
+ }
+
+ /**
+ * Adds a class realm with filtering.
+ * Only resources/classes whose name matches a given predicate are exposed.
+ * @param id The identifier for this realm, must not be <code>null</code>.
+ * @param classLoader The base class loader for this realm, may be
<code>null</code> to use the bootstrap class
+ * loader.
+ * @param filter a predicate to apply to each resource name to determine
if it should be loaded through this class loader
+ * @return the created class realm
+ * @throws DuplicateRealmException in case a realm with the given id does
already exist
+ * @since 2.7.0
+ * @see FilteredClassRealm
+ */
+ public synchronized ClassRealm newRealm(String id, ClassLoader
classLoader, Predicate<String> filter)
+ throws DuplicateRealmException {
+ if (realms.containsKey(id)) {
+ throw new DuplicateRealmException(this, id);
+ }
+
+ ClassRealm realm;
+
+ if (filter == null) {
+ realm = new ClassRealm(this, id, classLoader);
+ } else {
+ realm = new FilteredClassRealm(filter, this, id, classLoader);
+ }
+ realms.put(id, realm);
+
+ for (ClassWorldListener listener : listeners) {
+ listener.realmCreated(realm);
+ }
+
+ return realm;
+ }
+
+ /**
+ * Closes all contained class realms.
+ * @since 2.7.0
+ */
+ @Override
+ public synchronized void close() throws IOException {
+ realms.values().stream().forEach(this::disposeRealm);
+ realms.clear();
+ }
+
+ public synchronized void disposeRealm(String id) throws
NoSuchRealmException {
+ ClassRealm realm = realms.remove(id);
+
+ if (realm != null) {
+ disposeRealm(realm);
+ } else {
+ throw new NoSuchRealmException(this, id);
+ }
+ }
+
+ private void disposeRealm(ClassRealm realm) {
+ try {
+ realm.close();
+ } catch (IOException ignore) {
+ }
+ for (ClassWorldListener listener : listeners) {
+ listener.realmDisposed(realm);
+ }
+ }
+
+ public synchronized ClassRealm getRealm(String id) throws
NoSuchRealmException {
+ if (realms.containsKey(id)) {
+ return realms.get(id);
+ }
+
+ throw new NoSuchRealmException(this, id);
+ }
+
+ public synchronized Collection<ClassRealm> getRealms() {
+ return Collections.unmodifiableList(new ArrayList<>(realms.values()));
+ }
+
+ public synchronized void setModuleLayer(ModuleLayer moduleLayer,
ModuleLayer.Controller controller) {
+ this.moduleLayer = moduleLayer;
+ this.moduleLayerController = controller;
+ }
+
+ public ModuleLayer getModuleLayer() {
+ return moduleLayer;
+ }
+
+ public ModuleLayer.Controller getModuleLayerController() {
+ return moduleLayerController;
+ }
+
+ /**
+ * Exports a package from a named module to the given target module.
+ * Looks up the source module in the runtime layer first, then the boot
layer.
+ * Only runtime layer modules can be modified via the Controller; boot
layer
+ * modules require {@code --add-exports} in the launcher script.
+ *
+ * @param moduleName the source module name
+ * @param packageName the package to export
+ * @param target the target module to export to
+ */
+ public synchronized void addExports(String moduleName, String packageName,
Module target) {
+ applyModuleAccess(moduleName, packageName, target, false);
+ }
+
+ /**
+ * Opens a package from a named module to the given target module for deep
reflection.
+ * Looks up the source module in the runtime layer first, then the boot
layer.
+ * Only runtime layer modules can be modified via the Controller; boot
layer
+ * modules require {@code --add-opens} in the launcher script.
+ *
+ * @param moduleName the source module name
+ * @param packageName the package to open
+ * @param target the target module to open to
+ */
+ public synchronized void addOpens(String moduleName, String packageName,
Module target) {
+ applyModuleAccess(moduleName, packageName, target, true);
+ }
+
+ /**
+ * Adds a reads edge from the named source module to the given target
module.
+ * Only runtime layer modules can be modified via the Controller.
+ *
+ * @param sourceModuleName the source module name
+ * @param target the target module to read
+ */
+ public synchronized void addReads(String sourceModuleName, Module target) {
+ Module source = findModule(sourceModuleName);
+ if (source == null || moduleLayerController == null ||
isBootLayerModule(source)) {
+ return;
+ }
+ moduleLayerController.addReads(source, target);
+ }
+
+ private void applyModuleAccess(String moduleName, String packageName,
Module target, boolean open) {
+ Module source = findModule(moduleName);
+ if (source == null || moduleLayerController == null ||
isBootLayerModule(source)) {
+ return;
+ }
+ if (open) {
+ moduleLayerController.addOpens(source, packageName, target);
+ } else {
+ moduleLayerController.addExports(source, packageName, target);
+ }
+ }
+
+ private Module findModule(String moduleName) {
+ if (moduleLayer != null) {
+ Module m = moduleLayer.findModule(moduleName).orElse(null);
+ if (m != null) {
+ return m;
+ }
+ }
+ return ModuleLayer.boot().findModule(moduleName).orElse(null);
+ }
+
+ private boolean isBootLayerModule(Module module) {
+ return module.getLayer() == ModuleLayer.boot();
+ }
+
+ // from exports branch
+ public synchronized ClassRealm getClassRealm(String id) {
+ if (realms.containsKey(id)) {
+ return realms.get(id);
+ }
+
+ return null;
+ }
+
+ public synchronized void addListener(ClassWorldListener listener) {
+ // TODO ideally, use object identity, not equals
+ if (!listeners.contains(listener)) {
+ listeners.add(listener);
+ }
+ }
+
+ public synchronized void removeListener(ClassWorldListener listener) {
+ listeners.remove(listener);
+ }
+
+ // API interface methods - newRealm with filter is already implemented
above
+
+ @Override
+ public void
addListener(org.apache.maven.api.classworlds.ClassWorldListener listener) {
+ if (listener instanceof ClassWorldListener) {
+ addListener((ClassWorldListener) listener);
+ } else {
+ // Wrap the API listener
+ addListener(new ClassWorldListener() {
+ @Override
+ public void realmCreated(ClassRealm realm) {
+ listener.realmCreated(realm);
+ }
+
+ @Override
+ public void realmDisposed(ClassRealm realm) {
+ listener.realmDisposed(realm);
+ }
+
+ @Override
+ public void
realmCreated(org.apache.maven.api.classworlds.ClassRealm realm) {
+ listener.realmCreated(realm);
+ }
+
+ @Override
+ public void
realmDisposed(org.apache.maven.api.classworlds.ClassRealm realm) {
+ listener.realmDisposed(realm);
+ }
+ });
+ }
+ }
+
+ @Override
+ public void
removeListener(org.apache.maven.api.classworlds.ClassWorldListener listener) {
+ // For now, we'll need to track wrapped listeners if this becomes
important
Review Comment:
💡 **`removeListener` for API-typed listeners is a silent no-op**
The implementation comment is honest, but the current contract is broken: a
caller who registers a listener via `addListener(ClassWorldListener)` (the
wrapping path at line 274) cannot un-register it via `removeListener`. This
will cause listener leaks in long-running embedded Maven sessions.
Either:
- Track the `(api-listener → wrapped-listener)` mapping in a `Map` and
implement removal, or
- Throw `UnsupportedOperationException` with a clear message so callers
aren't silently surprised
A quiet no-op is the worst of both worlds.
##########
impl/maven-core/src/main/java/org/apache/maven/classrealm/DefaultClassRealmManager.java:
##########
@@ -364,4 +473,116 @@ private static Object getId(ClassLoader classLoader) {
}
return classLoader;
}
+
+ private static final String MODULE_ACCESS_DESCRIPTOR =
"META-INF/maven/module-access";
+
+ /**
+ * Reads {@code META-INF/maven/module-access} descriptors from the plugin
realm's classpath and
+ * applies the access directives they contain.
+ *
+ * <h4>Purpose</h4>
+ * <p>
+ * When a plugin (or extension) runs on the <em>classpath</em> (unnamed
module) it may need to
+ * call into packages of a <em>named</em> module that lives in Maven's
boot/runtime layer —
+ * for example JLine modules in {@code lib/modules/}, or any future Maven
API module that ships
+ * as a named module. Named modules do not export their internals to the
unnamed module by
+ * default; the JVM enforces this at runtime via strong encapsulation.
+ * </p>
+ * <p>
+ * The {@code META-INF/maven/module-access} file is a plugin-side escape
hatch: it lets the
+ * plugin declare which packages of which boot-layer modules it needs
opened or exported
+ * <strong>to itself</strong> (always {@code ALL-UNNAMED} as the target).
Maven applies these
+ * directives programmatically via {@link Module#addExports}/{@link
Module#addOpens} on startup
+ * of the plugin realm, before any plugin class is loaded.
+ * </p>
+ *
+ * <h4>Direction of access</h4>
+ * <p>
+ * Directives go from an <em>existing</em> named module in the
boot/runtime layer
+ * <strong>to</strong> the unnamed-module classloader of the plugin. In
other words:
+ * a plugin cannot use this mechanism to export its <em>own</em> packages;
it can only
+ * widen access from modules that Maven already owns (and that are loaded
before the plugin).
+ * </p>
+ *
+ * <h4>File format</h4>
+ * <p>Each non-blank, non-comment line is one of:</p>
+ * <pre>
+ * add-exports <module>/<package>=ALL-UNNAMED
+ * add-opens <module>/<package>=ALL-UNNAMED
+ * add-reads <module>
+ * </pre>
+ * <p>
+ * These mirror the JVM flags {@code --add-exports}, {@code --add-opens},
and
+ * {@code --add-reads} respectively. Lines starting with {@code #} are
comments.
+ * </p>
+ *
+ * <h4>Why not {@code module-info.java}?</h4>
+ * <p>
+ * The boot-layer modules (e.g. JLine) are compiled independently and
cannot reference
+ * the plugin's module name at compile time. Adding {@code exports … to
plugin.module} in
+ * their {@code module-info} would create an unacceptable coupling.
Programmatic
+ * {@code addExports}/{@code addOpens} is the standard JDK-endorsed
workaround for
+ * this exact scenario (see {@link java.lang.Module#addExports}).
+ * </p>
+ *
+ * @param classRealm the plugin class realm whose JAR(s) are scanned for
descriptors
+ */
+ private void applyModuleAccessDescriptors(ClassRealm classRealm) {
+ var implRealm = (org.codehaus.plexus.classworlds.realm.ClassRealm)
classRealm;
+ try {
+ Enumeration<URL> resources =
implRealm.getResources(MODULE_ACCESS_DESCRIPTOR);
+ while (resources.hasMoreElements()) {
+ URL resource = resources.nextElement();
+ try (BufferedReader reader =
+ new BufferedReader(new
InputStreamReader(resource.openStream(), StandardCharsets.UTF_8))) {
+ String line;
+ while ((line = reader.readLine()) != null) {
+ line = line.trim();
+ if (line.isEmpty() || line.startsWith("#")) {
+ continue;
+ }
+ applyModuleAccessDirective(implRealm, line);
+ }
+ }
+ }
+ } catch (IOException e) {
Review Comment:
💡 **`IOException` when reading module-access descriptors swallowed at DEBUG
— consider WARN**
A failure to read the module-access descriptor will silently skip all access
directives for the realm, which can cause hard-to-diagnose JPMS
`InaccessibleObjectException` later. `DEBUG` is appropriate for operational
noise, but a failed I/O read is user-actionable (wrong packaging, missing
file). Consider `WARN` so users can correlate it with the downstream error.
--
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]
