Skip to content

Fix mvnup effective model analysis for CI-friendly parent versions #12205

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 1 commit into
base: master
Choose a base branch
from
+254 −97
Open

Fix mvnup effective model analysis for CI-friendly parent versions #12205

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
129 changes: 49 additions & 80 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 @@ -126,61 +126,48 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> pomMap)
Set<Path> modifiedPoms = new HashSet<>();
Set<Path> errorPoms = new HashSet<>();

try {
// Phase 1: Write all modifications to temp directory (keeping project structure)
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);

// Phase 3: Add plugin management to the last local parent in hierarchy
for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
Path pomPath = entry.getKey();
Document pomDocument = entry.getValue();
processedPoms.add(pomPath);

context.info(pomPath + " (checking for plugin upgrades)");
context.indent();

try {
boolean hasUpgrades = false;

// Apply direct plugin upgrades in the document
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);
context.detail("Added plugin management to " + pomPath + " (target parent for "
+ pluginsForThisPom.size() + " plugins)");
}
// Phase 1: For each POM, build effective model from original paths and analyze plugins
Map<Path, Set<String>> pluginsNeedingManagement =
analyzePluginsUsingEffectiveModels(context, pomMap);

if (hasUpgrades) {
modifiedPoms.add(pomPath);
context.success("Plugin upgrades applied");
} else {
context.success("No plugin upgrades needed");
}
} catch (Exception e) {
context.failure("Failed to upgrade plugins: " + e.getMessage());
errorPoms.add(pomPath);
} finally {
context.unindent();
}
}
// Phase 2: Add plugin management to the last local parent in hierarchy
for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
Path pomPath = entry.getKey();
Document pomDocument = entry.getValue();
processedPoms.add(pomPath);

context.info(pomPath + " (checking for plugin upgrades)");
context.indent();

// Clean up temp directory
cleanupTempDirectory(tempDir);
try {
boolean hasUpgrades = false;

// Apply direct plugin upgrades in the document
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);
context.detail("Added plugin management to " + pomPath + " (target parent for "
+ pluginsForThisPom.size() + " plugins)");
}

} catch (Exception e) {
context.failure("Failed to create temp project structure: " + e.getMessage());
// Mark all POMs as errors
errorPoms.addAll(pomMap.keySet());
if (hasUpgrades) {
modifiedPoms.add(pomPath);
context.success("Plugin upgrades applied");
} else {
context.success("No plugin upgrades needed");
}
} catch (Exception e) {
context.failure("Failed to upgrade plugins: " + e.getMessage());
errorPoms.add(pomPath);
} finally {
context.unindent();
}
}

