[IO-790] Fix symbolic link file filter #450.

Clean ups post PR merge

Author: garydgregory

src/changes/changes.xml

@@ -162,7 +162,7 @@ The <action> type attribute can be add,update,fix,remove.
         Fix misleading comments in FileFilterTest #334.
       </action>
       <action dev="ggregory" type="fix" due-to="Diego Marcilio">
-        Add missing javadoc for exceptions thrown for invalid arguments #339.
+        Add missing Javadoc for exceptions thrown for invalid arguments #339.
       </action>
       <action dev="ggregory" type="fix" due-to="richarda23">
         FileFilterTest minor fixes #340.
@@ -215,6 +215,9 @@ The <action> type attribute can be add,update,fix,remove.
       <action issue="IO-782" dev="ggregory" type="fix" due-to="Matteo Di Giovinazzo, Gary Gregory">
         SequenceReader should close readers when its close method is called #391.
       </action>
+      <action issue="IO-790" dev="ggregory" type="fix" due-to="Miguel Muñoz, Gary Gregory">
+        Fix symbolic link file filter #450.
+      </action>
       <!-- ADD -->
       <action type="add" dev="ggregory" due-to="Gary Gregory">
         Add GitHub coverage.yml.

src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java

@@ -124,9 +124,11 @@ public FileVisitResult accept(final Path path, final BasicFileAttributes attribu
     }
 
     /**
-     * Package access, so the unit test may override to mock it. To
-     * facilitate unit testing, all calls to test if the file is a symbolic should go
-     * through this method. (See the unit test for why.)
+     * Delegates to {@link Files#isSymbolicLink(Path)} for testing.
+     * <p>
+     * Using package access for unit tests. To facilitate unit testing, all calls to test if the file is a symbolic should go through this method. (See the unit
+     * test for why.)
+     * </p>
      *
      * @param filePath The filePath to test
      * @return true if the file exists and is a symbolic link to either a file or directory, false otherwise.

src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java

@@ -17,22 +17,23 @@
 
 package org.apache.commons.io.filefilter;
 
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 import java.io.File;
 import java.io.IOException;
 import java.nio.file.FileVisitResult;
 import java.nio.file.Files;
 import java.nio.file.Path;
-import java.util.function.BiFunction;
 
 import org.apache.commons.io.file.PathUtils;
+import org.apache.commons.io.function.IOBiFunction;
+import org.apache.commons.lang3.SystemUtils;
 import org.junit.jupiter.api.AfterAll;
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Test;
 
-import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.assertFalse;
-import static org.junit.jupiter.api.Assertions.assertTrue;
-
 /**
  * Tests {@link SymbolicLinkFileFilter}.
  */
