From 63121bc4911317e292cd63ccb361ce661b2d6b47 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 27 May 2026 00:53:26 +0200 Subject: [PATCH] Use effective model to resolve properties from remote parent POMs in mvnup The undefined property detection in CompatibilityFixStrategy only performed static analysis of reactor POMs, missing properties inherited from remote parent POMs. This caused valid dependencyManagement entries to be incorrectly commented out, breaking child modules relying on them for version resolution. Now uses buildEffectiveModel to collect properties from the full parent chain, including remote parents resolved via relativePath or Maven repositories. Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/CompatibilityFixStrategy.java | 14 ++ .../goals/CompatibilityFixStrategyTest.java | 126 ++++++++++++++++++ 2 files changed, 140 insertions(+) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java index 8739d1766793..4fad1a81db7a 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java @@ -125,6 +125,7 @@ public UpgradeResult doApply(UpgradeContext context, Map pomMap) Set errorPoms = new HashSet<>(); Set allDefinedProperties = collectAllDefinedProperties(pomMap); + allDefinedProperties.addAll(collectEffectiveProperties(context, pomMap)); for (Map.Entry entry : pomMap.entrySet()) { Path pomPath = entry.getKey(); @@ -375,6 +376,19 @@ private void collectPropertiesFromDom(Document document, Set properties) propsElement.childElements().forEach(child -> properties.add(child.name()))))); } + private Set collectEffectiveProperties(UpgradeContext context, Map pomMap) { + Set properties = new HashSet<>(); + for (Path pomPath : pomMap.keySet()) { + try { + org.apache.maven.api.model.Model effectiveModel = buildEffectiveModel(pomPath); + properties.addAll(effectiveModel.getProperties().keySet()); + } catch (Exception e) { + context.debug("Failed to build effective model for " + pomPath + ": " + e.getMessage()); + } + } + return properties; + } + /** * Fixes dependencies with undefined property expressions by commenting them out. */ diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java index 00dacbb1c11b..7795c25d9d76 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategyTest.java @@ -919,6 +919,132 @@ void shouldRecognizePropertyFromGrandparent() throws Exception { deps.childElements("dependency").count(), "Dependency should not be commented out when property is inherited from grandparent"); } + + @Test + @DisplayName("should not comment out when property is defined in external parent not in reactor") + void shouldNotCommentOutWhenPropertyFromExternalParentNotInReactor() throws Exception { + String externalParentPom = """ + + + 4.0.0 + com.example + external-parent + 1.0.0 + pom + + 1.62.0 + + + """; + + String childPom = """ + + + 4.0.0 + + com.example + external-parent + 1.0.0 + ../external/pom.xml + + child + + + + org.apache.jackrabbit + oak-core + ${oak.version} + + + + + """; + + Path externalDir = Files.createDirectories(tempDir.resolve("external")); + Path externalParentPath = externalDir.resolve("pom.xml"); + Path childDir = Files.createDirectories(tempDir.resolve("child")); + Path childPomPath = childDir.resolve("pom.xml"); + + Files.writeString(externalParentPath, externalParentPom); + Files.writeString(childPomPath, childPom); + + Document childDoc = Document.of(childPom); + Map pomMap = Map.of(childPomPath, childDoc); + + UpgradeContext context = createMockContext(childDir); + strategy.doApply(context, pomMap); + + Element root = childDoc.root(); + Element depMgmt = DomUtils.findChildElement(root, "dependencyManagement"); + Element deps = DomUtils.findChildElement(depMgmt, "dependencies"); + assertEquals( + 1, + deps.childElements("dependency").count(), + "Managed dependency should not be commented out when property is inherited from external parent"); + } + + @Test + @DisplayName("should comment out when property is truly undefined even in effective model") + void shouldCommentOutWhenPropertyTrulyUndefinedInEffectiveModel() throws Exception { + String parentPom = """ + + + 4.0.0 + com.example + parent + 1.0.0 + pom + + """; + + String childPom = """ + + + 4.0.0 + + com.example + parent + 1.0.0 + ../pom.xml + + child + + + + com.google.guava + guava + ${truly.undefined.prop} + + + + + """; + + Path parentPath = tempDir.resolve("pom.xml"); + Path childDir = Files.createDirectories(tempDir.resolve("child")); + Path childPomPath = childDir.resolve("pom.xml"); + + Files.writeString(parentPath, parentPom); + Files.writeString(childPomPath, childPom); + + Document childDoc = Document.of(childPom); + Map pomMap = Map.of(childPomPath, childDoc); + + UpgradeContext context = createMockContext(childDir); + strategy.doApply(context, pomMap); + + String xml = DomUtils.toXml(childDoc); + assertTrue(xml.contains("mvnup: commented out"), "Should contain comment-out marker"); + assertTrue(xml.contains("truly.undefined.prop"), "Should mention the undefined property"); + } } @Nested