Skip to content

Commit a712820

Browse files
joviegasjoviegas
committed
Handled Anna-karin's comments
1 parent 95f389d commit a712820

File tree

6 files changed

+67
-82
lines changed

6 files changed

+67
-82
lines changed

‎services/s3/src/main/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClient.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,10 @@ protected <T extends S3Request, ReturnT> ReturnT invokeOperation(T request, Func
6969
if (exception.statusCode() == REDIRECT_STATUS_CODE) {
7070
updateCacheFromRedirectException(exception, bucketName);
7171
return operation.apply(
72-
requestWithDecoratedEndpointProvider(request, regionSupplier(bucketName),
73-
serviceClientConfiguration().endpointProvider().get()));
72+
requestWithDecoratedEndpointProvider(
73+
request,
74+
() -> bucketToRegionCache.computeIfAbsent(bucketName, this::fetchBucketRegion),
75+
serviceClientConfiguration().endpointProvider().get()));
7476
}
7577
throw exception;
7678
}
@@ -83,10 +85,6 @@ private void updateCacheFromRedirectException(S3Exception exception, String buck
8385
regionStr.ifPresent(region -> bucketToRegionCache.put(bucketName, Region.of(region)));
8486
}
8587

86-
private Supplier<Region> regionSupplier(String bucket) {
87-
return () -> bucketToRegionCache.computeIfAbsent(bucket, this::fetchBucketRegion);
88-
}
89-
9088
private Region fetchBucketRegion(String bucketName) {
9189
try {
9290
((S3Client) delegate()).headBucket(HeadBucketRequest.builder().bucket(bucketName).build());

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientRedirectTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@
3636
import software.amazon.awssdk.services.s3.model.S3Exception;
3737
import software.amazon.awssdk.utils.CompletableFutureUtils;
3838

39-
public class S3CrossRegionAsyncClientRedirectTest extends S3DecoratorRedirectBaseTest {
39+
public class S3CrossRegionAsyncClientRedirectTest extends S3DecoratorRedirectTestBase {
4040
private static S3AsyncClient mockDelegateAsyncClient;
4141
private S3AsyncClient decoratedS3AsyncClient;
4242

@@ -55,7 +55,7 @@ protected void stubRedirectSuccessSuccess() {
5555
}
5656

5757
@Override
58-
protected ListObjectsResponse firstApiCallToService() throws Throwable {
58+
protected ListObjectsResponse apiCallToService() throws Throwable {
5959
try{
6060
return decoratedS3AsyncClient.listObjects(i -> i.bucket(CROSS_REGION_BUCKET)).join();
6161
}catch (CompletionException exception){
@@ -70,7 +70,7 @@ protected void verifyTheApiServiceCall(int times, ArgumentCaptor<ListObjectsRequ
7070

7171
@Override
7272
protected void stubServiceClientConfiguration() {
73-
when(mockDelegateAsyncClient.serviceClientConfiguration()).thenReturn(ENDPOINT_CONFIGURED);
73+
when(mockDelegateAsyncClient.serviceClientConfiguration()).thenReturn(CONFIGURED_ENDPOINT_PROVIDER);
7474
}
7575

7676
@Override

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionAsyncClientTest.java

Lines changed: 12 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,9 @@
1717

1818
import static org.assertj.core.api.Assertions.assertThat;
1919
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
20-
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectBaseTest.CROSS_REGION;
21-
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectBaseTest.X_AMZ_BUCKET_REGION;
20+
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectTestBase.CROSS_REGION;
21+
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectTestBase.OVERRIDE_CONFIGURED_REGION;
22+
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectTestBase.X_AMZ_BUCKET_REGION;
2223

2324
import java.net.URI;
2425
import java.util.Arrays;
@@ -39,7 +40,6 @@
3940
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
4041
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
4142
import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
42-
import software.amazon.awssdk.endpoints.Endpoint;
4343
import software.amazon.awssdk.endpoints.EndpointProvider;
4444
import software.amazon.awssdk.http.AbortableInputStream;
4545
import software.amazon.awssdk.http.HttpExecuteResponse;
@@ -50,9 +50,6 @@
5050
import software.amazon.awssdk.regions.Region;
5151
import software.amazon.awssdk.services.s3.S3AsyncClient;
5252
import software.amazon.awssdk.services.s3.S3AsyncClientBuilder;
53-
import software.amazon.awssdk.services.s3.S3Client;
54-
import software.amazon.awssdk.services.s3.endpoints.S3EndpointParams;
55-
import software.amazon.awssdk.services.s3.endpoints.S3EndpointProvider;
5653
import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
5754
import software.amazon.awssdk.services.s3.internal.crossregion.endpointprovider.BucketEndpointProvider;
5855
import software.amazon.awssdk.services.s3.model.GetObjectRequest;
@@ -69,7 +66,6 @@ class S3CrossRegionAsyncClientTest {
6966
private static final String BUCKET = "bucket";
7067
private static final String KEY = "key";
7168
private static final String TOKEN = "token";
72-
7369
private final MockAsyncHttpClient mockAsyncHttpClient = new MockAsyncHttpClient();
7470
private CaptureInterceptor captureInterceptor;
7571
private S3AsyncClient s3Client;
@@ -148,16 +144,16 @@ void crossRegionClient_CallsHeadObject_when_regionNameNotPresentInFallBackCall()
148144
customHttpResponse(301, CROSS_REGION ),
149145
successHttpResponse(), successHttpResponse());
150146
S3AsyncClient crossRegionClient =
151-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
147+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
152148
crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY), AsyncResponseTransformer.toBytes()).join();
153149
assertThat(captureInterceptor.endpointProvider).isInstanceOf(BucketEndpointProvider.class);
154150

155151
List<SdkHttpRequest> requests = mockAsyncHttpClient.getRequests();
156152
assertThat(requests).hasSize(3);
157153

158154
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
159-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
160-
Region.US_WEST_2.toString(),
155+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
156+
OVERRIDE_CONFIGURED_REGION.toString(),
161157
CROSS_REGION));
162158

163159
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
@@ -187,7 +183,7 @@ void crossRegionClient_CallsHeadObjectErrors_shouldTerminateTheAPI() {
187183
customHttpResponse(400, null ),
188184
successHttpResponse(), successHttpResponse());
189185
S3AsyncClient crossRegionClient =
190-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2)
186+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION)
191187
.serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
192188

193189
assertThatExceptionOfType(CompletionException.class)
@@ -198,8 +194,8 @@ void crossRegionClient_CallsHeadObjectErrors_shouldTerminateTheAPI() {
198194
assertThat(requests).hasSize(2);
199195

200196
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
201-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
202-
Region.US_WEST_2.toString()));
197+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
198+
OVERRIDE_CONFIGURED_REGION.toString()));
203199

