From 4cc204011404cbae9768489a6a97e5a303f6d746 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 2 Jun 2026 09:26:05 +0200 Subject: [PATCH] Fix mvnup effective model analysis for CI-friendly parent versions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mvnup's PluginUpgradeStrategy copied POMs to a temp directory for effective model analysis. That temp directory lacked .mvn, so root detection failed for child modules with ${revision} parent versions, producing "Parent POM is located above the root directory" errors. Eliminate the temp directory entirely — build effective models from the original file paths, which already have proper .mvn and project structure for root detection. Also fix DefaultModelBuilder.doReadFileModel() to: - Enter the parent resolution block when parent version contains expressions (${revision}, etc.) - Only enforce isParentWithinRootDirectory when rootDirectory came from the session, not the fallback heuristic - Accept parent version match when version contains an expression Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/PluginUpgradeStrategy.java | 129 +++++++----------- .../goals/PluginUpgradeStrategyTest.java | 41 ++++-- .../maven/impl/model/DefaultModelBuilder.java | 13 +- .../impl/model/DefaultModelBuilderTest.java | 12 ++ .../poms/factory/ci-friendly-version.xml | 31 +++++ ...nITgh12184CIFriendlyParentVersionTest.java | 60 ++++++++ .../child/pom.xml | 30 ++++ .../pom.xml | 35 +++++ 8 files changed, 254 insertions(+), 97 deletions(-) create mode 100644 impl/maven-impl/src/test/resources/poms/factory/ci-friendly-version.xml create mode 100644 its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12184CIFriendlyParentVersionTest.java create mode 100644 its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/child/pom.xml create mode 100644 its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/pom.xml 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..5e3fa64948c8 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 @@ -126,61 +126,48 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) Set modifiedPoms = new HashSet<>(); Set 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> pluginsNeedingManagement = - analyzePluginsUsingEffectiveModels(context, pomMap, tempDir); - - // Phase 3: Add plugin management to the last local parent in hierarchy - for (Map.Entry 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 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> 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 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 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); @@ -465,26 +452,19 @@ public static List getPluginUpgrades() { * Returns a map of POM path to the set of plugin keys that need management. */ private Map> analyzePluginsUsingEffectiveModels( - UpgradeContext context, Map pomMap, Path tempDir) { + UpgradeContext context, Map pomMap) { Map> result = new HashMap<>(); Map pluginUpgrades = getPluginUpgradesAsMap(); for (Map.Entry 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 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<>()) @@ -497,7 +477,7 @@ private Map> 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()); } } @@ -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 pomMap, Path tempDir, Path commonRoot) { - - Model effectiveModel = buildEffectiveModel(tempPomPath); + UpgradeContext context, Path pomPath, Map 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; } 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 d1961700e76b..6b833ab847c1 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 @@ -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; @@ -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 = """ @@ -793,20 +793,35 @@ void shouldDetectInheritedPluginsFromRemoteParent() throws Exception { """; - Document document = Document.of(pomXml); - Path pomPath = Paths.get("/project/pom.xml").toAbsolutePath(); - Map 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 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("maven-enforcer-plugin"), - "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("maven-enforcer-plugin"), + "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) { + } + }); + } + } } } diff --git a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java index aa282d272353..f3c6b0e8589d 100644 --- a/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java +++ b/impl/maven-impl/src/main/java/org/apache/maven/impl/model/DefaultModelBuilder.java @@ -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)) { @@ -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 : ".."); @@ -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, @@ -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) diff --git a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java index d361faad123a..6dfd2c2219f3 100644 --- a/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java +++ b/impl/maven-impl/src/test/java/org/apache/maven/impl/model/DefaultModelBuilderTest.java @@ -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 diff --git a/impl/maven-impl/src/test/resources/poms/factory/ci-friendly-version.xml b/impl/maven-impl/src/test/resources/poms/factory/ci-friendly-version.xml new file mode 100644 index 000000000000..c33bbe3af0a2 --- /dev/null +++ b/impl/maven-impl/src/test/resources/poms/factory/ci-friendly-version.xml @@ -0,0 +1,31 @@ + + + + 4.0.0 + + org.apache.maven.test + ci-friendly-version-test + ${revision} + pom + + + 1.0.0-SNAPSHOT + + diff --git a/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12184CIFriendlyParentVersionTest.java b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12184CIFriendlyParentVersionTest.java new file mode 100644 index 000000000000..2ba11172c0a9 --- /dev/null +++ b/its/core-it-suite/src/test/java/org/apache/maven/it/MavenITgh12184CIFriendlyParentVersionTest.java @@ -0,0 +1,60 @@ +/* + * 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. + */ +package org.apache.maven.it; + +import java.io.File; + +import org.junit.jupiter.api.Test; + +/** + * Integration test for #12184. + * Verifies that CI-friendly {@code ${revision}} in parent version works in Maven 4 + * native mode (without maven3Personality) for model version 4.0.0 projects. + */ +class MavenITgh12184CIFriendlyParentVersionTest extends AbstractMavenIntegrationTestCase { + + /** + * Verify that a multi-module project with {@code ${revision}} in parent version + * builds successfully when the property is defined in the parent POM properties. + */ + @Test + void testCiFriendlyParentVersionFromProperties() throws Exception { + File testDir = extractResources("/gh-12184-ci-friendly-parent-version"); + + Verifier verifier = newVerifier(testDir.getAbsolutePath()); + verifier.addCliArgument("validate"); + verifier.execute(); + verifier.verifyErrorFreeLog(); + } + + /** + * Verify that a multi-module project with {@code ${revision}} in parent version + * builds successfully when the property is overridden via command line. + */ + @Test + void testCiFriendlyParentVersionFromCli() throws Exception { + File testDir = extractResources("/gh-12184-ci-friendly-parent-version"); + + Verifier verifier = newVerifier(testDir.getAbsolutePath()); + verifier.addCliArgument("-Drevision=2.0.0"); + verifier.addCliArgument("validate"); + verifier.execute(); + verifier.verifyErrorFreeLog(); + } +} diff --git a/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/child/pom.xml b/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/child/pom.xml new file mode 100644 index 000000000000..c826300db9e6 --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/child/pom.xml @@ -0,0 +1,30 @@ + + + + 4.0.0 + + + org.apache.maven.its.gh12184 + ci-friendly-parent + ${revision} + + + ci-friendly-child + diff --git a/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/pom.xml b/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/pom.xml new file mode 100644 index 000000000000..07ff1a29d9dc --- /dev/null +++ b/its/core-it-suite/src/test/resources/gh-12184-ci-friendly-parent-version/pom.xml @@ -0,0 +1,35 @@ + + + + 4.0.0 + + org.apache.maven.its.gh12184 + ci-friendly-parent + ${revision} + pom + + + child + + + + 1.0.0-SNAPSHOT + +