gnodet-bot commented on code in PR #13366:
URL: https://github.com/apache/maven/pull/13366#discussion_r4228086667
##########
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 = requireNonNull(delegate.getGoal(), "delegate.goal");
+ this.lifecyclePhase =
Optional.ofNullable(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))) // first-wins on
duplicate groupId:artifactId
+ // (transitive duplicates are normal)
+ : 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;
}
@Override
public List<Lifecycle> getLifecycles() {
try {
- return Collections.unmodifiableList(new
ArrayList<>(delegate.getMojoDescriptor()
- .getPluginDescriptor()
- .getLifecycleMappings()
- .values()));
+ return Collections.unmodifiableList(new ArrayList<>(
+
legacyPluginDescriptor.getLifecycleMappings().values()));
Review Comment:
⚠️ **Immutability violation — `getLifecycles()` lazily delegates to mutable
legacy object**
All other fields in the anonymous `Plugin` class (`dependenciesMap`,
`artifact`, `modelPlugin`, `classLoader`, `pluginDescriptorV4`) are eagerly
captured into `final` local variables before the class is created, then stored
directly. But `getLifecycles()` still captures `legacyPluginDescriptor` (the
mutable `PluginDescriptor`) and calls `getLifecycleMappings()` lazily on every
invocation.
This means:
- The `Plugin` object returned by `plugin()` is **not immutable** — its
lifecycle list can change if the underlying `legacyPluginDescriptor` is mutated
after snapshot
- The `@Immutable` annotation on `DefaultMojoExecution` is still not fully
honest
Fix: eagerly capture lifecycle mappings at construction time, just like the
other fields:
```suggestion
return Collections.unmodifiableList(new
ArrayList<>(legacyPluginDescriptor.getLifecycleMappings().values()));
```
Consider replacing it with:
```java
List<Lifecycle> lifecycles;
try {
lifecycles = Collections.unmodifiableList(
new
ArrayList<>(legacyPluginDescriptor.getLifecycleMappings().values()));
} catch (Exception e) {
throw new RuntimeException("Unable to load plugin lifecycles", e);
}
final List<Lifecycle> capturedLifecycles = lifecycles;
```
then returning `capturedLifecycles` from `getLifecycles()`.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/reflection/ReflectionValueExtractor.java:
##########
@@ -280,6 +281,12 @@ private static Object getPropertyValue(Object value,
String property) throws Int
return method.invoke(value, OBJECT_ARGS);
}
}
+ // Also support noun-based accessor style (e.g. plugin(),
descriptor(), goal())
+ // where the method name matches the property name exactly.
+ Method method = classMap.findMethod(property);
Review Comment:
💡 **Noun-based fallback applies globally — consider documenting the scope
and risks**
The fallback at this line applies to `getPropertyValue(Object, String)`
which is called for **any** object in the expression tree, not just
`MojoExecution`. This means `${project.plugin}`, `${session.goal()}`, etc.
would all resolve via noun-based lookup if a zero-arg method with a matching
name exists.
Two minor risks:
1. **Naming collision** — a class with an accidental method named exactly
like a common property (e.g. `type()`, `name()`, `value()`) would silently
match and return unexpected data
2. **Zero-arg methods on `Object`** — depending on `ClassMap.findMethod()`
internals, methods inherited from `Object` (e.g. if `getClass()` is somehow
mapped) could be reached, though this is unlikely with good filtering
Suggestion: add a brief Javadoc to `getPropertyValue` or the block
explaining that noun-based lookup is intentional and global, so future
maintainers don't inadvertently remove it as dead code or are surprised by the
scope.
Also worth adding a unit test that exercises `${mojo.plugin}` /
`${mojo.goal}` / `${mojo.descriptor}` through `ReflectionValueExtractor`
directly (not just via `PluginParameterExpressionEvaluatorV4Test`) to lock in
the intended behaviour.
--
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]
