Generate metrics configuration s3 operations

[peterrsongg]

Description

Generates 4 S3 operations.

Best way to review is by pulling up the custom version of the file on one side and the generated version of the file on the other side and compare. Although, now that we have fuzz testing I feel much more confident about the results.

Motivation and Context

Testing

Dry run passes: DRY_RUN-dc08f946-112a-4d5a-b85a-f33695a422d6
I've reviewed the fuzz-tests results. nothing backwards incompatible

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 S3 Metrics Configuration operations from custom (handwritten) code to generated code. The changes replace manual implementations with code generator templates while preserving custom logic through partial methods and customizations.

Key Changes:

• Moves PutBucketMetricsConfiguration, GetBucketMetricsConfiguration, ListBucketMetricsConfigurations, and DeleteBucketMetricsConfiguration operations from custom to generated code
• Preserves custom behavior through partial methods for marshalling/unmarshalling
• Updates the code generator to support required field checks and custom XML marshalling injection
• Adds appropriate dev config file for patch-level change

Reviewed Changes

Copilot reviewed 17 out of 38 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
sdk/src/Services/S3/Generated/Model/*.cs	Generated request/response model classes for metrics configuration operations
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/*.cs	Generated marshaller/unmarshaller implementations
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/*.cs	Custom partial implementations preserving legacy behavior
generator/ServiceModels/s3/s3.customizations.json	Customization configurations for code generation
generator/ServiceClientGeneratorLib/*.cs	Generator updates to support new features
generator/.DevConfigs/*.json	Dev config for version tracking

[Copilot]

Changing from else to if (listMarshallName == null) alters the logic. Previously, the default would only be set when there was no overrideDataType. Now it sets the default whenever listMarshallName is null, even after attempting to get it from overrideDataType.AlternateLocationName. This could cause incorrect marshalling behavior if overrideDataType.AlternateLocationName is explicitly set to null.

Suggested change

	if (listMarshallName == null)
	else

[Copilot]

Pull Request Overview

Copilot reviewed 17 out of 38 changed files in this pull request and generated no new comments.

[peterrsongg]

Here is the breaking change AI analysis (done internally via cline). What I did:

did a git reset to the commit right before all the generation commits so that the changed files were listed. Then i provided the diffs as context using the @git changes option in Cline.

Not gonna paste the prompt here b/c it is pretty long but this was the output. The things it listed as "breaking changes" are acceptable (changing from !String.IsNullOREmtpy to != null.

Breaking Changes Analysis Report - CORRECTED

After re-analyzing with the understanding that marshallers are split between custom and generated files that work together, I found the following:

NO BREAKING CHANGES in Marshaller Logic

The marshaller logic is correctly preserved through the split file pattern:

1. GetBucketMetricsConfigurationRequestMarshaller - ✅ NO ISSUES

• Custom file: Implements PreMarshallCustomization with defaultRequest.Suppress404Exceptions = true;
• Generated file: Calls PreMarshallCustomization(request, publicRequest);
• Result: Logic is preserved correctly

2. PutBucketMetricsConfigurationRequestMarshaller - ✅ NO ISSUES

• Custom file: Implements MetricsFilterCustomMarshall and PostMarshallCustomization with checksum logic
• Generated file: Calls both MetricsFilterCustomMarshall(publicRequest, xmlWriter); and PostMarshallCustomization(request, publicRequest);
• Result: Logic is preserved correctly

3. PutLifecycleConfigurationRequestMarshaller - ✅ NO ISSUES

• Custom file: Implements FilterCustomMarshall and CustomStatusMarshalling methods
• Generated file: Calls both FilterCustomMarshall(publicRequestConfigurationRulesValue, xmlWriter); and CustomStatusMarshalling(xmlWriter, publicRequestConfigurationRulesValue);
• Result: Logic is preserved correctly

BREAKING CHANGES FOUND in Model Classes

1. PutBucketMetricsConfigurationRequest.cs - ⚠️ BREAKING CHANGE

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

Issue: Different validation logic for IsSetExpectedBucketOwner() method

• Custom version: return !String.IsNullOrEmpty(this.expectedBucketOwner);
• Generated version: return this._expectedBucketOwner != null;
• Impact: The generated version will return true for empty strings, while the custom version returned false. This changes validation behavior and could cause different request processing.

2. ListBucketMetricsConfigurationsResponse.cs - ⚠️ BREAKING CHANGE

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

Issue: Different validation logic for IsSetNextToken() method

• Custom version: return !(string.IsNullOrEmpty(this.nextToken));
• Generated version: return this._nextToken != null;
• Impact: The generated version will return true for empty strings, while the custom version returned false. This affects pagination logic and could cause incorrect continuation token handling.

Property Implementation Changes - ✅ NO BREAKING CHANGES

The following changes are NOT breaking as they only affect private field naming:

• Custom: private string expectedBucketOwner; → Generated: private string _expectedBucketOwner;
• Custom: private string nextToken; → Generated: private string _nextToken;
• Public property getters/setters remain identical

Files Analyzed

Total files analyzed: 15 out of 43 files changed

Marshaller Files (6):

1. ✅ GetBucketMetricsConfigurationRequestMarshaller.cs (Custom + Generated pair)
2. ✅ PutBucketMetricsConfigurationRequestMarshaller.cs (Custom + Generated pair)
3. ✅ PutLifecycleConfigurationRequestMarshaller.cs (Custom + Generated pair)

Model Files (9):

4. ✅ DeleteBucketMetricsConfigurationRequest.cs - No issues
5. ✅ DeleteBucketMetricsConfigurationResponse.cs - No issues
6. ✅ GetBucketMetricsConfigurationRequest.cs - No issues
7. ✅ GetBucketMetricsConfigurationResponse.cs - No issues
8. ⚠️ ListBucketMetricsConfigurationsResponse.cs - BREAKING CHANGE
9. ✅ ListBucketMetricsConfigurationsRequest.cs - No issues
10. ✅ MetricsConfiguration.cs - No issues
11. ⚠️ PutBucketMetricsConfigurationRequest.cs - BREAKING CHANGE
12. ✅ PutBucketMetricsConfigurationResponse.cs - No issues

Summary

Breaking Changes Found: 2 critical validation logic changes

The breaking changes are in the IsSet*() validation methods where the logic changed from checking for null-or-empty strings to only checking for null. This will cause different behavior when empty strings are passed, potentially affecting:

1. Request validation and processing
2. Pagination continuation logic

Recommendation: The IsSet*() methods in the generated files should be updated to match the original custom logic to maintain backward compatibility.

[peterrsongg]

I tested ListBucketMetricsConfigurationsResponse. The IsSet doesn't matter in the response since we use context.TestExpression.

For the request, what the AI says is true, an empty string would get past the marshaller but either way an empty continuation token is invalid.

[peterrsongg]

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.