Skip to content

Commit c0a11cb

Browse files
jartjart
committed
Address review comments
1 parent cd7cf1a commit c0a11cb

File tree

10 files changed

+83
-63
lines changed

10 files changed

+83
-63
lines changed

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

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
<?xml version="1.0"?>
22
<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">
33
<modelVersion>4.0.0</modelVersion>
4-
<groupId>com.google.gcloud</groupId>
54
<artifactId>gcloud-java-nio</artifactId>
65
<packaging>jar</packaging>
76
<name>GCloud Java NIO</name>
@@ -37,12 +36,6 @@
3736
<artifactId>javax.inject</artifactId>
3837
<version>1</version>
3938
</dependency>
40-
<dependency>
41-
<groupId>com.google.auto.service</groupId>
42-
<artifactId>auto-service</artifactId>
43-
<version>1.0-rc2</version>
44-
<scope>provided</scope> <!-- to leave out of the all-deps jar -->
45-
</dependency>
4639
<dependency>
4740
<groupId>com.google.auto.value</groupId>
4841
<artifactId>auto-value</artifactId>
@@ -54,6 +47,28 @@
5447
<artifactId>auto-factory</artifactId>
5548
<version>1.0-beta3</version>
5649
<scope>provided</scope> <!-- to leave out of the all-deps jar -->
50+
<exclusions>
51+
<exclusion>
52+
<groupId>com.google.guava</groupId>
53+
<artifactId>guava</artifactId>
54+
</exclusion>
55+
</exclusions>
56+
</dependency>
57+
<dependency>
58+
<groupId>com.google.auto.service</groupId>
59+
<artifactId>auto-service</artifactId>
60+
<version>1.0-rc2</version>
61+
<scope>provided</scope> <!-- to leave out of the all-deps jar -->
62+
<exclusions>
63+
<exclusion>
64+
<groupId>com.google.guava</groupId>
65+
<artifactId>guava</artifactId>
66+
</exclusion>
67+
<exclusion>
68+
<groupId>com.google.auto</groupId>
69+
<artifactId>auto-common</artifactId>
70+
</exclusion>
71+
</exclusions>
5772
</dependency>
5873
<dependency>
5974
<groupId>junit</groupId>

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ public abstract class CloudStorageConfiguration {
3636
/**
3737
* Returns default GCS NIO configuration.
3838
*/
39-
public static CloudStorageConfiguration getDefault() {
39+
public static CloudStorageConfiguration defaultInstance() {
4040
return DEFAULT;
4141
}
4242

@@ -54,9 +54,13 @@ public static Builder builder() {
5454
}
5555

5656
abstract String workingDirectory();
57+
5758
abstract boolean permitEmptyPathComponents();
59+
5860
abstract boolean stripPrefixSlash();
61+
5962
abstract boolean usePseudoDirectories();
63+
6064
abstract int blockSize();
6165

6266
/**

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,11 @@ public final class CloudStorageFileSystem extends FileSystem {
5555

5656
/**
5757
* Invokes {@link #forBucket(String, CloudStorageConfiguration)} with
58-
* {@link CloudStorageConfiguration#getDefault()}.
58+
* {@link CloudStorageConfiguration#defaultInstance()}.
5959
*/
6060
@CheckReturnValue
6161
public static CloudStorageFileSystem forBucket(String bucket) {
62-
return forBucket(bucket, CloudStorageConfiguration.getDefault());
62+
return forBucket(bucket, CloudStorageConfiguration.defaultInstance());
6363
}
6464

6565
/**
@@ -154,10 +154,7 @@ public CloudStoragePath getPath(String first, String... more) {
154154
*/
155155
@Override
156156
public void close() throws IOException {
157-
// TODO(jean-philippe-martin,jart): Synchronously close all active channels associated with this
158-
// FileSystem instance on close, per NIO documentation. But we
159-
// probably shouldn't bother unless a legitimate reason can be
160-
// found to implement this behavior.
157+
// TODO(#809): Synchronously close all channels associated with this FileSystem instance.
161158
}
162159

163160
/**
@@ -207,7 +204,7 @@ public Set<String> supportedFileAttributeViews() {
207204
*/
208205
@Override
209206
public PathMatcher getPathMatcher(String syntaxAndPattern) {
210-
// TODO: Implement me.
207+
// TODO(#813): Implement me.
211208
throw new UnsupportedOperationException();
212209
}
213210

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

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

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

@@ -532,7 +532,7 @@ public void createDirectory(Path dir, FileAttribute<?>... attrs) {
532532
*/
533533
@Override
534534
public DirectoryStream<Path> newDirectoryStream(Path dir, Filter<? super Path> filter) {
535-
// TODO: Implement me.
535+
// TODO(#813): Implement me.
536536
throw new UnsupportedOperationException();
537537
}
538538

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

@@ -572,7 +573,7 @@ public String toString() {
572573
private IOException asIOException(StorageException oops) {
573574
// RPC API can only throw StorageException, but CloudStorageFileSystemProvider
574575
// can only throw IOException. Square peg, round hole.
575-
// TODO: research if other codes should be translated similarly.
576+
// TODO(#810): Research if other codes should be translated similarly.
576577
if (oops.code() == 404) {
577578
return new NoSuchFileException(oops.reason());
578579
}

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileAttributesTest.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -87,8 +87,9 @@ public void testContentDisposition() throws IOException {
8787
Path path = fs.getPath("/water");
8888
Files.write(path, HAPPY, withContentDisposition("crash call"));
8989
assertThat(
90-
Files.readAttributes(
91-
path, CloudStorageFileAttributes.class).contentDisposition().get())
90+
Files.readAttributes(path, CloudStorageFileAttributes.class)
91+
.contentDisposition()
92+
.get())
9293
.isEqualTo("crash call");
9394
}
9495
}

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -376,8 +376,8 @@ public void testWrite_absoluteObjectName_prefixSlashGetsRemoved() throws IOExcep
376376
@Test
377377
public void testWrite_absoluteObjectName_disableStrip_slashGetsPreserved() throws IOException {
378378
try (CloudStorageFileSystem fs =
379-
helper.forBucket(
380-
"greenbean", CloudStorageConfiguration.builder().stripPrefixSlash(false).build())) {
379+
helper.forBucket(
380+
"greenbean", CloudStorageConfiguration.builder().stripPrefixSlash(false).build())) {
381381
Path path = fs.getPath("/adipose/yep");
382382
Files.write(path, FILE_CONTENTS, UTF_8);
383383
assertThat(Files.readAllLines(path, UTF_8)).isEqualTo(FILE_CONTENTS);

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/CloudStorageFileSystemTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,8 @@ public void testNullness() throws IOException, NoSuchMethodException, SecurityEx
120120
NullPointerTester tester =
121121
new NullPointerTester()
122122
.ignore(CloudStorageFileSystem.class.getMethod("equals", Object.class))
123-
.setDefault(CloudStorageConfiguration.class, CloudStorageConfiguration.getDefault());
123+
.setDefault(
124+
CloudStorageConfiguration.class, CloudStorageConfiguration.defaultInstance());
124125
tester.testAllPublicStaticMethods(CloudStorageFileSystem.class);
125126
tester.testAllPublicInstanceMethods(fs);
126127
}

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/CloudStorageOptionsTest.java‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,9 @@ public void testWithContentDisposition() throws IOException {
8585
Path path = fs.getPath("/path");
8686
Files.write(path, "(✿◕ ‿◕ )ノ".getBytes(UTF_8), withContentDisposition("bubbly fun"));
8787
assertThat(
88-
Files.readAttributes(path, CloudStorageFileAttributes.class).contentDisposition().get())
88+
Files.readAttributes(path, CloudStorageFileAttributes.class)
89+
.contentDisposition()
90+
.get())
8991
.isEqualTo("bubbly fun");
9092
}
9193
}
@@ -95,7 +97,8 @@ public void testWithContentEncoding() throws IOException {
9597
try (FileSystem fs = helper.forBucket("bucket")) {
9698
Path path = fs.getPath("/path");
9799
Files.write(path, "(✿◕ ‿◕ )ノ".getBytes(UTF_8), withContentEncoding("gzip"));
98-
assertThat(Files.readAttributes(path, CloudStorageFileAttributes.class).contentEncoding().get())
100+
assertThat(
101+
Files.readAttributes(path, CloudStorageFileAttributes.class).contentEncoding().get())
99102
.isEqualTo("gzip");
100103
}
101104
}
@@ -110,7 +113,9 @@ public void testWithUserMetadata() throws IOException {
110113
withUserMetadata("nolo", "contendere"),
111114
withUserMetadata("eternal", "sadness"));
112115
assertThat(
113-
Files.readAttributes(path, CloudStorageFileAttributes.class).userMetadata().get("nolo"))
116+
Files.readAttributes(path, CloudStorageFileAttributes.class)
117+
.userMetadata()
118+
.get("nolo"))
114119
.isEqualTo("contendere");
115120
assertThat(
116121
Files.readAttributes(path, CloudStorageFileAttributes.class)

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/NioTestHelper.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ final class NioTestHelper {
2424
}
2525

2626
CloudStorageFileSystem forBucket(String bucket) {
27-
return forBucket(bucket, CloudStorageConfiguration.getDefault());
27+
return forBucket(bucket, CloudStorageConfiguration.defaultInstance());
2828
}
2929

3030
CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) {
@@ -35,12 +35,12 @@ private static Storage makeStorage(final StorageRpc storageRpc) {
3535
return StorageOptions.builder()
3636
.projectId("dummy-project-for-testing")
3737
.serviceRpcFactory(
38-
new ServiceRpcFactory<StorageRpc, StorageOptions>() {
39-
@Override
40-
public StorageRpc create(StorageOptions options) {
41-
return storageRpc;
42-
}
43-
})
38+
new ServiceRpcFactory<StorageRpc, StorageOptions>() {
39+
@Override
40+
public StorageRpc create(StorageOptions options) {
41+
return storageRpc;
42+
}
43+
})
4444
.build()
4545
.service();
4646
}

‎gcloud-java-contrib/gcloud-java-nio/src/test/java/com/google/gcloud/storage/contrib/nio/it/ITGcsNio.java‎

Lines changed: 19 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -39,32 +39,29 @@
3939
import java.util.logging.Level;
4040
import java.util.logging.Logger;
4141

42-
4342
/**
44-
* Integration test for gcloud-nio. This test actually talks to GCS (you need an account).
45-
* Tests both reading and writing.
46-
*
47-
* You *must* set the GOOGLE_APPLICATION_CREDENTIALS environment variable
48-
* for this test to work. It must contain the name of a local file that contains
49-
* your Service Account JSON Key.
43+
* Integration test for gcloud-nio.
5044
*
51-
* The instructions for how to get the Service Account JSON Key are
52-
* at https://cloud.google.com/storage/docs/authentication?hl=en#service_accounts
45+
* <p>This test actually talks to GCS (you need an account) and tests both reading and writing. You
46+
* *must* set the {@code GOOGLE_APPLICATION_CREDENTIALS} environment variable for this test to work.
47+
* It must contain the name of a local file that contains your Service Account JSON Key.
5348
*
54-
* The short version is this: go to cloud.google.com/console,
55-
* select your project, search for "API manager", click "Credentials",
56-
* click "create credentials/service account key", new service account,
57-
* JSON. The contents of the file that's sent to your browsers is your
58-
* "Service Account JSON Key".
49+
* <p>The instructions for how to get the Service Account JSON Key are
50+
* <a href="https://cloud.google.com/storage/docs/authentication?hl=en#service_accounts">here</a>.
5951
*
52+
* <p>The short version is this: go to <a href="https://cloud.google.com/console">Cloud Console</a>,
53+
* select your project, search for "API manager", click "Credentials", click "create
54+
* credentials/service account key", new service account, JSON. The contents of the file that's sent
55+
* to your browsers is your "Service Account JSON Key".
6056
*/
6157
@RunWith(JUnit4.class)
6258
public class ITGcsNio {
6359

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

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

9592
@AfterClass
9693
public static void afterClass() throws ExecutionException, InterruptedException {
97-
if (storage != null && !RemoteGcsHelper.forceDelete(storage, BUCKET, 5, TimeUnit.SECONDS) &&
98-
log.isLoggable(Level.WARNING)) {
99-
log.log(Level.WARNING, "Deletion of bucket {0} timed out, bucket is not empty", BUCKET);
94+
if (storage != null
95+
&& !RemoteGcsHelper.forceDelete(storage, BUCKET, 5, TimeUnit.SECONDS)
96+
&& log.isLoggable(Level.WARNING)) {
97+
log.log(Level.WARNING, "Deletion of bucket {0} timed out, bucket is not empty", BUCKET);
10098
}
10199
}
102100

@@ -335,7 +333,6 @@ private String randomSuffix() {
335333
return "-" + rnd.nextInt(99999);
336334
}
337335

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

0 commit comments

Comments
 (0)