204200
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
205201
.isEqualTo(Arrays.asList(SdkHttpMethod.GET,
@@ -212,7 +208,7 @@ void crossRegionClient_CallsHeadObjectWithNoRegion_shouldTerminateHeadBucketAPI(
212208
customHttpResponse(301, null ),
213209
successHttpResponse(), successHttpResponse());
214210
S3AsyncClient crossRegionClient =
215-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2)
211+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION)
216212
.serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
217213

218214
assertThatExceptionOfType(CompletionException.class)
@@ -224,8 +220,8 @@ void crossRegionClient_CallsHeadObjectWithNoRegion_shouldTerminateHeadBucketAPI(
224220
assertThat(requests).hasSize(2);
225221

226222
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
227-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
228-
Region.US_WEST_2.toString()));
223+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
224+
OVERRIDE_CONFIGURED_REGION.toString()));
229225

230226
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
231227
.isEqualTo(Arrays.asList(SdkHttpMethod.GET,

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClientRedirectTest.java

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@
3333
import software.amazon.awssdk.services.s3.model.ListObjectsResponse;
3434
import software.amazon.awssdk.services.s3.model.S3Exception;
3535

36-
public class S3CrossRegionSyncClientRedirectTest extends S3DecoratorRedirectBaseTest {
36+
public class S3CrossRegionSyncClientRedirectTest extends S3DecoratorRedirectTestBase {
3737

3838
private static S3Client mockDelegateClient;
3939
private S3Client decoratedS3Client;
@@ -100,7 +100,7 @@ protected void stubRedirectSuccessSuccess() {
100100
}
101101

102102
@Override
103-
protected ListObjectsResponse firstApiCallToService() {
103+
protected ListObjectsResponse apiCallToService() {
104104
return decoratedS3Client.listObjects(i -> i.bucket(CROSS_REGION_BUCKET));
105105
}
106106

@@ -116,7 +116,7 @@ protected void verifyHeadBucketServiceCall(int times) {
116116

117117
@Override
118118
protected void stubServiceClientConfiguration() {
119-
when(mockDelegateClient.serviceClientConfiguration()).thenReturn(ENDPOINT_CONFIGURED);
119+
when(mockDelegateClient.serviceClientConfiguration()).thenReturn(CONFIGURED_ENDPOINT_PROVIDER);
120120
}
121121

122122
@Override

‎services/s3/src/test/java/software/amazon/awssdk/services/s3/internal/crossregion/S3CrossRegionSyncClientTest.java

Lines changed: 11 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -15,21 +15,17 @@
1515

1616
package software.amazon.awssdk.services.s3.internal.crossregion;
1717

18-
import static org.assertj.core.api.Assertions.as;
1918
import static org.assertj.core.api.Assertions.assertThat;
2019
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2120
import static org.mockito.Mockito.verify;
2221
import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionAsyncClientTest.customHttpResponse;
2322
import static software.amazon.awssdk.services.s3.internal.crossregion.S3CrossRegionAsyncClientTest.successHttpResponse;
24-
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectBaseTest.CROSS_REGION;
25-
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectBaseTest.X_AMZ_BUCKET_REGION;
26-
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectBaseTest.X_AMZ_BUCKET_REGION;
23+
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectTestBase.CROSS_REGION;
24+
import static software.amazon.awssdk.services.s3.internal.crossregion.S3DecoratorRedirectTestBase.OVERRIDE_CONFIGURED_REGION;
2725

28-
import java.net.URI;
2926
import java.util.Arrays;
3027
import java.util.List;
3128
import java.util.function.Consumer;
32-
import java.util.function.Supplier;
3329
import java.util.stream.Collectors;
3430
import java.util.stream.Stream;
3531
import org.junit.jupiter.api.AfterEach;
@@ -38,30 +34,23 @@
3834
import org.junit.jupiter.params.ParameterizedTest;
3935
import org.junit.jupiter.params.provider.Arguments;
4036
import org.junit.jupiter.params.provider.MethodSource;
41-
import org.mockito.ArgumentCaptor;
4237
import software.amazon.awssdk.core.interceptor.Context;
4338
import software.amazon.awssdk.core.interceptor.ExecutionAttributes;
4439
import software.amazon.awssdk.core.interceptor.ExecutionInterceptor;
4540
import software.amazon.awssdk.core.interceptor.SdkInternalExecutionAttribute;
4641
import software.amazon.awssdk.endpoints.EndpointProvider;
47-
import software.amazon.awssdk.http.AbortableInputStream;
48-
import software.amazon.awssdk.http.HttpExecuteRequest;
49-
import software.amazon.awssdk.http.HttpExecuteResponse;
5042
import software.amazon.awssdk.http.SdkHttpMethod;
5143
import software.amazon.awssdk.http.SdkHttpRequest;
52-
import software.amazon.awssdk.http.SdkHttpResponse;
5344
import software.amazon.awssdk.regions.Region;
5445
import software.amazon.awssdk.services.s3.S3Client;
5546
import software.amazon.awssdk.services.s3.S3ClientBuilder;
56-
import software.amazon.awssdk.services.s3.endpoints.S3EndpointProvider;
5747
import software.amazon.awssdk.services.s3.endpoints.internal.DefaultS3EndpointProvider;
5848
import software.amazon.awssdk.services.s3.internal.crossregion.endpointprovider.BucketEndpointProvider;
5949
import software.amazon.awssdk.services.s3.model.GetObjectRequest;
6050
import software.amazon.awssdk.services.s3.model.ListObjectsV2Response;
6151
import software.amazon.awssdk.services.s3.model.S3Exception;
6252
import software.amazon.awssdk.services.s3.paginators.ListObjectsV2Iterable;
6353
import software.amazon.awssdk.testutils.service.http.MockSyncHttpClient;
64-
import software.amazon.awssdk.utils.StringInputStream;
6554

6655
class S3CrossRegionSyncClientTest {
6756

@@ -110,7 +99,6 @@ private static Stream<Arguments> stubOverriddenEndpointProviderResponses() {
11099
);
111100
}
112101

113-
114102
@ParameterizedTest
115103
@MethodSource("stubResponses")
116104
void standardOp_crossRegionClient_noOverrideConfig_SuccessfullyIntercepts(Consumer<MockSyncHttpClient> stubConsumer,
@@ -159,7 +147,6 @@ void crossRegionClient_createdWithWrapping_SuccessfullyIntercepts(Consumer<MockS
159147
assertThat(captureInterceptor.endpointProvider).isInstanceOf(endpointProviderType);
160148
}
161149

162-
163150
@ParameterizedTest
164151
@MethodSource("stubOverriddenEndpointProviderResponses")
165152
void standardOp_crossRegionClient_takesCustomEndpointProviderInRequest(Consumer<MockSyncHttpClient> stubConsumer,
@@ -209,16 +196,16 @@ void crossRegionClient_CallsHeadObject_when_regionNameNotPresentInFallBackCall()
209196
customHttpResponse(301, CROSS_REGION ),
210197
successHttpResponse(), successHttpResponse());
211198
S3Client crossRegionClient =
212-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
199+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
213200
crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY));
214201
assertThat(captureInterceptor.endpointProvider).isInstanceOf(BucketEndpointProvider.class);
215202

216203
List<SdkHttpRequest> requests = mockSyncHttpClient.getRequests();
217204
assertThat(requests).hasSize(3);
218205

219206
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
220-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
221-
Region.US_WEST_2.toString(),
207+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
208+
OVERRIDE_CONFIGURED_REGION.toString(),
222209
CROSS_REGION));
223210

224211
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
@@ -248,7 +235,7 @@ void crossRegionClient_CallsHeadObjectErrors_shouldTerminateTheAPI() {
248235
customHttpResponse(400, null ),
249236
successHttpResponse(), successHttpResponse());
250237
S3Client crossRegionClient =
251-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
238+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
252239

253240
assertThatExceptionOfType(S3Exception.class)
254241
.isThrownBy(() -> crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY)))
@@ -258,8 +245,8 @@ void crossRegionClient_CallsHeadObjectErrors_shouldTerminateTheAPI() {
258245
assertThat(requests).hasSize(2);
259246

260247
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
261-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
262-
Region.US_WEST_2.toString()));
248+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
249+
OVERRIDE_CONFIGURED_REGION.toString()));
263250

