Generate S3 CopyObject operation #4234
Conversation
There was a problem hiding this comment.
Pull request overview
This PR generates the S3 CopyObject operation by migrating from custom implementation to generated code. The changes consolidate marshalling logic, refactor ACL handling, and add comprehensive customizations for the operation's request and response models.
Key changes:
- Migrated
CopyObjectoperation from custom to generated implementation - Refactored ACL request marshalling into reusable
HeaderACLRequestMarshallerutility - Added extensive customizations in service model configuration for property mappings and marshalling behavior
Reviewed changes
Copilot reviewed 10 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ObjectNotInActiveTierErrorException.cs | New exception class for objects not in active tier |
| ObjectNotInActiveTierErrorExceptionUnmarshaller.cs | Unmarshaller for the new exception type |
| CopyObjectResponseUnmarshaller.cs (Generated) | Generated response unmarshaller with header processing |
| CopyObjectRequestMarshaller.cs (Generated) | Generated request marshaller with customization hooks |
| CopyObjectResponse.cs (Generated) | Generated response model with all properties |
| CopyObjectRequest.cs (Generated) | Generated request model with extensive documentation |
| PutBucketRequestMarshaller.cs | Refactored to use shared ACL marshaller |
| HeaderACLRequestMarshaller.cs | Added null checks for grants and permissions |
| CopyObjectResponseUnmarshaller.cs (Custom - deleted) | Removed custom implementation |
| CopyObjectRequestMarshaller.cs (Custom - refactored) | Reduced to customization hook calling shared utilities |
| CopyObjectResponse.cs (Custom - deleted) | Removed custom implementation |
| CopyObjectRequest.cs (Custom - deleted) | Removed custom implementation |
| s3.customizations.json | Added extensive CopyObject customizations for property mappings and marshalling |
| ServiceModel.cs | Added CopyObject to S3 allow list operations |
| Operation.cs | Added exclusion logic for marshalling properties |
| BaseMarshaller.tt | Added support for custom marshalling code injection and skipXmlIsSet |
Comments suppressed due to low confidence (1)
generator/ServiceClientGeneratorLib/Generators/Marshallers/BaseMarshaller.tt:1
- The opening brace on line 42 is not closed before the conditional block starting at line 48. This creates mismatched braces in the template logic flow.
<#@ template language="C#" inherits="Generators.BaseGenerator" #>
muhammad-othman
force-pushed
the
muhamoth/S3-generate-CopyObject-operation
branch
3 times, most recently
from
00aa82f to
1aa15bd
Compare
Breaking Change Analysis for CopyObject Operation Migration (Commit 1aa15bd)SummaryAfter detailed analysis of the CopyObject operation migration from custom to generated code, I have identified ZERO breaking changes. The migration appears to be backward compatible. Files Analyzed (22 total)
Detailed AnalysisCopyObjectRequest ModelPublic Properties: All custom properties preserved in generated version
Private Field Names Changed:
CopyObjectResponse ModelAll Properties Preserved:
CopyObjectRequestMarshallerLogic Split Between Generated + Custom Partial:
CopyObjectResponseUnmarshallerGenerated Version Includes All Original Logic:
FilterRule.csMinor Change:
New Exception Type
Key Findings✅ NO BREAKING CHANGES FOUND
New Functionality Added (Non-Breaking)
ConclusionThe migration of CopyObject operation from custom to generated code is CLEAN with ZERO breaking changes. The team successfully:
Files Analyzed: 22 of 22 (100%) |
muhammad-othman
force-pushed
the
muhamoth/S3-generate-PutBucketTagging-operation
branch
from
9a887f5 to
89357ed
Compare
muhammad-othman
force-pushed
the
muhamoth/S3-generate-CopyObject-operation
branch
from
1aa15bd to
b20a63d
Compare
| { | ||
| <#+ | ||
| if (member.CustomMarshallerTransformation != null) | ||
| } |
There was a problem hiding this comment.
you can use this method instead of looping through yourself
| foreach (var code in propertyModifier.InjectXmlUnmarshallCode) | ||
| { | ||
| #> | ||
| <#=code#> |
There was a problem hiding this comment.
same comment as above( there is also a method above the one i linked for exactly this scenario)
| "return !System.String.IsNullOrEmpty(this._copySourceServerSideEncryptionCustomerProvidedKey);" | ||
| ], | ||
| "injectXmlMarshallCode": [ | ||
| "request.Headers.Add(Amazon.Util.HeaderKeys.XAmzCopySourceSSECustomerKeyHeader, publicRequest.CopySourceServerSideEncryptionCustomerProvidedKey);", |
There was a problem hiding this comment.
for multi-line injects like this can you move this logic to a custom method and then just call the custom method here? That is what we have been doing for multi-liners.
| "return !System.String.IsNullOrEmpty(this._serverSideEncryptionCustomerProvidedKey);" | ||
| ], | ||
| "injectXmlMarshallCode": [ | ||
| "request.Headers.Add(Amazon.Util.HeaderKeys.XAmzSSECustomerKeyHeader, publicRequest.ServerSideEncryptionCustomerProvidedKey);", |
| { | ||
| "Metadata":{ | ||
| "injectXmlPropertyGetter":[ | ||
| "get {", |
There was a problem hiding this comment.
same comment, can you define a method and call the method here?
| "}" | ||
| ], | ||
| "injectXmlMarshallCode": [ | ||
| "var headers = publicRequest.Headers;", |
| { | ||
| "TaggingDirective": { | ||
| "injectXmlMarshallCode": [ | ||
| "if (publicRequest.TaggingDirective == TaggingDirective.REPLACE)", |
|
|
||
| <# | ||
| } | ||
| if (this.Config.ServiceModel.Customizations.TryGetPropertyModifier(member.OwningShape.Name, member.ModeledName, out var propertyModifier) && propertyModifier.SkipXmlIsSet) |
There was a problem hiding this comment.
curious, what did you need this for?
| /// </summary> | ||
| public partial class PutBucketRequestMarshaller : IMarshaller<IRequest, PutBucketRequest> ,IMarshaller<IRequest,Amazon.Runtime.AmazonWebServiceRequest> | ||
| { | ||
| protected internal static void ConvertPutWithACLRequest(PutWithACLRequest request, IRequest irequest) |
There was a problem hiding this comment.
So this was just the same thing as HeaderACLRequest.Marshall?
| { | ||
| request.Headers["x-amz-grant-write-acp"] = publicRequest.GrantWriteACP; | ||
| } | ||
| var headers = publicRequest.Headers; |
There was a problem hiding this comment.
acknowledging that this will preserve previous behavior where properties such as ContentLanguage didn't exist and won't override the HeadersCollection. This essentially overwrites those new members. Is this something you noticed?
I wonder though if it's worth just excluding those members altogether to prevent confusion. I don't feel too strongly about this but right now we are basically writing it twice.
2 participants
Description
Generates
CopyObjectS3 operationNew properties were added to the request.
Motivation and Context
DOTNET-8422Testing
DRY_RUN-68d245f4-de50-4bde-b7b5-25367b3daa55.Screenshots (if appropriate)
Types of changes
Checklist
License