Make some final changes to the GCS NIO API

This change prepares GCS NIO to be merged into master, by making some
last minute API changes.

- Documentation has been added to many methods to clarify the preferred
  method for instantiating the library, which is the non-SPI version.

- Unit tests have been updated to not rely on the SPI, because there's
  no way to guarantee clean isolation of SPI usage across tests. We'll
  be relying on integration testing to test the SPI interface.

- The unit testing methodology has changed somewhat. FakeStorageRpc
  should be a private final field on the test class so, if desired,
  we'll be able to have the tests dip directly into fake memory.

- IOException has been added back to the throws of file system close, in
  case we decide to implement the "close all owned channels" thing into
  that method in the future.

- The getters on the configuration class have been made package-private,
  since there's no foreseeable reason they would be needed by the user.

- Injectable constructors have been added for Dagger 2 users.

In a future change, a README.md file will be added to replace the
documentation in the package-info.java file.

Author: jart

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

@@ -49,6 +49,12 @@
       <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 -->
+    </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

@@ -3,43 +3,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 getDefault() {
+    return DEFAULT;
+  }
 
   /**
    * Creates a new builder, initialized with the following settings:
@@ -54,22 +37,29 @@ 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);
@@ -79,7 +69,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;
@@ -130,15 +120,13 @@ public CloudStorageConfiguration build() {
     Builder() {}
   }
 
-  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

@@ -3,7 +3,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.common.collect.Iterables;
 
 import java.io.IOException;
 import java.net.URI;
@@ -15,10 +19,12 @@
 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 javax.annotation.concurrent.Immutable;
+import javax.annotation.CheckReturnValue;
+import javax.annotation.concurrent.ThreadSafe;
 
 /**
  * Google Cloud Storage {@link FileSystem} implementation.
@@ -28,38 +34,48 @@
  * @see <a href="https://developers.google.com/storage/docs/bucketnaming">
  *        Bucket and Object Naming Guidelines</a>
  */
-@Immutable
+@ThreadSafe
 public final class CloudStorageFileSystem extends FileSystem {
 
   /**
-   * 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(java.net.URI)
+   * Invokes {@link #forBucket(String, CloudStorageConfiguration)} with
+   * {@link CloudStorageConfiguration#getDefault()}.
    */
+  @CheckReturnValue
   public static CloudStorageFileSystem forBucket(String bucket) {
-    return forBucket(bucket, CloudStorageConfiguration.DEFAULT);
+    return forBucket(bucket, CloudStorageConfiguration.getDefault());
   }
 
   /**
-   * 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 (SPI), e.g. {@code FileSystems.getFileSystem(URI.create("gs://bucket"))}. We
+   * discourage you from using the SPI if possible, for the reasons documented in
+   * {@link CloudStorageFileSystemProvider#newFileSystem(URI, java.util.Map)}
+   *
+   * @see #forBucket(String, CloudStorageConfiguration)
+   * @see java.nio.file.FileSystems#getFileSystem(java.net.URI)
    */
+  @CheckReturnValue
   public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
+    checkNotNull(config);
     checkArgument(
         !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket);
     return new CloudStorageFileSystem(
-        new CloudStorageFileSystemProvider(), bucket, checkNotNull(config));
+        // XXX: This is a kludge to get the provider instance from the SPI. This is necessary since
+        //      the behavior of NIO changes quite a bit if the provider instances aren't the same.
+        (CloudStorageFileSystemProvider)
+            Iterables.getOnlyElement(
+                Iterables.filter(
+                    FileSystemProvider.installedProviders(),
+                    Predicates.instanceOf(CloudStorageFileSystemProvider.class))),
+        config,
+        bucket);
   }
 
   public static final String URI_SCHEME = "gs";
@@ -69,12 +85,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;
@@ -113,13 +132,20 @@ 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(jean-philippe-martin,jart): Synchronously close all active channels associated with this
+    //                                  FileSystem instance on close, per NIO documentation. But we
+    //                                  probably shouldn't bother unless a legitimate reason can be
+    //                                  found to implement this behavior.
+  }
 
   /**
-   * Returns {@code true}.
+   * Returns {@code true}, even if you previously called the {@link #close()} method.
    */
   @Override
   public boolean isOpen() {
@@ -147,6 +173,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();