[IO-862] FileUtils.deleteDirectory fails for a directory containing a

broken symbolic link

Author: garydgregory

src/changes/changes.xml

@@ -57,6 +57,7 @@ The <action> type attribute can be add,update,fix,remove.
       <action dev="ggregory" type="fix" issue="IO-859" due-to="JD Dean, Gary Gregory">FileUtils.forceDelete on non-existent file on Windows throws IOException rather than FileNotFoundException.</action>
       <action dev="ggregory" type="fix"                due-to="Éamonn McManus">Use Unicode escapes for superscript characters. #701.</action>
       <action dev="ggregory" type="fix" issue="IO-863" due-to="Éamonn McManus, Gary Gregory">Recent incompatible change to FileUtils.listFiles re extensions, see IO-856.</action>
+      <action dev="ggregory" type="fix" issue="IO-862" due-to="Éamonn McManus, Gary Gregory">FileUtils.deleteDirectory fails for a directory containing a broken symbolic link.</action>
       <!-- ADD -->
       <action dev="ggregory" type="add"                due-to="Gary Gregory">Add @FunctionalInterface to ClassNameMatcher.</action>
       <action dev="ggregory" type="add"                due-to="Gary Gregory">Add ValidatingObjectInputStream.Builder and ValidatingObjectInputStream.builder().</action>

src/main/java/org/apache/commons/io/FileUtils.java

@@ -285,19 +285,28 @@ public static String byteCountToDisplaySize(final Number size) {
     }
 
     /**
-     * Requires that the given {@link File} exists, and throws a {@link FileNotFoundException} if it doesn't.
+     * Requires that the given {@link File} is non-null and exists (if strict is true).
      *
      * @param file The {@link File} to check.
-     * @throws FileNotFoundException if the file does not exist
+     * @param strict whether to check that the {@code file} exists.
+     * @throws FileNotFoundException if the file does not exist.
      * @throws NullPointerException  if the given {@link File} is {@code null}.
      */
