Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 24, 2025

Description

Generates PutBucketAccelerateConfiguration

Motivation and Context

Testing

DRY_RUN Build id: 207f32c4-32c7-4ed1-bb1e-fea3028bf563 succeeded
FUZZ tests ran no change

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

stack-info: PR: #4154, branch: peterrsongg/petesong/phase-3-pr4-3/1
stack-info: PR: #4155, branch: peterrsongg/petesong/phase-3-pr4-3/2
stack-info: PR: #4156, branch: peterrsongg/petesong/phase-3-pr4-3/3
stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4
stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr4-3/4 branch from ff0a3d1 to b795c6c Compare November 24, 2025 06:10
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr4-3/5 branch from d115b08 to 2698874 Compare November 24, 2025 06:10
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Nov 24, 2025

READ THIS

I reviewed all the breaking changes it called out below and none of them are actually breaking

Breaking Changes Analysis for PutBucketAccelerateConfiguration Migration

FILES ANALYZED: 9 files total

File 1: sdk/src/Services/S3/Generated/Model/PutBucketAccelerateConfigurationRequest.cs

Status: NO ISSUES

  • IsSetBucketName() correctly uses != null check (same as custom)
  • IsSetExpectedBucketOwner() correctly preserves !String.IsNullOrEmpty() due to customization in s3.customizations.json
  • No breaking changes detected

File 2: sdk/src/Services/S3/Generated/Model/PutBucketAccelerateConfigurationResponse.cs

Status: NO ISSUES

  • Response class is empty in both versions
  • NOTE: Custom file had typo in filename: PutBucketAccelerateConfiguarationResponse.cs (missing 'n' in Configuration)
  • Generated file has correct spelling: PutBucketAccelerateConfigurationResponse.cs
  • This is technically a fix, not a breaking change

File 3: sdk/src/Services/S3/Generated/Model/AccelerateConfiguration.cs

Status: CRITICAL BREAKING CHANGE

BREAKING CHANGE 1: IsSet method name changed (CRITICAL)

  • Custom: internal bool IsSetBucketAccelerateStatus()
  • Generated: internal bool IsSetStatus()
  • Impact: This is a BREAKING CHANGE per rule 2. The method name has changed. Any code that calls IsSetBucketAccelerateStatus() will break at compile time. The marshaller relies on this method.

File 4: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/PutBucketAccelerateConfigurationRequestMarshaller.cs

Status: ISSUES FOUND

BREAKING CHANGE 2: IsSet method call changed

  • Custom: if (accelerateConfiguration.IsSetBucketAccelerateStatus() && accelerateConfigurationStatus != null)
  • Generated: if(publicRequest.AccelerateConfiguration.IsSetStatus())
  • Impact: Direct consequence of Breaking Change 1. The marshaller now calls the renamed method.

CHANGE 3: Header addition method changed

  • Custom: Uses S3Constants.AmzHeaderSdkChecksumAlgorithm and S3Transforms.ToStringValue()
  • Generated: Direct assignment: request.Headers["x-amz-sdk-checksum-algorithm"] = publicRequest.ChecksumAlgorithm;
  • Impact: Bypasses S3Transforms.ToStringValue() transformation. May cause issues if transformation logic is needed.

CHANGE 4: Header addition for ExpectedBucketOwner

  • Custom: Uses S3Constants.AmzHeaderExpectedBucketOwner and S3Transforms.ToStringValue()
  • Generated: Direct assignment: request.Headers["x-amz-expected-bucket-owner"] = publicRequest.ExpectedBucketOwner;
  • Impact: Same as 3 - bypasses transformation logic.

CHANGE 5: XML namespace handling

  • Custom: Uses S3Constants.S3RequestXmlNamespace
  • Generated: Hard-coded: "http://s3.amazonaws.com/doc/2006-03-01/"
  • Impact: Loses abstraction through constant, but functionally equivalent if constant value matches.

CHANGE 6: Status serialization

  • Custom: Uses S3Transforms.ToXmlStringValue(accelerateConfiguration.Status)
  • Generated: Uses StringUtils.FromString(publicRequest.AccelerateConfiguration.Status)
  • Impact: Different transformation method. Need to verify both produce same output.

CHANGE 7: Checksum header name in ChecksumUtils

  • Custom: Uses S3Constants.AmzHeaderSdkChecksumAlgorithm parameter
  • Generated: Uses hard-coded "x-amz-sdk-checksum-algorithm" parameter
  • Impact: Same as other constant issues - loses abstraction.

CHANGE 8: Marshalling order changed

  • Custom: AddSubResource called after header setup
  • Generated: AddSubResource called immediately after HttpMethod
  • Impact: Order of operations changed, could affect custom middleware.

CHANGE 9: Null check removed

  • Custom: if (accelerateConfiguration.IsSetBucketAccelerateStatus() && accelerateConfigurationStatus != null)
  • Generated: if(publicRequest.AccelerateConfiguration.IsSetStatus())
  • Impact: The additional accelerateConfigurationStatus != null check was removed. This was redundant defensive coding.

CHANGE 10: XML encoding and content handling

  • Custom: Uses XMLEncodedStringWriter(System.Globalization.CultureInfo.InvariantCulture)
  • Generated: Uses XMLEncodedStringWriter(CultureInfo.InvariantCulture)
  • Impact: Functionally the same (using statement).

