Skip to content

Conversation

@peterrsongg
Copy link
Contributor

@peterrsongg peterrsongg commented Dec 17, 2025

Stacked PRs:


Generate DeleteObjects


Description

Generate DeleteObjects

Motivation and Context

Testing

DRY_RUN-e9266a93-9f5d-4b97-b7ef-710a68e1db2e
Assembly Comparison Output (empty)


Fuzz Test Results passed. One thing on the request side is

<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Quiet xmlns="">false</Quiet></Delete>

and now we send

<Delete xmlns="http://s3.amazonaws.com/doc/2006-03-01/"><Quiet>false</Quiet></Delete>

basically we remove the xmlns="" now and omit it altogether. This doesn't change what gets marshalled on the request side though.

This happens for "Object" too.

But these aren't breaking.

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: #4240, branch: peterrsongg/petesong/phase-3-pr6/2
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr6/1 branch from 3f66473 to 1347904 Compare December 17, 2025 06:27
@peterrsongg peterrsongg force-pushed the peterrsongg/petesong/phase-3-pr6/2 branch from 5e59374 to 8baad14 Compare December 17, 2025 06:27
@peterrsongg
Copy link
Contributor Author

Breaking Change Analysis for DeleteObjects Migration (Commit 8baad14)

Files Analyzed: 18 out of 18 files in the commit

⚠️ BREAKING CHANGES FOUND: 1


BREAKING CHANGE 1: DeletedObject.DeleteMarker Type Change

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

Status: ⚠️ POTENTIAL BREAKING CHANGE

Issue: The DeleteMarker property type has changed from bool? to bool? (nullable remains) BUT the unmarshaller changed from BoolUnmarshaller to NullableBoolUnmarshaller.

Analysis:
From the commit diff in DeletedObjectUnmarshaller.cs:

BEFORE (Custom):

if (context.TestExpression("DeleteMarker", targetDepth))
{
    deletedItem.DeleteMarker = BoolUnmarshaller.GetInstance().Unmarshall(context);
    continue;
}

AFTER (Generated):

if (context.TestExpression("DeleteMarker", targetDepth))
{
    var unmarshaller = NullableBoolUnmarshaller.Instance;
    unmarshalledObject.DeleteMarker = unmarshaller.Unmarshall(context);
    continue;
}

Impact: The change from BoolUnmarshaller to NullableBoolUnmarshaller could affect how the XML value is parsed:

  • BoolUnmarshaller typically converts "true"/"false" strings to bool values
  • NullableBoolUnmarshaller handles nullable cases differently
  • This could potentially change behavior if the XML contains empty elements or missing values

Custom Code:

// In sdk/src/Services/S3/Custom/Model/DeletedObject.cs
public bool? DeleteMarker
{
    get { return this.deleteMarker; }
    set { this.deleteMarker = value; }
}

internal bool IsSetDeleteMarker()
{
    return this.deleteMarker.HasValue;  // Correct for nullable bool
}

Generated Code (from diff):

// Same property and IsSet pattern preserved

Recommendation: Verify that NullableBoolUnmarshaller produces identical results to BoolUnmarshaller.GetInstance() for the DeleteMarker field in all scenarios (true, false, missing, empty element).


Detailed File-by-File Analysis

1. DeleteObjectsRequest.cs (Custom - Partially Generated)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • Custom file remains in Custom folder with partial class
  • All public properties preserved:
    • BucketName (string)
    • BypassGovernanceRetention (bool?)
    • ChecksumAlgorithm (ChecksumAlgorithm)
    • ExpectedBucketOwner (string)
    • Objects (List)
    • MfaCodes (MfaCodes)
    • RequestPayer (RequestPayer)
    • Quiet (bool?)
  • IsSet methods:
    • IsSetExpectedBucketOwner() uses !String.IsNullOrEmpty() ✅ Correct for request
    • IsSetMfaCodes() uses custom CustomMfaCodesIsSet() method ✅ Preserved
  • Custom CustomMfaCodesIsSet() method preserved in custom partial

2. DeleteObjectsResponse.cs (Custom → Generated)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • Moved from Custom to Generated with property name changes via customizations
  • Property mapping preserved via customizations.json:
    • Custom Deleted → Generated DeletedObjects
    • Custom DeleteErrors → Generated DeleteErrors ✅ (already correct name)
    • RequestCharged preserved
  • IsSet methods use != null checks (standard for response objects) ✅
  • Serialization support removed from generated (moved to custom attributes) - NOT breaking
  • Private backing fields renamed from deleted to _deleted pattern - NOT breaking

3. DeleteError.cs (Custom → Generated)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • All public properties preserved:
    • Key (string)
    • VersionId (string)
    • Code (string)
    • Message (string)
  • IsSet methods use != null checks ✅
  • Property order unchanged
  • Serialization attributes removed - NOT breaking (response object)

4. DeletedObject.cs (Custom → Generated)

Status: ⚠️ SEE BREAKING CHANGE 1 ABOVE

Analysis:

  • All public properties preserved:
    • DeleteMarker (bool?)
    • DeleteMarkerVersionId (string)
    • Key (string)
    • VersionId (string)
  • IsSet methods correct:
    • DeleteMarker: .HasValue
    • Others: != null
  • Unmarshaller change from BoolUnmarshaller to NullableBoolUnmarshaller ⚠️

