Skip to content

Commit 963b3aa

Browse files
lbergelsonlbergelson
authored
feat: Store bucket name in URI authority (#1218) (#1219)
1 parent 55b8b5e commit 963b3aa

File tree

6 files changed

+117
-6
lines changed

6 files changed

+117
-6
lines changed

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -332,7 +332,9 @@ public int hashCode() {
332332
@Override
333333
public String toString() {
334334
try {
335-
return new URI(URI_SCHEME, bucket, null, null).toString();
335+
// Store GCS bucket name in the URI authority, see
336+
// https://github.com/googleapis/java-storage-nio/issues/1218
337+
return new URI(URI_SCHEME, bucket, null, null, null).toString();
336338
} catch (URISyntaxException e) {
337339
throw new AssertionError(e);
338340
}

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -254,8 +254,12 @@ public CloudStorageFileSystem newFileSystem(URI uri, Map<String, ?> env) {
254254
"Cloud Storage URIs must have '%s' scheme: %s",
255255
CloudStorageFileSystem.URI_SCHEME,
256256
uri);
257+
// Bucket names may not be compatible with getHost(), see
258+
// https://github.com/googleapis/java-storage-nio/issues/1218
259+
// However, there may be existing code expecting the exception message to refer to the bucket
260+
// name as the "host".
257261
checkArgument(
258-
!isNullOrEmpty(uri.getHost()),
262+
!isNullOrEmpty(uri.getAuthority()),
259263
"%s:// URIs must have a host: %s",
260264
CloudStorageFileSystem.URI_SCHEME,
261265
uri);
@@ -266,11 +270,11 @@ && isNullOrEmpty(uri.getFragment())
266270
&& isNullOrEmpty(uri.getUserInfo()),
267271
"GCS FileSystem URIs mustn't have: port, userinfo, query, or fragment: %s",
268272
uri);
269-
CloudStorageUtil.checkBucket(uri.getHost());
273+
CloudStorageUtil.checkBucket(uri.getAuthority());
270274
initStorage();
271275
return new CloudStorageFileSystem(
272276
this,
273-
uri.getHost(),
277+
uri.getAuthority(),
274278
CloudStorageConfiguration.fromMap(
275279
CloudStorageFileSystem.getDefaultCloudStorageConfiguration(), env));
276280
}

‎java-storage-nio/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -368,10 +368,22 @@ public String toString() {
368368
@Override
369369
public URI toUri() {
370370
try {
371+
// First try storing GCS bucket name in the hostname for compatibility with earlier behavior.
371372
return new URI(
372373
CloudStorageFileSystem.URI_SCHEME, bucket(), path.toAbsolutePath().toString(), null);
373374
} catch (URISyntaxException e) {
374-
throw new AssertionError(e);
375+
try {
376+
// Store GCS bucket name in the URI authority, see
377+
// https://github.com/googleapis/java-storage-nio/issues/1218
378+
return new URI(
379+
CloudStorageFileSystem.URI_SCHEME,
380+
bucket(),
381+
path.toAbsolutePath().toString(),
382+
null,
383+
null);
384+
} catch (URISyntaxException unused) {
385+
throw new AssertionError(e);
386+
}
375387
}
376388
}
377389

‎java-storage-nio/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageUtil.java‎

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,14 @@ static URI stripPathFromUri(URI uri) {
6262
uri.getQuery(),
6363
uri.getFragment());
6464
} catch (URISyntaxException e) {
65-
throw new IllegalArgumentException(e.getMessage());
65+
try {
66+
// Store GCS bucket name in the URI authority, see
67+
// https://github.com/googleapis/java-storage-nio/issues/1218
68+
return new URI(
69+
uri.getScheme(), uri.getAuthority(), null, uri.getQuery(), uri.getFragment());
70+
} catch (URISyntaxException unused) {
71+
throw new IllegalArgumentException(e.getMessage());
72+
}
6673
}
6774
}
6875

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

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -815,6 +815,50 @@ public void testFromSpace() throws Exception {
815815
assertThat(path4.toString()).isEqualTo("/with/a%20percent");
816816
}
817817

