SymbolicLinkFileFilter

[Miguel Muñoz <sw...@gmail.com>]

It's not clear what exactly this filter is supposed to do. From its
behavior, I gather that it's supposed to distinguish files from
directories, treating Symbolic links as ordinary files. There are two
reasons why this isn't clear.

1. This behavior (usually) seems identical to that of a filter that just
relies on File.isDirectory().
2. The current behavior is inconsistent:

I made a test directory with four entries: a file, a directory, and a
symbolic link to each.

I wrote code using SymbolicLinkFileFilter.INSTANCE as a FileFilter, a
FileNameFilter, and a PathFilter. I created a second filter that relied on
File.isDirectory(). The two filters behaved the same for three of the
files, but differed for the symbolic link to the directory. (I'm not sure
if the filter should treat that as a file or a directory, since it's not a
directory, but it resolves as one.)

Used as a FileFilter and a FileNameFilter, the accept() methods returned
false. Used as a PathFilter, it returned FileVisitResult.CONTINUE, which
essentially means true.

Looking through the code, the difference in behavior is apparently because
the call to resolve the PathFilter is the only one that calls
Files.isSymbolicLink().

I could fix this, but I don't know which behavior is correct.

I'll send you my test code if you want it.

— Miguel Muñoz

Re: [IO] SymbolicLinkFileFilter

[Miguel Muñoz <sw...@gmail.com>]

Here's a better idea for how to write the unit test.

First, we put something in the SymbolicLinkFileFilter class like this:

public class SymbolicLinkFileFilter {
  private static LinkTester linkTester = new LinkTester(); // defined below
  static setLinkTesterForUnitTests(LinkTester unitTestLinkTester) { //
package access!
    linkTester = unitTestLinkTester;
  }
  // rest of class code here

  // everywhere the class needs to test if a file is a Symbolic link
calls this method:
  private static class LinkTester {
    public boolean isSymbolicLink(File file) { ... }
  }
}

Now we can write a unit test that replaces the LinkTester with a custom
tester that overrides the behavior on Windows only. This way, our unit test
just verifies the logic of the SymbolicLinkFileFilter class. We're still
stuck with the problem of how to test the default LinkTester on Windows,
but we've made the problem a lot smaller.

— Miguel


On Sun, Apr 16, 2023 at 1:10 PM Miguel Muñoz <sw...@gmail.com> wrote:

> "The class per its javadoc filters for symbolic links on files."
>
> It sounds like it's only supposed to pass files that are symbolic links.
> If that's the idea, the class doesn't work at all. If not, we need a
> clearer description.
>
> As for unit tests, maybe the test should detect the Windows OS and just
> return. To the Javadocs, we would add a note-to-developers explaining that
> unit tests can't work on Windows, and describing how to test the class
> separately. The alternative is a lot more complicated: On windows, without
> admin rights, it could return an Error with a message saying that it needs
> Admin Rights to run properly. And maybe we could allow the user to create a
> symbolic link somewhere, and define an environment variable with the path
> to that file which lets the test run properly. (I know that's very kludgy,
> but this situation can't be solved normally.) I can't write this test
> because I don't have a Windows machine.
>
> — Miguel
>
> On Fri, Apr 14, 2023 at 7:10 AM Gary Gregory <ga...@gmail.com>
> wrote:
>
>> Hi Miguel,
>>
>> Thank you for reviewing. A better unit test would help of course ;-) but
>> testing links can be a pain as the JVM requires admin rights on Windows 10
>> at least. This might not be an issue for GitHub but be aware of it if you
>> run builds locally.
>>
>> The class per its javadoc filters for symbolic links on files.
>>
>> Gary
>>
>> On Fri, Apr 14, 2023, 10:05 Gary Gregory <ga...@gmail.com> wrote:
>>
>> > Let's assume this is for IO, not VFS; in the future, please prefix email
>> > subjects with
>> > [IO] or [VFS] or whatever Commons component is being discussed.
>> >
>> > TY!
>> > Gary
>> >
>> > On Thu, Apr 13, 2023, 01:59 Miguel Muñoz <sw...@gmail.com>
>> wrote:
>> >
>> >> It's not clear what exactly this filter is supposed to do. From its
>> >> behavior, I gather that it's supposed to distinguish files from
>> >> directories, treating Symbolic links as ordinary files. There are two
>> >> reasons why this isn't clear.
>> >>
>> >> 1. This behavior (usually) seems identical to that of a filter that
>> just
>> >> relies on File.isDirectory().
>> >> 2. The current behavior is inconsistent:
>> >>
>> >> I made a test directory with four entries: a file, a directory, and a
>> >> symbolic link to each.
>> >>
>> >> I wrote code using SymbolicLinkFileFilter.INSTANCE as a FileFilter, a
>> >> FileNameFilter, and a PathFilter. I created a second filter that
>> relied on
>> >> File.isDirectory(). The two filters behaved the same for three of the
>> >> files, but differed for the symbolic link to the directory. (I'm not
>> sure
>> >> if the filter should treat that as a file or a directory, since it's
>> not a
>> >> directory, but it resolves as one.)
>> >>
>> >> Used as a FileFilter and a FileNameFilter, the accept() methods
>> returned
>> >> false. Used as a PathFilter, it returned FileVisitResult.CONTINUE,
>> which
>> >> essentially means true.
>> >>
>> >> Looking through the code, the difference in behavior is apparently
>> because
>> >> the call to resolve the PathFilter is the only one that calls
>> >> Files.isSymbolicLink().
>> >>
>> >> I could fix this, but I don't know which behavior is correct.
>> >>
>> >> I'll send you my test code if you want it.
>> >>
>> >> — Miguel Muñoz
>> >>
>> >
>>
>

