You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by pk...@apache.org on 2025/10/02 07:53:45 UTC
(commons-io) branch fix/file-delete created (now 7b93b2bcd)
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a change to branch fix/file-delete
in repository https://gitbox.apache.org/repos/asf/commons-io.git
at 7b93b2bcd Improve `FileUtils.forceDelete()` tests on Windows
This branch includes the following new commits:
new 7b93b2bcd Improve `FileUtils.forceDelete()` tests on Windows
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(commons-io) 01/01: Improve `FileUtils.forceDelete()` tests on Windows
Posted by pk...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
pkarwasz pushed a commit to branch fix/file-delete
in repository https://gitbox.apache.org/repos/asf/commons-io.git
commit 7b93b2bcd07328c68210633ee2eebff2cd83023f
Author: Piotr P. Karwasz <pk...@apache.org>
AuthorDate: Thu Oct 2 09:44:51 2025 +0200
Improve `FileUtils.forceDelete()` tests on Windows
On Windows, the `DeleteFile` Win32 API has a little quirk: it refuses to delete files with the legacy **DOS read-only attribute** set. (Because apparently 99% of Windows users don’t realize that “deleting a file” is actually an operation on the *directory*, not the file itself 😉).
So the usual drill is: clear the read-only flag first, then delete.
* Until JDK 25, `File.delete()` did this for you behind the scenes: it quietly stripped the flag before calling into `DeleteFile`. That meant your file might be left behind (with the flag missing) if the *real* ACLs didn’t allow deletion.
* From JDK 25 onward, `File.delete()` doesn’t touch the flag anymore. If the bit is set, `DeleteFile` fails, end of story.
* `FileUtils.forceDelete()` already knows how to juggle the flag itself, so its behavior didn’t change.
This PR:
* Updates two tests that were (unfairly) comparing `File.delete` with `FileUtils.forceDelete`. With JDK 25, their expectations diverged.
* Adds a new test to confirm that `FileUtils.forceDelete` restores the read-only flag if the actual deletion fails.
> [!WARNING]
> I didn’t develop this on a Windows box, so I couldn’t test it locally. Leaving this as a **draft PR** until CI tells us whether Windows agrees with me.
---
Would you like me to make it **shorter, tweet-style witty**, or keep it **longer, explanatory for reviewers**?
---
.../java/org/apache/commons/io/FileUtilsTest.java | 119 +++++++++++++++++----
1 file changed, 100 insertions(+), 19 deletions(-)
diff --git a/src/test/java/org/apache/commons/io/FileUtilsTest.java b/src/test/java/org/apache/commons/io/FileUtilsTest.java
index 4f6efa0f9..6a6f07d79 100644
--- a/src/test/java/org/apache/commons/io/FileUtilsTest.java
+++ b/src/test/java/org/apache/commons/io/FileUtilsTest.java
@@ -46,12 +46,17 @@
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.nio.charset.UnsupportedCharsetException;
+import java.nio.file.AccessDeniedException;
import java.nio.file.Files;
import java.nio.file.LinkOption;
import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.StandardCopyOption;
+import java.nio.file.attribute.AclEntry;
+import java.nio.file.attribute.AclEntryPermission;
+import java.nio.file.attribute.AclEntryType;
import java.nio.file.attribute.AclFileAttributeView;
+import java.nio.file.attribute.DosFileAttributeView;
import java.nio.file.attribute.FileTime;
import java.nio.file.attribute.PosixFilePermission;
import java.nio.file.attribute.PosixFilePermissions;
@@ -99,6 +104,7 @@
import org.junit.jupiter.api.condition.EnabledIf;
import org.junit.jupiter.api.condition.EnabledOnOs;
import org.junit.jupiter.api.condition.OS;
+import org.junit.jupiter.api.io.TempDir;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.ValueSource;
@@ -1731,28 +1737,97 @@ void testForceDeleteReadOnlyDirectory() throws Exception {
}
}
+ private boolean isAtLeastJava25() {
+ try {
+ return Integer.parseInt(SystemUtils.JAVA_SPECIFICATION_VERSION) >= 25;
+ } catch (final NumberFormatException e) {
+ return false;
+ }
+ }
+
@Test
void testForceDeleteReadOnlyFile() throws Exception {
try (TempFile destination = TempFile.create("test-", ".txt")) {
final File file = destination.toFile();
- assertTrue(file.setReadOnly());
- assertTrue(file.canRead());
- assertFalse(file.canWrite());
+ assertTrue(file.setReadOnly(), "Setting file read-only successful");
+ assertTrue(file.canRead(), "File must be readable");
+ assertFalse(file.canWrite(), "File must not be writable");
+ assertTrue(file.exists(), "File doesn't exist to delete");
// sanity check that File.delete() deletes read-only files.
- assertTrue(file.delete());
+ if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) {
+ // On Windows with Java 25+ File.delete() no longer deletes read-only files.
+ // See: https://bugs.openjdk.org/browse/JDK-8355954
+ assertTrue(file.setWritable(true), "Setting file writable successful");
+ }
+ assertTrue(file.delete(), "File.delete() must delete read-only file");
}
try (TempFile destination = TempFile.create("test-", ".txt")) {
final File file = destination.toFile();
// real test
- assertTrue(file.setReadOnly());
- assertTrue(file.canRead());
- assertFalse(file.canWrite());
+ assertTrue(file.setReadOnly(), "Setting file read-only successful");
+ assertTrue(file.canRead(), "File must be readable");
+ assertFalse(file.canWrite(), "File must not be writable");
assertTrue(file.exists(), "File doesn't exist to delete");
FileUtils.forceDelete(file);
- assertFalse(file.exists(), "Check deletion");
+ assertFalse(file.exists(), "FileUtils.forceDelete() must delete read-only file");
}
}
+ private static boolean supportsDos(Path p) {
+ return Files.getFileAttributeView(p, DosFileAttributeView.class) != null;
+ }
+
+ private static boolean supportsAcl(Path p) {
+ return Files.getFileAttributeView(p, AclFileAttributeView.class) != null;
+ }
+
+ private static boolean isDosReadOnly(Path p) throws IOException {
+ return (Boolean) Files.getAttribute(p, "dos:readonly", LinkOption.NOFOLLOW_LINKS);
+ }
+
+ private static void prependDenyForOwner(Path path, AclEntryPermission permission) throws IOException {
+ final AclFileAttributeView view = Files.getFileAttributeView(path, AclFileAttributeView.class);
+ assumeTrue(view != null, "ACL view not supported");
+ final AclEntry denyEntry = AclEntry.newBuilder().setType(AclEntryType.DENY).setPrincipal(view.getOwner()).setPermissions(permission).build();
+ final List<AclEntry> acl = new ArrayList<>(view.getAcl());
+ // ACL is processed in order, so add to the start of the list
+ acl.add(0, denyEntry);
+ view.setAcl(acl);
+ }
+
+ @Test
+ @EnabledOnOs(value = OS.WINDOWS)
+ void testForceDeleteRestoresReadOnlyOnFailure(@TempDir Path tempDir) throws Exception {
+ // Skip if the underlying FS doesn’t expose DOS and ACL views (e.g., some network shares).
+ assumeTrue(supportsDos(tempDir), "DOS attributes not supported");
+ assumeTrue(supportsAcl(tempDir), "ACLs not supported");
+
+ final Path readOnly = tempDir.resolve("read-only-file.txt");
+ Files.createFile(readOnly);
+
+ // 1) Set the DOS readonly bit (what we want to ensure is restored on failure)
+ Files.setAttribute(readOnly, "dos:readonly", true);
+ assertTrue(isDosReadOnly(readOnly), "Precondition: file must be DOS-readonly");
+
+ // 2) Cause deletion to fail due to permissions:
+ // deny DELETE on the file, and deny DELETE_CHILD on the parent directory.
+ prependDenyForOwner(readOnly, AclEntryPermission.DELETE);
+ prependDenyForOwner(tempDir, AclEntryPermission.DELETE_CHILD);
+
+ // Sanity checks:
+ // canWrite() on Windows reflects only the DOS readonly bit (ignores the ACL for write).
+ final File file = readOnly.toFile();
+ assertFalse(file.canWrite(), "Sanity: Windows canWrite() should be false when dos:readonly is set");
+
+ // 3) Attempt forced deletion; should fail with AccessDeniedException due to ACLs.
+ assertThrows(AccessDeniedException.class, () -> FileUtils.forceDelete(file), "Deletion must fail because DELETE/DELETE_CHILD are denied");
+
+ // 4) Critical assertion: even though deletion failed, the DOS readonly flag must be restored.
+ assertTrue(isDosReadOnly(readOnly), "dos:readonly must be preserved/restored after failed deletion");
+ assertFalse(file.canWrite(), "File must still not be writable per DOS attribute");
+ assertTrue(Files.exists(readOnly), "File should still exist after failed deletion");
+ }
+
@Test
public void testForceDeleteSymlink() throws Exception {
final ImmutablePair<Path, Path> pair = createTempSymbolicLinkedRelativeDir();
@@ -1823,23 +1898,29 @@ void testForceDeleteUnwritableDirectory() throws Exception {
void testForceDeleteUnwritableFile() throws Exception {
try (TempFile destination = TempFile.create("test-", ".txt")) {
final File file = destination.toFile();
- assertTrue(file.canWrite());
- assertTrue(file.setWritable(false));
- assertFalse(file.canWrite());
- assertTrue(file.canRead());
+ assertTrue(file.canWrite(), "File must be writable");
+ assertTrue(file.setWritable(false), "Setting file unwritable successful");
+ assertFalse(file.canWrite(), "File must not be writable");
+ assertTrue(file.canRead(), "File must be readable");
+ assertTrue(file.exists(), "File must exist to delete");
// sanity check that File.delete() deletes unwritable files.
- assertTrue(file.delete());
+ if (SystemUtils.IS_OS_WINDOWS && isAtLeastJava25()) {
+ // On Windows with Java 25+ File.delete() no longer deletes read-only files.
+ // See: https://bugs.openjdk.org/browse/JDK-8355954
+ assertTrue(file.setWritable(true), "Setting file writable successful");
+ }
+ assertTrue(file.delete(), "File.delete() must delete unwritable file");
}
try (TempFile destination = TempFile.create("test-", ".txt")) {
final File file = destination.toFile();
// real test
- assertTrue(file.canWrite());
- assertTrue(file.setWritable(false));
- assertFalse(file.canWrite());
- assertTrue(file.canRead());
- assertTrue(file.exists(), "File doesn't exist to delete");
+ assertTrue(file.canWrite(), "File must be writable");
+ assertTrue(file.setWritable(false), "Setting file unwritable successful");
+ assertFalse(file.canWrite(), "File must not be writable");
+ assertTrue(file.canRead(), "File must be readable");
+ assertTrue(file.exists(), "File must exist to delete");
FileUtils.forceDelete(file);
- assertFalse(file.exists(), "Check deletion");
+ assertFalse(file.exists(), "FileUtils.forceDelete() must delete unwritable file");
}
}