gnodet-bot commented on code in PR #13366:
URL: https://github.com/apache/maven/pull/13366#discussion_r4215986565
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMojoExecution.java:
##########
@@ -38,110 +38,156 @@
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 Optional<String> executionId;
+ private final String goal;
+ private final Optional<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 = Optional.ofNullable(delegate.getExecutionId());
+ this.goal = delegate.getGoal();
Review Comment:
💡 **No unit test for the immutable snapshot constructor**
The new constructor (immutable snapshot semantics) is not covered by any
test. In particular, the CLI-invocation path — where `executionId`,
`lifecyclePhase`, and `model` are all empty — is a meaningful edge case worth
asserting. A small `DefaultMojoExecutionTest` covering at least that scenario
and one normal execution would make the contract explicit and protect against
future regressions.
##########
impl/maven-core/src/main/java/org/apache/maven/internal/impl/DefaultMojoExecution.java:
##########
@@ -38,110 +38,156 @@
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 Optional<String> executionId;
+ private final String goal;
+ private final Optional<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 = Optional.ofNullable(delegate.getExecutionId());
+ this.goal = delegate.getGoal();
Review Comment:
💡 **Nullability contract gap — `goal` field**
`goal()` is declared `@Nonnull` in the interface, but `this.goal =
delegate.getGoal()` stores the value without a null guard. All other
contract-critical fields (e.g. `descriptor`) use `requireNonNull(...)`, so the
inconsistency is noticeable.
`MojoDescriptor.getGoal()` is a plain setter-backed field — nothing enforces
that it is non-null before the execution reaches this constructor. Consider:
```suggestion
this.goal = requireNonNull(delegate.getGoal(), "delegate.goal");
```
--
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]
