Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Nov 20, 2025

Description

Generate PutBucketIntelligentTieringConfiguration

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
@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

peterrsongg commented Nov 20, 2025

AI Prompt:
Everything it called out as breaking changes are not breaking changes. I'll explain in comments of the FilesChanged

COMPREHENSIVE BREAKING CHANGES ANALYSIS REPORT

TOTAL FILES ANALYZED: 14 of 14 files (Complete)

Files in Scope:

  1. ✅ generator/ServiceClientGeneratorLib/ServiceModel.cs (Modified)
  2. ✅ generator/ServiceModels/s3/s3.customizations.json (Modified)
  3. ✅ sdk/src/Services/S3/Custom/Model/IntelligentTieringConfiguration.cs (Deleted → Generated)
  4. ✅ sdk/src/Services/S3/Custom/Model/PutBucketIntelligentTieringConfigurationRequest.cs (Deleted → Generated)
  5. ✅ sdk/src/Services/S3/Custom/Model/PutBucketIntelligentTieringConfigurationResponse.cs (Deleted → Generated)
  6. ✅ sdk/src/Services/S3/Custom/Model/Tiering.cs (Deleted → Generated)
  7. ✅ sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketIntelligentTieringConfigurationRequestMarshaller.cs (Deleted → Generated + Custom Partial)
  8. ✅ sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketIntelligentTieringConfigurationResponseUnmarshaller.cs (Deleted → Generated)
  9. ✅ sdk/src/Services/S3/Generated/Model/IntelligentTieringConfiguration.cs (New)
  10. ✅ sdk/src/Services/S3/Generated/Model/PutBucketIntelligentTieringConfigurationRequest.cs (New)
  11. ✅ sdk/src/Services/S3/Generated/Model/PutBucketIntelligentTieringConfigurationResponse.cs (New)
  12. ✅ sdk/src/Services/S3/Generated/Model/Tiering.cs (New)
  13. ✅ sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/PutBucketIntelligentTieringConfigurationRequestMarshaller.cs (New)
  14. ✅ sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/PutBucketIntelligentTieringConfigurationResponseUnmarshaller.cs (New)

BREAKING CHANGE #1: IsSet Method Logic Changed for String Properties (REQUEST OBJECT)

Severity: CRITICAL
Files:

  • IntelligentTieringConfiguration.cs - IsSetIntelligentTieringId() method
  • PutBucketIntelligentTieringConfigurationRequest.cs - IsSetIntelligentTieringId(), IsSetBucketName(), IsSetExpectedBucketOwner() methods

Old (Custom - IntelligentTieringConfiguration):

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

New (Generated - IntelligentTieringConfiguration):

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

Old (Custom - PutBucketIntelligentTieringConfigurationRequest):

// Check to see if BucketName property is set
internal bool IsSetBucketName()
{
    return this.bucketName != null;
}

// Check to see if ExpectedBucketOwner property is set
internal bool IsSetExpectedBucketOwner()
{
    return this.expectedBucketOwner != null;
}

// Check to see if IntelligentTieringId property is set
internal bool IsSetIntelligentTieringId()
{
    return this.intelligentTieringId != null;
}

New (Generated - PutBucketIntelligentTieringConfigurationRequest):

// Check to see if BucketName property is set
internal bool IsSetBucketName()
{
    return this._bucketName != null;
}

// Check to see if ExpectedBucketOwner property is set
internal bool IsSetExpectedBucketOwner()
{
    return this._expectedBucketOwner != null;
}

// Check to see if IntelligentTieringId property is set
internal bool IsSetIntelligentTieringId()
{
    return this._intelligentTieringId != null;
}

Impact:

  • For IntelligentTieringConfiguration.IntelligentTieringId: This IS a BREAKING CHANGE per the task criteria. The validation changed from !string.IsNullOrEmpty to != null. Empty strings ("") will now be considered as "set" when they were previously considered as "not set".
  • For PutBucketIntelligentTieringConfigurationRequest string properties: The old custom code already used != null check, so these are NOT breaking changes.

BREAKING CHANGE #2: Internal Method Name Changed

Severity: MEDIUM
File: IntelligentTieringConfiguration.cs

Old (Custom):

// Check if the tieringList property is set
internal bool IsSetTieringList()
{
    return this.tierings != null && (this.tierings.Count > 0 || !AWSConfigs.InitializeCollections);
}

New (Generated):

// Check to see if Tierings property is set
internal bool IsSetTierings()
{
    return this._tierings != null && (this._tierings.Count > 0 || !AWSConfigs.InitializeCollections); 
}

Impact: Method name changed from IsSetTieringList() to IsSetTierings(). The custom marshaller in PutBucketIntelligentTieringConfigurationRequestMarshaller.cs PreMarshallCustomization correctly references IsSetTierings(), so the code will work. However, if any external code or other internal code referenced IsSetTieringList(), it will break.


BREAKING CHANGE #3: Internal Method Name Changed

Severity: MEDIUM
File: PutBucketIntelligentTieringConfigurationRequest.cs

Old (Custom):

// Check to see if IntelligentTieringConfiguration property is set
internal bool IsIntelligentTieringConfiguration()
{
    return this.intelligentTieringConfiguration != null;
}

New (Generated):

// Check to see if IntelligentTieringConfiguration property is set
internal bool IsSetIntelligentTieringConfiguration()
{
    return this._intelligentTieringConfiguration != null;
}

Impact: Method name changed from IsIntelligentTieringConfiguration() to IsSetIntelligentTieringConfiguration(). This breaks naming convention consistency. The generated marshaller correctly uses IsSetIntelligentTieringConfiguration(), and the custom marshaller PreMarshallCustomization checks the configuration object directly rather than using this method.


