Copilot commented on code in PR #12308:
URL: https://github.com/apache/maven/pull/12308#discussion_r3434945867
##########
impl/maven-cli/src/main/java/org/apache/maven/cling/invoker/mvnup/goals/CompatibilityFixStrategy.java:
##########
@@ -759,6 +760,46 @@ private boolean fixRepositoryExpressions(
return fixed;
}
+ /**
+ * Fixes deprecated Maven 2/3 shorthand property expressions throughout
the POM.
+ * Replaces ${version}, ${groupId}, and ${artifactId} with their
${project.*} equivalents,
+ * which are the only forms supported in Maven 4.
+ */
+ private boolean fixDeprecatedPropertyExpressions(Document pomDocument,
UpgradeContext context) {
+ return fixDeprecatedPropertyExpressionsInElement(pomDocument.root(),
context);
+ }
+
+ private boolean fixDeprecatedPropertyExpressionsInElement(Element element,
UpgradeContext context) {
+ boolean fixed = false;
+
+ List<Element> children = element.childElements().toList();
+
+ if (children.isEmpty()) {
+ String text = element.textContent();
+ if (text != null && !text.isEmpty()) {
+ String fixedText = replaceDeprecatedExpressions(text);
+ if (!fixedText.equals(text)) {
+ element.textContent(fixedText);
+ context.detail("Fixed: replaced deprecated property
expression in <" + element.name() + ">: "
+ + text.trim() + " → " + fixedText.trim());
+ fixed = true;
+ }
+ }
+ } else {
+ for (Element child : children) {
+ fixed |= fixDeprecatedPropertyExpressionsInElement(child,
context);
+ }
+ }
+
+ return fixed;
+ }
+
+ private static String replaceDeprecatedExpressions(String text) {
+ return text.replace("${version}", "${project.version}")
+ .replace("${groupId}", "${project.groupId}")
+ .replace("${artifactId}", "${project.artifactId}");
Review Comment:
The PR title mentions only "${version} → ${project.version}", but the
implementation also replaces ${groupId} and ${artifactId}. To avoid confusion
for reviewers/users, please align the PR title/description with the actual
scope of the fix.
##########
impl/maven-impl/src/test/java/org/apache/maven/impl/DefaultSourceRootTest.java:
##########
@@ -229,6 +229,34 @@ void testHandlesEmptyTargetPathFromResource() {
assertFalse(targetPath.isPresent(), "targetPath should be empty for
empty string");
}
+ /** GH-12306: {@code targetPath="."} normalizes to an empty path and must
be treated as absent. */
+ @Test
+ void testHandlesDotTargetPathFromResource() {
+ Resource resource = Resource.newBuilder()
+ .directory("src/main/resources")
+ .targetPath(".")
+ .build();
+
+ DefaultSourceRoot sourceRoot = new
DefaultSourceRoot(Path.of("myproject"), ProjectScope.MAIN, resource);
+
+ assertFalse(sourceRoot.targetPath().isPresent(), "targetPath \".\"
should be treated as absent");
+ }
+
+ /** GH-12306: {@code targetPath="./subdir"} normalizes to {@code "subdir"}
and must be preserved. */
+ @Test
+ void testHandlesDotRelativeTargetPathFromResource() {
+ Resource resource = Resource.newBuilder()
+ .directory("src/main/resources")
+ .targetPath("./subdir")
+ .build();
+
+ DefaultSourceRoot sourceRoot = new
DefaultSourceRoot(Path.of("myproject"), ProjectScope.MAIN, resource);
+
+ assertTrue(
+ sourceRoot.targetPath().isPresent(), "targetPath \"./subdir\"
should be present after normalization");
+ assertEquals(Path.of("subdir"), sourceRoot.targetPath().orElseThrow());
+ }
Review Comment:
PR title/description focus on deprecated property replacement in mvnup, but
this PR also changes DefaultSourceRoot targetPath normalization (GH-12306) and
adds related tests. Please update the PR description/title to cover this
additional change, or split into a separate PR to keep changes scoped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
