From 3de8ee1356bf92d0b3d2a88541b888c72bedbe87 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 22:40:42 +0200 Subject: [PATCH 1/5] Add mvnup SourceStrategy for migrating to elements Adds a new mvnup upgrade strategy that migrates legacy source configuration to Maven 4.1.0+ elements. Handles four migration phases: - Compiler properties (maven.compiler.release, source/target) to - Compiler plugin configuration (, /) to - Custom source/test directories to - Resource/testResource sections to with lang=resources Applies when --model-version is 4.1.0+ or --all is set. Runs at @Priority(20), after all other strategies. Co-Authored-By: Claude Opus 4.6 --- .../invoker/mvnup/goals/SourceStrategy.java | 493 ++++++++++ .../mvnup/goals/SourceStrategyTest.java | 855 ++++++++++++++++++ 2 files changed, 1348 insertions(+) create mode 100644 impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java create mode 100644 impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java new file mode 100644 index 000000000000..2fc07311eaa6 --- /dev/null +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java @@ -0,0 +1,493 @@ +/* + * 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.cling.invoker.mvnup.goals; + +import java.nio.file.Path; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import eu.maveniverse.domtrip.Document; +import eu.maveniverse.domtrip.Element; +import org.apache.maven.api.cli.mvnup.UpgradeOptions; +import org.apache.maven.api.di.Named; +import org.apache.maven.api.di.Priority; +import org.apache.maven.api.di.Singleton; +import org.apache.maven.cling.invoker.mvnup.UpgradeContext; + +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.ARTIFACT_ID; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.BUILD; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.CONFIGURATION; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGIN; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGINS; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGIN_MANAGEMENT; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PROPERTIES; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.SOURCE_DIRECTORY; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.TEST_SOURCE_DIRECTORY; +import static eu.maveniverse.domtrip.maven.MavenPomElements.ModelVersions.MODEL_VERSION_4_1_0; + +/** + * Strategy for migrating legacy source configuration to Maven 4.1.0+ {@code } elements. + * + *

Handles four migration phases: + *

    + *
  1. Compiler properties ({@code maven.compiler.release}, {@code maven.compiler.source/target}) + * → {@code }
    2. + *
  2. Compiler plugin configuration ({@code }, {@code /}) + * → {@code }
    3. + *
  3. Custom source/test directories → {@code } with {@code }
    4. + *
  4. Resource directories → {@code } with {@code resources}
    5. + *
+ */ +@Named +@Singleton +@Priority(20) +public class SourceStrategy extends AbstractUpgradeStrategy { + + private static final String MAVEN_COMPILER_RELEASE = "maven.compiler.release"; + private static final String MAVEN_COMPILER_SOURCE = "maven.compiler.source"; + private static final String MAVEN_COMPILER_TARGET = "maven.compiler.target"; + private static final String MAVEN_COMPILER_PLUGIN = "maven-compiler-plugin"; + + private static final String DEFAULT_SOURCE_DIR = "src/main/java"; + private static final String DEFAULT_TEST_SOURCE_DIR = "src/test/java"; + private static final String DEFAULT_RESOURCE_DIR = "src/main/resources"; + private static final String DEFAULT_TEST_RESOURCE_DIR = "src/test/resources"; + + @Override + public boolean isApplicable(UpgradeContext context) { + UpgradeOptions options = getOptions(context); + + if (options.all().orElse(false)) { + return true; + } + + String modelVersion = options.modelVersion().orElse(null); + return modelVersion != null && ModelVersionUtils.isVersionGreaterOrEqual(modelVersion, MODEL_VERSION_4_1_0); + } + + @Override + public String getDescription() { + return "Migrating source configuration to elements"; + } + + @Override + protected UpgradeResult doApply(UpgradeContext context, Map pomMap) { + Set processedPoms = new HashSet<>(); + Set modifiedPoms = new HashSet<>(); + Set errorPoms = new HashSet<>(); + + for (Map.Entry entry : pomMap.entrySet()) { + Path pomPath = entry.getKey(); + Document pomDocument = entry.getValue(); + processedPoms.add(pomPath); + + String currentVersion = ModelVersionUtils.detectModelVersion(pomDocument); + context.info(pomPath + " (current: " + currentVersion + ")"); + context.indent(); + + try { + if (!ModelVersionUtils.isVersionGreaterOrEqual(currentVersion, MODEL_VERSION_4_1_0)) { + context.success("Skipping (model version " + currentVersion + " < 4.1.0)"); + continue; + } + + boolean hasChanges = migrateSources(context, pomDocument); + + if (hasChanges) { + modifiedPoms.add(pomPath); + context.success("Source configuration migrated to elements"); + } else { + context.success("No source configuration to migrate"); + } + } catch (Exception e) { + context.failure("Failed to migrate source configuration: " + e.getMessage()); + errorPoms.add(pomPath); + } finally { + context.unindent(); + } + } + + return new UpgradeResult(processedPoms, modifiedPoms, errorPoms); + } + + private boolean migrateSources(UpgradeContext context, Document pomDocument) { + Element root = pomDocument.root(); + boolean hasChanges = false; + + String targetVersion = extractTargetVersionFromProperties(context, root); + + if (targetVersion == null) { + targetVersion = extractTargetVersionFromCompilerPlugin(context, root); + } + + if (targetVersion != null) { + Element sourcesElement = ensureSourcesElement(root); + Element sourceElement = DomUtils.insertNewElement("source", sourcesElement); + DomUtils.insertContentElement(sourceElement, "targetVersion", targetVersion); + context.detail("Set targetVersion: " + targetVersion); + hasChanges = true; + } + + hasChanges |= migrateSourceDirectories(context, root); + hasChanges |= migrateResources(context, root); + + return hasChanges; + } + + String extractTargetVersionFromProperties(UpgradeContext context, Element root) { + Element properties = root.childElement(PROPERTIES).orElse(null); + if (properties == null) { + return null; + } + + String targetVersion = null; + + Element releaseElement = properties.childElement(MAVEN_COMPILER_RELEASE).orElse(null); + if (releaseElement != null) { + targetVersion = releaseElement.textContent().trim(); + DomUtils.removeElement(releaseElement); + context.detail("Migrated property: " + MAVEN_COMPILER_RELEASE + " = " + targetVersion); + + // Also remove source/target if present (release takes precedence) + properties.childElement(MAVEN_COMPILER_SOURCE).ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed property: " + MAVEN_COMPILER_SOURCE + " (release takes precedence)"); + }); + properties.childElement(MAVEN_COMPILER_TARGET).ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed property: " + MAVEN_COMPILER_TARGET + " (release takes precedence)"); + }); + } else { + Element sourceElement = + properties.childElement(MAVEN_COMPILER_SOURCE).orElse(null); + Element targetElement = + properties.childElement(MAVEN_COMPILER_TARGET).orElse(null); + + if (sourceElement != null && targetElement != null) { + String sourceValue = sourceElement.textContent().trim(); + String targetValue = targetElement.textContent().trim(); + + if (sourceValue.equals(targetValue)) { + targetVersion = sourceValue; + DomUtils.removeElement(sourceElement); + DomUtils.removeElement(targetElement); + context.detail("Migrated properties: " + MAVEN_COMPILER_SOURCE + " = " + MAVEN_COMPILER_TARGET + + " = " + targetVersion); + } + } + } + + if (targetVersion != null) { + removeIfEmpty(properties); + } + + return targetVersion; + } + + String extractTargetVersionFromCompilerPlugin(UpgradeContext context, Element root) { + String targetVersion = extractFromPluginSection( + context, + root.childElement(BUILD).flatMap(b -> b.childElement(PLUGINS)).orElse(null)); + + if (targetVersion == null) { + targetVersion = extractFromPluginSection( + context, + root.childElement(BUILD) + .flatMap(b -> b.childElement(PLUGIN_MANAGEMENT)) + .flatMap(pm -> pm.childElement(PLUGINS)) + .orElse(null)); + } + + return targetVersion; + } + + private String extractFromPluginSection(UpgradeContext context, Element pluginsElement) { + if (pluginsElement == null) { + return null; + } + + Element compilerPlugin = findCompilerPlugin(pluginsElement); + if (compilerPlugin == null) { + return null; + } + + Element configuration = compilerPlugin.childElement(CONFIGURATION).orElse(null); + if (configuration == null) { + return null; + } + + String targetVersion = null; + + Element releaseElement = configuration.childElement("release").orElse(null); + if (releaseElement != null) { + targetVersion = releaseElement.textContent().trim(); + DomUtils.removeElement(releaseElement); + context.detail("Migrated compiler plugin : " + targetVersion); + + configuration.childElement("source").ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed compiler plugin (release takes precedence)"); + }); + configuration.childElement("target").ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed compiler plugin (release takes precedence)"); + }); + } else { + Element sourceElement = configuration.childElement("source").orElse(null); + Element targetElement = configuration.childElement("target").orElse(null); + + if (sourceElement != null && targetElement != null) { + String sourceValue = sourceElement.textContent().trim(); + String targetValue = targetElement.textContent().trim(); + + if (sourceValue.equals(targetValue)) { + targetVersion = sourceValue; + DomUtils.removeElement(sourceElement); + DomUtils.removeElement(targetElement); + context.detail("Migrated compiler plugin /: " + targetVersion); + } + } + } + + if (targetVersion != null) { + cleanupCompilerPlugin(compilerPlugin, configuration, pluginsElement); + } + + return targetVersion; + } + + private Element findCompilerPlugin(Element pluginsElement) { + return pluginsElement.childElements(PLUGIN).toList().stream() + .filter(plugin -> { + String artifactId = plugin.childTextTrimmed(ARTIFACT_ID); + return MAVEN_COMPILER_PLUGIN.equals(artifactId); + }) + .findFirst() + .orElse(null); + } + + private void cleanupCompilerPlugin(Element compilerPlugin, Element configuration, Element pluginsElement) { + if (!configuration.childElements().findAny().isPresent()) { + DomUtils.removeElement(configuration); + } + + boolean hasConfig = compilerPlugin.childElement(CONFIGURATION).isPresent(); + boolean hasExecutions = compilerPlugin.childElement("executions").isPresent(); + boolean hasDeps = compilerPlugin.childElement("dependencies").isPresent(); + + if (!hasConfig && !hasExecutions && !hasDeps) { + DomUtils.removeElement(compilerPlugin); + removeIfEmpty(pluginsElement); + } + } + + boolean migrateSourceDirectories(UpgradeContext context, Element root) { + Element buildElement = root.childElement(BUILD).orElse(null); + if (buildElement == null) { + return false; + } + + boolean hasChanges = false; + + Element sourceDir = buildElement.childElement(SOURCE_DIRECTORY).orElse(null); + if (sourceDir != null) { + String dir = sourceDir.textContent().trim(); + DomUtils.removeElement(sourceDir); + + if (!DEFAULT_SOURCE_DIR.equals(dir)) { + Element sourcesElement = ensureSourcesElement(root); + Element mainSource = findOrCreateMainJavaSource(sourcesElement); + DomUtils.insertContentElement(mainSource, "directory", dir); + context.detail("Migrated sourceDirectory: " + dir); + } else { + context.detail("Removed default sourceDirectory"); + } + hasChanges = true; + } + + Element testSourceDir = buildElement.childElement(TEST_SOURCE_DIRECTORY).orElse(null); + if (testSourceDir != null) { + String dir = testSourceDir.textContent().trim(); + DomUtils.removeElement(testSourceDir); + + if (!DEFAULT_TEST_SOURCE_DIR.equals(dir)) { + Element sourcesElement = ensureSourcesElement(root); + Element testSource = DomUtils.insertNewElement("source", sourcesElement); + DomUtils.insertContentElement(testSource, "scope", "test"); + DomUtils.insertContentElement(testSource, "directory", dir); + context.detail("Migrated testSourceDirectory: " + dir); + } else { + context.detail("Removed default testSourceDirectory"); + } + hasChanges = true; + } + + return hasChanges; + } + + boolean migrateResources(UpgradeContext context, Element root) { + Element buildElement = root.childElement(BUILD).orElse(null); + if (buildElement == null) { + return false; + } + + boolean hasChanges = false; + + hasChanges |= migrateResourceSection(context, root, buildElement, "resources", "resource", "main"); + hasChanges |= migrateResourceSection(context, root, buildElement, "testResources", "testResource", "test"); + + return hasChanges; + } + + private boolean migrateResourceSection( + UpgradeContext context, + Element root, + Element buildElement, + String containerName, + String elementName, + String scope) { + Element container = buildElement.childElement(containerName).orElse(null); + if (container == null) { + return false; + } + + List resources = container.childElements(elementName).toList(); + + for (Element resource : resources) { + if (isDefaultResource(resource, scope)) { + continue; + } + + Element sourcesElement = ensureSourcesElement(root); + Element sourceElement = DomUtils.insertNewElement("source", sourcesElement); + + if ("test".equals(scope)) { + DomUtils.insertContentElement(sourceElement, "scope", "test"); + } + DomUtils.insertContentElement(sourceElement, "lang", "resources"); + + String directory = resource.childTextTrimmed("directory"); + String defaultDir = "main".equals(scope) ? DEFAULT_RESOURCE_DIR : DEFAULT_TEST_RESOURCE_DIR; + if (directory != null && !directory.isEmpty() && !defaultDir.equals(directory)) { + DomUtils.insertContentElement(sourceElement, "directory", directory); + } + + String filtering = resource.childTextTrimmed("filtering"); + if ("true".equals(filtering)) { + DomUtils.insertContentElement(sourceElement, "stringFiltering", "true"); + } + + copyIncludesExcludes(resource, sourceElement); + + String targetPath = resource.childTextTrimmed("targetPath"); + if (targetPath != null && !targetPath.isEmpty()) { + DomUtils.insertContentElement(sourceElement, "targetPath", targetPath); + } + + context.detail("Migrated " + scope + " resource: " + (directory != null ? directory : defaultDir)); + } + + DomUtils.removeElement(container); + return true; + } + + private boolean isDefaultResource(Element resource, String scope) { + String directory = resource.childTextTrimmed("directory"); + String defaultDir = "main".equals(scope) ? DEFAULT_RESOURCE_DIR : DEFAULT_TEST_RESOURCE_DIR; + boolean isDefaultDir = directory == null || directory.isEmpty() || defaultDir.equals(directory); + if (!isDefaultDir) { + return false; + } + + String filtering = resource.childTextTrimmed("filtering"); + if ("true".equals(filtering)) { + return false; + } + + String targetPath = resource.childTextTrimmed("targetPath"); + if (targetPath != null && !targetPath.isEmpty()) { + return false; + } + + if (resource.childElement("includes").isPresent()) { + return false; + } + if (resource.childElement("excludes").isPresent()) { + return false; + } + + return true; + } + + private void copyIncludesExcludes(Element source, Element target) { + source.childElement("includes").ifPresent(includes -> { + Element newIncludes = DomUtils.insertNewElement("includes", target); + includes.childElements("include").forEach(include -> { + String value = include.textContent(); + if (value != null && !value.trim().isEmpty()) { + DomUtils.insertContentElement(newIncludes, "include", value.trim()); + } + }); + }); + + source.childElement("excludes").ifPresent(excludes -> { + Element newExcludes = DomUtils.insertNewElement("excludes", target); + excludes.childElements("exclude").forEach(exclude -> { + String value = exclude.textContent(); + if (value != null && !value.trim().isEmpty()) { + DomUtils.insertContentElement(newExcludes, "exclude", value.trim()); + } + }); + }); + } + + private Element ensureSourcesElement(Element root) { + Element buildElement = root.childElement(BUILD).orElse(null); + if (buildElement == null) { + buildElement = DomUtils.insertNewElement(BUILD, root); + } + + Element sourcesElement = buildElement.childElement("sources").orElse(null); + if (sourcesElement == null) { + sourcesElement = DomUtils.insertNewElement("sources", buildElement); + } + return sourcesElement; + } + + private Element findOrCreateMainJavaSource(Element sourcesElement) { + // Look for an existing main/java source element (e.g. one created for targetVersion) + for (Element source : sourcesElement.childElements("source").toList()) { + String scope = source.childTextTrimmed("scope"); + String lang = source.childTextTrimmed("lang"); + if ((scope == null || scope.isEmpty() || "main".equals(scope)) + && (lang == null || lang.isEmpty() || "java".equals(lang))) { + return source; + } + } + return DomUtils.insertNewElement("source", sourcesElement); + } + + private static void removeIfEmpty(Element element) { + if (!element.childElements().findAny().isPresent()) { + DomUtils.removeElement(element); + } + } +} diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java new file mode 100644 index 000000000000..69dc74297103 --- /dev/null +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java @@ -0,0 +1,855 @@ +/* + * 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.cling.invoker.mvnup.goals; + +import java.nio.file.Paths; +import java.util.HashMap; +import java.util.Map; + +import eu.maveniverse.domtrip.Document; +import eu.maveniverse.domtrip.Element; +import org.apache.maven.cling.invoker.mvnup.UpgradeContext; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +@DisplayName("SourceStrategy") +class SourceStrategyTest { + + private SourceStrategy strategy; + + @BeforeEach + void setUp() { + strategy = new SourceStrategy(); + } + + @Nested + @DisplayName("Applicability") + class ApplicabilityTests { + + @Test + @DisplayName("should not be applicable by default") + void shouldNotBeApplicableByDefault() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createDefaultOptions()); + assertFalse(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should be applicable with --model-version 4.1.0") + void shouldBeApplicableWithModelVersion410() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + assertTrue(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should be applicable with --model-version 4.2.0") + void shouldBeApplicableWithModelVersion420() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.2.0")); + assertTrue(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should not be applicable with --model-version 4.0.0") + void shouldNotBeApplicableWithModelVersion400() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.0.0")); + assertFalse(strategy.isApplicable(context)); + } + + @Test + @DisplayName("should be applicable with --all") + void shouldBeApplicableWithAll() { + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithAll(true)); + assertTrue(strategy.isApplicable(context)); + } + } + + @Nested + @DisplayName("Compiler Properties Migration") + class CompilerPropertiesTests { + + @Test + @DisplayName("should migrate maven.compiler.release to targetVersion") + void shouldMigrateCompilerRelease() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("17", source.childTextTrimmed("targetVersion")); + assertNull(DomUtils.findChildElement(doc.root(), "properties")); + } + + @Test + @DisplayName("should migrate matching maven.compiler.source and target") + void shouldMigrateMatchingSourceTarget() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 11 + 11 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("11", source.childTextTrimmed("targetVersion")); + assertNull(DomUtils.findChildElement(doc.root(), "properties")); + } + + @Test + @DisplayName("should skip when source and target differ") + void shouldSkipWhenSourceTargetDiffer() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 11 + 17 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertFalse(doc.root().childElement("build").isPresent()); + assertNotNull(DomUtils.findChildElement(doc.root(), "properties")); + assertEquals(0, result.modifiedCount()); + } + + @Test + @DisplayName("should prefer release over source/target") + void shouldPreferReleaseOverSourceTarget() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 21 + 17 + 17 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("21", source.childTextTrimmed("targetVersion")); + assertNull(DomUtils.findChildElement(doc.root(), "properties")); + } + + @Test + @DisplayName("should keep other properties when removing compiler properties") + void shouldKeepOtherProperties() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + UTF-8 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element properties = DomUtils.findChildElement(doc.root(), "properties"); + assertNotNull(properties); + assertEquals("UTF-8", properties.childTextTrimmed("project.build.sourceEncoding")); + assertNull(DomUtils.findChildElement(properties, "maven.compiler.release")); + } + } + + @Nested + @DisplayName("Compiler Plugin Migration") + class CompilerPluginTests { + + @Test + @DisplayName("should migrate plugin release configuration") + void shouldMigratePluginRelease() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + maven-compiler-plugin + + 17 + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("17", source.childTextTrimmed("targetVersion")); + + // Plugin should be removed since it has no remaining config + assertFalse(doc.root() + .childElement("build") + .orElseThrow() + .childElement("plugins") + .isPresent()); + } + + @Test + @DisplayName("should migrate plugin source/target configuration") + void shouldMigratePluginSourceTarget() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + maven-compiler-plugin + + 11 + 11 + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("11", source.childTextTrimmed("targetVersion")); + } + + @Test + @DisplayName("should keep plugin with remaining executions") + void shouldKeepPluginWithExecutions() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + maven-compiler-plugin + + 17 + + + + custom + + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + // Plugin should still exist (has executions) + Element plugin = doc.root() + .childElement("build") + .orElseThrow() + .childElement("plugins") + .orElseThrow() + .childElement("plugin") + .orElseThrow(); + + assertNotNull(plugin); + assertFalse(plugin.childElement("configuration").isPresent()); + assertTrue(plugin.childElement("executions").isPresent()); + } + + @Test + @DisplayName("should not extract from plugin when properties already provided targetVersion") + void shouldNotDuplicateTargetVersion() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 21 + + + + + maven-compiler-plugin + + 21 + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + // Should have exactly one element with targetVersion from properties + var sources = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElements("source") + .toList(); + + assertEquals(1, sources.size()); + assertEquals("21", sources.get(0).childTextTrimmed("targetVersion")); + } + } + + @Nested + @DisplayName("Source Directory Migration") + class SourceDirectoryTests { + + @Test + @DisplayName("should migrate non-default sourceDirectory") + void shouldMigrateNonDefaultSourceDirectory() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + src/main/java-custom + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("src/main/java-custom", source.childTextTrimmed("directory")); + assertNull( + DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "sourceDirectory")); + } + + @Test + @DisplayName("should migrate non-default testSourceDirectory") + void shouldMigrateNonDefaultTestSourceDirectory() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + src/test/java-custom + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("test", source.childTextTrimmed("scope")); + assertEquals("src/test/java-custom", source.childTextTrimmed("directory")); + } + + @Test + @DisplayName("should remove default sourceDirectory without creating source element") + void shouldRemoveDefaultSourceDirectory() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + src/main/java + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertNull( + DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "sourceDirectory")); + assertFalse(doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .isPresent()); + assertEquals(1, result.modifiedCount()); + } + } + + @Nested + @DisplayName("Resource Migration") + class ResourceTests { + + @Test + @DisplayName("should migrate resource with filtering") + void shouldMigrateResourceWithFiltering() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/main/resources + true + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("resources", source.childTextTrimmed("lang")); + assertEquals("true", source.childTextTrimmed("stringFiltering")); + assertNull( + DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "resources")); + } + + @Test + @DisplayName("should migrate resource with includes and excludes") + void shouldMigrateResourceWithIncludesExcludes() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/main/resources + + **/*.xml + + + **/*.bak + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("resources", source.childTextTrimmed("lang")); + + Element includes = source.childElement("includes").orElseThrow(); + assertEquals("**/*.xml", includes.childTextTrimmed("include")); + + Element excludes = source.childElement("excludes").orElseThrow(); + assertEquals("**/*.bak", excludes.childTextTrimmed("exclude")); + } + + @Test + @DisplayName("should migrate resource with targetPath") + void shouldMigrateResourceWithTargetPath() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/main/resources + META-INF + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("resources", source.childTextTrimmed("lang")); + assertEquals("META-INF", source.childTextTrimmed("targetPath")); + } + + @Test + @DisplayName("should migrate test resource") + void shouldMigrateTestResource() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/test/resources + true + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElement("source") + .orElseThrow(); + + assertEquals("test", source.childTextTrimmed("scope")); + assertEquals("resources", source.childTextTrimmed("lang")); + assertEquals("true", source.childTextTrimmed("stringFiltering")); + } + + @Test + @DisplayName("should remove default resource without creating source element") + void shouldRemoveDefaultResource() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/main/resources + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertNull( + DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "resources")); + assertFalse(doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .isPresent()); + assertEquals(1, result.modifiedCount()); + } + } + + @Nested + @DisplayName("Model Version Filtering") + class ModelVersionFilteringTests { + + @Test + @DisplayName("should skip POM at model version 4.0.0") + void shouldSkipPomAt400() { + String pomXml = """ + + + 4.0.0 + com.example + test + 1.0 + + 17 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertNotNull(DomUtils.findChildElement(doc.root(), "properties")); + assertEquals(0, result.modifiedCount()); + } + + @Test + @DisplayName("should process POM at model version 4.1.0") + void shouldProcessPomAt410() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + assertEquals(1, result.modifiedCount()); + } + } + + @Nested + @DisplayName("Combined Migration") + class CombinedTests { + + @Test + @DisplayName("should merge targetVersion and directory into single source element") + void shouldMergeTargetVersionAndDirectory() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + + + src/main/java-custom + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + var sources = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElements("source") + .toList(); + + assertEquals(1, sources.size()); + assertEquals("17", sources.get(0).childTextTrimmed("targetVersion")); + assertEquals("src/main/java-custom", sources.get(0).childTextTrimmed("directory")); + } + + @Test + @DisplayName("should create multiple source elements for different resource configs") + void shouldCreateMultipleResourceSources() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + src/main/resources + true + + + src/main/resources-extra + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + var sources = doc.root() + .childElement("build") + .orElseThrow() + .childElement("sources") + .orElseThrow() + .childElements("source") + .toList(); + + assertEquals(2, sources.size()); + } + } +} From 3db084d40170c357649d767cfbbec592cddd03ef Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 22:56:57 +0200 Subject: [PATCH 2/5] Use domtrip path() API in SourceStrategy tests Co-Authored-By: Claude Opus 4.6 --- .../mvnup/goals/SourceStrategyTest.java | 167 ++++-------------- 1 file changed, 31 insertions(+), 136 deletions(-) diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java index 69dc74297103..3c8deadf7ca6 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java @@ -32,8 +32,6 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; @DisplayName("SourceStrategy") @@ -110,16 +108,10 @@ void shouldMigrateCompilerRelease() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("17", source.childTextTrimmed("targetVersion")); - assertNull(DomUtils.findChildElement(doc.root(), "properties")); + assertFalse(doc.root().childElement("properties").isPresent()); } @Test @@ -143,16 +135,10 @@ void shouldMigrateMatchingSourceTarget() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("11", source.childTextTrimmed("targetVersion")); - assertNull(DomUtils.findChildElement(doc.root(), "properties")); + assertFalse(doc.root().childElement("properties").isPresent()); } @Test @@ -177,7 +163,7 @@ void shouldSkipWhenSourceTargetDiffer() { UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); assertFalse(doc.root().childElement("build").isPresent()); - assertNotNull(DomUtils.findChildElement(doc.root(), "properties")); + assertTrue(doc.root().childElement("properties").isPresent()); assertEquals(0, result.modifiedCount()); } @@ -203,16 +189,10 @@ void shouldPreferReleaseOverSourceTarget() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("21", source.childTextTrimmed("targetVersion")); - assertNull(DomUtils.findChildElement(doc.root(), "properties")); + assertFalse(doc.root().childElement("properties").isPresent()); } @Test @@ -236,10 +216,9 @@ void shouldKeepOtherProperties() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element properties = DomUtils.findChildElement(doc.root(), "properties"); - assertNotNull(properties); + Element properties = doc.root().childElement("properties").orElseThrow(); assertEquals("UTF-8", properties.childTextTrimmed("project.build.sourceEncoding")); - assertNull(DomUtils.findChildElement(properties, "maven.compiler.release")); + assertFalse(properties.childElement("maven.compiler.release").isPresent()); } } @@ -274,22 +253,12 @@ void shouldMigratePluginRelease() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("17", source.childTextTrimmed("targetVersion")); // Plugin should be removed since it has no remaining config - assertFalse(doc.root() - .childElement("build") - .orElseThrow() - .childElement("plugins") - .isPresent()); + assertFalse(doc.root().path("build", "plugins").isPresent()); } @Test @@ -320,13 +289,7 @@ void shouldMigratePluginSourceTarget() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("11", source.childTextTrimmed("targetVersion")); } @@ -363,16 +326,7 @@ void shouldKeepPluginWithExecutions() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - // Plugin should still exist (has executions) - Element plugin = doc.root() - .childElement("build") - .orElseThrow() - .childElement("plugins") - .orElseThrow() - .childElement("plugin") - .orElseThrow(); - - assertNotNull(plugin); + Element plugin = doc.root().path("build", "plugins", "plugin").orElseThrow(); assertFalse(plugin.childElement("configuration").isPresent()); assertTrue(plugin.childElement("executions").isPresent()); } @@ -407,11 +361,8 @@ void shouldNotDuplicateTargetVersion() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - // Should have exactly one element with targetVersion from properties var sources = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") + .path("build", "sources") .orElseThrow() .childElements("source") .toList(); @@ -445,17 +396,10 @@ void shouldMigrateNonDefaultSourceDirectory() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("src/main/java-custom", source.childTextTrimmed("directory")); - assertNull( - DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "sourceDirectory")); + assertFalse(doc.root().path("build", "sourceDirectory").isPresent()); } @Test @@ -478,13 +422,7 @@ void shouldMigrateNonDefaultTestSourceDirectory() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("test", source.childTextTrimmed("scope")); assertEquals("src/test/java-custom", source.childTextTrimmed("directory")); @@ -510,13 +448,8 @@ void shouldRemoveDefaultSourceDirectory() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - assertNull( - DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "sourceDirectory")); - assertFalse(doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .isPresent()); + assertFalse(doc.root().path("build", "sourceDirectory").isPresent()); + assertFalse(doc.root().path("build", "sources").isPresent()); assertEquals(1, result.modifiedCount()); } } @@ -550,18 +483,11 @@ void shouldMigrateResourceWithFiltering() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("resources", source.childTextTrimmed("lang")); assertEquals("true", source.childTextTrimmed("stringFiltering")); - assertNull( - DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "resources")); + assertFalse(doc.root().path("build", "resources").isPresent()); } @Test @@ -594,21 +520,11 @@ void shouldMigrateResourceWithIncludesExcludes() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("resources", source.childTextTrimmed("lang")); - - Element includes = source.childElement("includes").orElseThrow(); - assertEquals("**/*.xml", includes.childTextTrimmed("include")); - - Element excludes = source.childElement("excludes").orElseThrow(); - assertEquals("**/*.bak", excludes.childTextTrimmed("exclude")); + assertEquals("**/*.xml", source.path("includes").orElseThrow().childTextTrimmed("include")); + assertEquals("**/*.bak", source.path("excludes").orElseThrow().childTextTrimmed("exclude")); } @Test @@ -636,13 +552,7 @@ void shouldMigrateResourceWithTargetPath() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("resources", source.childTextTrimmed("lang")); assertEquals("META-INF", source.childTextTrimmed("targetPath")); @@ -673,13 +583,7 @@ void shouldMigrateTestResource() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - Element source = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .orElseThrow() - .childElement("source") - .orElseThrow(); + Element source = doc.root().path("build", "sources", "source").orElseThrow(); assertEquals("test", source.childTextTrimmed("scope")); assertEquals("resources", source.childTextTrimmed("lang")); @@ -710,13 +614,8 @@ void shouldRemoveDefaultResource() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - assertNull( - DomUtils.findChildElement(doc.root().childElement("build").orElseThrow(), "resources")); - assertFalse(doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") - .isPresent()); + assertFalse(doc.root().path("build", "resources").isPresent()); + assertFalse(doc.root().path("build", "sources").isPresent()); assertEquals(1, result.modifiedCount()); } } @@ -745,7 +644,7 @@ void shouldSkipPomAt400() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - assertNotNull(DomUtils.findChildElement(doc.root(), "properties")); + assertTrue(doc.root().childElement("properties").isPresent()); assertEquals(0, result.modifiedCount()); } @@ -801,9 +700,7 @@ void shouldMergeTargetVersionAndDirectory() { strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); var sources = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") + .path("build", "sources") .orElseThrow() .childElements("source") .toList(); @@ -842,9 +739,7 @@ void shouldCreateMultipleResourceSources() { strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); var sources = doc.root() - .childElement("build") - .orElseThrow() - .childElement("sources") + .path("build", "sources") .orElseThrow() .childElements("source") .toList(); From 7225b3d09a5c9eac8ac0552a7397d9fb4b95be76 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Tue, 23 Jun 2026 23:17:08 +0200 Subject: [PATCH 3/5] Fix review issues in SourceStrategy - Check groupId in findCompilerPlugin (not just artifactId) - Clean up compiler plugin config when properties already provide targetVersion (previously left stale release/source/target elements) - Clean up empty pluginManagement after plugin removal - Clean up empty build element after all children removed - Add test for pluginManagement compiler plugin migration Co-Authored-By: Claude Opus 4.6 --- .../invoker/mvnup/goals/SourceStrategy.java | 67 +++++++++++++++++-- .../mvnup/goals/SourceStrategyTest.java | 42 ++++++++++-- 2 files changed, 100 insertions(+), 9 deletions(-) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java index 2fc07311eaa6..1509545ebfd9 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java @@ -35,6 +35,7 @@ import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.ARTIFACT_ID; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.BUILD; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.CONFIGURATION; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.GROUP_ID; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGIN; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGINS; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.PLUGIN_MANAGEMENT; @@ -42,6 +43,7 @@ import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.SOURCE_DIRECTORY; import static eu.maveniverse.domtrip.maven.MavenPomElements.Elements.TEST_SOURCE_DIRECTORY; import static eu.maveniverse.domtrip.maven.MavenPomElements.ModelVersions.MODEL_VERSION_4_1_0; +import static eu.maveniverse.domtrip.maven.MavenPomElements.Plugins.DEFAULT_MAVEN_PLUGIN_GROUP_ID; /** * Strategy for migrating legacy source configuration to Maven 4.1.0+ {@code } elements. @@ -136,6 +138,8 @@ private boolean migrateSources(UpgradeContext context, Document pomDocument) { if (targetVersion == null) { targetVersion = extractTargetVersionFromCompilerPlugin(context, root); + } else { + cleanupCompilerPluginConfig(context, root); } if (targetVersion != null) { @@ -149,6 +153,8 @@ private boolean migrateSources(UpgradeContext context, Document pomDocument) { hasChanges |= migrateSourceDirectories(context, root); hasChanges |= migrateResources(context, root); + cleanupEmptyBuild(root); + return hasChanges; } @@ -278,27 +284,78 @@ private Element findCompilerPlugin(Element pluginsElement) { return pluginsElement.childElements(PLUGIN).toList().stream() .filter(plugin -> { String artifactId = plugin.childTextTrimmed(ARTIFACT_ID); - return MAVEN_COMPILER_PLUGIN.equals(artifactId); + String groupId = plugin.childTextTrimmed(GROUP_ID); + return MAVEN_COMPILER_PLUGIN.equals(artifactId) + && (groupId == null || groupId.isEmpty() || DEFAULT_MAVEN_PLUGIN_GROUP_ID.equals(groupId)); }) .findFirst() .orElse(null); } - private void cleanupCompilerPlugin(Element compilerPlugin, Element configuration, Element pluginsElement) { - if (!configuration.childElements().findAny().isPresent()) { - DomUtils.removeElement(configuration); + private void cleanupCompilerPluginConfig(UpgradeContext context, Element root) { + cleanupPluginSectionConfig( + context, + root.childElement(BUILD).flatMap(b -> b.childElement(PLUGINS)).orElse(null)); + cleanupPluginSectionConfig( + context, + root.childElement(BUILD) + .flatMap(b -> b.childElement(PLUGIN_MANAGEMENT)) + .flatMap(pm -> pm.childElement(PLUGINS)) + .orElse(null)); + } + + private void cleanupPluginSectionConfig(UpgradeContext context, Element pluginsElement) { + if (pluginsElement == null) { + return; + } + Element compilerPlugin = findCompilerPlugin(pluginsElement); + if (compilerPlugin == null) { + return; + } + Element configuration = compilerPlugin.childElement(CONFIGURATION).orElse(null); + if (configuration == null) { + return; } + configuration.childElement("release").ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed compiler plugin (properties take precedence)"); + }); + configuration.childElement("source").ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed compiler plugin (properties take precedence)"); + }); + configuration.childElement("target").ifPresent(e -> { + DomUtils.removeElement(e); + context.detail("Removed compiler plugin (properties take precedence)"); + }); + cleanupCompilerPlugin(compilerPlugin, configuration, pluginsElement); + } + + private void cleanupCompilerPlugin(Element compilerPlugin, Element configuration, Element pluginsElement) { + removeIfEmpty(configuration); boolean hasConfig = compilerPlugin.childElement(CONFIGURATION).isPresent(); boolean hasExecutions = compilerPlugin.childElement("executions").isPresent(); boolean hasDeps = compilerPlugin.childElement("dependencies").isPresent(); if (!hasConfig && !hasExecutions && !hasDeps) { + Element pluginsParent = pluginsElement.parent() instanceof Element parent ? parent : null; DomUtils.removeElement(compilerPlugin); removeIfEmpty(pluginsElement); + if (pluginsParent != null && PLUGIN_MANAGEMENT.equals(pluginsParent.name())) { + removeIfEmpty(pluginsParent); + } } } + private static void cleanupEmptyBuild(Element root) { + root.childElement(BUILD).ifPresent(build -> { + if (!build.childElements().findAny().isPresent()) { + DomUtils.removeElement(build); + } + }); + } + boolean migrateSourceDirectories(UpgradeContext context, Element root) { Element buildElement = root.childElement(BUILD).orElse(null); if (buildElement == null) { @@ -486,7 +543,7 @@ private Element findOrCreateMainJavaSource(Element sourcesElement) { } private static void removeIfEmpty(Element element) { - if (!element.childElements().findAny().isPresent()) { + if (element != null && !element.childElements().findAny().isPresent()) { DomUtils.removeElement(element); } } diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java index 3c8deadf7ca6..3a6f16903286 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java @@ -369,6 +369,42 @@ void shouldNotDuplicateTargetVersion() { assertEquals(1, sources.size()); assertEquals("21", sources.get(0).childTextTrimmed("targetVersion")); + assertFalse(doc.root().path("build", "plugins").isPresent()); + } + + @Test + @DisplayName("should migrate compiler plugin from pluginManagement") + void shouldMigrateFromPluginManagement() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + + + + org.apache.maven.plugins + maven-compiler-plugin + + 17 + + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element source = doc.root().path("build", "sources", "source").orElseThrow(); + assertEquals("17", source.childTextTrimmed("targetVersion")); + assertFalse(doc.root().path("build", "pluginManagement").isPresent()); } } @@ -448,8 +484,7 @@ void shouldRemoveDefaultSourceDirectory() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - assertFalse(doc.root().path("build", "sourceDirectory").isPresent()); - assertFalse(doc.root().path("build", "sources").isPresent()); + assertFalse(doc.root().childElement("build").isPresent()); assertEquals(1, result.modifiedCount()); } } @@ -614,8 +649,7 @@ void shouldRemoveDefaultResource() { UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); UpgradeResult result = strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); - assertFalse(doc.root().path("build", "resources").isPresent()); - assertFalse(doc.root().path("build", "sources").isPresent()); + assertFalse(doc.root().childElement("build").isPresent()); assertEquals(1, result.modifiedCount()); } } From 2f4a615000e66f838848d901c3e1bb8479b1044e Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Wed, 24 Jun 2026 23:24:49 +0200 Subject: [PATCH 4/5] Extract copyPatternList to deduplicate includes/excludes handling Address review feedback from desruisseaux: extract shared logic from copyIncludesExcludes into a static copyPatternList method. Co-Authored-By: Claude Opus 4.6 --- .../invoker/mvnup/goals/SourceStrategy.java | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java index 1509545ebfd9..94ac555e159e 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java @@ -494,23 +494,18 @@ private boolean isDefaultResource(Element resource, String scope) { return true; } - private void copyIncludesExcludes(Element source, Element target) { - source.childElement("includes").ifPresent(includes -> { - Element newIncludes = DomUtils.insertNewElement("includes", target); - includes.childElements("include").forEach(include -> { - String value = include.textContent(); - if (value != null && !value.trim().isEmpty()) { - DomUtils.insertContentElement(newIncludes, "include", value.trim()); - } - }); - }); + private static void copyIncludesExcludes(Element source, Element target) { + copyPatternList(source, target, "includes", "include"); + copyPatternList(source, target, "excludes", "exclude"); + } - source.childElement("excludes").ifPresent(excludes -> { - Element newExcludes = DomUtils.insertNewElement("excludes", target); - excludes.childElements("exclude").forEach(exclude -> { - String value = exclude.textContent(); + private static void copyPatternList(Element source, Element target, String containerName, String elementName) { + source.childElement(containerName).ifPresent(container -> { + Element newContainer = DomUtils.insertNewElement(containerName, target); + container.childElements(elementName).forEach(element -> { + String value = element.textContent(); if (value != null && !value.trim().isEmpty()) { - DomUtils.insertContentElement(newExcludes, "exclude", value.trim()); + DomUtils.insertContentElement(newContainer, elementName, value.trim()); } }); }); From da1a758d93a58035e4c16da98452d5636b208f99 Mon Sep 17 00:00:00 2001 From: Guillaume Nodet Date: Thu, 25 Jun 2026 04:47:25 +0000 Subject: [PATCH 5/5] Address Copilot review comments on SourceStrategy - Reuse existing main/java element when adding targetVersion instead of always creating a new one (avoids duplicate source elements) - Only remove compiler plugin config (release/source/target) when values match the migrated property value (preserves intentional overrides) - Add tests for both edge cases Co-Authored-By: Claude Opus 4.6 --- .../invoker/mvnup/goals/SourceStrategy.java | 52 ++++++++----- .../mvnup/goals/SourceStrategyTest.java | 76 +++++++++++++++++++ 2 files changed, 109 insertions(+), 19 deletions(-) diff --git a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java index 94ac555e159e..f6a6865255d6 100644 --- a/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java +++ b/impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategy.java @@ -139,12 +139,12 @@ private boolean migrateSources(UpgradeContext context, Document pomDocument) { if (targetVersion == null) { targetVersion = extractTargetVersionFromCompilerPlugin(context, root); } else { - cleanupCompilerPluginConfig(context, root); + cleanupCompilerPluginConfig(context, root, targetVersion); } if (targetVersion != null) { Element sourcesElement = ensureSourcesElement(root); - Element sourceElement = DomUtils.insertNewElement("source", sourcesElement); + Element sourceElement = findOrCreateMainJavaSource(sourcesElement); DomUtils.insertContentElement(sourceElement, "targetVersion", targetVersion); context.detail("Set targetVersion: " + targetVersion); hasChanges = true; @@ -292,19 +292,21 @@ private Element findCompilerPlugin(Element pluginsElement) { .orElse(null); } - private void cleanupCompilerPluginConfig(UpgradeContext context, Element root) { + private void cleanupCompilerPluginConfig(UpgradeContext context, Element root, String migratedValue) { cleanupPluginSectionConfig( context, - root.childElement(BUILD).flatMap(b -> b.childElement(PLUGINS)).orElse(null)); + root.childElement(BUILD).flatMap(b -> b.childElement(PLUGINS)).orElse(null), + migratedValue); cleanupPluginSectionConfig( context, root.childElement(BUILD) .flatMap(b -> b.childElement(PLUGIN_MANAGEMENT)) .flatMap(pm -> pm.childElement(PLUGINS)) - .orElse(null)); + .orElse(null), + migratedValue); } - private void cleanupPluginSectionConfig(UpgradeContext context, Element pluginsElement) { + private void cleanupPluginSectionConfig(UpgradeContext context, Element pluginsElement, String migratedValue) { if (pluginsElement == null) { return; } @@ -316,19 +318,31 @@ private void cleanupPluginSectionConfig(UpgradeContext context, Element pluginsE if (configuration == null) { return; } - configuration.childElement("release").ifPresent(e -> { - DomUtils.removeElement(e); - context.detail("Removed compiler plugin (properties take precedence)"); - }); - configuration.childElement("source").ifPresent(e -> { - DomUtils.removeElement(e); - context.detail("Removed compiler plugin (properties take precedence)"); - }); - configuration.childElement("target").ifPresent(e -> { - DomUtils.removeElement(e); - context.detail("Removed compiler plugin (properties take precedence)"); - }); - cleanupCompilerPlugin(compilerPlugin, configuration, pluginsElement); + boolean removed = false; + Element releaseElement = configuration.childElement("release").orElse(null); + if (releaseElement != null + && migratedValue.equals(releaseElement.textContent().trim())) { + DomUtils.removeElement(releaseElement); + context.detail("Removed compiler plugin (matches migrated value)"); + removed = true; + } + Element sourceElement = configuration.childElement("source").orElse(null); + if (sourceElement != null + && migratedValue.equals(sourceElement.textContent().trim())) { + DomUtils.removeElement(sourceElement); + context.detail("Removed compiler plugin (matches migrated value)"); + removed = true; + } + Element targetElement = configuration.childElement("target").orElse(null); + if (targetElement != null + && migratedValue.equals(targetElement.textContent().trim())) { + DomUtils.removeElement(targetElement); + context.detail("Removed compiler plugin (matches migrated value)"); + removed = true; + } + if (removed) { + cleanupCompilerPlugin(compilerPlugin, configuration, pluginsElement); + } } private void cleanupCompilerPlugin(Element compilerPlugin, Element configuration, Element pluginsElement) { diff --git a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java index 3a6f16903286..ea87657996c2 100644 --- a/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java +++ b/impl/maven-cli/src/test/java/org/apache/maven/cling/invoker/mvnup/goals/SourceStrategyTest.java @@ -372,6 +372,44 @@ void shouldNotDuplicateTargetVersion() { assertFalse(doc.root().path("build", "plugins").isPresent()); } + @Test + @DisplayName("should preserve plugin config when it differs from migrated property value") + void shouldPreservePluginConfigWhenDifferent() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + + + + + maven-compiler-plugin + + 21 + + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + Element plugin = doc.root().path("build", "plugins", "plugin").orElseThrow(); + Element configuration = plugin.childElement("configuration").orElseThrow(); + assertEquals("21", configuration.childTextTrimmed("release")); + + Element source = doc.root().path("build", "sources", "source").orElseThrow(); + assertEquals("17", source.childTextTrimmed("targetVersion")); + } + @Test @DisplayName("should migrate compiler plugin from pluginManagement") void shouldMigrateFromPluginManagement() { @@ -744,6 +782,44 @@ void shouldMergeTargetVersionAndDirectory() { assertEquals("src/main/java-custom", sources.get(0).childTextTrimmed("directory")); } + @Test + @DisplayName("should reuse existing source element when adding targetVersion") + void shouldReuseExistingSourceElement() { + String pomXml = """ + + + 4.1.0 + com.example + test + 1.0 + + 17 + + + + + src/main/java-custom + + + + + """; + + Document doc = Document.of(pomXml); + UpgradeContext context = TestUtils.createMockContext(TestUtils.createOptionsWithModelVersion("4.1.0")); + strategy.doApply(context, new HashMap<>(Map.of(Paths.get("pom.xml"), doc))); + + var sources = doc.root() + .path("build", "sources") + .orElseThrow() + .childElements("source") + .toList(); + + assertEquals(1, sources.size()); + assertEquals("17", sources.get(0).childTextTrimmed("targetVersion")); + assertEquals("src/main/java-custom", sources.get(0).childTextTrimmed("directory")); + } + @Test @DisplayName("should create multiple source elements for different resource configs") void shouldCreateMultipleResourceSources() {