gnodet-bot commented on code in PR #406:
URL: https://github.com/apache/maven-filtering/pull/406#discussion_r4102943850
##########
src/test/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFilteringTest.java:
##########
@@ -1041,6 +1041,47 @@ void filterPropertiesFiles() throws Exception {
assertTrue(contentEquals(Paths.get(unitFilesDir, "test.txt"),
targetPathFile.resolve("test.txt")));
}
+ /**
+ * MRESOURCES-281: Allow per-resource encoding override via {@link
Resource#setEncoding(String)}.
+ */
+ @Test
+ void filterResourcesWithPerResourceEncoding() throws Exception {
+ String unitFilesDir = getBasedir() +
"/src/test/units-files/resource-encoding";
+
+ // Resource 1: read and copy the UTF-8 encoded file
+ Resource utf8Resource = new Resource();
+ utf8Resource.setDirectory(unitFilesDir);
+ utf8Resource.setFiltering(false);
Review Comment:
⚠️ **Test doesn't validate encoding.** `filtering=false` means
`FilteringUtils.copyFile` takes the `wrappers == null || wrappers.length == 0`
branch (line 301), which does `Files.copy(from, os)` — a **binary copy that
ignores the encoding parameter entirely**.
This test would pass identically with or without the `setEncoding()` call,
and with any encoding value, because the encoding is never used in the
non-filtering copy path.
To actually validate the feature, you need `setFiltering(true)` with a
filter wrapper that exercises the read→decode→filter→encode→write pipeline
(lines 306-320 of `FilteringUtils`). For example, a resource with
`filtering=true` and a property placeholder like `${project.name}` in the test
file, combined with a per-resource encoding, would prove that the encoding is
correctly propagated through the filtering path.
Alternatively, a focused unit test on the encoding selection logic in
`DefaultMavenResourcesFiltering` (lines 278-284) that asserts
`resource.getEncoding()` takes precedence over `getEncoding(source, global,
propertiesEncoding)` would at least cover the branching logic.
##########
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##########
@@ -273,9 +273,15 @@ public void filterResources(MavenResourcesExecution
mavenResourcesExecution) thr
propertiesFiles.add(source);
}
- // Determine which encoding to use when filtering this file
- String encoding = getEncoding(
- source, mavenResourcesExecution.getEncoding(),
mavenResourcesExecution.getPropertiesEncoding());
+ // Determine which encoding to use when filtering this file.
+ // A per-resource encoding overrides the global encoding and
propertiesEncoding.
+ String resourceEncoding = resource.getEncoding();
+ String encoding = (resourceEncoding != null)
+ ? resourceEncoding
+ : getEncoding(
+ source,
+ mavenResourcesExecution.getEncoding(),
+
mavenResourcesExecution.getPropertiesEncoding());
Review Comment:
💡 **propertiesEncoding bypass is implicit.** When `resource.getEncoding()`
is set, this skips the `getEncoding(source, global, propertiesEncoding)` call
entirely — meaning `.properties` files in this resource set will use the
resource-level encoding instead of `propertiesEncoding`.
This is probably the right semantic (explicit per-resource encoding should
win), but it's worth a one-line comment here documenting that the bypass of
`propertiesEncoding` is intentional, not accidental. Something like:
```
// Per-resource encoding, when set, takes precedence over both the global
// encoding and the file-type-specific propertiesEncoding.
```
The existing comment on line 277 says "overrides the global encoding and
propertiesEncoding" which covers it, but someone reading the code months later
might wonder if the propertiesEncoding bypass was an oversight.
--
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]