264251
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
265252
.isEqualTo(Arrays.asList(SdkHttpMethod.GET,
@@ -272,7 +259,7 @@ void crossRegionClient_CallsHeadObjectWithNoRegion_shouldTerminateHeadBucketAPI(
272259
customHttpResponse(301, null ),
273260
successHttpResponse(), successHttpResponse());
274261
S3Client crossRegionClient =
275-
clientBuilder().endpointOverride(null).region(Region.US_WEST_2).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
262+
clientBuilder().endpointOverride(null).region(OVERRIDE_CONFIGURED_REGION).serviceConfiguration(c -> c.crossRegionAccessEnabled(true)).build();
276263

277264
assertThatExceptionOfType(S3Exception.class)
278265
.isThrownBy(() -> crossRegionClient.getObject(r -> r.bucket(BUCKET).key(KEY)))
@@ -282,8 +269,8 @@ void crossRegionClient_CallsHeadObjectWithNoRegion_shouldTerminateHeadBucketAPI(
282269
assertThat(requests).hasSize(2);
283270

284271
assertThat(requests.stream().map(req -> req.host().substring(10,req.host().length() - 14 )).collect(Collectors.toList()))
285-
.isEqualTo(Arrays.asList(Region.US_WEST_2.toString(),
286-
Region.US_WEST_2.toString()));
272+
.isEqualTo(Arrays.asList(OVERRIDE_CONFIGURED_REGION.toString(),
273+
OVERRIDE_CONFIGURED_REGION.toString()));
287274

288275
assertThat(requests.stream().map(req -> req.method()).collect(Collectors.toList()))
289276
.isEqualTo(Arrays.asList(SdkHttpMethod.GET,

0 commit comments

Comments
 (0)