Make some final changes to the GCS NIO API

gcloud-java-contrib/gcloud-java-nio/pom.xml

@@ -1,7 +1,6 @@
 <?xml version="1.0"?>
 <project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
   <modelVersion>4.0.0</modelVersion>
-  <groupId>com.google.gcloud</groupId>
   <artifactId>gcloud-java-nio</artifactId>
   <packaging>jar</packaging>
   <name>GCloud Java NIO</name>
@@ -37,18 +36,40 @@
       <artifactId>javax.inject</artifactId>
       <version>1</version>
     </dependency>
-    <dependency>
-      <groupId>com.google.auto.service</groupId>
-      <artifactId>auto-service</artifactId>
-      <version>1.0-rc2</version>
-      <scope>provided</scope> <!-- to leave out of the all-deps jar -->
-    </dependency>
     <dependency>
       <groupId>com.google.auto.value</groupId>
       <artifactId>auto-value</artifactId>
       <version>1.1</version>
       <scope>provided</scope> <!-- to leave out of the all-deps jar -->
     </dependency>
+    <dependency>
+      <groupId>com.google.auto.factory</groupId>
+      <artifactId>auto-factory</artifactId>
+      <version>1.0-beta3</version>
+      <scope>provided</scope> <!-- to leave out of the all-deps jar -->
+      <exclusions>
+        <exclusion>
+          <groupId>com.google.guava</groupId>
+          <artifactId>guava</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
+    <dependency>
+      <groupId>com.google.auto.service</groupId>
+      <artifactId>auto-service</artifactId>
+      <version>1.0-rc2</version>
+      <scope>provided</scope> <!-- to leave out of the all-deps jar -->
+      <exclusions>
+        <exclusion>
+          <groupId>com.google.guava</groupId>
+          <artifactId>guava</artifactId>
+        </exclusion>
+        <exclusion>
+          <groupId>com.google.auto</groupId>
+          <artifactId>auto-common</artifactId>
+        </exclusion>
+      </exclusions>
+    </dependency>
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>

gcloud-java-contrib/gcloud-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageConfiguration.java

@@ -19,43 +19,26 @@
 import static com.google.common.base.Preconditions.checkArgument;
 
 import com.google.auto.value.AutoValue;
+import com.google.common.base.Strings;
 
 import java.util.Map;
 
+import javax.annotation.Nullable;
+
 /**
  * Configuration for {@link CloudStorageFileSystem} instances.
  */
 @AutoValue
 public abstract class CloudStorageConfiguration {
 
-  /**
-   * Returns path of current working directory. This defaults to the root directory.
-   */
-  public abstract String workingDirectory();
-
-  /**
-   * Returns {@code true} if we <i>shouldn't</i> throw an exception when encountering object names
-   * containing superfluous slashes, e.g. {@code a//b}.
-   */
-  public abstract boolean permitEmptyPathComponents();
-
-  /**
-   * Returns {@code true} if '/' prefix on absolute object names should be removed before I/O.
-   *
-   * <p>If you disable this feature, please take into consideration that all paths created from a
-   * URI will have the leading slash.
-   */
-  public abstract boolean stripPrefixSlash();
-
-  /**
-   * Returns {@code true} if paths with a trailing slash should be treated as fake directories.
-   */
-  public abstract boolean usePseudoDirectories();
+  private static final CloudStorageConfiguration DEFAULT = builder().build();
 
   /**
-   * Returns block size (in bytes) used when talking to the GCS HTTP server.
+   * Returns default GCS NIO configuration.
    */
-  public abstract int blockSize();
+  public static CloudStorageConfiguration defaultInstance() {
+    return DEFAULT;
+  }
 
   /**
    * Creates a new builder, initialized with the following settings:
@@ -70,22 +53,33 @@ public static Builder builder() {
     return new Builder();
   }
 
+  abstract String workingDirectory();
+
+  abstract boolean permitEmptyPathComponents();
+
+  abstract boolean stripPrefixSlash();
+
+  abstract boolean usePseudoDirectories();
+
+  abstract int blockSize();
+
   /**
    * Builder for {@link CloudStorageConfiguration}.
    */
   public static final class Builder {
 
     private String workingDirectory = UnixPath.ROOT;
-    private boolean permitEmptyPathComponents = false;
+    private boolean permitEmptyPathComponents;
     private boolean stripPrefixSlash = true;
     private boolean usePseudoDirectories = true;
     private int blockSize = CloudStorageFileSystem.BLOCK_SIZE_DEFAULT;
 
     /**
-     * Changes current working directory for new filesystem. This cannot be changed once it's
-     * been set. You'll need to create another {@link CloudStorageFileSystem} object.
+     * Changes current working directory for new filesystem. This defaults to the root directory.
+     * The working directory cannot be changed once it's been set. You'll need to create another
+     * {@link CloudStorageFileSystem} object.
      *
-     * @throws IllegalArgumentException if {@code path} is not absolute.
+     * @throws IllegalArgumentException if {@code path} is not absolute
      */
     public Builder workingDirectory(String path) {
       checkArgument(UnixPath.getPath(false, path).isAbsolute(), "not absolute: %s", path);
@@ -95,7 +89,7 @@ public Builder workingDirectory(String path) {
 
     /**
      * Configures whether or not we should throw an exception when encountering object names
-     * containing superfluous slashes, e.g. {@code a//b}
+     * containing superfluous slashes, e.g. {@code a//b}.
      */
     public Builder permitEmptyPathComponents(boolean value) {
       permitEmptyPathComponents = value;
@@ -146,15 +140,13 @@ public CloudStorageConfiguration build() {
     Builder() {}
   }
 
-  public static final CloudStorageConfiguration DEFAULT = builder().build();
-
-  static CloudStorageConfiguration fromMap(Map<String, ?> env) {
+  static CloudStorageConfiguration fromMap(@Nullable String workingDirectory, Map<String, ?> env) {
     Builder builder = builder();
+    if (!Strings.isNullOrEmpty(workingDirectory)) {
+      builder.workingDirectory(workingDirectory);
+    }
     for (Map.Entry<String, ?> entry : env.entrySet()) {
       switch (entry.getKey()) {
-        case "workingDirectory":
-          builder.workingDirectory((String) entry.getValue());
-          break;
         case "permitEmptyPathComponents":
           builder.permitEmptyPathComponents((Boolean) entry.getValue());
           break;

gcloud-java-contrib/gcloud-java-nio/src/main/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystem.java

@@ -19,8 +19,11 @@
 import static com.google.common.base.Preconditions.checkArgument;
 import static com.google.common.base.Preconditions.checkNotNull;
 
+import com.google.auto.factory.AutoFactory;
+import com.google.auto.factory.Provided;
+import com.google.common.base.Predicates;
 import com.google.common.collect.ImmutableSet;
-import com.google.gcloud.storage.StorageOptions;
+import com.google.common.collect.Iterables;
 
 import java.io.IOException;
 import java.net.URI;
@@ -32,11 +35,13 @@
 import java.nio.file.WatchService;
 import java.nio.file.attribute.FileTime;
 import java.nio.file.attribute.UserPrincipalLookupService;
+import java.nio.file.spi.FileSystemProvider;
 import java.util.Objects;
 import java.util.Set;
+import java.util.logging.Logger;
 
-import javax.annotation.Nullable;
-import javax.annotation.concurrent.Immutable;
+import javax.annotation.CheckReturnValue;
+import javax.annotation.concurrent.ThreadSafe;
 
 /**
  * Google Cloud Storage {@link FileSystem} implementation.
@@ -46,49 +51,62 @@
  * @see <a href="https://developers.google.com/storage/docs/bucketnaming">
  *        Bucket and Object Naming Guidelines</a>
  */
-@Immutable
+@ThreadSafe
 public final class CloudStorageFileSystem extends FileSystem {
 
+  private static final Logger logger = Logger.getLogger(CloudStorageFileSystem.class.getName());
+
   /**
-   * Returns Google Cloud Storage {@link FileSystem} object for {@code bucket}.
-   *
-   * <p><b>NOTE:</b> You may prefer to use Java's standard API instead:<pre>   {@code
-   *
-   *   FileSystem fs = FileSystems.getFileSystem(URI.create("gs://bucket"));}</pre>
-   *
-   * <p>However some systems and build environments might be flaky when it comes to Java SPI. This
-   * is because services are generally runtime dependencies and depend on a META-INF file being
-   * present in your jar (generated by Google Auto at compile-time). In such cases, this method
-   * provides a simpler alternative.
-   *
-   * @see #forBucket(String, CloudStorageConfiguration)
-   * @see java.nio.file.FileSystems#getFileSystem(URI)
+   * Invokes {@link #forBucket(String, CloudStorageConfiguration)} with
+   * {@link CloudStorageConfiguration#defaultInstance()}.
    */
+  @CheckReturnValue
   public static CloudStorageFileSystem forBucket(String bucket) {
-    return forBucket(bucket, CloudStorageConfiguration.DEFAULT);
+    return forBucket(bucket, CloudStorageConfiguration.defaultInstance());
   }
 
   /**
-   * Creates new file system instance for {@code bucket}, with customizable settings.
+   * Returns Google Cloud Storage {@link FileSystem} object for {@code bucket}.
+   *
+   * <p>GCS file system objects are basically free. You can create as many as you want, even if you
+   * have multiple instances for the same bucket. There's no actual system resources associated
+   * with this object. Therefore calling {@link #close()} on the returned value is optional.
    *
-   * @see #forBucket(String)
+   * <p><b>Note:</b> It is also possible to instantiate this class via Java's Service Provider
+   * Interface, e.g. {@code FileSystems.getFileSystem(URI.create("gs://bucket"))}. We discourage you
+   * from using that if possible, for the reasons documented in
+   * {@link CloudStorageFileSystemProvider#newFileSystem(URI, java.util.Map)}
+   *
+   * @see java.nio.file.FileSystems#getFileSystem(URI)
    */
+  @CheckReturnValue
   public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
-    return forBucket(bucket, config, null);
+    checkNotNull(config);
+    checkArgument(
+        !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
+    return new CloudStorageFileSystem(getProvider(), config, bucket);
   }
 
-  /**
-   * Creates a new filesystem for a particular bucket, with customizable settings and storage
-   * options.
-   *
-   * @see #forBucket(String)
-   */
-  public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config,
-        @Nullable StorageOptions storageOptions) {
-    checkArgument(!bucket.startsWith(URI_SCHEME + ":"),
-        "Bucket name must not have schema: %s", bucket);
-    return new CloudStorageFileSystem(new CloudStorageFileSystemProvider(storageOptions),
-        bucket, checkNotNull(config));
+  private static CloudStorageFileSystemProvider getProvider() {
+    // We want to get the provider instance from the service loader if possible. This is important
+    // because the behavior of NIO (as implemented in classes like java.nio.files.Files) changes
+    // quite a bit if the provider instances aren't identical.
+    FileSystemProvider provider =
+        Iterables.getOnlyElement(
+            Iterables.filter(
+                FileSystemProvider.installedProviders(),
+                Predicates.instanceOf(CloudStorageFileSystemProvider.class)),
+            null);
+    if (provider != null) {
+      return (CloudStorageFileSystemProvider) provider;
+    }
+    // If the provider can not be found via the service loader, then we fall back to instantiating
+    // it ourselves. This should safeguard against needless user frustration in situations where the
+    // weird provider file created by @AutoService doesn't find its way into the jar. However this
+    // could lead to unexpected changes in behavior under the rare circumstance that the user
+    // instantiates multiple instances and uses them together on operations like copy.
+    logger.warning("Could not find CloudStorageFileSystemProvider via service loader");
+    return new CloudStorageFileSystemProvider();
   }
 
   public static final String URI_SCHEME = "gs";
@@ -98,12 +116,15 @@ public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfig
   public static final FileTime FILE_TIME_UNKNOWN = FileTime.fromMillis(0);
   public static final ImmutableSet<String> SUPPORTED_VIEWS = ImmutableSet.of(BASIC_VIEW, GCS_VIEW);
 
-  private final CloudStorageFileSystemProvider provider;
   private final String bucket;
+  private final CloudStorageFileSystemProvider provider;
   private final CloudStorageConfiguration config;
 
+  @AutoFactory
   CloudStorageFileSystem(
-      CloudStorageFileSystemProvider provider, String bucket, CloudStorageConfiguration config) {
+      @Provided CloudStorageFileSystemProvider provider,
+      @Provided CloudStorageConfiguration config,
+      String bucket) {
     checkArgument(!bucket.isEmpty(), "bucket");
     this.provider = provider;
     this.bucket = bucket;
@@ -142,13 +163,17 @@ public CloudStoragePath getPath(String first, String... more) {
   }
 
   /**
-   * Does nothing.
+   * Does nothing currently. This method <i>might</i> be updated in the future to close all channels
+   * associated with this file system object. However it's unlikely that even then, calling this
+   * method will become mandatory.
    */
   @Override
-  public void close() {}
+  public void close() throws IOException {
+    // TODO(#809): Synchronously close all channels associated with this FileSystem instance.
+  }
 
   /**
-   * Returns {@code true}.
+   * Returns {@code true}, even if you previously called the {@link #close()} method.
    */
   @Override
   public boolean isOpen() {
@@ -176,6 +201,9 @@ public Iterable<Path> getRootDirectories() {
     return ImmutableSet.<Path>of(CloudStoragePath.getPath(this, UnixPath.ROOT));
   }
 
+  /**
+   * Returns nothing because GCS doesn't have disk partitions of limited size, or anything similar.
+   */
   @Override
   public Iterable<FileStore> getFileStores() {
     return ImmutableSet.of();
@@ -191,7 +219,7 @@ public Set<String> supportedFileAttributeViews() {
    */
   @Override
   public PathMatcher getPathMatcher(String syntaxAndPattern) {
-    // TODO: Implement me.
+    // TODO(#813): Implement me.
     throw new UnsupportedOperationException();
   }