Re: [IO] SymbolicLinkFileFilter

[Miguel Muñoz <sw...@gmail.com>]

"The class per its javadoc filters for symbolic links on files."

It sounds like it's only supposed to pass files that are symbolic links. If
that's the idea, the class doesn't work at all. If not, we need a clearer
description.

As for unit tests, maybe the test should detect the Windows OS and just
return. To the Javadocs, we would add a note-to-developers explaining that
unit tests can't work on Windows, and describing how to test the class
separately. The alternative is a lot more complicated: On windows, without
admin rights, it could return an Error with a message saying that it needs
Admin Rights to run properly. And maybe we could allow the user to create a
symbolic link somewhere, and define an environment variable with the path
to that file which lets the test run properly. (I know that's very kludgy,
but this situation can't be solved normally.) I can't write this test
because I don't have a Windows machine.

— Miguel

On Fri, Apr 14, 2023 at 7:10 AM Gary Gregory <ga...@gmail.com> wrote:

> Hi Miguel,
>
> Thank you for reviewing. A better unit test would help of course ;-) but
> testing links can be a pain as the JVM requires admin rights on Windows 10
> at least. This might not be an issue for GitHub but be aware of it if you
> run builds locally.
>
> The class per its javadoc filters for symbolic links on files.
>
> Gary
>
> On Fri, Apr 14, 2023, 10:05 Gary Gregory <ga...@gmail.com> wrote:
>
> > Let's assume this is for IO, not VFS; in the future, please prefix email
> > subjects with
> > [IO] or [VFS] or whatever Commons component is being discussed.
> >
> > TY!
> > Gary
> >
> > On Thu, Apr 13, 2023, 01:59 Miguel Muñoz <sw...@gmail.com> wrote:
> >
> >> It's not clear what exactly this filter is supposed to do. From its
> >> behavior, I gather that it's supposed to distinguish files from
> >> directories, treating Symbolic links as ordinary files. There are two
> >> reasons why this isn't clear.
> >>
> >> 1. This behavior (usually) seems identical to that of a filter that just
> >> relies on File.isDirectory().
> >> 2. The current behavior is inconsistent:
> >>
> >> I made a test directory with four entries: a file, a directory, and a
> >> symbolic link to each.
> >>
> >> I wrote code using SymbolicLinkFileFilter.INSTANCE as a FileFilter, a
> >> FileNameFilter, and a PathFilter. I created a second filter that relied
> on
> >> File.isDirectory(). The two filters behaved the same for three of the
> >> files, but differed for the symbolic link to the directory. (I'm not
> sure
> >> if the filter should treat that as a file or a directory, since it's
> not a
> >> directory, but it resolves as one.)
> >>
> >> Used as a FileFilter and a FileNameFilter, the accept() methods returned
> >> false. Used as a PathFilter, it returned FileVisitResult.CONTINUE, which
> >> essentially means true.
> >>
> >> Looking through the code, the difference in behavior is apparently
> because
> >> the call to resolve the PathFilter is the only one that calls
> >> Files.isSymbolicLink().
> >>
> >> I could fix this, but I don't know which behavior is correct.
> >>
> >> I'll send you my test code if you want it.
> >>
> >> — Miguel Muñoz
> >>
> >
>