BREAKING CHANGE #4: Property Access Pattern Inconsistency (Potential Issue)

Severity: LOW
File: Tiering.cs

Old (Custom):

private int? days;
private IntelligentTieringAccessTier accessTier;

public int? Days
{
    get { return this.days; }
    set { this.days = value; }
}

public IntelligentTieringAccessTier AccessTier
{
    get { return this.accessTier; }
    set { this.accessTier = value; }
}

New (Generated):

private IntelligentTieringAccessTier _accessTier;
private int? _days;

public IntelligentTieringAccessTier AccessTier
{
    get { return this._accessTier; }
    set { this._accessTier = value; }
}

public int? Days
{
    get { return this._days; }
    set { this._days = value; }
}

Impact: Private field naming changed from days to _days and accessTier to _accessTier. Property declaration order changed (AccessTier before Days in generated vs Days before AccessTier in custom). This is NOT a breaking change for public API but represents an inconsistency in internal implementation. The generated version also adds [AWSProperty(Required=true)] attributes which is more strict.


NON-BREAKING CHANGES VERIFIED:

✅ Response Classes

  • PutBucketIntelligentTieringConfigurationResponse.cs: Both custom and generated are empty response classes. No breaking changes.

✅ Marshaller Logic Preserved

  • The custom marshaller partial class correctly implements:

    • IntelligentTieringFilterCustomMarshall() - Custom filter marshalling logic preserved
    • PreMarshallCustomization() - Validation logic preserved
    • PostMarshallCustomization() - Checksum logic preserved
  • The generated marshaller correctly calls these custom methods via partial method hooks

✅ Response Unmarshaller

  • Both custom and generated unmarshallers are functionally identical (empty response)

✅ Generator Files

  • ServiceModel.cs: Uncommented line to enable PutBucketIntelligentTieringConfiguration generation (expected change)
  • s3.customizations.json: Added proper customization entries for property name mappings and filter marshalling injection

SUMMARY:

Total Breaking Changes Found: 4

Critical Issues (Must Fix):

  1. Breaking Change 1 - IsSet method logic change for IntelligentTieringConfiguration.IntelligentTieringId from !string.IsNullOrEmpty to != null

Medium Issues (Review Required):

  1. Breaking Change 2 - Internal method name IsSetTieringList()IsSetTierings()
  2. Breaking Change 3 - Internal method name IsIntelligentTieringConfiguration()IsSetIntelligentTieringConfiguration()

Low Issues (Monitor):

  1. Breaking Change 4 - Property order and private field naming pattern changes in Tiering.cs

Files Analyzed: 14 of 14 (100% Complete)

RECOMMENDATION:

The most critical issue is Breaking Change #1. The IsSetIntelligentTieringId() method in IntelligentTieringConfiguration.cs must be customized to maintain the !string.IsNullOrEmpty check instead of the generated != null check to preserve backward compatibility for request object string property validation.

// Check to see if IntelligentTieringConfiguration property is set
internal bool IsIntelligentTieringConfiguration()
// 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.

I'm keeping the ISSet to !=null because we check !string.IsNullOrEmpty in the request marshaller

@peterrsongg peterrsongg marked this pull request as ready for review November 20, 2025 18:19
if (publicRequestIntelligentTieringConfigurationTieringsValue != null)
{
xmlWriter.WriteStartElement("Tiering");
if(publicRequestIntelligentTieringConfigurationTieringsValue.IsSetAccessTier())
Copy link
Contributor

Choose a reason for hiding this comment

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

Days used to come before AccessTier. It shouldn't matter in this case but calling it out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, doesn't matter in this case since it is a normal xml element. And there are tests here that test this code path:

public class IntelligentTieringTests : TestBase<AmazonS3Client>

xmlWriter.WriteStartElement("Tiering");
if(publicRequestIntelligentTieringConfigurationTieringsValue.IsSetAccessTier())
xmlWriter.WriteElementString("AccessTier", StringUtils.FromString(publicRequestIntelligentTieringConfigurationTieringsValue.AccessTier));
if(publicRequestIntelligentTieringConfigurationTieringsValue.IsSetDays())
Copy link
Contributor

Choose a reason for hiding this comment

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

It used to write these element strings if they were set or not. Now they will be skipped if not set. Is this correct?

Copy link
Contributor Author

@peterrsongg peterrsongg Nov 21, 2025

Choose a reason for hiding this comment

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

ooh nice catch... I just tested though and the behavior is the same.

Something like this (I've tried with no days, no access-tier and no both members)

        var putBucketTieringRequest = new PutBucketIntelligentTieringConfigurationRequest
        {
            BucketName = bucketName,
            IntelligentTieringId = "test-tiering-id-2",
            IntelligentTieringConfiguration = new IntelligentTieringConfiguration
            {
                IntelligentTieringId = "test-tiering-id",
                Status = IntelligentTieringStatus.Enabled,
                Tierings = new List<Tiering>
                {
                    new Tiering
                    {
                    }
                }
            }
        };

throws => AmazonS3Exception "xml provided was invalid or not well formed"

and the behavior now is the same: AmazonS3Exception "xml provided was invalid or not well formed"

Copy link
Member

Choose a reason for hiding this comment

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

xml provided was invalid or not well formed as a user I would really hate this exception, it doesn't give any direction to what went wrong, and it is very confusing since users don't provide xml.
But fixing exceptions isn't part of the PR anyway, so I will approve it, though I won't complain if you fixed it :D

@peterrsongg peterrsongg changed the base branch from petesong/phase-3-pr-3-base to petesong/4145-4149 November 21, 2025 18:17
@peterrsongg peterrsongg merged commit 597f456 into petesong/4145-4149 Nov 21, 2025
6 checks 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
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/1 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.

4 participants