IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES

[tresf]

Since [email protected], the copyDirectory() function uses COPY_ATTRIBUTES to honor the preserveFileDate boolean.

This properly preserves the date, but introduces an inadvertent effect on file permissions.

For example, if copying from %USERPROFILE% to C:\Program Files the following permissions changes can be observed.

Version	Permissions (icals <file>)
[email protected]	NT AUTHORITY\SYSTEM:(I)(F) BUILTIN\Administrators:(I)(F) BUILTIN\Users:(I)(RX) APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES:(I)(RX) APPLICATION PACKAGE AUTHORITY\ALL RESTRICTED APPLICATION PACKAGES:(I)(RX)
[email protected]	NT AUTHORITY\SYSTEM:(F) BUILTIN\Administrators:(F) WIN10ARM\owner:(F)

• This regression is very similar for Unixes. Prior to 2.9, files copied to a root:root (Linux) or root:admin (MacOS) owned directory would inherit the target permissions. Starting with 2.9, the files will retain their original ownership.
• The OLD behavior is a closer match to the OS's default copy behavior (Linux, Mac and Windows will use the permissions of the target directory when copying).

The regression can be traced to to the 2.9 release and all releases thereafter.

Prior to submitting the PR, a conversation was had on the mailing list and stackoverflow.

• Mailing list convo: https://lists.apache.org/thread/schfj3m01ppd3287mxnn04lpp25hgw74
• Stackoverflow convo: https://stackoverflow.com/questions/73428286

Version	Status
[email protected] (2005)	:white_check_mark: Inherits permissions of destination
[email protected] (2013)	:white_check_mark: Inherits permissions of destination
[email protected] (2016)	:white_check_mark: Inherits permissions of destination
[email protected] (2017)	:white_check_mark: Inherits permissions of destination
[email protected] (2020)	:white_check_mark: Inherits permissions of destination
[email protected] (2020)	:white_check_mark: Inherits permissions of destination
[email protected] (2021)	:no_entry_sign: Does not inherit permissions of destination
[email protected] (2021)	:no_entry_sign: Does not inherit permissions of destination
[email protected] (2021)	:no_entry_sign: Does not inherit permissions of destination

This PR removes the COPY_ATTRIBUTES flag unless a file is being moved (to match the OS default move behavior), and instead uses NIO calls to preserve the timestamp information.

TODO:

• [ ] :speech_balloon: Discussion: With this PR, the solution uses BasicFileAttributeView.setTimes(FileTime, FileTime, FileTime) to set ALL timestamp information. This differs slightly from pre-2.9 versions. According to the JavaDocs, this may actually set file stamps to beginning of epoch, so this may be undesired. If there's an IOException, it will fallback to the older File.setLastModified(long) API, but it could be argued that this should remain the only API for maximum backwards compatibility.
• [ ] :speech_balloon: Discussion: Why did the previous setLastModified(File, File) need PathUtils.setLastModifiedTime() if sourceFile.isFile() was true, but needed setLastModifiedTime(File, long) if sourceFile.isFile() was false. This is the code I'm inquiring about which will be lost with this PR, so I'd like to understand the history of this to ensure we aren't introducing a regression with the new change.
• [ ] :hourglass_flowing_sand: Action: Hand-test this change and impacting APIs on various platforms and compare behavior to pre-2.9 versions.

[codecov-commenter]

Codecov Report

Merging #377 (081e43c) into master (44a2185) will increase coverage by 0.03%. The diff coverage is 91.30%.

@@             Coverage Diff              @@
##             master     #377      +/-   ##
============================================
+ Coverage     85.58%   85.61%   +0.03%     
+ Complexity     3099     3095       -4     
============================================
  Files           204      204              
  Lines          7365     7360       -5     
  Branches        904      901       -3     
============================================
- Hits           6303     6301       -2     
  Misses          813      813              
+ Partials        249      246       -3     

Impacted Files	Coverage Δ
src/main/java/org/apache/commons/io/FileUtils.java	94.91% <91.30%> (+0.49%)	:arrow_up:

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[garydgregory]

See also:

• https://issues.apache.org/jira/browse/IO-769 This issue
• https://issues.apache.org/jira/browse/IO-678 FileUtils.copyFile does not maintain file permissions, fixed in 2.9.0.
• https://issues.apache.org/jira/browse/IO-663 FileUtils.copyDirectory(File srcDir, File destDir) fails on Windows
• https://issues.apache.org/jira/browse/IO-575 copyDirectory (all overloads) does not maintain file permissions

[garydgregory]

Hi @tresf Thank you for taking the time and putting in the effort to create this PR as well as explaining the issue clearly here and on the development mailing list.

I'd like feedback from the community. @kinow ? @chtompki ?

[garydgregory]

Notice that in the (old) Jira tickets above, two are about attributes not being copied, which we now do but this PR proposes to undo. So catch-22. Ideas?

[tresf]

Notice that in the (old) Jira tickets above, two are about attributes not being copied, which we now do but this PR proposes to undo. So catch-22. Ideas?

The API already offers COPY_ATTRIBUTES in several APIs as CopyOptions parameters. Note, the current API doesn't (at least not obviously) allow the removal of the assumed COPY_ATTRIBUTES and if you look at the history of the StackOverflow and mailing list posts, this is often the first instinct, e.g:

1. User doesn't get desired copy behavior
2. Community recommends adjusting CopyOptions

... however the current behavior doesn't give this control to the user, despite it being the first instinct by the community, quoting:

Gary wrote: Use org.apache.commons.io.FileUtils.copyFile(File, File, boolean, CopyOption...) to exercise full control over what you want to do. Under the covers it ends up calling java.nio.file.Files.copy(Path, Path, CopyOption...). [read full version in context]

So if this regression is not reverted, I'd at least advocate for an API which would make the above statement correct. The current behavior silently adds COPY_ATTRIBUTES, which removes the ability to control this flag.

[kinow]

Notice that in the (old) Jira tickets above, two are about attributes not being copied, which we now do but this PR proposes to undo. So catch-22. Ideas?

I think we should start by reverting the change from 2.9 with this PR, and prepare a new release. I was only suggesting that someone looks at the other JIRA issues to close if it is fixed by this PR, or leave it open for the next minor or major release, as needed.

So if this regression is not reverted, I'd at least advocate for an API which would make the above statement correct. The current behavior silently adds COPY_ATTRIBUTES, which removes the ability to control this flag.

That sounds like a compromise. Probably one of the two would then have to be marked as deprecated. I don't have a large-enough Java code base that I am maintaining a the moment to measure the impact, so take my comments with a pinch of salt :slightly_smiling_face: No hard feelings if other options are chosen here :+1:

[garydgregory]

Hi @tresf, Please rebase on git master to see what happens with the new test for IO-575.

[tresf]

Hi @tresf, Please rebase on git master to see what happens with the new test for IO-575.

Done, I think you need to kick-start the workflow now. :)

[menscikov]

Hello, FileUtils.copyInputStreamToFile() has the same issue starting from 2.9 version. Please fix it also.

[Schmidor]

FileUtils.moveFile(File srcFile, File destFile) was also changed and got COPY_ATTRIBUTES in 2.9

[garydgregory]

What is the status of this PR?

[garydgregory]

@tresf I plan on releasing soon, so if we can get more eyes here and a rebase, that would be great.

[kinow]

What is the status of this PR?

I haven't seen any new commits since my last review, so I take it if this is to be merged, it'd revert the COPY_ATTRIBUTES change. I think the last two comments reporting regressions in 2.9 now make it more complicated. If we are to revert copyDirectory, and it's confirmed copyInputStreamToFile & moveFile were also affected in 2.9, then I think it would be expected - by precedent - to revert those two too?

[garydgregory]

Why did the previous setLastModified(File, File) need PathUtils.setLastModifiedTime() if sourceFile.isFile() was true, but needed setLastModifiedTime(File, long) if sourceFile.isFile() was false. This is the code I'm inquiring about which will be lost with this PR, so I'd like to understand the history of this to ensure we aren't introducing a regression with the new change.

This might have to do with different APIs [not] being able to set directory timestamps.

[garydgregory]

The consensus, reading this PR and the ticket, is to revert the behavior, so I will merge this PR.

[menscikov]

Hello, FileUtils.copyInputStreamToFile() still has the same issue in the 2.13.0 version starting from 2.8 version. I'm testing on Android API 23 on Samsung mobile phone. Version 2.8 is working fine.

The worst thing that there is no crash or error, it just hangs and i have infinite copying on version starting from 2.9.