This is an automated email from the ASF dual-hosted git repository. gnodet pushed a commit to branch maven-4.0.x-test-fixes in repository https://gitbox.apache.org/repos/asf/maven.git
commit 680cd1d14129a1d1d42f2f137da8513d80d9c7a4 Author: Guillaume Nodet <[email protected]> AuthorDate: Wed May 27 00:55:07 2026 +0200 Remove invalid combine.self and combine.children attributes instead of converting them Maven 3 silently ignored invalid combine.self and combine.children attribute values, so removing them entirely preserves actual Maven 3 behavior. Previously mvnup converted specific invalid values (e.g. combine.self="append" → "merge"), which could miss other invalid values and subtly changed semantics. Now both fixers detect any invalid value against the full set of valid values (combine.self: override/merge/remove; combine.children: append/merge), remove the attribute, and collect to a list before mutating to avoid potential stream processing issues. Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../mvnup/goals/CompatibilityFixStrategy.java | 66 ++-- .../mvnup/goals/CompatibilityFixStrategyTest.java | 410 +++++++++++++++++++++ 2 files changed, 452 insertions(+), 24 deletions(-) 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 c8c99d30ac..343caf147b 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 @@ -77,6 +77,10 @@ public class CompatibilityFixStrategy extends AbstractUpgradeStrategy { private static final Pattern EXPRESSION_PATTERN = Pattern.compile("\\$\\{([^}]+)}"); + private static final Set<String> VALID_COMBINE_SELF_VALUES = Set.of(COMBINE_OVERRIDE, COMBINE_MERGE, "remove"); + + private static final Set<String> VALID_COMBINE_CHILDREN_VALUES = Set.of(COMBINE_APPEND, COMBINE_MERGE); + @Override public boolean isApplicable(UpgradeContext context) { UpgradeOptions options = getOptions(context); @@ -165,44 +169,44 @@ public UpgradeResult doApply(UpgradeContext context, Map<Path, Document> pomMap) /** * Fixes unsupported combine.children attribute values. - * Maven 4 only supports 'append' and 'merge', not 'override'. + * Maven 4 only supports 'append' and 'merge' (default is merge). + * Invalid values are removed entirely since Maven 3 silently ignored them. */ private boolean fixUnsupportedCombineChildrenAttributes(Document pomDocument, UpgradeContext context) { - boolean fixed = false; Element root = pomDocument.root(); - // Find all elements with combine.children="override" and change to "merge" - long fixedCombineChildrenCount = findElementsWithAttribute(root, COMBINE_CHILDREN, COMBINE_OVERRIDE) - .peek(element -> { - element.attributeObject(COMBINE_CHILDREN).value(COMBINE_MERGE); - context.detail("Fixed: " + COMBINE_CHILDREN + "='" + COMBINE_OVERRIDE + "' → '" + COMBINE_MERGE - + "' in " + element.name()); - }) - .count(); - fixed |= fixedCombineChildrenCount > 0; + List<Element> invalidElements = findElementsWithInvalidAttribute( + root, COMBINE_CHILDREN, VALID_COMBINE_CHILDREN_VALUES) + .toList(); - return fixed; + for (Element element : invalidElements) { + String invalidValue = element.attribute(COMBINE_CHILDREN); + element.removeAttribute(COMBINE_CHILDREN); + context.detail( + "Fixed: removed invalid " + COMBINE_CHILDREN + "='" + invalidValue + "' from " + element.name()); + } + + return !invalidElements.isEmpty(); } /** * Fixes unsupported combine.self attribute values. - * Maven 4 only supports 'override', 'merge', and 'remove' (default is merge), not 'append'. + * Maven 4 only supports 'override', 'merge', and 'remove' (default is merge). + * Invalid values are removed entirely since Maven 3 silently ignored them. */ private boolean fixUnsupportedCombineSelfAttributes(Document pomDocument, UpgradeContext context) { - boolean fixed = false; Element root = pomDocument.root(); - // Find all elements with combine.self="append" and change to "merge" - long fixedCombineSelfCount = findElementsWithAttribute(root, COMBINE_SELF, COMBINE_APPEND) - .peek(element -> { - element.attributeObject(COMBINE_SELF).value(COMBINE_MERGE); - context.detail("Fixed: " + COMBINE_SELF + "='" + COMBINE_APPEND + "' → '" + COMBINE_MERGE + "' in " - + element.name()); - }) - .count(); - fixed |= fixedCombineSelfCount > 0; + List<Element> invalidElements = findElementsWithInvalidAttribute(root, COMBINE_SELF, VALID_COMBINE_SELF_VALUES) + .toList(); - return fixed; + for (Element element : invalidElements) { + String invalidValue = element.attribute(COMBINE_SELF); + element.removeAttribute(COMBINE_SELF); + context.detail("Fixed: removed invalid " + COMBINE_SELF + "='" + invalidValue + "' from " + element.name()); + } + + return !invalidElements.isEmpty(); } /** @@ -527,6 +531,20 @@ private Stream<Element> findElementsWithAttribute(Element element, String attrib .flatMap(child -> findElementsWithAttribute(child, attributeName, attributeValue))); } + /** + * Recursively finds all elements with an attribute whose value is not in the set of valid values. + */ + private Stream<Element> findElementsWithInvalidAttribute( + Element element, String attributeName, Set<String> validValues) { + return Stream.concat( + Stream.of(element).filter(e -> { + String attr = e.attribute(attributeName); + return attr != null && !validValues.contains(attr); + }), + element.childElements() + .flatMap(child -> findElementsWithInvalidAttribute(child, attributeName, validValues))); + } + /** * Helper methods extracted from BaseUpgradeGoal for compatibility fixes. */ 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 7795c25d9d..051a33fd34 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 @@ -140,6 +140,416 @@ void shouldHandleAllOptionsDisabled() { } } + @Nested + @DisplayName("Unsupported combine.children Attribute Fixes") + class UnsupportedCombineChildrenFixesTests { + + @Test + @DisplayName("should remove combine.children='override' attribute") + void shouldRemoveCombineChildrenOverride() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs combine.children="override"> + <arg>-Xlint:all</arg> + </compilerArgs> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.children attribute"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.children"), "combine.children attribute should be removed entirely"); + assertTrue(xml.contains("compilerArgs"), "Element should still exist"); + } + + @Test + @DisplayName("should remove any invalid combine.children value") + void shouldRemoveAnyCombineChildrenInvalidValue() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs combine.children="invalid_value"> + <arg>-Xlint:all</arg> + </compilerArgs> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.children attribute"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.children"), "combine.children attribute should be removed entirely"); + } + + @Test + @DisplayName("should not remove valid combine.children values") + void shouldNotRemoveValidCombineChildrenValues() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs combine.children="append"> + <arg>-Xlint:all</arg> + </compilerArgs> + <includes combine.children="merge"> + <include>**/*.java</include> + </includes> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertEquals(0, result.modifiedCount(), "Should not have modified any POMs"); + + String xml = DomUtils.toXml(document); + assertTrue(xml.contains("combine.children=\"append\""), "append should be preserved"); + assertTrue(xml.contains("combine.children=\"merge\""), "merge should be preserved"); + } + + @Test + @DisplayName("should remove invalid combine.children in profile plugin configuration") + void shouldRemoveInvalidCombineChildrenInProfile() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <profiles> + <profile> + <id>release</id> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs combine.children="override"> + <descriptorRef>src</descriptorRef> + </descriptorRefs> + </configuration> + </plugin> + </plugins> + </build> + </profile> + </profiles> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.children in profile"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.children"), "combine.children attribute should be removed from profile"); + } + } + + @Nested + @DisplayName("Unsupported combine.self Attribute Fixes") + class UnsupportedCombineSelfFixesTests { + + @Test + @DisplayName("should remove combine.self='append' attribute") + void shouldRemoveCombineSelfAppend() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs combine.self="append"> + <descriptorRef>jar-with-dependencies</descriptorRef> + </descriptorRefs> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.self attribute"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.self"), "combine.self attribute should be removed entirely"); + assertTrue(xml.contains("descriptorRefs"), "Element should still exist"); + } + + @Test + @DisplayName("should remove any invalid combine.self value") + void shouldRemoveAnyCombineSelfInvalidValue() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs combine.self="invalid_value"> + <arg>-Xlint:all</arg> + </compilerArgs> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.self attribute"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.self"), "combine.self attribute should be removed entirely"); + } + + @Test + @DisplayName("should not remove valid combine.self values") + void shouldNotRemoveValidCombineSelfValues() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <source combine.self="override">11</source> + <target combine.self="merge">11</target> + <compilerArgs combine.self="remove"/> + </configuration> + </plugin> + </plugins> + </build> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertEquals(0, result.modifiedCount(), "Should not have modified any POMs"); + + String xml = DomUtils.toXml(document); + assertTrue(xml.contains("combine.self=\"override\""), "override should be preserved"); + assertTrue(xml.contains("combine.self=\"merge\""), "merge should be preserved"); + assertTrue(xml.contains("combine.self=\"remove\""), "remove should be preserved"); + } + + @Test + @DisplayName("should remove invalid combine.self in profile plugin configuration") + void shouldRemoveInvalidCombineSelfInProfile() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <profiles> + <profile> + <id>release</id> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs combine.self="append"> + <descriptorRef>src</descriptorRef> + </descriptorRefs> + </configuration> + </plugin> + </plugins> + </build> + </profile> + </profiles> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.self in profile"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.self"), "combine.self attribute should be removed from profile"); + } + + @Test + @DisplayName("should remove all occurrences of invalid combine.self across the POM") + void shouldRemoveAllOccurrencesOfInvalidCombineSelf() throws Exception { + String pomXml = """ + <?xml version="1.0" encoding="UTF-8"?> + <project xmlns="http://maven.apache.org/POM/4.0.0";> + <modelVersion>4.0.0</modelVersion> + <groupId>test</groupId> + <artifactId>test</artifactId> + <version>1.0.0</version> + <build> + <pluginManagement> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-compiler-plugin</artifactId> + <configuration> + <compilerArgs combine.self="append"> + <arg>-Xlint:all</arg> + </compilerArgs> + </configuration> + </plugin> + </plugins> + </pluginManagement> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs combine.self="append"> + <descriptorRef>jar-with-dependencies</descriptorRef> + </descriptorRefs> + </configuration> + </plugin> + </plugins> + </build> + <profiles> + <profile> + <id>release</id> + <build> + <plugins> + <plugin> + <groupId>org.apache.maven.plugins</groupId> + <artifactId>maven-assembly-plugin</artifactId> + <configuration> + <descriptorRefs combine.self="append"> + <descriptorRef>src</descriptorRef> + </descriptorRefs> + </configuration> + </plugin> + </plugins> + </build> + </profile> + </profiles> + </project> + """; + + Document document = Document.of(pomXml); + Map<Path, Document> pomMap = Map.of(Paths.get("pom.xml"), document); + + UpgradeContext context = createMockContext(); + UpgradeResult result = strategy.doApply(context, pomMap); + + assertTrue(result.success(), "Compatibility fix should succeed"); + assertTrue(result.modifiedCount() > 0, "Should have fixed combine.self attributes"); + + String xml = DomUtils.toXml(document); + assertFalse(xml.contains("combine.self"), "All combine.self attributes should be removed"); + } + } + @Nested @DisplayName("Duplicate Dependency Fixes") class DuplicateDependencyFixesTests {
