-
Notifications
You must be signed in to change notification settings - Fork 874
Generate PutBucketOwnershipControls S3 operation #4202
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
base: muhamoth/phase-3-prs5-base
Are you sure you want to change the base?
Generate PutBucketOwnershipControls S3 operation #4202
Conversation
|
No breaking changes were found Breaking Change Analysis for Commit 597d355After thorough analysis of the PutBucketOwnershipControls operation migration from custom to generated code, I have identified the following: Files Analyzed: 9/9 filesFiles Changed:
✅ NO BREAKING CHANGES DETECTEDAnalysis Details:PutBucketOwnershipControlsRequest.cs:
PutBucketOwnershipControlsRequestMarshaller.cs:
PutBucketOwnershipControlsResponseUnmarshaller.cs:
PutBucketOwnershipControlsResponse.cs:
Key Preservation Mechanisms:
Conclusion:The migration successfully moved PutBucketOwnershipControls from custom to generated code without introducing breaking changes. The customization configuration file correctly addresses the potential breaking change for the ExpectedBucketOwner IsSet method, and all other functionality is preserved. Files Analyzed: 9 out of 9 total files changed |
597d355 to
92e8bd1
Compare
| { | ||
| xmlWriter.WriteStartElement("OwnershipControls", "http://s3.amazonaws.com/doc/2006-03-01/"); | ||
| var publicRequestOwnershipControlsRules = publicRequest.OwnershipControls.Rules; | ||
| if (publicRequestOwnershipControlsRules != null && (publicRequestOwnershipControlsRules.Count > 0 || !AWSConfigs.InitializeCollections)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just calling this out but the old code only checked if ownershipControls.Rules != null
https://github.com/aws/aws-sdk-net/blob/main/sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketOwnershipControlsRequestMarshaller.cs#L70C1-L71C1
and now we're checking (publicRequestOwnershipControlsRules.Count > 0 || !AWSConfigs.InitializeCollections). This could be a breaking change if someone sends an empty list. We should validate that the behavior is the same. Does
mean the same as not sending a rule at all?
S3 is strange, sometimes they can mean different things. Tagging for example, if you send an empty tagset that in a PUT request, that is the equivalent of deleting all tags. We should make sure what the behavior is before doing this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the current code will produce the same XML as the old code for nulls and empty lists.
In the old code
if (ownershipControls.Rules != null)
{
foreach (var rule in ownershipControls.Rules)
{
xmlWriter.WriteStartElement("Rule");
if (rule.IsSetObjectOwnership())
{
xmlWriter.WriteElementString("ObjectOwnership", S3Transforms.ToXmlStringValue(rule.ObjectOwnership));
}
xmlWriter.WriteEndElement();
}
}
nothing will be added to the XML if ownershipControls.Rules is null or an empty as the foreach wont run for empty lists.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay good point. never mind then :)
peterrsongg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just one comment that could be a silent breaking change
Description
Generates
PutBucketOwnershipControlsoperationContentMD5property was added to the request.Motivation and Context
DOTNET-8422Testing
DRY_RUN-68d245f4-de50-4bde-b7b5-25367b3daa55.Screenshots (if appropriate)
Types of changes
Checklist
License