-    private static void checkExists(final File file) throws FileNotFoundException {
-        Objects.requireNonNull(file, "file");
-        if (!file.exists()) {
+    private static void checkExists(final File file, final boolean strict) throws FileNotFoundException {
+        Objects.requireNonNull(file, PROTOCOL_FILE);
+        if (strict && !file.exists()) {
             throw new FileNotFoundException(file.toString());
         }
     }
 
+    /**
+     * Requires that the given {@link File} exists, and throws a {@link FileNotFoundException} if it doesn't.
+     *
+     * @param file The {@link File} to check.
+     * @param name The NullPointerException message.
+     * @throws FileNotFoundException if the file does not exist.
+     * @throws NullPointerException  if the given {@link File} is {@code null}.
+     */
     private static void checkFileExists(final File file, final String name) throws FileNotFoundException {
         Objects.requireNonNull(file, name);
         if (!file.isFile()) {
@@ -350,8 +359,8 @@ public static Checksum checksum(final File file, final Checksum checksum) throws
      *
      * @param file the file to checksum, must not be {@code null}
      * @return the checksum value
-     * @throws NullPointerException if the given {@link File} is {@code null}.
-     * @throws IllegalArgumentException if the given {@link File} does not exist or is not a file.
+     * @throws NullPointerException if the {@code file} is {@code null}.
+     * @throws IllegalArgumentException if the {@code file} does not exist or is not a file.
      * @throws IOException              if an IO error occurs reading the file.
      * @since 1.3
      */
@@ -364,20 +373,20 @@ public static long checksumCRC32(final File file) throws IOException {
      *
      * @param directory directory to clean
      * @throws NullPointerException if the given {@link File} is {@code null}.
-     * @throws IllegalArgumentException if directory does not exist or is not a directory.
+     * @throws IllegalArgumentException if the {@code directory} does not exist or is not a directory.
      * @throws IOException if an I/O error occurs.
      * @see #forceDelete(File)
      */
     public static void cleanDirectory(final File directory) throws IOException {
-        IOConsumer.forAll(FileUtils::forceDelete, listFiles(directory, null));
+        IOConsumer.forAll(f -> forceDelete(f, false), listFiles(directory, null));
     }
 
     /**
      * Cleans a directory without deleting it.
      *
      * @param directory directory to clean, must not be {@code null}
      * @throws NullPointerException if the given {@link File} is {@code null}.
-     * @throws IllegalArgumentException if directory does not exist or is not a directory.
+     * @throws IllegalArgumentException if the {@code directory} does not exist or is not a directory.
      * @throws IOException if an I/O error occurs.
      * @see #forceDeleteOnExit(File)
      */
@@ -1386,8 +1395,28 @@ private static void doCopyDirectory(final File srcDir, final File destDir, final
      * @throws IOException           in case deletion is unsuccessful.
      */
     public static void forceDelete(final File file) throws IOException {
+        forceDelete(file, true);
+    }
+
+    /**
+     * Deletes a file or directory. For a directory, delete it and all subdirectories.
+     * <p>
+     * The difference between File.delete() and this method are:
+     * </p>
+     * <ul>
+     * <li>The directory does not have to be empty.</li>
+     * <li>You get an exception when a file or directory cannot be deleted.</li>
+     * </ul>
+     *
+     * @param file file or directory to delete, must not be {@code null}.
+     * @param strict whether to throw a FileNotFoundException.
+     * @throws NullPointerException  if the file is {@code null}.
+     * @throws FileNotFoundException if the file was not found.
+     * @throws IOException           in case deletion is unsuccessful.
+     */
+    private static void forceDelete(final File file, final boolean strict) throws IOException {
         Objects.requireNonNull(file, PROTOCOL_FILE);
-        checkExists(file); // fail-fast
+        checkExists(file, strict); // fail-fast
         final Counters.PathCounters deleteCounters;
         try {
             deleteCounters = PathUtils.delete(file.toPath(), PathUtils.EMPTY_LINK_OPTION_ARRAY, StandardDeleteOption.OVERRIDE_READ_ONLY);
@@ -2265,11 +2294,11 @@ private static AccumulatorPathVisitor listAccumulate(final File directory, final
     /**
      * Lists files in a directory, asserting that the supplied directory exists and is a directory.
      *
-     * @param directory The directory to list
+     * @param directory The directory to list.
      * @param fileFilter Optional file filter, may be null.
      * @return The files in the directory, never {@code null}.
      * @throws NullPointerException if directory is {@code null}.
-     * @throws IllegalArgumentException if {@link directory} exists but is not a directory
+     * @throws IllegalArgumentException if {@link directory} exists but is not a directory.
      * @throws IOException if an I/O error occurs.
      */
     private static File[] listFiles(final File directory, final FileFilter fileFilter) throws IOException {

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

@@ -248,10 +248,10 @@ private void createFilesForTestCopyDirectory(final File grandParentDir, final Fi
 
     private ImmutablePair<Path, Path> createTempSymbolicLinkedRelativeDir() throws IOException {
         final Path targetDir = tempDirPath.resolve("subdir");
-        final Path symlinkDir = tempDirPath.resolve("symlinked-dir");
+        final Path symLinkedDir = tempDirPath.resolve("symlinked-dir");
         Files.createDirectory(targetDir);
-        Files.createSymbolicLink(symlinkDir, targetDir);
-        return ImmutablePair.of(symlinkDir, targetDir);
+        Files.createSymbolicLink(symLinkedDir, targetDir);
+        return ImmutablePair.of(symLinkedDir, targetDir);
     }
 
     private Set<String> getFilePathSet(final List<File> files) {
@@ -318,7 +318,6 @@ private boolean setLastModifiedMillis(final File testFile, final long millis) {
     public void setUp() throws Exception {
         testFile1 = new File(tempDirFile, "file1-test.txt");
         testFile2 = new File(tempDirFile, "file1a-test.txt");
-
         testFile1Size = testFile1.length();
         testFile2Size = testFile2.length();
         if (!testFile1.getParentFile().exists()) {
@@ -1586,6 +1585,7 @@ public void testDeleteDirectorySymbolicLinkAbsentTarget() throws IOException {
         final Path symlinkedDir = pair.getLeft();
         final Path targetDir = pair.getRight();
         assertTrue(Files.exists(symlinkedDir), symlinkedDir::toString);
+        // remove target directory, keeping symbolic link
         Files.delete(targetDir);
         assertFalse(Files.exists(targetDir), targetDir::toString);
         assertFalse(Files.exists(symlinkedDir), symlinkedDir::toString);
@@ -1594,6 +1594,26 @@ public void testDeleteDirectorySymbolicLinkAbsentTarget() throws IOException {
         assertFalse(Files.exists(symlinkedDir), symlinkedDir::toString);
     }
 
+    @Test
+    public void testDeleteDirectorySymbolicLinkAbsentDeepTarget() throws IOException {
+        final ImmutablePair<Path, Path> pair = createTempSymbolicLinkedRelativeDir();
+        final Path symLinkedDir = pair.getLeft();
+        final Path targetDir = pair.getRight();
+        // more setup
+        final Path targetDir2 = targetDir.resolve("subdir2");
+        final Path symLinkedDir2 = targetDir.resolve("symlinked-dir2");
+        Files.createDirectory(targetDir2);
+        Files.createSymbolicLink(symLinkedDir2, targetDir2);
+        assertTrue(Files.exists(symLinkedDir2), symLinkedDir2::toString);
+        // remove target directory, keeping symbolic link
+        Files.delete(targetDir2);
+        assertFalse(Files.exists(targetDir2), targetDir2::toString);
+        assertFalse(Files.exists(symLinkedDir2), symLinkedDir2::toString);
+        // actual test
+        FileUtils.deleteDirectory(targetDir.toFile());
+        assertFalse(Files.exists(targetDir), targetDir::toString);
+    }
+
     @Test
     public void testDeleteQuietlyDir() throws IOException {
         final File testDirectory = new File(tempDirFile, "testDeleteQuietlyDir");