return new UpgradeResult(processedPoms, modifiedPoms, errorPoms);
Expand Down Expand Up @@ -465,26 +452,19 @@ public static List<PluginUpgrade> getPluginUpgrades() {
* Returns a map of POM path to the set of plugin keys that need management.
*/
private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels(
UpgradeContext context, Map<Path, Document> pomMap, Path tempDir) {
UpgradeContext context, Map<Path, Document> pomMap) {
Map<Path, Set<String>> result = new HashMap<>();
Map<String, PluginUpgrade> pluginUpgrades = getPluginUpgradesAsMap();

for (Map.Entry<Path, Document> entry : pomMap.entrySet()) {
Path originalPomPath = entry.getKey();
Path pomPath = entry.getKey();

try {
// Find the corresponding temp POM path
Path commonRoot = findCommonRoot(pomMap.keySet());
Path relativePath = commonRoot.relativize(originalPomPath);
Path tempPomPath = tempDir.resolve(relativePath);

// Build effective model using Maven 4 API
Set<String> pluginsNeedingUpgrade =
analyzeEffectiveModelForPlugins(context, tempPomPath, pluginUpgrades);
analyzeEffectiveModelForPlugins(context, pomPath, pluginUpgrades);

// Determine where to add plugin management (last local parent)
Path targetPomForManagement =
findLastLocalParentForPluginManagement(context, tempPomPath, pomMap, tempDir, commonRoot);
findLastLocalParentForPluginManagement(context, pomPath, pomMap);

if (targetPomForManagement != null) {
result.computeIfAbsent(targetPomForManagement, k -> new HashSet<>())
Expand All @@ -497,7 +477,7 @@ private Map<Path, Set<String>> analyzePluginsUsingEffectiveModels(
}

} catch (Exception e) {
context.warning("Failed to analyze effective model for " + originalPomPath + ": " + e.getMessage());
context.warning("Failed to analyze effective model for " + pomPath + ": " + e.getMessage());
}
}

Expand Down Expand Up @@ -584,37 +564,26 @@ private String getPluginKey(Plugin plugin) {
* if so continue to its parent, else that's the target.
*/
private Path findLastLocalParentForPluginManagement(
UpgradeContext context, Path tempPomPath, Map<Path, Document> pomMap, Path tempDir, Path commonRoot) {

Model effectiveModel = buildEffectiveModel(tempPomPath);
UpgradeContext context, Path pomPath, Map<Path, Document> pomMap) {

// Convert the temp path back to the original path
Path relativePath = tempDir.relativize(tempPomPath);
Path currentOriginalPath = commonRoot.resolve(relativePath);
Model effectiveModel = buildEffectiveModel(pomPath);

// Start with current POM as the candidate
Path lastLocalParent = currentOriginalPath;
Path lastLocalParent = pomPath;

// Walk up the parent hierarchy
Model currentModel = effectiveModel;
while (currentModel.getParent() != null) {
Parent parent = currentModel.getParent();

// Check if this parent is in our local pomMap
Path parentPath = findParentInPomMap(parent, pomMap);
if (parentPath != null) {
// Parent is local, so it becomes our new candidate
lastLocalParent = parentPath;

Path parentTempPath = tempDir.resolve(commonRoot.relativize(parentPath));
currentModel = buildEffectiveModel(parentTempPath);
currentModel = buildEffectiveModel(parentPath);
} else {
// Parent is external, stop here
break;
}
}

context.debug("Last local parent for " + currentOriginalPath + " is " + lastLocalParent);
context.debug("Last local parent for " + pomPath + " is " + lastLocalParent);
return lastLocalParent;
}

Expand Down
41 changes: 28 additions & 13 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 @@ -18,6 +18,8 @@
*/
package org.apache.maven.cling.invoker.mvnup.goals;

import java.io.IOException;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.List;
Expand Down Expand Up @@ -776,8 +778,6 @@ 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.
// Uses an absolute path because the effective model analysis path resolution
// requires it to match between phases.
String pomXml = """
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns="http://maven.apache.org/POM/4.0.0">
Expand All @@ -793,20 +793,35 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception {
</project>
""";

Document document = Document.of(pomXml);
Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath();
Map<Path, Document> pomMap = Map.of(pomPath, document);
Path tempDir = Files.createTempDirectory("mvnup-test-");
try {
Files.createDirectories(tempDir.resolve(".mvn"));
Path pomPath = tempDir.resolve("pom.xml");
Files.writeString(pomPath, pomXml);

UpgradeContext context = createMockContext();
UpgradeResult result = strategy.doApply(context, pomMap);
Document document = Document.of(pomXml);
Map<Path, Document> pomMap = Map.of(pomPath, document);

assertTrue(result.success(), "Strategy should succeed");
assertTrue(result.modifiedCount() > 0, "Should have added plugin management for inherited plugins");
UpgradeContext context = createMockContext();
UpgradeResult result = strategy.doApply(context, pomMap);

String xml = DomUtils.toXml(document);
assertTrue(
xml.contains("<artifactId>maven-enforcer-plugin</artifactId>"),
"Should add pluginManagement for maven-enforcer-plugin inherited from parent");
assertTrue(result.success(), "Strategy should succeed");
assertTrue(result.modifiedCount() > 0, "Should have added plugin management for inherited plugins");

String xml = DomUtils.toXml(document);
assertTrue(
xml.contains("<artifactId>maven-enforcer-plugin</artifactId>"),
"Should add pluginManagement for maven-enforcer-plugin inherited from parent");
} finally {
try (var walk = Files.walk(tempDir)) {
walk.sorted(java.util.Comparator.reverseOrder()).forEach(p -> {
try {
Files.delete(p);
} catch (IOException ignored) {
}
});
}
}
}
}

Expand Down
13 changes: 9 additions & 4 deletions impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1500,12 +1500,14 @@ Model doReadFileModel() throws ModelBuilderException {
ModelSource modelSource = request.getSource();
Model model;
Path rootDirectory;
boolean rootDirectoryFromSession = false;
setSource(modelSource.getLocation());
logger.debug("Reading file model from " + modelSource.getLocation());
try {
boolean strict = isBuildRequest();
try {
rootDirectory = request.getSession().getRootDirectory();
rootDirectoryFromSession = true;
} catch (IllegalStateException ignore) {
rootDirectory = modelSource.getPath();
while (rootDirectory != null && !Files.isDirectory(rootDirectory)) {
Expand Down Expand Up @@ -1584,7 +1586,8 @@ Model doReadFileModel() throws ModelBuilderException {
String artifactId = parent.getArtifactId();
String version = parent.getVersion();
String path = parent.getRelativePath();
if ((groupId == null || artifactId == null || version == null)
boolean versionContainsExpression = version != null && version.contains("${");
if ((groupId == null || artifactId == null || version == null || versionContainsExpression)
&& (path == null || !path.isEmpty())) {
Path pomFile = model.getPomFile();
Path relativePath = Paths.get(path != null ? path : "..");
Expand All @@ -1593,8 +1596,8 @@ Model doReadFileModel() throws ModelBuilderException {
pomPath = modelProcessor.locateExistingPom(pomPath);
}
if (pomPath != null && Files.isRegularFile(pomPath)) {
// Check if parent POM is above the root directory
if (!isParentWithinRootDirectory(pomPath, rootDirectory)) {
if (rootDirectoryFromSession
&& !isParentWithinRootDirectory(pomPath, rootDirectory)) {
add(
Severity.FATAL,
Version.BASE,
Expand All @@ -1612,7 +1615,9 @@ Model doReadFileModel() throws ModelBuilderException {
String parentVersion = getVersion(parentModel);
if ((groupId == null || groupId.equals(parentGroupId))
&& (artifactId == null || artifactId.equals(parentArtifactId))
&& (version == null || version.equals(parentVersion))) {
&& (version == null
|| version.equals(parentVersion)
|| versionContainsExpression)) {
model = model.withParent(parent.with()
.groupId(parentGroupId)
.artifactId(parentArtifactId)
Expand Down
12 changes: 12 additions & 0 deletions impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,18 @@ public void testDirectoryPropertiesInProfilesAndRepositories() {
expectedUrl, result.getEffectiveModel().getRepositories().get(0).getUrl());
}

@Test
public void testCiFriendlyVersion() {
ModelBuilderRequest request = ModelBuilderRequest.builder()
.session(session)
.requestType(ModelBuilderRequest.RequestType.BUILD_PROJECT)
.source(Sources.buildSource(getPom("ci-friendly-version")))
.build();
ModelBuilderResult result = builder.newSession().build(request);
assertNotNull(result);
assertEquals("1.0.0-SNAPSHOT", result.getEffectiveModel().getVersion());
}

@Test
public void testMissingDependencyGroupIdInference() throws Exception {
// Test that dependencies with missing groupId but present version are inferred correctly in model 4.1.0
Expand Down
31 changes: 31 additions & 0 deletions impl/maven-impl/src/test/resources/poms/factory/ci-friendly-version.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
<?xml version="1.0" encoding="UTF-8"?>
<!---
Licensed to the Apache Software Foundation (ASF) under one or more
contributor license agreements. See the NOTICE file distributed with
this work for additional information regarding copyright ownership.
The ASF licenses this file to You under the Apache License, Version 2.0
(the "License"); you may not use this file except in compliance with
the License. You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
-->
<project xmlns="http://maven.apache.org/POM/4.0.0"
xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
<modelVersion>4.0.0</modelVersion>

<groupId>org.apache.maven.test</groupId>
<artifactId>ci-friendly-version-test</artifactId>
<version>${revision}</version>
<packaging>pom</packaging>

<properties>
<revision>1.0.0-SNAPSHOT</revision>
</properties>
</project>
Loading
Loading