Re: [IO] SymbolicLinkFileFilter

[Gary Gregory <ga...@gmail.com>]

Hi Miguel,

Thank you for reviewing. A better unit test would help of course ;-) but
testing links can be a pain as the JVM requires admin rights on Windows 10
at least. This might not be an issue for GitHub but be aware of it if you
run builds locally.

The class per its javadoc filters for symbolic links on files.

Gary

On Fri, Apr 14, 2023, 10:05 Gary Gregory <ga...@gmail.com> wrote:

> Let's assume this is for IO, not VFS; in the future, please prefix email
> subjects with
> [IO] or [VFS] or whatever Commons component is being discussed.
>
> TY!
> Gary
>
> On Thu, Apr 13, 2023, 01:59 Miguel Muñoz <sw...@gmail.com> wrote:
>
>> It's not clear what exactly this filter is supposed to do. From its
>> behavior, I gather that it's supposed to distinguish files from
>> directories, treating Symbolic links as ordinary files. There are two
>> reasons why this isn't clear.
>>
>> 1. This behavior (usually) seems identical to that of a filter that just
>> relies on File.isDirectory().
>> 2. The current behavior is inconsistent:
>>
>> I made a test directory with four entries: a file, a directory, and a
>> symbolic link to each.
>>
>> I wrote code using SymbolicLinkFileFilter.INSTANCE as a FileFilter, a
>> FileNameFilter, and a PathFilter. I created a second filter that relied on
>> File.isDirectory(). The two filters behaved the same for three of the
>> files, but differed for the symbolic link to the directory. (I'm not sure
>> if the filter should treat that as a file or a directory, since it's not a
>> directory, but it resolves as one.)
>>
>> Used as a FileFilter and a FileNameFilter, the accept() methods returned
>> false. Used as a PathFilter, it returned FileVisitResult.CONTINUE, which
>> essentially means true.
>>
>> Looking through the code, the difference in behavior is apparently because
>> the call to resolve the PathFilter is the only one that calls
>> Files.isSymbolicLink().
>>
>> I could fix this, but I don't know which behavior is correct.
>>
>> I'll send you my test code if you want it.
>>
>> — Miguel Muñoz
>>
>

Re: SymbolicLinkFileFilter

[Gary Gregory <ga...@gmail.com>]

Let's assume this is for IO, not VFS; in the future, please prefix email
subjects with
[IO] or [VFS] or whatever Commons component is being discussed.

TY!
Gary

On Thu, Apr 13, 2023, 01:59 Miguel Muñoz <sw...@gmail.com> wrote:

> It's not clear what exactly this filter is supposed to do. From its
> behavior, I gather that it's supposed to distinguish files from
> directories, treating Symbolic links as ordinary files. There are two
> reasons why this isn't clear.
>
> 1. This behavior (usually) seems identical to that of a filter that just
> relies on File.isDirectory().
> 2. The current behavior is inconsistent:
>
> I made a test directory with four entries: a file, a directory, and a
> symbolic link to each.
>
> I wrote code using SymbolicLinkFileFilter.INSTANCE as a FileFilter, a
> FileNameFilter, and a PathFilter. I created a second filter that relied on
> File.isDirectory(). The two filters behaved the same for three of the
> files, but differed for the symbolic link to the directory. (I'm not sure
> if the filter should treat that as a file or a directory, since it's not a
> directory, but it resolves as one.)
>
> Used as a FileFilter and a FileNameFilter, the accept() methods returned
> false. Used as a PathFilter, it returned FileVisitResult.CONTINUE, which
> essentially means true.
>
> Looking through the code, the difference in behavior is apparently because
> the call to resolve the PathFilter is the only one that calls
> Files.isSymbolicLink().
>
> I could fix this, but I don't know which behavior is correct.
>
> I'll send you my test code if you want it.
>
> — Miguel Muñoz
>