Re: [IO] 2 Patches for SymbolicLinkFileFilter
This should likely be one PR, see comments in the PR. In general, when you submit a fix in a PR, it should contain changes to both main and test. The test changed or added to the test folder should fail without the changes to main. Gary On Wed, Apr 19, 2023, 01:30 Miguel Muñoz wrote: > Today I resubmitted both changes as Pull Requests. > > — Miguel Muñoz > > On Tue, Apr 18, 2023 at 3:44 AM Gary Gregory > wrote: > > > Thanks Miguel, > > > > Please provide changes as PRs on GiHub. This is the best way to go IMO > > because it runs code against all tests on all configured OSs and Java > > versions, and provides the least friction for managing changes. > > > > TY! > > Gary > > > > On Tue, Apr 18, 2023, 04:20 Miguel Muñoz wrote: > > > > > 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 > > > > > > > > > > > > - > > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > > For additional commands, e-mail: dev-h...@commons.apache.org > > >
Re: [IO] 2 Patches for SymbolicLinkFileFilter
Today I resubmitted both changes as Pull Requests. — Miguel Muñoz On Tue, Apr 18, 2023 at 3:44 AM Gary Gregory wrote: > Thanks Miguel, > > Please provide changes as PRs on GiHub. This is the best way to go IMO > because it runs code against all tests on all configured OSs and Java > versions, and provides the least friction for managing changes. > > TY! > Gary > > On Tue, Apr 18, 2023, 04:20 Miguel Muñoz wrote: > > > 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 > > > > > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org >
Re: [IO] 2 Patches for SymbolicLinkFileFilter
Thanks Miguel, Please provide changes as PRs on GiHub. This is the best way to go IMO because it runs code against all tests on all configured OSs and Java versions, and provides the least friction for managing changes. TY! Gary On Tue, Apr 18, 2023, 04:20 Miguel Muñoz wrote: > 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 > > > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org
Re: [IO] 2 Patches for SymbolicLinkFileFilter
> > Or as a Github PR: > +1 On Tue, 18 Apr 2023 at 11:18, sebb wrote: > Or as a Github PR: > > https://github.com/apache/commons-io/pulls > > These systems are easier for developers to track than emails, which > can get overlooked. > > On Tue, 18 Apr 2023 at 09:54, Gilles Sadowski > wrote: > > > > Hi. > > > > Le mar. 18 avr. 2023 à 10:20, Miguel Muñoz a > écrit : > > > > > > Developers, > > > > > > (I'm new to Apache, > > > > Welcome. > > > > > so I hope this is the right way to submit patches.) > > > > The usual way would be to file a report on the bug-tracking system: > > https://issues.apache.org/jira/projects/IO > > and upload patches there. > > > > Regards, > > Gilles > > > > > [...] > > > > - > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
Re: [IO] 2 Patches for SymbolicLinkFileFilter
Or as a Github PR: https://github.com/apache/commons-io/pulls These systems are easier for developers to track than emails, which can get overlooked. On Tue, 18 Apr 2023 at 09:54, Gilles Sadowski wrote: > > Hi. > > Le mar. 18 avr. 2023 à 10:20, Miguel Muñoz a écrit : > > > > Developers, > > > > (I'm new to Apache, > > Welcome. > > > so I hope this is the right way to submit patches.) > > The usual way would be to file a report on the bug-tracking system: > https://issues.apache.org/jira/projects/IO > and upload patches there. > > Regards, > Gilles > > > [...] > > - > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
Re: [IO] 2 Patches for SymbolicLinkFileFilter
Hi. Le mar. 18 avr. 2023 à 10:20, Miguel Muñoz a écrit : > > Developers, > > (I'm new to Apache, Welcome. > so I hope this is the right way to submit patches.) The usual way would be to file a report on the bug-tracking system: https://issues.apache.org/jira/projects/IO and upload patches there. Regards, Gilles > [...] - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[IO] 2 Patches for SymbolicLinkFileFilter
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.
*
* 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