818+
@Test
819+
public void testBucketWithHost() {
820+
// User should be able to create buckets whose name contains a host name.
821+
Path path1 = Paths.get(URI.create("gs://bucket-with-host/path"));
822+
CloudStorageFileSystemProvider provider =
823+
(CloudStorageFileSystemProvider) path1.getFileSystem().provider();
824+
825+
Path path2 = provider.getPath("gs://bucket-with-host/path");
826+
// Both approaches should be equivalent
827+
assertThat(path1.getFileSystem().provider()).isEqualTo(path2.getFileSystem().provider());
828+
assertThat(path1.toUri()).isEqualTo(path2.toUri());
829+
assertThat(path1.toUri().getHost()).isEqualTo("bucket-with-host");
830+
assertThat(path1.toUri().getAuthority()).isEqualTo("bucket-with-host");
831+
}
832+
833+
@Test
834+
public void testBucketWithAuthority() {
835+
// User should be able to create buckets whose name contains an authority that is not a host.
836+
Path path1 = Paths.get(URI.create("gs://bucket_with_authority/path"));
837+
CloudStorageFileSystemProvider provider =
838+
(CloudStorageFileSystemProvider) path1.getFileSystem().provider();
839+
840+
Path path2 = provider.getPath("gs://bucket_with_authority/path");
841+
// Both approaches should be equivalent
842+
assertThat(path1.getFileSystem().provider()).isEqualTo(path2.getFileSystem().provider());
843+
assertThat(path1.toUri()).isEqualTo(path2.toUri());
844+
assertThat(path1.toUri().getHost()).isNull();
845+
assertThat(path1.toUri().getAuthority()).isEqualTo("bucket_with_authority");
846+
}
847+
848+
@Test
849+
public void testBucketWithoutAuthority() {
850+
Path path1 = Paths.get(URI.create("gs://bucket_with_authority/path"));
851+
CloudStorageFileSystemProvider provider =
852+
(CloudStorageFileSystemProvider) path1.getFileSystem().provider();
853+
854+
try {
855+
provider.getFileSystem(URI.create("gs:///path"));
856+
Assert.fail();
857+
} catch (IllegalArgumentException e) {
858+
assertThat(e.getMessage()).isEqualTo("gs:// URIs must have a host: gs:///path");
859+
}
860+
}
861+
818862
@Test
819863
public void testVersion_matchesAcceptablePatterns() {
820864
String acceptableVersionPattern = "|(?:\\d+\\.\\d+\\.\\d+(?:-.*?)?(?:-SNAPSHOT)?)";

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

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,48 @@ public void testGetPath() throws IOException {
139139
}
140140
}
141141

142+
@Test
143+
public void testGetHost_valid_dns() throws IOException {
144+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) {
145+
assertThat(fs.getPath("/angel").toUri().getHost()).isEqualTo("bucket-with-host");
146+
}
147+
}
148+
149+
@Test
150+
public void testGetHost_invalid_dns() throws IOException {
151+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) {
152+
assertThat(fs.getPath("/angel").toUri().getHost()).isNull();
153+
}
154+
}
155+
156+
@Test
157+
public void testGetAuthority_valid_dns() throws IOException {
158+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) {
159+
assertThat(fs.getPath("/angel").toUri().getAuthority()).isEqualTo("bucket-with-host");
160+
}
161+
}
162+
163+
@Test
164+
public void testGetAuthority_invalid_dns() throws IOException {
165+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) {
166+
assertThat(fs.getPath("/angel").toUri().getAuthority()).isEqualTo("bucket_with_authority");
167+
}
168+
}
169+
170+
@Test
171+
public void testToString_valid_dns() throws IOException {
172+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) {
173+
assertThat(fs.toString()).isEqualTo("gs://bucket-with-host");
174+
}
175+
}
176+
177+
@Test
178+
public void testToString_invalid_dns() throws IOException {
179+
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) {
180+
assertThat(fs.toString()).isEqualTo("gs://bucket_with_authority");
181+
}
182+
}
183+
142184
@Test
143185
public void testWrite() throws IOException {
144186
try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket")) {

0 commit comments

Comments
 (0)