Transforming S3 PUT override #5218
Conversation
This transforms the AmazonS3#putObject(String,String,File) override.
|
| + " }\n" | ||
| + "}\n", | ||
| "import com.amazonaws.services.s3.AmazonS3Client;\n" | ||
| + "import com.amazonaws.services.s3.model.PutObjectRequest;\n" |
There was a problem hiding this comment.
Should these imports be updated to v2 or is this handled by another recipe?
There was a problem hiding this comment.
It's odd the tests are not failing. Maybe we can apply multiple recipes? That's what I did for other tests https://github.com/aws/aws-sdk-java-v2/blob/feature/master/migration-tool/migration-tool/src/test/java/software/amazon/awssdk/migration/recipe/ChangeConfigTypesTest.java#L42
There was a problem hiding this comment.
Yeah It's only testing for this recipe. I see less value in adding other recipes here since I want to test just the changes this specific recipe is expected to make
There was a problem hiding this comment.
Oh, so the tests actually are not failing due to method reference not found (don't remember the exact message)?
There was a problem hiding this comment.
Yeah. I think that only fails when loading the before code, where it does the type validation
| + "\n" | ||
| + " File myFile = new File(\"test.txt\");\n" | ||
| + "\n" | ||
| + " s3.putObject(new PutObjectRequest(BUCKET, KEY), RequestBody.fromFile(myFile));\n" |
There was a problem hiding this comment.
Should it be builder for PutObjectRequest? What about AsyncRequestBody for async client?
There was a problem hiding this comment.
Should it be builder for PutObjectRequest?
No, this is intentional. Changing this to the builder pattern is left to a downstream recipe https://github.com/aws/aws-sdk-java-v2/blob/e59e29d7927e8bf099a9118ee287d96ec1e5c565/migration-tool/src/main/resources/META-INF/rewrite/s3-putbject-constructor-to-fluent.yml
What about AsyncRequestBody for async client?
There is no async client for v1 s3.
| @Override | ||
| public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext executionContext) { | ||
| if (PUT_OBJECT.matches(method, false)) { | ||
| method = transformPutFileOverload(method); |
There was a problem hiding this comment.
Don't we need to invoke super.visitMethodInvocation?
There was a problem hiding this comment.
We do, on 72. I don't think ordering matters here
|
|
||
| J.Identifier requestBodyId = IdentifierUtils.makeId(REQUEST_BODY.getClassName(), REQUEST_BODY); | ||
|
|
||
| JavaType.Method fromFileType = new JavaType.Method( |
There was a problem hiding this comment.
Those 9-argu ctors always look intimating to me 😨
| --- | ||
| type: specs.openrewrite.org/v1beta/recipe | ||
| name: software.amazon.awssdk.S3PutObjectConstructorToFluent | ||
| displayName: Change auth related classes |
There was a problem hiding this comment.
nit: display name is not updated
| + " }\n" | ||
| + "}\n", | ||
| "import com.amazonaws.services.s3.AmazonS3Client;\n" | ||
| + "import com.amazonaws.services.s3.model.PutObjectRequest;\n" |
There was a problem hiding this comment.
It's odd the tests are not failing. Maybe we can apply multiple recipes? That's what I did for other tests https://github.com/aws/aws-sdk-java-v2/blob/feature/master/migration-tool/migration-tool/src/test/java/software/amazon/awssdk/migration/recipe/ChangeConfigTypesTest.java#L42
This transforms the AmazonS3#putObject(String,String,File) override.
Motivation and Context
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License