Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 20, 2025

Description

Generate DeleteBucketIntelligentTieringConfiguration

Motivation and Context

Testing

Dry run for base branch passed
Fuzz testing passed.
AI Prompt run
Manual testing with null IntelligentTieringId yielded same exception before and after 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

  • All new and existing tests passed

License

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

stack-info: PR: #4145, branch: peterrsongg/petesong/phase-3-pr-3/1
stack-info: PR: #4146, branch: peterrsongg/petesong/phase-3-pr-3/2
stack-info: PR: #4147, branch: peterrsongg/petesong/phase-3-pr-3/3
stack-info: PR: #4148, branch: peterrsongg/petesong/phase-3-pr-3/4
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-3/3 branch from 831e331 to 43818b1 Compare November 20, 2025 17:37
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-3/4 branch from 34bdfc9 to 367749a Compare November 20, 2025 17:37
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Nov 20, 2025

AI report:

BREAKING CHANGES ANALYSIS REPORT

Files Analyzed: 4 out of 4 total files in PR


BREAKING CHANGES IDENTIFIED:

1. DeleteBucketIntelligentTieringConfigurationRequest.cs

ISSUE 1: BucketName IsSet method logic changed

  • Location: BucketName property IsSetBucketName() method

  • Custom Code (deleted):

    internal bool IsSetBucketName()
    {
        return !(string.IsNullOrEmpty(this.bucketName));
    }
  • Generated Code (added):

    internal bool IsSetBucketName()
    {
        return this._bucketName != null;
    }
  • Impact: BREAKING CHANGE - The IsSet check changed from !string.IsNullOrEmpty() to != null. This means an empty string ("") will now be considered "set" in the generated version, whereas it was considered "not set" in the custom version. This violates rule Adds ability to sort backwards when using DynamoDBContext. #5 from the requirements for request objects with string properties.

ISSUE 2: IntelligentTieringId IsSet method logic changed

  • Location: IntelligentTieringId property IsSetIntelligentTieringId() method

  • Custom Code (deleted):

    internal bool IsSetIntelligentTieiringId()
    {
        return !(string.IsNullOrEmpty(this.IntelligentTieringId));
    }
  • Generated Code (added):

    internal bool IsSetIntelligentTieringId()
    {
        return this._intelligentTieringId != null;
    }
  • Impact: BREAKING CHANGE - The IsSet check changed from !string.IsNullOrEmpty() to != null. This means an empty string ("") will now be considered "set" in the generated version, whereas it was considered "not set" in the custom version. This violates rule 5 from the requirements for request objects with string properties.


2. DeleteBucketIntelligentTieringConfigurationRequestMarshaller.cs

ISSUE 3: New validation logic throws exception

  • Location: Marshall method

  • Custom Code (deleted): No validation for IntelligentTieringId being null/empty

  • Generated Code (added):

    if (string.IsNullOrEmpty(publicRequest.IntelligentTieringId))
        throw new AmazonS3Exception("Request object does not have required field IntelligentTieringId set");
  • Impact: BREAKING CHANGE - The generated code now throws an AmazonS3Exception if IntelligentTieringId is null or empty. The custom code did not have this validation and would proceed without throwing an exception. This is new behavior that could break existing code that relied on being able to call this operation without explicitly setting the IntelligentTieringId (even though it would likely fail server-side).

ISSUE 4: Header constant changed to literal string

  • Location: ExpectedBucketOwner header setting

  • Custom Code (deleted):

    request.Headers.Add(S3Constants.AmzHeaderExpectedBucketOwner, S3Transforms.ToStringValue(deleteBucketIntelligentTieringConfigurationRequest.ExpectedBucketOwner));
  • Generated Code (added):

    request.Headers["x-amz-expected-bucket-owner"] = publicRequest.ExpectedBucketOwner;
  • Impact: POTENTIAL BREAKING CHANGE - Changed from using S3Constants.AmzHeaderExpectedBucketOwner constant and S3Transforms.ToStringValue() to using literal string "x-amz-expected-bucket-owner" and direct assignment. While the header name should be the same, the transformation method change (S3Transforms.ToStringValue() vs direct assignment) could behave differently for edge cases (null handling, formatting, etc.).

ISSUE 5: IntelligentTieringId parameter handling method changed

  • Location: Adding IntelligentTieringId to request

  • Custom Code (deleted):

    request.AddSubResource("id", deleteBucketIntelligentTieringConfigurationRequest.IntelligentTieringId);
  • Generated Code (added):

    if (publicRequest.IsSetIntelligentTieringId())
        request.Parameters.Add("id", StringUtils.FromString(publicRequest.IntelligentTieringId));
  • Impact: POTENTIAL BREAKING CHANGE - The method changed from AddSubResource() to Parameters.Add() with StringUtils.FromString(). Additionally, the generated code now checks IsSetIntelligentTieringId() before adding, while the custom code always added it. Given that Issue 3 throws an exception if it's not set, this conditional check may not matter, but the method change could affect URL generation or parameter encoding.


SUMMARY:

  • Files with Breaking Changes: 2 out of 4 files
  • Total Breaking Changes: 5
  • Critical Issues: 3 (Issues 1, 2, 3)
  • Potential Issues: 2 (Issues 4, 5)

RECOMMENDATION:

The customization configuration should be updated to preserve the original !string.IsNullOrEmpty() logic for the IsSet methods on BucketName and IntelligentTieringId properties using the injectXmlIsSet customization as shown in the s3.customizations.json for other similar cases.

@peterrsongg
Copy link
Contributor Author

The change for Id and BucketName (like the other PRs) is not a breaking change. This is because we check that these required parameters are set so the case where an empty or null string reaches the IsSet method in the marshaller never happens.

We've discussed in other PRs why going from AddSubresource to AddParameter is an okay change to make as well.

@peterrsongg peterrsongg marked this pull request as ready for review November 20, 2025 19:41
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr-3/3 to petesong/4145-4149 November 21, 2025 18:19
@peterrsongg peterrsongg merged commit 9ef9b4c into petesong/4145-4149 Nov 21, 2025
1 check passed
peterrsongg added a commit that referenced this pull request Nov 21, 2025
* generate PutBucketIntelligentTieringConfiguration

stack-info: PR: #4145, branch: peterrsongg/petesong/phase-3-pr-3/1

* Generate GetBucketIntelligentTieringConfiguration

stack-info: PR: #4146, branch: peterrsongg/petesong/phase-3-pr-3/2

* Generate ListBucketIntelligentTieringConfiguration

stack-info: PR: #4147, branch: peterrsongg/petesong/phase-3-pr-3/3

* Generate DeleteBucketIntelligentTiering

stack-info: PR: #4148, branch: peterrsongg/petesong/phase-3-pr-3/4

* add shapes to S3NeedsCustomUpdate and add devconfig

stack-info: PR: #4149, branch: peterrsongg/petesong/phase-3-pr-3/5
@peterrsongg peterrsongg deleted the peterrsongg/petesong/phase-3-pr-3/4 branch November 21, 2025 18:21
@peterrsongg peterrsongg mentioned this pull request Nov 21, 2025
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