From f393680be34939b9ff86ae0948cbc63e5095bc53 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 26 Jun 2018 11:27:39 -0700 Subject: [PATCH 1/6] Requester-Pays bucket support. Code and integration test. To use this feature, set the "userProject" setting in the CloudStorageConfiguration. Optionally, set autoDetectRequesterPays to avoid automatically unset userProject if the bucket isn't requester-pays. --- .../nio/CloudStorageConfiguration.java | 45 ++++- .../contrib/nio/CloudStorageFileSystem.java | 22 ++- .../nio/CloudStorageFileSystemProvider.java | 138 +++++++++++-- .../contrib/nio/CloudStorageReadChannel.java | 53 +++-- .../nio/CloudStorageReadChannelTest.java | 8 +- .../storage/contrib/nio/it/ITGcsNio.java | 181 +++++++++++++++++- 6 files changed, 410 insertions(+), 37 deletions(-) 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 0469af72f35a..5cf374f4f4ce 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 @@ -17,6 +17,7 @@ package com.google.cloud.storage.contrib.nio; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Preconditions.checkNotNull; import com.google.auto.value.AutoValue; @@ -65,6 +66,25 @@ public abstract class CloudStorageConfiguration { */ public abstract int maxChannelReopens(); + /** + * Returns the project to be billed when accessing buckets. Leave empty for normal semantics, + * set to bill that project (project you own) for all accesses. This is required for accessing + * requester-pays buckets. This value cannot be null. + */ + public abstract String userProject(); + + /** + * Returns whether userProject will be ignored for non-requester-pays buckets. That is, + * if false (the default value), setting userProject causes that project to be billed + * regardless of whether the bucket is requester-pays or not. If true, setting + * userProject will only cause that project to be billed when the project is requester-pays. + * + * Setting this will cause the bucket to be accessed when the CloudStorageFileSystem object + * is created. + */ + public abstract boolean autoDetectRequesterPays(); + + /** * Creates a new builder, initialized with the following settings: * @@ -90,6 +110,8 @@ public static final class Builder { private boolean usePseudoDirectories = true; private int blockSize = CloudStorageFileSystem.BLOCK_SIZE_DEFAULT; private int maxChannelReopens = 0; + private String userProject=""; + private boolean autoDetectRequesterPays=false; /** * Changes current working directory for new filesystem. This defaults to the root directory. @@ -99,6 +121,7 @@ public static final class Builder { * @throws IllegalArgumentException if {@code path} is not absolute. */ public Builder workingDirectory(String path) { + checkNotNull(path); checkArgument(UnixPath.getPath(false, path).isAbsolute(), "not absolute: %s", path); workingDirectory = path; return this; @@ -147,6 +170,16 @@ public Builder maxChannelReopens(int value) { return this; } + public Builder userProject(String value) { + userProject = checkNotNull(value); + return this; + } + + public Builder autoDetectRequesterPays(boolean value) { + autoDetectRequesterPays = value; + return this; + } + /** * Creates new instance without destroying builder. */ @@ -157,7 +190,9 @@ public CloudStorageConfiguration build() { stripPrefixSlash, usePseudoDirectories, blockSize, - maxChannelReopens); + maxChannelReopens, + userProject, + autoDetectRequesterPays); } Builder(CloudStorageConfiguration toModify) { @@ -167,6 +202,8 @@ public CloudStorageConfiguration build() { usePseudoDirectories = toModify.usePseudoDirectories(); blockSize = toModify.blockSize(); maxChannelReopens = toModify.maxChannelReopens(); + userProject = toModify.userProject(); + autoDetectRequesterPays = toModify.autoDetectRequesterPays(); } Builder() {} @@ -201,6 +238,12 @@ static private CloudStorageConfiguration fromMap(Builder builder, Map case "maxChannelReopens": builder.maxChannelReopens((Integer) entry.getValue()); break; + case "userProject": + builder.userProject((String) entry.getValue()); + break; + case "autoDetectRequesterPays": + builder.autoDetectRequesterPays((Boolean) entry.getValue()); + break; default: throw new IllegalArgumentException(entry.getKey()); } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 04745080791f..bb9081189ccd 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -33,6 +33,7 @@ import java.nio.file.WatchService; import java.nio.file.attribute.FileTime; import java.nio.file.attribute.UserPrincipalLookupService; +import java.util.HashMap; import java.util.Objects; import java.util.Set; @@ -112,8 +113,9 @@ public static CloudStorageFileSystem forBucket(String bucket) { public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) { checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); + checkNotNull(config); return new CloudStorageFileSystem( - new CloudStorageFileSystemProvider(), bucket, checkNotNull(config)); + new CloudStorageFileSystemProvider(config.userProject()), bucket, config); } /** @@ -136,15 +138,29 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig @Nullable StorageOptions storageOptions) { checkArgument(!bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - return new CloudStorageFileSystem(new CloudStorageFileSystemProvider(storageOptions), + return new CloudStorageFileSystem(new CloudStorageFileSystemProvider(config.userProject(), storageOptions), bucket, checkNotNull(config)); } CloudStorageFileSystem( CloudStorageFileSystemProvider provider, String bucket, CloudStorageConfiguration config) { checkArgument(!bucket.isEmpty(), "bucket"); - this.provider = provider; this.bucket = bucket; + if (config.autoDetectRequesterPays()) { + if (config.userProject().isEmpty()) { + throw new IllegalArgumentException("If autoDetectRequesterPays is set, then userProject must be set too."); + } + // detect whether we want to pay for these accesses or not. + if (!provider.isRequesterPays(bucket)) { + // update config (just to ease debugging, we're not actually using config.userProject later. + HashMap disableUserProject = new HashMap<>(); + disableUserProject.put("userProject", ""); + config = CloudStorageConfiguration.fromMap(config, disableUserProject); + // update the provider (this is the most important bit) + provider = provider.disableRequesterPays(); + } + } + this.provider = provider; this.config = config; } 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 215829cddca1..bdf273a1fc3e 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 @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import static com.google.common.base.Strings.isNullOrEmpty; +import com.google.api.gax.paging.Page; import com.google.auto.service.AutoService; import com.google.cloud.storage.Acl; import com.google.cloud.storage.Blob; @@ -27,11 +28,12 @@ import com.google.cloud.storage.BlobInfo; import com.google.cloud.storage.CopyWriter; import com.google.cloud.storage.Storage; +import com.google.cloud.storage.Storage.BlobGetOption; +import com.google.cloud.storage.Storage.BlobSourceOption; import com.google.cloud.storage.StorageException; import com.google.cloud.storage.StorageOptions; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.MoreObjects; -import com.google.common.base.Throwables; import com.google.common.collect.AbstractIterator; import com.google.common.net.UrlEscapers; import com.google.common.primitives.Ints; @@ -88,7 +90,9 @@ public final class CloudStorageFileSystemProvider extends FileSystemProvider { private Storage storage; - private StorageOptions storageOptions; + final private StorageOptions storageOptions; + // if non-empty, we pay via this one (TODO) + final private String userProject; // used only when we create a new instance of CloudStorageFileSystemProvider. private static StorageOptions futureStorageOptions; @@ -168,11 +172,23 @@ public static void setDefaultCloudStorageConfiguration(@Nullable CloudStorageCon * @see CloudStorageFileSystem#forBucket(String) */ public CloudStorageFileSystemProvider() { - this(futureStorageOptions); + this("", futureStorageOptions); } - CloudStorageFileSystemProvider(@Nullable StorageOptions gcsStorageOptions) { + /** + * Internal constructor to use the user-provided default config, and a given userProject setting. + */ + CloudStorageFileSystemProvider(String userProject) { + this(userProject, futureStorageOptions); + } + + /** + * Internal constructor, fully configurable. Note that null options means + * to use the system defaults (NOT the user-provided ones). + */ + CloudStorageFileSystemProvider(String userProject, @Nullable StorageOptions gcsStorageOptions) { this.storageOptions = gcsStorageOptions; + this.userProject = userProject; } // Initialize this.storage, once. This may throw an exception if default authentication @@ -252,6 +268,16 @@ public CloudStoragePath getPath(String uriInStringForm) { return getPath(URI.create(escaped)); } + /** + * Open a file for reading or writing. + * To read receiver-pays buckets, specify the BlobSourceOption.userProject option. + * + * @param path: the path to the file to open or create + * @param options: options specifying how the file is opened, e.g. StandardOpenOption.WRITE or BlobSourceOption.userProject + * @param attrs: (not supported, values will be ignored) + * @return + * @throws IOException + */ @Override public SeekableByteChannel newByteChannel( Path path, Set options, FileAttribute... attrs) throws IOException { @@ -270,6 +296,7 @@ private SeekableByteChannel newReadChannel(Path path, Set throws IOException { initStorage(); int maxChannelReopens = CloudStorageUtil.getMaxChannelReopensFromPath(path); + List blobSourceOptions = new ArrayList<>(); for (OpenOption option : options) { if (option instanceof StandardOpenOption) { switch ((StandardOpenOption) option) { @@ -293,6 +320,8 @@ private SeekableByteChannel newReadChannel(Path path, Set } } else if (option instanceof OptionMaxChannelReopens) { maxChannelReopens = ((OptionMaxChannelReopens) option).maxChannelReopens(); + } else if (option instanceof BlobSourceOption) { + blobSourceOptions.add((BlobSourceOption)option); } else { throw new UnsupportedOperationException(option.toString()); } @@ -301,7 +330,13 @@ private SeekableByteChannel newReadChannel(Path path, Set if (cloudPath.seemsLikeADirectoryAndUsePseudoDirectories()) { throw new CloudStoragePseudoDirectoryException(cloudPath); } - return CloudStorageReadChannel.create(storage, cloudPath.getBlobId(), 0, maxChannelReopens); + return CloudStorageReadChannel.create( + storage, + cloudPath.getBlobId(), + 0, + maxChannelReopens, + userProject, + blobSourceOptions.toArray(new BlobSourceOption[0])); } private SeekableByteChannel newWriteChannel(Path path, Set options) @@ -361,6 +396,9 @@ private SeekableByteChannel newWriteChannel(Path path, Set throw new UnsupportedOperationException(option.toString()); } } + if (!userProject.isEmpty()) { + writeOptions.add(Storage.BlobWriteOption.userProject(userProject)); + } if (!metas.isEmpty()) { infoBuilder.setMetadata(metas); @@ -413,7 +451,11 @@ public boolean deleteIfExists(Path path) throws IOException { // Loop will terminate via an exception if all retries are exhausted while (true) { try { - return storage.delete(cloudPath.getBlobId()); + if (userProject.isEmpty()) { + return storage.delete(cloudPath.getBlobId()); + } else { + return storage.delete(cloudPath.getBlobId(), Storage.BlobSourceOption.userProject(userProject)); + } } catch (StorageException exs) { // Will rethrow a StorageException if all retries/reopens are exhausted retryHandler.handleStorageException(exs); @@ -532,7 +574,12 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep while (true) { try { if ( wantCopyAttributes ) { - BlobInfo blobInfo = storage.get(fromPath.getBlobId()); + BlobInfo blobInfo; + if (userProject.isEmpty()) { + blobInfo = storage.get(fromPath.getBlobId()); + } else { + blobInfo = storage.get(fromPath.getBlobId(), BlobGetOption.userProject(userProject)); + } if ( null == blobInfo ) { throw new NoSuchFileException(fromPath.toString()); } @@ -560,6 +607,11 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep } else { copyReqBuilder = copyReqBuilder.setTarget(tgtInfo, Storage.BlobTargetOption.doesNotExist()); } + if (!fromPath.getFileSystem().config().userProject().isEmpty()) { + copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(fromPath.getFileSystem().config().userProject())); + } else if (!toPath.getFileSystem().config().userProject().isEmpty()) { + copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(toPath.getFileSystem().config().userProject())); + } CopyWriter copyWriter = storage.copy(copyReqBuilder.build()); copyWriter.getResult(); break; @@ -605,14 +657,28 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(path)); // Loop will terminate via an exception if all retries are exhausted + BlobGetOption[] options; + while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); if ( cloudPath.seemsLikeADirectoryAndUsePseudoDirectories() ) { return; } - if ( storage.get(cloudPath.getBlobId(), Storage.BlobGetOption.fields(Storage.BlobField.ID)) - == null ) { + boolean nullId; + if (userProject.isEmpty()) { + nullId = storage.get( + cloudPath.getBlobId(), + Storage.BlobGetOption.fields(Storage.BlobField.ID)) + == null; + } else { + nullId = storage.get( + cloudPath.getBlobId(), + Storage.BlobGetOption.fields(Storage.BlobField.ID), + Storage.BlobGetOption.userProject(userProject)) + == null; + } + if (nullId) { throw new NoSuchFileException(path.toString()); } break; @@ -644,7 +710,12 @@ public A readAttributes( A result = (A) new CloudStoragePseudoDirectoryAttributes(cloudPath); return result; } - BlobInfo blobInfo = storage.get(cloudPath.getBlobId()); + BlobInfo blobInfo; + if (userProject.isEmpty()) { + blobInfo = storage.get(cloudPath.getBlobId()); + } else { + blobInfo = storage.get(cloudPath.getBlobId(), BlobGetOption.userProject(userProject)); + } // 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 ) { throw new NoSuchFileException( @@ -705,9 +776,17 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter blobIterator = storage.list(cloudPath.bucket(), - Storage.BlobListOption.prefix(prefix), Storage.BlobListOption.currentDirectory(), - Storage.BlobListOption.fields()).iterateAll().iterator(); + Page dirList; + if (userProject.isEmpty()) { + dirList = storage.list(cloudPath.bucket(), + Storage.BlobListOption.prefix(prefix), Storage.BlobListOption.currentDirectory(), + Storage.BlobListOption.fields()); + } else { + dirList = storage.list(cloudPath.bucket(), + Storage.BlobListOption.prefix(prefix), Storage.BlobListOption.currentDirectory(), + Storage.BlobListOption.fields(), Storage.BlobListOption.userProject(userProject)); + } + final Iterator blobIterator = dirList.iterateAll().iterator(); return new DirectoryStream() { @Override public Iterator iterator() { @@ -761,6 +840,35 @@ public String toString() { return MoreObjects.toStringHelper(this).add("storage", storage).toString(); } + /** + * @param bucketName - the name of the bucket to check + * @return whether requester pays is enabled for that bucket + */ + public boolean isRequesterPays(String bucketName) { + initStorage(); + try { + // instead of true/false, this method returns true/null. + Boolean isRP = storage.get(bucketName).requesterPays(); + return isRP != null && isRP.booleanValue(); + } catch (StorageException sex) { + if (sex.getCode() == 400 && sex.getMessage().contains("Bucket is requester pays")) { + return true; + } + throw sex; + } + } + + /** + * Returns a NEW CloudStorageFileSystemProvider identical to this one, but with + * requester pays disabled. + * + * Perhaps you want to call this is you realize you'll be working on a bucket that is + * not requester-pays. + */ + public CloudStorageFileSystemProvider disableRequesterPays() { + return new CloudStorageFileSystemProvider("", this.storageOptions); + } + private IOException asIoException(StorageException oops) { // RPC API can only throw StorageException, but CloudStorageFileSystemProvider // can only throw IOException. Square peg, round hole. @@ -775,7 +883,9 @@ private IOException asIoException(StorageException oops) { throw new FileAlreadyExistsException(((FileAlreadyExistsException) cause).getReason()); } // fallback - Throwables.propagateIfInstanceOf(oops.getCause(), IOException.class); + if (cause != null && cause instanceof IOException) { + return (IOException)cause; + } } catch (IOException okEx) { return okEx; } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java index eb9a8c9e0a22..4bcad6233a12 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java @@ -20,9 +20,15 @@ import com.google.cloud.storage.BlobId; import com.google.cloud.storage.BlobInfo; import com.google.cloud.storage.Storage; +import com.google.cloud.storage.Storage.BlobSourceOption; import com.google.cloud.storage.StorageException; import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableList; +import com.google.common.collect.Lists; + +import java.util.ArrayList; +import java.util.List; import javax.annotation.CheckReturnValue; import javax.annotation.concurrent.ThreadSafe; import java.io.IOException; @@ -54,6 +60,8 @@ final class CloudStorageReadChannel implements SeekableByteChannel { final int maxChannelReopens; // max # of times we may retry a GCS operation final int maxRetries; + // open options, we keep them around for reopens. + final BlobSourceOption[] blobSourceOptions; private ReadChannel channel; private long position; private long size; @@ -62,33 +70,49 @@ final class CloudStorageReadChannel implements SeekableByteChannel { private Long generation; /** - * @param maxChannelReopens max number of times to try re-opening the channel if it closes on us unexpectedly. + * @param maxChannelReopens max number of times to try re-opening the channel if it closes on us + * unexpectedly. + * @param blobSourceOptions BlobSourceOption.userProject if you want to pay the charges (required + * for requester-pays buckets). Note: + * Buckets that have Requester Pays disabled still accept requests that include a billing + * project, and charges are applied to the billing project supplied in the request. + * Consider any billing implications prior to including a billing project in all of your + * requests. + * Source: https://cloud.google.com/storage/docs/requester-pays + * @param userProject: the project you want billed (set this for requester-pays buckets). Leave + * empty otherwise. */ @CheckReturnValue @SuppressWarnings("resource") - static CloudStorageReadChannel create(Storage gcsStorage, BlobId file, long position, int maxChannelReopens) + static CloudStorageReadChannel create(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, String userProject, BlobSourceOption... blobSourceOptions) throws IOException { - return new CloudStorageReadChannel(gcsStorage, file, position, maxChannelReopens); + return new CloudStorageReadChannel(gcsStorage, file, position, maxChannelReopens, userProject, blobSourceOptions); } - private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, int maxChannelReopens) throws IOException { + private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, String userProject, BlobSourceOption... blobSourceOptions) throws IOException { this.gcsStorage = gcsStorage; this.file = file; this.position = position; this.maxChannelReopens = maxChannelReopens; this.maxRetries = Math.max(3, maxChannelReopens); - fetchSize(gcsStorage, file); + // get the generation, enshrine that in our options + fetchSize(gcsStorage, userProject, file); + List options = Lists.newArrayList(blobSourceOptions); + if (null != generation) { + options.add(Storage.BlobSourceOption.generationMatch(generation)); + } + if (!userProject.isEmpty()) { + options.add(BlobSourceOption.userProject(userProject)); + } + this.blobSourceOptions = (BlobSourceOption[]) options.toArray(new BlobSourceOption[0]); + // innerOpen checks that it sees the same generation as fetchSize did, // which ensure the file hasn't changed. innerOpen(); } private void innerOpen() throws IOException { - if (null != generation) { - this.channel = gcsStorage.reader(file, Storage.BlobSourceOption.generationMatch(generation)); - } else { - this.channel = gcsStorage.reader(file); - } + this.channel = gcsStorage.reader(file, blobSourceOptions); if (position > 0) { channel.seek(position); } @@ -183,12 +207,17 @@ private void checkOpen() throws ClosedChannelException { } } - private long fetchSize(Storage gcsStorage, BlobId file) throws IOException { + private long fetchSize(Storage gcsStorage, String userProject, BlobId file) throws IOException { final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(maxRetries, maxChannelReopens); while (true) { try { - BlobInfo blobInfo = gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE)); + BlobInfo blobInfo; + if (userProject.isEmpty()) { + blobInfo = gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE)); + } else { + blobInfo = gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE), Storage.BlobGetOption.userProject(userProject)); + } if ( blobInfo == null ) { throw new NoSuchFileException(String.format("gs://%s/%s", file.getBucket(), file.getName())); } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java index bccab0514092..98d84acc296b 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java @@ -31,6 +31,10 @@ import com.google.cloud.storage.Storage; import com.google.cloud.storage.StorageException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; import org.junit.Before; import org.junit.Rule; import org.junit.Test; @@ -67,7 +71,7 @@ public void before() throws IOException { when(gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE))).thenReturn(metadata); when(gcsStorage.reader(file, Storage.BlobSourceOption.generationMatch(2L))).thenReturn(gcsChannel); when(gcsChannel.isOpen()).thenReturn(true); - chan = CloudStorageReadChannel.create(gcsStorage, file, 0, 1); + chan = CloudStorageReadChannel.create(gcsStorage, file, 0, 1, ""); verify(gcsStorage).get(eq(file), eq(Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE))); verify(gcsStorage).reader(eq(file), eq(Storage.BlobSourceOption.generationMatch(2L))); } @@ -208,7 +212,7 @@ public void testChannelPositionDoesNotGetTruncatedToInt() throws IOException { // Invoke CloudStorageReadChannel.create() to trigger a call to the private // CloudStorageReadChannel.innerOpen() method, which does a seek on our gcsChannel. - CloudStorageReadChannel.create(gcsStorage, file, startPosition, 1); + CloudStorageReadChannel.create(gcsStorage, file, startPosition, 1, ""); // Confirm that our position did not overflow during the seek in CloudStorageReadChannel.innerOpen() verify(gcsChannel).seek(captor.capture()); 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 c7af8851b21f..33316e703f09 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 @@ -19,8 +19,13 @@ import static com.google.common.truth.Truth.assertThat; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.api.client.http.HttpResponseException; +import com.google.cloud.storage.Storage.BlobTargetOption; +import com.google.cloud.storage.Storage.BucketGetOption; +import com.google.cloud.storage.StorageException; import com.google.cloud.storage.contrib.nio.CloudStorageConfiguration; import com.google.cloud.storage.contrib.nio.CloudStorageFileSystem; +import com.google.cloud.storage.contrib.nio.CloudStoragePath; import com.google.common.collect.ImmutableList; import com.google.cloud.storage.BlobInfo; import com.google.cloud.storage.BucketInfo; @@ -43,12 +48,14 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadableByteChannel; import java.nio.channels.SeekableByteChannel; +import java.nio.file.CopyOption; import java.nio.file.FileSystem; import java.nio.file.FileVisitResult; import java.nio.file.Files; import java.nio.file.NoSuchFileException; import java.nio.file.Path; import java.nio.file.SimpleFileVisitor; +import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; import java.nio.file.attribute.BasicFileAttributes; import java.util.ArrayList; @@ -87,11 +94,14 @@ public class ITGcsNio { private static final Logger log = Logger.getLogger(ITGcsNio.class.getName()); private static final String BUCKET = RemoteStorageHelper.generateBucketName(); + private static final String REQUESTER_PAYS_BUCKET = RemoteStorageHelper.generateBucketName()+"_rp"; private static final String SML_FILE = "tmp-test-small-file.txt"; + private static final String TMP_FILE = "tmp/tmp-test-rnd-file.txt"; private static final int SML_SIZE = 100; private static final String BIG_FILE = "tmp-test-big-file.txt"; // it's big, relatively speaking. private static final int BIG_SIZE = 2 * 1024 * 1024 - 50; // arbitrary size that's not too round. private static final String PREFIX = "tmp-test-file"; + private static final String PROJECT = "jps-testing-project"; private static Storage storage; private static StorageOptions storageOptions; @@ -105,8 +115,11 @@ public static void beforeClass() throws IOException { storage = storageOptions.getService(); // create and populate test bucket storage.create(BucketInfo.of(BUCKET)); - fillFile(storage, SML_FILE, SML_SIZE); - fillFile(storage, BIG_FILE, BIG_SIZE); + fillFile(BUCKET, storage, SML_FILE, SML_SIZE); + fillFile(BUCKET, storage, BIG_FILE, BIG_SIZE); + BucketInfo requesterPaysBucket = BucketInfo.newBuilder(REQUESTER_PAYS_BUCKET).setRequesterPays(true).build(); + storage.create(requesterPaysBucket); + fillRequesterPaysFile(storage, SML_FILE, SML_SIZE); } @AfterClass @@ -123,10 +136,160 @@ private static byte[] randomContents(int size) { return bytes; } - private static void fillFile(Storage storage, String fname, int size) throws IOException { - storage.create(BlobInfo.newBuilder(BUCKET, fname).build(), randomContents(size)); + private static void fillFile(String bucket, Storage storage, String fname, int size) throws IOException { + storage.create(BlobInfo.newBuilder(bucket, fname).build(), randomContents(size)); + } + + private static void fillRequesterPaysFile(Storage storage, String fname, int size) throws IOException { + storage.create(BlobInfo.newBuilder(REQUESTER_PAYS_BUCKET, fname).build(), randomContents(size), + BlobTargetOption.userProject(PROJECT)); + } + + // ----------------------------------------------------------------------------------- + // Tests related to the "requester pays" feature + + @Test + public void testFileExistsRequesterPaysNoUserProject() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, ""); + Path path = testBucket.getPath(SML_FILE); + try { + // fails because we must pay for every access, including metadata. + Files.exists(path); + Assert.fail("It should have thrown an exception."); + } catch (StorageException sex) { + assertIsRequesterPaysException(sex); + } + } + + @Test + public void testFileExistsRequesterPays() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + Path path = testBucket.getPath(SML_FILE); + // should succeed because we specified a project + Files.exists(path); + } + + @Test + public void testFileExistsRequesterPaysWithAutodetect() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(true, PROJECT); + Path path = testBucket.getPath(SML_FILE); + // should succeed because we specified a project + Files.exists(path); + } + + @Test + public void testCantCreateWithoutUserProject() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, ""); + Path path = testBucket.getPath(SML_FILE); + try { + // fails + Files.write(path, "I would like to write".getBytes()); + Assert.fail("It should have thrown an exception."); + } catch (StorageException sex) { + assertIsRequesterPaysException(sex); + } + } + + @Test + public void testCanCreateWithUserProject() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + Path path = testBucket.getPath(TMP_FILE); + // should succeed because we specified a project + Files.write(path, "I would like to write, please?".getBytes()); + } + + @Test + public void testCantReadWithoutUserProject() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, ""); + Path path = testBucket.getPath(SML_FILE); + try { + // fails + Files.readAllBytes(path); + Assert.fail("It should have thrown an exception."); + } catch (StorageException sex) { + assertIsRequesterPaysException(sex); + } + } + + @Test + public void testCanReadWithUserProject() throws IOException { + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + Path path = testBucket.getPath(SML_FILE); + // should succeed because we specified a project + Files.readAllBytes(path); + } + + @Test + public void testCantCopyWithoutUserProject() throws IOException { + CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(false, ""); + CloudStorageFileSystem testBucket = getTestBucket(); + CloudStoragePath[] sources = new CloudStoragePath[] { testBucket.getPath(SML_FILE), testRPBucket.getPath(SML_FILE) }; + CloudStoragePath[] dests = new CloudStoragePath[] {testBucket.getPath(TMP_FILE), testRPBucket.getPath(TMP_FILE) }; + for (int s=0; s<2; s++) { + for (int d=0; d<2; d++) { + // normal to normal is out of scope of RP testing. + if (s==0 && d==0) continue; + String sdesc = (s==0?"normal bucket":"requester-pays bucket"); + String ddesc = (d==0?"normal bucket":"requester-pays bucket"); + try { + System.out.println("Copying from " + sdesc + " to " + ddesc); + Files.copy(sources[s], dests[d]); + Assert.fail("Shouldn't have been able to copy from " + sdesc + " to " + ddesc); + // for some reason this throws "GoogleJsonResponseException" instead of "StorageException" + // when going from requester pays bucket to requester pays bucket, but otherwise we get a + // normal StorageException. + } catch (HttpResponseException hex) { + Assert.assertEquals(hex.getStatusCode(), 400); + Assert.assertTrue(hex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); + } catch (StorageException sex) { + assertIsRequesterPaysException(sex); + } + } + } } + @Test + public void testCanCopyWithUserProject() throws IOException { + CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(false, PROJECT); + CloudStorageFileSystem testBucket = getTestBucket(); + CloudStoragePath[] sources = new CloudStoragePath[] { testBucket.getPath(SML_FILE), testRPBucket.getPath(SML_FILE) }; + CloudStoragePath[] dests = new CloudStoragePath[] {testBucket.getPath(TMP_FILE), testRPBucket.getPath(TMP_FILE) }; + for (int s=0; s<2; s++) { + for (int d=0; d<2; d++) { + // normal to normal is out of scope of RP testing. + if (s == 0 && d == 0) continue; + String sdesc = (s == 0 ? "normal bucket" : "requester-pays bucket"); + String ddesc = (d == 0 ? "normal bucket" : "requester-pays bucket"); + System.out.println("Copying from " + sdesc + " to " + ddesc); + Files.copy(sources[s], dests[d], StandardCopyOption.REPLACE_EXISTING); + } + } + } + + @Test + public void testAutodetectWhenRequesterPays() throws IOException { + CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(true, PROJECT); + Assert.assertEquals("Autodetect should have detected the RP bucket", testRPBucket.config().userProject(), PROJECT); + + } + + @Test + public void testAutodetectWhenNotRequesterPays() throws IOException { + CloudStorageConfiguration config = CloudStorageConfiguration.builder() + .autoDetectRequesterPays(true) + .userProject(PROJECT).build(); + CloudStorageFileSystem testBucket = CloudStorageFileSystem.forBucket(BUCKET, config, storageOptions); + Assert.assertEquals("Autodetect should have detected the bucket is not RP", testBucket.config().userProject(), ""); + } + + private void assertIsRequesterPaysException(StorageException sex) { + Assert.assertEquals(sex.getCode(), 400); + Assert.assertTrue(sex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); + } + + // End of tests related to the "requester pays" feature + // ----------------------------------------------------------------------------------- + @Test public void testFileExists() throws IOException { CloudStorageFileSystem testBucket = getTestBucket(); @@ -346,7 +509,7 @@ public void testListFiles() throws IOException { paths.addAll(goodPaths); goodPaths.add(fs.getPath("dir/dir2/")); for (Path path : paths) { - fillFile(storage, path.toString(), SML_SIZE); + fillFile(BUCKET, storage, path.toString(), SML_SIZE); } List got = new ArrayList<>(); @@ -458,4 +621,12 @@ private CloudStorageFileSystem getTestBucket() throws IOException { BUCKET, CloudStorageConfiguration.DEFAULT, storageOptions); } + // same as getTestBucket, but for the requester-pays bucket. + private CloudStorageFileSystem getRequesterPaysBucket(boolean autodetect, String userProject) throws IOException { + CloudStorageConfiguration config = CloudStorageConfiguration.builder() + .autoDetectRequesterPays(autodetect) + .userProject(userProject).build(); + return CloudStorageFileSystem.forBucket( + REQUESTER_PAYS_BUCKET, config, storageOptions); + } } From 38f609593e52ab84d26972262919bf1af1e56dfc Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 26 Jun 2018 13:13:52 -0700 Subject: [PATCH 2/6] linter fixes --- .../contrib/nio/CloudStorageFileSystemProvider.java | 2 +- .../storage/contrib/nio/CloudStorageReadChannel.java | 7 ------- .../contrib/nio/CloudStorageReadChannelTest.java | 4 ---- .../cloud/storage/contrib/nio/it/ITGcsNio.java | 12 +++++++----- 4 files changed, 8 insertions(+), 17 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 bdf273a1fc3e..9db459e08521 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 @@ -336,7 +336,7 @@ private SeekableByteChannel newReadChannel(Path path, Set 0, maxChannelReopens, userProject, - blobSourceOptions.toArray(new BlobSourceOption[0])); + blobSourceOptions.toArray(new BlobSourceOption[blobSourceOptions.size()])); } private SeekableByteChannel newWriteChannel(Path path, Set options) diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java index 4bcad6233a12..25b6d195d368 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java @@ -24,10 +24,8 @@ import com.google.cloud.storage.StorageException; import com.google.common.annotations.VisibleForTesting; -import com.google.common.collect.ImmutableList; import com.google.common.collect.Lists; -import java.util.ArrayList; import java.util.List; import javax.annotation.CheckReturnValue; import javax.annotation.concurrent.ThreadSafe; @@ -38,11 +36,6 @@ import java.nio.channels.SeekableByteChannel; import java.nio.file.NoSuchFileException; -import javax.net.ssl.SSLException; -import java.io.EOFException; -import java.net.SocketException; -import java.net.SocketTimeoutException; - import static com.google.common.base.Preconditions.checkArgument; /** diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java index 98d84acc296b..292ddff7e727 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannelTest.java @@ -31,10 +31,6 @@ import com.google.cloud.storage.Storage; import com.google.cloud.storage.StorageException; -import java.net.URI; -import java.nio.file.Files; -import java.nio.file.Path; -import java.nio.file.Paths; import org.junit.Before; import org.junit.Rule; import org.junit.Test; 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 33316e703f09..1b03df285f07 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 @@ -21,7 +21,6 @@ import com.google.api.client.http.HttpResponseException; import com.google.cloud.storage.Storage.BlobTargetOption; -import com.google.cloud.storage.Storage.BucketGetOption; import com.google.cloud.storage.StorageException; import com.google.cloud.storage.contrib.nio.CloudStorageConfiguration; import com.google.cloud.storage.contrib.nio.CloudStorageFileSystem; @@ -48,7 +47,6 @@ import java.nio.ByteBuffer; import java.nio.channels.ReadableByteChannel; import java.nio.channels.SeekableByteChannel; -import java.nio.file.CopyOption; import java.nio.file.FileSystem; import java.nio.file.FileVisitResult; import java.nio.file.Files; @@ -73,7 +71,8 @@ *

