gnodet-bot commented on code in PR #13366:
URL: https://github.com/apache/maven/pull/13366#discussion_r4215705810
##########
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 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 = delegate.getExecutionId();
Review Comment:
⚠️ **`@Nonnull` contract violation:** `delegate.getExecutionId()` can return
`null` — the legacy `MojoExecution` constructors explicitly set `executionId =
null` for CLI-invoked goals (e.g. `MojoExecution(Plugin plugin, String goal,
String executionId)` is called with a null executionId, and the two-arg
constructors used internally set it to null). This field is stored without a
null guard and returned by `executionId()` which is declared `@Nonnull` in the
interface — callers trusting the annotation may NPE.
```suggestion
this.executionId = requireNonNull(delegate.getExecutionId(),
"delegate.executionId");
```
If `null` is a legitimate value for CLI-invoked goals, the interface
contract should be loosened to `@Nullable` or changed to `Optional<String>`.
--
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]
