Skip to content

Commit

Permalink
Address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
jart committed Mar 29, 2016
1 parent cd7cf1a commit c0a11cb
Show file tree
Hide file tree
Showing 10 changed files with 83 additions and 63 deletions.
29 changes: 22 additions & 7 deletions gcloud-java-contrib/gcloud-java-nio/pom.xml
Original file line number Diff line number Diff line change
@@ -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>
Expand Down Expand Up @@ -37,12 +36,6 @@
<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>
Expand All @@ -54,6 +47,28 @@
<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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public abstract class CloudStorageConfiguration {
/**
* Returns default GCS NIO configuration.
*/
public static CloudStorageConfiguration getDefault() {
public static CloudStorageConfiguration defaultInstance() {
return DEFAULT;
}

Expand All @@ -54,9 +54,13 @@ public static Builder builder() {
}

abstract String workingDirectory();

abstract boolean permitEmptyPathComponents();

abstract boolean stripPrefixSlash();

abstract boolean usePseudoDirectories();

abstract int blockSize();

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,11 +55,11 @@ public final class CloudStorageFileSystem extends FileSystem {

/**
* Invokes {@link #forBucket(String, CloudStorageConfiguration)} with
* {@link CloudStorageConfiguration#getDefault()}.
* {@link CloudStorageConfiguration#defaultInstance()}.
*/
@CheckReturnValue
public static CloudStorageFileSystem forBucket(String bucket) {
return forBucket(bucket, CloudStorageConfiguration.getDefault());
return forBucket(bucket, CloudStorageConfiguration.defaultInstance());
}

/**
Expand Down Expand Up @@ -154,10 +154,7 @@ public CloudStoragePath getPath(String first, String... more) {
*/
@Override
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.
// TODO(#809): Synchronously close all channels associated with this FileSystem instance.
}

/**
Expand Down Expand Up @@ -207,7 +204,7 @@ public Set<String> supportedFileAttributeViews() {
*/
@Override
public PathMatcher getPathMatcher(String syntaxAndPattern) {
// TODO: Implement me.
// TODO(#813): Implement me.
throw new UnsupportedOperationException();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,11 +113,11 @@ public CloudStorageFileSystem getFileSystem(URI uri) {
* Returns Cloud Storage file system, provided a URI with no path.
*
* <p><b>Note:</b> This method should be invoked indirectly via the SPI by calling
* {@link java.nio.file.FileSystems#newFileSystem(URI, Map) FileSystems.newFileSystem()}. However
* we recommend that you don't use the SPI if possible; the recommended approach is to write a
* dependency injection module that calls the statically-linked type-safe version of this method,
* which is: {@link CloudStorageFileSystem#forBucket(String, CloudStorageConfiguration)}. Please
* see that method for further documentation on creating GCS file systems.
* {@link java.nio.file.FileSystems#newFileSystem(URI, Map) FileSystems.newFileSystem()}; however,
* we recommend that you don't use the API if possible. The recommended approach is to write a
* dependency injection module that calls the statically-linked, type-safe version of this method:
* {@link CloudStorageFileSystem#forBucket(String, CloudStorageConfiguration)}. Please see that
* method for further documentation on creating GCS file systems.
*
* @param uri bucket and current working directory, e.g. {@code gs://bucket}
* @param env map of configuration options, whose keys correspond to the method names of
Expand Down Expand Up @@ -498,9 +498,9 @@ public <A extends BasicFileAttributes> A readAttributes(

@Override
public Map<String, Object> readAttributes(Path path, String attributes, LinkOption... options) {
// Java 7 NIO defines at least eleven string attributes we'd want to support
// (eg. BasicFileAttributeView and PosixFileAttributeView), so rather than a partial
// implementation we rely on the other overload for now.
// TODO(#811): Java 7 NIO defines at least eleven string attributes we'd want to support (eg.
// BasicFileAttributeView and PosixFileAttributeView), so rather than a partial
// implementation we rely on the other overload for now.
throw new UnsupportedOperationException();
}

Expand Down Expand Up @@ -532,7 +532,7 @@ public void createDirectory(Path dir, FileAttribute<?>... attrs) {
*/
@Override
public DirectoryStream<Path> newDirectoryStream(Path dir, Filter<? super Path> filter) {
// TODO: Implement me.
// TODO(#813): Implement me.
throw new UnsupportedOperationException();
}

Expand All @@ -541,6 +541,7 @@ public DirectoryStream<Path> newDirectoryStream(Path dir, Filter<? super Path> f
*/
@Override
public void setAttribute(Path path, String attribute, Object value, LinkOption... options) {
// TODO(#811): Implement me.
throw new CloudStorageObjectImmutableException();
}

Expand Down Expand Up @@ -572,7 +573,7 @@ public String toString() {
private IOException asIOException(StorageException oops) {
// RPC API can only throw StorageException, but CloudStorageFileSystemProvider
// can only throw IOException. Square peg, round hole.
// TODO: research if other codes should be translated similarly.
// TODO(#810): Research if other codes should be translated similarly.
if (oops.code() == 404) {
return new NoSuchFileException(oops.reason());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,8 +87,9 @@ public void testContentDisposition() throws IOException {
Path path = fs.getPath("/water");
Files.write(path, HAPPY, withContentDisposition("crash call"));
assertThat(
Files.readAttributes(
path, CloudStorageFileAttributes.class).contentDisposition().get())
Files.readAttributes(path, CloudStorageFileAttributes.class)
.contentDisposition()
.get())
.isEqualTo("crash call");
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -376,8 +376,8 @@ public void testWrite_absoluteObjectName_prefixSlashGetsRemoved() throws IOExcep
@Test
public void testWrite_absoluteObjectName_disableStrip_slashGetsPreserved() throws IOException {
try (CloudStorageFileSystem fs =
helper.forBucket(
"greenbean", CloudStorageConfiguration.builder().stripPrefixSlash(false).build())) {
helper.forBucket(
"greenbean", CloudStorageConfiguration.builder().stripPrefixSlash(false).build())) {
Path path = fs.getPath("/adipose/yep");
Files.write(path, FILE_CONTENTS, UTF_8);
assertThat(Files.readAllLines(path, UTF_8)).isEqualTo(FILE_CONTENTS);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,8 @@ public void testNullness() throws IOException, NoSuchMethodException, SecurityEx
NullPointerTester tester =
new NullPointerTester()
.ignore(CloudStorageFileSystem.class.getMethod("equals", Object.class))
.setDefault(CloudStorageConfiguration.class, CloudStorageConfiguration.getDefault());
.setDefault(
CloudStorageConfiguration.class, CloudStorageConfiguration.defaultInstance());
tester.testAllPublicStaticMethods(CloudStorageFileSystem.class);
tester.testAllPublicInstanceMethods(fs);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,9 @@ public void testWithContentDisposition() throws IOException {
Path path = fs.getPath("/path");
Files.write(path, "(✿◕ ‿◕ )ノ".getBytes(UTF_8), withContentDisposition("bubbly fun"));
assertThat(
Files.readAttributes(path, CloudStorageFileAttributes.class).contentDisposition().get())
Files.readAttributes(path, CloudStorageFileAttributes.class)
.contentDisposition()
.get())
.isEqualTo("bubbly fun");
}
}
Expand All @@ -95,7 +97,8 @@ public void testWithContentEncoding() throws IOException {
try (FileSystem fs = helper.forBucket("bucket")) {
Path path = fs.getPath("/path");
Files.write(path, "(✿◕ ‿◕ )ノ".getBytes(UTF_8), withContentEncoding("gzip"));
assertThat(Files.readAttributes(path, CloudStorageFileAttributes.class).contentEncoding().get())
assertThat(
Files.readAttributes(path, CloudStorageFileAttributes.class).contentEncoding().get())
.isEqualTo("gzip");
}
}
Expand All @@ -110,7 +113,9 @@ public void testWithUserMetadata() throws IOException {
withUserMetadata("nolo", "contendere"),
withUserMetadata("eternal", "sadness"));
assertThat(
Files.readAttributes(path, CloudStorageFileAttributes.class).userMetadata().get("nolo"))
Files.readAttributes(path, CloudStorageFileAttributes.class)
.userMetadata()
.get("nolo"))
.isEqualTo("contendere");
assertThat(
Files.readAttributes(path, CloudStorageFileAttributes.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ final class NioTestHelper {
}

CloudStorageFileSystem forBucket(String bucket) {
return forBucket(bucket, CloudStorageConfiguration.getDefault());
return forBucket(bucket, CloudStorageConfiguration.defaultInstance());
}

CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
Expand All @@ -35,12 +35,12 @@ private static Storage makeStorage(final StorageRpc storageRpc) {
return StorageOptions.builder()
.projectId("dummy-project-for-testing")
.serviceRpcFactory(
new ServiceRpcFactory<StorageRpc, StorageOptions>() {
@Override
public StorageRpc create(StorageOptions options) {
return storageRpc;
}
})
new ServiceRpcFactory<StorageRpc, StorageOptions>() {
@Override
public StorageRpc create(StorageOptions options) {
return storageRpc;
}
})
.build()
.service();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,32 +39,29 @@
import java.util.logging.Level;
import java.util.logging.Logger;


/**
* Integration test for gcloud-nio. This test actually talks to GCS (you need an account).
* Tests both reading and writing.
*
* You *must* set the 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.
* Integration test for gcloud-nio.
*
* The instructions for how to get the Service Account JSON Key are
* at https://cloud.google.com/storage/docs/authentication?hl=en#service_accounts
* <p>This test actually talks to GCS (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.
*
* The short version is this: go to cloud.google.com/console,
* select your project, search for "API manager", click "Credentials",
* click "create credentials/service account key", new service account,
* JSON. The contents of the file that's sent to your browsers is your
* "Service Account JSON Key".
* <p>The instructions for how to get the Service Account JSON Key are
* <a href="https://cloud.google.com/storage/docs/authentication?hl=en#service_accounts">here</a>.
*
* <p>The short version is this: go to <a href="https://cloud.google.com/console">Cloud Console</a>,
* select your project, search for "API manager", click "Credentials", click "create
* credentials/service account key", new service account, JSON. The contents of the file that's sent
* to your browsers is your "Service Account JSON Key".
*/
@RunWith(JUnit4.class)
public class ITGcsNio {

private static final List<String> FILE_CONTENTS = ImmutableList.of(
"Tous les êtres humains naissent libres et égaux en dignité et en droits.",
"Ils sont doués de raison et de conscience et doivent agir ",
"les uns envers les autres dans un esprit de fraternité.");
private static final List<String> FILE_CONTENTS =
ImmutableList.of(
"Tous les êtres humains naissent libres et égaux en dignité et en droits.",
"Ils sont doués de raison et de conscience et doivent agir ",
"les uns envers les autres dans un esprit de fraternité.");

private static final Logger log = Logger.getLogger(ITGcsNio.class.getName());
private static final String BUCKET = RemoteGcsHelper.generateBucketName();
Expand Down Expand Up @@ -94,9 +91,10 @@ public static void beforeClass() throws IOException {

@AfterClass
public static void afterClass() throws ExecutionException, InterruptedException {
if (storage != null && !RemoteGcsHelper.forceDelete(storage, BUCKET, 5, TimeUnit.SECONDS) &&
log.isLoggable(Level.WARNING)) {
log.log(Level.WARNING, "Deletion of bucket {0} timed out, bucket is not empty", BUCKET);
if (storage != null
&& !RemoteGcsHelper.forceDelete(storage, BUCKET, 5, TimeUnit.SECONDS)
&& log.isLoggable(Level.WARNING)) {
log.log(Level.WARNING, "Deletion of bucket {0} timed out, bucket is not empty", BUCKET);
}
}

Expand Down Expand Up @@ -335,7 +333,6 @@ private String randomSuffix() {
return "-" + rnd.nextInt(99999);
}


private CloudStorageFileSystem getTestBucket() throws IOException {
// in typical usage we use the single-argument version of forBucket
// and rely on the user being logged into their project with the
Expand All @@ -348,5 +345,4 @@ private CloudStorageFileSystem getTestBucket() throws IOException {
return CloudStorageFileSystem.forBucket(
BUCKET, CloudStorageConfiguration.DEFAULT, storageOptions);
}

}

0 comments on commit c0a11cb

Please sign in to comment.