This test actually talks to Google Cloud Storage (you need an account) and tests both reading * and writing. You *must* set the {@code GOOGLE_APPLICATION_CREDENTIALS} environment variable for * this test to work. It must contain the name of a local file that contains your Service Account - * JSON Key. + * JSON Key. You also *must* set the PROJECT environment variable to the name of the project you + * want to use. * *

See * Service Accounts for instructions on how to get the Service Account JSON Key. @@ -101,7 +100,7 @@ public class ITGcsNio { private static final String BIG_FILE = "tmp-test-big-file.txt"; // it's big, relatively speaking. private static final int BIG_SIZE = 2 * 1024 * 1024 - 50; // arbitrary size that's not too round. private static final String PREFIX = "tmp-test-file"; - private static final String PROJECT = "jps-testing-project"; + private static final String PROJECT = System.getenv("PROJECT"); private static Storage storage; private static StorageOptions storageOptions; @@ -111,6 +110,7 @@ public class ITGcsNio { public static void beforeClass() throws IOException { // loads the credentials from local disk as par README RemoteStorageHelper gcsHelper = RemoteStorageHelper.create(); + System.out.println("Using project: " + PROJECT); storageOptions = gcsHelper.getOptions(); storage = storageOptions.getService(); // create and populate test bucket @@ -228,7 +228,9 @@ public void testCantCopyWithoutUserProject() throws IOException { for (int s=0; s<2; s++) { for (int d=0; d<2; d++) { // normal to normal is out of scope of RP testing. - if (s==0 && d==0) continue; + if (s==0 && d==0) { + continue; + } String sdesc = (s==0?"normal bucket":"requester-pays bucket"); String ddesc = (d==0?"normal bucket":"requester-pays bucket"); try { From 110c37fc8a8d5174f52a1eba27d3546876f03d85 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Tue, 26 Jun 2018 13:18:59 -0700 Subject: [PATCH 3/6] minor linter fixes --- .../storage/contrib/nio/CloudStorageFileSystemProvider.java | 2 -- .../cloud/storage/contrib/nio/CloudStorageReadChannel.java | 2 +- 2 files changed, 1 insertion(+), 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 9db459e08521..16d468cb6ee1 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 @@ -657,8 +657,6 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(CloudStorageUtil.getMaxChannelReopensFromPath(path)); // Loop will terminate via an exception if all retries are exhausted - BlobGetOption[] options; - while (true) { try { CloudStoragePath cloudPath = CloudStorageUtil.checkPath(path); diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java index 25b6d195d368..40afca7192a2 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java @@ -97,7 +97,7 @@ private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, if (!userProject.isEmpty()) { options.add(BlobSourceOption.userProject(userProject)); } - this.blobSourceOptions = (BlobSourceOption[]) options.toArray(new BlobSourceOption[0]); + this.blobSourceOptions = (BlobSourceOption[]) options.toArray(new BlobSourceOption[options.size()]); // innerOpen checks that it sees the same generation as fetchSize did, // which ensure the file hasn't changed. From e87a642de2514e951e472d1320d51a9a7a079f7f Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Thu, 28 Jun 2018 12:30:20 -0700 Subject: [PATCH 4/6] reviewer comments --- .../nio/CloudStorageConfiguration.java | 7 ++-- .../contrib/nio/CloudStorageFileSystem.java | 7 ++-- .../nio/CloudStorageFileSystemProvider.java | 33 ++++++++++--------- .../contrib/nio/CloudStorageReadChannel.java | 12 ++++--- .../storage/contrib/nio/it/ITGcsNio.java | 28 ++++++++-------- 5 files changed, 45 insertions(+), 42 deletions(-) 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 5cf374f4f4ce..0a9374007e93 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 @@ -21,6 +21,7 @@ import com.google.auto.value.AutoValue; +import javax.annotation.Nullable; import java.util.Map; /** @@ -71,7 +72,7 @@ public abstract class CloudStorageConfiguration { * set to bill that project (project you own) for all accesses. This is required for accessing * requester-pays buckets. This value cannot be null. */ - public abstract String userProject(); + public abstract @Nullable String userProject(); /** * Returns whether userProject will be ignored for non-requester-pays buckets. That is, @@ -110,7 +111,7 @@ public static final class Builder { private boolean usePseudoDirectories = true; private int blockSize = CloudStorageFileSystem.BLOCK_SIZE_DEFAULT; private int maxChannelReopens = 0; - private String userProject=""; + private @Nullable String userProject=null; private boolean autoDetectRequesterPays=false; /** @@ -171,7 +172,7 @@ public Builder maxChannelReopens(int value) { } public Builder userProject(String value) { - userProject = checkNotNull(value); + userProject = value; return this; } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index bb9081189ccd..33a07c473b8b 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -20,6 +20,7 @@ import static com.google.common.base.Preconditions.checkNotNull; import com.google.cloud.storage.StorageOptions; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableSet; import java.io.IOException; @@ -147,17 +148,17 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig checkArgument(!bucket.isEmpty(), "bucket"); this.bucket = bucket; if (config.autoDetectRequesterPays()) { - if (config.userProject().isEmpty()) { + if (Strings.isNullOrEmpty(config.userProject())) { throw new IllegalArgumentException("If autoDetectRequesterPays is set, then userProject must be set too."); } // detect whether we want to pay for these accesses or not. - if (!provider.isRequesterPays(bucket)) { + if (!provider.requesterPays(bucket)) { // update config (just to ease debugging, we're not actually using config.userProject later. HashMap disableUserProject = new HashMap<>(); disableUserProject.put("userProject", ""); config = CloudStorageConfiguration.fromMap(config, disableUserProject); // update the provider (this is the most important bit) - provider = provider.disableRequesterPays(); + provider = provider.withNoUserProject(); } } this.provider = provider; 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 16d468cb6ee1..2ae00476ee2a 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; @@ -91,8 +92,8 @@ public final class CloudStorageFileSystemProvider extends FileSystemProvider { private Storage storage; final private StorageOptions storageOptions; - // if non-empty, we pay via this one (TODO) - final private String userProject; + // if non-null, we pay via this project. + final private @Nullable String userProject; // used only when we create a new instance of CloudStorageFileSystemProvider. private static StorageOptions futureStorageOptions; @@ -178,7 +179,7 @@ public CloudStorageFileSystemProvider() { /** * Internal constructor to use the user-provided default config, and a given userProject setting. */ - CloudStorageFileSystemProvider(String userProject) { + CloudStorageFileSystemProvider(@Nullable String userProject) { this(userProject, futureStorageOptions); } @@ -186,7 +187,7 @@ public CloudStorageFileSystemProvider() { * Internal constructor, fully configurable. Note that null options means * to use the system defaults (NOT the user-provided ones). */ - CloudStorageFileSystemProvider(String userProject, @Nullable StorageOptions gcsStorageOptions) { + CloudStorageFileSystemProvider(@Nullable String userProject, @Nullable StorageOptions gcsStorageOptions) { this.storageOptions = gcsStorageOptions; this.userProject = userProject; } @@ -396,7 +397,7 @@ private SeekableByteChannel newWriteChannel(Path path, Set throw new UnsupportedOperationException(option.toString()); } } - if (!userProject.isEmpty()) { + if (!Strings.isNullOrEmpty(userProject)) { writeOptions.add(Storage.BlobWriteOption.userProject(userProject)); } @@ -451,7 +452,7 @@ public boolean deleteIfExists(Path path) throws IOException { // Loop will terminate via an exception if all retries are exhausted while (true) { try { - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { return storage.delete(cloudPath.getBlobId()); } else { return storage.delete(cloudPath.getBlobId(), Storage.BlobSourceOption.userProject(userProject)); @@ -575,7 +576,7 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep try { if ( wantCopyAttributes ) { BlobInfo blobInfo; - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { blobInfo = storage.get(fromPath.getBlobId()); } else { blobInfo = storage.get(fromPath.getBlobId(), BlobGetOption.userProject(userProject)); @@ -607,9 +608,9 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep } else { copyReqBuilder = copyReqBuilder.setTarget(tgtInfo, Storage.BlobTargetOption.doesNotExist()); } - if (!fromPath.getFileSystem().config().userProject().isEmpty()) { + if (!Strings.isNullOrEmpty(fromPath.getFileSystem().config().userProject())) { copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(fromPath.getFileSystem().config().userProject())); - } else if (!toPath.getFileSystem().config().userProject().isEmpty()) { + } else if (!Strings.isNullOrEmpty(toPath.getFileSystem().config().userProject())) { copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(toPath.getFileSystem().config().userProject())); } CopyWriter copyWriter = storage.copy(copyReqBuilder.build()); @@ -664,7 +665,7 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { return; } boolean nullId; - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { nullId = storage.get( cloudPath.getBlobId(), Storage.BlobGetOption.fields(Storage.BlobField.ID)) @@ -709,7 +710,7 @@ public A readAttributes( return result; } BlobInfo blobInfo; - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { blobInfo = storage.get(cloudPath.getBlobId()); } else { blobInfo = storage.get(cloudPath.getBlobId(), BlobGetOption.userProject(userProject)); @@ -775,7 +776,7 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter dirList; - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { dirList = storage.list(cloudPath.bucket(), Storage.BlobListOption.prefix(prefix), Storage.BlobListOption.currentDirectory(), Storage.BlobListOption.fields()); @@ -839,10 +840,10 @@ public String toString() { } /** - * @param bucketName - the name of the bucket to check + * @param bucketName the name of the bucket to check * @return whether requester pays is enabled for that bucket */ - public boolean isRequesterPays(String bucketName) { + public boolean requesterPays(String bucketName) { initStorage(); try { // instead of true/false, this method returns true/null. @@ -858,12 +859,12 @@ public boolean isRequesterPays(String bucketName) { /** * Returns a NEW CloudStorageFileSystemProvider identical to this one, but with - * requester pays disabled. + * userProject removed. * * Perhaps you want to call this is you realize you'll be working on a bucket that is * not requester-pays. */ - public CloudStorageFileSystemProvider disableRequesterPays() { + public CloudStorageFileSystemProvider withNoUserProject() { return new CloudStorageFileSystemProvider("", this.storageOptions); } diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java index 40afca7192a2..fdb1ec66e451 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageReadChannel.java @@ -24,10 +24,12 @@ import com.google.cloud.storage.StorageException; import com.google.common.annotations.VisibleForTesting; +import com.google.common.base.Strings; import com.google.common.collect.Lists; import java.util.List; import javax.annotation.CheckReturnValue; +import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; import java.io.IOException; import java.nio.ByteBuffer; @@ -77,12 +79,12 @@ final class CloudStorageReadChannel implements SeekableByteChannel { */ @CheckReturnValue @SuppressWarnings("resource") - static CloudStorageReadChannel create(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, String userProject, BlobSourceOption... blobSourceOptions) + static CloudStorageReadChannel create(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, @Nullable String userProject, BlobSourceOption... blobSourceOptions) throws IOException { return new CloudStorageReadChannel(gcsStorage, file, position, maxChannelReopens, userProject, blobSourceOptions); } - private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, String userProject, BlobSourceOption... blobSourceOptions) throws IOException { + private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, int maxChannelReopens, @Nullable String userProject, BlobSourceOption... blobSourceOptions) throws IOException { this.gcsStorage = gcsStorage; this.file = file; this.position = position; @@ -94,7 +96,7 @@ private CloudStorageReadChannel(Storage gcsStorage, BlobId file, long position, if (null != generation) { options.add(Storage.BlobSourceOption.generationMatch(generation)); } - if (!userProject.isEmpty()) { + if (!Strings.isNullOrEmpty(userProject)) { options.add(BlobSourceOption.userProject(userProject)); } this.blobSourceOptions = (BlobSourceOption[]) options.toArray(new BlobSourceOption[options.size()]); @@ -200,13 +202,13 @@ private void checkOpen() throws ClosedChannelException { } } - private long fetchSize(Storage gcsStorage, String userProject, BlobId file) throws IOException { + private long fetchSize(Storage gcsStorage, @Nullable String userProject, BlobId file) throws IOException { final CloudStorageRetryHandler retryHandler = new CloudStorageRetryHandler(maxRetries, maxChannelReopens); while (true) { try { BlobInfo blobInfo; - if (userProject.isEmpty()) { + if (Strings.isNullOrEmpty(userProject)) { blobInfo = gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE)); } else { blobInfo = gcsStorage.get(file, Storage.BlobGetOption.fields(Storage.BlobField.GENERATION, Storage.BlobField.SIZE), Storage.BlobGetOption.userProject(userProject)); 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 1b03df285f07..0fe518a98102 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 @@ -71,8 +71,7 @@ *

This test actually talks to Google Cloud Storage (you need an account) and tests both reading * and writing. You *must* set the {@code GOOGLE_APPLICATION_CREDENTIALS} environment variable for * this test to work. It must contain the name of a local file that contains your Service Account - * JSON Key. You also *must* set the PROJECT environment variable to the name of the project you - * want to use. + * JSON Key. We use the project in those credentials. * *

See * Service Accounts for instructions on how to get the Service Account JSON Key. @@ -100,7 +99,7 @@ public class ITGcsNio { private static final String BIG_FILE = "tmp-test-big-file.txt"; // it's big, relatively speaking. private static final int BIG_SIZE = 2 * 1024 * 1024 - 50; // arbitrary size that's not too round. private static final String PREFIX = "tmp-test-file"; - private static final String PROJECT = System.getenv("PROJECT"); + private static String PROJECT; private static Storage storage; private static StorageOptions storageOptions; @@ -110,13 +109,14 @@ public class ITGcsNio { public static void beforeClass() throws IOException { // loads the credentials from local disk as par README RemoteStorageHelper gcsHelper = RemoteStorageHelper.create(); - System.out.println("Using project: " + PROJECT); storageOptions = gcsHelper.getOptions(); + PROJECT = storageOptions.getProjectId(); + System.out.println("Using project: " + PROJECT); storage = storageOptions.getService(); // create and populate test bucket storage.create(BucketInfo.of(BUCKET)); - fillFile(BUCKET, storage, SML_FILE, SML_SIZE); - fillFile(BUCKET, storage, BIG_FILE, BIG_SIZE); + fillFile(storage, BUCKET, SML_FILE, SML_SIZE); + fillFile(storage, BUCKET, BIG_FILE, BIG_SIZE); BucketInfo requesterPaysBucket = BucketInfo.newBuilder(REQUESTER_PAYS_BUCKET).setRequesterPays(true).build(); storage.create(requesterPaysBucket); fillRequesterPaysFile(storage, SML_FILE, SML_SIZE); @@ -136,7 +136,7 @@ private static byte[] randomContents(int size) { return bytes; } - private static void fillFile(String bucket, Storage storage, String fname, int size) throws IOException { + private static void fillFile(Storage storage, String bucket, String fname, int size) throws IOException { storage.create(BlobInfo.newBuilder(bucket, fname).build(), randomContents(size)); } @@ -145,8 +145,7 @@ private static void fillRequesterPaysFile(Storage storage, String fname, int siz BlobTargetOption.userProject(PROJECT)); } - // ----------------------------------------------------------------------------------- - // Tests related to the "requester pays" feature + // Start of tests related to the "requester pays" feature @Test public void testFileExistsRequesterPaysNoUserProject() throws IOException { @@ -254,10 +253,10 @@ public void testCantCopyWithoutUserProject() throws IOException { public void testCanCopyWithUserProject() throws IOException { CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(false, PROJECT); CloudStorageFileSystem testBucket = getTestBucket(); - CloudStoragePath[] sources = new CloudStoragePath[] { testBucket.getPath(SML_FILE), testRPBucket.getPath(SML_FILE) }; - CloudStoragePath[] dests = new CloudStoragePath[] {testBucket.getPath(TMP_FILE), testRPBucket.getPath(TMP_FILE) }; - for (int s=0; s<2; s++) { - for (int d=0; d<2; d++) { + CloudStoragePath[] sources = new CloudStoragePath[] {testBucket.getPath(SML_FILE), testRPBucket.getPath(SML_FILE)}; + CloudStoragePath[] dests = new CloudStoragePath[] {testBucket.getPath(TMP_FILE), testRPBucket.getPath(TMP_FILE)}; + for (int s = 0; s < 2; s++) { + for (int d = 0; d < 2; d++) { // normal to normal is out of scope of RP testing. if (s == 0 && d == 0) continue; String sdesc = (s == 0 ? "normal bucket" : "requester-pays bucket"); @@ -290,7 +289,6 @@ private void assertIsRequesterPaysException(StorageException sex) { } // End of tests related to the "requester pays" feature - // ----------------------------------------------------------------------------------- @Test public void testFileExists() throws IOException { @@ -511,7 +509,7 @@ public void testListFiles() throws IOException { paths.addAll(goodPaths); goodPaths.add(fs.getPath("dir/dir2/")); for (Path path : paths) { - fillFile(BUCKET, storage, path.toString(), SML_SIZE); + fillFile(storage, BUCKET, path.toString(), SML_SIZE); } List got = new ArrayList<>(); From b76894d940ce03ec1b5c1ce9e2fd4e946014e4b7 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 29 Jun 2018 15:22:07 -0700 Subject: [PATCH 5/6] apply all codacy recommendations --- .../nio/CloudStorageConfiguration.java | 22 +++--- .../contrib/nio/CloudStorageFileSystem.java | 4 +- .../nio/CloudStorageFileSystemProvider.java | 16 ++-- .../storage/contrib/nio/it/ITGcsNio.java | 77 ++++++++++--------- 4 files changed, 63 insertions(+), 56 deletions(-) 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 0a9374007e93..17cc04981166 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 @@ -75,7 +75,7 @@ public abstract class CloudStorageConfiguration { public abstract @Nullable String userProject(); /** - * Returns whether userProject will be ignored for non-requester-pays buckets. That is, + * Returns whether userProject will be cleared for non-requester-pays buckets. That is, * if false (the default value), setting userProject causes that project to be billed * regardless of whether the bucket is requester-pays or not. If true, setting * userProject will only cause that project to be billed when the project is requester-pays. @@ -83,7 +83,7 @@ public abstract class CloudStorageConfiguration { * Setting this will cause the bucket to be accessed when the CloudStorageFileSystem object * is created. */ - public abstract boolean autoDetectRequesterPays(); + public abstract boolean useUserProjectOnlyForRequesterPaysBuckets(); /** @@ -110,9 +110,13 @@ public static final class Builder { private boolean stripPrefixSlash = true; private boolean usePseudoDirectories = true; private int blockSize = CloudStorageFileSystem.BLOCK_SIZE_DEFAULT; - private int maxChannelReopens = 0; - private @Nullable String userProject=null; - private boolean autoDetectRequesterPays=false; + // default: 0 + private int maxChannelReopens; + // default: null + private @Nullable String userProject; + // This of this as "clear userProject if not RequesterPays" + // default: false + private boolean useUserProjectOnlyForRequesterPaysBuckets; /** * Changes current working directory for new filesystem. This defaults to the root directory. @@ -177,7 +181,7 @@ public Builder userProject(String value) { } public Builder autoDetectRequesterPays(boolean value) { - autoDetectRequesterPays = value; + useUserProjectOnlyForRequesterPaysBuckets = value; return this; } @@ -193,7 +197,7 @@ public CloudStorageConfiguration build() { blockSize, maxChannelReopens, userProject, - autoDetectRequesterPays); + useUserProjectOnlyForRequesterPaysBuckets); } Builder(CloudStorageConfiguration toModify) { @@ -204,7 +208,7 @@ public CloudStorageConfiguration build() { blockSize = toModify.blockSize(); maxChannelReopens = toModify.maxChannelReopens(); userProject = toModify.userProject(); - autoDetectRequesterPays = toModify.autoDetectRequesterPays(); + useUserProjectOnlyForRequesterPaysBuckets = toModify.useUserProjectOnlyForRequesterPaysBuckets(); } Builder() {} @@ -242,7 +246,7 @@ static private CloudStorageConfiguration fromMap(Builder builder, Map case "userProject": builder.userProject((String) entry.getValue()); break; - case "autoDetectRequesterPays": + case "useUserProjectOnlyForRequesterPaysBuckets": builder.autoDetectRequesterPays((Boolean) entry.getValue()); break; default: diff --git a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 33a07c473b8b..2b613d94c5b2 100644 --- a/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-clients/google-cloud-contrib/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -147,9 +147,9 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig CloudStorageFileSystemProvider provider, String bucket, CloudStorageConfiguration config) { checkArgument(!bucket.isEmpty(), "bucket"); this.bucket = bucket; - if (config.autoDetectRequesterPays()) { + if (config.useUserProjectOnlyForRequesterPaysBuckets()) { if (Strings.isNullOrEmpty(config.userProject())) { - throw new IllegalArgumentException("If autoDetectRequesterPays is set, then userProject must be set too."); + throw new IllegalArgumentException("If useUserProjectOnlyForRequesterPaysBuckets is set, then userProject must be set too."); } // detect whether we want to pay for these accesses or not. if (!provider.requesterPays(bucket)) { 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 2ae00476ee2a..978a3eddaaea 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 @@ -397,7 +397,7 @@ private SeekableByteChannel newWriteChannel(Path path, Set throw new UnsupportedOperationException(option.toString()); } } - if (!Strings.isNullOrEmpty(userProject)) { + if (!isNullOrEmpty(userProject)) { writeOptions.add(Storage.BlobWriteOption.userProject(userProject)); } @@ -452,7 +452,7 @@ public boolean deleteIfExists(Path path) throws IOException { // Loop will terminate via an exception if all retries are exhausted while (true) { try { - if (Strings.isNullOrEmpty(userProject)) { + if (isNullOrEmpty(userProject)) { return storage.delete(cloudPath.getBlobId()); } else { return storage.delete(cloudPath.getBlobId(), Storage.BlobSourceOption.userProject(userProject)); @@ -576,7 +576,7 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep try { if ( wantCopyAttributes ) { BlobInfo blobInfo; - if (Strings.isNullOrEmpty(userProject)) { + if (isNullOrEmpty(userProject)) { blobInfo = storage.get(fromPath.getBlobId()); } else { blobInfo = storage.get(fromPath.getBlobId(), BlobGetOption.userProject(userProject)); @@ -608,9 +608,9 @@ public void copy(Path source, Path target, CopyOption... options) throws IOExcep } else { copyReqBuilder = copyReqBuilder.setTarget(tgtInfo, Storage.BlobTargetOption.doesNotExist()); } - if (!Strings.isNullOrEmpty(fromPath.getFileSystem().config().userProject())) { + if (!isNullOrEmpty(fromPath.getFileSystem().config().userProject())) { copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(fromPath.getFileSystem().config().userProject())); - } else if (!Strings.isNullOrEmpty(toPath.getFileSystem().config().userProject())) { + } else if (!isNullOrEmpty(toPath.getFileSystem().config().userProject())) { copyReqBuilder = copyReqBuilder.setSourceOptions(BlobSourceOption.userProject(toPath.getFileSystem().config().userProject())); } CopyWriter copyWriter = storage.copy(copyReqBuilder.build()); @@ -665,7 +665,7 @@ public void checkAccess(Path path, AccessMode... modes) throws IOException { return; } boolean nullId; - if (Strings.isNullOrEmpty(userProject)) { + if (isNullOrEmpty(userProject)) { nullId = storage.get( cloudPath.getBlobId(), Storage.BlobGetOption.fields(Storage.BlobField.ID)) @@ -710,7 +710,7 @@ public A readAttributes( return result; } BlobInfo blobInfo; - if (Strings.isNullOrEmpty(userProject)) { + if (isNullOrEmpty(userProject)) { blobInfo = storage.get(cloudPath.getBlobId()); } else { blobInfo = storage.get(cloudPath.getBlobId(), BlobGetOption.userProject(userProject)); @@ -776,7 +776,7 @@ public DirectoryStream newDirectoryStream(Path dir, final Filter dirList; - if (Strings.isNullOrEmpty(userProject)) { + if (isNullOrEmpty(userProject)) { dirList = storage.list(cloudPath.bucket(), Storage.BlobListOption.prefix(prefix), Storage.BlobListOption.currentDirectory(), Storage.BlobListOption.fields()); 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 0fe518a98102..3b35352154cc 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 @@ -99,7 +99,7 @@ public class ITGcsNio { private static final String BIG_FILE = "tmp-test-big-file.txt"; // it's big, relatively speaking. private static final int BIG_SIZE = 2 * 1024 * 1024 - 50; // arbitrary size that's not too round. private static final String PREFIX = "tmp-test-file"; - private static String PROJECT; + private static String project; private static Storage storage; private static StorageOptions storageOptions; @@ -110,8 +110,7 @@ public static void beforeClass() throws IOException { // loads the credentials from local disk as par README RemoteStorageHelper gcsHelper = RemoteStorageHelper.create(); storageOptions = gcsHelper.getOptions(); - PROJECT = storageOptions.getProjectId(); - System.out.println("Using project: " + PROJECT); + project = storageOptions.getProjectId(); storage = storageOptions.getService(); // create and populate test bucket storage.create(BucketInfo.of(BUCKET)); @@ -142,7 +141,7 @@ private static void fillFile(Storage storage, String bucket, String fname, int s private static void fillRequesterPaysFile(Storage storage, String fname, int size) throws IOException { storage.create(BlobInfo.newBuilder(REQUESTER_PAYS_BUCKET, fname).build(), randomContents(size), - BlobTargetOption.userProject(PROJECT)); + BlobTargetOption.userProject(project)); } // Start of tests related to the "requester pays" feature @@ -156,13 +155,13 @@ public void testFileExistsRequesterPaysNoUserProject() throws IOException { Files.exists(path); Assert.fail("It should have thrown an exception."); } catch (StorageException sex) { - assertIsRequesterPaysException(sex); + assertIsRequesterPaysException("testFileExistsRequesterPaysNoUserProject", sex); } } @Test public void testFileExistsRequesterPays() throws IOException { - CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, project); Path path = testBucket.getPath(SML_FILE); // should succeed because we specified a project Files.exists(path); @@ -170,7 +169,7 @@ public void testFileExistsRequesterPays() throws IOException { @Test public void testFileExistsRequesterPaysWithAutodetect() throws IOException { - CloudStorageFileSystem testBucket = getRequesterPaysBucket(true, PROJECT); + CloudStorageFileSystem testBucket = getRequesterPaysBucket(true, project); Path path = testBucket.getPath(SML_FILE); // should succeed because we specified a project Files.exists(path); @@ -185,13 +184,13 @@ public void testCantCreateWithoutUserProject() throws IOException { Files.write(path, "I would like to write".getBytes()); Assert.fail("It should have thrown an exception."); } catch (StorageException sex) { - assertIsRequesterPaysException(sex); + assertIsRequesterPaysException("testCantCreateWithoutUserProject", sex); } } @Test public void testCanCreateWithUserProject() throws IOException { - CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, project); Path path = testBucket.getPath(TMP_FILE); // should succeed because we specified a project Files.write(path, "I would like to write, please?".getBytes()); @@ -206,13 +205,13 @@ public void testCantReadWithoutUserProject() throws IOException { Files.readAllBytes(path); Assert.fail("It should have thrown an exception."); } catch (StorageException sex) { - assertIsRequesterPaysException(sex); + assertIsRequesterPaysException("testCantReadWithoutUserProject", sex); } } @Test public void testCanReadWithUserProject() throws IOException { - CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, PROJECT); + CloudStorageFileSystem testBucket = getRequesterPaysBucket(false, project); Path path = testBucket.getPath(SML_FILE); // should succeed because we specified a project Files.readAllBytes(path); @@ -230,38 +229,42 @@ public void testCantCopyWithoutUserProject() throws IOException { if (s==0 && d==0) { continue; } - String sdesc = (s==0?"normal bucket":"requester-pays bucket"); - String ddesc = (d==0?"normal bucket":"requester-pays bucket"); - try { - System.out.println("Copying from " + sdesc + " to " + ddesc); - Files.copy(sources[s], dests[d]); - Assert.fail("Shouldn't have been able to copy from " + sdesc + " to " + ddesc); - // for some reason this throws "GoogleJsonResponseException" instead of "StorageException" - // when going from requester pays bucket to requester pays bucket, but otherwise we get a - // normal StorageException. - } catch (HttpResponseException hex) { - Assert.assertEquals(hex.getStatusCode(), 400); - Assert.assertTrue(hex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); - } catch (StorageException sex) { - assertIsRequesterPaysException(sex); - } + innerTestCantCopyWithoutUserProject(s==0, d==0, sources[s], dests[d]); } } } + // Try to copy the file, make sure that we were prevented. + private void innerTestCantCopyWithoutUserProject(boolean sourceNormal, boolean destNormal, Path source, Path dest) throws IOException { + String sdesc = (sourceNormal?"normal bucket":"requester-pays bucket"); + String ddesc = (destNormal?"normal bucket":"requester-pays bucket"); + String description = "Copying from " + sdesc + " to " + ddesc; + try { + Files.copy(source, dest); + Assert.fail("Shouldn't have been able to copy from " + sdesc + " to " + ddesc); + // for some reason this throws "GoogleJsonResponseException" instead of "StorageException" + // when going from requester pays bucket to requester pays bucket, but otherwise we get a + // normal StorageException. + } catch (HttpResponseException hex) { + Assert.assertEquals(description, hex.getStatusCode(), 400); + Assert.assertTrue(description, hex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); + } catch (StorageException sex) { + assertIsRequesterPaysException(description, sex); + } + } + @Test public void testCanCopyWithUserProject() throws IOException { - CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(false, PROJECT); + CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(false, project); CloudStorageFileSystem testBucket = getTestBucket(); CloudStoragePath[] sources = new CloudStoragePath[] {testBucket.getPath(SML_FILE), testRPBucket.getPath(SML_FILE)}; CloudStoragePath[] dests = new CloudStoragePath[] {testBucket.getPath(TMP_FILE), testRPBucket.getPath(TMP_FILE)}; for (int s = 0; s < 2; s++) { for (int d = 0; d < 2; d++) { // normal to normal is out of scope of RP testing. - if (s == 0 && d == 0) continue; - String sdesc = (s == 0 ? "normal bucket" : "requester-pays bucket"); - String ddesc = (d == 0 ? "normal bucket" : "requester-pays bucket"); - System.out.println("Copying from " + sdesc + " to " + ddesc); + if (s == 0 && d == 0) { + continue; + } Files.copy(sources[s], dests[d], StandardCopyOption.REPLACE_EXISTING); } } @@ -269,8 +272,8 @@ public void testCanCopyWithUserProject() throws IOException { @Test public void testAutodetectWhenRequesterPays() throws IOException { - CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(true, PROJECT); - Assert.assertEquals("Autodetect should have detected the RP bucket", testRPBucket.config().userProject(), PROJECT); + CloudStorageFileSystem testRPBucket = getRequesterPaysBucket(true, project); + Assert.assertEquals("Autodetect should have detected the RP bucket", testRPBucket.config().userProject(), project); } @@ -278,14 +281,14 @@ public void testAutodetectWhenRequesterPays() throws IOException { public void testAutodetectWhenNotRequesterPays() throws IOException { CloudStorageConfiguration config = CloudStorageConfiguration.builder() .autoDetectRequesterPays(true) - .userProject(PROJECT).build(); + .userProject(project).build(); CloudStorageFileSystem testBucket = CloudStorageFileSystem.forBucket(BUCKET, config, storageOptions); Assert.assertEquals("Autodetect should have detected the bucket is not RP", testBucket.config().userProject(), ""); } - private void assertIsRequesterPaysException(StorageException sex) { - Assert.assertEquals(sex.getCode(), 400); - Assert.assertTrue(sex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); + private void assertIsRequesterPaysException(String message, StorageException sex) { + Assert.assertEquals(message, sex.getCode(), 400); + Assert.assertTrue(message, sex.getMessage().contains("Bucket is requester pays bucket but no user project provided")); } // End of tests related to the "requester pays" feature From af4e4c5d11f39a0a8691e4169e02b4c59a64a1c5 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Martin Date: Fri, 29 Jun 2018 15:57:39 -0700 Subject: [PATCH 6/6] Put defaults back, remove unused import. --- .../storage/contrib/nio/CloudStorageConfiguration.java | 9 +++------ .../contrib/nio/CloudStorageFileSystemProvider.java | 1 - 2 files changed, 3 insertions(+), 7 deletions(-) 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 17cc04981166..b4a586785013 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 @@ -110,13 +110,10 @@ public static final class Builder { private boolean stripPrefixSlash = true; private boolean usePseudoDirectories = true; private int blockSize = CloudStorageFileSystem.BLOCK_SIZE_DEFAULT; - // default: 0 - private int maxChannelReopens; - // default: null - private @Nullable String userProject; + private int maxChannelReopens = 0; + private @Nullable String userProject = null; // This of this as "clear userProject if not RequesterPays" - // default: false - private boolean useUserProjectOnlyForRequesterPaysBuckets; + private boolean useUserProjectOnlyForRequesterPaysBuckets = false; /** * Changes current working directory for new filesystem. This defaults to the root directory. 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 978a3eddaaea..b28e833763c7 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,7 +34,6 @@ 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;