Simpler S3 CopyObjectRequest#copySource

[worpet]

In the original SDK, the source bucket and key vs the destination bucket and key was clear:

new CopyObjectRequest(sourceBucket, sourceKey, destinationBucket, destinationKey)

But in SDK 2, the way the Builder methods are named makes this overly confusing.

Expected Behavior

CopyObjectRequest.Builder should have methods like sourceBucket(bucket) and sourceKey(key) to clearly set the source bucket and source key.

The bucket(bucket) and key(key) methods should be documented as being for the destination values.

Current Behavior

CopyObjectRequest.Builder has only the method copySource(String copySource) which accepts the source bucket and source key, separated by a slash. More importantly, you are required to URL-encode this value, which is easy to overlook. For example, even the AWS documentation example makes the mistake of not encoding. A casual test will appear to work, but may fail later when you happen to have a key that contains an unsafe URL character.

The bucket(bucket) and key(key) methods are not documented as being for the destination values so, when coding, there is no clear way to simply set a copy source and destination without searching for more information.

[millems]

I agree the naming is less clear. This change is due to the migration from the hand-written S3 client to the generated S3 client. The SDK now matches the naming and semantics that the S3 team applied to the fields, instead of what the SDK team decided to apply to the fields.

The SDK should consider adding an S3 customization for this, because having to encode the copySource field is not a good experience. Having to specify the bucket and source key separated by a slash, in general, is not a good experience.

Marking as a feature request.

[jeffalder]

@millems There's no clear customization route that I can see.

1. The existing bucket and key methods on the builder need to be deprecated. They can't be simply renamed, and no existing customization route provides this.
2. There's no requirement that the property be renamed everywhere; the real need is simply a renamed builder method. Although -- it would be useful to rename it consistently everywhere.

I think what makes sense is to add an emitPropertyName modification with the new name, then inject a deprecated member with the old name and existing location (if possible). Later, the injected member can be removed. Is there something I'm missing? Should a new modification path be added, instead?

[jeffalder]

I was experimenting with this and I opted for adding a boolean key to the "Modify" customization; this tells codegen to produce getters, setters, and fluent builder methods using the previous name, marking them with the Deprecated annotation. The migration path for any given property rename would be to introduce the "modify" customization with "deprecate existing name" set to true; when the deprecated names can be removed, simply remove the boolean property or set to false.

[millems]

I kind of see two issues here:

1. The "bucket" and "key" are ambiguous - are they the source or the destination?
2. The "copySource" is confusing, because it requires encoding the bucket and key a certain way. And what about version?

I think these problems have two different solutions.

For (1), adding a destinationBucket/destinationKey and deprecating the old ones might be the right call. It's either that or just leave them ambiguous and hope that the other being named "source" is enough to disambiguate.

For (2), we could add sourceBucket, sourceKey and sourceVersion as convenience methods that can be combined into a copySource by an execution interceptor. We'd probably want to make those new fields mutually exclusive with copySource in that interceptor. I'm not sure if deprecating copySource is the right call in this case, because who knows what the S3 group might cook up for that string in 4 years time. Paranoia justification: we recently repurposed "bucket" to also mean "...or access point ARN".

[jeffalder]

My in-progress solution only solves (1).

Since these issues may have disjoint solutions, should we separate them?

[jeffalder]

OK, I dug into Execution Interceptors, so now I have an idea of how they work and your recommended solution for (2) makes sense to me.

Per the API docs, https://docs.aws.amazon.com/AmazonS3/latest/API/API_CopyObject.html#AmazonS3-CopyObject-request-header-CopySource

CopySource should be / + URL-encoded bucket name + / + URL-encoded key (are / characters in the key encoded?) + optionally (/ + URL-encoded version). Is that correct?

I don't think I would deprecate copySource either, for that just being one of the issues.

Then the algo runs something like:

1. if copySource is not blank, use it.
2. If sourceBucket and sourceKey are both not blank, use /sourceBucket/sourceKey + (sourceVersion ? /sourceVersion : "");
3. otherwise ... throw an exception?

[millems]

Sounds good to me! We should make sure to test sourceKeys with / in them and validate that sourceBucket has run through the bucket validation logic in other interceptors before url-encoding. I'm not entirely sure how the source keys are encoded, but we should be able to test it.

[jeffalder]

The EndpointAddressInterceptor, to which I believe you're referring, calls into S3EndpointUtils.applyEndpointConfiguration. That explicitly only pulls the Bucket field. So if we were to have SourceBucket and SourceKey fields, all of that logic in applyEndpointConfiguration would have to apply to those fields as well. Correct? Again - just want to make sure I'm understanding what you are saying.

[jeffalder]

I have this so far for part 1: master...jeffalder:jeffalder/deprecatedRename . I need to write tests but I feel pretty confident in the approach so far.

[jeffalder]

I added a test. "DeprecatedNameResponse" has no customizations; "DeprecatedNameRequest" is the same shape with "emitPropertyName" customizations applied. You can diff the results to see what changed.

[millems]

It's looking good! I have some comments, but we can bang them out in CR.