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 empty (no backwards incompatible changes)
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: #4249, branch: peterrsongg/petesong/phase-3-pr7-2/1
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr7-2/1 branch from 19820f0 to fec8357 Compare December 19, 2025 18:02
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Dec 19, 2025

READ THIS

S3BucketLoggingConfig does contain those three public methods. It has been preserved.

Breaking Changes Analysis for GetBucketLogging Migration

Files Analyzed: 19 of 19

BREAKING CHANGE FOUND:


File: S3BucketLoggingConfig.cs

Issue Type: Public API Methods Removed

Details:

The custom S3BucketLoggingConfig.cs file previously contained three public methods that have been completely removed in the migration to generated code:

  1. public void AddGrant(S3Grantee grantee, S3Permission permission)

    • Allowed users to programmatically add grants to the Grants collection
    • Created a new S3Grant object and added it to the Grants list
  2. public void RemoveGrant(S3Grantee grantee, S3Permission permission)

    • Allowed users to remove a specific grant with a specific permission
    • Searched the Grants collection for matching grantee and permission
  3. public void RemoveGrant(S3Grantee grantee)

    • Allowed users to remove all grants for a given grantee
    • Removed all S3Grant objects from the Grants collection that matched the grantee

Impact:

Any customer code that calls these methods will break at compile time. These were convenience methods for managing the Grants collection.

Previous Code (Custom):

public partial class S3BucketLoggingConfig
{
    public void AddGrant(S3Grantee grantee, S3Permission permission)
    {
        if (Grants == null)
        {
            Grants = new List<S3Grant>();
        }
        S3Grant grant = new S3Grant{ Grantee = grantee, Permission = permission };
        Grants.Add(grant);
    }

    public void RemoveGrant(S3Grantee grantee, S3Permission permission) { ... }
    public void RemoveGrant(S3Grantee grantee) { ... }
}

Current Code (Generated):

public partial class S3BucketLoggingConfig
{
    // Methods completely removed
    // Only properties remain: Grants, TargetBucketName, TargetObjectKeyFormat, TargetPrefix
}

Non-Breaking Changes Successfully Preserved:

  1. GetBucketLoggingResponse.BucketLoggingConfigCustomGetter() - Custom getter preserved via injectXmlPropertyGetter customization
  2. GetBucketLoggingRequestMarshaller.PreMarshallCustomization() - Suppress404Exceptions logic preserved in custom partial
  3. GetBucketLoggingRequest.IsSetExpectedBucketOwner() - Uses !String.IsNullOrEmpty() as specified in customizations
  4. Property name mappings - TargetBucket→TargetBucketName and TargetGrants→Grants correctly applied
  5. Unmarshaller logic - GetBucketLoggingResponseUnmarshaller properly unmarshalls to S3BucketLoggingConfig using S3BucketLoggingConfigUnmarshaller
  6. XML parsing paths - TargetGrants/Grant path correctly specified in generated unmarshaller
  7. IsSet methods - All generated IsSet methods use appropriate null checks
  8. PutBucketLoggingRequestMarshaller - Remains unchanged and compatible with generated model

Summary:

Total Files Changed: 19
Files Analyzed: 19 of 19
Breaking Changes: 1
Files with Issues: 1

The migration successfully preserved most of the custom behavior through the partial class pattern and customizations. However, the removal of the three public helper methods from S3BucketLoggingConfig represents a breaking API change that will affect any customers currently using these methods to manage bucket logging grants.

@peterrsongg peterrsongg marked this pull request as ready for review December 19, 2025 18:09
{
if (context.IsStartElement || context.IsAttribute)
{
if (context.TestExpression("TargetGrants/Grant", targetDepth))
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is replacing https://github.com/aws/aws-sdk-net/blob/main/sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/LoggingEnabledUnmarshaller.cs, right?

If yes, should (could?) that file be deleted in the PR too? And the old unmarshaller uses context.TestExpression("Grant", targetDepth + 1) instead, I assume that's because it was hand-written?

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.

2 participants