From 5db08fe7b6ec84c7a50946e3f239c44811876f47 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Thu, 4 Oct 2018 17:05:45 -0700 Subject: [PATCH 01/10] Try harder to make believable directories Previous implementation of fake folders just looked for a trailing '/'. Now we also list the files with this prefix and use this to determine whether the input is a folder. This will detect 'dir' in addition to just 'dir/' before. This version will still say 'yes' to anything that ends in a slash, even if they don't exist, maintaining the previous behavior. --- .../nio/CloudStorageFileSystemProvider.java | 14 +++---- .../storage/contrib/nio/CloudStoragePath.java | 33 ++++++++++++++++- .../contrib/nio/testing/FakeStorageRpc.java | 14 +++++++ .../storage/contrib/nio/it/ITGcsNio.java | 37 +++++++++++++++++++ 4 files changed, 89 insertions(+), 9 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..712346631ee8 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 @@ -332,7 +332,7 @@ private SeekableByteChannel newReadChannel(Path path, Set } } CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories()) { + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { throw new CloudStoragePseudoDirectoryException(cloudPath); } return CloudStorageReadChannel.create( @@ -348,7 +348,7 @@ private SeekableByteChannel newWriteChannel(Path path, Set throws IOException { initStorage(); CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories()) { + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { throw new CloudStoragePseudoDirectoryException(cloudPath); } BlobId file = cloudPath.getBlobId(); @@ -439,7 +439,7 @@ public InputStream newInputStream(Path path, OpenOption... options) throws IOExc public boolean deleteIfExists(Path path) throws IOException { initStorage(); CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories()) { + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { // if the "folder" is empty then we're fine, otherwise complain // that we cannot act on folders. try (DirectoryStream paths = Files.newDirectoryStream(path)) { @@ -567,10 +567,10 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep "File systems associated with paths don't agree on pseudo-directories."); } } - if (fromPath.seemsLikeADirectoryAndUsePseudoDirectories()) { + if (fromPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { throw new CloudStoragePseudoDirectoryException(fromPath); } - if (toPath.seemsLikeADirectoryAndUsePseudoDirectories()) { + if (toPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { throw new CloudStoragePseudoDirectoryException(toPath); } @@ -665,7 +665,7 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories() ) { + if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { return; } boolean nullId; @@ -708,7 +708,7 @@ public A readAttributes( while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories() ) { + if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { @SuppressWarnings("unchecked") A result = (A) new CloudStoragePseudoDirectoryAttributes(cloudPath); return result; diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java index 4a74dcadbb6e..651ff54c948e 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java @@ -19,7 +19,10 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import com.google.api.gax.paging.Page; +import com.google.cloud.storage.Blob; import com.google.cloud.storage.BlobId; +import com.google.cloud.storage.Storage; import com.google.common.collect.UnmodifiableIterator; import java.io.File; @@ -83,8 +86,34 @@ boolean seemsLikeADirectory() { return path.seemsLikeADirectory(); } - boolean seemsLikeADirectoryAndUsePseudoDirectories() { - return path.seemsLikeADirectory() && fileSystem.config().usePseudoDirectories(); + boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { + if (!fileSystem.config().usePseudoDirectories()) { + return false; + } + if (path.seemsLikeADirectory()) { + return true; + } + // fancy case: the file name doesn't end in slash, but we've been asked to have pseudo dirs. + // Let's see if there are any files with this prefix. + if (null==storage) { + // we are in a context where we don't want to access the storage, so we conservatively + // say this isn't a directory. + return false; + } + Page list = storage.list( + this.bucket(), + Storage.BlobListOption.prefix(path.removeBeginningSeparator().toString()), + Storage.BlobListOption.pageSize(1)); + for (Blob b : list.getValues()) { + // if this blob starts with our prefix and then a slash, then prefix is indeed a folder! + if (null==b.getBlobId()) continue; + String name = b.getBlobId().getName(); + if (("/" + name).startsWith(this.path.toAbsolutePath() + "/")) { + return true; + } + } + // no match, so it's not a folder + return false; } @Override diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/testing/FakeStorageRpc.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/testing/FakeStorageRpc.java index d27664683db8..3bec1b9fd713 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/testing/FakeStorageRpc.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/testing/FakeStorageRpc.java @@ -124,6 +124,7 @@ public Tuple> list(String bucket, Map throws StorageException { String delimiter = null; String preprefix = ""; + long maxResults = Long.MAX_VALUE; for (Map.Entry e : options.entrySet()) { switch (e.getKey()) { case PREFIX: @@ -138,6 +139,9 @@ public Tuple> list(String bucket, Map case FIELDS: // ignore and return all the fields break; + case MAX_RESULTS: + maxResults = (Long) e.getValue(); + break; default: throw new UnsupportedOperationException("Unknown option: " + e.getKey()); } @@ -156,6 +160,16 @@ public Tuple> list(String bucket, Map values.add(so); } values.addAll(folders.values()); + + // truncate to max allowed length + if (values.size() > maxResults) { + List newValues = new ArrayList<>(); + for (int i=0; i < maxResults; i++) { + newValues.add(values.get(i)); + } + values = newValues; + } + // null cursor to indicate there is no more data (empty string would cause us to be called again). // The type cast seems to be necessary to help Java's typesystem remember that collections are iterable. return Tuple.of(null, (Iterable) values); 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..6207fbdb41e1 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 @@ -582,6 +582,43 @@ public void testListFilesInRootDirectory() throws IOException { assertThat(objectNames).containsExactly(BIG_FILE, SML_FILE); } + @Test + public void testFakeDirectories() throws IOException { + try (FileSystem fs = getTestBucket()) { + List paths = new ArrayList<>(); + paths.add(fs.getPath("dir/angel")); + paths.add(fs.getPath("dir/deeper/fish")); + for (Path path : paths) { + fillFile(storage, BUCKET, path.toString(), SML_SIZE); + } + + // ends with slash, must be a directory + assertThat(Files.isDirectory(fs.getPath("dir/"))).isTrue(); + // files are not directories + assertThat(Files.isDirectory(fs.getPath("dir/angel"))).isFalse(); + // directories are recognized even without the trailing "/" + assertThat(Files.isDirectory(fs.getPath("dir"))).isTrue(); + // also works for absolute paths + assertThat(Files.isDirectory(fs.getPath("/dir"))).isTrue(); + // non-existent files are not directories (but they don't make us crash) + assertThat(Files.isDirectory(fs.getPath("di"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dirs"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dir/deep"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper/fi"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper/fi"))).isFalse(); + // also works for subdirectories + assertThat(Files.isDirectory(fs.getPath("dir/deeper/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper"))).isTrue(); + + // clean up + for (Path path : paths) { + Files.delete(path); + } + } + } + /** * Delete the given directory and all of its contents if non-empty. * @param directory the directory to delete From 04a70103658f52e70f73c28e56977c0111d236db Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Thu, 4 Oct 2018 17:22:17 -0700 Subject: [PATCH 02/10] Clarify documentation and add test case --- .../storage/contrib/nio/CloudStorageConfiguration.java | 6 +++++- .../com/google/cloud/storage/contrib/nio/it/ITGcsNio.java | 3 +++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java index b4a586785013..a0e699b07005 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageConfiguration.java @@ -52,7 +52,11 @@ public abstract class CloudStorageConfiguration { public abstract boolean stripPrefixSlash(); /** - * Returns {@code true} if paths with a trailing slash should be treated as fake directories. + * Returns {@code true} if directories and paths with a trailing slash should be treated as fake + * directories. + * + *

