Skip to content

Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators #4151

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

Merged

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Jun 29, 2023

Attach crossRegionAccessEnabled as CustomClientParams to a client Builder, also mention of Composers are renamed as decorators

Motivation and Context

S3Client.builder().crossRegionAccessEnabled(true).build();
  1. Attach crossRegionAccessEnabled to Client Builder directly.
  2. Removed crossRegionAccessEnabled from S3Configuration.
  3. Customization to support Custom Client Params in customization.config
  4. Rename Composers as Decorators
  5. Remove the Type Casting in the Decorators (earlier known as composers)

Modifications

  1. Codegen to support new Client Custom Param context
  2. Remove generic type and interface for Composers
  3. Replace Composers as decorators
  4. Remove crossRegionAccessEnabled API from S3Configuration

Testing

  1. Junits added .

License

  • I confirm that this pull request can be released under the Apache 2 license

…lder, also mention of Composers are renamed as decorators
@joviegas joviegas requested a review from a team as a code owner June 29, 2023 06:40
Comment on lines 74 to 75
clientContextParams.get(JsonClientContextParams.CROSS_REGION_ACCESS_ENABLED) == null,
"Cross region access enabled has been configured on both ServiceConfiguration and the client level. Please limit this configuration to one location.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we support both places? Is it hard to not support it in S3ClientConfiguration?

Copy link
Contributor Author

@joviegas joviegas Jun 29, 2023

Choose a reason for hiding this comment

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

The reason it is supported in both places is that the existing S3ClientContextParams are available in both locations. To access the values of these context parameters, they are accessed through getters on the S3Configuration.

        if (serviceConfigBuilder.crossRegionAccessEnabled() != null) {
            Validate.validState(Throw Exception in both places);
        } else {
            serviceConfigBuilder.crossRegionAccessEnabled(clientContextParams
                    .get(S3ClientContextParams.CROSS_REGION_ACCESS_ENABLED));
        }

What if the user wants to create a Config object solely to set the Client config in that S3 Configuration?"

"Is it difficult to not support it in S3ClientConfiguration?"

"It's not that difficult. We need to pass ContextParams to the decorator along with the service configuration to determine if it's a cross-region request."

"If we do not support this in S3Configuration, we will not be able to group it along with other client settings like useArnRegionEnabled, multiRegionEnabled, etc."

Copy link
Contributor Author

@joviegas joviegas Jun 29, 2023

Choose a reason for hiding this comment

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

Done ,removed it from S3ClientConfiguration

@joviegas joviegas force-pushed the joviegas_pre_release_fixes branch from 4e0e5a1 to 5c0193a Compare June 29, 2023 23:33
@joviegas joviegas force-pushed the joviegas_pre_release_fixes branch from 5c0193a to 8cb76a2 Compare June 29, 2023 23:49
Copy link
Contributor

@millems millems left a comment

Choose a reason for hiding this comment

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

Can you review the sonar bugs and mark them as a false positive or fix them?

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 30, 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 52 Code Smells

88.1% 88.1% Coverage
1.3% 1.3% Duplication

@joviegas joviegas merged commit fd2f046 into cross_region_bucket_access/project_branch Jul 1, 2023
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]>
@joviegas joviegas deleted the joviegas_pre_release_fixes branch January 15, 2025 17:37
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.

2 participants