[IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES by tresf · Pull Request #377 · apache/commons-io

tresf · 2022-08-29T14:36:18Z

Since commons-io@2.9, 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>`)
`commons-io@2.8`	`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)`
`commons-io@2.9`	`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
`commons-io@1.1` (2005)	✅ Inherits permissions of destination
`commons-io@2.4` (2013)	✅ Inherits permissions of destination
`commons-io@2.5` (2016)	✅ Inherits permissions of destination
`commons-io@2.6` (2017)	✅ Inherits permissions of destination
`commons-io@2.7` (2020)	✅ Inherits permissions of destination
`commons-io@2.8` (2020)	✅ Inherits permissions of destination
`commons-io@2.9` (2021)	🚫 Does not inherit permissions of destination
`commons-io@2.10` (2021)	🚫 Does not inherit permissions of destination
`commons-io@2.11` (2021)	🚫 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:

💬 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.
💬 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.
⏳ Action: Hand-test this change and impacting APIs on various platforms and compare behavior to pre-2.9 versions.

codecov-commenter · 2022-08-29T15:18:44Z

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%)`	⬆️

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

garydgregory · 2022-08-29T18:41:05Z

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 · 2022-09-01T11:24:59Z

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 ?

kinow

Echoing @garydgregory 's words to @tresf, thanks for taking your time to prepare the PR, and for the great work.

The code looks good to me, and the rationale as well. I think @tresf 's checklist could have an extra item to verify which of the JIRA issue @garydgregory linked could be fixed by this change. I had a read of the issues, and it looks like at least a couple could be fixed by this change (or at least have a different result regarding permissions).

Cheers
Bruno

kinow · 2022-09-02T03:16:49Z

+        }
+
+        // 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


Thanks for adding the note. No idea why that changed, didn't have time to dig into git's history, sorry.

garydgregory · 2022-09-03T13:56:05Z

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 · 2022-09-04T16:23:24Z

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:

User doesn't get desired copy behavior
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 · 2022-09-05T10:22:28Z

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 🙂 No hard feelings if other options are chosen here 👍

garydgregory · 2022-09-06T16:12:33Z

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

tresf · 2022-09-06T16:27:02Z

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 · 2023-01-15T15:03:30Z

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

Schmidor · 2023-01-30T16:06:50Z

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

garydgregory · 2023-04-07T13:40:24Z

What is the status of this PR?

garydgregory · 2023-04-11T14:40:10Z

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

kinow · 2023-04-17T19:01:24Z

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 · 2023-04-18T18:27:15Z

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 · 2023-04-18T18:30:50Z

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

menscikov · 2023-06-12T13:07:52Z

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.

tresf added 3 commits August 26, 2022 00:37

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

fbdf524

Fix docs, formatting, add unit tests

b1df837

Merge branch 'apache:master' into attributes

081e43c

tresf changed the title ~~FileUtils copyDirectory() should not use COPY_ATTRIBUTES~~ IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES Aug 29, 2022

kinow reviewed Sep 2, 2022

View reviewed changes

Merge branch 'apache:master' into attributes

2d0ad5c

garydgregory marked this pull request as ready for review April 18, 2023 13:43

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

45a354e

garydgregory changed the title ~~IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES~~ [IO-769] FileUtils copyDirectory() should not use COPY_ATTRIBUTES Apr 18, 2023

garydgregory changed the title ~~[IO-769] FileUtils copyDirectory() should not use COPY_ATTRIBUTES~~ [IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES Apr 18, 2023

garydgregory merged commit 22f6525 into apache:master Apr 18, 2023

vsethi09 mentioned this pull request Feb 19, 2025

[CDAP-21027] Add java dependency on common-io:2.12.0 in k8s profile of cdap-master/pom.xml cdapio/cdap#15906

Merged

3 tasks

Uh oh!

Conversation

tresf commented Aug 29, 2022 • edited by garydgregory Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Aug 29, 2022

Codecov Report

Uh oh!

garydgregory commented Aug 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

garydgregory commented Sep 1, 2022

Uh oh!

kinow left a comment

Choose a reason for hiding this comment

Uh oh!

kinow Sep 2, 2022

Choose a reason for hiding this comment

Uh oh!

garydgregory commented Sep 3, 2022

Uh oh!

tresf commented Sep 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinow commented Sep 5, 2022

Uh oh!

garydgregory commented Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tresf commented Sep 6, 2022

Uh oh!

menscikov commented Jan 15, 2023

Uh oh!

Schmidor commented Jan 30, 2023

Uh oh!

garydgregory commented Apr 7, 2023

Uh oh!

garydgregory commented Apr 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kinow commented Apr 17, 2023

Uh oh!

garydgregory commented Apr 18, 2023

Uh oh!

garydgregory commented Apr 18, 2023

Uh oh!

menscikov commented Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tresf commented Aug 29, 2022 •

edited by garydgregory

Loading

garydgregory commented Aug 29, 2022 •

edited

Loading

tresf commented Sep 4, 2022 •

edited

Loading

garydgregory commented Sep 6, 2022 •

edited

Loading

garydgregory commented Apr 11, 2023 •

edited

Loading

menscikov commented Jun 12, 2023 •

edited

Loading