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

Change response types for POJOs that map one to many #6024

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

davidh44
Copy link
Contributor

Motivation and Context

There are a handful of POJOs in v1 that map to two types in v2. For instance, v1 AccessControlList is used as input to setObjectAcl(), and also returned as response from getObjectAcl().

In v2, AccessControlPolicy is used as input, while GetObjectAclResponse is returned as the response.

Modifications

Forked ChangeMethodInvocationReturnType recipe from OpenRewrite. It is a newer recipe so requires OR version bump, which introduces breaking changes with existing SDK recipes.

https://github.com/openrewrite/rewrite/blob/main/rewrite-java/src/main/java/org/openrewrite/java/ChangeMethodInvocationReturnType.java

Using this recipe to correctly change response types based on the method, returning the following responses:

GetObjectResponse
GetBucketAclResponse
GetObjectAclResponse
GetBucketAccelerateConfigurationResponse
GetBucketLifecycleConfigurationResponse
GetBucketCorsResponse
GetBucketLoggingResponse
GetBucketNotificationConfigurationResponse
GetBucketPolicyResponse
GetBucketReplicationResponse
GetBucketTaggingResponse
GetBucketVersioningResponse
GetBucketWebsiteResponse

Additional changes:

  • Map AmazonS3Exception to S3Exception
  • Map BucketLoggingConfiguration to BucketLoggingStatus
  • Revert converting CannedAccessControlList to ObjectCannedACL
    • We will leave as v1 POJO, and add comments in follow up PR to tell users to manually change to the appropriate v2 enum based on the method used, i.e. BucketCannedACL or ObjectCannedACL
  • Replace BucketName with Bucket in getter methods (already in place for setters)
  • Transform getBucketLocation() to append locationConstraint().toString() as v1 returns the String region

Testing

Added end to end tests

@davidh44 davidh44 requested a review from a team as a code owner April 10, 2025 22:56
@@ -253,7 +253,7 @@ private void singleBucketArgMethods(S3Client s3, String bucket) {
s3.getBucketPolicy(GetBucketPolicyRequest.builder().bucket(bucket)
.build());
s3.getBucketLocation(GetBucketLocationRequest.builder().bucket(bucket)
.build());
.build()).locationConstraint().toString();
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 update it to return a variable?

Copy link
Contributor Author

@davidh44 davidh44 Apr 11, 2025

Choose a reason for hiding this comment

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

Its covered it in S3RequestConstructor

import software.amazon.awssdk.annotations.SdkInternalApi;

/**
* This recipe is forked from
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

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