From cd0e071602f9a688db4db8450a2631a9ed98dab2 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 10:40:39 +0000 Subject: [PATCH 1/2] [#11606] Fix mvnup spurious pluginManagement injection for plugins inherited from remote parent POM MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a project inherits from a remote parent POM, plugins declared only in the remote parent's build/plugins or pluginManagement should not have local pluginManagement entries injected by mvnup. The project does not control the remote parent, so overriding its plugin versions locally is spurious. The fix collects all plugin keys declared in local POMs (build/plugins and build/pluginManagement/plugins) and skips plugins in the effective model analysis that are not locally declared — they come from remote parents that the project does not control. Plugins that ARE declared locally (even without a version, inheriting from a remote parent's pluginManagement) are still eligible for pluginManagement injection, since the project explicitly uses them. Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/PluginUpgradeStrategy.java | 63 ++++++++- .../goals/PluginUpgradeStrategyTest.java | 129 ++++++++++++------ 2 files changed, 145 insertions(+), 47 deletions(-) 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..2d6044aa9e9a 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 @@ -475,6 +475,7 @@ private PluginAnalysisResults analyzePluginsUsingEffectiveModels( Map> managementResult = new HashMap<>(); Map> directOverrideResult = new HashMap<>(); Map pluginUpgrades = getPluginUpgradesAsMap(); + Set localPluginKeys = collectLocallyDeclaredPluginKeys(pomMap); for (Map.Entry entry : pomMap.entrySet()) { Path originalPomPath = entry.getKey(); @@ -486,7 +487,8 @@ private PluginAnalysisResults analyzePluginsUsingEffectiveModels( Path tempPomPath = tempDir.resolve(relativePath); // Build effective model using Maven 4 API - PluginAnalysis analysis = analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); + PluginAnalysis analysis = + analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades, localPluginKeys); // Determine where to add plugin management (last local parent) Path targetPom = @@ -528,9 +530,12 @@ private Map getPluginUpgradesAsMap() { } private PluginAnalysis analyzeEffectiveModelForPlugins( - UpgradeContext context, Path tempPomPath, Map pluginUpgrades) { + UpgradeContext context, + Path tempPomPath, + Map pluginUpgrades, + Set localPluginKeys) { Model effectiveModel = buildEffectiveModel(tempPomPath); - return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades); + return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades, localPluginKeys); } /** @@ -540,7 +545,10 @@ private PluginAnalysis analyzeEffectiveModelForPlugins( * parent's build/plugins, not via pluginManagement). */ private PluginAnalysis analyzePluginsFromEffectiveModel( - UpgradeContext context, Model effectiveModel, Map pluginUpgrades) { + UpgradeContext context, + Model effectiveModel, + Map pluginUpgrades, + Set localPluginKeys) { Set needsManagement = new HashSet<>(); Set needsDirectOverride = new HashSet<>(); @@ -561,6 +569,13 @@ private PluginAnalysis analyzePluginsFromEffectiveModel( String pluginKey = getPluginKey(plugin); PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); if (upgrade != null) { + // Skip plugins not declared in any local POM — they are inherited + // from a remote parent POM that the project does not control + if (!localPluginKeys.contains(pluginKey)) { + context.debug( + "Plugin " + pluginKey + " is inherited from a remote parent POM — skipping upgrade"); + continue; + } String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { needsManagement.add(pluginKey); @@ -587,6 +602,10 @@ private PluginAnalysis analyzePluginsFromEffectiveModel( String pluginKey = getPluginKey(plugin); PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); if (upgrade != null && !needsManagement.contains(pluginKey)) { + // Skip plugins not declared in any local POM + if (!localPluginKeys.contains(pluginKey)) { + continue; + } String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { needsManagement.add(pluginKey); @@ -808,6 +827,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..4350fcd15a1e 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 not inject pluginManagement for plugins only inherited from remote parent") + void shouldNotInjectPluginManagementForPluginsOnlyInRemoteParent() 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 NOT + // get pluginManagement overrides — the version is locked by the remote parent + // that the project does not control. String pomXml = """ @@ -806,12 +807,15 @@ 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"); + assertEquals( + 0, + result.modifiedCount(), + "Should not modify POM when plugins are only inherited from remote parent"); String xml = DomUtils.toXml(document); - assertTrue( - xml.contains("maven-enforcer-plugin"), - "Should add pluginManagement for maven-enforcer-plugin inherited from parent"); + assertFalse( + xml.contains(""), + "Should not inject pluginManagement for plugins only from remote parent"); } finally { try (var walk = Files.walk(tempDir)) { walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { @@ -825,12 +829,12 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { } @Test - @DisplayName("should not add direct build/plugins override when plugin version comes from pluginManagement") - void shouldNotAddDirectOverrideWhenVersionFromPluginManagement() 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. + @DisplayName("should not inject any overrides for plugins only from remote parent") + void shouldNotInjectAnyOverridesForPluginsOnlyFromRemoteParent() throws Exception { + // org.apache:apache:23 has maven-enforcer-plugin in build/plugins and + // pluginManagement. Since the child POM does not declare the plugin itself, + // mvnup should not inject any overrides — neither pluginManagement nor + // direct build/plugins entries. String pomXml = """ @@ -855,34 +859,9 @@ void shouldNotAddDirectOverrideWhenVersionFromPluginManagement() throws Exceptio assertTrue(result.success(), "Strategy should succeed"); - Editor editor = new Editor(document); - Element root = editor.root(); - - // Verify pluginManagement entry exists for enforcer - Element pmPlugins = - root.path("build", "pluginManagement", "plugins").orElse(null); - assertNotNull(pmPlugins, "Should have pluginManagement/plugins"); - boolean hasEnforcerInPM = pmPlugins.childElements("plugin").anyMatch(p -> "maven-enforcer-plugin" - .equals(p.childElement("artifactId") - .map(Element::textContentTrimmed) - .orElse(""))); - assertTrue(hasEnforcerInPM, "Should have enforcer in pluginManagement"); - - // Verify NO direct build/plugins entry for enforcer (PM override is sufficient) - Element buildPlugins = root.childElement("build") - .flatMap(b -> b.childElement("plugins")) - .orElse(null); - if (buildPlugins != null) { - boolean hasEnforcerInPlugins = buildPlugins - .childElements("plugin") - .anyMatch(p -> "maven-enforcer-plugin" - .equals(p.childElement("artifactId") - .map(Element::textContentTrimmed) - .orElse(""))); - assertFalse( - hasEnforcerInPlugins, - "Should NOT add enforcer in build/plugins when pluginManagement override suffices"); - } + String xml = DomUtils.toXml(document); + assertFalse( + xml.contains(""), "Should not add any build elements for plugins only from remote parent"); } @Test @@ -949,6 +928,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 From e2bdf3ec0d4b25360244bf9c96adf7de7541c6b1 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 15:40:56 +0200 Subject: [PATCH 2/2] Override remote parent plugins with comment instead of skipping Instead of skipping plugins inherited from remote parent POMs, mvnup now adds pluginManagement overrides with a comment "Override version inherited from parent". This ensures Maven 4 incompatible plugin versions get upgraded even when they come from a parent POM the project does not control. Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/PluginUpgradeStrategy.java | 70 +++++++++--------- .../goals/PluginUpgradeStrategyTest.java | 71 ++++++++++++++----- 2 files changed, 86 insertions(+), 55 deletions(-) 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 2d6044aa9e9a..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) { @@ -475,7 +480,6 @@ private PluginAnalysisResults analyzePluginsUsingEffectiveModels( Map> managementResult = new HashMap<>(); Map> directOverrideResult = new HashMap<>(); Map pluginUpgrades = getPluginUpgradesAsMap(); - Set localPluginKeys = collectLocallyDeclaredPluginKeys(pomMap); for (Map.Entry entry : pomMap.entrySet()) { Path originalPomPath = entry.getKey(); @@ -487,8 +491,7 @@ private PluginAnalysisResults analyzePluginsUsingEffectiveModels( Path tempPomPath = tempDir.resolve(relativePath); // Build effective model using Maven 4 API - PluginAnalysis analysis = - analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades, localPluginKeys); + PluginAnalysis analysis = analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); // Determine where to add plugin management (last local parent) Path targetPom = @@ -530,12 +533,9 @@ private Map getPluginUpgradesAsMap() { } private PluginAnalysis analyzeEffectiveModelForPlugins( - UpgradeContext context, - Path tempPomPath, - Map pluginUpgrades, - Set localPluginKeys) { + UpgradeContext context, Path tempPomPath, Map pluginUpgrades) { Model effectiveModel = buildEffectiveModel(tempPomPath); - return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades, localPluginKeys); + return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades); } /** @@ -545,10 +545,7 @@ private PluginAnalysis analyzeEffectiveModelForPlugins( * parent's build/plugins, not via pluginManagement). */ private PluginAnalysis analyzePluginsFromEffectiveModel( - UpgradeContext context, - Model effectiveModel, - Map pluginUpgrades, - Set localPluginKeys) { + UpgradeContext context, Model effectiveModel, Map pluginUpgrades) { Set needsManagement = new HashSet<>(); Set needsDirectOverride = new HashSet<>(); @@ -569,13 +566,6 @@ private PluginAnalysis analyzePluginsFromEffectiveModel( String pluginKey = getPluginKey(plugin); PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); if (upgrade != null) { - // Skip plugins not declared in any local POM — they are inherited - // from a remote parent POM that the project does not control - if (!localPluginKeys.contains(pluginKey)) { - context.debug( - "Plugin " + pluginKey + " is inherited from a remote parent POM — skipping upgrade"); - continue; - } String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { needsManagement.add(pluginKey); @@ -602,10 +592,6 @@ private PluginAnalysis analyzePluginsFromEffectiveModel( String pluginKey = getPluginKey(plugin); PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); if (upgrade != null && !needsManagement.contains(pluginKey)) { - // Skip plugins not declared in any local POM - if (!localPluginKeys.contains(pluginKey)) { - continue; - } String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { needsManagement.add(pluginKey); @@ -716,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; @@ -747,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; } } @@ -781,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)"); + } } /** @@ -794,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; @@ -814,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() 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 4350fcd15a1e..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,12 +773,12 @@ void shouldHaveValidPluginUpgradeDefinitions() throws Exception { class InheritedPluginDetectionTests { @Test - @DisplayName("should not inject pluginManagement for plugins only inherited from remote parent") - void shouldNotInjectPluginManagementForPluginsOnlyInRemoteParent() throws Exception { + @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 NOT - // get pluginManagement overrides — the version is locked by the remote parent - // that the project does not control. + // 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 = """ @@ -807,15 +807,18 @@ void shouldNotInjectPluginManagementForPluginsOnlyInRemoteParent() throws Except UpgradeResult result = strategy.doApply(context, pomMap); assertTrue(result.success(), "Strategy should succeed"); - assertEquals( - 0, - result.modifiedCount(), - "Should not modify POM when plugins are only inherited from remote parent"); + assertTrue(result.modifiedCount() > 0, "Should have modified POM for remote parent plugin upgrade"); String xml = DomUtils.toXml(document); - assertFalse( + assertTrue( xml.contains(""), - "Should not inject pluginManagement for plugins only from remote parent"); + "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"); } finally { try (var walk = Files.walk(tempDir)) { walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> { @@ -829,12 +832,12 @@ void shouldNotInjectPluginManagementForPluginsOnlyInRemoteParent() throws Except } @Test - @DisplayName("should not inject any overrides for plugins only from remote parent") - void shouldNotInjectAnyOverridesForPluginsOnlyFromRemoteParent() throws Exception { - // org.apache:apache:23 has maven-enforcer-plugin in build/plugins and - // pluginManagement. Since the child POM does not declare the plugin itself, - // mvnup should not inject any overrides — neither pluginManagement nor - // direct build/plugins entries. + @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 with a comment in the child + // is sufficient; no direct build/plugins entry should be added for enforcer. String pomXml = """ @@ -859,9 +862,39 @@ void shouldNotInjectAnyOverridesForPluginsOnlyFromRemoteParent() throws Exceptio assertTrue(result.success(), "Strategy should succeed"); + Editor editor = new Editor(document); + Element root = editor.root(); + + // Verify pluginManagement entry exists for enforcer + Element pmPlugins = + root.path("build", "pluginManagement", "plugins").orElse(null); + assertNotNull(pmPlugins, "Should have pluginManagement/plugins"); + boolean hasEnforcerInPM = pmPlugins.childElements("plugin").anyMatch(p -> "maven-enforcer-plugin" + .equals(p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse(""))); + assertTrue(hasEnforcerInPM, "Should have enforcer in pluginManagement"); + String xml = DomUtils.toXml(document); - assertFalse( - xml.contains(""), "Should not add any build elements for plugins only from remote parent"); + 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")) + .orElse(null); + if (buildPlugins != null) { + boolean hasEnforcerInPlugins = buildPlugins + .childElements("plugin") + .anyMatch(p -> "maven-enforcer-plugin" + .equals(p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse(""))); + assertFalse( + hasEnforcerInPlugins, + "Should NOT add enforcer in build/plugins when pluginManagement override suffices"); + } } @Test