gnodet-bot commented on code in PR #290:
URL: https://github.com/apache/maven-filtering/pull/290#discussion_r4093257800
##########
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##########
@@ -476,26 +478,38 @@ private String
getRelativeOutputDirectory(MavenResourcesExecution execution) {
*/
private String filterFileName(String name, List<FilterWrapper> wrappers)
throws MavenFilteringException {
- Reader reader = new StringReader(name);
- for (FilterWrapper wrapper : wrappers) {
- reader = wrapper.getReader(reader);
- }
-
- try (StringWriter writer = new StringWriter()) {
- char[] buffer = new char[BUFFER_LENGTH];
- int nRead;
- while ((nRead = reader.read(buffer, 0, buffer.length)) >= 0) {
- writer.write(buffer, 0, nRead);
+ char[] buffer = new char[BUFFER_LENGTH];
Review Comment:
⚠️ **Code clarity:** `buffer` is declared outside the per-component loop but
is consumed inside it. While this is safe (only `nRead` bytes are ever written
from it), it ties the buffer's lifetime to the outer scope for no benefit. Move
it inside the `try` block so readers don't have to reason about whether stale
data from a previous iteration could leak.
```suggestion
StringBuilder sb = new StringBuilder();
Path path = Path.of(name);
Iterator<Path> iterator = path.iterator();
while (iterator.hasNext()) {
String component = iterator.next().toString();
Reader reader = new StringReader(component);
for (FilterWrapper wrapper : wrappers) {
reader = wrapper.getReader(reader);
}
try (StringWriter writer = new StringWriter()) {
char[] buffer = new char[BUFFER_LENGTH];
int nRead;
```
##########
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##########
@@ -476,26 +478,38 @@ private String
getRelativeOutputDirectory(MavenResourcesExecution execution) {
*/
private String filterFileName(String name, List<FilterWrapper> wrappers)
throws MavenFilteringException {
- Reader reader = new StringReader(name);
- for (FilterWrapper wrapper : wrappers) {
- reader = wrapper.getReader(reader);
- }
-
- try (StringWriter writer = new StringWriter()) {
- char[] buffer = new char[BUFFER_LENGTH];
- int nRead;
- while ((nRead = reader.read(buffer, 0, buffer.length)) >= 0) {
- writer.write(buffer, 0, nRead);
+ char[] buffer = new char[BUFFER_LENGTH];
+ StringBuilder sb = new StringBuilder();
+ Path path = Path.of(name);
+ Iterator<Path> iterator = path.iterator();
+ while (iterator.hasNext()) {
+ String component = iterator.next().toString();
+ Reader reader = new StringReader(component);
+ for (FilterWrapper wrapper : wrappers) {
+ reader = wrapper.getReader(reader);
}
- String filteredFilename = writer.toString();
+ try (StringWriter writer = new StringWriter()) {
+ int nRead;
+ while ((nRead = reader.read(buffer, 0, buffer.length)) >= 0) {
+ writer.write(buffer, 0, nRead);
+ }
+ String filteredComponent = writer.toString();
+ sb.append(filteredComponent);
+ if (iterator.hasNext()) {
+ sb.append(FileSystems.getDefault().getSeparator());
Review Comment:
💡 **Minor:** `FileSystems.getDefault().getSeparator()` is called once per
path component, but the result is a JVM constant. Use `File.separator` (or
cache it in a local variable before the loop) — same semantics, no unnecessary
object lookup per iteration.
```suggestion
sb.append(File.separator);
```
(Also add `import java.io.File;` — or simply use `java.io.File.separator`
inline.)
##########
src/test/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFilteringTest.java:
##########
@@ -943,6 +944,49 @@ public void testFilterFileName() throws Exception {
List<Path> files = list(targetPathFile);
assertEquals(1, files.size());
+ assertEquals("subfolder", filename(files.get(0)));
+ assertTrue(Files.isDirectory(files.get(0)));
+
+ files = list(files.get(0));
+ assertEquals(1, files.size());
+ assertEquals("1.0.txt", filename(files.get(0)));
+ }
+
+ @Test
+ public void testFilterFileNameWithFileSeparatorAsEscape() throws Exception
{
+
+ String unitFilesDir = getBasedir() +
"/src/test/units-files/maven-filename-filtering";
+
+ Resource resource = new Resource();
+ resource.setDirectory(unitFilesDir);
+ resource.setFiltering(true);
+ resource.addInclude("**/${pom.version}*");
+ resource.setTargetPath("testTargetPath");
+
+ MavenResourcesExecution mavenResourcesExecution = new
MavenResourcesExecution(
+ Collections.singletonList(resource),
+ outputDirectory,
+ mavenProject,
+ "UTF-8",
+ Collections.<String>emptyList(),
+ Collections.<String>emptyList(),
+ new StubSession());
+ mavenResourcesExecution.setFilterFilenames(true);
+
+ // more likely to occur on windows, where the file
+ // separator is the same as the common escape string "\"
+
mavenResourcesExecution.setEscapeString(FileSystems.getDefault().getSeparator());
+ mavenResourcesFiltering.filterResources(mavenResourcesExecution);
+
+ Path targetPathFile = outputDirectory.resolve("testTargetPath");
+
+ List<Path> files = list(targetPathFile);
+ assertEquals(1, files.size());
+ assertEquals("subfolder", filename(files.get(0)));
+ assertTrue(Files.isDirectory(files.get(0)));
+
+ files = list(files.get(0));
Review Comment:
⚠️ **Same variable-reuse issue flagged by @elharo at line 950:** `files` is
reassigned here rather than a new variable declared. This pattern obscures the
test's assertion chain — if the second `assertEquals` fails, it's unclear which
`files` list is being asserted against without tracing the assignment back.
Declare a separate variable:
```suggestion
List<Path> subfolderFiles = list(files.get(0));
assertEquals(1, subfolderFiles.size());
assertEquals("1.0.txt", filename(subfolderFiles.get(0)));
```
(The same fix applies at line 950 in `testFilterFileName` per @elharo's
existing comment.)
--
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]
