gnodet-bot commented on code in PR #290:
URL: https://github.com/apache/maven-filtering/pull/290#discussion_r4093415077
##########
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);
+ 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);
}
Review Comment:
⚠️ **Resource leak: `reader` chain is never closed.**
`FilterWrapper.getReader()` can return any `Reader` implementation — including
readers backed by file I/O (e.g. when filter properties files are loaded
lazily). The outermost reader is never closed; if any wrapper holds a resource,
it leaks silently on every path component, every file.
The fix is to put the whole reader chain inside a try-with-resources block
alongside the `StringWriter`:
```suggestion
Reader reader = new StringReader(component);
for (FilterWrapper wrapper : wrappers) {
reader = wrapper.getReader(reader);
}
```
Replace the `try (StringWriter writer = ...)` with `try (StringWriter writer
= new StringWriter(); Reader reader = ...)` — but since the reader chain is
built dynamically (each wrapper wraps the previous), you need an
effectively-final holder or a separate try block:
```java
Reader reader = new StringReader(component);
for (FilterWrapper wrapper : wrappers) {
reader = wrapper.getReader(reader);
}
try (Reader closeable = reader; StringWriter writer = new
StringWriter()) {
```
This ensures the outermost reader (and transitively its wrapped chain) is
closed on every exit, including exception paths.
##########
src/test/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFilteringTest.java:
##########
@@ -943,7 +944,50 @@ public void testFilterFileName() throws Exception {
List<Path> files = list(targetPathFile);
assertEquals(1, files.size());
- assertEquals("1.0.txt", filename(files.get(0)));
+ assertEquals("subfolder", filename(files.get(0)));
+ assertTrue(Files.isDirectory(files.get(0)));
+
+ List<Path> subfolderFiles = list(files.get(0));
+ assertEquals(1, subfolderFiles.size());
+ assertEquals("1.0.txt", filename(subfolderFiles.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());
Review Comment:
💡 **Test coverage gap: this test doesn't exercise the Windows-specific
failure mode.** On Linux, `FileSystems.getDefault().getSeparator()` returns
`/`, which is also the path delimiter — but since `/` doesn't appear in the
filter expression `${pom.version}`, the escape string has no observable effect
here. The test passes on Linux for the same reason the unfixed code would have:
the escape string never collides with anything in the filename component.
The actual bug (issue #289) is that on Windows, the escape string `\`
appears inside `${pom.version}` expressions and causes premature termination of
placeholder resolution. To truly guard against regression, the test should set
a hardcoded `"\\"` escape string and verify the property still expands:
```java
mavenResourcesExecution.setEscapeString("\\\\"); // simulate Windows
escape character
```
Or add an `@EnabledOnOs(OS.WINDOWS)` sibling test. As-is this test provides
confidence on Linux but leaves the Windows path untested in CI.
--
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]
