Skip to content
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

fix(CopySourceRequestInS3): Updated codegen to customize the request before calling service #5244

Merged
merged 6 commits into from
May 28, 2024

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented May 24, 2024

Updated codegen to customize the request before sending, instead of using interceptors. This change ensures that customized parameters are available for EndpointResolveInterceptors during execution.

Motivation and Context

  • In AWS SDK Java 2.x, the “copySource” API provided by model in CopyObjectRequest is deprecated.
  • It has been replaced with "sourceBucket," "sourceKey," and "sourceVersionId" via customization.
  • Recently, S3 wants “copySource” to be included in the Endpoint Params by adding in endpoint-ruleset.json
  • Since "copySource" is deprecated, the new parameters "sourceBucket," "sourceKey," and "sourceVersionId" are used to update the request.
  • The current binding of Endpoint Parameters from Request Parameters via the EndpointResolverInterceptors does not copy the customized values.
  • The current CopySourceInterceptor copies over CopySource during the modifyRequest , but we need these to be updated in. 'beforeExecution` of EndpointResolverInterceptors

Modifications

  • Add customization to transform Custom Request
  "preClientExecutionRequestCustomizer": {
    "OperationWithCustomMember": {
      "methodName": "dummyRequestModifier",
      "className": "software.amazon.awssdk.codegen.internal.UtilsTest"
    }
  }

Will result in

    @Override
    public CompletableFuture<OperationWithCustomMemberResponse> operationWithCustomMember(
        OperationWithCustomMemberRequest operationWithCustomMemberRequest) {
        // New single line generated from above customization
        operationWithCustomMemberRequest = UtilsTest.dummyRequestModifier(operationWithCustomMemberRequest);
        SdkClientConfiguration clientConfiguration = updateSdkClientConfiguration(operationWithCustomMemberRequest,
        // Existing Code
  

}

Testing

  • Added Junits
  • Already Integ test were present and these passed

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

License

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

@joviegas joviegas requested a review from a team as a code owner May 24, 2024 19:40
@joviegas joviegas force-pushed the joviegas/api_prepare_request branch 3 times, most recently from fb488a1 to d70d43c Compare May 24, 2024 21:01
…before sending, instead of using interceptors. This change ensures that customized parameters are available for EndpointResolveInterceptors during beforeExecution.
@joviegas joviegas force-pushed the joviegas/api_prepare_request branch from d70d43c to 3bf3f76 Compare May 24, 2024 21:44
*/
@SdkInternalApi
public final class CopySourceInterceptor implements ExecutionInterceptor {
public final class RequestModifierUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to keep a reference to CopySource in this class name, CopySourceModifierUtils maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the name as CustomRequestTransformerUtils since in future if we need to do similar customization to any API then we can just add an API in this class.
Thus this class will have broader scope of all the RequestTransformer rather than just CopySource

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you were thinking of reusing this class for other things. I see. Would it be better to keep this class only for CopySource purpose or not?

Copy link
Contributor Author

@joviegas joviegas May 28, 2024

Choose a reason for hiding this comment

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

I was thinking of using this class to hold the static APIs that do the requests transformations via customization.
In this case we have two API copyObject and UploadPartCopy.
Since this utils is created to hold the CustomRequestTransformers thart will be used in Customizations I was thinking to keep its scope at this level rather than CopySource which is just a Field which gets transformed.

@@ -21,5 +21,11 @@
}
}
}
]
],
"customRequestTransformerMap": {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I would drop the map suffix (everything is a map in json), simply customRequestTransformers sounds more clear to me. Other keys, like customOperationContextParams don't have the map suffix, it would make the customization file more consistent. But it is not a blocker for me.

*/
@SdkInternalApi
public final class CopySourceInterceptor implements ExecutionInterceptor {
public final class RequestModifierUtils {
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh you were thinking of reusing this class for other things. I see. Would it be better to keep this class only for CopySource purpose or not?

Copy link

sonarcloud bot commented May 28, 2024

@joviegas joviegas merged commit 43fed73 into master May 28, 2024
16 of 18 checks passed
akidambisrinivasan pushed a commit to akidambisrinivasan/aws-sdk-java-v2 that referenced this pull request Jun 28, 2024
…before calling service (aws#5244)

* fix(CopySourceRequestInS3): Updated codegen to customize the request before sending, instead of using interceptors. This change ensures that customized parameters are available for EndpointResolveInterceptors during beforeExecution.

* Handle review comments

* Handle review comments 2

* Handle review comments 3

* Added change log
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