CHANGE 11: Partial methods added

  • Generated: Adds PreMarshallCustomization and PostMarshallCustomization partial methods
  • Impact: No custom implementations found. Changes execution flow slightly but should be transparent.

File 5: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/PutBucketAccelerateConfigurationResponseUnmarshaller.cs

Status: NO ISSUES

  • Response unmarshaller is functionally equivalent (empty response)
  • Generated version adds PostUnmarshallCustomization partial method (no implementation found)

File 6: generator/ServiceClientGeneratorLib/ServiceModel.cs

Status: NO ISSUES

  • Uncommented the operation to enable code generation

File 7: generator/ServiceModels/s3/s3.customizations.json

Status: NO ISSUES

  • Correctly adds injectXmlIsSet for ExpectedBucketOwner
  • Customization is properly configured

SUMMARY

Total Files Changed: 9 Files Analyzed: 9/9 (100%) Files With Issues: 2 Total Breaking Changes Found: 2 (+ 9 non-breaking changes)

CRITICAL BREAKING CHANGES:

  1. AccelerateConfiguration.IsSetBucketAccelerateStatus() method renamed to IsSetStatus() (Rule 2 violation)

    • This is a PUBLIC API breaking change
    • The method is marked internal so it's not public API, but it's used by the marshaller
    • Impact: Marshaller now calls different method name
  2. PutBucketAccelerateConfigurationRequestMarshaller calls renamed IsSet method

    • Direct consequence of change 1
    • Impact: If any custom code checks IsSetBucketAccelerateStatus(), it will fail to compile

NON-BREAKING CHANGES:

  • Header additions bypass S3Transforms/S3Constants (changes 3, 4, 5, 7)
  • Status serialization uses different method (6)
  • Marshalling order changed (8)
  • Redundant null check removed (9)
  • XML encoding using statement simplified (10)
  • Partial methods added (11)

ASSESSMENT: The primary breaking change is the renamed IsSetBucketAccelerateStatus() method to IsSetStatus() in the AccelerateConfiguration class. This breaks backward compatibility at the API level, though the method is marked internal. If any custom code directly uses AccelerateConfiguration and calls this method, it will break.

RECOMMENDATION: Consider preserving the original method name IsSetBucketAccelerateStatus() or add it as an alias to maintain backward compatibility. Alternatively, if this is acceptable as a breaking change (since it's internal), ensure comprehensive testing of all marshal/unmarshal operations.

peterrsongg added a commit that referenced this pull request Nov 24, 2025
stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5
@peterrsongg peterrsongg marked this pull request as ready for review November 24, 2025 09:07
peterrsongg added a commit that referenced this pull request Nov 25, 2025
stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5
peterrsongg added a commit that referenced this pull request Nov 26, 2025
stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr4-3/4 branch from b795c6c to 5d0f259 Compare November 26, 2025 20:35
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr4-3/4 to petesong/phase-3-pr4-base November 26, 2025 20:39
@peterrsongg peterrsongg merged commit 061f718 into petesong/phase-3-pr4-base Nov 26, 2025
1 check passed
peterrsongg added a commit that referenced this pull request Nov 26, 2025
* Generate PutBucketInventoryConfiguration

stack-info: PR: #4154, branch: peterrsongg/petesong/phase-3-pr4-3/1

* Generate GetBucketInventoryConfiguration

stack-info: PR: #4155, branch: peterrsongg/petesong/phase-3-pr4-3/2

* Generate ListBucketInventoryConfigurations

stack-info: PR: #4156, branch: peterrsongg/petesong/phase-3-pr4-3/3

* Geneate DeleteBucketInventoryConfiguration

stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4

* Generate PutBucketAcclerateConfiguration

stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5

* Generate PutBucketRequestPayment

stack-info: PR: #4159, branch: peterrsongg/petesong/phase-3-pr4-3/6
peterrsongg added a commit that referenced this pull request Nov 26, 2025
* Geneate DeleteBucketInventoryConfiguration

stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4

* Generate PutBucketAcclerateConfiguration

stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5

* Generate PutBucketRequestPayment

stack-info: PR: #4159, branch: peterrsongg/petesong/phase-3-pr4-3/6

* Generate CreateBucketMetadataTableConfiguration

stack-info: PR: #4160, branch: peterrsongg/petesong/phase-3-pr4-3/7
peterrsongg added a commit that referenced this pull request Nov 26, 2025
* Geneate DeleteBucketInventoryConfiguration

stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4

* Generate PutBucketAcclerateConfiguration

stack-info: PR: #4158, branch: peterrsongg/petesong/phase-3-pr4-3/5

* Generate PutBucketRequestPayment

stack-info: PR: #4159, branch: peterrsongg/petesong/phase-3-pr4-3/6

* Generate CreateBucketMetadataTableConfiguration

stack-info: PR: #4160, branch: peterrsongg/petesong/phase-3-pr4-3/7

* Generate GetBucketMetadataConfiguration

stack-info: PR: #4161, branch: peterrsongg/petesong/phase-3-pr4-3/8
@dscpinheiro dscpinheiro deleted the peterrsongg/petesong/phase-3-pr4-3/5 branch November 27, 2025 12:45
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