5. KeyVersion.cs (Custom DELETED → Generated CREATED)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • File moved from Custom (deleted 126 lines) to Generated (created 167 lines)
  • Public properties preserved:
    • Key (string) - Required
    • VersionId (string)
    • ETag (string) - NEW in generated
    • LastModifiedTime (DateTime?) - NEW in generated
    • Size (long?) - NEW in generated
  • New properties added are additive (non-breaking)
  • IsSet method for ETag uses !String.IsNullOrEmpty() per customizations.json ✅
  • Other IsSet methods use appropriate checks

6. Delete.cs (NEW Generated Shape)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • New generated class (92 lines)
  • Not a breaking change since it's a new internal structure
  • Used for flattening Delete structure into DeleteObjectsRequest

7. DeleteObjectsRequestMarshaller.cs (Custom + Generated Split)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • Custom marshaller remains for actual marshalling logic
  • Custom marshaller handles:
    • All header marshalling (BypassGovernanceRetention, MfaCodes, RequestPayer, ExpectedBucketOwner, ChecksumAlgorithm)
    • XML body generation with Objects list
    • Checksum calculation
    • MFA codes handling via IsSetMfaCodes() custom method
  • Generated marshaller would call custom marshaller or inject customizations
  • All previous marshalling behavior preserved

8. DeleteObjectsResponseUnmarshaller.cs (Custom → Generated)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • Unmarshaller logic preserved:
    • DeletedObjects list unmarshalling
    • DeleteErrors list unmarshalling
    • RequestCharged header
  • Unmarshallers changed from GetInstance() pattern to Instance singleton pattern - NOT breaking
  • Structure unchanged

9. DeleteErrorUnmarshaller.cs (Renamed/Moved)

Status: ✅ NO BREAKING CHANGES

Analysis:

  • File renamed from ErrorsItemUnmarshaller.cs to DeleteErrorUnmarshaller.cs
  • Unmarshaller logic preserved for all fields (Code, Key, Message, VersionId)
  • Singleton pattern updated - NOT breaking

10. DeletedObjectUnmarshaller.cs (Renamed/Moved)

Status: ⚠️ SEE BREAKING CHANGE 1 ABOVE

Analysis:

  • File renamed from DeletedItemUnmarshaller.cs to DeletedObjectUnmarshaller.cs
  • DeleteMarker unmarshaller changed ⚠️
  • Other fields unchanged

11. s3.customizations.json

Status: ✅ Properly Configured

Key Customizations:

"DeleteObjectsRequest":{
    "modify":[
        {"MFA":{"emitPropertyName" :"MfaCodes"}},
        {"MfaCodes":{
            "injectXmlIsSet": ["return CustomMfaCodesIsSet();"],
            "injectXmlMarshallCode" : ["MfaCodesCustomMarshall(request, publicRequest);"]
        }},
        {"ExpectedBucketOwner":{
            "injectXmlIsSet": ["return !String.IsNullOrEmpty(this._expectedBucketOwner);"]
        }}
    ]
},
"DeleteObjectsOutput":{
    "modify":[
        {"Deleted":{"emitPropertyName":"DeletedObjects"}},
        {"Errors":{"emitPropertyName":"DeleteErrors"}}
    ]
},
"Delete":{
    "modify":[
        {"Objects":{"skipXmlIsSet": true}}
    ]
}

All customizations properly configured to preserve backward compatibility.


Summary

Total Breaking Changes: 1
Files With Issues: 1
Files Analyzed: 18/18

The Single Breaking Change

The only potential breaking change is in DeletedObject.DeleteMarker unmarshalling, where the unmarshaller changed from BoolUnmarshaller to NullableBoolUnmarshaller. This needs verification to ensure identical behavior.

All Other Changes Are Non-Breaking

  1. ✅ DeleteObjectsRequest - Properly split with custom partial methods
  2. ✅ DeleteObjectsResponse - Property names preserved via customizations
  3. ✅ DeleteError - All properties preserved
  4. ✅ KeyVersion - New properties are additive only
  5. ✅ Delete - New shape (non-breaking)
  6. ✅ Marshallers - Logic preserved with custom partials
  7. ✅ Unmarshallers - Structure preserved
  8. ✅ IsSet methods - Correct patterns used (!String.IsNullOrEmpty for request strings, != null for responses)
  9. ✅ MFA handling - Custom method preserved
  10. ✅ Customizations - Properly configured

Action Required

Verify that NullableBoolUnmarshaller and BoolUnmarshaller produce identical results for the DeleteMarker field in DeletedObject across all scenarios:

  • When XML contains <DeleteMarker>true</DeleteMarker>
  • When XML contains <DeleteMarker>false</DeleteMarker>
  • When XML element is missing
  • When XML element is empty <DeleteMarker/>

If these produce different results, this would be a breaking change requiring a fix.

@peterrsongg peterrsongg marked this pull request as ready for review December 17, 2025 07:48
peterrsongg added a commit that referenced this pull request Dec 19, 2025
stack-info: PR: #4240, branch: peterrsongg/petesong/phase-3-pr6/2
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