Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Dec 19, 2025

Stacked PRs:


Description

Generate GetBucketLogging

Motivation and Context

Testing

Assembly Comparison output (non breaking)

AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.PutBucketLoggingRequest/MethodAdded: New method System.String get_ContentMD5() in Amazon.S3.Model.PutBucketLoggingRequest
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.PutBucketLoggingRequest/MethodAdded: New method System.Void set_ContentMD5(System.String) in Amazon.S3.Model.PutBucketLoggingRequest
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.BucketLoggingStatus/TypeAdded: New type Amazon.S3.Model.BucketLoggingStatus

Fuzz Testing run: No backwards incompatibilities
DRY_RUN passed Build id: d30dff17-75cd-433b-8c3a-ea1ca887a28d

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: #4250, branch: peterrsongg/petesong/phase-3-pr7-2/2
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr7-2/1 branch from 19820f0 to fec8357 Compare December 19, 2025 18:02
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr7-2/2 branch from 46b4848 to 838b1b6 Compare December 19, 2025 18:02
@peterrsongg peterrsongg changed the base branch from peterrsongg/petesong/phase-3-pr7-2/1 to petesong/phase-3-pr7-base December 19, 2025 18:04
@peterrsongg peterrsongg changed the base branch from petesong/phase-3-pr7-base to peterrsongg/petesong/phase-3-pr7-2/1 December 19, 2025 18:04
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Dec 19, 2025

READ THIS

Change 1 was intentional, I'll comment why on the PR

Breaking Changes Analysis for Commit 16a3578

Summary

Analyzed the PutBucketLogging migration from custom to generated code. Found 1 CRITICAL BREAKING CHANGE.


BREAKING CHANGES

1. PutBucketLoggingRequest.cs - IsSetLoggingConfig() Method Logic Changed

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

Issue: The IsSetLoggingConfig() method has different logic between custom and generated versions.

Old Custom Logic (deleted file):

internal bool IsSetLoggingConfig()
{
    return this.LoggingConfig != null;
}

New Generated Logic:

internal bool IsSetLoggingConfig()
{
    return this._loggingConfig != null && this._loggingConfig.IsSetTargetBucketName();
}

Breaking Impact:

  • Previously, IsSetLoggingConfig() would return true if the LoggingConfig object was set, regardless of its internal state
  • Now it returns true ONLY if LoggingConfig is set AND TargetBucketName is also set
  • This changes the behavior for cases where LoggingConfig is instantiated but TargetBucketName is null/not set
  • Code depending on the old behavior (checking if LoggingConfig object exists) will now behave differently
  • This could cause marshalling logic to skip the LoggingConfig section when it should be included

NON-BREAKING CHANGES

1. PutBucketLoggingRequest.cs - New ContentMD5 Property Added

  • New property ContentMD5 with getter, setter, and IsSetContentMD5() method was added
  • This is an additive change and not breaking for existing code

2. PutBucketLoggingRequestMarshaller.cs - Logic Preserved via Custom Partial

  • The custom marshalling logic for TargetPrefix (writing empty element when not set) is preserved
  • Implemented in custom partial method TargetPrefixCustomMarshall in /Custom/Model/Internal/MarshallTransformations/PutBucketLoggingRequestMarshaller.cs
  • Generated marshaller correctly calls this custom method
  • NO BREAKING CHANGE

3. PutBucketLoggingResponseUnmarshaller.cs - Moved to Generated

  • File moved from Custom to Generated folder
  • Standard unmarshaller implementation with no logic changes
  • NO BREAKING CHANGE

4. PutBucketLoggingResponse.cs - Moved to Generated

  • Empty response class, no functional changes
  • NO BREAKING CHANGE

5. BucketLoggingStatus.cs - New File

  • New generated model class added
  • Additive change only
  • NO BREAKING CHANGE

6. String Property IsSet Methods - No Changes

  • IsSetExpectedBucketOwner() uses !String.IsNullOrEmpty() in both versions (no change)
  • IsSetBucketName() uses != null check in both versions (no change)
  • NO BREAKING CHANGE

Files Analyzed: 26 out of 26

  • ✅ PutBucketLoggingRequest.cs (BREAKING CHANGE FOUND)
  • ✅ PutBucketLoggingRequestMarshaller.cs (Custom + Generated)
  • ✅ PutBucketLoggingResponseUnmarshaller.cs
  • ✅ PutBucketLoggingResponse.cs
  • ✅ BucketLoggingStatus.cs
  • ✅ S3BucketLoggingConfig.cs (Custom + Generated)
  • ✅ RestXmlRequestMarshaller.cs/.tt (generator template changes)
  • ✅ ServiceModel.cs (generator changes)
  • ✅ s3.customizations.json (configuration)
  • ✅ Other affected marshallers (reviewed for pattern consistency)

Total Breaking Changes Found: 1

Fuzz Test Output

Fuzz tests successfull

@peterrsongg peterrsongg marked this pull request as ready for review December 19, 2025 18:09
// Check to see if LoggingConfig property is set
internal bool IsSetLoggingConfig()
{
return this._loggingConfig != null && this._loggingConfig.IsSetTargetBucketName();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

var marshallName = member.Shape.IsList ? member.Shape.ListMarshallName ?? "member" : member.MarshallName;
if(member.Shape.IsFlattened || member.IsFlattened)
marshallName = member.LocationName ?? member.ModeledName;
if (member.Shape.IsList)
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 had to add this so that I can just customize the IsSet method to control marshalling logic. But also, it's the rightr thing to do. It's also related to:
#4247

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.

1 participant