Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 24, 2025

Description

Generates DeleteBucketInventoryConfiguration

Motivation and Context

Testing

DRY_RUN Build id: 207f32c4-32c7-4ed1-bb1e-fea3028bf563 succeeded
FUZZ_TESTS run. Nothing changed

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 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/3 branch from 028eb81 to 524c8be Compare November 24, 2025 06:10
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Nov 24, 2025

Read this!!

I've reviewed all the "breaking changes" it called out and none of them are breaking changes.

IsSetBucketName() and IsSetInventoryId aren't breaking changes because we check String.IsNullOrEmpty when we check the required query string parameters.

Also adding to the parameters instead of adding the subresource isn't a breaking change as discussed in previous PRs


AI prompt output
BREAKING CHANGES:

  1. IsSetBucketName() method changed (CRITICAL)

    • Custom: return !(string.IsNullOrEmpty(this.bucketName));
    • Generated: return this._bucketName != null;
    • Impact: This is a breaking change per rule Adds ability to sort backwards when using DynamoDBContext. #5. The method now returns true for empty strings (""), whereas previously it returned false. This could cause requests with empty BucketName strings to be treated as "set" when they should not be.
  2. IsSetInventoryId() method changed (CRITICAL)

    • Custom: return !(string.IsNullOrEmpty(this.inventoryId));
    • Generated: return this._inventoryId != null;
    • Impact: This is a breaking change per rule Adds ability to sort backwards when using DynamoDBContext. #5. The method now returns true for empty strings (""), whereas previously it returned false. This could cause requests with empty InventoryId strings to be treated as "set" when they should not be.

NOTE: ExpectedBucketOwner correctly preserves the !String.IsNullOrEmpty() check due to customization in s3.customizations.json.


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

Status: NO ISSUES

  • Response class is identical between custom and generated versions (empty response object)

File 3: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/DeleteBucketInventoryConfigurationRequestMarshaller.cs

Status: ISSUES FOUND

BREAKING CHANGES:

  1. AddSubResource order changed

    • Custom: request.AddSubResource("inventory") called after header setup
    • Generated: request.AddSubResource("inventory") called immediately after HttpMethod set
    • Impact: While this may not cause functional issues, the order of operations has changed which could affect custom middleware or request inspection logic.
  2. Header addition method changed

    • Custom: request.Headers.Add(S3Constants.AmzHeaderExpectedBucketOwner, S3Transforms.ToStringValue(deleteInventoryConfigurationRequest.ExpectedBucketOwner));
    • Generated: request.Headers["x-amz-expected-bucket-owner"] = publicRequest.ExpectedBucketOwner;
    • Impact: Changed from using constant and Transform method to hard-coded header name and direct assignment. This bypasses the S3Transforms.ToStringValue() transformation.
  3. InventoryId validation added (NEW LOGIC)

    • Custom: No validation - directly uses request.AddSubResource("id", deleteInventoryConfigurationRequest.InventoryId);
    • Generated: Adds validation if (string.IsNullOrEmpty(publicRequest.InventoryId)) throw new AmazonS3Exception("Request object does not have required field InventoryId set");
    • Impact: NEW behavior that throws exception for null/empty InventoryId. This is more strict than the previous implementation.
  4. InventoryId parameter addition logic changed

    • Custom: request.AddSubResource("id", deleteInventoryConfigurationRequest.InventoryId); (unconditional)
    • Generated: if (publicRequest.IsSetInventoryId()) request.Parameters.Add("id", StringUtils.FromString(publicRequest.InventoryId)); (conditional with IsSet check)
    • Impact: Combined with issue TVMClient Raw #2 (IsSetInventoryId now uses != null), this means empty strings will now be added as parameters when they wouldn't have been before.
  5. Partial methods added

    • Generated: Adds PreMarshallCustomization and PostMarshallCustomization partial methods
    • Impact: No custom implementations found in Custom folder, so these are no-ops. However, this changes the execution flow slightly.

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

Status: NO ISSUES

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

File 5: generator/ServiceClientGeneratorLib/ServiceModel.cs

Status: NO ISSUES

  • Uncommented the operation to enable code generation

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

Status: CONFIGURATION ISSUE

MISSING CUSTOMIZATION:

  1. InventoryId IsSet customization missing

    • The customizations.json correctly adds "injectXmlIsSet" for ExpectedBucketOwner
    • However, it does NOT add "injectXmlIsSet" for the Id/InventoryId property
    • Impact: This causes InventoryId to use the default != null check instead of !String.IsNullOrEmpty(), which is the root cause of breaking change TVMClient Raw #2.

REQUIRED FIX:

"DeleteBucketInventoryConfigurationRequest":{
    "modify":[
        {
            "Id":{"emitPropertyName":"InventoryId"}
        },
        {
            "InventoryId":{"injectXmlIsSet":["return !String.IsNullOrEmpty(this._inventoryId);"]}  // ADD THIS
        },
        {
            "ExpectedBucketOwner":{"injectXmlIsSet":["return !String.IsNullOrEmpty(this._expectedBucketOwner);"]}
        }
    ]
}

SUMMARY

Total Files Changed: 8 Files Analyzed: 8/8 (100%) Files With Issues: 3 Total Breaking Changes Found: 8

Critical Breaking Changes:

  1. IsSetBucketName() changed from !string.IsNullOrEmpty to != null (Rule Adds ability to sort backwards when using DynamoDBContext. #5 violation)
  2. IsSetInventoryId() changed from !string.IsNullOrEmpty to != null (Rule Adds ability to sort backwards when using DynamoDBContext. #5 violation)
  3. Missing customization for BucketName IsSet in s3.customizations.json
  4. Header addition logic changed (bypasses S3Transforms.ToStringValue)
  5. New validation logic throws exceptions for null/empty InventoryId
  6. InventoryId parameter addition changed from unconditional to conditional with flawed IsSet check
  7. Request marshalling order changed
  8. Missing InventoryId IsSet customization in s3.customizations.json

RECOMMENDATION: The s3.customizations.json needs to be updated to include IsSet customizations for BOTH BucketName and InventoryId properties to preserve backward compatibility.

peterrsongg added a commit that referenced this pull request Nov 24, 2025
stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4
@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: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4
stack-info: PR: #4157, branch: peterrsongg/petesong/phase-3-pr4-3/4
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr4-3/3 to petesong/phase-3-pr4-base November 26, 2025 20:29
@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 merged commit 0384a3b 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
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/4 branch November 27, 2025 12:45
peterrsongg added a commit that referenced this pull request Dec 5, 2025
…onfiguration. Generate PutBucketAcclerateConfiguration,CreateBucketMetadataTableConfiguration, PutBucketRequestPayment, GetBucketMetadataConfiguration

* Generate PutBucketInventoryConfiguration
* Generate GetBucketInventoryConfiguration
* Generate ListBucketInventoryConfigurations
* Geneate DeleteBucketInventoryConfiguration (#4157)
* Generate PutBucketAcclerateConfiguration (#4158)
* Generate PutBucketRequestPayment (#4159)
* Generate CreateBucketMetadataTableConfiguration (#4160)
* Generate GetBucketMetadataConfiguration (#4161)
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