fix for issue 399 (awslabs#400)

* fix for issue 399 * fix for issue 399 * Fix integration tests where clients were being re-used after they were closed.
stefanofornari · Mar 16, 2024 · bd506dd · bd506dd
1 parent 3cd7d42
commit bd506dd
Show file tree

Hide file tree

Showing 6 changed files with 41 additions and 40 deletions.
diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/FilesExistsTest.java b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesExistsTest.java
@@ -5,18 +5,17 @@
 
 package software.amazon.nio.spi.s3;
 
-import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Nested;
-import org.junit.jupiter.api.Test;
+import static org.assertj.core.api.BDDAssertions.then;
+import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
+import static software.amazon.nio.spi.s3.Containers.putObject;
 
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Paths;
-
-import static org.assertj.core.api.BDDAssertions.then;
-import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
-import static software.amazon.nio.spi.s3.Containers.putObject;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Test;
 
 @DisplayName("Files$exists()")
 public class FilesExistsTest {
@@ -44,23 +43,25 @@ public void fileExistsShouldReturnFalseWhenBucketExistsAndFileNotFound() {
     @Nested
     @DisplayName("should be true")
     class FileExists {
+        String bucketName;
 
         @BeforeEach
         public void createBucket() {
-            Containers.createBucket("sink");
+            bucketName = "sink"+ System.currentTimeMillis();
+            Containers.createBucket(bucketName);
         }
 
         @Test
         @DisplayName("when bucket and file exist")
         public void fileExistsShouldReturnTrueWhenBucketExistsAndFileFound() {
-            final var path = putObject("sink", "sample-file.txt");
+            final var path = putObject(bucketName, "sample-file.txt");
             then(Files.exists(path)).isTrue();
         }
 
         @Test
         @DisplayName("for bucket path when it exists")
         public void fileExistsShouldReturnTrueWhenBucketExists() {
-            final var path = Paths.get(URI.create(localStackConnectionEndpoint() + "/sink/"));
+            final var path = Paths.get(URI.create(localStackConnectionEndpoint() + "/"+bucketName+"/"));
             then(Files.exists(path)).isTrue();
         }
     }

diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java
@@ -5,27 +5,27 @@
 
 package software.amazon.nio.spi.s3;
 
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInstance;
+import static org.assertj.core.api.BDDAssertions.then;
+import static software.amazon.nio.spi.s3.Containers.putObject;
 
 import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
-
-import static org.assertj.core.api.BDDAssertions.then;
-import static software.amazon.nio.spi.s3.Containers.putObject;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
 
 @DisplayName("Files$read* should load file contents from localstack")
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 public class FilesReadTest {
     private Path path;
 
-    @BeforeAll
+    @BeforeEach
     public void createBucketAndFile(){
-        Containers.createBucket("sink");
-        path = putObject("sink", "files-read.txt", "some content");
+        String containerName = "sink"+ System.currentTimeMillis();
+        Containers.createBucket(containerName);
+        path = putObject(containerName, "files-read.txt", "some content");
     }
 
     @Test

diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/FilesWriteTest.java b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesWriteTest.java
@@ -5,34 +5,36 @@
 
 package software.amazon.nio.spi.s3;
 
-import org.junit.jupiter.api.BeforeAll;
-import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.api.TestInstance;
+import static org.assertj.core.api.Assertions.assertThat;
+import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
+import static software.amazon.nio.spi.s3.Containers.putObject;
 
 import java.io.IOException;
 import java.net.URI;
 import java.nio.file.Files;
 import java.nio.file.Paths;
 import java.nio.file.StandardOpenOption;
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static software.amazon.nio.spi.s3.Containers.localStackConnectionEndpoint;
-import static software.amazon.nio.spi.s3.Containers.putObject;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestInstance;
 
 @DisplayName("Files$write* should write file contents on s3 service")
 @TestInstance(TestInstance.Lifecycle.PER_CLASS)
 public class FilesWriteTest {
 
-    @BeforeAll
+    String bucketName;
+
+    @BeforeEach
     public void createBucket() {
-        Containers.createBucket("write-bucket");
+        bucketName = "write-bucket"+ System.currentTimeMillis();
+        Containers.createBucket(bucketName);
     }
 
     @Test
     @DisplayName("append to an existing file")
     public void writeToExistingFile() throws IOException {
-        final var path = putObject("write-bucket", "existing-file.txt", "some content");
+        final var path = putObject(bucketName, "existing-file.txt", "some content");
 
         Files.writeString(path, " more content", StandardOpenOption.APPEND);
 
@@ -42,7 +44,7 @@ public void writeToExistingFile() throws IOException {
     @Test
     @DisplayName("to a new file")
     public void writeToNewFile() throws IOException {
-        final var path = Paths.get(URI.create(localStackConnectionEndpoint() + "/write-bucket/new-file.txt"));
+        final var path = Paths.get(URI.create(localStackConnectionEndpoint() + "/"+bucketName+"/new-file.txt"));
 
         Files.writeString(path, "content of new file", StandardOpenOption.CREATE_NEW);
 

diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java
@@ -281,7 +281,7 @@ private S3AsyncClient configureClientForRegion(
 
     private S3AsyncClient configureCrtClientForRegion(String regionName) {
         var region = getRegionFromRegionName(regionName);
-        logger.debug("bucket region is: '{}'", region.id());
+        logger.debug("bucket region is: '{}'", region);
 
         var endpointUri = configuration.endpointUri();
         if (endpointUri != null) {
@@ -299,7 +299,7 @@ private S3AsyncClient configureCrtClientForRegion(String regionName) {
     }
 
     private static Region getRegionFromRegionName(String regionName) {
-        return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName);
+        return (regionName == null || regionName.isBlank()) ? null : Region.of(regionName);
     }
 
 }
diff --git a/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java b/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java
@@ -62,9 +62,8 @@ private S3SeekableByteChannel(S3Path s3Path, S3AsyncClient s3Client, long startA
             position = 0L;
         } else if (options.contains(StandardOpenOption.READ) || options.isEmpty()) {
             LOGGER.debug("using S3ReadAheadByteChannel as read delegate for path '{}'", s3Path.toUri());
-            var readClient = s3Path.getFileSystem().readClient();
             readDelegate =
-                new S3ReadAheadByteChannel(s3Path, config.getMaxFragmentSize(), config.getMaxFragmentNumber(), readClient, this,
+                new S3ReadAheadByteChannel(s3Path, config.getMaxFragmentSize(), config.getMaxFragmentNumber(), s3Client, this,
                     timeout, timeUnit);
             writeDelegate = null;
         } else {

diff --git a/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java b/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java
@@ -22,7 +22,6 @@
 import org.mockito.junit.jupiter.MockitoExtension;
 import software.amazon.awssdk.awscore.exception.AwsErrorDetails;
 import software.amazon.awssdk.http.SdkHttpResponse;
-import software.amazon.awssdk.regions.Region;
 import software.amazon.awssdk.services.s3.S3AsyncClient;
 import software.amazon.awssdk.services.s3.model.GetBucketLocationResponse;
 import software.amazon.awssdk.services.s3.model.HeadBucketResponse;
@@ -165,7 +164,7 @@ public void generateAsyncClientByEndpointBucketCredentials() {
 
         // THEN
         verify(BUILDER, times(1)).endpointOverride(URI.create("https://endpoint1:1010"));
-        verify(BUILDER, times(1)).region(Region.US_EAST_1);
+        verify(BUILDER, times(1)).region(null);
 
         // GIVEN
         BUILDER = spy(S3AsyncClient.crtBuilder());
@@ -177,6 +176,6 @@ public void generateAsyncClientByEndpointBucketCredentials() {
 
         // THEN
         verify(BUILDER, times(1)).endpointOverride(URI.create("https://endpoint2:2020"));
-        verify(BUILDER, times(1)).region(Region.US_EAST_1);
+        verify(BUILDER, times(1)).region(null);
     }
 }