From 253ce8b7fe060172e943d7938063f870664b4dd8 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Thu, 4 Oct 2018 15:55:43 -0700 Subject: [PATCH 1/4] walkFiles consistently relative or absolute 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 --- .../nio/CloudStorageFileSystemProvider.java | 13 +++- .../storage/contrib/nio/it/ITGcsNio.java | 75 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java index 26dc440480a4..d9489155adfb 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java @@ -102,15 +102,19 @@ private static class LazyPathIterator extends AbstractIterator { private final Filter filter; private final CloudStorageFileSystem fileSystem; private final String prefix; + // whether to make the paths absolute before returning them. + private final boolean absolutePaths; LazyPathIterator(CloudStorageFileSystem fileSystem, String prefix, Iterator blobIterator, - Filter filter) { + Filter filter, + boolean absolutePaths) { this.prefix = prefix; this.blobIterator = blobIterator; this.filter = filter; this.fileSystem = fileSystem; + this.absolutePaths = absolutePaths; } @Override @@ -123,6 +127,9 @@ protected Path computeNext() { continue; } if (filter.accept(path)) { + if (absolutePaths) { + return path.toAbsolutePath(); + } return path; } } catch (IOException ex) { @@ -769,7 +776,7 @@ public void createDirectory(Path dir, FileAttribute... attrs) { } @Override - public DirectoryStream newDirectoryStream(Path dir, final Filter filter) { + public DirectoryStream newDirectoryStream(final Path dir, final Filter filter) { final CloudStoragePath cloudPath = CloudStorageUtil.checkPath(dir); checkNotNull(filter); initStorage(); @@ -793,7 +800,7 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter() { @Override public Iterator iterator() { - return new LazyPathIterator(cloudPath.getFileSystem(), prefix, blobIterator, filter); + return new LazyPathIterator(cloudPath.getFileSystem(), prefix, blobIterator, filter, dir.isAbsolute()); } @Override diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 8057cbe50ee8..127dc6caab3d 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -17,6 +17,7 @@ package com.google.cloud.storage.contrib.nio.it; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assertWithMessage; import static java.nio.charset.StandardCharsets.UTF_8; import com.google.api.client.http.HttpResponseException; @@ -53,10 +54,12 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; +import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; +import java.nio.file.spi.FileSystemProvider; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -538,6 +541,52 @@ public void testListFiles() throws IOException { } } + @Test + public void testRelativityOfResolve() throws IOException { + try (FileSystem fs = getTestBucket()) { + Path abs1 = fs.getPath("/dir"); + Path abs2 = abs1.resolve("subdir/"); + Path rel1 = fs.getPath("dir"); + Path rel2 = rel1.resolve("subdir/"); + // children of absolute paths are absolute, + // children of relative paths are relative. + assertThat(abs1.isAbsolute()).isTrue(); + assertThat(abs2.isAbsolute()).isTrue(); + assertThat(rel1.isAbsolute()).isFalse(); + assertThat(rel2.isAbsolute()).isFalse(); + } + } + + @Test + public void testWalkFiles() throws IOException { + try (FileSystem fs = getTestBucket()) { + List goodPaths = new ArrayList<>(); + List paths = new ArrayList<>(); + goodPaths.add(fs.getPath("dir/angel")); + goodPaths.add(fs.getPath("dir/alone")); + paths.add(fs.getPath("dir/dir2/another_angel")); + paths.add(fs.getPath("atroot")); + paths.addAll(goodPaths); + goodPaths.add(fs.getPath("dir/dir2/")); + for (Path path : paths) { + fillFile(storage, BUCKET, path.toString(), SML_SIZE); + } + // Given a relative path as starting point, walkFileTree must return only relative paths. + List relativePaths = postTraversalWalker.walkFileTree(fs.getPath("dir/")); + for (Path p : relativePaths) { + assertWithMessage("Should have been relative: " + p.toString()).that(p.isAbsolute()).isFalse(); + } + assertThat(relativePaths.size()).isEqualTo(5); + + // Given an absolute path as starting point, walkFileTree must return only relative paths. + List absolutePaths = postTraversalWalker.walkFileTree(fs.getPath("/dir/")); + for (Path p : absolutePaths) { + assertWithMessage("Should have been absolute: " + p.toString()).that(p.isAbsolute()).isTrue(); + } + assertThat(absolutePaths.size()).isEqualTo(5); + } + } + @Test public void testDeleteRecursive() throws IOException { @@ -620,6 +669,32 @@ private String randomSuffix() { return "-" + rnd.nextInt(99999); } + private static class postTraversalWalker extends SimpleFileVisitor { + private List paths = new ArrayList<>(); + + // Traverse the tree, return the list of files and folders. + static public ImmutableList walkFileTree(Path start) throws IOException { + postTraversalWalker walker = new postTraversalWalker(); + Files.walkFileTree(start, walker); + return walker.getPaths(); + } + + @Override + public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) throws IOException { + paths.add(file); + return FileVisitResult.CONTINUE; + } + + @Override + public FileVisitResult postVisitDirectory(Path dir, IOException exc) throws IOException { + paths.add(dir); + return FileVisitResult.CONTINUE; + } + + public ImmutableList getPaths() { + return ImmutableList.copyOf(paths); + } + } private CloudStorageFileSystem getTestBucket() throws IOException { // in typical usage we use the single-argument version of forBucket From 618291478f8d1b4f3f8e770fe84102bb11efb8f1 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 5 Oct 2018 09:12:36 -0700 Subject: [PATCH 2/4] address issues found by linter --- .../com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 127dc6caab3d..716590a08fef 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -54,12 +54,10 @@ import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; -import java.nio.file.Paths; import java.nio.file.SimpleFileVisitor; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; -import java.nio.file.spi.FileSystemProvider; import java.util.ArrayList; import java.util.Arrays; import java.util.List; @@ -670,7 +668,7 @@ private String randomSuffix() { } private static class postTraversalWalker extends SimpleFileVisitor { - private List paths = new ArrayList<>(); + private final List paths = new ArrayList<>(); // Traverse the tree, return the list of files and folders. static public ImmutableList walkFileTree(Path start) throws IOException { From b27fb4003fde1fb2ca901772f88ee9f07a368a16 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 8 Oct 2018 09:57:02 -0700 Subject: [PATCH 3/4] rename PostTraversalWalker --- .../com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 716590a08fef..9a739bc10199 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -570,14 +570,14 @@ public void testWalkFiles() throws IOException { fillFile(storage, BUCKET, path.toString(), SML_SIZE); } // Given a relative path as starting point, walkFileTree must return only relative paths. - List relativePaths = postTraversalWalker.walkFileTree(fs.getPath("dir/")); + List relativePaths = PostTraversalWalker.walkFileTree(fs.getPath("dir/")); for (Path p : relativePaths) { assertWithMessage("Should have been relative: " + p.toString()).that(p.isAbsolute()).isFalse(); } assertThat(relativePaths.size()).isEqualTo(5); // Given an absolute path as starting point, walkFileTree must return only relative paths. - List absolutePaths = postTraversalWalker.walkFileTree(fs.getPath("/dir/")); + List absolutePaths = PostTraversalWalker.walkFileTree(fs.getPath("/dir/")); for (Path p : absolutePaths) { assertWithMessage("Should have been absolute: " + p.toString()).that(p.isAbsolute()).isTrue(); } @@ -667,12 +667,12 @@ private String randomSuffix() { return "-" + rnd.nextInt(99999); } - private static class postTraversalWalker extends SimpleFileVisitor { + private static class PostTraversalWalker extends SimpleFileVisitor { private final List paths = new ArrayList<>(); // Traverse the tree, return the list of files and folders. static public ImmutableList walkFileTree(Path start) throws IOException { - postTraversalWalker walker = new postTraversalWalker(); + PostTraversalWalker walker = new PostTraversalWalker(); Files.walkFileTree(start, walker); return walker.getPaths(); } From 7e822db618729e32f2711ac034f72be03d49778e Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 8 Oct 2018 16:51:10 -0700 Subject: [PATCH 4/4] apply reviewer feedback --- .../java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java index 9a739bc10199..6a9df1551e00 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/it/ITGcsNio.java @@ -565,7 +565,6 @@ public void testWalkFiles() throws IOException { paths.add(fs.getPath("dir/dir2/another_angel")); paths.add(fs.getPath("atroot")); paths.addAll(goodPaths); - goodPaths.add(fs.getPath("dir/dir2/")); for (Path path : paths) { fillFile(storage, BUCKET, path.toString(), SML_SIZE); } @@ -574,6 +573,8 @@ public void testWalkFiles() throws IOException { for (Path p : relativePaths) { assertWithMessage("Should have been relative: " + p.toString()).that(p.isAbsolute()).isFalse(); } + // The 5 paths are: + // dir/, dir/angel, dir/alone, dir/dir2/, dir/dir2/another_angel. assertThat(relativePaths.size()).isEqualTo(5); // Given an absolute path as starting point, walkFileTree must return only relative paths.