gnodet-bot commented on code in PR #290:
URL: https://github.com/apache/maven-filtering/pull/290#discussion_r4093473665
##########
src/test/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFilteringTest.java:
##########
@@ -943,7 +943,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)));
Review Comment:
⚠️ **Test coverage gap: the flat-file case is no longer tested.** Moving the
fixture to `subfolder/` converted `testFilterFileName` into a subdirectory
test. The original scenario — a token in a flat filename (`${pom.version}.txt`
at the resource root, no path component boundary involved) — is no longer
exercised. Since `filterFileName()` now processes each path component
separately, a regression on the simple single-component case would not be
caught.
Add a second fixture (e.g. `maven-filename-filtering/${pom.artifactId}.txt`)
and assert it is correctly renamed, or re-add a flat file alongside
`subfolder/` and assert both are handled.
##########
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##########
@@ -476,26 +478,39 @@ 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);
}
- String filteredFilename = writer.toString();
+ try (Reader closeable = reader;
+ 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);
+ }
+ String filteredComponent = writer.toString();
+ sb.append(filteredComponent);
+ if (iterator.hasNext()) {
+ sb.append(File.separator);
+ }
- if (LOGGER.isDebugEnabled()) {
- LOGGER.debug("renaming filename " + name + " to " +
filteredFilename);
+ } catch (IOException e) {
+ throw new MavenFilteringException("Failed filtering filename"
+ name, e);
Review Comment:
💡 **Nit: missing separator in error message.** `"Failed filtering filename"
+ name` concatenates without a space or colon, producing messages like `"Failed
filtering filenamefoo/bar.txt"`. This typo was in the original code and carried
over here; since this line is in the diff it's worth fixing now.
```suggestion
throw new MavenFilteringException("Failed filtering
filename: " + name, e);
```
--
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]