@@ -44,63 +45,69 @@ public class SymbolicLinkFileFilterTest {
     public static final String DIRECTORY_NAME = "SLFF_TargetDirectory";
     public static final String DIRECTORY_LINK_NAME = "SLFF_LinkDirectory";
     public static final String MISSING = "Missing";
-    private static File testTargetFile;         // hard file
-    private static Path testTargetPath;         // hard file Path
-    private static File parentDirectoryFile;    // System Temp directory
-    private static File testLinkFile;           // symbolic link to hard file
-    private static String linkName;             // Name of link file
-    private static Path testLinkPath;           // symbolic link to hard file Path
-    private static File targetDirFile;          //
-    private static Path targetDirPath;          // hard directory Path
-    private static Path testLinkDirPath;        // symbolic link to hardDirectory
+
+    private static File testTargetFile; // hard file
+    private static Path testTargetPath; // hard file Path
+    private static File parentDirectoryFile; // System Temp directory
+    private static File testLinkFile; // symbolic link to hard file
+    private static String linkName; // Name of link file
+    private static Path testLinkPath; // symbolic link to hard file Path
+    private static File targetDirFile; //
+    private static Path targetDirPath; // hard directory Path
+    private static Path testLinkDirPath; // symbolic link to hardDirectory
     private static File testLinkDirFile;
-    private static File missingFile;            // non-existent file
+    private static File missingFile; // non-existent file
     private static SymbolicLinkFileFilter filter;
 
-    private static Path createRealSymbolicLink(Path link, Path target) {
-        try {
-            if (Files.exists(link)) {
-                Files.delete(link);
-            }
-            return Files.createSymbolicLink(link, target);
-        } catch (IOException e) {
-            throw new IllegalStateException("Failure to create Symbolic Link", e);
-        }
-    }
-
-    private static Path createMockSymbolicLink(Path lnk, Path tgt) {
-        try {
-            return Files.createFile(lnk);
-        } catch (IOException e) {
-            throw new IllegalStateException("Failure to create Symbolic Link", e);
-        }
-    }
-
     // Mock filter for testing on Windows.
     private static SymbolicLinkFileFilter createMockFilter() {
         return new SymbolicLinkFileFilter() {
+
+            private static final long serialVersionUID = 1L;
+
             @Override
             boolean isSymbolicLink(final Path filePath) {
                 return filePath.toFile().exists() && filePath.toString().contains("Link"); // Mock test
             }
         };
     }
 
+    private static Path createMockSymbolicLink(final Path link, final Path target) throws IOException {
+        return Files.createFile(link);
+    }
+
+    private static Path createRealSymbolicLink(final Path link, final Path target) throws IOException {
+        Files.deleteIfExists(link);
+        return Files.createSymbolicLink(link, target);
+    }
+
+    @AfterAll
+    static void tearDown() {
+        // Fortunately, delete() doesn't throw an exception if the file doesn't exist.
+        testLinkDirFile.delete();
+        targetDirFile.delete();
+        testLinkFile.delete();
+        testTargetFile.delete();
+    }
+
     /**
-     * <p>Unit test setup creates a hard file, a symbolic link to the hard file, a hard directory,
-     * and a symbolic link to that directory. All are created in the temp directory</p>
-     * <p>Unit test teardown deletes all four of these files.</p>
+     * <p>
+     * Unit test setup creates a hard file, a symbolic link to the hard file, a hard directory, and a symbolic link to that directory. All are created in the
+     * temp directory
+     * </p>
+     * <p>
+     * Unit test teardown deletes all four of these files.
+     * </p>
      *
      * @throws IOException If it fails to create the temporary files
      */
     @BeforeAll
     static void testSetup() throws IOException {
-        final BiFunction<Path, Path, Path> symbolicLinkCreator;
+        final IOBiFunction<Path, Path, Path> symbolicLinkCreator;
 
         // We can't create symbolic links on Windows without admin privileges,
         // so iff that's our OS, we mock them.
-        final String os = System.getProperty("os.name");
-        if (os.toLowerCase().contains("windows")) {
+        if (SystemUtils.IS_OS_WINDOWS) {
             symbolicLinkCreator = SymbolicLinkFileFilterTest::createMockSymbolicLink;
             filter = createMockFilter();
         } else {
@@ -123,18 +130,9 @@ static void testSetup() throws IOException {
         missingFile = new File(parentDirectoryPath.toFile(), MISSING);
     }
 
-    @AfterAll
-    static void tearDown() {
-        // Fortunately, delete() doesn't throw an exception if the file doesn't exist.
-        testLinkDirFile.delete();
-        targetDirFile.delete();
-        testLinkFile.delete();
-        testTargetFile.delete();
-    }
-
     @Test
-    public void testSymbolicLinkFileFilter() {
-        assertEquals(FileVisitResult.TERMINATE, SymbolicLinkFileFilter.INSTANCE.accept(PathUtils.current(), null));
+    public void testFileFilter_HardDirectory() {
+        assertFalse(filter.accept(targetDirFile));
     }
 
     @Test
@@ -148,8 +146,8 @@ public void testFileFilter_Link() {
     }
 
     @Test
-    public void testFileFilter_HardDirectory() {
-        assertFalse(filter.accept(targetDirFile));
+    public void testFileFilter_missingFile() {
+        assertFalse(filter.accept(missingFile));
     }
 
     @Test
@@ -158,8 +156,8 @@ public void testFileFilter_PathLink() {
     }
 
     @Test
-    public void testFileFilter_missingFile() {
-        assertFalse(filter.accept(missingFile));
+    public void testFileNameFilter_HardDirectory() {
+        assertFalse(filter.accept(parentDirectoryFile, DIRECTORY_NAME));
     }
 
     @Test
@@ -173,8 +171,8 @@ public void testFileNameFilter_Link() {
     }
 
     @Test
-    public void testFileNameFilter_HardDirectory() {
-        assertFalse(filter.accept(parentDirectoryFile, DIRECTORY_NAME));
+    public void testFileNameFilter_missingFile() {
+        assertFalse(filter.accept(parentDirectoryFile, MISSING));
     }
 
     @Test
@@ -183,8 +181,8 @@ public void testFileNameFilter_PathLink() {
     }
 
     @Test
-    public void testFileNameFilter_missingFile() {
-        assertFalse(filter.accept(parentDirectoryFile, MISSING));
+    public void testPathFilter_HardDirectory() {
+        assertEquals(FileVisitResult.TERMINATE, filter.accept(targetDirPath, null));
     }
 
     @Test
@@ -198,8 +196,8 @@ public void testPathFilter_Link() {
     }
 
     @Test
-    public void testPathFilter_HardDirectory() {
-        assertEquals(FileVisitResult.TERMINATE, filter.accept(targetDirPath, null));
+    public void testPathFilter_missingFile() {
+        assertEquals(FileVisitResult.TERMINATE, filter.accept(missingFile.toPath(), null));
     }
 
     @Test
@@ -208,7 +206,7 @@ public void testPathFilter_PathLink() {
     }
 
     @Test
-    public void testPathFilter_missingFile() {
-        assertEquals(FileVisitResult.TERMINATE, filter.accept(missingFile.toPath(), null));
+    public void testSymbolicLinkFileFilter() {
+        assertEquals(FileVisitResult.TERMINATE, SymbolicLinkFileFilter.INSTANCE.accept(PathUtils.current(), null));
     }
 }