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
Assembly comparison passed
Fuzz testing passed

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 peterrsongg changed the title Petesong/generate delete bucket website Generate DeleteBucketWebsite Jan 7, 2026
@peterrsongg
Copy link
Contributor Author

READ THIS

the breaking change it flagged is fine because we check !string.IsNullOrEmpty for required uri params

BREAKING CHANGES ANALYSIS FOR COMMIT 502daa2

BREAKING CHANGE FOUND

DeleteBucketWebsiteRequest.cs - BucketName IsSet Method Changed

File: sdk/src/Services/S3/Generated/Model/DeleteBucketWebsiteRequest.cs

BREAKING CHANGE: The IsSetBucketName() method changed from !string.IsNullOrEmpty to != null

BucketName Property IsSet Method

OLD Custom Version (deleted):

internal bool IsSetBucketName()
{
    return !string.IsNullOrEmpty(this.bucketName);
}

NEW Generated Version:

internal bool IsSetBucketName()
{
    return this._bucketName != null;
}

Impact:

  • BEFORE: Empty string "" would return false from IsSetBucketName(), treating it as "not set"
  • AFTER: Empty string "" would return true from IsSetBucketName(), treating it as "set"
  • This is a REQUEST object, and per requirements: "For request objects, with string properties, check to see if the IsSet methods have changed from !string.IsNullOREmpty to != null and call that out as a breaking change."
  • This changes request validation behavior - the marshaller will now proceed with an empty bucket name instead of treating it as unset

Customer Impact:

  • If a customer previously set BucketName to an empty string, the OLD behavior would treat it as not set
  • NEW behavior treats empty string as set, which could lead to different validation or API behavior
  • The marshaller checks if (string.IsNullOrEmpty(publicRequest.BucketName)) and throws an exception, but the IsSet method change could affect other code paths that check IsSetBucketName() before accessing the property

FILES ANALYZED: 9 of 9

Files WITHOUT Issues - ✅ OK:

  1. ServiceModel.cs - ✅ Added DeleteBucketWebsite to allow list (generator infrastructure)

  2. s3.customizations.json - ✅ Added customization for ExpectedBucketOwner to preserve !String.IsNullOrEmpty

  3. DeleteBucketWebsiteRequest.cs - ❌ 1 BREAKING CHANGE (BucketName IsSet method)

    • ExpectedBucketOwner: IsSet correctly preserved as !String.IsNullOrEmpty via customization ✅
    • BucketName: IsSet changed from !string.IsNullOrEmpty to != null ❌ BREAKING CHANGE
  4. DeleteBucketWebsiteResponse.cs - ✅ OK

    • Empty response class (void operation)
    • Moved from Custom to Generated
  5. DeleteBucketWebsiteRequestMarshaller.cs (deleted Custom) - ✅ Logic preserved in new Generated

  6. DeleteBucketWebsiteRequestMarshaller.cs (new Generated) - ✅ OK

    • Proper marshalling logic
    • HTTP method: DELETE
    • Adds "website" subresource
    • Checks for ExpectedBucketOwner header
    • Validates BucketName is not empty
    • Partial methods for customization provided
  7. DeleteBucketWebsiteResponseUnmarshaller.cs (deleted Custom) - ✅ Logic preserved in new Generated

  8. DeleteBucketWebsiteResponseUnmarshaller.cs (new Generated) - ✅ OK

    • Proper unmarshaller implementation
    • Partial method for customization provided

SUMMARY

Total Files Changed: 9
Files Analyzed: 9
Breaking Changes Found: 1
Files with Issues: 1 (DeleteBucketWebsiteRequest.cs)

Root Cause:

The customization in s3.customizations.json only addressed the ExpectedBucketOwner property but failed to preserve the !string.IsNullOrEmpty pattern for the BucketName property.

Required Fix:

The customization should include:

"DeleteBucketWebsiteRequest": {
    "modify": [
        {
            "BucketName": {"injectXmlIsSet": ["return !String.IsNullOrEmpty(this._bucketName);"]}
        },
        {
            "ExpectedBucketOwner": {"injectXmlIsSet": ["return !String.IsNullOrEmpty(this._expectedBucketOwner);"]}
        }
    ]
}

@peterrsongg peterrsongg merged commit ba2bb08 into petesong/generate-put-bucket-website Jan 9, 2026
6 checks passed
peterrsongg added a commit that referenced this pull request Jan 9, 2026
* 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 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