S3 Generate RenameObject operation #4198
Conversation
|
Same breaking changes mentioned above Breaking Changes Analysis for RenameObject Operation Migration (Commit b90e83b3)Files Analyzed: 10 out of 10 files
CRITICAL BREAKING CHANGES FOUND1. DateTime Property Type Changes - BREAKING CHANGEFile: Issue: Four DateTime properties changed from non-nullable Affected Properties:
OLD Behavior (Custom): public DateTime DestinationIfModifiedSince
{
get { return this.destinationIfModifiedSince.GetValueOrDefault(); }
set { this.destinationIfModifiedSince = value; }
}
NEW Behavior (Generated): public DateTime? DestinationIfModifiedSince
{
get { return this._destinationIfModifiedSince; }
set { this._destinationIfModifiedSince = value; }
}
Impact:
Same issue applies to:
2. Validation Order Change - BREAKING CHANGEFile: Issue: The order in which required parameters are validated has changed, which will cause different exceptions to be thrown first. OLD Validation Order (Custom): // Line 88-91 in old marshaller
if (!renameObjectRequest.IsSetBucketName())
throw new ArgumentException("BucketName is a required property...");
if (!renameObjectRequest.IsSetKey())
throw new ArgumentException("Key is a required property...");
if (!renameObjectRequest.IsSetRenameSource())
throw new ArgumentException("RenameSource is a required property...");NEW Validation Order (Split): // Custom PreMarshallCustomization (called FIRST):
if (!publicRequest.IsSetRenameSource())
throw new ArgumentException("RenameSource is a required property...");
// Then Generated Marshall method:
if (string.IsNullOrEmpty(publicRequest.BucketName))
throw new ArgumentException("BucketName is a required property...");
if (string.IsNullOrEmpty(publicRequest.Key))
throw new ArgumentException("Key is a required property...");Impact:
SUMMARYTotal Breaking Changes: 2 critical issues
Recommendations:
Files Requiring Fixes:
|
sdk/src/Services/S3/generator/.DevConfigs/3582a2a8-4d11-4902-9d69-ba78554b7a2c.json
Outdated
Show resolved
Hide resolved
| request.AddSubResource("renameObject"); | ||
|
|
||
| if (renameObjectRequest.IsSetClientToken()) | ||
| if (!publicRequest.IsSetRenameSource()) |
There was a problem hiding this comment.
Maybe a question for @peterrsongg: In your PRs these usually were generated as well. Why do they have to be hand-written here?
There was a problem hiding this comment.
yeah i can answer that. this is because RenameSource is a required header, and currently we only validate that required querystring members and resource path members are set in the generator
There was a problem hiding this comment.
We don't validate all the properties, just RequestUriMembers and RenameSource is added to the header so we don't validate it as part of the generated code.
https://github.com/aws/aws-sdk-net/blob/muhamoth/S3-generate-RenameObject-operation/generator/ServiceClientGeneratorLib/Generators/Marshallers/BaseMarshaller.tt#L135
muhammad-othman
force-pushed
the
muhamoth/S3-generate-RenameObject-operation
branch
from
58116f5 to
3391b8c
Compare
| [SuppressMessage("Microsoft.Globalization", "CA1303:Do not pass literals as localized parameters")] | ||
| public class RenameObjectRequestMarshaller : IMarshaller<IRequest, RenameObjectRequest>, IMarshaller<IRequest, AmazonWebServiceRequest> | ||
| /// </summary> | ||
| public partial class RenameObjectRequestMarshaller : IMarshaller<IRequest, RenameObjectRequest>, IMarshaller<IRequest, AmazonWebServiceRequest> |
There was a problem hiding this comment.
THis is a minor difference and i don't know why anyone would do this but the original code also checked the actual request wasn't null. Can we add that to the preMarshallCustomization too?🫠
| request.AddSubResource("renameObject"); | ||
|
|
||
| if (renameObjectRequest.IsSetClientToken()) | ||
| if (!publicRequest.IsSetRenameSource()) |
There was a problem hiding this comment.
yeah i can answer that. this is because RenameSource is a required header, and currently we only validate that required querystring members and resource path members are set in the generator
| request.HttpMethod = "PUT"; | ||
| request.AddSubResource("renameObject"); | ||
|
|
||
| if (publicRequest.IsSetClientToken()) |
There was a problem hiding this comment.
Instead of doing this and then relying on preMarshallCustomization as well, can we instead just preserve the if else logic in PreMarshallCustomization and add a customization.json entry? that way the logic for the same member isn't split between two files
"excludeFromMarshalling":[
"ClientToken"
]
Then inside PreMarshallCustomization:
if (renameObjectRequest.IsSetClientToken())
{
request.Headers.Add(HeaderKeys.XAmzClientTokenHeader, S3Transforms.ToStringValue(renameObjectRequest.ClientToken));
}
else
{
request.Headers.Add(HeaderKeys.XAmzClientTokenHeader, Guid.NewGuid().ToString());
}
There was a problem hiding this comment.
I think we should keep most of the logic in the generated version whenever possible, though I added a comment explaining it and why the other condition can be found
muhammad-othman
force-pushed
the
muhamoth/S3-generate-RenameObject-operation
branch
from
3391b8c to
5a66ae6
Compare
muhammad-othman
deleted the
muhamoth/S3-generate-RenameObject-operation
branch
3 participants
Description
Generates
RenameObjectoperationThe following properties was changed from
DateTimetoDateTime?:We agreed that this is acceptable and will call it out in the changelog.
Another breaking change is that the order of required parameters validation has changed, which will cause different exceptions messages to be thrown for the same parameters combinations, but the exceptions are the same.
Additionally
request.UseQueryString = true;was removed, though it didn't affect the result URL since this operation uses the request headers and query string wasn't affected by this removal.Motivation and Context
DOTNET-8422Testing
DRY_RUN-967ff3a2-7405-463b-b78e-7ba84e29a500.Screenshots (if appropriate)
Types of changes
Checklist
License