From 99179e822421e5280040f7baa95ada91b52c9f04 Mon Sep 17 00:00:00 2001 From: Louis Bergelson Date: Thu, 20 Jul 2023 12:36:29 -0700 Subject: [PATCH] feat: Store bucket name in URI authority (#1218) (#1219) --- .../contrib/nio/CloudStorageFileSystem.java | 4 +- .../nio/CloudStorageFileSystemProvider.java | 10 +++-- .../storage/contrib/nio/CloudStoragePath.java | 14 +++++- .../storage/contrib/nio/CloudStorageUtil.java | 9 +++- .../CloudStorageFileSystemProviderTest.java | 44 +++++++++++++++++++ .../nio/CloudStorageFileSystemTest.java | 42 ++++++++++++++++++ 6 files changed, 117 insertions(+), 6 deletions(-) diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java index 8f923c50..b1cc374d 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystem.java @@ -332,7 +332,9 @@ public int hashCode() { @Override public String toString() { try { - return new URI(URI_SCHEME, bucket, null, null).toString(); + // Store GCS bucket name in the URI authority, see + // https://github.com/googleapis/java-storage-nio/issues/1218 + return new URI(URI_SCHEME, bucket, null, null, null).toString(); } catch (URISyntaxException e) { throw new AssertionError(e); } diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java index bba5d875..823e99e4 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProvider.java @@ -254,8 +254,12 @@ public CloudStorageFileSystem newFileSystem(URI uri, Map env) { "Cloud Storage URIs must have '%s' scheme: %s", CloudStorageFileSystem.URI_SCHEME, uri); + // Bucket names may not be compatible with getHost(), see + // https://github.com/googleapis/java-storage-nio/issues/1218 + // However, there may be existing code expecting the exception message to refer to the bucket + // name as the "host". checkArgument( - !isNullOrEmpty(uri.getHost()), + !isNullOrEmpty(uri.getAuthority()), "%s:// URIs must have a host: %s", CloudStorageFileSystem.URI_SCHEME, uri); @@ -266,11 +270,11 @@ && isNullOrEmpty(uri.getFragment()) && isNullOrEmpty(uri.getUserInfo()), "GCS FileSystem URIs mustn't have: port, userinfo, query, or fragment: %s", uri); - CloudStorageUtil.checkBucket(uri.getHost()); + CloudStorageUtil.checkBucket(uri.getAuthority()); initStorage(); return new CloudStorageFileSystem( this, - uri.getHost(), + uri.getAuthority(), CloudStorageConfiguration.fromMap( CloudStorageFileSystem.getDefaultCloudStorageConfiguration(), env)); } diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java index 29b32229..1c49f8e1 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStoragePath.java @@ -368,10 +368,22 @@ public String toString() { @Override public URI toUri() { try { + // First try storing GCS bucket name in the hostname for compatibility with earlier behavior. return new URI( CloudStorageFileSystem.URI_SCHEME, bucket(), path.toAbsolutePath().toString(), null); } catch (URISyntaxException e) { - throw new AssertionError(e); + try { + // Store GCS bucket name in the URI authority, see + // https://github.com/googleapis/java-storage-nio/issues/1218 + return new URI( + CloudStorageFileSystem.URI_SCHEME, + bucket(), + path.toAbsolutePath().toString(), + null, + null); + } catch (URISyntaxException unused) { + throw new AssertionError(e); + } } } diff --git a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageUtil.java b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageUtil.java index eda81fcd..3802fef0 100644 --- a/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageUtil.java +++ b/google-cloud-nio/src/main/java/com/google/cloud/storage/contrib/nio/CloudStorageUtil.java @@ -62,7 +62,14 @@ static URI stripPathFromUri(URI uri) { uri.getQuery(), uri.getFragment()); } catch (URISyntaxException e) { - throw new IllegalArgumentException(e.getMessage()); + try { + // Store GCS bucket name in the URI authority, see + // https://github.com/googleapis/java-storage-nio/issues/1218 + return new URI( + uri.getScheme(), uri.getAuthority(), null, uri.getQuery(), uri.getFragment()); + } catch (URISyntaxException unused) { + throw new IllegalArgumentException(e.getMessage()); + } } } diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java index f44a932c..7f6bc0df 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemProviderTest.java @@ -815,6 +815,50 @@ public void testFromSpace() throws Exception { assertThat(path4.toString()).isEqualTo("/with/a%20percent"); } + @Test + public void testBucketWithHost() { + // User should be able to create buckets whose name contains a host name. + Path path1 = Paths.get(URI.create("gs://bucket-with-host/path")); + CloudStorageFileSystemProvider provider = + (CloudStorageFileSystemProvider) path1.getFileSystem().provider(); + + Path path2 = provider.getPath("gs://bucket-with-host/path"); + // Both approaches should be equivalent + assertThat(path1.getFileSystem().provider()).isEqualTo(path2.getFileSystem().provider()); + assertThat(path1.toUri()).isEqualTo(path2.toUri()); + assertThat(path1.toUri().getHost()).isEqualTo("bucket-with-host"); + assertThat(path1.toUri().getAuthority()).isEqualTo("bucket-with-host"); + } + + @Test + public void testBucketWithAuthority() { + // User should be able to create buckets whose name contains an authority that is not a host. + Path path1 = Paths.get(URI.create("gs://bucket_with_authority/path")); + CloudStorageFileSystemProvider provider = + (CloudStorageFileSystemProvider) path1.getFileSystem().provider(); + + Path path2 = provider.getPath("gs://bucket_with_authority/path"); + // Both approaches should be equivalent + assertThat(path1.getFileSystem().provider()).isEqualTo(path2.getFileSystem().provider()); + assertThat(path1.toUri()).isEqualTo(path2.toUri()); + assertThat(path1.toUri().getHost()).isNull(); + assertThat(path1.toUri().getAuthority()).isEqualTo("bucket_with_authority"); + } + + @Test + public void testBucketWithoutAuthority() { + Path path1 = Paths.get(URI.create("gs://bucket_with_authority/path")); + CloudStorageFileSystemProvider provider = + (CloudStorageFileSystemProvider) path1.getFileSystem().provider(); + + try { + provider.getFileSystem(URI.create("gs:///path")); + Assert.fail(); + } catch (IllegalArgumentException e) { + assertThat(e.getMessage()).isEqualTo("gs:// URIs must have a host: gs:///path"); + } + } + @Test public void testVersion_matchesAcceptablePatterns() { String acceptableVersionPattern = "|(?:\\d+\\.\\d+\\.\\d+(?:-.*?)?(?:-SNAPSHOT)?)"; diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java index 4033e788..6c63a7e6 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageFileSystemTest.java @@ -139,6 +139,48 @@ public void testGetPath() throws IOException { } } + @Test + public void testGetHost_valid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) { + assertThat(fs.getPath("/angel").toUri().getHost()).isEqualTo("bucket-with-host"); + } + } + + @Test + public void testGetHost_invalid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) { + assertThat(fs.getPath("/angel").toUri().getHost()).isNull(); + } + } + + @Test + public void testGetAuthority_valid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) { + assertThat(fs.getPath("/angel").toUri().getAuthority()).isEqualTo("bucket-with-host"); + } + } + + @Test + public void testGetAuthority_invalid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) { + assertThat(fs.getPath("/angel").toUri().getAuthority()).isEqualTo("bucket_with_authority"); + } + } + + @Test + public void testToString_valid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket-with-host")) { + assertThat(fs.toString()).isEqualTo("gs://bucket-with-host"); + } + } + + @Test + public void testToString_invalid_dns() throws IOException { + try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket_with_authority")) { + assertThat(fs.toString()).isEqualTo("gs://bucket_with_authority"); + } + } + @Test public void testWrite() throws IOException { try (FileSystem fs = CloudStorageFileSystem.forBucket("bucket")) {