Skip to content

Commit 028dd62

Browse files
authored
Improve FileUtils.forceDelete() tests on Windows (#791)
* 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. * fix: always clear read-only bit * fix: check for `IOException`, not `AccessDeniedException` * feat: add holder for file and parent attributes Introduce a helper that snapshots file and parent directory attributes before `setReadOnly` is applied. If a deletion attempt fails, the holder can restore the original attributes to keep the filesystem state consistent. * fix: Spotbugs failure * fix: prefer POSIX to DOS attributes when both are available On Linux/Unix, both POSIX and DOS attribute views may be supported, while on Windows only DOS attributes are available. Check for POSIX first to ensure the correct view is used across platforms. * fix: revert read-only related changes
1 parent 5331074 commit 028dd62

File tree

1 file changed

+36
-21
lines changed

1 file changed

+36
-21
lines changed

src/test/java/org/apache/commons/io/FileUtilsTest.java

Lines changed: 36 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -52,6 +52,7 @@
5252
import java.nio.file.Paths;
5353
import java.nio.file.StandardCopyOption;
5454
import java.nio.file.attribute.AclFileAttributeView;
55+
import java.nio.file.attribute.DosFileAttributeView;
5556
import java.nio.file.attribute.FileTime;
5657
import java.nio.file.attribute.PosixFilePermission;
5758
import java.nio.file.attribute.PosixFilePermissions;
@@ -1735,21 +1736,31 @@ void testForceDeleteReadOnlyDirectory() throws Exception {
17351736
void testForceDeleteReadOnlyFile() throws Exception {
17361737
try (TempFile destination = TempFile.create("test-", ".txt")) {
17371738
final File file = destination.toFile();
1738-
assertTrue(file.setReadOnly());
1739-
assertTrue(file.canRead());
1740-
assertFalse(file.canWrite());
1741-
// sanity check that File.delete() deletes read-only files.
1742-
assertTrue(file.delete());
1739+
assertTrue(file.setReadOnly(), "Setting file read-only successful");
1740+
assertTrue(file.canRead(), "File must be readable");
1741+
assertFalse(file.canWrite(), "File must not be writable");
1742+
assertTrue(file.exists(), "File doesn't exist to delete");
1743+
// Since JDK 25 on Windows, File.delete() refuses to remove files
1744+
// with the DOS readonly bit set (JDK-8355954).
1745+
// We clear the bit here for consistency across JDK versions.
1746+
setDosReadOnly(file.toPath(), false);
1747+
assertTrue(file.delete(), "File.delete() must delete read-only file");
17431748
}
17441749
try (TempFile destination = TempFile.create("test-", ".txt")) {
17451750
final File file = destination.toFile();
17461751
// real test
1747-
assertTrue(file.setReadOnly());
1748-
assertTrue(file.canRead());
1749-
assertFalse(file.canWrite());
1752+
assertTrue(file.setReadOnly(), "Setting file read-only successful");
1753+
assertTrue(file.canRead(), "File must be readable");
1754+
assertFalse(file.canWrite(), "File must not be writable");
17501755
assertTrue(file.exists(), "File doesn't exist to delete");
17511756
FileUtils.forceDelete(file);
1752-
assertFalse(file.exists(), "Check deletion");
1757+
assertFalse(file.exists(), "FileUtils.forceDelete() must delete read-only file");
1758+
}
1759+
}
1760+
1761+
private static void setDosReadOnly(Path p, boolean readOnly) throws IOException {
1762+
if (Files.getFileStore(p).supportsFileAttributeView(DosFileAttributeView.class)) {
1763+
Files.setAttribute(p, "dos:readonly", readOnly, LinkOption.NOFOLLOW_LINKS);
17531764
}
17541765
}
17551766

@@ -1823,23 +1834,27 @@ void testForceDeleteUnwritableDirectory() throws Exception {
18231834
void testForceDeleteUnwritableFile() throws Exception {
18241835
try (TempFile destination = TempFile.create("test-", ".txt")) {
18251836
final File file = destination.toFile();
1826-
assertTrue(file.canWrite());
1827-
assertTrue(file.setWritable(false));
1828-
assertFalse(file.canWrite());
1829-
assertTrue(file.canRead());
1830-
// sanity check that File.delete() deletes unwritable files.
1831-
assertTrue(file.delete());
1837+
assertTrue(file.canWrite(), "File must be writable");
1838+
assertTrue(file.setWritable(false), "Setting file unwritable successful");
1839+
assertFalse(file.canWrite(), "File must not be writable");
1840+
assertTrue(file.canRead(), "File must be readable");
1841+
assertTrue(file.exists(), "File must exist to delete");
1842+
// Since JDK 25 on Windows, File.delete() refuses to remove files
1843+
// with the DOS readonly bit set (JDK-8355954).
1844+
// We clear the bit here for consistency across JDK versions.
1845+
setDosReadOnly(file.toPath(), false);
1846+
assertTrue(file.delete(), "File.delete() must delete unwritable file");
18321847
}
18331848
try (TempFile destination = TempFile.create("test-", ".txt")) {
18341849
final File file = destination.toFile();
18351850
// real test
1836-
assertTrue(file.canWrite());
1837-
assertTrue(file.setWritable(false));
1838-
assertFalse(file.canWrite());
1839-
assertTrue(file.canRead());
1840-
assertTrue(file.exists(), "File doesn't exist to delete");
1851+
assertTrue(file.canWrite(), "File must be writable");
1852+
assertTrue(file.setWritable(false), "Setting file unwritable successful");
1853+
assertFalse(file.canWrite(), "File must not be writable");
1854+
assertTrue(file.canRead(), "File must be readable");
1855+
assertTrue(file.exists(), "File must exist to delete");
18411856
FileUtils.forceDelete(file);
1842-
assertFalse(file.exists(), "Check deletion");
1857+
assertFalse(file.exists(), "FileUtils.forceDelete() must delete unwritable file");
18431858
}
18441859
}
18451860

0 commit comments

Comments
 (0)