Skip to content

Wraps s3 client in cross regional client when enabled #4080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

cenedhryn
Copy link
Contributor

@cenedhryn cenedhryn commented Jun 8, 2023

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

  • Adds a customization flag to point out where the logic for creating a composed S3 client is located
  • Changes the builder logic to check the customization flag and if so, call the composing code instead of just returning the client
  • Also refactors the builder code to make it more readable
  • Adds a decorator utility that can do conditional transforms
  • Adds S3 specific logic for creating a list of decorators for client construction and conditions on when to apply them, that are submitted to the decorator utility. Adds the cross region client to the list.

Testing

Unit and functional testing.

@cenedhryn cenedhryn requested a review from a team as a code owner June 8, 2023 21:54
@cenedhryn cenedhryn force-pushed the salande/cross_region_builder branch from 3805867 to c3d4502 Compare June 9, 2023 17:39
@cenedhryn cenedhryn force-pushed the salande/cross_region_builder branch from c3d4502 to 45f255c Compare June 9, 2023 17:52
"hasAccelerateModeEnabledProperty":true
},
"customRetryPolicy": "software.amazon.TestRetryPolicy",
"clientComposerFactory": "software.amazon.awssdk.services.builder.ClientComposerFactory",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are other parameters required in the test ?
Is it possible to keep just clientComposerFactory in this test customization.config ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed some unused parameters

return clientComposerFactory;
}

public void setClientComposerFactory(String clientComposerFactory) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't seem like we have any checks for similar parameters (fqcn). What do you suggest would be allowed namespace patterns?

Copy link
Contributor

@joviegas joviegas Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 13, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 74 Code Smells

88.4% 88.4% Coverage
1.8% 1.8% Duplication

@cenedhryn cenedhryn merged commit 578cc0d into cross_region_bucket_access/project_branch Jun 14, 2023
@cenedhryn cenedhryn deleted the salande/cross_region_builder branch June 14, 2023 16:13
joviegas added a commit that referenced this pull request Jul 5, 2023
* 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 <[email protected]>
L-Applin pushed a commit that referenced this pull request Jul 24, 2023
* 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 <[email protected]>
L-Applin pushed a commit that referenced this pull request Jul 24, 2023
* 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants