Generate ListObjects

[peterrsongg]

Description

Generates ListObjects

Assembly Comparison Output (empty)

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

Build succeeded in 1.1s

Build succeeded in 1.0s

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

[peterrsongg]

READ THIS

None of the aforementioned breaking changes are actually breaking. The bucketName because we check String.IsNullOrEmpty when validating required uri params. The BucketName added to the resource path is a hallucination. It got foncused on which is the old and which is the new. None of them ever added BucketName to the resource path.

Breaking Changes Analysis for Commit ca9c530

CRITICAL BREAKING CHANGES FOUND: 2

POTENTIAL BREAKING CHANGES: 1

---

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

❌ BREAKING CHANGE #1: BucketName IsSet Method Logic Changed

Location: IsSetBucketName() method

OLD (Custom):

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 Preserved

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

• IsSetDelimiter() - return this._delimiter != null; (same as before)
• IsSetMarker() - return this._marker != null; (same as before)
• IsSetPrefix() - return this._prefix != null; (same as before)
• IsSetExpectedBucketOwner() - Correctly customized via injection to use !String.IsNullOrEmpty

---

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

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

Location: Marshall() method

OLD (Custom - deleted file):

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}";

NEW (Generated):

if (string.IsNullOrEmpty(publicRequest.BucketName))
    throw new System.ArgumentException("BucketName is a required property and must be set before making this call.", "ListObjectsRequest.BucketName");
// ... 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}?delimiter=...&marker=...
Actual Request Path: /?delimiter=...&marker=...

Custom Partial Check: I verified there are NO custom partial implementations of ListObjectsRequestMarshaller 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 ListObjects will fail because the bucket name is not in the request URL.

---

✅ POSITIVE: Query Parameters Properly Marshalled

The generated marshaller correctly adds all query parameters:

• delimiter, encoding-type, marker, max-keys, prefix - All properly added
• Headers (x-amz-expected-bucket-owner, x-amz-optional-object-attributes, x-amz-request-payer) - All properly added

---

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

✅ NO BREAKING CHANGES - Custom Logic Preserved

Property Name Mapping Preserved:

• Contents renamed to S3Objects via customization (matches old behavior)
• EncodingType mapped to Encoding property (not used in old code, but configuration exists)

Custom NextMarker Getter Preserved:

public string NextMarker
{
    get { return NextMarkerCustomGetter(); }
    set { this._nextMarker = value; }
}

The custom NextMarkerCustomGetter() method in the Custom partial file correctly implements the fallback logic:

• If NextMarker is not provided by service AND list is truncated AND there are objects, it uses the last object's Key as NextMarker
• This matches the old behavior exactly

---

File 4: sdk/src/Services/S3/Custom/Model/ListObjectsResponse.cs

✅ NO BREAKING CHANGES - Custom Partial Preserved

The custom partial file correctly implements:

private string NextMarkerCustomGetter()
{
    if (String.IsNullOrEmpty(_nextMarker) &&
        _isTruncated.GetValueOrDefault() &&
        (this.S3Objects?.Count > 0))
    {
        int lastObjIdx = this.S3Objects.Count - 1;
        _nextMarker = this.S3Objects[lastObjIdx].Key;
    }
    return this._nextMarker;
}

This preserves the original logic that was in the old custom code.

---

File 5: sdk/src/Services/S3/Generated/Model/Internal/MarshallTransformations/ListObjectsResponseUnmarshaller.cs

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

Location: UnmarshallException() method

OLD (Custom - partially deleted):

if (errorResponse.Code != null && errorResponse.Code.Equals("NoSuchBucket"))
{
    return new NoSuchBucketException(errorResponse.Message);
}
return new AmazonS3Exception(errorResponse.Message, innerException, errorResponse.Type, 
    errorResponse.Code, errorResponse.RequestId, statusCode);

NEW (Generated):

if (errorResponse.Code != null && errorResponse.Code.Equals("NoSuchBucket"))
{
    return NoSuchBucketExceptionUnmarshaller.Instance.Unmarshall(contextCopy, errorResponse);
}
return base.ConstructS3Exception(context, errorResponse, innerException, statusCode);

Impact: The exception construction has changed:

1. NoSuchBucket exceptions now use an unmarshaller instead of direct instantiation
2. Other exceptions use ConstructS3Exception() instead of direct AmazonS3Exception instantiation

This could affect error handling if the unmarshaller or ConstructS3Exception() constructs exceptions differently. However, this is likely just a refactoring to use common patterns. Needs verification.

---

✅ POSITIVE: Custom Contents Unmarshalling Preserved

The generated unmarshaller correctly calls the custom method:

if (context.TestExpression("Contents", targetDepth))
{
    ContentsCustomUnmarshall(context, response);
    continue;
}

The custom partial file implements ContentsCustomUnmarshall() which adds the bucket name to each S3Object:

private static void ContentsCustomUnmarshall(XmlUnmarshallerContext context, ListObjectsResponse response)
{
    if (response.S3Objects == null)
    {
        response.S3Objects = new List<S3Object>();
    }
    var s3Object = ContentsItemUnmarshaller.Instance.Unmarshall(context);
    s3Object.BucketName = response.Name;  // CRITICAL: Adds bucket name for PowerShell pipelining
    response.S3Objects.Add(s3Object);
}

This preserves the important behavior of populating BucketName on each S3Object for better PowerShell experience.

---

File 6: sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/ListObjectsResponseUnmarshaller.cs

✅ NO BREAKING CHANGES

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

---

Files 7-8: Generator Configuration Files

✅ NO BREAKING CHANGES

Files:

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

These files add configuration to:

• Add ListObjects to the allow list
• Configure property name mappings (Contents → S3Objects, EncodingType → Encoding)
• Add custom unmarshalling hooks
• Configure CommonPrefixes type swap to List<string>
• Add ExpectedBucketOwner IsSet customization

No breaking changes in configuration.

---

SUMMARY

Files Analyzed: 9 out of 9 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:

• ❌ ListObjectsRequest.cs - 1 breaking change (IsSetBucketName)
• 🚨 ListObjectsRequestMarshaller.cs - 1 critical breaking change (missing bucket name in path)
• ⚠️ ListObjectsResponseUnmarshaller.cs - 1 potential breaking change (exception construction)

Files Without Issues:

• ✅ ListObjectsResponse.cs (Generated) - Custom logic preserved via injection
• ✅ ListObjectsResponse.cs (Custom partial) - Preserved NextMarker custom getter
• ✅ ListObjectsResponseUnmarshaller.cs (Custom partial) - Preserved Contents custom unmarshalling
• ✅ 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 ListObjects API calls to fail with 404 or similar errors.

Note: The custom unmarshalling logic for Contents and NextMarker was correctly preserved through customization hooks, which is excellent. The marshaller issue needs the same treatment - either the bucket name needs to be added in generated code, or a custom partial implementation must be created.

[dscpinheiro]

Nit but the indentation on this line looks odd (more spaces than the other branches)

[dscpinheiro]

I had never seen service models use <important>... Does the doc generator handle it properly? (you can check by getting the docs from the dry-run)

[dscpinheiro]

I know it was there before but this return is redundant.

[dscpinheiro]

Since this was missing from the hand-written class, do you think it'd be worth to customize it to Encoding as well? (to match the request class)

I don't feel strongly about either option.