Developers,
(I'm new to Apache, so I hope this is the right way to submit patches.)
Here are two patches, which must be applied in order.
The first one fixes bugs in the file, and adds a method that will
facilitate testing.
The second one is a thorough unit test that handles issues on Windows
relatively cleanly. The test creates temporary files and temporary symbolic
links for testing. It should run on all platforms, and should run correctly
on Windows without actually creating symbolic links. I don't have access to
a Windows machine, but I tested the windows-only code on my Mac with a
simple temporary change to the test for which platform it's running on.
(Very little runs differently on Windows.)
The patches must be applied in order. The first one changes the class
itself, and the second changes the unit test.
— Miguel Muñoz
Index: src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java b/src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java
--- a/src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java (revision 8985de8fe74f6622a419b37a6eed0dbc484dc128)
+++ b/src/main/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilter.java (date 1681804450065)
@@ -25,6 +25,7 @@
/**
* This filter accepts {@code File}s that are symbolic links.
+ * Files that do not exist are not symbolic links, so they will be rejected without throwing an exception.
* <p>
* For example, here is how to print out a list of the real files
* within the current directory:
@@ -61,6 +62,17 @@
* @see FileFilterUtils#fileFileFilter()
*/
public class SymbolicLinkFileFilter extends AbstractFileFilter implements Serializable {
+ /*
+ * Note to developers: The unit test needs to create symbolic links to files. However, on
+ * Windows, this can't be done without admin privileges. The unit test works around this
+ * by doing two things: 1 separating the class logic from the actual test for a symbolic
+ * link, and 2 on Windows only, overriding the method that tests for symbolic links.
+ * This means the tests will run on all machines, but on Windows they won't completely
+ * test the necessary behavior. Be careful not to break this, but be aware of it when
+ * writing unit tests. You can still maintain this class and its unit est on Windows. The
+ * one method that won't get tested on Windows is not likely to change, and will be tested
+ * properly when it gets run on Apache servers.
+ */
/**
* Singleton instance of file filter.
@@ -76,25 +88,35 @@
}
/**
- * Checks to see if the file is a file.
+ * Checks to see if the file is a symbolic link.
*
* @param file the File to check
- * @return true if the file is a file
+ * @return true if the file is a symbolic link to either another file or a directory.
*/
@Override
public boolean accept(final File file) {
- return file.isFile();
+ return file.exists() && isSymbolicLink(file.toPath());
}
+
/**
* Checks to see if the file is a file.
- * @param file the File to check
*
- * @return true if the file is a file
+ * @param path the File Path to check
+ * @return true if the file is a symbolic link to either another file or a directory.
*/
@Override
- public FileVisitResult accept(final Path file, final BasicFileAttributes attributes) {
- return toFileVisitResult(Files.isSymbolicLink(file), file);
+ public FileVisitResult accept(final Path path, final BasicFileAttributes attributes) {
+ final boolean isSymbolicLink = Files.exists(path) && isSymbolicLink(path);
+ return toFileVisitResult(isSymbolicLink, path);
}
+ /*
+ * Package access, so the unit test may override to mock it. All calls to
+ * test if the file is a symbolic should go through this method, to
+ * facilitate unit testing. (See the unit test for why.)
+ */
+ boolean isSymbolicLink(Path filePath) {
+ return Files.isSymbolicLink(filePath);
+ }
}
Index: src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>ISO-8859-1
===================================================================
diff --git a/src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java b/src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java
--- a/src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java (revision 8985de8fe74f6622a419b37a6eed0dbc484dc128)
+++ b/src/test/java/org/apache/commons/io/filefilter/SymbolicLinkFileFilterTest.java (date 1681804596443)
@@ -17,21 +17,196 @@
package org.apache.commons.io.filefilter;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-
+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.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
+import static org.junit.jupiter.api.Assertions.*;
+
/**
* Tests {@link SymbolicLinkFileFilter}.
*/
public class SymbolicLinkFileFilterTest {
+ public static final String TARGET_SHORT_NAME = "SLFF_Target";
+ public static final String TARGET_EXT = ".txt";
+ public static final String TARGET_NAME = TARGET_SHORT_NAME + TARGET_EXT;
+ 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 linkDirPath; // symbolic link to hardDirectory
+ private static File linkDirFile;
+ private static SymbolicLinkFileFilter filter;
+ private static File missingFile; // non-existent file
+
+ 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() {
+ @Override
+ boolean isSymbolicLink(final Path filePath) {
+ return filePath.toFile().exists() && filePath.toString().contains("Link"); // Mock test
+ }
+ };
+ }
+
+ /**
+ * <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 {
+ BiFunction<Path, Path, Path> symbolicLinkCreator;
+
+ // We can't create symbolic links on Windows without admin privileges,
+ // so iff that's our OS, we mock them.
+ String os = System.getProperty("os.name");
+ if (os.toLowerCase().contains("windows")) {
+ symbolicLinkCreator = SymbolicLinkFileFilterTest::createMockSymbolicLink;
+ filter = createMockFilter();
+ } else {
+ symbolicLinkCreator = SymbolicLinkFileFilterTest::createRealSymbolicLink;
+ filter = SymbolicLinkFileFilter.INSTANCE;
+ }
+
+ testTargetFile = File.createTempFile(TARGET_SHORT_NAME, TARGET_EXT);
+ testTargetPath = testTargetFile.toPath();
+ parentDirectoryFile = testTargetFile.getParentFile();
+ // parent directory
+ Path parentDirectoryPath = parentDirectoryFile.toPath();
+ linkName = "SLFF_LinkTo" + testTargetFile.getName();
+ testLinkPath = symbolicLinkCreator.apply(parentDirectoryPath.resolve(linkName), testTargetPath);
+ testLinkFile = testLinkPath.toFile();
+ targetDirPath = Files.createDirectories(parentDirectoryPath.resolve(DIRECTORY_NAME));
+ targetDirFile = targetDirPath.toFile();
+ linkDirPath = symbolicLinkCreator.apply(parentDirectoryPath.resolve(DIRECTORY_LINK_NAME), targetDirPath);
+ linkDirFile = linkDirPath.toFile();
+ missingFile = new File(parentDirectoryPath.toFile(), MISSING);
+ }
+
+ @AfterAll
+ static void tearDown() {
+ // Fortunately, delete() doesn't throw an exception if the file doesn't exist.
+ linkDirFile.delete();
+ targetDirFile.delete();
+ testLinkFile.delete();
+ testTargetFile.delete();
+ }
+
@Test
public void testSymbolicLinkFileFilter() {
assertEquals(FileVisitResult.TERMINATE, SymbolicLinkFileFilter.INSTANCE.accept(PathUtils.current(), null));
+ }
+
+ @Test
+ public void testFileFilter_HardFile() {
+ assertFalse(filter.accept(testTargetFile));
+ }
+
+ @Test
+ public void testFileFilter_Link() {
+ assertTrue(filter.accept(testLinkFile));
+ }
+
+ @Test
+ public void testFileFilter_HardDirectory() {
+ assertFalse(filter.accept(targetDirFile));
+ }
+
+ @Test
+ public void testFileFilter_PathLink() {
+ assertTrue(filter.accept(linkDirFile));
+ }
+
+ @Test
+ public void testFileFilter_missingFile() {
+ assertFalse(filter.accept(missingFile));
+ }
+
+ @Test
+ public void testFileNameFilter_HardFile() {
+ assertFalse(filter.accept(parentDirectoryFile, TARGET_NAME));
+ }
+
+ @Test
+ public void testFileNameFilter_Link() {
+ assertTrue(filter.accept(parentDirectoryFile, linkName));
+ }
+
+ @Test
+ public void testFileNameFilter_HardDirectory() {
+ assertFalse(filter.accept(parentDirectoryFile, DIRECTORY_NAME));
+ }
+
+ @Test
+ public void testFileNameFilter_PathLink() {
+ assertTrue(filter.accept(parentDirectoryFile, DIRECTORY_LINK_NAME));
+ }
+
+ @Test
+ public void testFileNameFilter_missingFile() {
+ assertFalse(filter.accept(parentDirectoryFile, MISSING));
+ }
+ @Test
+ public void testPathFilter_HardFile() {
+ assertEquals(FileVisitResult.TERMINATE, filter.accept(testTargetPath, null));
+ }
+
+ @Test
+ public void testPathFilter_Link() {
+ assertEquals(FileVisitResult.CONTINUE, filter.accept(testLinkPath, null));
+ }
+
+ @Test
+ public void testPathFilter_HardDirectory() {
+ assertEquals(FileVisitResult.TERMINATE, filter.accept(targetDirPath, null));
+ }
+
+ @Test
+ public void testPathFilter_PathLink() {
+ assertEquals(FileVisitResult.CONTINUE, filter.accept(linkDirPath, null));
+ }
+
+ @Test
+ public void testPathFilter_missingFile() {
+ assertEquals(FileVisitResult.TERMINATE, filter.accept(missingFile.toPath(), null));
}
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
