Wraps s3 client in cross regional client when enabled#4080
Conversation
3805867 to
c3d4502
Compare
c3d4502 to
45f255c
Compare
utils/src/main/java/software/amazon/awssdk/utils/ConditionalDecorator.java
Show resolved
Hide resolved
...es/s3/src/main/java/software/amazon/awssdk/services/s3/internal/client/S3ClientComposer.java
Outdated
Show resolved
Hide resolved
| "hasAccelerateModeEnabledProperty":true | ||
| }, | ||
| "customRetryPolicy": "software.amazon.TestRetryPolicy", | ||
| "clientComposerFactory": "software.amazon.awssdk.services.builder.ClientComposerFactory", |
There was a problem hiding this comment.
Are other parameters required in the test ?
Is it possible to keep just clientComposerFactory in this test customization.config ?
There was a problem hiding this comment.
Removed some unused parameters
| return clientComposerFactory; | ||
| } | ||
|
|
||
| public void setClientComposerFactory(String clientComposerFactory) { |
There was a problem hiding this comment.
Can we add some validator here using regex to match the expected fqcn pattern and error out if it fails to match the supported package patters ?
There was a problem hiding this comment.
It doesn't seem like we have any checks for similar parameters (fqcn). What do you suggest would be allowed namespace patterns?
There was a problem hiding this comment.
We can do it in future something like this
String pattern = "^(?:[a-zA-Z_$][a-zA-Z\\d_$]*\\.)+[a-zA-Z_$][a-zA-Z\\d_$]*$";
Validate.true(Pattern.matches(pattern, fqcnSuppliedUser) , "Class name is not fqcn");| .collect(Collectors.toList()); | ||
| assertThat(ConditionalDecorator.decorate(0, decorators)).isEqualTo(20); | ||
| } | ||
| } |
There was a problem hiding this comment.
I appreciate the elegant utilization of the ConditionalDecorator, effectively incorporating the decorator pattern and lambda functions in this implementation.
I used String representation just to test for my understanding , you can add this test cases if you want.
Commenting just to save this test case in string form
import static org.assertj.core.api.Assertions.assertThat;
import java.util.*;
import org.junit.jupiter.api.Test;
public class ConditionalDecoratorTest {
@Test
void testStringDecoration() {
StringComposer stringComposer = new StringComposer();
// assertThat(stringComposer.composeString("test", star?, dollar?).getBaseString()).isEqualTo(Result);
assertThat(stringComposer.composeString("test", false, false).getBaseString()).isEqualTo("test");
assertThat(stringComposer.composeString("test", false, true).getBaseString()).isEqualTo("$test$");
assertThat(stringComposer.composeString("test", true, false).getBaseString()).isEqualTo("*test*");
assertThat(stringComposer.composeString("test", true, true).getBaseString()).isEqualTo("$*test*$");
}
class BaseString {
String baseString;
public BaseString(String baseString) {
this.baseString = baseString;
}
protected String getBaseString() {
return baseString;
}
}
class DefaultString extends BaseString {
private DefaultString(String baseString) {
super(baseString);
}
}
class StarDecorated extends BaseString {
private StarDecorated(BaseString baseString) {
super(baseString.getBaseString());
}
@Override
public String getBaseString() {
return "*" + baseString + "*";
}
}
class DollarDecorated extends BaseString {
private DollarDecorated(BaseString baseString) {
super(baseString.getBaseString());
}
@Override
public String getBaseString() {
return "$" + baseString + "$";
}
}
class StringComposer {
public BaseString composeString(String base, boolean starDecorated, boolean dollarDecorated) {
List<ConditionalDecorator<BaseString>> decorators = new ArrayList<>();
decorators.add(ConditionalDecorator.create(testOne -> starDecorated, StarDecorated::new));
decorators.add(ConditionalDecorator.create(testTwo -> dollarDecorated, DollarDecorated::new));
return ConditionalDecorator.decorate(new DefaultString(base), decorators);
}
}
}|
Kudos, SonarCloud Quality Gate passed! |
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <salande@amazon.com>
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <salande@amazon.com>
* Adding a generic method invokeOperation for abstract delegating clien… (#3996) Adding a generic method invokeOperation for abstract delegating client to allow overriding behavior changes for all operations * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration (#3978) * Add endpointProvider in ClientConfig and RequestOverrideConfig. Also add crossRegionBucketAccess in S3Configuration * updated review comments 1 * updated review comments 2 * Adds S3CrossRegion clients and codegen for retrieving bucket name (#4008) * Adds S3CrossRegion clients and codegen for retrieving bucket name * Removes codegen of bucket parameter since getValueForField() is better * Add back overloads * Adds cross region client logic for decorating endpoint provider (#4026) * Adds cross region client logic for decorating endpoint provider * Paginated methods returning publisher or iterable implement logic in … (#4046) * Paginated methods returning publisher or iterable implement logic in interface instead of throwing unsupported exception * Added internal annotation to class * Wraps s3 client in cross regional client when enabled (#4080) * Wraps s3 client in cross regional client when enabled * Adds composer interface * Add User Agent Api name for Cross region API calls (#4105) * Add User Agent Api name for Cross region API calls * Added test case for default client not to have user agent related to cross region * S3CrossRegion Sync and Async Clients Redirect implementation (#4089) * S3CrossRegionSyncClient Redirect implementation * Added implementation for Async client Decorator * Updated older Cross region test cases * Added paramterized test * Async Exception checged to completableException * Updated test cases and changes the Exception handling when HeadBucket Call fails * Handled Anna-karin's comments * Removed async execution of HeadBucket and attached it to the completableFuture of main request * Handled Zoe's comments * Added test case when Redirected after the Region is cached * Changed region constant to Region Type in Tests * Cross region support for CRT Client (#4129) * Cross region support for CRT Client * removing common class * handled review comments * Integration test cases for Cross region Async and Sync Clients (#4128) * Integration test cases for Cross region Sync and Sync Clients * Renamed files as Integration test * Handled CR comments to take care of retries thus captureInterceptor is removed * rebased and added CRT in Integ test * removed sout from integ * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators (#4151) * Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators * Removed from S3Configurations * Move the optional parameter in getter --------- Co-authored-by: Anna-Karin Salander <salande@amazon.com>
Motivation and Context
Users want cross region ability, but do not want or need to know implementation details, they just want to enable the feature. This change modifies the S3 client builder code to check if cross region is enabled and then wraps the regular client in the cross region client and returns it (as an S3Client/S3AsyncClient).
Modifications
Testing
Unit and functional testing.