Skip to content

[4.0.x] Fix mvnup plugin upgrade for plugins inherited in build/plugins #12159

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

Closed
gnodet wants to merge 1 commit into from
+219 −26
Closed

[4.0.x] Fix mvnup plugin upgrade for plugins inherited in build/plugins #12159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
118 changes: 92 additions & 26 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 @@ -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();
Expand All @@ -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) {
Expand Down Expand Up @@ -462,11 +468,15 @@ 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 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<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()) {
Expand All @@ -479,20 +489,28 @@ 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 =
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());
}
}

Expand All @@ -501,7 +519,7 @@ private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels(
}
}

return result;
return new PluginAnalysisResults(managementResult, directOverrideResult);
}

/**
Expand All @@ -513,18 +531,22 @@ 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);
}

/**
* 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<String> analyzePluginsFromEffectiveModel(
private PluginAnalysis analyzePluginsFromEffectiveModel(
UpgradeContext context, Model effectiveModel, Map<String, PluginUpgrade> pluginUpgrades) {
Set<String> pluginsNeedingUpgrade = new HashSet<>();
Set<String> allPluginsNeedingUpgrade = new HashSet<>();
Set<String> buildPluginsNeedingUpgrade = new HashSet<>();

Build build = effectiveModel.getBuild();
if (build != null) {
Expand All @@ -535,7 +557,8 @@ private Set<String> 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());
}
Expand All @@ -551,7 +574,7 @@ private Set<String> 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());
}
Expand All @@ -560,7 +583,7 @@ private Set<String> analyzePluginsFromEffectiveModel(
}
}

return pluginsNeedingUpgrade;
return new PluginAnalysis(allPluginsNeedingUpgrade, buildPluginsNeedingUpgrade);
}

/**
Expand Down Expand Up @@ -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<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 plugin " + upgrade.groupId() + ":" + upgrade.artifactId() + " version "
+ upgrade.minVersion()
+ " in build/plugins (overrides inherited version from remote parent)");
}
}
}

return hasUpgrades;
}

private record PluginAnalysis(Set<String> allPluginsNeedingUpgrade, Set<String> buildPluginsNeedingUpgrade) {}

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
Expand Down
127 changes: 127 additions & 0 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 @@ -808,6 +808,133 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception {
xml.contains("<artifactId>maven-enforcer-plugin</artifactId>"),
"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 = """
<?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");
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 = """
<?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");

// 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
Expand Down
Loading