You are viewing a plain text version of this content. The canonical link for it is here.

Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/09/25 09:00:30 UTC

[PR] Add per-resource encoding support (MRESOURCES-281) [maven-filtering]

gnodet opened a new pull request, #406:
URL: https://github.com/apache/maven-filtering/pull/406

   ## Summary
   
   Allow users to specify a character encoding directly on a `<resource>` element in the plugin configuration. When set, the resource-level encoding takes priority over the global `encoding` parameter and `propertiesEncoding`, enabling different resource sets within the same execution to be processed with different encodings.
   
   ## Motivation
   
   Fixes: https://github.com/apache/maven-resources-plugin/issues/125 (MRESOURCES-281)
   
   Mixed-encoding projects (e.g. legacy projects with `windows-1252` as the global encoding but with `*.xml`/`*.json` files that must be `UTF-8`) had no clean solution. The workaround was multiple plugin executions or separate source directories, both of which are cumbersome.
   
   ## Solution
   
   Add an `encoding` field to `Resource` in maven-filtering. The implementation in `DefaultMavenResourcesFiltering` checks for a resource-level encoding first; if absent, falls back to the existing `propertiesEncoding`/`encoding` logic unchanged.
   
   ## Usage
   
   ```xml
   <configuration>
     <resources>
       <resource>
         <directory>src/main/resources</directory>
         <encoding>UTF-8</encoding>
         <includes><include>**/*.xml</include></includes>
       </resource>
       <resource>
         <directory>src/main/resources</directory>
         <encoding>windows-1252</encoding>
         <excludes><exclude>**/*.xml</exclude></excludes>
       </resource>
     </resources>
   </configuration>
   ```
   
   ## Changes
   
   - `Resource.java`: add `encoding` field with getter/setter (`@since 3.4.0`)
   - `DefaultMavenResourcesFiltering.java`: check `resource.getEncoding()` before falling back to global encoding
   - New test `filterResourcesWithPerResourceEncoding()` with real UTF-8 and windows-1252 encoded test files


-- 
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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Add per-resource encoding support (MRESOURCES-281) [maven-filtering]

Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.

gnodet-bot commented on code in PR #406:
URL: https://github.com/apache/maven-filtering/pull/406#discussion_r4105933569


