Skip to content

[Backport 4.0.x] Fix mvnup spurious pluginManagement injection for remote parent plugins #12351

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
gnodet wants to merge 2 commits into
base: maven-4.0.x
Choose a base branch
from
+153 −24
Open

[Backport 4.0.x] Fix mvnup spurious pluginManagement injection for remote parent plugins #12351

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cd0e071
[#11606] Fix mvnup spurious pluginManagement injection for plugins in…
gnodet Jun 23, 2026
e2bdf3e
Override remote parent plugins with comment instead of skipping
gnodet Jun 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 66 additions & 13 deletions ...n-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -132,6 +133,9 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> 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<String> localPluginKeys = collectLocallyDeclaredPluginKeys(pomMap);
Comment on lines +136 to +137

// 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();
Expand All @@ -151,8 +155,8 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> pomMap)
Set<String> 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)");
}
Expand All @@ -162,7 +166,8 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> pomMap)
Set<String> pluginsForDirectOverride =
analysisResults.pluginsNeedingDirectOverride().get(pomPath);
if (pluginsForDirectOverride != null && !pluginsForDirectOverride.isEmpty()) {
hasUpgrades |= addDirectPluginOverrides(context, pomDocument, pluginsForDirectOverride);
hasUpgrades |= addDirectPluginOverrides(
context, pomDocument, pluginsForDirectOverride, localPluginKeys);
}

if (hasUpgrades) {
Expand Down Expand Up @@ -697,7 +702,7 @@ private Path findParentInPomMap(Parent parent, Map<Path, Document> pomMap) {
* Adds plugin management entries for plugins found through effective model analysis.
*/
private boolean addPluginManagementForEffectivePlugins(
UpgradeContext context, Document pomDocument, Set<String> pluginKeys) {
UpgradeContext context, Document pomDocument, Set<String> pluginKeys, Set<String> localPluginKeys) {

Map<String, PluginUpgrade> pluginUpgrades = getPluginUpgradesAsMap();
boolean hasUpgrades = false;
Expand Down Expand Up @@ -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;
}
}
Expand Down Expand Up @@ -762,20 +768,27 @@ 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)");
}
}

/**
* 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) {
private boolean addDirectPluginOverrides(
UpgradeContext context, Document pomDocument, Set<String> pluginKeys, Set<String> localPluginKeys) {
Map<String, PluginUpgrade> pluginUpgrades = getPluginUpgradesAsMap();
boolean hasUpgrades = false;

Expand All @@ -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()
Expand All @@ -808,6 +825,42 @@ private boolean addDirectPluginOverrides(UpgradeContext context, Document pomDoc
return hasUpgrades;
}

private Set<String> collectLocallyDeclaredPluginKeys(Map<Path, Document> pomMap) {
Set<String> 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<String> 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<String> needsManagement, Set<String> needsDirectOverride) {}

private record PluginAnalysisResults(
Expand Down
98 changes: 87 additions & 11 deletions ...i/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/PluginUpgradeStrategyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment on lines +776 to +777
// 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
Expand Down Expand Up @@ -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(
Comment on lines 809 to +813
xml.contains("<pluginManagement>"),
"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("<artifactId>maven-enforcer-plugin</artifactId>"),
"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 -> {
Expand All @@ -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 {
Comment on lines +835 to +836
// 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 = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
Expand Down Expand Up @@ -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");
Comment on lines +878 to +881

// Verify NO direct build/plugins entry for enforcer (PM override is sufficient)
Element buildPlugins = root.childElement("build")
.flatMap(b -> b.childElement("plugins"))
Expand Down Expand Up @@ -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 = """
<?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>
</plugin>
</plugins>
</build>
</project>
""";

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<Path, Document> 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("<pluginManagement>"), "Should add pluginManagement for locally declared plugin");
assertTrue(
xml.contains("<artifactId>maven-enforcer-plugin</artifactId>"),
"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
Expand Down
Loading