Copilot commented on code in PR #287:
URL:
https://github.com/apache/maven-remote-resources-plugin/pull/287#discussion_r4230132063
##########
src/main/java/org/apache/maven/plugin/resources/remote/ModelInheritanceAssembler.java:
##########
@@ -568,33 +567,30 @@ protected String appendPath(String parentPath, String
childPath, String pathAdju
uncleanPath = uncleanPath.substring(protocolIdx + 3);
}
- if (uncleanPath.startsWith("/")) {
- cleanedPath += "/";
- }
-
return cleanedPath + resolvePath(uncleanPath);
}
- // TODO Move this to plexus-utils' PathTool.
+ /**
+ * Normalizes the path part of an SCM URL.
+ */
private static String resolvePath(String uncleanPath) {
- LinkedList<String> pathElements = new LinkedList<>();
-
- StringTokenizer tokenizer = new StringTokenizer(uncleanPath, "/");
+ boolean trailingSeparator = uncleanPath.endsWith("/");
+ boolean absolute = uncleanPath.startsWith("/");
- while (tokenizer.hasMoreTokens()) {
- String token = tokenizer.nextToken();
+ LinkedList<String> pathElements = new LinkedList<>();
+ for (String token : uncleanPath.split("/", -1)) {
switch (token) {
case "":
- // Empty path entry ("...//.."), remove.
+ case ".":
+ // Redundant separators and current-directory (".")
segments are removed.
break;
case "..":
- if (pathElements.isEmpty()) {
- // FIXME: somehow report to the user
- // that there are too many '..' elements.
- // For now, ignore the extra '..'.
- } else {
+ if (!pathElements.isEmpty() &&
!"..".equals(pathElements.getLast())) {
pathElements.removeLast();
+ } else if (!absolute) {
+ // A ".." that cannot be resolved against a preceding
element is kept.
+ pathElements.addLast(token);
}
break;
Review Comment:
For absolute paths, an excess leading `..` segment is silently dropped
(e.g., `"/../x"` becomes `"/x"`), which contradicts the stated goal of
detecting/retaining excess `..` rather than swallowing it. Consider either (a)
retaining unresolved `..` even when `absolute == true`, or (b) surfacing a
warning/error signal to the caller when an unresolved `..` is encountered for
absolute paths.
##########
src/main/java/org/apache/maven/plugin/resources/remote/ModelInheritanceAssembler.java:
##########
@@ -568,33 +567,30 @@ protected String appendPath(String parentPath, String
childPath, String pathAdju
uncleanPath = uncleanPath.substring(protocolIdx + 3);
}
- if (uncleanPath.startsWith("/")) {
- cleanedPath += "/";
- }
-
return cleanedPath + resolvePath(uncleanPath);
}
- // TODO Move this to plexus-utils' PathTool.
+ /**
+ * Normalizes the path part of an SCM URL.
+ */
private static String resolvePath(String uncleanPath) {
Review Comment:
The PR description says `resolvePath` was rewritten using
`java.nio.file.Path.normalize()` and that “excess `..` segments are detected”
(with a warning), but the implementation shown is a custom tokenizer/stack
algorithm and does not emit/detect warnings. Either update the PR description
to match the actual approach/behavior, or adjust the implementation to align
with the described `Path.normalize()`-based solution and warning behavior.
##########
src/main/java/org/apache/maven/plugin/resources/remote/ModelInheritanceAssembler.java:
##########
@@ -568,33 +567,30 @@ protected String appendPath(String parentPath, String
childPath, String pathAdju
uncleanPath = uncleanPath.substring(protocolIdx + 3);
}
- if (uncleanPath.startsWith("/")) {
- cleanedPath += "/";
- }
-
return cleanedPath + resolvePath(uncleanPath);
}
- // TODO Move this to plexus-utils' PathTool.
+ /**
+ * Normalizes the path part of an SCM URL.
+ */
private static String resolvePath(String uncleanPath) {
- LinkedList<String> pathElements = new LinkedList<>();
-
- StringTokenizer tokenizer = new StringTokenizer(uncleanPath, "/");
+ boolean trailingSeparator = uncleanPath.endsWith("/");
+ boolean absolute = uncleanPath.startsWith("/");
- while (tokenizer.hasMoreTokens()) {
- String token = tokenizer.nextToken();
+ LinkedList<String> pathElements = new LinkedList<>();
+ for (String token : uncleanPath.split("/", -1)) {
Review Comment:
`String#split` uses regex and allocates an array for all segments, which can
be relatively expensive in a hot path. If this method can be invoked frequently
(e.g., during model inheritance merging across many artifacts), consider
replacing this with a non-regex splitter (manual scan, or a small purpose-built
parser) to reduce allocations and CPU overhead.
##########
src/test/java/org/apache/maven/plugin/resources/remote/ModelInheritanceAssemblerTest.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.plugin.resources.remote;
+
+import org.junit.Test;
+
+import static org.junit.Assert.assertEquals;
+
+/**
+ * Unit tests for the SCM path normalization in {@link
ModelInheritanceAssembler}.
+ */
+public class ModelInheritanceAssemblerTest {
+
+ private final ModelInheritanceAssembler assembler = new
ModelInheritanceAssembler();
+
+ @Test
+ public void appendPathPreservesTrailingSlash() {
+ assertEquals(
+ "http://svn.example.com/repo/";,
+ assembler.appendPath("http://svn.example.com/repo/";, null,
null, false));
+ }
+
+ @Test
+ public void appendPathAppendsChild() {
+ assertEquals(
+ "http://svn.example.com/repo/child";,
+ assembler.appendPath("http://svn.example.com/repo";, "child",
null, true));
+ }
+
+ @Test
+ public void appendPathCollapsesDotSegment() {
+ assertEquals(
+ "http://svn.example.com/repo/child";,
+ assembler.appendPath("http://svn.example.com/repo/./child";,
null, null, false));
+ }
+
+ @Test
+ public void appendPathResolvesParentDirectory() {
+ assertEquals(
+ "http://svn.example.com/repos";,
+
assembler.appendPath("http://svn.example.com/repos/project/..";, null, null,
false));
+ }
+
+ @Test
+ public void appendPathCollapsesRedundantSeparators() {
+ assertEquals(
+ "http://svn.example.com/repo/child";,
+ assembler.appendPath("http://svn.example.com/repo//child";,
null, null, false));
+ }
+
+ @Test
+ public void appendPathPreservesExcessParentDirectory() {
+ assertEquals("../x", assembler.appendPath("a/../../x", null, null,
false));
+ }
Review Comment:
The PR description mentions that a warning is emitted for excess `..`
segments, but the tests currently only assert the normalized output. If warning
emission is part of the intended contract, add an assertion that the warning is
produced (e.g., via whatever logging/problem-collection mechanism
`ModelInheritanceAssembler` uses) so the behavior doesn’t regress silently.
--
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]
