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 8c1a13d7..e73ee603 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 @@ -18,6 +18,7 @@ import static com.google.common.base.Preconditions.checkArgument; import static com.google.common.base.Preconditions.checkNotNull; +import static java.util.Objects.requireNonNull; import com.google.api.gax.paging.Page; import com.google.cloud.storage.Bucket; @@ -25,7 +26,11 @@ import com.google.cloud.storage.StorageException; import com.google.cloud.storage.StorageOptions; import com.google.common.base.Strings; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.CacheLoader; +import com.google.common.cache.LoadingCache; import com.google.common.collect.ImmutableSet; +import com.google.common.util.concurrent.UncheckedExecutionException; import java.io.IOException; import java.net.URI; import java.net.URISyntaxException; @@ -38,10 +43,9 @@ import java.nio.file.attribute.FileTime; import java.nio.file.attribute.UserPrincipalLookupService; import java.util.HashMap; -import java.util.HashSet; -import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ExecutionException; import javax.annotation.CheckReturnValue; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; @@ -66,8 +70,21 @@ public final class CloudStorageFileSystem extends FileSystem { private final CloudStorageFileSystemProvider provider; private final String bucket; private final CloudStorageConfiguration config; - private static final Map> - CONFIG_TO_PROVIDERS_MAP = new HashMap<>(); + private static final LoadingCache + PROVIDER_CACHE_BY_CONFIG = + CacheBuilder.newBuilder() + .build( + new CacheLoader() { + @Override + public CloudStorageFileSystemProvider load(ConfigTuple key) { + CloudStorageConfiguration config = key.cloudStorageConfiguration; + StorageOptions storageOptions = key.storageOptions; + String userProject = config.userProject(); + return (storageOptions == null) + ? new CloudStorageFileSystemProvider(userProject) + : new CloudStorageFileSystemProvider(userProject, storageOptions); + } + }); // Users can change this: then this affects every filesystem object created // later, including via SPI. This is meant to be done only once, at the beginning @@ -144,32 +161,7 @@ public static CloudStorageFileSystem forBucket(String bucket) { */ @CheckReturnValue public static CloudStorageFileSystem forBucket(String bucket, CloudStorageConfiguration config) { - checkArgument( - !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - checkNotNull(config); - return new CloudStorageFileSystem( - getCloudStorageFileSystemProvider(config, null), bucket, config); - } - - private static CloudStorageFileSystemProvider getCloudStorageFileSystemProvider( - CloudStorageConfiguration config, StorageOptions storageOptions) { - CloudStorageFileSystemProvider newProvider = - (storageOptions == null) - ? new CloudStorageFileSystemProvider(config.userProject()) - : new CloudStorageFileSystemProvider(config.userProject(), storageOptions); - Set existingProviders = CONFIG_TO_PROVIDERS_MAP.get(config); - if (existingProviders == null) { - existingProviders = new HashSet<>(); - } else { - for (CloudStorageFileSystemProvider existiningProvider : existingProviders) { - if (existiningProvider.equals(newProvider)) { - return existiningProvider; - } - } - } - existingProviders.add(newProvider); - CONFIG_TO_PROVIDERS_MAP.put(config, existingProviders); - return newProvider; + return forBucket(bucket, config, null); } /** @@ -192,8 +184,16 @@ public static CloudStorageFileSystem forBucket( String bucket, CloudStorageConfiguration config, @Nullable StorageOptions storageOptions) { checkArgument( !bucket.startsWith(URI_SCHEME + ":"), "Bucket name must not have schema: %s", bucket); - return new CloudStorageFileSystem( - getCloudStorageFileSystemProvider(config, storageOptions), bucket, checkNotNull(config)); + checkNotNull(config); + CloudStorageFileSystemProvider result; + ConfigTuple configTuple = new ConfigTuple(config, storageOptions); + try { + result = PROVIDER_CACHE_BY_CONFIG.get(configTuple); + } catch (ExecutionException | UncheckedExecutionException e) { + throw new IllegalStateException( + "Unable to resolve CloudStorageFileSystemProvider for the provided configuration", e); + } + return new CloudStorageFileSystem(result, bucket, checkNotNull(config)); } CloudStorageFileSystem( @@ -335,4 +335,45 @@ public String toString() { throw new AssertionError(e); } } + + private static final class ConfigTuple { + private final CloudStorageConfiguration cloudStorageConfiguration; + @Nullable private final StorageOptions storageOptions; + + public ConfigTuple( + CloudStorageConfiguration cloudStorageConfiguration, + @Nullable StorageOptions storageOptions) { + this.cloudStorageConfiguration = + requireNonNull(cloudStorageConfiguration, "cloudStorageConfiguration must be non null"); + this.storageOptions = storageOptions; + } + + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (!(o instanceof ConfigTuple)) { + return false; + } + ConfigTuple that = (ConfigTuple) o; + return cloudStorageConfiguration.equals(that.cloudStorageConfiguration) + && Objects.equals(storageOptions, that.storageOptions); + } + + @Override + public int hashCode() { + return Objects.hash(cloudStorageConfiguration, storageOptions); + } + + @Override + public String toString() { + return "ConfigTuple{" + + "cloudStorageConfiguration=" + + cloudStorageConfiguration + + ", storageOptions=" + + storageOptions + + '}'; + } + } } 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 4f2953b3..9a57a028 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 @@ -73,7 +73,6 @@ import java.util.Set; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; -import javax.inject.Singleton; /** * Google Cloud Storage {@link FileSystemProvider} implementation. @@ -83,7 +82,6 @@ * the javadocs in this class serve as useful documentation for the behavior of the Google Cloud * Storage NIO library. */ -@Singleton @ThreadSafe public final class CloudStorageFileSystemProvider extends FileSystemProvider { 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 2b901737..436daef5 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 @@ -27,13 +27,19 @@ import static java.nio.file.StandardOpenOption.TRUNCATE_EXISTING; import static java.nio.file.StandardOpenOption.WRITE; +import com.google.auth.Credentials; +import com.google.auth.oauth2.ServiceAccountCredentials; +import com.google.cloud.NoCredentials; +import com.google.cloud.storage.StorageOptions; import com.google.cloud.storage.contrib.nio.testing.LocalStorageHelper; import com.google.cloud.testing.junit4.MultipleAttemptsRule; import com.google.common.collect.ImmutableList; import com.google.common.testing.NullPointerTester; +import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; +import java.lang.reflect.Field; import java.net.URI; import java.nio.ByteBuffer; import java.nio.channels.ReadableByteChannel; @@ -795,6 +801,48 @@ public void testFromSpace() throws Exception { assertThat(path4.toString()).isEqualTo("/with/a%20percent"); } + // port of test from + // https://github.com/broadinstitute/cromwell/pull/6491/files#diff-758dbbe823e71cc26fee7bc89cd5c434dfb76e604d51005b8327db59aab96068R300-R336 + @Test + public void cromwell6491() throws Exception { + + CloudStorageConfiguration config = + CloudStorageConfiguration.builder() + .permitEmptyPathComponents(true) + .stripPrefixSlash(true) + .usePseudoDirectories(true) + .build(); + + Credentials noCredentials = NoCredentials.getInstance(); + FileInputStream fileInputStream = + new FileInputStream(System.getenv("GOOGLE_APPLICATION_CREDENTIALS")); + Credentials saCredentials = ServiceAccountCredentials.fromStream(fileInputStream); + fileInputStream.close(); + + StorageOptions noOptions = + StorageOptions.newBuilder() + .setProjectId("public-project") + .setCredentials(noCredentials) + .build(); + + StorageOptions saOptions = + StorageOptions.newBuilder() + .setProjectId("private-project") + .setCredentials(saCredentials) + .build(); + + CloudStorageFileSystem noFs = + CloudStorageFileSystem.forBucket("public-bucket", config, noOptions); + CloudStorageFileSystem saFs = + CloudStorageFileSystem.forBucket("private-bucket", config, saOptions); + + CloudStoragePath noPath = noFs.getPath("public-file"); + CloudStoragePath saPath = saFs.getPath("private-file"); + + assertThat(credentialsForPath(noPath)).isEqualTo(noCredentials); + assertThat(credentialsForPath(saPath)).isEqualTo(saCredentials); + } + private static CloudStorageConfiguration permitEmptyPathComponents(boolean value) { return CloudStorageConfiguration.builder().permitEmptyPathComponents(value).build(); } @@ -802,4 +850,19 @@ private static CloudStorageConfiguration permitEmptyPathComponents(boolean value private static CloudStorageConfiguration usePseudoDirectories(boolean value) { return CloudStorageConfiguration.builder().usePseudoDirectories(value).build(); } + + private static Credentials credentialsForPath(Path p) + throws NoSuchFieldException, IllegalAccessException { + CloudStorageFileSystemProvider cloudFilesystemProvider = + (CloudStorageFileSystemProvider) p.getFileSystem().provider(); + Field storageOptionsField = + cloudFilesystemProvider.getClass().getDeclaredField("storageOptions"); + storageOptionsField.setAccessible(true); + StorageOptions storageOptions = + (StorageOptions) storageOptionsField.get(cloudFilesystemProvider); + Field credentialsField = + storageOptions.getClass().getSuperclass().getDeclaredField("credentials"); + credentialsField.setAccessible(true); + return (Credentials) credentialsField.get(storageOptions); + } } diff --git a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java index ebd201d8..16647fb0 100644 --- a/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java +++ b/google-cloud-nio/src/test/java/com/google/cloud/storage/contrib/nio/CloudStorageIsDirectoryTest.java @@ -32,6 +32,7 @@ import org.junit.Before; import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TestName; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -40,12 +41,17 @@ public class CloudStorageIsDirectoryTest { @Rule public final MultipleAttemptsRule multipleAttemptsRule = new MultipleAttemptsRule(3); + @Rule public final TestName testName = new TestName(); + private StorageOptions mockOptions; private Storage mockStorage; @Before public void before() { - mockOptions = mock(StorageOptions.class); + mockOptions = + mock( + StorageOptions.class, + String.format("storage-options-mock_%s", testName.getMethodName())); mockStorage = mock(Storage.class); when(mockOptions.getService()).thenReturn(mockStorage); CloudStorageFileSystemProvider.setStorageOptions(mockOptions); @@ -54,7 +60,7 @@ public void before() { @Test public void testIsDirectoryNoUserProject() { CloudStorageFileSystem fs = - CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT); + CloudStorageFileSystem.forBucket("bucket", CloudStorageConfiguration.DEFAULT, mockOptions); when(mockStorage.get(BlobId.of("bucket", "test", null))) .thenThrow(new IllegalArgumentException()); Page pages = mock(Page.class); @@ -74,7 +80,9 @@ public void testIsDirectoryNoUserProject() { public void testIsDirectoryWithUserProject() { CloudStorageFileSystem fs = CloudStorageFileSystem.forBucket( - "bucket", CloudStorageConfiguration.builder().userProject("project-id").build()); + "bucket", + CloudStorageConfiguration.builder().userProject("project-id").build(), + mockOptions); when(mockStorage.get(BlobId.of("bucket", "test", null))) .thenThrow(new IllegalArgumentException()); Page pages = mock(Page.class);