generate deletebucketmetricsconfiguration #4104

peterrsongg · 2025-11-06T08:15:59Z

Description

This PR is a continuation of #4103

This PR generates DeleteBucketMetricsConfiguration

Motivation and Context

Testing

dry run passes

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

Copilot

Pull Request Overview

This PR migrates the DeleteBucketMetricsConfiguration operation from custom handwritten code to generated code using the AWS SDK code generator. The operation is now included in the S3 allow-list for code generation.

Adds generated marshaller/unmarshaller classes for DeleteBucketMetricsConfiguration
Removes custom implementations in favor of generated code
Configures property name mapping (Id -> MetricsId) in generator customizations
Includes dev config file indicating a patch-level change

Reviewed Changes

Copilot reviewed 6 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs	Adds generated response unmarshaller with error handling
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationRequestMarshaller.cs	Adds generated request marshaller with validation and parameter mapping
sdk/src/Services/S3/Generated/Model/DeleteBucketMetricsConfigurationResponse.cs	Updates response class with generated documentation and imports
sdk/src/Services/S3/Generated/Model/DeleteBucketMetricsConfigurationRequest.cs	Adds fully generated request class with comprehensive documentation
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs	Removes custom unmarshaller implementation
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationRequestMarshaller.cs	Removes custom marshaller implementation
sdk/src/Services/S3/Custom/Model/DeleteBucketMetricsConfigurationRequest.cs	Removes custom request class
generator/ServiceModels/s3/s3.customizations.json	Adds property name mapping configuration for MetricsId
generator/ServiceClientGeneratorLib/ServiceModel.cs	Enables generation by uncommenting operation in S3 allow-list
generator/.DevConfigs/b75d0287-d293-4a03-ad2f-1aa096a32665.json	Adds dev config for tracking this patch-level change

Copilot · 2025-11-06T16:10:38Z

...del/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs

+    /// <summary>
+    /// Response Unmarshaller for DeleteBucketMetricsConfiguration operation
+    /// </summary>  
+    public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ReponseUnmarshaller


Corrected spelling of 'S3ReponseUnmarshaller' to 'S3ResponseUnmarshaller'.

Suggested change

public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ReponseUnmarshaller

public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ResponseUnmarshaller

Is renaming to S3ReponseUnmarshaller a breaking change at this point? I had never noticed it had a typo...

It is a breaking change but it is in the internal namespace so we may choose to make it anyway.

...del/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs

peterrsongg · 2025-11-06T17:57:19Z

I'm pasting the results of the AI analysis done on the larger PR here
Updated AI prompt run on latest revision.

The added validation isn't a breaking change because the request will fail and throw AmazonS3Exception and since we are throwing the same exception but just earlier in the api call it isn't a breaking change. I intentionally made it the base service exception instead of a System.ArgumentException for that reason.

Now I have all the information needed to provide a comprehensive analysis. Let me compare the deleted custom files with the generated ones and examine all the marshaller changes:

Task Completed

Breaking Changes Analysis - S3 Custom to Generated Migration

After analyzing all deleted custom files, their generated replacements, and modified marshallers, I have identified the following breaking changes:

BREAKING CHANGES IDENTIFIED:

1. AbortMultipartUploadRequestMarshaller.cs - NEW VALIDATION ADDED

Issue: Added new required field validation that didn't exist before:

if (string.IsNullOrEmpty(publicRequest.UploadId))
    throw new AmazonS3Exception("Request object does not have required field UploadId set");

Impact: Customer code that previously could make AbortMultipartUpload requests without UploadId set will now throw an exception.

2. CopyPartRequestMarshaller.cs - NEW VALIDATIONS ADDED

Issue: Added two new required field validations:

if (publicRequest.PartNumber == null)
    throw new AmazonS3Exception("Request object does not have required field PartNumber set");
if (string.IsNullOrEmpty(publicRequest.UploadId))
    throw new AmazonS3Exception("Request object does not have required field UploadId set");

Impact: Customer code that previously could make CopyPart requests without PartNumber or UploadId set will now throw exceptions.

3. ListPartsRequestMarshaller.cs - NEW VALIDATION ADDED

Issue: Added new required field validation:

if (string.IsNullOrEmpty(publicRequest.UploadId))
    throw new AmazonS3Exception("Request object does not have required field UploadId set");

Impact: Customer code that previously could make ListParts requests without UploadId set will now throw an exception.

NO BREAKING CHANGES FOUND IN:

Model Files Migration (✅ SAFE)

DeleteBucketMetricsConfiguration Request/Response
GetBucketMetricsConfiguration Request/Response
ListBucketMetricsConfigurations Request/Response
MetricsConfiguration
PutBucketMetricsConfiguration Request/Response

Differences are non-breaking:

Private field naming changes (bucketName → _bucketName)
Added [AWSProperty(Required=true)] attributes (not breaking for existing code)
Minor documentation text changes
Collection initialization patterns updated to newer SDK standards

