Generate ListBucketMetricsConfiguration

[peterrsongg]

Description

Continuation of #4102

Motivation and Context

Generates ListBucketMetricsConfiguration

Testing

All testng done in larger PR with all 4 operations which this PR is based off of.
dry run passed

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

[peterrsongg]

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.

[dscpinheiro]

Was this supposed to be in this PR?

[peterrsongg]

no, that should only be in the very first PR. this change is in both branches so not sure why it is showing up here😅

[peterrsongg]

okay i fixed it

[boblodgett]

Isn't t his a runtime breaking change? Before it wouldn't be returned as null even though the type was bool? and now it can. I agree the new behavior is correct now but it is different than it used to be.

[peterrsongg]

wouldn't this still return the same value since the underlying default value is null now? This is what the BoolUnmarshaller does

        public static T Unmarshall(JsonUnmarshallerContext context, ref StreamingUtf8JsonReader reader)
        {
            context.Read(ref reader);
            string text = context.ReadText(ref reader);
            if (text == null)
                return default(T);

            return (T)Convert.ChangeType(text, typeof(T), CultureInfo.InvariantCulture);
        }

[boblodgett]

The BoolUnmarshaller that it was setup the simple type as:

public bool Unmarshall(XmlUnmarshallerContext context)
        {
            return SimpleTypeUnmarshaller<bool>.Unmarshall(context);
        }

It isn't a bool?, it is a <bool> and in that case it will return False for the default when the text is null.

The NullableBoolUnmarshaller doesn't even use Default(T). It does;

 public bool? Unmarshall(XmlUnmarshallerContext context)
        {
            context.Read();
            string text = context.ReadText();
            if (string.IsNullOrEmpty(text))
            {
                return null;
            }

            return bool.Parse(text);
        }

So in this case if the text is null it will return null. So this is a behavior change.

[peterrsongg]

Ah you’re right, would you be okay with just calling this out in the changelog as a breaking change instead of adding a customization for it? I could be convinced either way but given this is fixing something I lean towards the changelog entry

[normj]

To make sure of my understanding the public interface is already bool? but because we were using the non nullable marshaller we would always marshal a value. But the IsTruncated element would have to exist in the xml output without a value. Essentially having a <IsTruncated />. My understanding is S3 will always returned back the IsTruncated element in the out with a value. So user's using S3, not considering S3 compatible services, will not notice any change in behavior. My suggestion is if we have a simple way to add the customization we should use it but I don't want to add any significant complexity to the SDK and generation for this.

[peterrsongg]

yup and I just confirmed IsTruncated will always return a value

<?xml version="1.0" encoding="UTF-8"?>
<ListMetricsConfigurationsResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
<IsTruncated>false</IsTruncated>
</ListMetricsConfigurationsResult>

So actually, from the user's perspective it would be the same

[boblodgett]

Fair points Then it is likely safe to change it to NullableBoolUnmarshaller.