You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by GitBox <gi...@apache.org> on 2022/08/29 14:36:21 UTC
[GitHub] [commons-io] tresf opened a new pull request, #377: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
tresf opened a new pull request, #377:
URL: https://github.com/apache/commons-io/pull/377
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)`<br>`BUILTIN\Administrators:(I)(F)`<br>`BUILTIN\Users:(I)(RX)`<br>`APPLICATION PACKAGE AUTHORITY\ALL APPLICATION PACKAGES:(I)(RX)`<br>`APPLICATION PACKAGE AUTHORITY\ALL RESTRICTED APPLICATION PACKAGES:(I)(RX)` |
| `commons-io@2.9` | `NT AUTHORITY\SYSTEM:(F)`<br>`BUILTIN\Administrators:(F)`<br>`WIN10ARM\owner:(F)`<br> |
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 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 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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] tresf commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
tresf commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1238382739
> Hi @tresf, Please rebase on git master to see what happens with the new test for [IO-575](https://issues.apache.org/jira/browse/IO-575).
Done, I think you need to kick-start the workflow now. :)
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by "kinow (via GitHub)" <gi...@apache.org>.
kinow commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1511928178
> 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?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] menscikov commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
menscikov commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1383174106
Hello, `FileUtils.copyInputStreamToFile()` has the same issue starting from **2.9** version.
Please fix it also.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: [IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1513619421
> 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](https://github.com/apache/commons-io/blob/f22a4227401855ecbfdf8184bbe37275c3aeb5c3/src/main/java/org/apache/commons/io/FileUtils.java#L2854-L2857) 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.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] menscikov commented on pull request #377: [IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES
Posted by "menscikov (via GitHub)" <gi...@apache.org>.
menscikov commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1587311126
Hello, `FileUtils.copyInputStreamToFile()` still has the same issue in the 2.13.0 version starting from 2.9 version.
I'm testing on Android API 23 on Samsung mobile phone. Version 2.9 is working fine.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1238362100
Hi @tresf
Please rebase on git master to see what happens with the new test for [IO-575](https://issues.apache.org/jira/browse/IO-575).
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory merged pull request #377: [IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #377:
URL: https://github.com/apache/commons-io/pull/377
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1500298538
What is the status of this PR?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1234144089
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 ?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1230711275
See also https://issues.apache.org/jira/projects/IO/issues/IO-769
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] codecov-commenter commented on pull request #377: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1230463751
# [Codecov](https://codecov.io/gh/apache/commons-io/pull/377?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#377](https://codecov.io/gh/apache/commons-io/pull/377?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (081e43c) into [master](https://codecov.io/gh/apache/commons-io/commit/44a21854a442ad798abd54eb29197d9807c4a9de?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (44a2185) will **increase** coverage by `0.03%`.
> The diff coverage is `91.30%`.
```diff
@@ 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](https://codecov.io/gh/apache/commons-io/pull/377?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [src/main/java/org/apache/commons/io/FileUtils.java](https://codecov.io/gh/apache/commons-io/pull/377/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-c3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2NvbW1vbnMvaW8vRmlsZVV0aWxzLmphdmE=) | `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](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] tresf commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
tresf commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1236373760
> 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 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...)`.
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.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] Schmidor commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by "Schmidor (via GitHub)" <gi...@apache.org>.
Schmidor commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1408889665
`FileUtils.moveFile(File srcFile, File destFile) ` was also changed and got COPY_ATTRIBUTES in 2.9
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1503498264
I plan on releasing soon, so if we can get more eyes here and a rebase, that would be great.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1236125211
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?
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on a diff in pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
kinow commented on code in PR #377:
URL: https://github.com/apache/commons-io/pull/377#discussion_r961252278
##########
src/main/java/org/apache/commons/io/FileUtils.java:
##########
@@ -2840,37 +2836,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
Review Comment:
Thanks for adding the note. No idea why that changed, didn't have time to dig into `git`'s history, sorry.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] kinow commented on pull request #377: IO-769: FileUtils copyDirectory() should not use COPY_ATTRIBUTES
Posted by GitBox <gi...@apache.org>.
kinow commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1236816687
>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:
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [commons-io] garydgregory commented on pull request #377: [IO-769] FileUtils copyDirectory() should NOT use COPY_ATTRIBUTES
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #377:
URL: https://github.com/apache/commons-io/pull/377#issuecomment-1513623294
The consensus, reading this PR and the ticket, is to revert the behavior, so I will merge this PR.
--
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@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org