diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java index 3402b78c2fa9..8df694b2e246 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java @@ -26,6 +26,7 @@ import java.util.Set; import java.util.stream.Collectors; +import eu.maveniverse.domtrip.Comment; import eu.maveniverse.domtrip.Document; import eu.maveniverse.domtrip.Editor; import eu.maveniverse.domtrip.Element; @@ -132,6 +133,9 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) // Phase 2: For each POM, build effective model using the session and analyze plugins PluginAnalysisResults analysisResults = analyzePluginsUsingEffectiveModels(context, pomMap, tempDir); + // Collect locally declared plugin keys so we can add comments for remote-parent overrides + Set localPluginKeys = collectLocallyDeclaredPluginKeys(pomMap); + // Phase 3: Add plugin management and direct overrides to the last local parent in hierarchy for (Map.Entry entry : pomMap.entrySet()) { Path pomPath = entry.getKey(); @@ -151,8 +155,8 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) Set pluginsForManagement = analysisResults.pluginsNeedingManagement().get(pomPath); if (pluginsForManagement != null && !pluginsForManagement.isEmpty()) { - hasUpgrades |= - addPluginManagementForEffectivePlugins(context, pomDocument, pluginsForManagement); + hasUpgrades |= addPluginManagementForEffectivePlugins( + context, pomDocument, pluginsForManagement, localPluginKeys); context.detail("Added plugin management to " + pomPath + " (target parent for " + pluginsForManagement.size() + " plugins)"); } @@ -162,7 +166,8 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) Set pluginsForDirectOverride = analysisResults.pluginsNeedingDirectOverride().get(pomPath); if (pluginsForDirectOverride != null && !pluginsForDirectOverride.isEmpty()) { - hasUpgrades |= addDirectPluginOverrides(context, pomDocument, pluginsForDirectOverride); + hasUpgrades |= addDirectPluginOverrides( + context, pomDocument, pluginsForDirectOverride, localPluginKeys); } if (hasUpgrades) { @@ -697,7 +702,7 @@ private Path findParentInPomMap(Parent parent, Map pomMap) { * Adds plugin management entries for plugins found through effective model analysis. */ private boolean addPluginManagementForEffectivePlugins( - UpgradeContext context, Document pomDocument, Set pluginKeys) { + UpgradeContext context, Document pomDocument, Set pluginKeys, Set localPluginKeys) { Map pluginUpgrades = getPluginUpgradesAsMap(); boolean hasUpgrades = false; @@ -728,7 +733,8 @@ private boolean addPluginManagementForEffectivePlugins( if (upgrade != null) { // Check if plugin is already managed if (!isPluginAlreadyManagedInElement(managedPluginsElement, upgrade)) { - addPluginManagementEntryFromUpgrade(managedPluginsElement, upgrade, context); + boolean fromRemoteParent = !localPluginKeys.contains(pluginKey); + addPluginManagementEntryFromUpgrade(managedPluginsElement, upgrade, context, fromRemoteParent); hasUpgrades = true; } } @@ -762,12 +768,18 @@ private boolean isPluginAlreadyManagedInElement(Element pluginsElement, PluginUp * Adds a plugin management entry from a PluginUpgrade. */ private void addPluginManagementEntryFromUpgrade( - Element managedPluginsElement, PluginUpgrade upgrade, UpgradeContext context) { - // Create plugin element using DomUtils convenience method for proper formatting - DomUtils.createPlugin(managedPluginsElement, upgrade.groupId(), upgrade.artifactId(), upgrade.minVersion()); - - context.detail("Added plugin management for " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " - + upgrade.minVersion() + " (found through effective model analysis)"); + Element managedPluginsElement, PluginUpgrade upgrade, UpgradeContext context, boolean fromRemoteParent) { + Element plugin = DomUtils.createPlugin( + managedPluginsElement, upgrade.groupId(), upgrade.artifactId(), upgrade.minVersion()); + + if (fromRemoteParent) { + managedPluginsElement.insertChildBefore(plugin, Comment.of(" Override version inherited from parent ")); + context.detail("Added plugin management for " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " + + upgrade.minVersion() + " (overrides version inherited from parent)"); + } else { + context.detail("Added plugin management for " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " + + upgrade.minVersion() + " (found through effective model analysis)"); + } } /** @@ -775,7 +787,8 @@ private void addPluginManagementEntryFromUpgrade( * This is necessary when a parent POM sets an explicit version in its build/plugins * that pluginManagement alone cannot override. */ - private boolean addDirectPluginOverrides(UpgradeContext context, Document pomDocument, Set pluginKeys) { + private boolean addDirectPluginOverrides( + UpgradeContext context, Document pomDocument, Set pluginKeys, Set localPluginKeys) { Map pluginUpgrades = getPluginUpgradesAsMap(); boolean hasUpgrades = false; @@ -795,8 +808,12 @@ private boolean addDirectPluginOverrides(UpgradeContext context, Document pomDoc PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); if (upgrade != null) { if (!isPluginAlreadyManagedInElement(pluginsElement, upgrade)) { - DomUtils.createPlugin( + Element plugin = DomUtils.createPlugin( pluginsElement, upgrade.groupId(), upgrade.artifactId(), upgrade.minVersion()); + if (!localPluginKeys.contains(pluginKey)) { + pluginsElement.insertChildBefore( + plugin, Comment.of(" Override version inherited from parent ")); + } hasUpgrades = true; context.detail("Added " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " + upgrade.minVersion() @@ -808,6 +825,42 @@ private boolean addDirectPluginOverrides(UpgradeContext context, Document pomDoc return hasUpgrades; } + private Set collectLocallyDeclaredPluginKeys(Map pomMap) { + Set localPluginKeys = new HashSet<>(); + for (Document doc : pomMap.values()) { + Element root = doc.root(); + Element buildElement = root.childElement(BUILD).orElse(null); + if (buildElement != null) { + Element pluginsElement = buildElement.childElement(PLUGINS).orElse(null); + if (pluginsElement != null) { + collectPluginKeysFromElement(pluginsElement, localPluginKeys); + } + Element pmElement = buildElement.childElement(PLUGIN_MANAGEMENT).orElse(null); + if (pmElement != null) { + Element managedPluginsElement = + pmElement.childElement(PLUGINS).orElse(null); + if (managedPluginsElement != null) { + collectPluginKeysFromElement(managedPluginsElement, localPluginKeys); + } + } + } + } + return localPluginKeys; + } + + private void collectPluginKeysFromElement(Element pluginsElement, Set keys) { + pluginsElement.childElements(PLUGIN).forEach(pluginElement -> { + String groupId = getChildText(pluginElement, GROUP_ID); + String artifactId = getChildText(pluginElement, ARTIFACT_ID); + if (groupId == null && artifactId != null && artifactId.startsWith(MAVEN_PLUGIN_PREFIX)) { + groupId = DEFAULT_MAVEN_PLUGIN_GROUP_ID; + } + if (groupId != null && artifactId != null) { + keys.add(groupId + ":" + artifactId); + } + }); + } + private record PluginAnalysis(Set needsManagement, Set needsDirectOverride) {} private record PluginAnalysisResults( diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java index 3702a07e05ac..5315cf1ad8b2 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java @@ -773,11 +773,12 @@ void shouldHaveValidPluginUpgradeDefinitions() throws Exception { class InheritedPluginDetectionTests { @Test - @DisplayName("should detect inherited plugins from remote parent POM and add pluginManagement") - void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { - // org.apache:apache:23 defines maven-enforcer-plugin:1.4.1 in pluginManagement. - // A child POM that inherits from this parent should get pluginManagement overrides - // added by mvnup for plugins that need Maven 4 compatibility upgrades. + @DisplayName("should inject pluginManagement with comment for plugins inherited from remote parent") + void shouldInjectPluginManagementWithCommentForRemoteParentPlugins() throws Exception { + // org.apache:apache:23 defines maven-enforcer-plugin in pluginManagement and + // build/plugins. A child POM that does NOT itself declare the plugin should + // still get a pluginManagement override with a comment explaining it overrides + // the parent, so that Maven 4 incompatible plugin versions get upgraded. String pomXml = """ @@ -806,12 +807,18 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { UpgradeResult result = strategy.doApply(context, pomMap); assertTrue(result.success(), "Strategy should succeed"); - assertTrue(result.modifiedCount() > 0, "Should have added plugin management for inherited plugins"); + assertTrue(result.modifiedCount() > 0, "Should have modified POM for remote parent plugin upgrade"); String xml = DomUtils.toXml(document); + assertTrue( + xml.contains(""), + "Should inject pluginManagement for plugins from remote parent"); + assertTrue( + xml.contains("Override version inherited from parent"), + "Should add comment explaining the override"); assertTrue( xml.contains("maven-enforcer-plugin"), - "Should add pluginManagement for maven-enforcer-plugin inherited from parent"); + "Should add pluginManagement for maven-enforcer-plugin"); } finally { try (var walk = Files.walk(tempDir)) { walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { @@ -825,12 +832,12 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { } @Test - @DisplayName("should not add direct build/plugins override when plugin version comes from pluginManagement") - void shouldNotAddDirectOverrideWhenVersionFromPluginManagement() throws Exception { + @DisplayName("should override remote parent plugin via pluginManagement with comment, not direct build/plugins") + void shouldOverrideRemoteParentPluginViaPluginManagementWithComment() throws Exception { // org.apache:apache:23 has maven-enforcer-plugin in build/plugins WITHOUT // an explicit version — the version (1.4.1) comes from pluginManagement. - // In this case, adding a pluginManagement override in the child is sufficient; - // no direct build/plugins entry should be added for enforcer. + // In this case, adding a pluginManagement override with a comment in the child + // is sufficient; no direct build/plugins entry should be added for enforcer. String pomXml = """ @@ -868,6 +875,11 @@ void shouldNotAddDirectOverrideWhenVersionFromPluginManagement() throws Exceptio .orElse(""))); assertTrue(hasEnforcerInPM, "Should have enforcer in pluginManagement"); + String xml = DomUtils.toXml(document); + assertTrue( + xml.contains("Override version inherited from parent"), + "Should add comment explaining the override"); + // Verify NO direct build/plugins entry for enforcer (PM override is sufficient) Element buildPlugins = root.childElement("build") .flatMap(b -> b.childElement("plugins")) @@ -949,6 +961,70 @@ void shouldNotDuplicatePluginInBuildPluginsWhenAlreadyDeclared() throws Exceptio .orElse(null); assertEquals("3.5.0", version, "Existing enforcer-plugin version should be upgraded to 3.5.0"); } + + @Test + @DisplayName("should inject pluginManagement when plugin is locally declared without version") + void shouldInjectPluginManagementForLocallyDeclaredPluginWithoutVersion() throws Exception { + // Child POM explicitly declares maven-enforcer-plugin in build/plugins without + // a version. The version comes from the remote parent's pluginManagement. + // Since the child does declare the plugin, mvnup should add a pluginManagement + // entry to override the inherited version. + String pomXml = """ + + + 4.0.0 + + org.apache + apache + 23 + + org.example + test-child + 1.0.0-SNAPSHOT + + + + org.apache.maven.plugins + maven-enforcer-plugin + + + + + """; + + Path tempDir = Files.createTempDirectory("mvnup-test-"); + try { + Files.createDirectories(tempDir.resolve(".mvn")); + Path pomPath = tempDir.resolve("pom.xml"); + Files.writeString(pomPath, pomXml); + + Document document = Document.of(pomXml); + Map pomMap = Map.of(pomPath, document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Strategy should succeed"); + assertTrue( + result.modifiedCount() > 0, "Should have added pluginManagement for locally declared plugin"); + + String xml = DomUtils.toXml(document); + assertTrue( + xml.contains(""), "Should add pluginManagement for locally declared plugin"); + assertTrue( + xml.contains("maven-enforcer-plugin"), + "Should add pluginManagement for maven-enforcer-plugin"); + } finally { + try (var walk = Files.walk(tempDir)) { + walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { + try { + Files.delete(p); + } catch (IOException ignored) { + } + }); + } + } + } } @Nested