S3 Gen Phase 2: Generate PutBucket #3909

peterrsongg · 2025-07-10T06:10:44Z

Description

This generates PutBucket. The major changes are

Added InheritAlternateBaseClass customization for PutBucketRequest to inherit from PutWithAclRequest as defined in custom
I moved PostMarshallCustomization to before the try catch block where we set the content because we could be altering the content in that method
There is some custom logic that exists for PutBucketRequestMarshaller where the user could set BucketRegion or BucketRegionName and not have to create a PutBucketConfiguration object and we would create the appropriate xml for the user. I moved that logic to the setters of both BucketRegion and BucketRegionName
I excluded some un-used members since PutBucketRequest has logic with setting ACLs

Motivation and Context

S3 Generation Phase 2.

Testing

I did some manual checks to make sure that a user could still only specify the bucket region and name and create a bucket without having to create the PutBucketConfiguration object themselves.

Assembly comparer output (empty)

C:\Scripts> .\assembly-comparison.ps1
Already on 'petesong/generate-s3-phase-2-pr1'
  AWSSDK.Core.NetStandard net8.0 succeeded (39.0s) → C:\Dev\Repos\aws-sdk-net-staging\sdk\src\Core\bin\Debug\net8.0\AWSSDK.Core.dll
  AWSSDK.S3.NetStandard net8.0 succeeded (8.8s) → bin\Debug\net8.0\AWSSDK.S3.dll

Build succeeded in 53.0s

Workload updates are available. Run `dotnet workload list` for more information.
Switched to branch 'development'
Your branch is up to date with 'public/development'.
  AWSSDK.Core.NetStandard net8.0 succeeded (5.3s) → C:\Dev\Repos\aws-sdk-net-staging\sdk\src\Core\bin\Debug\net8.0\AWSSDK.Core.dll
  AWSSDK.S3.NetStandard net8.0 succeeded (4.5s) → bin\Debug\net8.0\AWSSDK.S3.dll

Build succeeded in 10.3s

Dry_Run passes caeb33aa-9f50-4418-af23-d054dcd47197

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 · 2025-07-10T06:11:33Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/RestXmlRequestMarshaller.tt