With this feature, if file "foo/bar.txt" exists then both "foo" and "foo/" will be + * treated as if they were existing directories. */ public abstract boolean usePseudoDirectories(); 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 6207fbdb41e1..3697fe34702d 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 @@ -611,6 +611,9 @@ public void testFakeDirectories() throws IOException { assertThat(Files.isDirectory(fs.getPath("dir/deeper"))).isTrue(); assertThat(Files.isDirectory(fs.getPath("/dir/deeper/"))).isTrue(); assertThat(Files.isDirectory(fs.getPath("/dir/deeper"))).isTrue(); + // the root folder is a directory + assertThat(Files.isDirectory(fs.getPath("/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath(""))).isTrue(); // clean up for (Path path : paths) { From 01daca9833474347beceffeed0b75d07ee8afacf Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 5 Oct 2018 09:07:28 -0700 Subject: [PATCH 03/10] add braces for linter --- .../google/cloud/storage/contrib/nio/CloudStoragePath.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java index 651ff54c948e..739e86c5158d 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java @@ -106,8 +106,13 @@ boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { Storage.BlobListOption.pageSize(1)); for (Blob b : list.getValues()) { // if this blob starts with our prefix and then a slash, then prefix is indeed a folder! - if (null==b.getBlobId()) continue; + if (null==b.getBlobId()) { + continue; + } String name = b.getBlobId().getName(); + if (null == name) { + continue; + } if (("/" + name).startsWith(this.path.toAbsolutePath() + "/")) { return true; } From ae2996d7034a004886de2a727fd22e4c1e07923d Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 8 Oct 2018 10:18:09 -0700 Subject: [PATCH 04/10] apply reviewer suggestion --- .../google/cloud/storage/contrib/nio/CloudStoragePath.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java index 739e86c5158d..4f325e2e28ab 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java @@ -95,7 +95,7 @@ boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { } // fancy case: the file name doesn't end in slash, but we've been asked to have pseudo dirs. // Let's see if there are any files with this prefix. - if (null==storage) { + if (storage == null) { // we are in a context where we don't want to access the storage, so we conservatively // say this isn't a directory. return false; @@ -106,11 +106,11 @@ boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { Storage.BlobListOption.pageSize(1)); for (Blob b : list.getValues()) { // if this blob starts with our prefix and then a slash, then prefix is indeed a folder! - if (null==b.getBlobId()) { + if (b.getBlobId() == null) { continue; } String name = b.getBlobId().getName(); - if (null == name) { + if (name == null) { continue; } if (("/" + name).startsWith(this.path.toAbsolutePath() + "/")) { From be0a391e070447570d9428c0a5df780744e4970c Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 8 Oct 2018 16:35:37 -0700 Subject: [PATCH 05/10] fix listing folders that don't end in / --- .../nio/CloudStorageFileSystemProvider.java | 8 +++++++- .../cloud/storage/contrib/nio/it/ITGcsNio.java | 17 ++++++++--------- 2 files changed, 15 insertions(+), 10 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 712346631ee8..728ccef76124 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 @@ -778,7 +778,13 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter dirList; if (isNullOrEmpty(userProject)) { dirList = storage.list(cloudPath.bucket(), 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 3697fe34702d..894043214048 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; @@ -520,17 +521,15 @@ public void testListFiles() throws IOException { } List got = new ArrayList<>(); - for (Path path : Files.newDirectoryStream(fs.getPath("/dir/"))) { - got.add(path); + for (String folder : new String[]{"/dir/", "/dir", "dir/", "dir"}) { + got.clear(); + for (Path path : Files.newDirectoryStream(fs.getPath(folder))) { + got.add(path); + } + assertWithMessage("Listing " + folder+": ").that(got).containsExactlyElementsIn(goodPaths); } - assertThat(got).containsExactlyElementsIn(goodPaths); - // Must also work with relative path - got.clear(); - for (Path path : Files.newDirectoryStream(fs.getPath("dir/"))) { - got.add(path); - } - assertThat(got).containsExactlyElementsIn(goodPaths); + // clean up for (Path path : paths) { Files.delete(path); From 78485f68360271a7e53f2857a77e3da1fce3f23b Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Mon, 8 Oct 2018 16:38:49 -0700 Subject: [PATCH 06/10] fix comment --- .../storage/contrib/nio/CloudStorageFileSystemProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 728ccef76124..6e8c9f8fb4d4 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 @@ -780,7 +780,7 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter Date: Tue, 9 Oct 2018 10:31:09 -0700 Subject: [PATCH 07/10] Perf, tests, comments Improved performance by moving the pseudodir code to only run on file not found, leaving the common-case path to run faster. Added tests for pseudodirs, and one in LocalStorage too. Improved comments following reviewer suggestions. --- .../nio/CloudStorageFileSystemProvider.java | 56 ++++++++++++++----- .../storage/contrib/nio/CloudStoragePath.java | 11 +++- .../CloudStorageFileSystemProviderTest.java | 49 ++++++++++++++++ .../storage/contrib/nio/it/ITGcsNio.java | 35 +++++++----- 4 files changed, 120 insertions(+), 31 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 6e8c9f8fb4d4..845c5483cf43 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 @@ -34,6 +34,7 @@ import com.google.cloud.storage.StorageOptions; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; +import com.google.common.base.Strings; import com.google.common.collect.AbstractIterator; import com.google.common.net.UrlEscapers; import com.google.common.primitives.Ints; @@ -665,9 +666,6 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { - return; - } boolean nullId; if (isNullOrEmpty(userProject)) { nullId = storage.get( @@ -682,6 +680,11 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { == null; } if (nullId) { + if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { + // there is no such file, but we're not signalling error because the + // path is a pseudo-directory. + return; + } throw new NoSuchFileException(path.toString()); } break; @@ -689,6 +692,14 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { // Will rethrow a StorageException if all retries/reopens are exhausted retryHandler.handleStorageException(exs); // we're being aggressive by retrying even on scenarios where we'd normally reopen. + } catch (IllegalArgumentException exs) { + if ( CloudStorageUtil.checkPath(path).seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { + // there is no such file, but we're not signalling error because the + // path is a pseudo-directory. + return; + } + // Other cause for IAE, forward the exception. + throw exs; } } } @@ -708,19 +719,34 @@ public A readAttributes( while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage) ) { - @SuppressWarnings("unchecked") - A result = (A) new CloudStoragePseudoDirectoryAttributes(cloudPath); - return result; - } - BlobInfo blobInfo; - if (isNullOrEmpty(userProject)) { - blobInfo = storage.get(cloudPath.getBlobId()); - } else { - blobInfo = storage.get(cloudPath.getBlobId(), BlobGetOption.userProject(userProject)); + BlobInfo blobInfo = null; + try { + BlobId blobId = cloudPath.getBlobId(); + // Null or empty name won't give us a file, so skip. But perhaps it's a pseudodir. + if (!Strings.isNullOrEmpty(blobId.getName())) { + if (isNullOrEmpty(userProject)) { + blobInfo = storage.get(blobId); + } else { + blobInfo = storage.get(blobId, BlobGetOption.userProject(userProject)); + } + } + } catch (IllegalArgumentException ex) { + // the path may be invalid but look like a folder. In that case, return a folder. + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + @SuppressWarnings("unchecked") + A result = (A) new CloudStoragePseudoDirectoryAttributes(cloudPath); + return result; + } + // No? Propagate. + throw ex; } // null size indicate a file that we haven't closed yet, so GCS treats it as not there yet. if ( null == blobInfo || blobInfo.getSize() == null ) { + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + @SuppressWarnings("unchecked") + A result = (A) new CloudStoragePseudoDirectoryAttributes(cloudPath); + return result; + } throw new NoSuchFileException( "gs://" + cloudPath.getBlobId().getBucket() + "/" + cloudPath.getBlobId().getName()); } @@ -778,10 +804,10 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter list = storage.list( this.bucket(), - Storage.BlobListOption.prefix(path.removeBeginningSeparator().toString()), + Storage.BlobListOption.prefix(prefix), + // we only look at the first result, so no need for a bigger page. Storage.BlobListOption.pageSize(1)); for (Blob b : list.getValues()) { // if this blob starts with our prefix and then a slash, then prefix is indeed a folder! @@ -117,7 +124,7 @@ boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { return true; } } - // no match, so it's not a folder + // no match, so it's not a directory return false; } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java index 2e4329f19585..d07bec2a045d 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java @@ -38,6 +38,7 @@ import org.junit.runner.RunWith; import org.junit.runners.JUnit4; +import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.net.URI; @@ -54,6 +55,7 @@ import java.nio.file.OpenOption; import java.nio.file.Path; import java.nio.file.Paths; +import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -397,6 +399,53 @@ public void testExists_trailingSlash_disablePseudoDirectories() throws Exception } } + @Test + public void testFakeDirectories() throws IOException { + try (FileSystem fs = forBucket("military")) { + List paths = new ArrayList<>(); + paths.add(fs.getPath("dir/angel")); + paths.add(fs.getPath("dir/deepera")); + paths.add(fs.getPath("dir/deeperb")); + paths.add(fs.getPath("dir/deeper_")); + paths.add(fs.getPath("dir/deeper.sea/hasfish")); + paths.add(fs.getPath("dir/deeper/fish")); + for (Path path : paths) { + Files.createFile(path); + } + + // ends with slash, must be a directory + assertThat(Files.isDirectory(fs.getPath("dir/"))).isTrue(); + // files are not directories + assertThat(Files.exists(fs.getPath("dir/angel"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/angel"))).isFalse(); + // directories are recognized even without the trailing "/" + assertThat(Files.isDirectory(fs.getPath("dir"))).isTrue(); + // also works for absolute paths + assertThat(Files.isDirectory(fs.getPath("/dir"))).isTrue(); + // non-existent files are not directories (but they don't make us crash) + assertThat(Files.isDirectory(fs.getPath("di"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dirs"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dir/deep"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper/fi"))).isFalse(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper/fi"))).isFalse(); + // also works for subdirectories + assertThat(Files.isDirectory(fs.getPath("dir/deeper/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("/dir/deeper"))).isTrue(); + // dot and .. folders are directories + assertThat(Files.isDirectory(fs.getPath("dir/deeper/."))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper/.."))).isTrue(); + // dots in the name are fine + assertThat(Files.isDirectory(fs.getPath("dir/deeper.sea/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper.sea"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper.seax"))).isFalse(); + // the root folder is a directory + assertThat(Files.isDirectory(fs.getPath("/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath(""))).isTrue(); + } + } + @Test public void testDelete() throws Exception { Files.write(Paths.get(URI.create("gs://love/fashion")), "(✿◕ ‿◕ )ノ".getBytes(UTF_8)); 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 894043214048..f08ae6f3e1fe 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,20 +565,15 @@ public void testListFilesInRootDirectory() throws IOException { BUCKET, CloudStorageConfiguration.builder().permitEmptyPathComponents(true) .build(), storageOptions); - // test absolute path - Path rootPath = fs.getPath(""); - List objectNames = new ArrayList(); - for (Path path : Files.newDirectoryStream(rootPath)) { - objectNames.add(path.toString()); - } - assertThat(objectNames).containsExactly(BIG_FILE, SML_FILE); - - // test relative path - rootPath = fs.getPath("."); - for (Path path : Files.newDirectoryStream(rootPath)) { - objectNames.add(path.toString()); + // test absolute path, relative path. + for (String check : new String[]{".", "/", ""}) { + Path rootPath = fs.getPath(check); + List objectNames = new ArrayList<>(); + for (Path path : Files.newDirectoryStream(rootPath)) { + objectNames.add(path.toString()); + } + assertWithMessage("Listing " + check + ": ").that(objectNames).containsExactly(BIG_FILE, SML_FILE); } - assertThat(objectNames).containsExactly(BIG_FILE, SML_FILE); } @Test @@ -586,14 +581,19 @@ public void testFakeDirectories() throws IOException { try (FileSystem fs = getTestBucket()) { List paths = new ArrayList<>(); paths.add(fs.getPath("dir/angel")); + paths.add(fs.getPath("dir/deepera")); + paths.add(fs.getPath("dir/deeperb")); + paths.add(fs.getPath("dir/deeper_")); + paths.add(fs.getPath("dir/deeper.sea/hasfish")); paths.add(fs.getPath("dir/deeper/fish")); for (Path path : paths) { - fillFile(storage, BUCKET, path.toString(), SML_SIZE); + Files.createFile(path); } // ends with slash, must be a directory assertThat(Files.isDirectory(fs.getPath("dir/"))).isTrue(); // files are not directories + assertThat(Files.exists(fs.getPath("dir/angel"))).isTrue(); assertThat(Files.isDirectory(fs.getPath("dir/angel"))).isFalse(); // directories are recognized even without the trailing "/" assertThat(Files.isDirectory(fs.getPath("dir"))).isTrue(); @@ -610,6 +610,13 @@ public void testFakeDirectories() throws IOException { assertThat(Files.isDirectory(fs.getPath("dir/deeper"))).isTrue(); assertThat(Files.isDirectory(fs.getPath("/dir/deeper/"))).isTrue(); assertThat(Files.isDirectory(fs.getPath("/dir/deeper"))).isTrue(); + // dot and .. folders are directories + assertThat(Files.isDirectory(fs.getPath("dir/deeper/."))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper/.."))).isTrue(); + // dots in the name are fine + assertThat(Files.isDirectory(fs.getPath("dir/deeper.sea/"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper.sea"))).isTrue(); + assertThat(Files.isDirectory(fs.getPath("dir/deeper.seax"))).isFalse(); // the root folder is a directory assertThat(Files.isDirectory(fs.getPath("/"))).isTrue(); assertThat(Files.isDirectory(fs.getPath(""))).isTrue(); From 08714ce981dceed80b1c03c56d3d4de9d65cf56f Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 9 Oct 2018 10:36:31 -0700 Subject: [PATCH 08/10] stylistic change --- .../storage/contrib/nio/CloudStorageFileSystemProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 845c5483cf43..25f193afa0f2 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 @@ -808,7 +808,7 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter dirList; From a49c585153c986af4cec6d781ccf066f6e2bac81 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 9 Oct 2018 16:57:40 -0700 Subject: [PATCH 09/10] Optimize Remove check for path without '/' in places where it's not required. --- .../contrib/nio/CloudStorageFileSystemProvider.java | 12 ++++++++---- .../cloud/storage/contrib/nio/CloudStoragePath.java | 4 ++++ 2 files changed, 12 insertions(+), 4 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 25f193afa0f2..b0df6d5e5d6d 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 @@ -333,7 +333,8 @@ private SeekableByteChannel newReadChannel(Path path, Set } } CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + // passing false since we just want to check if it ends with / + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(null)) { throw new CloudStoragePseudoDirectoryException(cloudPath); } return CloudStorageReadChannel.create( @@ -349,7 +350,7 @@ private SeekableByteChannel newWriteChannel(Path path, Set throws IOException { initStorage(); CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); - if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories(null)) { throw new CloudStoragePseudoDirectoryException(cloudPath); } BlobId file = cloudPath.getBlobId(); @@ -568,10 +569,13 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep "File systems associated with paths don't agree on pseudo-directories."); } } - if (fromPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + // We refuse to use paths that end in '/'. If the user puts a folder name + // but without the '/', they'll get whatever error GCS will return normally + // (if any). + if (fromPath.seemsLikeADirectoryAndUsePseudoDirectories(null)) { throw new CloudStoragePseudoDirectoryException(fromPath); } - if (toPath.seemsLikeADirectoryAndUsePseudoDirectories(storage)) { + if (toPath.seemsLikeADirectoryAndUsePseudoDirectories(null)) { throw new CloudStoragePseudoDirectoryException(toPath); } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java index b8aa28a1351a..03c4a9afe86c 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java @@ -86,6 +86,10 @@ boolean seemsLikeADirectory() { return path.seemsLikeADirectory(); } + // True if this path may be a directory (and pseudo-directories are enabled) + // Checks: + // 1) does the path end in / ? + // 2) (optional, if storage is set) is there a file whose name starts with path+/ ? boolean seemsLikeADirectoryAndUsePseudoDirectories(Storage storage) { if (!fileSystem.config().usePseudoDirectories()) { return false; From 59f6c5407cf9bdc058a4a340108da14aad68be2c Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Wed, 10 Oct 2018 13:42:37 -0700 Subject: [PATCH 10/10] cosmetic change for linter --- .../storage/contrib/nio/CloudStorageFileSystemProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 b0df6d5e5d6d..b8f8f32b8fed 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 @@ -727,7 +727,7 @@ public A readAttributes( try { BlobId blobId = cloudPath.getBlobId(); // Null or empty name won't give us a file, so skip. But perhaps it's a pseudodir. - if (!Strings.isNullOrEmpty(blobId.getName())) { + if (!isNullOrEmpty(blobId.getName())) { if (isNullOrEmpty(userProject)) { blobInfo = storage.get(blobId); } else {