This is an automated email from the ASF dual-hosted git repository. gnodet pushed a commit to branch cake-dew in repository https://gitbox.apache.org/repos/asf/maven.git
commit 7bcb282c33bba1f233581c49f4d85fd7ee7753ee Author: Guillaume Nodet <[email protected]> AuthorDate: Wed May 27 09:09:08 2026 +0200 Fix mvnup plugin upgrade for versions locked by parent's build/plugins When a remote parent POM declares a plugin with an explicit version in build/plugins (not via pluginManagement), adding a pluginManagement entry in the child is insufficient — the parent's explicit version takes precedence. The analysis now compares each plugin's effective version in build/plugins against build/pluginManagement/plugins: - Same version: version comes from PM, pluginManagement override works - Different version (or absent from PM): explicit version in parent's plugins, needs a direct build/plugins entry to override Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../invoker/mvnup/goals/PluginUpgradeStrategy.java | 152 ++++++++++++++++----- .../mvnup/goals/PluginUpgradeStrategyTest.java | 128 +++++++++++++++++ 2 files changed, 245 insertions(+), 35 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 6a6d16cb15..28353a8b1e 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<Path, Document> pomMap) Path tempDir = createTempProjectStructure(context, pomMap); // Phase 2: For each POM, build effective model using the session and analyze plugins - Map<Path, Set<String>> 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<Path, Document> entry : pomMap.entrySet()) { Path pomPath = entry.getKey(); Document pomDocument = entry.getValue(); @@ -150,14 +149,21 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> 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<String> pluginsForThisPom = pluginsNeedingManagement.get(pomPath); - if (pluginsForThisPom != null && !pluginsForThisPom.isEmpty()) { - hasUpgrades |= addPluginManagementForEffectivePlugins(context, pomDocument, pluginsForThisPom); + Set<String> 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<String> pluginsForDirectOverride = + analysisResults.pluginsNeedingDirectOverride().get(pomPath); + if (pluginsForDirectOverride != null && !pluginsForDirectOverride.isEmpty()) { + hasUpgrades |= addDirectPluginOverrides(context, pomDocument, pluginsForDirectOverride); } if (hasUpgrades) { @@ -462,11 +468,13 @@ public static List<PluginUpgrade> 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 analysis results with two maps: plugins needing pluginManagement entries + * and plugins needing direct build/plugins overrides. */ - private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels( + private PluginAnalysisResults analyzePluginsUsingEffectiveModels( UpgradeContext context, Map<Path, Document> pomMap, Path tempDir) { - Map<Path, Set<String>> result = new HashMap<>(); + Map<Path, Set<String>> managementResult = new HashMap<>(); + Map<Path, Set<String>> directOverrideResult = new HashMap<>(); Map<String, PluginUpgrade> pluginUpgrades = getPluginUpgradesAsMap(); for (Map.Entry<Path, Document> entry : pomMap.entrySet()) { @@ -479,20 +487,27 @@ private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels( Path tempPomPath = tempDir.resolve(relativePath); // Build effective model using Maven 4 API - Set<String> pluginsNeedingUpgrade = - analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); + PluginAnalysis analysis = analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades); // Determine where to add plugin management (last local parent) - Path targetPomForManagement = + Path targetPom = findLastLocalParentForPluginManagement(context, tempPomPath, pomMap, tempDir, commonRoot); - if (targetPomForManagement != null) { - result.computeIfAbsent(targetPomForManagement, k -> new HashSet<>()) - .addAll(pluginsNeedingUpgrade); - - if (!pluginsNeedingUpgrade.isEmpty()) { - context.debug("Will add plugin management to " + targetPomForManagement + " for plugins: " - + pluginsNeedingUpgrade); + if (targetPom != null) { + managementResult + .computeIfAbsent(targetPom, k -> new HashSet<>()) + .addAll(analysis.needsManagement()); + directOverrideResult + .computeIfAbsent(targetPom, k -> new HashSet<>()) + .addAll(analysis.needsDirectOverride()); + + if (!analysis.needsManagement().isEmpty()) { + context.debug("Will add plugin management to " + targetPom + " for plugins: " + + analysis.needsManagement()); + } + if (!analysis.needsDirectOverride().isEmpty()) { + context.debug("Will add direct plugin overrides to " + targetPom + " for plugins: " + + analysis.needsDirectOverride()); } } @@ -501,7 +516,7 @@ private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels( } } - return result; + return new PluginAnalysisResults(managementResult, directOverrideResult); } /** @@ -513,7 +528,7 @@ private Map<String, PluginUpgrade> getPluginUpgradesAsMap() { upgrade -> upgrade.groupId() + ":" + upgrade.artifactId(), upgrade -> upgrade)); } - private Set<String> analyzeEffectiveModelForPlugins( + private PluginAnalysis analyzeEffectiveModelForPlugins( UpgradeContext context, Path tempPomPath, Map<String, PluginUpgrade> pluginUpgrades) { Model effectiveModel = buildEffectiveModel(tempPomPath); return analyzePluginsFromEffectiveModel(context, effectiveModel, pluginUpgrades); @@ -521,13 +536,27 @@ private Set<String> analyzeEffectiveModelForPlugins( /** * Analyzes plugins from the effective model and determines which ones need upgrades. + * Separates plugins into those overridable via pluginManagement and those requiring + * a direct build/plugins entry (because the version is set explicitly in an inherited + * parent's build/plugins, not via pluginManagement). */ - private Set<String> analyzePluginsFromEffectiveModel( + private PluginAnalysis analyzePluginsFromEffectiveModel( UpgradeContext context, Model effectiveModel, Map<String, PluginUpgrade> pluginUpgrades) { - Set<String> pluginsNeedingUpgrade = new HashSet<>(); + Set<String> needsManagement = new HashSet<>(); + Set<String> needsDirectOverride = new HashSet<>(); Build build = effectiveModel.getBuild(); if (build != null) { + // Collect managed plugin versions for comparison + Map<String, String> managedVersions = new HashMap<>(); + PluginManagement pluginManagement = build.getPluginManagement(); + if (pluginManagement != null) { + for (Plugin plugin : pluginManagement.getPlugins()) { + String pluginKey = getPluginKey(plugin); + managedVersions.put(pluginKey, plugin.getVersion()); + } + } + // Check build/plugins - these are the actual plugins used in the build for (Plugin plugin : build.getPlugins()) { String pluginKey = getPluginKey(plugin); @@ -535,23 +564,33 @@ private Set<String> analyzePluginsFromEffectiveModel( if (upgrade != null) { String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { - pluginsNeedingUpgrade.add(pluginKey); - context.debug("Plugin " + pluginKey + " version " + effectiveVersion + " needs upgrade to " - + upgrade.minVersion()); + needsManagement.add(pluginKey); + String managedVersion = managedVersions.get(pluginKey); + if (managedVersion == null || !managedVersion.equals(effectiveVersion)) { + // Version differs from pluginManagement (or not in PM at all): + // the parent sets an explicit version in build/plugins that + // pluginManagement alone cannot override + needsDirectOverride.add(pluginKey); + context.debug("Plugin " + pluginKey + " version " + effectiveVersion + + " has explicit version in inherited build/plugins" + + " — needs direct override to " + upgrade.minVersion()); + } else { + context.debug("Plugin " + pluginKey + " version " + effectiveVersion + + " is managed via pluginManagement — needs upgrade to " + upgrade.minVersion()); + } } } } - // Check build/pluginManagement/plugins - these provide version management - PluginManagement pluginManagement = build.getPluginManagement(); + // Check build/pluginManagement/plugins for managed-only plugins if (pluginManagement != null) { for (Plugin plugin : pluginManagement.getPlugins()) { String pluginKey = getPluginKey(plugin); PluginUpgrade upgrade = pluginUpgrades.get(pluginKey); - if (upgrade != null) { + if (upgrade != null && !needsManagement.contains(pluginKey)) { String effectiveVersion = plugin.getVersion(); if (isVersionBelow(effectiveVersion, upgrade.minVersion())) { - pluginsNeedingUpgrade.add(pluginKey); + needsManagement.add(pluginKey); context.debug("Managed plugin " + pluginKey + " version " + effectiveVersion + " needs upgrade to " + upgrade.minVersion()); } @@ -560,7 +599,7 @@ private Set<String> analyzePluginsFromEffectiveModel( } } - return pluginsNeedingUpgrade; + return new PluginAnalysis(needsManagement, needsDirectOverride); } /** @@ -732,6 +771,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 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<String> pluginKeys) { + Map<String, PluginUpgrade> 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 " + upgrade.groupId() + ":" + upgrade.artifactId() + " version " + + upgrade.minVersion() + + " in build/plugins (overrides version locked by parent)"); + } + } + } + + return hasUpgrades; + } + + private record PluginAnalysis(Set<String> needsManagement, Set<String> needsDirectOverride) {} + + private record PluginAnalysisResults( + Map<Path, Set<String>> pluginsNeedingManagement, Map<Path, Set<String>> 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 d1961700e7..c10f00864b 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,134 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { xml.contains("<artifactId>maven-enforcer-plugin</artifactId>"), "Should add pluginManagement for maven-enforcer-plugin inherited from parent"); } + + @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. + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache</groupId> + <artifactId>apache</artifactId> + <version>23</version> + </parent> + <groupId>org.example</groupId> + <artifactId>test-child</artifactId> + <version>1.0.0-SNAPSHOT</version> + </project> + """; + + Document document = Document.of(pomXml); + Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath(); + Map<Path, Document> pomMap = Map.of(pomPath, document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + 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"); + } + } + + @Test + @DisplayName("should not duplicate plugin in build/plugins when already locally declared") + void shouldNotDuplicatePluginInBuildPluginsWhenAlreadyDeclared() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <parent> + <groupId>org.apache</groupId> + <artifactId>apache</artifactId> + <version>23</version> + </parent> + <groupId>org.example</groupId> + <artifactId>test-child</artifactId> + <version>1.0.0-SNAPSHOT</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-enforcer-plugin</artifactId> + <version>3.0.0</version> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath(); + Map<Path, Document> pomMap = Map.of(pomPath, document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Strategy should succeed"); + + Editor editor = new Editor(document); + Element root = editor.root(); + Element buildPlugins = root.childElement("build") + .flatMap(b -> b.childElement("plugins")) + .orElse(null); + assertNotNull(buildPlugins, "Should have build/plugins section"); + + long enforcerCount = buildPlugins + .childElements("plugin") + .filter(p -> "maven-enforcer-plugin" + .equals(p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse(""))) + .count(); + assertEquals(1, enforcerCount, "Should have exactly one maven-enforcer-plugin in build/plugins"); + + String version = buildPlugins + .childElements("plugin") + .filter(p -> "maven-enforcer-plugin" + .equals(p.childElement("artifactId") + .map(Element::textContentTrimmed) + .orElse(""))) + .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