##########
src/main/java/org/apache/maven/shared/filtering/FilteringUtils.java:
##########
@@ -292,25 +292,115 @@ static boolean isEmpty(final String string) {
      *
      * @param from the file to copy
      * @param to the destination file
-     * @param encoding the file output encoding (only if wrappers is not empty)
+     * @param encoding the encoding used for both reading and writing (only if wrappers is not empty)
      * @param wrappers array of {@link FilterWrapper}
+     * @param overwrite if {@code true}, uses {@link ChangeDetection#ALWAYS}; if {@code false}, uses {@link ChangeDetection#CONTENT}
      * @throws IOException if an IO error occurs during copying or filtering
+     * @deprecated use {@link #copyFile(Path, Path, String, FilterWrapper[], ChangeDetection)} instead
      */
+    @Deprecated
     public static void copyFile(Path from, Path to, String encoding, FilterWrapper[] wrappers, boolean overwrite)
             throws IOException {
+        copyFile(from, to, encoding, wrappers, overwrite ? ChangeDetection.ALWAYS : ChangeDetection.CONTENT);
+    }
+
+    /**
+     * Copies a file from the source to the destination, applying the specified filters if provided.
+     *
+     * @param from the file to copy
+     * @param to the destination file
+     * @param encoding the file output encoding (only if wrappers is not empty)
+     * @param wrappers array of {@link FilterWrapper}
+     * @param changeDetection the strategy to apply if to is existing file
+     * @return {@code true} if the file was copied.
+     * @throws IOException if an IO error occurs during copying or filtering
+     * @since 4.0.0-beta-2
+     */
+    public static boolean copyFile(
+            Path from, Path to, String encoding, FilterWrapper[] wrappers, ChangeDetection changeDetection)
+            throws IOException {
+        return copyFile(from, to, encoding, encoding, wrappers, changeDetection);
+    }
+
+    /**
+     * Copy and optionally filter a file using separate input and output encodings. This allows
+     * encoding conversion during resource filtering (e.g. reading ISO-8859-1 and writing UTF-8).
+     * <b>If wrappers is null or empty, the file is copied as raw bytes — encoding parameters are
+     * ignored in that case.</b>
+     *
+     * @param from the file to copy
+     * @param to the destination file
+     * @param inputEncoding the charset used to read {@code from} (only when wrappers is not empty)
+     * @param outputEncoding the charset used to write {@code to} (only when wrappers is not empty)
+     * @param wrappers array of {@link FilterWrapper}
+     * @param changeDetection the strategy to apply if to is existing file
+     * @return {@code true} if the file was copied.
+     * @throws IOException if an IO error occurs during copying or filtering
+     * @since 4.0.0-beta-3
+     */
+    public static boolean copyFile(
+            Path from,
+            Path to,
+            String inputEncoding,
+            String outputEncoding,
+            FilterWrapper[] wrappers,
+            ChangeDetection changeDetection)
+            throws IOException {
+        boolean needsCopy = false;
+        boolean unconditionally = false;
+        switch (changeDetection) {
+            case NEVER:
+                needsCopy = !Files.isRegularFile(to);
+                unconditionally = true;
+                break;
+            case ALWAYS:
+                needsCopy = true;
+                unconditionally = true;
+                break;
+            case TIMESTAMP:
+                needsCopy = !Files.exists(to)
+                        || Files.getLastModifiedTime(to).toMillis()
+                                < Files.getLastModifiedTime(from).toMillis();
+                unconditionally = true;
+                break;
+            case CONTENT:
+                needsCopy = true;
+                break;
+            case TIMESTAMP_AND_CONTENT:
+                needsCopy = !Files.exists(to)
+                        || Files.getLastModifiedTime(to).toMillis()
+                                < Files.getLastModifiedTime(from).toMillis();
+                break;
+            default:
+                throw new IllegalArgumentException("Unsupported change detection mode: " + changeDetection);
+        }
+        boolean copied = false;
+        if (needsCopy) {
+            if (unconditionally) {
+                copied = copyUnconditionally(from, to, inputEncoding, outputEncoding, wrappers);
+            } else {
+                copied = copyIfContentsChanged(from, to, inputEncoding, outputEncoding, wrappers);
+            }
+        }
+        return copied;
+    }

Review Comment:
   ⚠️ **Missing test coverage.** This 5-branch switch statement (NEVER, ALWAYS, TIMESTAMP, CONTENT, TIMESTAMP_AND_CONTENT) controls whether files get copied or skipped during resource filtering — a core build behavior. None of the non-default branches have test coverage:
   
   - `TIMESTAMP` — timestamp comparison logic untested (what if source and dest have equal timestamps?)
   - `NEVER` — "never overwrite" semantics untested (does it correctly create new files but skip existing?)
   - `ALWAYS` — unconditional overwrite untested
   - `TIMESTAMP_AND_CONTENT` — combined strategy untested
   
   Only `CONTENT` is implicitly covered by existing tests. At minimum, a focused unit test on `FilteringUtils.copyFile()` that exercises each mode with an existing/non-existing destination file would catch regressions in the timestamp comparison and the needsCopy/unconditionally logic.



##########
src/main/java/org/apache/maven/shared/filtering/DefaultMavenResourcesFiltering.java:
##########
@@ -252,13 +252,17 @@ public void filterResources(MavenResourcesExecution mavenResourcesExecution) thr
 
             for (String name : includedFiles) {
 
-                LOGGER.debug("Copying file " + name);
                 Path source = resourceDirectory.resolve(name);
 
                 Path destinationFile = getDestinationFile(outputDirectory, targetPath, name, mavenResourcesExecution);
 
+                // Per-resource changeDetection overrides the request-level setting
+                ChangeDetection changeDetection = resource.getChangeDetection() != null
+                        ? resource.getChangeDetection()
+                        : mavenResourcesExecution.getChangeDetection();
+
                 if (mavenResourcesExecution.isFlatten() && Files.exists(destinationFile)) {
-                    if (mavenResourcesExecution.isOverwrite()) {
+                    if (changeDetection == ChangeDetection.ALWAYS) {

Review Comment:
   💡 **Stale error message.** The `else` branch on line 270 still says `"overwrite was not set to true"`. Since `overwrite` was replaced by `ChangeDetection`, this should reference `changeDetection` instead. Something like:
   
   ```suggestion
                       if (changeDetection == ChangeDetection.ALWAYS) {
                           LOGGER.warn(
                                   "existing file " + destinationFile.getFileName() + " will be overwritten by " + name);
                       } else {
                           throw new MavenFilteringException("existing file " + destinationFile.getFileName()
                                   + " will be overwritten by " + name
                                   + " and changeDetection is not set to ALWAYS");
   ```
   
   (Can't put the suggestion on the exact line since it's outside the diff hunk, but line 270 reads: `+ " and overwrite was not set to true"`)



-- 
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: issues-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org