Phase 2 final operations #3949
Phase 2 final operations #3949
Conversation
| ListObjectsV2Response response = new ListObjectsV2Response(); | ||
|
|
||
| while (context.Read()) | ||
| //https://github.com/aws/aws-sdk-net/blob/79cbc392fc3f1c74fcdf34efd77ad681da8af328/sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/ListObjectsV2ResponseUnmarshaller.cs#L75-L87 |
There was a problem hiding this comment.
i included the permalink here so we know not to get rid of this. This is why we need the InjectXmlUnmarshallCustomcode customization
| @@ -495,20 +495,6 @@ using Amazon.Runtime.Internal.Util; | |||
|
|
|||
| protected void GenerateRequestChecksumHandling(Operation operation, string requestContent) | |||
| { | |||
| // If the request has a Content-MD5 property and it's set by the user, copy the value to the header. | |||
There was a problem hiding this comment.
This change is so that we don't set the content-md5 header twice. I ran the generator for s3 control and confirmed that nothing changed.
We also have this code https://github.com/aws/aws-sdk-net-staging/blob/main/sdk/src/Core/Amazon.Runtime/Internal/Util/ChecksumUtils.cs#L157 which won't calculate the md5 checksum if one is provided, so we can be confident it won't overwrite it. If one isn't provided we will auto-calculate in the checksum handler.
| <#+ | ||
| if (!string.IsNullOrEmpty(member.Shape.XmlNamespace)) |
There was a problem hiding this comment.
For the Grantee member of Grant S3 expects the namespace and prefix to be written, so we need to check the shape's xmlNamespace since the xmlNamespace data is on the shape and not the member. The protocol test models usually put the xmlNamespace data on the member, but S3 doesn't seem to do that so I made the update here.
| @@ -33,7 +33,7 @@ private IEnumerable<string> GetServiceModelEnumValues(string enumName) | |||
| public void AssertConstantsMatch(IReflect constantClassType, string enumName) | |||
| { | |||
| var constantClassValues = new HashSet<string>(GetConstantClassValues(constantClassType)); | |||
| var serviceModelEnumValues = new HashSet<string>(GetServiceModelEnumValues(enumName)); | |||
| var serviceModelEnumValues = string.IsNullOrEmpty(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)) ? GetServiceModelEnumValues(enumName) : GetServiceModelEnumValues(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)); | |||
There was a problem hiding this comment.
Since ObjectStorageClass was renamed to S3StorageClass, the model thinks that ObjectStorageClass doesn't exist. We have to account for this case by checking for the renamed shape instead.
There was a problem hiding this comment.
Pull Request Overview
This PR generates new AWS S3 operations and adds customization hooks to the generator to support additional marshalling/unmarshalling scenarios. The changes enable generation of GetObjectAcl, HeadObject, ListObjectVersions, PutBucketAcl, PutBucketCors, PutObjectAcl, and ListObjectsV2 operations by adding necessary customizations and refactoring existing custom code to work with generated models.
Key changes include:
- Extended the generator with new customization hooks for XML unmarshalling code injection
- Refactored S3 custom models to use
partialclasses to integrate with generated code - Updated generator logic to support excluding properties from unmarshalling and adding custom post-unmarshalling methods
Reviewed Changes
Copilot reviewed 27 out of 125 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/test/UnitTests/Custom/ConstantClassTestBase.cs | Updated to handle shape name overrides for enum constants |
| sdk/src/Services/S3/Custom/Model/S3ObjectVersion.cs | Converted to partial class and removed generated properties |
| sdk/src/Services/S3/Custom/Model/S3Object.cs | Converted to partial class and removed generated properties |
| sdk/src/Services/S3/Custom/Model/S3AccessControlList.cs | Converted to partial class and updated field references |
| sdk/src/Services/S3/Custom/Model/ListVersionsResponse.cs | Removed custom implementation (now generated) |
| Multiple unmarshaller files | Converted to partial classes or removed (now generated) |
| sdk/src/Services/S3/Custom/Model/GetObjectMetadataResponse.cs | Converted to partial class and removed generated properties |
| sdk/src/Services/S3/Custom/Model/GetObjectMetadataRequest.cs | Converted to partial class and removed generated properties |
| generator/ServiceModels/s3/s3.customizations.json | Added extensive customizations for new operations |
| generator/ServiceClientGeneratorLib/ServiceModel.cs | Enabled new S3 operations in allow list |
| Multiple generator files | Added support for new customization features |
| @@ -33,7 +33,7 @@ private IEnumerable<string> GetServiceModelEnumValues(string enumName) | |||
| public void AssertConstantsMatch(IReflect constantClassType, string enumName) | |||
| { | |||
| var constantClassValues = new HashSet<string>(GetConstantClassValues(constantClassType)); | |||
| var serviceModelEnumValues = new HashSet<string>(GetServiceModelEnumValues(enumName)); | |||
| var serviceModelEnumValues = string.IsNullOrEmpty(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)) ? GetServiceModelEnumValues(enumName) : GetServiceModelEnumValues(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)); | |||
There was a problem hiding this comment.
[nitpick] This ternary operator creates a complex nested call that's difficult to read. Consider extracting the override shape name to a variable first for better readability.
| var serviceModelEnumValues = string.IsNullOrEmpty(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)) ? GetServiceModelEnumValues(enumName) : GetServiceModelEnumValues(_serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName)); | |
| var overrideShapeName = _serviceModelUnderTest.Customizations.GetOverrideShapeName(enumName); | |
| var serviceModelEnumValues = string.IsNullOrEmpty(overrideShapeName) | |
| ? GetServiceModelEnumValues(enumName) | |
| : GetServiceModelEnumValues(overrideShapeName); |
Copilot uses AI. Check for mistakes.
| { | ||
| foreach (var grant in grantList) | ||
| foreach (var grant in _grants) |
There was a problem hiding this comment.
Same issue as above - using '_grants' field name that doesn't appear to be defined in this partial class. This will likely cause a compilation error.
Copilot uses AI. Check for mistakes.
Description
Generator + custom code changes - 4642adf
Generated Changes - e19e49d
Generates the following operations
I had to add more customization hooks to get this to generate correctly. For ListObjectsV2 and ListObjectVersions we were doing some weird stuff where we would change the type of a member and use a different member's unmarshaller to unmarhshall it. I'll make a comment in the PR to make it clear, but that is why I had to add the InjectXmlUnmarshallCode customization, so that we can inject some custom code into the unmarshallers.
I purposefully left out
UploadPartCopybecause I want to give that its own PR because it seems like too much to fit into this PR.Motivation and Context
Testing
DRY_RUN-2ff2f349-de26-4a96-a09c-1858198f835f passed (08/07/2025)
The backwards compatibility comparer only output new methods/new types added which is backwards compatible.
Screenshots (if appropriate)
Types of changes
Checklist
License