From ccfd0318d4a2b83740a91ee732b414bb8791d19b Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 27 May 2026 00:51:44 +0200 Subject: [PATCH] Fix mvnup plugin upgrade for plugins inherited in build/plugins When a remote parent POM declares a plugin with an explicit version in , adding a entry in the child is insufficient to override it. The parent's version takes precedence over the child's . Now the plugin upgrade strategy adds plugins to both (for downstream children) and (to override the inherited version) when the effective model shows outdated plugins in the build/plugins section. Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/PluginUpgradeStrategy.java | 118 ++++++++++++---- .../goals/PluginUpgradeStrategyTest.java | 127 ++++++++++++++++++ 2 files changed, 219 insertions(+), 26 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 6a6d16cb1510..cc40e5b0adcc 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 @@ -131,10 +131,9 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) Path tempDir = createTempProjectStructure(context, pomMap); // Phase 2: For each POM, build effective model using the session and analyze plugins - Map> pluginsNeedingManagement = - analyzePluginsUsingEffectiveModels(context, pomMap, tempDir); + PluginAnalysisResults analysisResults = analyzePluginsUsingEffectiveModels(context, pomMap, tempDir); - // Phase 3: Add plugin management to the last local parent in hierarchy + // 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(); Document pomDocument = entry.getValue(); @@ -150,14 +149,21 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) hasUpgrades |= upgradePluginsInDocument(pomDocument, context); // Add plugin management based on effective model analysis - // Note: pluginsNeedingManagement only contains entries for POMs that should receive plugin - // management - // (i.e., the "last local parent" for each plugin that needs management) - Set pluginsForThisPom = pluginsNeedingManagement.get(pomPath); - if (pluginsForThisPom != null && !pluginsForThisPom.isEmpty()) { - hasUpgrades |= addPluginManagementForEffectivePlugins(context, pomDocument, pluginsForThisPom); + Set pluginsForManagement = + analysisResults.pluginsNeedingManagement().get(pomPath); + if (pluginsForManagement != null && !pluginsForManagement.isEmpty()) { + hasUpgrades |= + addPluginManagementForEffectivePlugins(context, pomDocument, pluginsForManagement); context.detail("Added plugin management to " + pomPath + " (target parent for " - + pluginsForThisPom.size() + " plugins)"); + + pluginsForManagement.size() + " plugins)"); + } + + // Add direct plugin overrides in build/plugins for inherited plugins + // whose versions cannot be overridden via pluginManagement alone + Set pluginsForDirectOverride = + analysisResults.pluginsNeedingDirectOverride().get(pomPath); + if (pluginsForDirectOverride != null && !pluginsForDirectOverride.isEmpty()) { + hasUpgrades |= addDirectPluginOverrides(context, pomDocument, pluginsForDirectOverride); } if (hasUpgrades) { @@ -462,11 +468,15 @@ public static List getPluginUpgrades() { /** * Analyzes plugins using effective models built from the temp directory. - * Returns a map of POM path to the set of plugin keys that need management. + * Returns a PluginAnalysisResults with two maps: + * - pluginsNeedingManagement: plugins that need pluginManagement entries + * - pluginsNeedingDirectOverride: plugins from build/plugins that need direct overrides + * (because pluginManagement alone cannot override versions inherited from a parent's build/plugins) */ - private Map> analyzePluginsUsingEffectiveModels( + private PluginAnalysisResults analyzePluginsUsingEffectiveModels( UpgradeContext context, Map pomMap, Path tempDir) { - Map> result = new HashMap<>(); + Map> managementResult = new HashMap<>(); + Map> directOverrideResult = new HashMap<>(); Map pluginUpgrades = getPluginUpgradesAsMap(); for (Map.Entry entry : pomMap.entrySet()) { @@ -479,20 +489,28 @@ private Map> analyzePluginsUsingEffectiveModels( Path tempPomPath = tempDir.resolve(relativePath); // Build effective model using Maven 4 API - Set pluginsNeedingUpgrade = - analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); + PluginAnalysis analysis = analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); // Determine where to add plugin management (last local parent) Path targetPomForManagement = findLastLocalParentForPluginManagement(context, tempPomPath, pomMap, tempDir, commonRoot); if (targetPomForManagement != null) { - result.computeIfAbsent(targetPomForManagement, k -> new HashSet<>()) - .addAll(pluginsNeedingUpgrade); + managementResult + .computeIfAbsent(targetPomForManagement, k -> new HashSet<>()) + .addAll(analysis.allPluginsNeedingUpgrade()); - if (!pluginsNeedingUpgrade.isEmpty()) { + directOverrideResult + .computeIfAbsent(targetPomForManagement, k -> new HashSet<>()) + .addAll(analysis.buildPluginsNeedingUpgrade()); + + if (!analysis.allPluginsNeedingUpgrade().isEmpty()) { context.debug("Will add plugin management to " + targetPomForManagement + " for plugins: " - + pluginsNeedingUpgrade); + + analysis.allPluginsNeedingUpgrade()); + } + if (!analysis.buildPluginsNeedingUpgrade().isEmpty()) { + context.debug("Will add direct plugin overrides to " + targetPomForManagement + " for plugins: " + + analysis.buildPluginsNeedingUpgrade()); } } @@ -501,7 +519,7 @@ private Map> analyzePluginsUsingEffectiveModels( } } - return result; + return new PluginAnalysisResults(managementResult, directOverrideResult); } /** @@ -513,7 +531,7 @@ private Map getPluginUpgradesAsMap() { upgrade -> upgrade.groupId() + ":" + upgrade.artifactId(), upgrade -> upgrade)); } - private Set analyzeEffectiveModelForPlugins( + private PluginAnalysis analyzeEffectiveModelForPlugins( UpgradeContext context, Path tempPomPath, Map pluginUpgrades) { Model effectiveModel = buildEffectiveModel(tempPomPath); return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades); @@ -521,10 +539,14 @@ private Set analyzeEffectiveModelForPlugins( /** * Analyzes plugins from the effective model and determines which ones need upgrades. + * Returns a PluginAnalysis with two sets: + * - allPluginsNeedingUpgrade: plugins from both build/plugins and build/pluginManagement/plugins + * - buildPluginsNeedingUpgrade: plugins from build/plugins only (these may need direct overrides) */ - private Set analyzePluginsFromEffectiveModel( + private PluginAnalysis analyzePluginsFromEffectiveModel( UpgradeContext context, Model effectiveModel, Map pluginUpgrades) { - Set pluginsNeedingUpgrade = new HashSet<>(); + Set allPluginsNeedingUpgrade = new HashSet<>(); + Set buildPluginsNeedingUpgrade = new HashSet<>(); Build build = effectiveModel.getBuild(); if (build != null) { @@ -535,7 +557,8 @@ private Set analyzePluginsFromEffectiveModel( if (upgrade != null) { String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { - pluginsNeedingUpgrade.add(pluginKey); + allPluginsNeedingUpgrade.add(pluginKey); + buildPluginsNeedingUpgrade.add(pluginKey); context.debug("Plugin " + pluginKey + " version " + effectiveVersion + " needs upgrade to " + upgrade.minVersion()); } @@ -551,7 +574,7 @@ private Set analyzePluginsFromEffectiveModel( if (upgrade != null) { String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { - pluginsNeedingUpgrade.add(pluginKey); + allPluginsNeedingUpgrade.add(pluginKey); context.debug("Managed plugin " + pluginKey + " version " + effectiveVersion + " needs upgrade to " + upgrade.minVersion()); } @@ -560,7 +583,7 @@ private Set analyzePluginsFromEffectiveModel( } } - return pluginsNeedingUpgrade; + return new PluginAnalysis(allPluginsNeedingUpgrade, buildPluginsNeedingUpgrade); } /** @@ -732,6 +755,49 @@ private void addPluginManagementEntryFromUpgrade( + upgrade.minVersion() + " (found through effective model analysis)"); } + /** + * Adds direct plugin entries in build/plugins for plugins inherited from remote parents. + * This is necessary because pluginManagement alone cannot override plugin versions + * that are explicitly set in an inherited parent's build/plugins section. + */ + private boolean addDirectPluginOverrides(UpgradeContext context, Document pomDocument, Set pluginKeys) { + Map pluginUpgrades = getPluginUpgradesAsMap(); + boolean hasUpgrades = false; + + Element root = pomDocument.root(); + + Element buildElement = root.childElement(BUILD).orElse(null); + if (buildElement == null) { + buildElement = DomUtils.insertNewElement(BUILD, root); + } + + Element pluginsElement = buildElement.childElement(PLUGINS).orElse(null); + if (pluginsElement == null) { + pluginsElement = DomUtils.insertNewElement(PLUGINS, buildElement); + } + + for (String pluginKey : pluginKeys) { + PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); + if (upgrade != null) { + if (!isPluginAlreadyManagedInElement(pluginsElement, upgrade)) { + DomUtils.createPlugin( + pluginsElement, upgrade.groupId(), upgrade.artifactId(), upgrade.minVersion()); + hasUpgrades = true; + context.detail("Added plugin " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " + + upgrade.minVersion() + + " in build/plugins (overrides inherited version from remote parent)"); + } + } + } + + return hasUpgrades; + } + + private record PluginAnalysis(Set allPluginsNeedingUpgrade, Set buildPluginsNeedingUpgrade) {} + + private record PluginAnalysisResults( + Map> pluginsNeedingManagement, Map> pluginsNeedingDirectOverride) {} + /** * Holds plugin upgrade information for Maven 4 compatibility. * This class contains the minimum version requirements for plugins 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 8a6aa1a47098..e978b09ba004 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 @@ -808,6 +808,133 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { xml.contains("maven-enforcer-plugin"), "Should add pluginManagement for maven-enforcer-plugin inherited from parent"); } + + @Test + @DisplayName("should add direct build/plugins override for inherited plugins from remote parent") + void shouldAddDirectBuildPluginsOverrideForInheritedPlugins() throws Exception { + // When a remote parent declares a plugin in build/plugins (not just pluginManagement), + // adding a pluginManagement entry in the child is insufficient to override the version. + // mvnup should also add the plugin directly in the child's build/plugins section. + // org.apache:apache:23 has maven-enforcer-plugin in its effective build/plugins, + // so the child should get BOTH pluginManagement AND build/plugins entries. + String pomXml = """ + + + 4.0.0 + + org.apache + apache + 23 + + org.example + test-child + 1.0.0-SNAPSHOT + + """; + + Document document = Document.of(pomXml); + Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath(); + 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 overrides for inherited plugins"); + + Editor editor = new Editor(document); + Element root = editor.root(); + + // Verify pluginManagement entry exists + Element pmPlugin = + root.path("build", "pluginManagement", "plugins", "plugin").orElse(null); + assertNotNull(pmPlugin, "Should have a pluginManagement/plugins/plugin entry"); + + // Verify build/plugins entry exists (direct override for inherited plugin) + Element buildPluginsElement = root.childElement("build") + .flatMap(b -> b.childElement("plugins")) + .orElse(null); + assertNotNull(buildPluginsElement, "Should have a build/plugins section"); + + boolean hasEnforcerInBuildPlugins = buildPluginsElement + .childElements("plugin") + .anyMatch(p -> p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse("") + .equals("maven-enforcer-plugin")); + assertTrue( + hasEnforcerInBuildPlugins, + "Should add maven-enforcer-plugin in build/plugins to override inherited version"); + } + + @Test + @DisplayName("should not duplicate plugin in build/plugins when already locally declared") + void shouldNotDuplicatePluginInBuildPluginsWhenAlreadyDeclared() throws Exception { + // When the child POM already declares the plugin in build/plugins, + // the strategy should NOT add a duplicate entry. + String pomXml = """ + + + 4.0.0 + + org.apache + apache + 23 + + org.example + test-child + 1.0.0-SNAPSHOT + + + + org.apache.maven.plugins + maven-enforcer-plugin + 3.0.0 + + + + + """; + + Document document = Document.of(pomXml); + Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath(); + Map pomMap = Map.of(pomPath, document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Strategy should succeed"); + + // Count enforcer-plugin entries in build/plugins - should be exactly 1 + Editor editor = new Editor(document); + Element root = editor.root(); + Element buildPluginsElement = root.childElement("build") + .flatMap(b -> b.childElement("plugins")) + .orElse(null); + assertNotNull(buildPluginsElement, "Should have build/plugins section"); + + long enforcerCount = buildPluginsElement + .childElements("plugin") + .filter(p -> p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse("") + .equals("maven-enforcer-plugin")) + .count(); + assertEquals(1, enforcerCount, "Should have exactly one maven-enforcer-plugin in build/plugins"); + + // Verify the version was upgraded (not the original 3.0.0) + String version = buildPluginsElement + .childElements("plugin") + .filter(p -> p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse("") + .equals("maven-enforcer-plugin")) + .findFirst() + .flatMap(p -> p.childElement("version")) + .map(Element::textContentTrimmed) + .orElse(null); + assertEquals("3.5.0", version, "Existing enforcer-plugin version should be upgraded to 3.5.0"); + } } @Nested