Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 20, 2025

Description

Generate GetBucketIntelligentTieringConfiguration

Motivation and Context

Testing

Dry run for base branch passed
Fuzz testing passed.
AI Prompt run

NOTE: doing IntelligentTiering validation changed the behavior. Previously, not setting this would call ListBucketIntelligentTieringConfiguration. Will call this out in the changelog.

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
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-3/2 branch from 798da71 to f8ef622 Compare November 20, 2025 17:37
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr-3/1 branch from adf364e to 17ee8cf Compare November 20, 2025 17:37
@peterrsongg
Copy link
Contributor Author

GetBucketInteligentTiering AI report

Breaking Changes Analysis Report

I have analyzed all 14 files affected in this PR migration from custom to generated S3 code. Here are the findings:

❌ BREAKING CHANGE #1: GetBucketIntelligentTieringConfigurationRequest.cs

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

Issue: IsSet method changed for string properties

Custom (deleted) code:

internal bool IsSetIntelligentTieringId()
{
    return !string.IsNullOrEmpty(this.intelligentTieringId);
}

Generated code:

internal bool IsSetIntelligentTieringId()
{
    return this._intelligentTieringId != null;
}

Impact: Per instruction #5, for request objects with string properties, changing IsSet from !string.IsNullOrEmpty to != null is a BREAKING CHANGE. This means an empty string ("") will now be considered "set" when previously it was not.


⚠️ POTENTIAL ISSUE #1: GetBucketIntelligentTieringConfigurationRequestMarshaller.cs

Files:

  • Custom: sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/GetBucketIntelligentTieringConfigurationRequestMarshaller.cs
  • Generated: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/GetBucketIntelligentTieringConfigurationRequestMarshaller.cs

Issue: Different methods used to add the id query parameter

Custom (deleted) code:

request.AddSubResource("id", getBucketIntelligentTieringConfigurationRequest.IntelligentTieringId);

Generated code:

if (publicRequest.IsSetIntelligentTieringId())
    request.Parameters.Add("id", StringUtils.FromString(publicRequest.IntelligentTieringId));

Analysis: The custom code used AddSubResource() while generated uses request.Parameters.Add(). Both should add query parameters, but:

  1. Generated code adds conditional check with IsSetIntelligentTieringId()
  2. Generated code explicitly converts via StringUtils.FromString()

Recommendation: Verify that AddSubResource and request.Parameters.Add produce identical query string behavior for the id parameter.


⚠️ POTENTIAL ISSUE #2: TieringUnmarshaller.cs

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

Issue: Different unmarshaller used for Days property

Custom (deleted) code:

tiering.Days = IntUnmarshaller.GetInstance().Unmarshall(context);

Generated code:

var unmarshaller = NullableIntUnmarshaller.Instance;
unmarshalledObject.Days = unmarshaller.Unmarshall(context);

Analysis: The Tiering model's Days property is defined as int? (nullable int). The custom code used IntUnmarshaller which returns non-nullable int, while the generated correctly uses NullableIntUnmarshaller for nullable int. This is likely a BUG FIX in the generated code rather than a breaking change, as the custom implementation was technically incorrect for a nullable property. However, this changes unmarshalling behavior.


✅ NO ISSUES FOUND:

The following files have no breaking changes:

  1. GetBucketIntelligentTieringConfigurationResponse.cs - IsSet methods use != null for object properties (responses exempt per instructions)
  2. GetBucketIntelligentTieringConfigurationResponseUnmarshaller.cs - Functionally equivalent logic
  3. IntelligentTieringConfigurationUnmarshaller.cs - Functionally equivalent logic
  4. IntelligentTieringConfiguration.cs (model) - Properties are compatible
  5. Tiering.cs (model) - Properties are compatible with nullable Days

Summary

Files Analyzed: 14 out of 14 (100%)

Confirmed Breaking Changes: 1

  • IsSet method change for string property in request object

Potential Issues Requiring Verification: 2

  • Different parameter addition methods in marshaller
  • Different unmarshaller type for Days property (likely a fix, not a break)

Files With No Issues: 11

Critical Action Required: Fix the IsSet method for IntelligentTieringId in the generated request class to use !string.IsNullOrEmpty instead of != null to maintain backwards compatibility.

}

// Check to see if IntelligentTieringId property is set
internal bool IsSetIntelligentTieringId()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not changing this to String.ISNullOREmpty because we check this in the marshaller and will throw an exception if the string is null or empty

{
tiering.AccessTier = StringUnmarshaller.GetInstance().Unmarshall(context);

var unmarshaller = NullableIntUnmarshaller.Instance;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is fine because Days must be specified for the AccessTier when you put the intelligenttiering configuration, meaning that if the AccessTier xmlElement is present on the Get side then there will always be a non-empty Days xml element

@peterrsongg peterrsongg marked this pull request as ready for review November 20, 2025 19:24
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr-3/1 to petesong/4145-4149 November 21, 2025 18:04
@peterrsongg peterrsongg changed the base branch from petesong/4145-4149 to peterrsongg/petesong/phase-3-pr-3/1 November 21, 2025 18:04
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr-3/1 to petesong/4145-4149 November 21, 2025 18:17
@peterrsongg peterrsongg merged commit 398dd1a 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
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
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/2 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