Marshaller Logic Preservation (✅ SAFE)

GetBucketMetricsConfigurationRequestMarshaller: Custom PreMarshallCustomization (Suppress404Exceptions) properly preserved
PutBucketMetricsConfigurationRequestMarshaller: Custom MetricsFilterCustomMarshall and PostMarshallCustomization (checksum logic) properly preserved through partial methods
PutLifecycleConfigurationRequestMarshaller: Filter marshalling logic properly moved to FilterCustomMarshall method

SUMMARY:

Files analyzed: 30 out of 30 total changed files
Breaking changes found: 3 files with new required field validations
Safe migrations: 27 files with logic properly preserved

The breaking changes are all related to new validation logic being added to existing marshallers rather than logic being lost during migration. These validations enforce that required fields are set before making API calls, which technically makes the SDK more strict than before.

.../Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationRequestMarshaller.cs

dscpinheiro · 2025-11-06T19:31:29Z

...del/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs

+    /// <summary>
+    /// Response Unmarshaller for DeleteBucketMetricsConfiguration operation
+    /// </summary>  
+    public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ReponseUnmarshaller


Is renaming to S3ReponseUnmarshaller a breaking change at this point? I had never noticed it had a typo...

peterrsongg · 2025-11-06T19:54:50Z

@dscpinheiro

Is renaming to S3ReponseUnmarshaller a breaking change at this point? I had never noticed it had a typo...

Yeah... this has always bothered me, but technically it'd be a breaking change right b/c it's a public class and we're changing the name? I just left it as is

boblodgett · 2025-11-07T20:54:16Z

sdk/src/Services/S3/Generated/Model/DeleteBucketMetricsConfigurationRequest.cs

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


Was return !String.IsNullOrEmpty(this.expectedBucketOwner); please check

okay i had to merge the pt3 into this branch and force push but it should be updated now.

boblodgett · 2025-11-07T20:54:36Z

sdk/src/Services/S3/Generated/Model/DeleteBucketMetricsConfigurationRequest.cs

+        // Check to see if MetricsId property is set
+        internal bool IsSetMetricsId()
+        {
+            return this._metricsId != null;


Was return !(string.IsNullOrEmpty(this.MetricsId)); please check

okay i had to merge the pt3 into this branch and force push but it should be updated now.

.../Model/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationRequestMarshaller.cs

boblodgett · 2025-11-07T20:59:19Z

...del/Internal/MarshallTransformations/DeleteBucketMetricsConfigurationResponseUnmarshaller.cs

+    /// <summary>
+    /// Response Unmarshaller for DeleteBucketMetricsConfiguration operation
+    /// </summary>  
+    public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ReponseUnmarshaller


It is a breaking change but it is in the internal namespace so we may choose to make it anyway.

generate deletebucketmetricsconfiguration

5b380b3

GarrettBeatty requested a review from Copilot November 6, 2025 16:00

Copilot AI reviewed Nov 6, 2025

View reviewed changes

peterrsongg requested a review from dscpinheiro November 6, 2025 17:23

eddiemcs3 requested a review from boblodgett November 6, 2025 18:34

dscpinheiro reviewed Nov 6, 2025

View reviewed changes

dscpinheiro approved these changes Nov 6, 2025

View reviewed changes

peterrsongg deleted the branch petesong/phase-3-pr-1-pt3 November 7, 2025 18:23

peterrsongg closed this Nov 7, 2025

peterrsongg reopened this Nov 7, 2025

boblodgett reviewed Nov 7, 2025

View reviewed changes

fix IsSet for string properties

5ec3802

peterrsongg requested a review from boblodgett November 7, 2025 23:18

peterrsongg force-pushed the petesong/phase-3-pr-1-pt4 branch from b0719c1 to 5ec3802 Compare November 8, 2025 02:19

fix merge conflicts

4b898f5

boblodgett approved these changes Nov 8, 2025

View reviewed changes

	public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ReponseUnmarshaller
	public partial class DeleteBucketMetricsConfigurationResponseUnmarshaller : S3ResponseUnmarshaller

generate deletebucketmetricsconfiguration #4104

Are you sure you want to change the base?

generate deletebucketmetricsconfiguration #4104

Uh oh!

Conversation

peterrsongg commented Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterrsongg commented Nov 6, 2025

Breaking Changes Analysis - S3 Custom to Generated Migration

BREAKING CHANGES IDENTIFIED:

1. AbortMultipartUploadRequestMarshaller.cs - NEW VALIDATION ADDED

2. CopyPartRequestMarshaller.cs - NEW VALIDATIONS ADDED

3. ListPartsRequestMarshaller.cs - NEW VALIDATION ADDED

NO BREAKING CHANGES FOUND IN:

Model Files Migration (✅ SAFE)

Marshaller Logic Preservation (✅ SAFE)

SUMMARY:

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterrsongg commented Nov 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

peterrsongg commented Nov 6, 2025 •

edited

Loading