gnodet-bot commented on code in PR #13625:
URL: https://github.com/apache/maven/pull/13625#discussion_r4313771635
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Plugin.java:
##########
@@ -46,7 +46,7 @@ public interface Plugin {
List<Lifecycle> getLifecycles();
@Nonnull
- ClassLoader getClassLoader();
+ PluginRealm getRealm();
Review Comment:
⚠️ **Breaking change** — `getClassLoader()` is removed from the `Plugin` API
and replaced by `getRealm()`. This is a source and binary breaking change for
all existing embedders calling `plugin.getClassLoader()`.
Please also add `@since 4.1.0` on `getRealm()` — the class-level `@since
4.0.0` does not cover new methods added later.
```suggestion
/**
* Returns the runtime realm for this plugin.
*
* @return the plugin realm
* @since 4.1.0
*/
@Nonnull
PluginRealm getRealm();
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/PluginManagerException.java:
##########
@@ -0,0 +1,34 @@
+/*
+ * 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.apache.maven.api.services;
+
+import org.apache.maven.api.annotations.Experimental;
+
+/**
+ * Thrown when a plugin runtime cannot be resolved or loaded.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public class PluginManagerException extends MavenException {
+
+ public PluginManagerException(String message, Throwable cause) {
Review Comment:
💡 Two issues with `PluginManagerException`:
1. **Missing `@Serial serialVersionUID`** — every `Serializable` subclass in
maven-api should declare a `serialVersionUID` (see `BuildException`,
`VersionRangeResolverException`, etc.).
2. **Single constructor** — consider adding a message-only constructor for
cases where there is no underlying cause (e.g. a validation failure in
`findPlugin`), following the convention of other `MavenException` subclasses.
```suggestion
@java.io.Serial
private static final long serialVersionUID = 1L;
public PluginManagerException(String message) {
super(message);
}
public PluginManagerException(String message, Throwable cause) {
super(message, cause);
}
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/PluginRealm.java:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.apache.maven.api;
+
+import java.util.Optional;
+
+import org.apache.maven.api.annotations.Experimental;
+import org.apache.maven.api.annotations.Nonnull;
+import org.apache.maven.api.services.Lookup;
+
+/**
+ * Represents the runtime scope of a Maven plugin.
+ *
+ * @since 4.1.0
+ */
+@Experimental
+public interface PluginRealm {
+
+ /**
+ * Returns the classloader for this realm.
+ */
+ @Nonnull
+ ClassLoader getClassLoader();
Review Comment:
💡 **Missing `@return` tags** — all three methods have "Returns…" prose in
their Javadoc but no `@return` tag. Maven API convention requires the `@return`
tag. Same applies to `getModuleLayer()` (L45) and `getLookup()` (L51).
```suggestion
/**
* Returns the classloader for this realm.
*
* @return the classloader, never {@code null}
*/
@Nonnull
ClassLoader getClassLoader();
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultPluginManager.java:
##########
@@ -0,0 +1,92 @@
+/*
+ * 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.apache.maven.internal.impl;
+
+import javax.inject.Inject;
+import javax.inject.Named;
+import javax.inject.Singleton;
+
+import java.util.Objects;
+
+import org.apache.maven.api.Plugin;
+import org.apache.maven.api.Project;
+import org.apache.maven.api.Session;
+import org.apache.maven.api.services.Lookup;
+import org.apache.maven.api.services.PluginManager;
+import org.apache.maven.api.services.PluginManagerException;
+import org.apache.maven.execution.MavenSession;
+import org.apache.maven.lifecycle.internal.LifecyclePluginResolver;
+import org.apache.maven.plugin.MavenPluginManager;
+import org.apache.maven.plugin.descriptor.PluginDescriptor;
+import org.apache.maven.project.MavenProject;
+
+import static java.util.Objects.requireNonNull;
+import static org.apache.maven.internal.impl.CoreUtils.cast;
+
+@Named
+@Singleton
+public final class DefaultPluginManager implements PluginManager {
+
+ private final MavenPluginManager pluginManager;
+ private final LifecyclePluginResolver lifecyclePluginResolver;
+ private final Lookup lookup;
+
+ @Inject
+ public DefaultPluginManager(
+ MavenPluginManager pluginManager, LifecyclePluginResolver
lifecyclePluginResolver, Lookup lookup) {
+ this.pluginManager = requireNonNull(pluginManager, "pluginManager");
+ this.lifecyclePluginResolver = requireNonNull(lifecyclePluginResolver,
"lifecyclePluginResolver");
+ this.lookup = requireNonNull(lookup, "lookup");
+ }
+
+ @Override
+ public Plugin loadPlugin(Session session, Project project,
org.apache.maven.api.model.Plugin plugin) {
+ requireNonNull(session, "session");
+ requireNonNull(project, "project");
+ requireNonNull(plugin, "plugin");
+
+ InternalMavenSession internalSession =
InternalMavenSession.from(session);
+ MavenProject mavenProject =
+ cast(DefaultProject.class, project, "project").getProject();
+ MavenSession executionSession =
internalSession.getMavenSession().clone();
+ executionSession.setCurrentProject(mavenProject);
+
+ try {
+ lifecyclePluginResolver.resolveMissingPluginVersions(mavenProject,
executionSession);
+ org.apache.maven.model.Plugin legacyPlugin =
findPlugin(mavenProject, plugin);
+ PluginDescriptor descriptor = pluginManager.getPluginDescriptor(
+ legacyPlugin, mavenProject.getRemotePluginRepositories(),
executionSession.getRepositorySession());
+ pluginManager.setupPluginRealm(descriptor, executionSession, null,
null, null);
+ return new DefaultPlugin(internalSession, legacyPlugin,
descriptor, lookup);
+ } catch (Exception e) {
+ throw new PluginManagerException(
+ "Unable to load plugin " + plugin.getGroupId() + ":" +
plugin.getArtifactId(), e);
Review Comment:
⚠️ **`catch(Exception)` masks `IllegalArgumentException` from
`findPlugin()`** — `findPlugin()` throws `IllegalArgumentException` when the
plugin is not declared by the project. That exception gets caught here and
re-thrown as a generic `"Unable to load plugin …"` message, losing the specific
context ("Plugin X:Y is not declared by project Z").
Consider catching the specific infrastructure exceptions from
`pluginManager` separately and letting `IllegalArgumentException` propagate
directly, or at minimum wrapping `findPlugin` outside the `try` block:
```suggestion
requireNonNull(session, "session");
requireNonNull(project, "project");
requireNonNull(plugin, "plugin");
InternalMavenSession internalSession =
InternalMavenSession.from(session);
MavenProject mavenProject =
cast(DefaultProject.class, project, "project").getProject();
MavenSession executionSession =
internalSession.getMavenSession().clone();
executionSession.setCurrentProject(mavenProject);
lifecyclePluginResolver.resolveMissingPluginVersions(mavenProject,
executionSession);
org.apache.maven.model.Plugin legacyPlugin =
findPlugin(mavenProject, plugin);
try {
PluginDescriptor descriptor = pluginManager.getPluginDescriptor(
legacyPlugin,
mavenProject.getRemotePluginRepositories(),
executionSession.getRepositorySession());
pluginManager.setupPluginRealm(descriptor, executionSession,
null, null, null);
return new DefaultPlugin(internalSession, legacyPlugin,
descriptor, lookup);
} catch (Exception e) {
throw new PluginManagerException(
"Unable to load plugin " + plugin.getGroupId() + ":" +
plugin.getArtifactId(), e);
}
```
--
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]
