Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Dec 9, 2025

Description

Generates DeleteBucketMetadataConfiguration

Assembly Comparison Output (empty)

C:\Scripts> .\assembly-comparison.ps1

Build succeeded in 16.2s
Build succeeded in 13.2s

Workload updates are available. Run `dotnet workload list` for more information.

Fuzz Tests yielded no breaking change

Motivation and Context

Testing

DRY_RUN-e581806f-264b-46d0-a331-1318e27c2273

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: #4204, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/1
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr5-rebased-2/1 branch from 829a2a2 to 04b1ed4 Compare December 9, 2025 01:47
@peterrsongg
Copy link
Contributor Author

peterrsongg commented Dec 9, 2025

READ THIS

Issue 1 is not a breaking change. Since we validate the BucketName as a required uri param and use String.IsNullOrEmpty
Issue 2 is correct behavior. We're not supposed to append the bucketname to the resource path and almost every operation doesn't do it. S3 just ignores the bucketname as part of the resource path and works as expected.
Issue 3: Calling the base's construct Exception creates the same exception type.


Based on my analysis of the git diff, I have identified BREAKING CHANGES in this migration:

BREAKING CHANGES FOUND:

File 1: sdk/src/Services/S3/Generated/Model/DeleteBucketMetadataConfigurationRequest.cs

ISSUE 1: BucketName IsSet Method Logic Changed

  • OLD (Custom): return !String.IsNullOrEmpty(this.bucketName);
  • NEW (Generated): return this._bucketName != null;
  • Impact: This is a breaking change for request objects with string properties. The old code would treat empty strings ("") as NOT SET, while the new code treats empty strings as SET. This changes the validation behavior and could cause different runtime behavior.

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

ISSUE 2: CRITICAL - Bucket Name Not Added to Request Path

  • OLD (Custom):

    request.AddPathResource("{Bucket}", StringUtils.FromString(publicRequest.BucketName));
    request.ResourcePath = "/{Bucket}";
  • NEW (Generated):

    request.ResourcePath = "/";
  • Impact: This is a CRITICAL BREAKING CHANGE. The bucket name is no longer being added to the request path. The old code properly constructed the path with the bucket name (e.g., /my-bucket?metadataConfiguration), but the new code only sets the path to /. Without the bucket name in the path, the API request will be malformed and will fail.

    Note: The generated marshaller includes PreMarshallCustomization and PostMarshallCustomization hooks, but there are NO custom partial implementations to add the bucket name back. I confirmed this by searching the Custom folder - no partials exist for DeleteBucketMetadataConfiguration.


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

ISSUE 3: Exception Construction Method Changed

  • OLD (Custom):

    return new AmazonS3Exception(errorResponse.Message, innerException, errorResponse.Type, 
        errorResponse.Code, errorResponse.RequestId, statusCode);
  • NEW (Generated):

    return base.ConstructS3Exception(context, errorResponse, innerException, statusCode);
  • Impact: The exception construction logic has changed. While this may be a refactoring to use a common method, the change in how the exception is instantiated could affect error handling behavior if ConstructS3Exception constructs the exception differently or with different parameters. This needs verification that ConstructS3Exception produces an equivalent exception.


Summary:

Files Analyzed: 4 out of 7 total files in the commit

  • DeleteBucketMetadataConfigurationRequest.cs - 1 BREAKING CHANGE
  • DeleteBucketMetadataConfigurationResponse.cs - No changes (0 lines modified, just moved)
  • DeleteBucketMetadataConfigurationRequestMarshaller.cs - 1 CRITICAL BREAKING CHANGE
  • DeleteBucketMetadataConfigurationResponseUnmarshaller.cs - 1 POTENTIAL BREAKING CHANGE

Remaining Files to Analyze:

  • generator/ServiceClientGeneratorLib/ServiceModel.cs
  • generator/ServiceModels/s3/s3.customizations.json
  • (2 files - generator configuration changes)

Would you like me to continue analyzing the remaining generator configuration files?

peterrsongg added a commit that referenced this pull request Dec 9, 2025
stack-info: PR: #4204, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/1
@peterrsongg peterrsongg marked this pull request as ready for review December 9, 2025 17:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This pull request migrates the DeleteBucketMetadataConfiguration operation from handwritten custom code to SDK-generated code. This is part of the AWS .NET SDK's ongoing effort to standardize S3 operations using code generation instead of manually maintained code.

Key Changes

  • Replaced custom Request and Response model classes with generated versions
  • Migrated request marshaller and response unmarshaller to use generated code with customization hooks
  • Added operation to generator's S3 allowlist and exception handling configuration
  • Added customization for ExpectedBucketOwner property validation in s3.customizations.json

Reviewed changes

Copilot reviewed 3 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
sdk/src/Services/S3/Generated/Model/DeleteBucketMetadataConfigurationRequest.cs New generated request model with standard .NET naming conventions for private fields
sdk/src/Services/S3/Generated/Model/DeleteBucketMetadataConfigurationResponse.cs New generated empty response model (appropriate for DELETE operations)
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/DeleteBucketMetadataConfigurationRequestMarshaller.cs Generated request marshaller with significant changes to resource path handling
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/DeleteBucketMetadataConfigurationResponseUnmarshaller.cs Generated response unmarshaller with standardized error handling
sdk/src/Services/S3/Custom/Model/DeleteBucketMetadataConfigurationRequest.cs Removed custom implementation (migrated to generated code)
generator/ServiceModels/s3/s3.customizations.json Added customization for ExpectedBucketOwner validation
generator/ServiceClientGeneratorLib/ServiceModel.cs Added operation to S3 allowlist and exception handling configuration

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