gnodet-bot commented on code in PR #13366:
URL: https://github.com/apache/maven/pull/13366#discussion_r4215490535
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMojoExecution.java:
##########
@@ -38,110 +38,153 @@
import org.apache.maven.api.plugin.descriptor.lifecycle.Lifecycle;
import org.apache.maven.api.xml.XmlNode;
import org.apache.maven.impl.DefaultNode;
-import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.eclipse.aether.graph.DependencyNode;
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Immutable snapshot of a mojo execution, captured at the point when
execution begins
+ * (after configuration merging, descriptor resolution, and lifecycle phase
assignment are complete).
+ * All state is copied at construction time; no reference to the mutable legacy
+ * {@link org.apache.maven.plugin.MojoExecution} is retained after
construction.
+ */
public class DefaultMojoExecution implements MojoExecution {
- private final InternalMavenSession session;
- private final org.apache.maven.plugin.MojoExecution delegate;
+
+ private final Plugin plugin;
+ private final Optional<PluginExecution> model;
+ private final MojoDescriptor descriptor;
+ private final String executionId;
+ private final String goal;
+ private final String lifecyclePhase;
+ private final XmlNode configuration;
public DefaultMojoExecution(InternalMavenSession session,
org.apache.maven.plugin.MojoExecution delegate) {
- this.session = session;
- this.delegate = delegate;
+ requireNonNull(session, "session");
+ requireNonNull(delegate, "delegate");
+ this.descriptor = requireNonNull(delegate.getMojoDescriptor(),
"delegate.mojoDescriptor")
+ .getMojoDescriptorV4();
+ this.executionId = delegate.getExecutionId();
+ this.goal = delegate.getGoal();
+ this.lifecyclePhase = delegate.getLifecyclePhase();
Review Comment:
⚠️ **Null stored into @Nonnull field:** `delegate.getLifecyclePhase()`
returns `null` for CLI-invoked direct goals. The `lifecyclePhase` field is then
returned by `lifecyclePhase()` which is declared `@Nonnull` on the interface —
callers trusting the annotation will NPE.
Either add a null guard here (e.g.
`Objects.requireNonNullElse(delegate.getLifecyclePhase(), "")` — though empty
string is semantically wrong) or change the interface method to return
`Optional<String>` as suggested above.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/MojoExecution.java:
##########
@@ -31,30 +33,118 @@
* An instance of this object is bound to the {@link
org.apache.maven.api.di.MojoExecutionScoped}
* and available as {@code mojoExecution} within {@link
org.apache.maven.api.plugin.annotations.Parameter}
* expressions.
+ * <p>
+ * Instances are immutable snapshots taken at the point when execution begins
(after configuration
+ * merging, descriptor resolution, and lifecycle phase assignment are all
complete).
*
* @since 4.0.0
*/
@Experimental
+@Immutable
public interface MojoExecution {
+ /** {@return the plugin that owns this execution} */
@Nonnull
- Plugin getPlugin();
+ Plugin plugin();
+ /**
+ * {@return the {@code <execution>} element from the POM (or from the
default lifecycle bindings)
+ * that corresponds to this execution, or empty for a direct CLI invocation
+ * (e.g. {@code mvn groupId:artifactId:goal})}
+ * <p>
+ * Default lifecycle bindings (such as {@code
maven-compiler-plugin:compile} bound to the
+ * {@code compile} phase for {@code jar} packaging) are synthesised from
lifecycle mapping
+ * metadata and injected as {@code <execution>} elements before model
resolution, so they
+ * are also present here. Only a CLI-invoked goal — which has no backing
{@code <plugin>}
+ * entry in the model — results in an empty {@code Optional}.
+ */
@Nonnull
- PluginExecution getModel();
+ Optional<PluginExecution> model();
+ /** {@return the descriptor of the mojo being executed} */
@Nonnull
- MojoDescriptor getDescriptor();
+ MojoDescriptor descriptor();
+ /** {@return the execution identifier as declared in the POM} */
@Nonnull
- String getExecutionId();
+ String executionId();
+ /** {@return the goal being executed} */
@Nonnull
- String getGoal();
+ String goal();
+ /** {@return the lifecycle phase this execution is bound to} */
@Nonnull
- String getLifecyclePhase();
+ String lifecyclePhase();
Review Comment:
⚠️ **@Nonnull contract violation:** `lifecyclePhase()` returns `null` for
direct CLI-invoked goals (e.g. `mvn groupId:artifactId:goal`) where no
lifecycle phase is assigned. `DefaultMojoExecution` stores
`delegate.getLifecyclePhase()` verbatim with no null guard, and the legacy
`MojoExecution.lifecyclePhase` field defaults to `null` when
`setLifecyclePhase()` is never called.
For consistency with how `model()` handles optionality:
```suggestion
/** {@return the lifecycle phase this execution is bound to, or empty
for a direct CLI invocation} */
@Nonnull
Optional<String> lifecyclePhase();
```
The deprecated bridge would then be:
```java
@Deprecated(since = "4.1.0", forRemoval = true)
@Nullable
default String getLifecyclePhase() { return lifecyclePhase().orElse(null); }
```
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMojoExecution.java:
##########
@@ -38,110 +38,153 @@
import org.apache.maven.api.plugin.descriptor.lifecycle.Lifecycle;
import org.apache.maven.api.xml.XmlNode;
import org.apache.maven.impl.DefaultNode;
-import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.eclipse.aether.graph.DependencyNode;
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Immutable snapshot of a mojo execution, captured at the point when
execution begins
+ * (after configuration merging, descriptor resolution, and lifecycle phase
assignment are complete).
+ * All state is copied at construction time; no reference to the mutable legacy
+ * {@link org.apache.maven.plugin.MojoExecution} is retained after
construction.
+ */
public class DefaultMojoExecution implements MojoExecution {
- private final InternalMavenSession session;
- private final org.apache.maven.plugin.MojoExecution delegate;
+
+ private final Plugin plugin;
+ private final Optional<PluginExecution> model;
+ private final MojoDescriptor descriptor;
+ private final String executionId;
+ private final String goal;
+ private final String lifecyclePhase;
+ private final XmlNode configuration;
public DefaultMojoExecution(InternalMavenSession session,
org.apache.maven.plugin.MojoExecution delegate) {
- this.session = session;
- this.delegate = delegate;
+ requireNonNull(session, "session");
+ requireNonNull(delegate, "delegate");
+ this.descriptor = requireNonNull(delegate.getMojoDescriptor(),
"delegate.mojoDescriptor")
+ .getMojoDescriptorV4();
+ this.executionId = delegate.getExecutionId();
+ this.goal = delegate.getGoal();
+ this.lifecyclePhase = delegate.getLifecyclePhase();
+ this.configuration = delegate.getConfiguration() != null
+ ? delegate.getConfiguration().getDom()
+ : null;
+ this.plugin = buildPlugin(session, delegate);
+ this.model = buildModel(delegate);
}
- public org.apache.maven.plugin.MojoExecution getDelegate() {
- return delegate;
- }
+ private static Plugin buildPlugin(InternalMavenSession session,
org.apache.maven.plugin.MojoExecution delegate) {
+ org.apache.maven.plugin.descriptor.MojoDescriptor legacyDescriptor =
delegate.getMojoDescriptor();
+ org.apache.maven.plugin.descriptor.PluginDescriptor
legacyPluginDescriptor =
+ legacyDescriptor.getPluginDescriptor();
+ PluginDescriptor pluginDescriptorV4 =
legacyPluginDescriptor.getPluginDescriptorV4();
+
+ ClassLoader classLoader = legacyDescriptor.getRealm();
+
+ org.apache.maven.artifact.Artifact legacyArtifact =
legacyPluginDescriptor.getPluginArtifact();
+ org.eclipse.aether.artifact.Artifact resolverArtifact =
RepositoryUtils.toArtifact(legacyArtifact);
+ Artifact artifact = resolverArtifact != null ?
session.getArtifact(resolverArtifact) : null;
+
+ DependencyNode resolverNode =
legacyPluginDescriptor.getDependencyNode();
+ Map<String, Dependency> dependenciesMap = resolverNode != null
+ ? Collections.unmodifiableMap(new DefaultNode(session,
resolverNode, false)
+ .stream()
+ .filter(Objects::nonNull)
+ .map(Node::getDependency)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toMap(
+ d -> d.getGroupId() + ":" +
d.getArtifactId(), d -> d, (a, b) -> a)))
Review Comment:
⚠️ **@Nonnull contract violation on Plugin.getModel():** `modelPlugin` can
be `null` when `delegate.getPlugin() == null` (CLI-invoked goals have no
backing `<plugin>` in the model). The anonymous `Plugin.getModel()` returns it
directly, but `Plugin.getModel()` is declared `@Nonnull` in the `Plugin`
interface.
Same for `Plugin.getArtifact()`: `artifact` is `null` when
`RepositoryUtils.toArtifact(legacyArtifact)` returns null, but `getArtifact()`
is declared `@Nonnull Artifact`.
Both were pre-existing issues in the old lazy-delegation code but the
snapshot pattern makes them structural — worth fixing now. Either add
`requireNonNull` guards (with a sensible sentinel), update the `Plugin`
interface contracts to `@Nullable`, or ensure null-hostile call sites are
guarded.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMojoExecution.java:
##########
@@ -38,110 +38,153 @@
import org.apache.maven.api.plugin.descriptor.lifecycle.Lifecycle;
import org.apache.maven.api.xml.XmlNode;
import org.apache.maven.impl.DefaultNode;
-import org.codehaus.plexus.util.xml.Xpp3Dom;
import org.eclipse.aether.graph.DependencyNode;
+import static java.util.Objects.requireNonNull;
+
+/**
+ * Immutable snapshot of a mojo execution, captured at the point when
execution begins
+ * (after configuration merging, descriptor resolution, and lifecycle phase
assignment are complete).
+ * All state is copied at construction time; no reference to the mutable legacy
+ * {@link org.apache.maven.plugin.MojoExecution} is retained after
construction.
+ */
public class DefaultMojoExecution implements MojoExecution {
- private final InternalMavenSession session;
- private final org.apache.maven.plugin.MojoExecution delegate;
+
+ private final Plugin plugin;
+ private final Optional<PluginExecution> model;
+ private final MojoDescriptor descriptor;
+ private final String executionId;
+ private final String goal;
+ private final String lifecyclePhase;
+ private final XmlNode configuration;
public DefaultMojoExecution(InternalMavenSession session,
org.apache.maven.plugin.MojoExecution delegate) {
- this.session = session;
- this.delegate = delegate;
+ requireNonNull(session, "session");
+ requireNonNull(delegate, "delegate");
+ this.descriptor = requireNonNull(delegate.getMojoDescriptor(),
"delegate.mojoDescriptor")
+ .getMojoDescriptorV4();
+ this.executionId = delegate.getExecutionId();
+ this.goal = delegate.getGoal();
+ this.lifecyclePhase = delegate.getLifecyclePhase();
+ this.configuration = delegate.getConfiguration() != null
+ ? delegate.getConfiguration().getDom()
+ : null;
+ this.plugin = buildPlugin(session, delegate);
+ this.model = buildModel(delegate);
}
- public org.apache.maven.plugin.MojoExecution getDelegate() {
- return delegate;
- }
+ private static Plugin buildPlugin(InternalMavenSession session,
org.apache.maven.plugin.MojoExecution delegate) {
+ org.apache.maven.plugin.descriptor.MojoDescriptor legacyDescriptor =
delegate.getMojoDescriptor();
+ org.apache.maven.plugin.descriptor.PluginDescriptor
legacyPluginDescriptor =
+ legacyDescriptor.getPluginDescriptor();
+ PluginDescriptor pluginDescriptorV4 =
legacyPluginDescriptor.getPluginDescriptorV4();
+
+ ClassLoader classLoader = legacyDescriptor.getRealm();
+
+ org.apache.maven.artifact.Artifact legacyArtifact =
legacyPluginDescriptor.getPluginArtifact();
+ org.eclipse.aether.artifact.Artifact resolverArtifact =
RepositoryUtils.toArtifact(legacyArtifact);
+ Artifact artifact = resolverArtifact != null ?
session.getArtifact(resolverArtifact) : null;
+
+ DependencyNode resolverNode =
legacyPluginDescriptor.getDependencyNode();
+ Map<String, Dependency> dependenciesMap = resolverNode != null
+ ? Collections.unmodifiableMap(new DefaultNode(session,
resolverNode, false)
+ .stream()
+ .filter(Objects::nonNull)
+ .map(Node::getDependency)
+ .filter(Objects::nonNull)
+ .collect(Collectors.toMap(
+ d -> d.getGroupId() + ":" +
d.getArtifactId(), d -> d, (a, b) -> a)))
+ : Collections.emptyMap();
+
+ org.apache.maven.api.model.Plugin modelPlugin =
+ delegate.getPlugin() != null ?
delegate.getPlugin().getDelegate() : null;
- @Override
- public Plugin getPlugin() {
return new Plugin() {
@Override
public org.apache.maven.api.model.Plugin getModel() {
- return delegate.getPlugin().getDelegate();
+ return modelPlugin;
}
@Override
public PluginDescriptor getDescriptor() {
- return
delegate.getMojoDescriptor().getPluginDescriptor().getPluginDescriptorV4();
+ return pluginDescriptorV4;
}
Review Comment:
💡 **Behavioral change worth documenting:** The old `Collectors.toMap()`
without a merge function would throw `IllegalStateException` on duplicate
`groupId:artifactId` keys in the dependency graph. The new `(a, b) -> a`
silently picks the first entry. This is a fix (duplicate transitive
dependencies are normal), but a brief comment would help future readers
understand the intent:
```suggestion
.collect(Collectors.toMap(
d -> d.getGroupId() + ":" +
d.getArtifactId(),
d -> d,
(a, b) -> a))) // first-wins on duplicate
groupId:artifactId (transitive duplicates are normal)
```
--
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]
