From 432d6408c4b4a3283e29d2a81a3e8c4244b85f85 Mon Sep 17 00:00:00 2001 From: guicamest Date: Thu, 2 Nov 2023 18:18:09 +0100 Subject: [PATCH 1/7] test(Files): Add integration tests for Files.copy / read* from issue #236 [skip ci] --- .../amazon/nio/spi/s3/Containers.java | 11 +++++ .../amazon/nio/spi/s3/FilesCopyTest.java | 32 +++++++++++++++ .../amazon/nio/spi/s3/FilesReadTest.java | 40 +++++++++++++++++++ 3 files changed, 83 insertions(+) create mode 100644 src/integrationTest/java/software/amazon/nio/spi/s3/FilesCopyTest.java create mode 100644 src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/Containers.java b/src/integrationTest/java/software/amazon/nio/spi/s3/Containers.java index 2a0987c7..43ab03fd 100644 --- a/src/integrationTest/java/software/amazon/nio/spi/s3/Containers.java +++ b/src/integrationTest/java/software/amazon/nio/spi/s3/Containers.java @@ -37,6 +37,17 @@ public static void putObject(String bucket, String key) { .doesNotThrowAnyException(); } + public static void putObject(String bucket, String key, String content) { + assertThatCode(() -> { + Container.ExecResult execResultCreateFile = LOCAL_STACK_CONTAINER.execInContainer("sh", "-c", "echo -n '" + content + "' > " + key); + Container.ExecResult execResultPut = LOCAL_STACK_CONTAINER.execInContainer(("awslocal s3api put-object --bucket " + bucket + " --key " + key + " --body " + key).split(" ")); + + assertThat(execResultCreateFile.getExitCode()).isZero(); + assertThat(execResultPut.getExitCode()).withFailMessage("Failed put: %s ", execResultPut.getStderr()).isZero(); + }).as("Failed to put object '%s' in bucket '%s'", key, bucket) + .doesNotThrowAnyException(); + } + public static String localStackConnectionEndpoint() { return localStackConnectionEndpoint(null, null); } diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/FilesCopyTest.java b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesCopyTest.java new file mode 100644 index 00000000..478bf4d1 --- /dev/null +++ b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesCopyTest.java @@ -0,0 +1,32 @@ +package software.amazon.nio.spi.s3; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.Paths; + +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; + +@DisplayName("Files$copy should load file contents from localstack") +public class FilesCopyTest +{ + @TempDir + Path tempDir; + + @Test + @DisplayName("when doing copy of existing file") + public void fileCopyShouldCopyFileWhenFileFound() throws IOException { + Containers.createBucket("sink"); + putObject("sink", "files-copy.txt", "some content"); + final Path path = Paths.get(URI.create(localStackConnectionEndpoint() + "/sink/files-copy.txt")); + Path copiedFile = Files.copy(path, tempDir.resolve("sample-file-local.txt")); + assertThat(copiedFile).hasContent("some content"); + } +} diff --git a/src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java new file mode 100644 index 00000000..f58e53a0 --- /dev/null +++ b/src/integrationTest/java/software/amazon/nio/spi/s3/FilesReadTest.java @@ -0,0 +1,40 @@ +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 java.io.IOException; +import java.net.URI; +import java.nio.file.Files; +import java.nio.file.Path; +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; + +@DisplayName("Files$read* should load file contents from localstack") +public class FilesReadTest +{ + private final Path path = Paths.get(URI.create(localStackConnectionEndpoint() + "/sink/files-read.txt")); + + @BeforeAll + public static void createBucketAndFile(){ + Containers.createBucket("sink"); + putObject("sink", "files-read.txt", "some content"); + } + + @Test + @DisplayName("when doing readAllBytes from existing file in s3") + public void fileReadAllBytesShouldReturnFileContentsWhenFileFound() throws IOException { + then(Files.readAllBytes(path)).isEqualTo("some content".getBytes()); + } + + @Test + @DisplayName("when doing readAllLines from existing file in s3") + public void fileReadAllLinesShouldReturnFileContentWhenFileFound() throws IOException { + then(String.join("", Files.readAllLines(path))).isEqualTo("some content"); + } + +} From 7e95317b6dc098c4837a5f3144a04b4a0f57398d Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 16:44:56 +0100 Subject: [PATCH 2/7] chore(S3ClientProvider): reformat --- .../amazon/nio/spi/s3/S3ClientProvider.java | 20 ++++++++----------- 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index ff48d097..b3969fc3 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -130,11 +130,11 @@ S3Client universalClient() { * that can be used by certain S3 operations for discovery * * @param async true to return an asynchronous client, false otherwise - * @param type of AwsClient + * @param type of AwsClient * @return a S3Client not bound to a region */ T universalClient(boolean async) { - return (T)((async) ? DEFAULT_ASYNC_CLIENT : DEFAULT_CLIENT); + return (T) ((async) ? DEFAULT_ASYNC_CLIENT : DEFAULT_CLIENT); } /** @@ -142,22 +142,19 @@ T universalClient(boolean async) { * discovery client. * * @param bucket the named of the bucket to make the client for - * * @return an S3 client appropriate for the region of the named bucket - * */ protected S3AsyncClient generateAsyncClient(String bucket) { - return generateAsyncClient(bucket, universalClient()); + return generateAsyncClient(bucket, universalClient()); } /** * Generate a client for the named bucket using a provided client to * determine the location of the named client * - * @param bucketName the name of the bucket to make the client for + * @param bucketName the name of the bucket to make the client for * @param locationClient the client used to determine the location of the - * named bucket, recommend using DEFAULT_CLIENT - * + * named bucket, recommend using DEFAULT_CLIENT * @return an S3 client appropriate for the region of the named bucket */ S3Client generateSyncClient(String bucketName, S3Client locationClient) { @@ -168,13 +165,12 @@ S3Client generateSyncClient(String bucketName, S3Client locationClient) { * Generate an async client for the named bucket using a provided client to * determine the location of the named client * - * @param bucketName the name of the bucket to make the client for + * @param bucketName the name of the bucket to make the client for * @param locationClient the client used to determine the location of the - * named bucket, recommend using DEFAULT_CLIENT - * + * named bucket, recommend using DEFAULT_CLIENT * @return an S3 client appropriate for the region of the named bucket */ - S3AsyncClient generateAsyncClient (String bucketName, S3Client locationClient) { + S3AsyncClient generateAsyncClient(String bucketName, S3Client locationClient) { return getClientForBucket(bucketName, locationClient, this::asyncClientForRegion); } From d6ca42f9d98c550241a8547ae231a4b7c8ac482e Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 17:02:53 +0100 Subject: [PATCH 3/7] rewrite: Extract `getRegionFromRegionName` and `endpointURI` methods --- .../amazon/nio/spi/s3/S3ClientProvider.java | 30 ++++++++++--------- .../spi/s3/config/S3NioSpiConfiguration.java | 9 ++++++ 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index b3969fc3..ec741cfa 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -244,12 +244,10 @@ private String getBucketRegionFromResponse(SdkHttpResponse response) { } private S3Client clientForRegion(String regionName) { - String endpoint = configuration.getEndpoint(); - AwsCredentials credentials = configuration.getCredentials(); - Region region = ((regionName == null) || (regionName.trim().isEmpty())) ? Region.US_EAST_1 : Region.of(regionName); - + Region region = getRegionFromRegionName(regionName); logger.debug("bucket region is: '{}'", region.id()); + S3ClientBuilder clientBuilder = S3Client.builder() .forcePathStyle(configuration.getForcePathStyle()) .region(region) @@ -258,11 +256,13 @@ private S3Client clientForRegion(String regionName) { builder -> builder.retryCondition(retryCondition).backoffStrategy(backoffStrategy) ) ); - - if (!endpoint.isBlank()) { - clientBuilder.endpointOverride(URI.create(configuration.getEndpointProtocol() + "://" + endpoint)); + + URI endpointUri = configuration.endpointURI(); + if (endpointUri != null) { + clientBuilder.endpointOverride(endpointUri); } + AwsCredentials credentials = configuration.getCredentials(); if (credentials != null) { clientBuilder.credentialsProvider(() -> credentials); } @@ -271,21 +271,23 @@ private S3Client clientForRegion(String regionName) { } private S3AsyncClient asyncClientForRegion(String regionName) { - String endpoint = configuration.getEndpoint(); - AwsCredentials credentials = configuration.getCredentials(); - - Region region = ((regionName == null) || (regionName.trim().isEmpty())) ? Region.US_EAST_1 : Region.of(regionName); - + Region region = getRegionFromRegionName(regionName); logger.debug("bucket region is: '{}'", region.id()); - if (!endpoint.isBlank()) { - asyncClientBuilder.endpointOverride(URI.create(configuration.getEndpointProtocol() + "://" + endpoint)); + URI endpointUri = configuration.endpointURI(); + if (endpointUri != null) { + asyncClientBuilder.endpointOverride(endpointUri); } + AwsCredentials credentials = configuration.getCredentials(); if (credentials != null) { asyncClientBuilder.credentialsProvider(() -> credentials); } return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build(); } + + private static Region getRegionFromRegionName(String regionName) { + return ((regionName == null) || (regionName.trim().isEmpty())) ? Region.US_EAST_1 : Region.of(regionName); + } } diff --git a/src/main/java/software/amazon/nio/spi/s3/config/S3NioSpiConfiguration.java b/src/main/java/software/amazon/nio/spi/s3/config/S3NioSpiConfiguration.java index e0ba67e4..913ab483 100644 --- a/src/main/java/software/amazon/nio/spi/s3/config/S3NioSpiConfiguration.java +++ b/src/main/java/software/amazon/nio/spi/s3/config/S3NioSpiConfiguration.java @@ -6,6 +6,7 @@ package software.amazon.nio.spi.s3.config; +import java.net.URI; import java.util.HashMap; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -416,4 +417,12 @@ private int parseIntProperty(String propName, int defaultVal){ return defaultVal; } } + + public URI endpointURI() { + String endpoint = getEndpoint(); + if (endpoint.isBlank()) { + return null; + } + return URI.create(String.format("%s://%s", getEndpointProtocol(), getEndpoint())); + } } From 63c07d801d05fbc6245a93260140a68fe2f61aac Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 17:07:03 +0100 Subject: [PATCH 4/7] chore: Use `isBlank` instead of `trim().isEmpty()` --- src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index ec741cfa..6367679d 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -288,6 +288,6 @@ private S3AsyncClient asyncClientForRegion(String regionName) { } private static Region getRegionFromRegionName(String regionName) { - return ((regionName == null) || (regionName.trim().isEmpty())) ? Region.US_EAST_1 : Region.of(regionName); + return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName); } } From 9bd8b4f5e2fcda920b544bc9c252fc86748f1c75 Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 17:24:30 +0100 Subject: [PATCH 5/7] rewrite(S3ClientProvider): Extract method to configure and build client using S3BaseClientBuilder --- .../amazon/nio/spi/s3/S3ClientProvider.java | 50 +++++++++++-------- 1 file changed, 28 insertions(+), 22 deletions(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index 6367679d..97ca4254 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -30,8 +30,8 @@ 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.S3BaseClientBuilder; import software.amazon.awssdk.services.s3.S3Client; -import software.amazon.awssdk.services.s3.S3ClientBuilder; import software.amazon.awssdk.services.s3.S3CrtAsyncClientBuilder; import software.amazon.awssdk.services.s3.model.HeadBucketResponse; import software.amazon.awssdk.services.s3.model.S3Exception; @@ -244,50 +244,56 @@ private String getBucketRegionFromResponse(SdkHttpResponse response) { } private S3Client clientForRegion(String regionName) { + return configureClientForRegion(regionName, S3Client.builder()); + } + + private S3AsyncClient asyncClientForRegion(String regionName) { Region region = getRegionFromRegionName(regionName); logger.debug("bucket region is: '{}'", region.id()); - - S3ClientBuilder clientBuilder = S3Client.builder() - .forcePathStyle(configuration.getForcePathStyle()) - .region(region) - .overrideConfiguration( - conf -> conf.retryPolicy( - builder -> builder.retryCondition(retryCondition).backoffStrategy(backoffStrategy) - ) - ); - URI endpointUri = configuration.endpointURI(); if (endpointUri != null) { - clientBuilder.endpointOverride(endpointUri); + asyncClientBuilder.endpointOverride(endpointUri); } AwsCredentials credentials = configuration.getCredentials(); if (credentials != null) { - clientBuilder.credentialsProvider(() -> credentials); + asyncClientBuilder.credentialsProvider(() -> credentials); } - return clientBuilder.build(); + return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build(); } - private S3AsyncClient asyncClientForRegion(String regionName) { + private static Region getRegionFromRegionName(String regionName) { + return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName); + } + + private > ActualClient configureClientForRegion( + String regionName, + S3BaseClientBuilder builder) + { Region region = getRegionFromRegionName(regionName); logger.debug("bucket region is: '{}'", region.id()); + builder + .forcePathStyle(configuration.getForcePathStyle()) + .region(region) + .overrideConfiguration( + conf -> conf.retryPolicy( + configBuilder -> configBuilder.retryCondition(retryCondition).backoffStrategy(backoffStrategy) + ) + ); + URI endpointUri = configuration.endpointURI(); if (endpointUri != null) { - asyncClientBuilder.endpointOverride(endpointUri); + builder.endpointOverride(endpointUri); } AwsCredentials credentials = configuration.getCredentials(); if (credentials != null) { - asyncClientBuilder.credentialsProvider(() -> credentials); + builder.credentialsProvider(() -> credentials); } - return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build(); - } - - private static Region getRegionFromRegionName(String regionName) { - return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName); + return builder.build(); } } From 7677368a879bc602d93b586928135d406d900ca8 Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 20:25:52 +0100 Subject: [PATCH 6/7] fix(236): Use non-crt async client for `S3ReadAheadByteChannel` --- .../amazon/nio/spi/s3/S3ClientProvider.java | 15 ++++++++++----- .../software/amazon/nio/spi/s3/S3FileSystem.java | 6 +++++- .../amazon/nio/spi/s3/S3ReadAheadByteChannel.java | 1 + .../amazon/nio/spi/s3/S3SeekableByteChannel.java | 3 ++- .../amazon/nio/spi/s3/FixedS3ClientProvider.java | 2 +- .../amazon/nio/spi/s3/S3ClientProviderTest.java | 12 ++++++------ 6 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index 97ca4254..9f495844 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -142,10 +142,11 @@ T universalClient(boolean async) { * discovery client. * * @param bucket the named of the bucket to make the client for + * @param crt whether to return a CRT async client or not * @return an S3 client appropriate for the region of the named bucket */ - protected S3AsyncClient generateAsyncClient(String bucket) { - return generateAsyncClient(bucket, universalClient()); + protected S3AsyncClient generateAsyncClient(String bucket, boolean crt) { + return generateAsyncClient(bucket, universalClient(), crt); } /** @@ -168,10 +169,11 @@ S3Client generateSyncClient(String bucketName, S3Client locationClient) { * @param bucketName the name of the bucket to make the client for * @param locationClient the client used to determine the location of the * named bucket, recommend using DEFAULT_CLIENT + * @param crt whether to return a CRT async client or not * @return an S3 client appropriate for the region of the named bucket */ - S3AsyncClient generateAsyncClient(String bucketName, S3Client locationClient) { - return getClientForBucket(bucketName, locationClient, this::asyncClientForRegion); + S3AsyncClient generateAsyncClient(String bucketName, S3Client locationClient, boolean crt) { + return getClientForBucket(bucketName, locationClient, (region) -> asyncClientForRegion(region, crt)); } private T getClientForBucket( @@ -247,7 +249,10 @@ private S3Client clientForRegion(String regionName) { return configureClientForRegion(regionName, S3Client.builder()); } - private S3AsyncClient asyncClientForRegion(String regionName) { + private S3AsyncClient asyncClientForRegion(String regionName, boolean crt) { + if (!crt) { + return configureClientForRegion(regionName, S3AsyncClient.builder()); + } Region region = getRegionFromRegionName(regionName); logger.debug("bucket region is: '{}'", region.id()); diff --git a/src/main/java/software/amazon/nio/spi/s3/S3FileSystem.java b/src/main/java/software/amazon/nio/spi/s3/S3FileSystem.java index 17b9dc45..8741d32f 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3FileSystem.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3FileSystem.java @@ -404,12 +404,16 @@ public void clientProvider(S3ClientProvider clientProvider) { */ S3AsyncClient client() { if (client == null) { - client = clientProvider.generateAsyncClient(bucketName); + client = clientProvider.generateAsyncClient(bucketName, true); } return client; } + S3AsyncClient readClient() { + return clientProvider.generateAsyncClient(bucketName, false); + } + /** * Obtain the name of the bucket represented by this FileSystem instance * @return the bucket name diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannel.java b/src/main/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannel.java index d133d087..17d541ef 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannel.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ReadAheadByteChannel.java @@ -169,6 +169,7 @@ public void close() { open = false; readAheadBuffersCache.invalidateAll(); readAheadBuffersCache.cleanUp(); + client.close(); } private void clearPriorFragments(int currentFragIndx) { diff --git a/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java b/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java index 8590bc4a..d0bfc003 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3SeekableByteChannel.java @@ -62,7 +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()); - readDelegate = new S3ReadAheadByteChannel(s3Path, config.getMaxFragmentSize(), config.getMaxFragmentNumber(), s3Client, this, timeout, timeUnit); + S3AsyncClient readClient = s3Path.getFileSystem().readClient(); + readDelegate = new S3ReadAheadByteChannel(s3Path, config.getMaxFragmentSize(), config.getMaxFragmentNumber(), readClient, this, timeout, timeUnit); writeDelegate = null; } else { throw new IOException("Invalid channel mode"); diff --git a/src/test/java/software/amazon/nio/spi/s3/FixedS3ClientProvider.java b/src/test/java/software/amazon/nio/spi/s3/FixedS3ClientProvider.java index ffc5a221..9a05b68b 100644 --- a/src/test/java/software/amazon/nio/spi/s3/FixedS3ClientProvider.java +++ b/src/test/java/software/amazon/nio/spi/s3/FixedS3ClientProvider.java @@ -29,7 +29,7 @@ S3Client universalClient() { } @Override - protected S3AsyncClient generateAsyncClient(String bucketName) { + protected S3AsyncClient generateAsyncClient(String bucketName, boolean crt) { return (S3AsyncClient)client; } diff --git a/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java b/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java index 3802da22..77198ff6 100644 --- a/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java +++ b/src/test/java/software/amazon/nio/spi/s3/S3ClientProviderTest.java @@ -64,7 +64,7 @@ public void initialization() { public void testGenerateAsyncClientWithNoErrors() { when(mockClient.getBucketLocation(anyConsumer())) .thenReturn(GetBucketLocationResponse.builder().locationConstraint("us-west-2").build()); - final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient); + final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient, true); assertNotNull(s3Client); } @@ -107,7 +107,7 @@ public void testGenerateAsyncClientWith403Response() { .build()); // which should get you a client - final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient); + final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient, true); assertNotNull(s3Client); final InOrder inOrder = inOrder(mockClient); @@ -135,7 +135,7 @@ public void testGenerateAsyncClientWith403Then301Responses(){ ); // then you should be able to get a client as long as the error response header contains the region - final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient); + final S3AsyncClient s3Client = provider.generateAsyncClient("test-bucket", mockClient, true); assertNotNull(s3Client); final InOrder inOrder = inOrder(mockClient); @@ -189,7 +189,7 @@ public void testGenerateAsyncClientWith403Then301ResponsesNoHeader(){ ); // then you should get a NoSuchElement exception when you try to get the header - assertThrows(NoSuchElementException.class, () -> provider.generateAsyncClient("test-bucket", mockClient)); + assertThrows(NoSuchElementException.class, () -> provider.generateAsyncClient("test-bucket", mockClient, true)); final InOrder inOrder = inOrder(mockClient); inOrder.verify(mockClient).getBucketLocation(anyConsumer()); @@ -203,12 +203,12 @@ public void generateAsyncClientByEndpointBucketCredentials() { provider.asyncClientBuilder = BUILDER; provider.configuration.withEndpoint("endpoint1:1010"); - provider.generateAsyncClient("bucket1"); + provider.generateAsyncClient("bucket1", true); then(BUILDER.endpointOverride.toString()).isEqualTo("https://endpoint1:1010"); then(BUILDER.region).isEqualTo(Region.US_EAST_1); // just a default in the case not provide provider.configuration.withEndpoint("endpoint2:2020"); - provider.generateAsyncClient("bucket2"); + provider.generateAsyncClient("bucket2", true); then(BUILDER.endpointOverride.toString()).isEqualTo("https://endpoint2:2020"); then(BUILDER.region).isEqualTo(Region.US_EAST_1); // just a default in the case not provide } From abd07fd22286b4cbf847340a09861a10a0a67488 Mon Sep 17 00:00:00 2001 From: guicamest Date: Sat, 4 Nov 2023 21:46:05 +0100 Subject: [PATCH 7/7] chore: extract configureCrtClientForRegion method --- .../amazon/nio/spi/s3/S3ClientProvider.java | 52 ++++++++++--------- 1 file changed, 27 insertions(+), 25 deletions(-) diff --git a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java index 9f495844..1731a6a3 100644 --- a/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java +++ b/src/main/java/software/amazon/nio/spi/s3/S3ClientProvider.java @@ -55,7 +55,7 @@ public class S3ClientProvider { final protected S3NioSpiConfiguration configuration; /** - * Default client using the "https://s3.us-east-1.amazonaws.com" endpoint + * Default client using the "..." endpoint */ private static final S3Client DEFAULT_CLIENT = S3Client.builder() .endpointOverride(URI.create("https://s3.us-east-1.amazonaws.com")) @@ -63,7 +63,7 @@ public class S3ClientProvider { .build(); /** - * Default asynchronous client using the "https://s3.us-east-1.amazonaws.com" endpoint + * Default asynchronous client using the "..." endpoint */ private static final S3AsyncClient DEFAULT_ASYNC_CLIENT = S3AsyncClient.builder() .endpointOverride(URI.create("https://s3.us-east-1.amazonaws.com")) @@ -184,11 +184,8 @@ private T getClientForBucket( logger.debug("generating client for bucket: '{}'", bucketName); T bucketSpecificClient = null; - if ((configuration.getEndpoint() == null) || configuration.getEndpoint().isBlank()) { - // - // we try to locate a bucket only if no endpoint is provided, which - // means we are dealing with AWS S3 buckets - // + if (configuration.endpointURI() == null) { + // we try to locate a bucket only if no endpoint is provided, which means we are dealing with AWS S3 buckets String bucketLocation = determineBucketLocation(bucketName, locationClient); if ( bucketLocation != null) { @@ -253,24 +250,7 @@ private S3AsyncClient asyncClientForRegion(String regionName, boolean crt) { if (!crt) { return configureClientForRegion(regionName, S3AsyncClient.builder()); } - Region region = getRegionFromRegionName(regionName); - logger.debug("bucket region is: '{}'", region.id()); - - URI endpointUri = configuration.endpointURI(); - if (endpointUri != null) { - asyncClientBuilder.endpointOverride(endpointUri); - } - - AwsCredentials credentials = configuration.getCredentials(); - if (credentials != null) { - asyncClientBuilder.credentialsProvider(() -> credentials); - } - - return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build(); - } - - private static Region getRegionFromRegionName(String regionName) { - return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName); + return configureCrtClientForRegion(regionName); } private > ActualClient configureClientForRegion( @@ -301,4 +281,26 @@ private credentials); + } + + return asyncClientBuilder.forcePathStyle(configuration.getForcePathStyle()).region(region).build(); + } + + private static Region getRegionFromRegionName(String regionName) { + return (regionName == null || regionName.isBlank()) ? Region.US_EAST_1 : Region.of(regionName); + } + }