Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Jan 7, 2026

Description

Motivation and Context

Testing

DRY_RUN-98a2b771-b69c-4c45-8285-4ce70f26b273
AssemblyComparison:

AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.PutBucketWebsiteRequest/MethodAdded: New method System.String get_ContentMD5() in Amazon.S3.Model.PutBucketWebsiteRequest
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.PutBucketWebsiteRequest/MethodAdded: New method System.Void set_ContentMD5(System.String) in Amazon.S3.Model.PutBucketWebsiteRequest

Fuzz Testing Results show differences but that's because what we were sending before was wrong. We were writing the wrong child elements for RedirectAllRequestsTo. Refer to https://docs.aws.amazon.com/AmazonS3/latest/API/API_PutBucketWebsite.html for proof

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

  • I confirm that this pull request can be released under the Apache 2 license

@peterrsongg
Copy link
Contributor Author

peterrsongg commented Jan 7, 2026

READ THIS

The breaking change analysis report missed something, which was that we are now writing the right XML.
Previously we were incorrectly writing additional xml elements that aren't modeled when writing "RedirectAllRequestsTo"
https://github.com/aws/aws-sdk-net/blob/main/sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketWebsiteRequestMarshaller.cs#L87-L113

However, now we are only writing elements which are modeled. This is because the shape for RedirectAllRequestsTo is shared for Redirect and RedirectAllRequestsTo, which is techincally incorrect, but must be preserved for backwards compatibility.

what we're writing now: https://github.com/aws/aws-sdk-net/pull/4274/changes#diff-3e227a55b4804766dc93cfcfe6d748e7bd3dfa3d95a578194747e817bc7e1003R90-R95

BREAKING CHANGES ANALYSIS FOR COMMIT 4169120

You're correct - I apologize for the misreading. After careful re-analysis of all 10 files in this commit:

ANALYSIS COMPLETE: ✅ NO BREAKING CHANGES DETECTED

FILES ANALYZED: 10 of 10

All files have been migrated correctly from custom to generated code for the PutBucketWebsite operation:

Generator Infrastructure (2 files) - ✅ OK

  1. ServiceModel.cs - Added PutBucketWebsite to allow list
  2. s3.customizations.json - Added proper customizations

Request Marshaller (2 files) - ✅ OK

  1. PutBucketWebsiteRequestMarshaller.cs (Custom partial) - Custom marshalling preserved:

    • ErrorDocumentCustomMarshall() - Wraps in <ErrorDocument><Key> tags
    • IndexDocumentSuffixCustomMarshall() - Wraps in <IndexDocument><Suffix> tags
  2. PutBucketWebsiteRequestMarshaller.cs (Generated) - Proper marshalling logic with calls to custom methods

Response Unmarshaller (2 files) - ✅ OK

  1. PutBucketWebsiteResponseUnmarshaller.cs (Custom - deleted) - Old custom logic removed
  2. PutBucketWebsiteResponseUnmarshaller.cs (Generated - new) - Proper implementation

Model Classes (4 files) - ✅ OK

  1. PutBucketWebsiteRequest.cs (Generated) - All properties correct:

    • BucketName: IsSet uses != null
    • ChecksumAlgorithm: IsSet uses != null
    • ContentMD5: IsSet uses != null
    • ExpectedBucketOwner: IsSet uses !String.IsNullOrEmpty (customized correctly) ✅
    • WebsiteConfiguration: IsSet uses != null
  2. PutBucketWebsiteResponse.cs - Empty response class (moved from Custom to Generated) ✅

  3. WebsiteConfiguration.cs (Custom - deleted, Generated - new):

    • ErrorDocument: Property and IsSet method properly implemented ✅
    • IndexDocumentSuffix: Property and IsSet method properly implemented ✅
    • RedirectAllRequestsTo: Property and IsSet method properly implemented ✅
    • RoutingRules: Property and IsSet method properly implemented ✅
    • All IsSet methods use appropriate checks ✅
  4. Old Custom files deleted - Properly replaced with generated equivalents ✅

SUMMARY

Total Files Changed: 10
Files Analyzed: 10
Breaking Changes Found: 0
Files with Issues: 0

The migration from custom to generated code for PutBucketWebsite has been executed correctly with full backward compatibility maintained. All custom marshalling logic has been preserved through partial methods, and all property implementations are correct.

Copy link
Member

@muhammad-othman muhammad-othman left a comment

Choose a reason for hiding this comment

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

Approved assuming that this operation was tested with and without RedirectAllRequestsTo.

@peterrsongg
Copy link
Contributor Author

Approved assuming that this operation was tested with and without RedirectAllRequestsTo.

yup, it was tested and I even validated that if I set those properties on RedirectAllRequestsTo in the current version of S3 the backend will not accept it, complaining about malformed xml. Now, that isn't possible.

* generate putbucketwebsite

* Generate DeleteBucketWebsite

* generate putbucketwebsite

* Generate GetBucketWebsite

* fix errors
@peterrsongg peterrsongg merged commit bda75d0 into petesong/generate-get-bucket-website Jan 9, 2026
1 check passed
peterrsongg added a commit that referenced this pull request Jan 9, 2026
* Generate PutBucketLogging

* Generate GetBucketWebsite

* remove todo

* Generate PutBucketWebsite (#4274)

* Generate GetBucketWebsite

* generate putbucketwebsite

* fix errors

* remove wrongly staged files

* Generate DeleteBucketWebsite (#4275)

* generate putbucketwebsite

* Generate DeleteBucketWebsite

* generate putbucketwebsite

* Generate GetBucketWebsite

* fix errors
peterrsongg added a commit that referenced this pull request Jan 9, 2026
* Generate PutBucketLogging

* Generate GetBucketWebsite

* remove todo

* Generate PutBucketWebsite (#4274)

* Generate GetBucketWebsite

* generate putbucketwebsite

* fix errors

* remove wrongly staged files

* Generate DeleteBucketWebsite (#4275)

* generate putbucketwebsite

* Generate DeleteBucketWebsite

* generate putbucketwebsite

* Generate GetBucketWebsite

* fix errors
@peterrsongg peterrsongg mentioned this pull request Jan 9, 2026
10 tasks
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.

3 participants