-
Notifications
You must be signed in to change notification settings - Fork 869
Generate copy part #3951
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Generate copy part #3951
Conversation
@@ -1087,20 +1086,6 @@ public ShapeModifier(JsonData data) | |||
// } | |||
// }, | |||
// Above is an example of how to specify the originalMember on the injected member. | |||
private void Validate(JsonData data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this when I was working on splitting GetACL PutACL into their respective GetObjectACL and GetBucketACL a while back because I thought if we were excluding a member we would always be injecting another member and referencing the original member that the injected member is referencing. However, as I started to generate more S3 operations I realize that this is not always the case. Sometimes we exclude a member and inject a member but that member doesn't reference an "originalMember" at all.
|
There was a problem hiding this 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 generates the CopyPart (UploadPartCopy) operation for the AWS S3 SDK, adding support for multipart upload copying functionality. The implementation includes both request marshalling and response unmarshalling components, along with customizations to handle S3-specific requirements.
- Generate CopyPart operation code from service model
- Refactor existing custom code to use generated components with partial classes
- Update customizations to support flattened shapes and proper marshalling
Reviewed Changes
Copilot reviewed 12 out of 16 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
CopyPartResponseUnmarshaller.cs | Removes entire custom unmarshaller implementation to allow generation |
CopyPartRequestMarshaller.cs | Converts to partial class and moves logic to PostMarshallCustomization method |
CopyPartResponse.cs | Converts to partial class, removing most properties to allow generation |
CopyPartRequest.cs | Converts to partial class, keeping only custom range properties |
s3.customizations.json | Adds extensive customizations for UploadPartCopy operation marshalling |
ServiceModel.cs | Enables UploadPartCopy operation in S3 allow list |
RestXmlResponseUnmarshaller.tt | Adds support for flattened shapes in response unmarshalling |
BaseResponseUnmarshaller.tt | Adds check to skip flattened shape members during unmarshalling |
Customizations.cs | Removes validation that was blocking the customization approach |
Comments suppressed due to low confidence (1)
generator/ServiceClientGeneratorLib/Customizations.cs:1069
- Removing the validation entirely could allow invalid customizations to pass through undetected. Consider replacing this with a more flexible validation that handles the specific case being addressed rather than removing all validation.
}
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -27,92 +27,8 @@ namespace Amazon.S3.Model.Internal.MarshallTransformations | |||
/// <summary> | |||
/// Upload Part Copy Request Marshaller | |||
/// </summary> | |||
public class CopyPartRequestMarshaller : IMarshaller<IRequest, CopyPartRequest> ,IMarshaller<IRequest,Amazon.Runtime.AmazonWebServiceRequest> | |||
public partial class CopyPartRequestMarshaller : IMarshaller<IRequest, CopyPartRequest> ,IMarshaller<IRequest,Amazon.Runtime.AmazonWebServiceRequest> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] There should be a space after the comma in the interface implementation list for consistency with C# formatting conventions.
public partial class CopyPartRequestMarshaller : IMarshaller<IRequest, CopyPartRequest> ,IMarshaller<IRequest,Amazon.Runtime.AmazonWebServiceRequest> | |
public partial class CopyPartRequestMarshaller : IMarshaller<IRequest, CopyPartRequest>, IMarshaller<IRequest, Amazon.Runtime.AmazonWebServiceRequest> |
Copilot uses AI. Check for mistakes.
"type": "patch", | ||
"changeLogMessages": [ | ||
"Generate CopyPart.", | ||
"[Breaking Change] Amazon.S3.Model.CopyPartResponse's BucketKeyEnabled will not return bool? instead of bool, in line with other S3 nullable types." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if its breaking change should it be minor
or what have we been doing for other breaking changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the other ones were also just patches so i made this a patch. I think it is okay because we call it out explicitly here and it is what all the other services are doing, we just missed S3 b/c it is handcoded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should make it minor
.
} | ||
], | ||
"exclude": [ | ||
"CopySource", | ||
"Key" | ||
"Key", | ||
"CopySourceRange" | ||
], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this need any changes? not really 100% sure what its saying but please confirm @peterrsongg (see copilots other comment)
"type": "patch", | ||
"changeLogMessages": [ | ||
"Generate CopyPart.", | ||
"[Breaking Change] Amazon.S3.Model.CopyPartResponse's BucketKeyEnabled will not return bool? instead of bool, in line with other S3 nullable types." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that we should make it minor
.
im gonna hold off on merging until the bug fix is out so we don't have 2 s3 changes going out at once |
Description
Generates CopyPart (UploadPartCopy)
Motivation and Context
Testing
DRY_RUN-af4c3b28-315f-4b35-bbda-b8fe124c50cd
1 breaking change (bool => bool?) which I call out
Screenshots (if appropriate)
Types of changes
Checklist
License