Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Dec 9, 2025

Description

Generates DeleteBucketMetadataConfiguration

Assembly Comparison Output ( new property added, which isn't a breaking change)

AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.ListMultipartUploadsResponse/MethodAdded: New method System.Void set_EncodingType(Amazon.S3.EncodingType) in Amazon.S3.Model.ListMultipartUploadsResponse
AssemblyComparer AWSSDK.S3.New.dll: Message Amazon.S3.Model.ListMultipartUploadsResponse/MethodAdded: New method Amazon.S3.EncodingType get_EncodingType() in Amazon.S3.Model.ListMultipartUploadsResponse

Fuzz Tests yielded no breaking change

Motivation and Context

Testing

DRY RUN: 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: #4207, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/4
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr5-rebased-2/3 branch from 32a5768 to 50bdfc2 Compare December 9, 2025 01:47
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr5-rebased-2/4 branch from a1013dd to ec4fd4a Compare December 9, 2025 01:47
@peterrsongg
Copy link
Contributor Author

READ THIS

The 3 breaking changes the AI analysis looked at is not breaking.

Breaking Changes Analysis for Commit ec4fd4a

CRITICAL BREAKING CHANGES FOUND: 2

POTENTIAL BREAKING CHANGES: 1


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

❌ BREAKING CHANGE #1: BucketName IsSet Method Logic Changed

Location: IsSetBucketName() method

OLD (Custom - from git diff):

internal bool IsSetBucketName()
{
    return !String.IsNullOrEmpty(this.bucketName);
}

NEW (Generated):

internal bool IsSetBucketName()
{
    return this._bucketName != null;
}

Impact: This is a breaking change for request objects with string properties. The old code treated empty strings ("") as NOT SET, while the new code treats empty strings as SET:

  • Before: IsSetBucketName() returns false for both null and ""
  • After: IsSetBucketName() returns true for "", false only for null

This changes validation behavior and could cause API calls with empty bucket names to be processed differently.


✅ NO ISSUE: Other String Property IsSet Methods

The following IsSet methods use null-only checking (which matches the old behavior for non-bucket properties):

  • IsSetDelimiter() - return this._delimiter != null;
  • IsSetKeyMarker() - return this._keyMarker != null;
  • IsSetPrefix() - return this._prefix != null;
  • IsSetUploadIdMarker() - return this._uploadIdMarker != null;
  • IsSetExpectedBucketOwner() - Correctly customized via injection to use !String.IsNullOrEmpty

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

🚨 CRITICAL BREAKING CHANGE #2: Bucket Name NOT Added to Request Path

Location: Marshall() method

OLD (Custom - deleted file, from git diff):

if (!publicRequest.IsSetBucketName())
    throw new AmazonS3Exception("Request object does not have required field BucketName set");
request.AddPathResource("{Bucket}", StringUtils.FromString(publicRequest.BucketName));
request.ResourcePath = "/{Bucket}?uploads";

NEW (Generated):

if (string.IsNullOrEmpty(publicRequest.BucketName))
    throw new System.ArgumentException("BucketName is a required property and must be set before making this call.", "ListMultipartUploadsRequest.BucketName");
request.AddSubResource("uploads");
// ... headers and parameters ...
request.ResourcePath = "/";

Impact: THIS IS A CRITICAL FAILURE. The bucket name is completely missing from the request path. The API call will be malformed and will FAIL at runtime.

Expected Request Path: /{BucketName}?uploads&delimiter=...
Actual Request Path: /?uploads&delimiter=...

Custom Partial Check: I verified there are NO custom partial implementations of ListMultipartUploadsRequestMarshaller with PreMarshallCustomization or PostMarshallCustomization methods to add the bucket name back. The generated marshaller has the hooks but nothing implements them.

Result: Every call to ListMultipartUploads will fail because the bucket name is not in the request URL.


✅ POSITIVE: Query Parameters and Headers Properly Marshalled

The generated marshaller correctly:

  • Adds uploads subresource
  • Adds all query parameters: delimiter, encoding-type, key-marker, max-uploads, prefix, upload-id-marker
  • Adds headers: x-amz-expected-bucket-owner, x-amz-request-payer

File 3: sdk/src/Services/S3/Generated/Model/ListMultipartUploadsResponse.cs

✅ NO BREAKING CHANGES - Property Name Mapping Preserved

Property Name Changes:

  • UploadsMultipartUploads via customization (matches old behavior)
  • EncodingType mapped to Encoding property (configured but not actively used in old code)

All properties correctly preserve old behavior:

  • BucketName, CommonPrefixes, Delimiter, EncodingType, IsTruncated
  • KeyMarker, MaxUploads, NextKeyMarker, NextUploadIdMarker
  • Prefix, RequestCharged, UploadIdMarker

File 4: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/ListMultipartUploadsResponseUnmarshaller.cs

⚠️ POTENTIAL BREAKING CHANGE #3: Exception Construction Method Changed

Location: UnmarshallException() method

OLD (Custom - partially deleted, from git diff):

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 from direct instantiation to using a base class method. This could affect error handling if ConstructS3Exception() constructs the exception differently. However, this is likely just refactoring to use common patterns. Needs verification.


✅ POSITIVE: Custom CommonPrefixes Unmarshalling Preserved

The generated unmarshaller correctly calls the custom method:

if (context.TestExpression("CommonPrefixes", targetDepth))
{
    CommonPrefixesCustomUnmarshall(context, response);
    continue;
}

The custom partial file implements CommonPrefixesCustomUnmarshall():

private static void CommonPrefixesCustomUnmarshall(XmlUnmarshallerContext context, ListMultipartUploadsResponse response)
{
    var prefix = CommonPrefixesItemUnmarshaller.Instance.Unmarshall(context);
    if (prefix != null)
    {
        if (response.CommonPrefixes == null)
        {
            response.CommonPrefixes = new List<string>();
        }
        response.CommonPrefixes.Add(prefix);
    }
}

This preserves the custom unmarshalling logic for CommonPrefixes.


File 5: sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/ListMultipartUploadsResponseUnmarshaller.cs

✅ NO BREAKING CHANGES

The custom partial file is correctly preserved with the CommonPrefixesCustomUnmarshall method that is called by the generated unmarshaller.


File 6: sdk/src/Services/S3/Generated/Model/MultipartUpload.cs

✅ NO BREAKING CHANGES - All Properties Preserved

Moved from Custom to Generated with proper preservation:

All properties maintain backward compatibility:

  • ChecksumAlgorithm - Same getter/setter, same IsSet logic
  • ChecksumType - Same getter/setter, same IsSet logic
  • Initiated - Same getter/setter, same IsSet logic
  • Initiator - Same getter/setter, same IsSet logic
  • Key - Same getter/setter, same IsSet logic (now has [AWSProperty(Min=1)] attribute but doesn't change behavior)
  • Owner - Same getter/setter, same IsSet logic
  • StorageClass - Same getter/setter, same IsSet logic
  • UploadId - Same getter/setter, same IsSet logic

Private field name changes (not breaking):

  • OLD: key, uploadId, owner, etc.
  • NEW: _key, _uploadId, _owner, etc.
  • This is just a naming convention change and doesn't affect public API

File 7: sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/MultipartUploadUnmarshaller.cs

✅ NO BREAKING CHANGES - Code Reformatted Only

The git diff shows this file was modified but the changes are primarily:

  • Code formatting/style changes
  • Variable name changes (unmarshalledObject → consistent naming)
  • No logical changes to the unmarshalling behavior
  • All the same XML elements are unmarshalled to the same properties

Files 8-9: Generator Configuration Files

✅ NO BREAKING CHANGES

Files:

  • generator/ServiceClientGeneratorLib/ServiceModel.cs
  • generator/ServiceModels/s3/s3.customizations.json

These files add configuration to:

  • Add ListMultipartUploads to the allow list
  • Configure property name mappings (UploadsMultipartUploads, EncodingTypeEncoding)
  • Add custom unmarshalling hooks for CommonPrefixes
  • Configure CommonPrefixes type swap to List<string>
  • Add ExpectedBucketOwner IsSet customization

No breaking changes in configuration.


SUMMARY

Files Analyzed: 10 out of 10 total files in the commit

Breaking Changes by Severity:

  1. CRITICAL (1): Missing bucket name in request path - All API calls will fail
  2. BREAKING (1): IsSetBucketName logic changed for empty strings
  3. POTENTIAL (1): Exception construction method changed (likely just refactoring)

Files with Issues:

  • ListMultipartUploadsRequest.cs - 1 breaking change (IsSetBucketName)
  • 🚨 ListMultipartUploadsRequestMarshaller.cs - 1 critical breaking change (missing bucket name in path)
  • ⚠️ ListMultipartUploadsResponseUnmarshaller.cs - 1 potential breaking change (exception construction)

Files Without Issues:

  • ListMultipartUploadsResponse.cs (Generated) - All properties preserved
  • ListMultipartUploadsResponseUnmarshaller.cs (Custom partial) - Custom unmarshalling preserved
  • MultipartUpload.cs (Generated) - All properties preserved, just moved
  • MultipartUploadUnmarshaller.cs (Custom) - Code reformatted only, no logic changes
  • ServiceModel.cs - Configuration only
  • s3.customizations.json - Configuration only

RECOMMENDATION: The missing bucket name in the request path (Breaking Change #2) MUST be fixed before this code can be merged. This is a blocking issue that will cause ALL ListMultipartUploads API calls to fail with 404 or similar errors.

Pattern Observed: This is the SAME ISSUE as the previous commits (DeleteBucketMetadataConfiguration and ListObjects). The generated marshallers are consistently missing the bucket name in the request path. A systematic fix is needed across all these operations.

peterrsongg added a commit that referenced this pull request Dec 9, 2025
stack-info: PR: #4207, branch: peterrsongg/petesong/phase-3-pr5-rebased-2/4
@peterrsongg peterrsongg marked this pull request as ready for review December 9, 2025 17:28
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 PR migrates the ListMultipartUploads S3 operation from handwritten code to generated code based on the s3-2006-03-01.normal.json service model. The change adds the EncodingType property to the response model and standardizes field naming conventions across the models and marshallers. Custom unmarshalling logic for CommonPrefixes is preserved in a partial class implementation.

Key Changes:

  • Replaces handwritten model classes and marshallers with auto-generated equivalents
  • Adds EncodingType property to ListMultipartUploadsResponse (non-breaking addition)
  • Updates field naming from camelCase to underscore-prefixed style (_fieldName)

Reviewed changes

Copilot reviewed 4 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
sdk/src/Services/S3/Generated/Model/MultipartUpload.cs Converted to generated code with updated field naming and reorganized property order (ChecksumType moved for alphabetical ordering)
sdk/src/Services/S3/Generated/Model/ListMultipartUploadsResponse.cs Converted to generated code, added EncodingType property, updated field naming and documentation
sdk/src/Services/S3/Generated/Model/ListMultipartUploadsRequest.cs Converted to generated code with updated field naming, documentation improvements, and AWSProperty attribute on BucketName
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/MultipartUploadUnmarshaller.cs Updated to use Instance singleton pattern and NullableDateTimeUnmarshaller for consistency
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/ListMultipartUploadsResponseUnmarshaller.cs New generated unmarshaller that integrates with custom CommonPrefixes unmarshalling logic
sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/ListMultipartUploadsRequestMarshaller.cs New generated marshaller replacing handwritten implementation
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/ListMultipartUploadsResponseUnmarshaller.cs Reduced to partial class containing only custom CommonPrefixes unmarshalling method
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/ListMultipartUploadsRequestMarshaller.cs Removed - functionality moved to generated code
generator/ServiceModels/s3/s3.customizations.json Added generation customizations for ListMultipartUploads request/output including property renaming and custom unmarshalling hooks
generator/ServiceClientGeneratorLib/ServiceModel.cs Added ListMultipartUploads to S3 allow list for code generation

Note: The PR description contains a copy-paste error stating "Generates DeleteBucketMetadataConfiguration" when it should say "Generates ListMultipartUploads". The actual changes and title are correct.

Critical Issue: This PR is missing a required DevConfig file. According to the CONTRIBUTING.md guidelines, all pull requests that modify code must include a DevConfig file in the generator/.DevConfigs directory for the release process and changelog generation.

Comment on lines +1596 to +1602
"ListMultipartUploadsRequest":{
"CommonPrefixes" : {
"Type": "List<string>",
"Marshaller": "StringUtils.FromString",
"Unmarshaller" : "CommonPrefixesItemUnmarshaller"
}
},
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The dataTypeSwap configuration includes an entry for ListMultipartUploadsRequest.CommonPrefixes (lines 1596-1602), but CommonPrefixes is a response-only property that doesn't exist in request objects. This entry should likely be removed, as only ListMultipartUploadsOutput.CommonPrefixes (lines 1603-1609) is needed. Compare with similar operations like ListObjects which only has a dataTypeSwap entry for the Output, not the Request.

Suggested change
"ListMultipartUploadsRequest":{
"CommonPrefixes" : {
"Type": "List<string>",
"Marshaller": "StringUtils.FromString",
"Unmarshaller" : "CommonPrefixesItemUnmarshaller"
}
},

Copilot uses AI. Check for mistakes.
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