walkFiles consistently relative or absolute

[jean-philippe-martin]

The expected behavior from Files.walkFileTree is that if the starting path is absolute, then all the visited paths are absolute. If the starting path is relative, then all the visited paths are relative.

This PR adds a test for this behavior, and shows that our current code
fails for an absolute path: it returns a mix of relative and absolute
paths.

This PR also adds a code change to fix the problem, so the actual
behavior matches what is expected.

This should fix issue #3772

[pongad]

@hzyi-google Do you also want to take a look?

[olifly]

Hi.

I would think that these modifications, along with the ones in #3775, would allow the following unit test to pass:

  @Test
  public void testWalkFileTree() throws IOException {
    Path baseDir = Paths.get(URI.create("gs://bucket/liverpool"));
    //Base directory with an ending forwards slash is not passing either
    //Path baseDir = Paths.get(URI.create("gs://bucket/liverpool/"));
    Path file1 = baseDir.resolve("firmino.player");
    Path file2 = baseDir.resolve("salah.player");
    Files.createFile(file1);
    Files.createFile(file2);

    Set<Path> expectedDirs = Sets.newHashSet(baseDir);
    Set<Path> expectedFiles = Sets.newHashSet(file1,file2);
    final Set<Path> foundDirs = Sets.newHashSet();
    final Set<Path> foundFiles = Sets.newHashSet();

    FileVisitor<Path> testVisitor = new SimpleFileVisitor<Path>()  {

      @Override
      public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
        assertThat(foundDirs).doesNotContain(dir);
        foundDirs.add(dir);
        return FileVisitResult.CONTINUE;
      }

      @Override
      public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
        foundFiles.add(file);
        return FileVisitResult.CONTINUE;
      }

      @Override
      public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
        assertThat(foundDirs).contains(dir);
        return FileVisitResult.CONTINUE;
      }
    };

    Files.walkFileTree(baseDir,Sets.newHashSet(FileVisitOption.FOLLOW_LINKS),Integer.MAX_VALUE,testVisitor);

    assertThat(foundFiles).containsExactlyElementsIn(expectedFiles);
    assertThat(foundDirs).containsExactlyElementsIn(expectedDirs);

  }

Files.walkFileTree visits the Basedir twice, first without an ending slash, and then with an ending slash.

Best regards,
Ólafur Haukur Flygenring

[jean-philippe-martin]

@olifly I see there is a bug in this code snippet: newHashSet expects an iterator, not a single element. Instead of a set that contains the path, you get a set that contains a list of the elements that compose the path. So for example /foo/bar/baz/ will get you a set with foo, bar, and baz.

Once I fixed the code snippet, it passed with just the code in this PR.

[olifly]

@olifly I see there is a bug in this code snippet: newHashSet expects an iterator, not a single element. Instead of a set that contains the path, you get a set that contains a list of the elements that compose the path. So for example /foo/bar/baz/ will get you a set with foo, bar, and baz.

Once I fixed the code snippet, it passed with just the code in this PR.

Hi.

Maybe it's the combination of these two pull requests, but changing the test to insert the expected Paths one at a time into the Sets is not making it pass for me. Files.walkFileTree is still calling preVisitDirectory twice for the base dir, once without a forward slash and once with it.

Here's my modified code

@Test
  public void testWalkFileTree() throws IOException {
    Path baseDir = Paths.get(URI.create("gs://bucket/liverpool"));
    Path file1 = baseDir.resolve("firmino.player");
    Path file2 = baseDir.resolve("salah.player");
    Files.createFile(file1);
    Files.createFile(file2);

    Set<Path> expectedDirs = Sets.newHashSet();
    expectedDirs.add(baseDir);
    Set<Path> expectedFiles = Sets.newHashSet();
    expectedFiles.add(file1);
    expectedFiles.add(file2);

    final Set<Path> foundDirs = Sets.newHashSet();
    final Set<Path> foundFiles = Sets.newHashSet();

    FileVisitor<Path> testVisitor = new SimpleFileVisitor<Path>()  {

      @Override
      public FileVisitResult preVisitDirectory(Path dir, BasicFileAttributes attrs) {
        assertThat(foundDirs).doesNotContain(dir);
        foundDirs.add(dir);
        return FileVisitResult.CONTINUE;
      }

      @Override
      public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
        foundFiles.add(file);
        return FileVisitResult.CONTINUE;
      }

      @Override
      public FileVisitResult postVisitDirectory(Path dir, IOException exc) {
        assertThat(foundDirs).contains(dir);
        return FileVisitResult.CONTINUE;
      }
    };

    Files.walkFileTree(baseDir,Sets.newHashSet(FileVisitOption.FOLLOW_LINKS),Integer.MAX_VALUE,testVisitor);

    assertThat(foundFiles).containsExactlyElementsIn(expectedFiles);
    assertThat(foundDirs).containsExactlyElementsIn(expectedDirs);

  }

The test passes if the baseDir has a forward slash, gs://bucket/liverpool/, but fails on: gs://bucket/liverpool

I'm pretty sure Liverpool is not to blame though :)

JDK Used: 1.8, u 181.

Best Regards,
Ólafur Haukur Flygenring

[chingor13]

Looks straight-forward - just a question about the clarity of the test.

[jean-philippe-martin]

@olifly you're right, the "/liverpool" case (where we have to merge the two PRs) still didn't work right. I updated #3775 to fixt listing of a pretend-folder that doesn't end in slash (and added a test case for it). Thank you very much! Once you add that change, I expect your code should work.

[jean-philippe-martin]

@chingor13 the functions returns the folders and files traversed, so the 5 paths are:
dir/, dir/angel, dir/alone, dir/dir2/, dir/dir2/another_angel.

I added them as comment to the code in case another reader wonders too.

(sorry, for some reason GitHub isn't letting me reply to your comment directly).

[olifly]

@jean-philippe-martin - The changes you did to pull request #3775 did indeed solve the issue. Thanks a bunch.

Can't wait for these changes to be merged upstream :)

Best Regards
Ólafur Haukur Flygenring.

[jean-philippe-martin]

@olifly thanks for checking, that's great! I'm glad this works for you now. I see the reviewers have already merged in this PR so we're halfway there!

[jean-philippe-martin]

Thank you @chingor13 !