fix: for issue #460

awslabs · May 17, 2024 · a64ca47 · a64ca47
1 parent 66bede8
commit a64ca47
Show file tree

Hide file tree

Showing 9 changed files with 76 additions and 89 deletions.
diff --git a/src/examples/java/software/amazon/nio/spi/examples/CreateBucket.java b/src/examples/java/software/amazon/nio/spi/examples/CreateBucket.java
@@ -23,11 +23,17 @@ public static void main(String[] args) throws IOException {
         }
 
         System.out.println("Creating bucket " + args[0]);
-        try (var fs = FileSystems.newFileSystem(URI.create(args[0]),
+        var uri = URI.create(args[0]);
+        try (var fs = FileSystems.newFileSystem(uri,
                 Map.of("locationConstraint", "us-west-2"))) {
             System.out.println(fs.toString());
         } catch (FileSystemAlreadyExistsException e) {
             System.err.printf("Bucket already exists: %s\n", e.getMessage());
         }
+
+        try (var fileSystem = FileSystems.getFileSystem(uri))
+        {
+            fileSystem.getRootDirectories().forEach(System.out::println);
+        }
     }
 }
diff --git a/src/main/java/software/amazon/nio/spi/s3/S3FileSystemProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3FileSystemProvider.java
@@ -99,6 +99,13 @@ public class S3FileSystemProvider extends FileSystemProvider {
 
     private final Logger logger = LoggerFactory.getLogger(this.getClass().getName());
 
+    /**
+     * Get an unmodifiable copy of the Filesystem Cache. Mainly used for testing purposes.
+     * @return An immutable copy of the filesystem cache.
+     */
+    protected Map<String, S3FileSystem> getFsCache() {
+        return Map.copyOf(FS_CACHE);
+    }
 
     /**
      * Returns the URI scheme that identifies this provider.
@@ -189,7 +196,7 @@ public FileSystem newFileSystem(final URI uri, final Map<String, ?> env) throws
             }
             throw new IOException(e.getMessage(), e);
         }
-        return getFileSystem(uri, true);
+        return getFileSystem(uri);
     }
 
     /**
@@ -227,7 +234,14 @@ public FileSystem newFileSystem(final URI uri, final Map<String, ?> env) throws
      */
     @Override
     public FileSystem getFileSystem(URI uri) {
-        return getFileSystem(uri, false);
+        var info = fileSystemInfo(uri);
+        return FS_CACHE.computeIfAbsent(info.key(), (key) -> {
+            var config = new S3NioSpiConfiguration().withEndpoint(info.endpoint()).withBucketName(info.bucket());
+            if (info.accessKey() != null) {
+                config.withCredentials(info.accessKey(), info.accessSecret());
+            }
+            return new S3FileSystem(this, config);
+        });
     }
 
     /**
@@ -259,7 +273,7 @@ public FileSystem getFileSystem(URI uri) {
     @Override
     public Path getPath(URI uri) throws IllegalArgumentException, FileSystemNotFoundException, SecurityException {
         Objects.requireNonNull(uri);
-        return getFileSystem(uri, true).getPath(uri.getScheme() + ":/" + uri.getPath());
+        return getFileSystem(uri).getPath(uri.getScheme() + ":/" + uri.getPath());
     }
 
     /**
@@ -770,33 +784,6 @@ public void setAttribute(Path path, String attribute, Object value, LinkOption..
         throw new UnsupportedOperationException("s3 file attributes cannot be modified by this class");
     }
 
-    /**
-     * Similar to getFileSystem(uri), but it allows to create the file system if
-     * not yet created.
-     *
-     * @param uri    URI reference
-     * @param create if true, the file system is created if not already done
-     * @return The file system
-     * @throws IllegalArgumentException    If the pre-conditions for the {@code uri} parameter aren't met
-     * @throws FileSystemNotFoundException If the file system does not exist
-     * @throws SecurityException           If a security manager is installed, and it denies an unspecified
-     *                                     permission.
-     */
-    S3FileSystem getFileSystem(URI uri, boolean create) {
-        var info = fileSystemInfo(uri);
-        return FS_CACHE.computeIfAbsent(info.key(), (key) -> {
-            if (!create) {
-                throw new FileSystemNotFoundException("file system not found for '" + info.key() + "'");
-            }
-
-            var config = new S3NioSpiConfiguration().withEndpoint(info.endpoint()).withBucketName(info.bucket());
-            if (info.accessKey() != null) {
-                config.withCredentials(info.accessKey(), info.accessSecret());
-            }
-            return new S3FileSystem(this, config);
-        });
-    }
-
     void closeFileSystem(FileSystem fs) {
         for (var key : FS_CACHE.keySet()) {
             if (fs == FS_CACHE.get(key)) {

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3BasicFileAttributeViewTest.java b/src/test/java/software/amazon/nio/spi/s3/S3BasicFileAttributeViewTest.java
@@ -5,7 +5,8 @@
 
 package software.amazon.nio.spi.s3;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
 
 import java.net.URI;
 import org.junit.jupiter.api.Test;
@@ -14,7 +15,7 @@ class S3BasicFileAttributeViewTest {
     final String uriString = "s3://mybucket";
     final S3FileSystemProvider provider = new S3FileSystemProvider();
 
-    S3FileSystem fileSystem = provider.getFileSystem(URI.create(uriString), true);
+    S3FileSystem fileSystem = (S3FileSystem) provider.getFileSystem(URI.create(uriString));
     S3Path path = S3Path.getPath(fileSystem, uriString);
     S3BasicFileAttributeView view = new S3BasicFileAttributeView(path);
 

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3FileSystemProviderTest.java b/src/test/java/software/amazon/nio/spi/s3/S3FileSystemProviderTest.java
@@ -90,7 +90,7 @@ public void init() {
         provider = new S3FileSystemProvider();
         lenient().when(mockClient.headObject(anyConsumer())).thenReturn(
                 CompletableFuture.supplyAsync(() -> HeadObjectResponse.builder().contentLength(100L).build()));
-        fs = provider.getFileSystem(URI.create(pathUri), true);
+        fs = (S3FileSystem) provider.getFileSystem(URI.create(pathUri));
         fs.clientProvider(new FixedS3ClientProvider(mockClient));
     }
 
@@ -131,7 +131,7 @@ public void getFileSystem() {
         //
         // New AWS S3 file system
         //
-        var cfs = provider.getFileSystem(URI.create("s3://foo2/baa"), true);
+        var cfs = provider.getFileSystem(URI.create("s3://foo2/baa"));
         var gfs = provider.getFileSystem(URI.create("s3://foo2"));
         assertNotSame(fs, gfs); assertSame(cfs, gfs);
         gfs = provider.getFileSystem(URI.create("s3://foo2"));
@@ -141,26 +141,18 @@ public void getFileSystem() {
         //
         // New AWS S3 file system with same bucket but different path
         //
-        cfs = provider.getFileSystem(URI.create("s3://foo3"), true);
+        cfs = provider.getFileSystem(URI.create("s3://foo3"));
         gfs = provider.getFileSystem(URI.create("s3://foo3/dir"));
         assertNotSame(fs, gfs); assertSame(cfs, gfs);
         gfs = provider.getFileSystem(URI.create("s3://foo3/dir"));
         assertNotSame(fs, gfs); assertSame(cfs, gfs);
         provider.closeFileSystem(cfs);
-
-        assertThrows(
-            FileSystemNotFoundException.class, () -> provider.getFileSystem(URI.create("s3://nobucket"))
-        );
     }
 
     @Test
     public void closingFileSystemDiscardsItFromCache() {
         provider.closeFileSystem(fs);
-
-        assertThrows(
-            FileSystemNotFoundException.class,
-            () -> provider.getFileSystem(URI.create(pathUri))
-        );
+        assertFalse(provider.getFsCache().containsKey(pathUri));
     }
 
     @Test

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3FileSystemTest.java b/src/test/java/software/amazon/nio/spi/s3/S3FileSystemTest.java
@@ -5,6 +5,21 @@
 
 package software.amazon.nio.spi.s3;
 
+import static org.assertj.core.api.BDDAssertions.then;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.lenient;
+import static software.amazon.nio.spi.s3.Constants.PATH_SEPARATOR;
+import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
+
+import java.io.IOException;
+import java.net.URI;
+import java.nio.file.FileSystems;
+import java.util.Collections;
+import java.util.concurrent.CompletableFuture;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
@@ -14,19 +29,6 @@
 import software.amazon.awssdk.services.s3.S3AsyncClient;
 import software.amazon.awssdk.services.s3.model.HeadObjectResponse;
 
-import java.io.IOException;
-import java.net.URI;
-import java.nio.file.FileSystemNotFoundException;
-import java.nio.file.FileSystems;
-import java.util.Collections;
-import java.util.concurrent.CompletableFuture;
-
-import static org.assertj.core.api.BDDAssertions.then;
-import static org.junit.jupiter.api.Assertions.*;
-import static org.mockito.Mockito.lenient;
-import static software.amazon.nio.spi.s3.Constants.PATH_SEPARATOR;
-import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
-
 @ExtendWith(MockitoExtension.class)
 public class S3FileSystemTest {
     S3FileSystemProvider provider;
@@ -39,7 +41,7 @@ public class S3FileSystemTest {
     @BeforeEach
     public void init() {
         provider = new S3FileSystemProvider();
-        s3FileSystem = provider.getFileSystem(s3Uri, true);
+        s3FileSystem = (S3FileSystem) provider.getFileSystem(s3Uri);
         s3FileSystem.clientProvider = new FixedS3ClientProvider(mockClient);
         lenient().when(mockClient.headObject(anyConsumer())).thenReturn(
                 CompletableFuture.supplyAsync(() -> HeadObjectResponse.builder().contentLength(100L).build()));
@@ -62,7 +64,7 @@ public void close() throws IOException {
         assertFalse(s3FileSystem.isOpen(), "File system should return false from isOpen when closed has been called");
 
         // close() should also remove the instance from the provider
-        assertThrows(FileSystemNotFoundException.class, () -> provider.getFileSystem(s3Uri));
+        assertFalse(provider.getFsCache().containsKey(s3Uri.toString()));
     }
 
     @Test

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3PathTest.java b/src/test/java/software/amazon/nio/spi/s3/S3PathTest.java
@@ -8,7 +8,12 @@
 
 import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.assertj.core.api.BDDAssertions.then;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.lenient;
 import static software.amazon.nio.spi.s3.Constants.PATH_SEPARATOR;
 import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
@@ -46,7 +51,7 @@ public class S3PathTest {
 
     @BeforeEach
     public void init(){
-        fileSystem = provider.getFileSystem(URI.create(uriString), true);
+        fileSystem = (S3FileSystem) provider.getFileSystem(URI.create(uriString));
         fileSystem.clientProvider(new FixedS3ClientProvider(mockClient));
         lenient().when(mockClient.headObject(anyConsumer())).thenReturn(
                 CompletableFuture.supplyAsync(() -> HeadObjectResponse.builder().contentLength(100L).build()));
@@ -206,7 +211,7 @@ public void startsWith() {
 
         assertFalse(relativeObject.startsWith(S3Path.getPath(fileSystem, "dir1/dir2")));
         assertFalse(absoluteObject.startsWith(relativeBeginning));
-        assertFalse(absoluteObject.startsWith(S3Path.getPath(provider.getFileSystem(URI.create("s3://different-bucket"), true), "/dir1/")));
+        assertFalse(absoluteObject.startsWith(S3Path.getPath((S3FileSystem) provider.getFileSystem(URI.create("s3://different-bucket")), "/dir1/")));
     }
 
     @Test
@@ -477,7 +482,7 @@ public void testEquals() {
 
         S3FileSystem fooFS = null;
         try{
-            fooFS = provider.getFileSystem(URI.create("s3://foo"), true);
+            fooFS = (S3FileSystem) provider.getFileSystem(URI.create("s3://foo"));
             assertNotEquals(S3Path.getPath(fileSystem, "dir1/"), S3Path.getPath(fooFS, "/dir1/"));
         } finally {
             if ( fooFS != null ) provider.closeFileSystem(fooFS);

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannelTest.java b/src/test/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannelTest.java
@@ -5,28 +5,29 @@
 
 package software.amazon.nio.spi.s3;
 
-import org.mockito.Mock;
-import software.amazon.awssdk.core.ResponseBytes;
-import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer;
-import software.amazon.awssdk.services.s3.S3AsyncClient;
-import software.amazon.awssdk.services.s3.model.GetObjectResponse;
+import static org.junit.jupiter.api.Assertions.assertArrayEquals;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.when;
+import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
 
 import java.io.IOException;
 import java.net.URI;
 import java.nio.ByteBuffer;
 import java.nio.charset.StandardCharsets;
 import java.util.concurrent.CompletableFuture;
-import static org.junit.jupiter.api.Assertions.*;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
-
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.lenient;
-import static org.mockito.Mockito.when;
-import static software.amazon.nio.spi.s3.S3Matchers.anyConsumer;
-
+import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
+import software.amazon.awssdk.core.ResponseBytes;
+import software.amazon.awssdk.core.internal.async.ByteArrayAsyncResponseTransformer;
+import software.amazon.awssdk.services.s3.S3AsyncClient;
+import software.amazon.awssdk.services.s3.model.GetObjectResponse;
 
 @ExtendWith(MockitoExtension.class)
 @SuppressWarnings("unchecked")
@@ -47,7 +48,7 @@ public class S3ReadAheadByteChannelTest {
 
     @BeforeEach
     public void setup() throws IOException {
-        path = S3Path.getPath(provider.getFileSystem(URI.create("s3://my-bucket"), true), "/object");
+        path = S3Path.getPath((S3FileSystem) provider.getFileSystem(URI.create("s3://my-bucket")), "/object");
 
         // mocking
         lenient().when(delegator.size()).thenReturn(52L);

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3SeekableByteChannelTest.java b/src/test/java/software/amazon/nio/spi/s3/S3SeekableByteChannelTest.java
@@ -73,7 +73,7 @@ public void init() {
                         bytes)));
 
         var provider = new S3FileSystemProvider();
-        fs = provider.getFileSystem(URI.create("s3://test-bucket"), true);
+        fs = (S3FileSystem) provider.getFileSystem(URI.create("s3://test-bucket"));
         fs.clientProvider(new FixedS3ClientProvider(mockClient));
         path = (S3Path) fs.getPath("/object");
     }

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3XFileSystemProviderTest.java b/src/test/java/software/amazon/nio/spi/s3/S3XFileSystemProviderTest.java
@@ -6,15 +6,13 @@
 package software.amazon.nio.spi.s3;
 
 import static com.github.stefanbirkner.systemlambda.SystemLambda.restoreSystemProperties;
-import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.assertj.core.api.BDDAssertions.then;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 
 import java.net.URI;
 import java.nio.file.FileSystem;
-import java.nio.file.FileSystemNotFoundException;
 import java.nio.file.Paths;
 import java.util.Collections;
 import org.junit.jupiter.api.DisplayName;
@@ -54,20 +52,15 @@ public void newFileSystemPath() {
     public void getFileSystem() {
         var provider = new S3XFileSystemProvider();
 
-        FileSystem fs2 = provider.getFileSystem(URI1, true);
+        FileSystem fs2 = provider.getFileSystem(URI1);
         then(provider.getFileSystem(URI1)).isSameAs(fs2);
-        FileSystem fs3 = provider.getFileSystem(URI3, true);
+        FileSystem fs3 = provider.getFileSystem(URI3);
         then(fs3).isNotSameAs(fs2);
         then(provider.getFileSystem(URI2)).isSameAs(fs2);
-        then(provider.getFileSystem(URI7, true)).isNotSameAs(fs3);
+        then(provider.getFileSystem(URI7)).isNotSameAs(fs3);
         then(provider.getFileSystem(URI8)).isNotSameAs(fs3);
         provider.closeFileSystem(fs2);
         provider.closeFileSystem(fs3);
-
-        assertThatCode(() -> provider.getFileSystem(URI.create("s3://nowhere.com:2000/foo2/baa2")))
-                .as("missing error")
-                .isInstanceOf(FileSystemNotFoundException.class)
-                .hasMessageContaining("file system not found for 'nowhere.com:2000/foo2'");
     }
 
     @Test
@@ -77,7 +70,7 @@ public void setCredentialsThroughURI() throws Exception {
         restoreSystemProperties(() -> {
             System.setProperty("aws.region", "us-west-1");
 
-            var fs = p.getFileSystem(URI.create("s3x://urikey:urisecret@some.where.com:1010/bucket"), true);
+            var fs = (S3FileSystem) p.getFileSystem(URI.create("s3x://urikey:urisecret@some.where.com:1010/bucket"));
             fs.clientProvider().asyncClientBuilder(BUILDER);
             fs.client();
             fs.close();