@@ -181,17 +187,41 @@ WriteXmlAttributeString(level, member, variableName, isPayload: false);
 #>
 				if (<#=variableName + ".IsSet" + operation.RequestPayloadMember.PropertyName#>())
 				{
+<#+
+					// S3 doesn't follow the rule where if the request payload member's location name is the same as the payload member name, we marshall with the payload member's shape's marshall name instead


without this, we would incorrectly write PutBucketConfiguration as the xml element name when it should be CreateBucketConfiguration. S3 doesn't follow this rule.

normj · 2025-07-14T19:04:45Z

...s/S3/Generated/Model/Internal/MarshallTransformations/PutObjectRetentionRequestMarshaller.cs

@@ -97,7 +97,7 @@ public IRequest Marshall(PutObjectRetentionRequest publicRequest)
            {   
                if (publicRequest.IsSetRetention())
                {
-                    xmlWriter.WriteStartElement("ObjectLockRetention", "http://s3.amazonaws.com/doc/2006-03-01/");
+                    xmlWriter.WriteStartElement("Retention", "http://s3.amazonaws.com/doc/2006-03-01/");


I assume this was a benign bug that we were using the wrong root element name. I see in the docs Retention is the correct value.

yup, i also tested the operation when i did a sweep of all released operations

Copilot

Pull Request Overview

This PR adds the S3 PutBucket operation support by:

Customizing the base class of PutBucketRequest to inherit from PutWithACLRequest
Moving PostMarshallCustomization calls into the generated marshaller template for more precise timing
Implementing logic in the BucketRegion and BucketRegionName setters to auto-create a PutBucketConfiguration and removing unused members

Reviewed Changes

Copilot reviewed 9 out of 38 changed files in this pull request and generated 4 comments.

File	Description
sdk/src/Services/S3/Custom/Model/PutBucketRequest.cs	Updated inheritance, moved ACL logic to base class, added region-based configuration in property setters, removed unused properties
sdk/src/Services/S3/Custom/Model/PutBucketConfiguration.cs	Deleted manual copy of `PutBucketConfiguration` (now generated)
sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketRequestMarshaller.cs	Converted to partial class, preserved ACL header logic in `PostMarshallCustomization`
generator/ServiceClientGeneratorLib/*	Added support for `inheritAlternateBaseClass` in generator and patched the T4 marshaller template to invoke `PostMarshallCustomization`

Copilot · 2025-07-16T01:16:30Z

sdk/src/Services/S3/Custom/Model/PutBucketRequest.cs

+                        _putBucketConfiguration.LocationConstraint = bucketRegion.Value;
+                    }


The BucketRegion setter only initializes the configuration once, so updating the region a second time won’t update LocationConstraint. Consider always assigning or updating _putBucketConfiguration.LocationConstraint to reflect the latest BucketRegion.

Suggested change

_putBucketConfiguration.LocationConstraint = bucketRegion.Value;

}

}

_putBucketConfiguration.LocationConstraint = bucketRegion.Value;

Copilot · 2025-07-16T01:16:30Z

sdk/src/Services/S3/Custom/Model/PutBucketRequest.cs

            set
            {
                this.bucketRegionName = value;
+                if (bucketRegionName != "us-east-1")


When BucketRegionName is reset to "us-east-1", the existing _putBucketConfiguration isn’t cleared, potentially leaving a stale LocationConstraint. Consider clearing or resetting the configuration when returning to the default region.

Copilot · 2025-07-16T01:16:31Z

sdk/src/Services/S3/Custom/Model/PutBucketRequest.cs

+                {
+                    _putBucketConfiguration = new PutBucketConfiguration();
+                    if (bucketRegionName == "eu-west-1")
+                        _putBucketConfiguration.LocationConstraint = "EU";


[nitpick] Hard-coded region strings (e.g., "eu-west-1", "EU") can lead to typos or drift. Prefer using defined constants or the S3Region enum to ensure consistency.

Suggested change

_putBucketConfiguration.LocationConstraint = "EU";

_putBucketConfiguration.LocationConstraint = S3Region.EU.Value;

Copilot · 2025-07-16T01:16:31Z

sdk/src/Services/S3/Custom/Model/Internal/MarshallTransformations/PutBucketRequestMarshaller.cs

+            // the NoAcl logic exists because it was originally a part of the IsSetCannedACL()
+            // method https://github.com/aws/aws-sdk-net/blob/623dc261499331cb38bfec47789ddc4ef456222c/sdk/src/Services/S3/Custom/Model/PutBucketRequest.cs#L195-L198
+            if (publicRequest.IsSetCannedACL() && publicRequest.CannedACL == S3CannedACL.NoACL)
+                defaultRequest.Headers.Remove("x-amz-acl");


Use the HeaderKeys.XAmzAclHeader constant instead of a literal string to remove the ACL header, improving consistency and reducing the risk of typos.

Suggested change

defaultRequest.Headers.Remove("x-amz-acl");

defaultRequest.Headers.Remove(HeaderKeys.XAmzAclHeader);

This reverts commit 6501007.

peterrsongg added 5 commits July 9, 2025 23:03

Generate PutBucket

ec22502

Other generated changes from moving PostMarshallCustomization

2de978b

Add Devconfig

b0fec81

Remove CreateBucketConfiguration

b82db3c

Fix merge conflicts

fc7b27a

peterrsongg commented Jul 10, 2025

View reviewed changes

normj self-requested a review July 10, 2025 06:56

sankettangade requested a review from muhammad-othman July 10, 2025 17:27

normj reviewed Jul 14, 2025

View reviewed changes

normj approved these changes Jul 14, 2025

View reviewed changes

GarrettBeatty requested a review from Copilot July 16, 2025 01:15

Copilot AI reviewed Jul 16, 2025

View reviewed changes

muhammad-othman approved these changes Jul 18, 2025

View reviewed changes

peterrsongg merged commit 6501007 into development Jul 21, 2025
6 checks passed

peterrsongg deleted the petesong/generate-s3-phase-2-pr1 branch July 21, 2025 15:21

peterrsongg added a commit that referenced this pull request Jul 21, 2025

Revert "S3 Gen Phase 2: Generate PutBucket (#3909)"

c055f73

This reverts commit 6501007.

peterrsongg added a commit that referenced this pull request Jul 21, 2025

Revert "S3 Gen Phase 2: Generate PutBucket (#3909)" (#3925)

44e692b

This reverts commit 6501007.

peterrsongg restored the petesong/generate-s3-phase-2-pr1 branch July 21, 2025 19:56

peterrsongg mentioned this pull request Jul 22, 2025

Generate PutBucket #3928

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

S3 Gen Phase 2: Generate PutBucket #3909

S3 Gen Phase 2: Generate PutBucket #3909

Uh oh!

peterrsongg commented Jul 10, 2025 •

edited

Loading

Uh oh!

peterrsongg Jul 10, 2025

Uh oh!

normj Jul 14, 2025

Uh oh!

peterrsongg Jul 17, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jul 16, 2025

Uh oh!

Copilot AI Jul 16, 2025

Uh oh!

Copilot AI Jul 16, 2025

Uh oh!

Copilot AI Jul 16, 2025

Uh oh!

Uh oh!

Uh oh!

		_putBucketConfiguration.LocationConstraint = bucketRegion.Value;
		}

	defaultRequest.Headers.Remove("x-amz-acl");
	defaultRequest.Headers.Remove(HeaderKeys.XAmzAclHeader);

S3 Gen Phase 2: Generate PutBucket #3909

S3 Gen Phase 2: Generate PutBucket #3909

Uh oh!

Conversation

peterrsongg commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

Uh oh!

peterrsongg Jul 10, 2025

Choose a reason for hiding this comment

Uh oh!

normj Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

peterrsongg Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jul 16, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

peterrsongg commented Jul 10, 2025 •

edited

Loading