gnodet commented on code in PR #13041:
URL: https://github.com/apache/maven/pull/13041#discussion_r3945201910
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultLifecycleBindingsInjector.java:
##########
@@ -86,10 +84,19 @@ public Model injectLifecycleBindings(Model model,
ModelBuilderRequest request, M
Model lifecycleModel = Model.newBuilder()
.build(Build.newBuilder().plugins(allPlugins.values()).build())
.build();
- return merger.merge(model, lifecycleModel);
+ return new
LifecycleBindingsMerger(getPhaseToLifecycleMap()).merge(model, lifecycleModel);
}
}
+ private Map<String, String> getPhaseToLifecycleMap() {
+ Map<String, String> phaseToLifecycle = new HashMap<>();
+ lifecycleRegistry.stream().forEach(lifecycle -> {
+ lifecycleRegistry.computePhases(lifecycle).forEach(phase ->
phaseToLifecycle.put(phase, lifecycle.id()));
+ lifecycle.aliases().forEach(alias ->
phaseToLifecycle.put(alias.v3Phase(), lifecycle.id()));
Review Comment:
π **Consistency with legacy model:** The legacy model's
`getPhaseToLifecycleMap()` delegates to
`DefaultLifecycles.getPhaseToLifecycleMap()`, while the Maven 4 model
implementation computes the map directly from `LifecycleRegistry` including
aliases. This means the two implementations might produce different maps if
alias handling differs.
This is likely fine (the legacy model doesn't have Maven 4 lifecycle
aliases), but a comment explaining the difference would help future maintainers.
##########
impl/maven-core/src/main/java/org/apache/maven/model/plugin/DefaultLifecycleBindingsInjector.java:
##########
@@ -146,6 +160,26 @@ protected void mergePluginContainer_Plugins(
}
}
+ private Plugin mergePluginManagement(Plugin lifecyclePlugin, Plugin
managedPlugin, boolean sourceDominant) {
+ Plugin plugin = managedPlugin.clone();
+ plugin.getExecutions().removeIf(execution ->
!isFromSameLifecycle(lifecyclePlugin, execution));
+ mergePlugin(plugin, lifecyclePlugin, sourceDominant,
Collections.emptyMap());
+ return plugin;
+ }
+
+ private boolean isFromSameLifecycle(Plugin lifecyclePlugin,
PluginExecution managedExecution) {
+ String managedPhase = managedExecution.getPhase();
+ if (managedPhase == null) {
+ return true;
+ }
+
+ String managedLifecycle = phaseToLifecycle.get(managedPhase);
+ return lifecyclePlugin.getExecutions().stream()
+ .anyMatch(execution ->
managedPhase.equals(execution.getPhase())
+ || managedLifecycle != null
+ &&
managedLifecycle.equals(phaseToLifecycle.get(execution.getPhase())));
+ }
Review Comment:
π **Edge case to consider:** When `managedPhase` is a custom/unknown phase
not in `phaseToLifecycleMap`, `managedLifecycle` is `null` and the logic falls
back to exact phase matching only. This means a managed execution bound to a
custom phase (e.g. from a lifecycle extension) will be filtered out unless a
lifecycle execution is bound to the exact same phase.
Is this the intended behavior? It seems reasonable (err on the side of not
activating unknown phases), but worth documenting as a conscious design
decision, especially since custom lifecycle extensions exist in the wild.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng5359PluginManagementExecutionTest.java:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.it;
+
+import java.nio.file.Path;
+
+import org.junit.jupiter.api.Test;
+
+/**
+ * Verifies that executions declared only in {@code pluginManagement} are not
activated by default lifecycle bindings.
+ *
+ * @see <a href="https://github.com/apache/maven/issues/6918";>MNG-5359</a>
+ * @since 4.1.0
+ */
+class MavenITmng5359PluginManagementExecutionTest extends
AbstractMavenIntegrationTestCase {
+
+ @Test
+ void testManagedExecutionRequiresPluginDeclaration() throws Exception {
+ Path testDir = extractResources("mng-5359");
+
+ Verifier verifier = newVerifier(testDir);
+ verifier.setAutoclean(false);
+ verifier.deleteDirectory("target");
+ verifier.addCliArgument("package");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+ verifier.verifyFileNotPresent("target/managed-clean.txt");
+
+ verifier = newVerifier(testDir);
+ verifier.setAutoclean(false);
+ verifier.deleteDirectory("target");
+ verifier.addCliArgument("-Pactivate-clean-plugin");
+ verifier.addCliArgument("package");
+ verifier.execute();
+ verifier.verifyErrorFreeLog();
+ verifier.verifyFilePresent("target/managed-clean.txt");
+ }
Review Comment:
π‘ **Suggestion:** Both test phases could use separate test methods (e.g.,
`testManagedExecutionNotActivatedWithoutDeclaration` and
`testManagedExecutionActivatedWithExplicitDeclaration`) for clearer test
isolation and failure reporting. If one phase fails, you immediately know which
scenario broke.
Also, the test uses `package` as the target phase, but the managed execution
is also bound to `package` β so the test verifies that a managed `clean` plugin
execution bound to `package` (a default lifecycle phase) is NOT activated when
the clean plugin is introduced only via lifecycle bindings (clean lifecycle).
This is a good cross-lifecycle test. A brief comment explaining this would help
readability.
##########
impl/maven-core/src/main/java/org/apache/maven/model/plugin/DefaultLifecycleBindingsInjector.java:
##########
@@ -75,17 +77,31 @@ public void injectLifecycleBindings(Model model,
ModelBuildingRequest request, M
lifecycleModel.setBuild(new Build());
lifecycleModel.getBuild().getPlugins().addAll(defaultPlugins);
- merger.merge(model, lifecycleModel);
+ new LifecycleBindingsMerger(getPhaseToLifecycleMap()).merge(model,
lifecycleModel);
Review Comment:
π‘ **Performance:** `getPhaseToLifecycleMap()` is called on every
`injectLifecycleBindings` invocation, creating a new `HashMap` and a new
`LifecycleBindingsMerger` each time. The previous code cached a single
`LifecycleBindingsMerger` as a field.
Since the phase-to-lifecycle map doesn't change after startup, this could be
computed once in the constructor:
```suggestion
new LifecycleBindingsMerger(phaseToLifecycleMap).merge(model,
lifecycleModel);
```
β¦with `phaseToLifecycleMap` as a `final` field initialized in the
constructor. Not critical β model building isn't in a tight loop β but it's a
free optimization.
##########
impl/maven-core/src/test/java/org/apache/maven/model/plugin/DefaultLifecycleBindingsInjectorTest.java:
##########
@@ -0,0 +1,167 @@
+/*
+ * 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.model.plugin;
+
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+
+import org.apache.maven.model.Build;
+import org.apache.maven.model.Dependency;
+import org.apache.maven.model.InputLocation;
+import org.apache.maven.model.InputSource;
+import org.apache.maven.model.Model;
+import org.apache.maven.model.Plugin;
+import org.apache.maven.model.PluginExecution;
+import org.apache.maven.model.PluginManagement;
+import org.codehaus.plexus.util.xml.Xpp3Dom;
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+class DefaultLifecycleBindingsInjectorTest {
+
+ @Test
+ void mergePluginManagementOnlyActivatesExecutionsFromTheSameLifecycle() {
+ InputSource lifecycleSource = inputSource("lifecycle");
+ InputSource managementSource = inputSource("plugin-management");
+
+ PluginExecution lifecycleExecution = execution("default-clean",
"clean", lifecycleSource);
+ Plugin lifecyclePlugin = plugin("lifecycle-version",
lifecycleExecution, lifecycleSource);
+ lifecyclePlugin.setConfiguration(configuration("shared", "lifecycle",
"lifecycle", "default"));
+
+ PluginExecution sameLifecycleExecution = execution("managed-clean",
"clean", managementSource);
+ PluginExecution crossLifecycleExecution =
execution("managed-initialize", "initialize", managementSource);
+ PluginExecution defaultPhaseExecution =
execution("managed-default-phase", null, managementSource);
+ Plugin managedPlugin = plugin(
+ "managed-version",
+ managementSource,
+ sameLifecycleExecution,
+ crossLifecycleExecution,
+ defaultPhaseExecution);
+ managedPlugin.setConfiguration(configuration("shared", "managed",
"managed", "configured"));
+ managedPlugin.setExtensions(true);
+ managedPlugin.setInherited(false);
+ managedPlugin.addDependency(dependency("managed-dependency"));
+
+ Model target = modelWithPluginManagement(managedPlugin);
+ Model source = modelWithPlugin(lifecyclePlugin);
+
+ new
DefaultLifecycleBindingsInjector.LifecycleBindingsMerger(Map.of("clean",
"clean", "initialize", "default"))
+ .merge(target, source);
+
+ Plugin result = target.getBuild().getPlugins().get(0);
+ Xpp3Dom resultConfiguration = (Xpp3Dom) result.getConfiguration();
+
+ assertEquals("managed-version", result.getVersion());
+ assertEquals("managed",
resultConfiguration.getChild("shared").getValue());
+ assertEquals("configured",
resultConfiguration.getChild("managed").getValue());
+ assertEquals("default",
resultConfiguration.getChild("lifecycle").getValue());
+ assertEquals(3, result.getExecutions().size());
+ assertEquals(
+ Set.of("default-clean", "managed-clean",
"managed-default-phase"),
+
result.getExecutions().stream().map(PluginExecution::getId).collect(Collectors.toSet()));
+ assertEquals(
+ List.of("managed-dependency"),
+
result.getDependencies().stream().map(Dependency::getArtifactId).toList());
+ assertEquals("true", result.getExtensions());
+ assertEquals("false", result.getInherited());
+ assertEquals("plugin-management",
result.getLocation("").getSource().getModelId());
+ assertEquals(
+ "plugin-management",
result.getLocation("version").getSource().getModelId());
+ assertEquals(
+ "lifecycle",
+ result.getExecutions().stream()
+ .filter(execution ->
"default-clean".equals(execution.getId()))
Review Comment:
π‘ **Nit:** The test verifies the result has 3 executions and checks the
expected set, but doesn't verify that the *filtered* execution
(`managed-initialize`) is absent. While the `Set.of(...)` assertion implicitly
covers this (3 elements, none is `managed-initialize`), an explicit
`assertFalse` would make the intent clearer:
```java
assertFalse(result.getExecutions().stream()
.anyMatch(e -> "managed-initialize".equals(e.getId())),
"Cross-lifecycle managed execution should be filtered out");
```
--
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]
