[IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES (#377)

* Initial commit: Don't copy all attributes, only timestamp information

* Fix docs, formatting, add unit tests

* Format: "catch(" -> "catch ("

---------

Co-authored-by: Gary Gregory <garydgregory@users.noreply.github.com>

Author: tresf

src/main/java/org/apache/commons/io/FileUtils.java

@@ -43,6 +43,8 @@
 import java.nio.file.NotDirectoryException;
 import java.nio.file.Path;
 import java.nio.file.StandardCopyOption;
+import java.nio.file.attribute.BasicFileAttributeView;
+import java.nio.file.attribute.BasicFileAttributes;
 import java.nio.file.attribute.FileTime;
 import java.time.Duration;
 import java.time.Instant;
@@ -54,7 +56,6 @@
 import java.time.chrono.ChronoLocalDateTime;
 import java.time.chrono.ChronoZonedDateTime;
 import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Date;
@@ -199,23 +200,6 @@ public class FileUtils {
      */
     public static final File[] EMPTY_FILE_ARRAY = {};
 
-    /**
-     * Copies the given array and adds StandardCopyOption.COPY_ATTRIBUTES.
-     *
-     * @param copyOptions sorted copy options.
-     * @return a new array.
-     */
-    private static CopyOption[] addCopyAttributes(final CopyOption... copyOptions) {
-        // Make a copy first since we don't want to sort the call site's version.
-        final CopyOption[] actual = Arrays.copyOf(copyOptions, copyOptions.length + 1);
-        Arrays.sort(actual, 0, copyOptions.length);
-        if (Arrays.binarySearch(copyOptions, 0, copyOptions.length, StandardCopyOption.COPY_ATTRIBUTES) >= 0) {
-            return copyOptions;
-        }
-        actual[actual.length - 1] = StandardCopyOption.COPY_ATTRIBUTES;
-        return actual;
-    }
-
     /**
      * Returns a human-readable version of the file size, where the input represents a specific number of bytes.
      * <p>
@@ -489,9 +473,10 @@ public static File[] convertFileCollectionToFileArray(final Collection<File> fil
      * method merges the source with the destination, with the source taking precedence.
      * </p>
      * <p>
-     * <strong>Note:</strong> This method tries to preserve the files' last modified date/times using
-     * {@link File#setLastModified(long)}, however it is not guaranteed that those operations will succeed. If the
-     * modification operation fails, the methods throws IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param srcDir an existing directory to copy, must not be {@code null}.
@@ -594,9 +579,10 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
      * method merges the source with the destination, with the source taking precedence.
      * </p>
      * <p>
-     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the files' last
-     * modified date/times using {@link File#setLastModified(long)}, however it is not guaranteed that those operations
-     * will succeed. If the modification operation fails, the methods throws IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      * <b>Example: Copy directories only</b>
      *
@@ -643,9 +629,10 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
      * method merges the source with the destination, with the source taking precedence.
      * </p>
      * <p>
-     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the files' last
-     * modified date/times using {@link File#setLastModified(long)}, however it is not guaranteed that those operations
-     * will succeed. If the modification operation fails, the methods throws IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      * <b>Example: Copy directories only</b>
      *
@@ -698,7 +685,7 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
                 }
             }
         }
-        doCopyDirectory(srcDir, destDir, fileFilter, exclusionList, preserveFileDate, preserveFileDate ? addCopyAttributes(copyOptions) : copyOptions);
+        doCopyDirectory(srcDir, destDir, fileFilter, exclusionList, preserveFileDate, copyOptions);
     }
 
     /**
@@ -712,9 +699,10 @@ public static void copyDirectory(final File srcDir, final File destDir, final Fi
      * method merges the source with the destination, with the source taking precedence.
      * </p>
      * <p>
-     * <strong>Note:</strong> This method tries to preserve the files' last modified date/times using
-     * {@link File#setLastModified(long)}, however it is not guaranteed that those operations will succeed. If the
-     * modification operation fails, the methods throws IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param sourceDir an existing directory to copy, must not be {@code null}.
@@ -740,8 +728,9 @@ public static void copyDirectoryToDirectory(final File sourceDir, final File des
      * </p>
      * <p>
      * <strong>Note:</strong> This method tries to preserve the file's last modified date/times using
-     * {@link StandardCopyOption#COPY_ATTRIBUTES}, however it is not guaranteed that the operation will succeed. If the
-     * modification operation fails, the methods throws IOException.
+     * {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is not guaranteed that the
+     * operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param srcFile an existing file to copy, must not be {@code null}.
@@ -754,7 +743,7 @@ public static void copyDirectoryToDirectory(final File sourceDir, final File des
      * @see #copyFile(File, File, boolean)
      */
     public static void copyFile(final File srcFile, final File destFile) throws IOException {
-        copyFile(srcFile, destFile, StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING);
+        copyFile(srcFile, destFile, StandardCopyOption.REPLACE_EXISTING);
     }
 
     /**
@@ -766,8 +755,9 @@ public static void copyFile(final File srcFile, final File destFile) throws IOEx
      * </p>
      * <p>
      * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
-     * modified date/times using {@link StandardCopyOption#COPY_ATTRIBUTES}, however it is not guaranteed that the operation
-     * will succeed. If the modification operation fails, the methods throws IOException.
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param srcFile an existing file to copy, must not be {@code null}.
@@ -781,9 +771,7 @@ public static void copyFile(final File srcFile, final File destFile) throws IOEx
      */
     public static void copyFile(final File srcFile, final File destFile, final boolean preserveFileDate) throws IOException {
         // @formatter:off
-        copyFile(srcFile, destFile, preserveFileDate
-                ? new CopyOption[] {StandardCopyOption.COPY_ATTRIBUTES, StandardCopyOption.REPLACE_EXISTING}
-                : new CopyOption[] {StandardCopyOption.REPLACE_EXISTING});
+        copyFile(srcFile, destFile, preserveFileDate, new CopyOption[] {StandardCopyOption.REPLACE_EXISTING});
         // @formatter:on
     }
 
@@ -796,8 +784,9 @@ public static void copyFile(final File srcFile, final File destFile, final boole
      * </p>
      * <p>
      * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
-     * modified date/times using {@link StandardCopyOption#COPY_ATTRIBUTES}, however it is not guaranteed that the operation
-     * will succeed. If the modification operation fails, the methods throws IOException.
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param srcFile an existing file to copy, must not be {@code null}.
@@ -813,7 +802,20 @@ public static void copyFile(final File srcFile, final File destFile, final boole
      * @since 2.8.0
      */
     public static void copyFile(final File srcFile, final File destFile, final boolean preserveFileDate, final CopyOption... copyOptions) throws IOException {
-        copyFile(srcFile, destFile, preserveFileDate ? addCopyAttributes(copyOptions) : copyOptions);
+        requireFileCopy(srcFile, destFile);
+        requireFile(srcFile, "srcFile");
+        requireCanonicalPathsNotEquals(srcFile, destFile);
+        createParentDirectories(destFile);
+        requireFileIfExists(destFile, "destFile");
+        if (destFile.exists()) {
+            requireCanWrite(destFile, "destFile");
+        }
+        Files.copy(srcFile.toPath(), destFile.toPath(), copyOptions);
+
+        // On Windows, the last modified time is copied by default.
+        if(preserveFileDate) {
+            setTimes(srcFile, destFile);
+        }
     }
 
     /**
@@ -835,16 +837,7 @@ public static void copyFile(final File srcFile, final File destFile, final boole
      * @since 2.9.0
      */
     public static void copyFile(final File srcFile, final File destFile, final CopyOption... copyOptions) throws IOException {
-        requireFileCopy(srcFile, destFile);
-        requireFile(srcFile, "srcFile");
-        requireCanonicalPathsNotEquals(srcFile, destFile);
-        createParentDirectories(destFile);
-        requireFileIfExists(destFile, "destFile");
-        if (destFile.exists()) {
-            requireCanWrite(destFile, "destFile");
-        }
-        // On Windows, the last modified time is copied by default.
-        Files.copy(srcFile.toPath(), destFile.toPath(), copyOptions);
+        copyFile(srcFile, destFile, true, copyOptions);
     }
 
     /**
@@ -876,8 +869,9 @@ public static long copyFile(final File input, final OutputStream output) throws
      * </p>
      * <p>
      * <strong>Note:</strong> This method tries to preserve the file's last modified date/times using
-     * {@link StandardCopyOption#COPY_ATTRIBUTES}, however it is not guaranteed that the operation will succeed. If the
-     * modification operation fails, the methods throws IOException.
+     * {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is not guaranteed that the
+     * operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param srcFile an existing file to copy, must not be {@code null}.
@@ -900,8 +894,9 @@ public static void copyFileToDirectory(final File srcFile, final File destDir) t
      * </p>
      * <p>
      * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
-     * modified date/times using {@link StandardCopyOption#COPY_ATTRIBUTES}, however it is not guaranteed that the operation
-     * will succeed. If the modification operation fails, the methods throws IOException.
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param sourceFile an existing file to copy, must not be {@code null}.
@@ -957,10 +952,10 @@ public static void copyInputStreamToFile(final InputStream source, final File de
      * merges the source with the destination, with the source taking precedence.
      * </p>
      * <p>
-     * <strong>Note:</strong> This method tries to preserve the files' last modified date/times using
-     * {@link StandardCopyOption#COPY_ATTRIBUTES} or {@link File#setLastModified(long)} depending on the input, however it
-     * is not guaranteed that those operations will succeed. If the modification operation fails, the methods throws
-     * IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param sourceFile an existing file or directory to copy, must not be {@code null}.
@@ -993,10 +988,10 @@ public static void copyToDirectory(final File sourceFile, final File destination
      * If the destination file exists, then this method will overwrite it.
      * </p>
      * <p>
-     * <strong>Note:</strong> This method tries to preserve the file's last
-     * modified date/times using {@link File#setLastModified(long)}, however
-     * it is not guaranteed that the operation will succeed.
-     * If the modification operation fails, the methods throws IOException.
+     * <strong>Note:</strong> Setting {@code preserveFileDate} to {@code true} tries to preserve the file's last
+     * modified date/times using {@link BasicFileAttributeView#setTimes(FileTime, FileTime, FileTime)}, however it is
+     * not guaranteed that the operation will succeed. If the modification operation fails it will fallback to
+     * {@link File#setLastModified(long)} and if that fails, the methods throws IOException.
      * </p>
      *
      * @param sourceIterable  existing files to copy, must not be {@code null}.
@@ -1310,13 +1305,13 @@ private static void doCopyDirectory(final File srcDir, final File destDir, final
                 if (srcFile.isDirectory()) {
                     doCopyDirectory(srcFile, dstFile, fileFilter, exclusionList, preserveDirDate, copyOptions);
                 } else {
-                    copyFile(srcFile, dstFile, copyOptions);
+                    copyFile(srcFile, dstFile, preserveDirDate, copyOptions);
                 }
             }
         }
         // Do this last, as the above has probably affected directory metadata
         if (preserveDirDate) {
-            setLastModified(srcDir, destDir);
+            setTimes(srcDir, destDir);
         }
     }
 
@@ -2386,7 +2381,8 @@ public static void moveFile(final File srcFile, final File destFile, final CopyO
         requireAbsent(destFile, "destFile");
         final boolean rename = srcFile.renameTo(destFile);
         if (!rename) {
-            copyFile(srcFile, destFile, copyOptions);
+            // Don't interfere with file date on move, handled by StandardCopyOption.COPY_ATTRIBUTES
+            copyFile(srcFile, destFile, false, copyOptions);
             if (!srcFile.delete()) {
                 FileUtils.deleteQuietly(destFile);
                 throw new IOException("Failed to delete original file '" + srcFile + "' after copy to '" + destFile + "'");
@@ -2836,37 +2832,30 @@ private static File requireFileIfExists(final File file, final String name) {
     }
 
     /**
-     * Sets the given {@code targetFile}'s last modified date to the value from {@code sourceFile}.
+     * Set file lastModifiedTime, lastAccessTime and creationTime to match source file
      *
      * @param sourceFile The source file to query.
      * @param targetFile The target file or directory to set.
      * @throws NullPointerException if sourceFile is {@code null}.
      * @throws NullPointerException if targetFile is {@code null}.
      * @throws IOException if setting the last-modified time failed.
      */
-    private static void setLastModified(final File sourceFile, final File targetFile) throws IOException {
+    private static void setTimes(final File sourceFile, final File targetFile) throws IOException {
         Objects.requireNonNull(sourceFile, "sourceFile");
         Objects.requireNonNull(targetFile, "targetFile");
-        if (targetFile.isFile()) {
-            PathUtils.setLastModifiedTime(targetFile.toPath(), sourceFile.toPath());
-        } else {
-            setLastModified(targetFile, lastModified(sourceFile));
-        }
-    }
-
-    /**
-     * Sets the given {@code targetFile}'s last modified date to the given value.
-     *
-     * @param file The source file to query.
-     * @param timeMillis The new last-modified time, measured in milliseconds since the epoch 01-01-1970 GMT.
-     * @throws NullPointerException if file is {@code null}.
-     * @throws IOException if setting the last-modified time failed.
-     */
-    private static void setLastModified(final File file, final long timeMillis) throws IOException {
-        Objects.requireNonNull(file, "file");
-        if (!file.setLastModified(timeMillis)) {
-            throw new IOException(String.format("Failed setLastModified(%s) on '%s'", timeMillis, file));
-        }
+        try {
+            // Set creation, modified, last accessed to match source file
+            final BasicFileAttributes srcAttr = Files.readAttributes(sourceFile.toPath(), BasicFileAttributes.class);
+            final BasicFileAttributeView destAttrView = Files.getFileAttributeView(targetFile.toPath(), BasicFileAttributeView.class);
+            // null guards are not needed; BasicFileAttributes.setTimes(...) is null safe
+            destAttrView.setTimes(srcAttr.lastModifiedTime(), srcAttr.lastAccessTime(), srcAttr.creationTime());
+        } catch (IOException unused) {
+            // Fallback: Only set modified time to match source file
+            targetFile.setLastModified(sourceFile.lastModified());
+        }
+
+        // TODO: (Help!) Determine historically why setLastModified(File, File) needed PathUtils.setLastModifiedTime() if
+        //  sourceFile.isFile() was true, but needed setLastModifiedTime(File, long) if sourceFile.isFile() was false
     }
 
     /**