[Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model #3847

peterrsongg · 2025-05-30T03:47:15Z

Description

This is the first PR of a larger effort to generate s3 fully in a backwards compatible manner. This PR includes updates to the generator so that the generator can fully run without erroring out for S3 and necessary adjustments to generate the first three operations: AbortMultipartUpload, CreateBucketMetadataTableConfiguration, and DeleteBucket. There is a list of operations that will be included in "Phase 1", which contains many commented out operations. As more PRs are created, the operations will be uncommented.

I've added a folder in our generator .tt files which will most likely grow in size as we add more operations. Some of the marshallers have custom logic and since we cannot split a method across multiple files I had to add custom .tt files for specific operations which only trigger for that operation. (See DeleteBucketRequestCustomMarshallLogic.tt).

Motivation and Context

Testing

DRY_RUN-f27a8b08-6008-4bbc-9756-3c984c5d34a0

I also checked using beyond compare that there were no differences (other than order of generated code). For those where there are differences I will leave a comment & explanation.

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

…ion from model

peterrsongg · 2025-05-30T03:48:30Z

generator/ServiceClientGeneratorLib/GeneratorDriver.cs

@@ -207,10 +205,16 @@ public void Execute()
                string.Format("ServiceEnumerations.{0}.cs", Configuration.ClassName) : "ServiceEnumerations.cs";

            // Any enumerations for the service
-            this.ExecuteGenerator(new ServiceEnumerations(), enumFileName);
+            // skip s3 until we're at the end of s3 client generation
+            if (this.Configuration.ServiceId != "S3")


As the comment says, I'm skipping enumerations for now. None of these operations touch the enumerations and there is some shape renaming that must happen that we won't deal with in phase 1.

peterrsongg · 2025-05-30T03:48:51Z

generator/ServiceClientGeneratorLib/GeneratorDriver.cs


            // Any paginators for the service
-            if (Configuration.ServiceModel.HasPaginators)
+            // skip paginators for s3 until we're at the end of s3 client generation
+            if (Configuration.ServiceModel.HasPaginators && Configuration.ServiceId != "S3")


as the comment says, paginators will not be included in phase 1

peterrsongg · 2025-05-30T03:49:47Z

generator/ServiceClientGeneratorLib/GeneratorDriver.cs

@@ -227,24 +231,43 @@ public void Execute()

            // Do not generate base exception if this is a child model.
            // We use the base exceptions generated for the parent model.
-            if (!this.Configuration.IsChildConfig)
+            if (!this.Configuration.IsChildConfig && this.Configuration.ServiceId != "S3")


the base AmazonS3Exception class and its custom code should be left as is. There is no need to create a custom .tt file just for this one class.

peterrsongg · 2025-05-30T03:50:43Z

generator/ServiceClientGeneratorLib/GeneratorDriver.cs

@@ -1002,6 +1036,14 @@ void GenerateStructures(Operation operation)
                if (definition.IsEventStream && !Configuration.ServiceModel.Operations.Any(x => string.Equals(x.ResponseEventStreamingMember?.Shape.Name, definition.Name)))
                    continue;

+                if (this.Configuration.ServiceId == "S3")


As mentioned in the design doc, events and all eventstream based classes will be kept as custom code since SelectObjectContentEventStream has been deprecated.

peterrsongg · 2025-05-30T03:53:09Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/BaseMarshaller.tt

-				if(isGreedy)
+            if (this.Config.ServiceId == "S3" && marshallLocationName == "{Bucket}")
+                continue;
+		    if(isGreedy && this.Config.ServiceId != "S3")


We were calling TrimStart on the resource path if it is greedy, however this was causing integ tests to fail. I did a test myself of putting an object with a leading slash and then calling GetObjectACL on the object. It failed with KeyDoesn'tExist. For now I skip this TrimStart logic only for S3, but we should take a closer look at this. With the way we split the resource path now on the / and with the way we removed the leading slash trimming logic in v4, we shouldn't be doing this anymore.

peterrsongg · 2025-05-30T03:53:51Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/RestXmlRequestMarshaller.tt

@@ -65,10 +65,26 @@ namespace <#=this.Config.Namespace #>.Model.Internal.MarshallTransformations
 	ProcessHeaderMembers("publicRequest", this.Operation.RequestHeaderMembers);		
 	ProcessUriMembers("publicRequest", this.Operation);	
 	ProcessQueryStringMembers("publicRequest", this.Operation);
+	if (this.Config.ServiceId == "S3")
+	{
+		// even though RequestUri contains the bucket name in s3. In reality, the bucket Name


we already do this in the custom code, i just put the logic in the generator since we are generating s3 now.

peterrsongg · 2025-05-30T03:56:22Z

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

-        /// <returns>true, if ExpectedBucketOwner property is set.</returns>
-        internal bool IsSetExpectedBucketOwner()
-        {
-            return !String.IsNullOrEmpty(this.expectedBucketOwner);


here we are doing String.IsNullOrEmpty, we are inconsistent with the IsSet(). I think i just missed some string IsSet() when doing the s3 nullability sweep for v4. because strings are nullable by default, I was searching for long, int which aren't nullable by default. In the new generated code we just check != null. If we want to add customizations for this kinda stuff, we can.

GarrettBeatty · 2025-05-30T15:16:59Z

sdk/src/Services/S3/Generated/Model/CreateBucketMetadataTableConfigurationRequest.cs

+#pragma warning disable CS0612,CS0618,CS1570
+namespace Amazon.S3.Model
+{
+    /// <summary>


just curious when you used the file diff too? it said there werent any changes even though the whitespace and everything changed for these comments?

hmm not sure what you mean, are you saying that when you used the file diff tool it told you there weren't any changes?

GarrettBeatty · 2025-05-30T15:18:44Z

generator/ServiceClientGeneratorLib/Shape.cs

@@ -764,10 +764,10 @@ public string DeprecationMessage
            {
                string message = this.model.Customizations.GetShapeModifier(this._name)?.DeprecationMessage ??
                                 data[DeprecatedMessageKey].CastToString();
-                if (message == null)
+                if (this.model.ServiceId.Equals("S3") && message == null)


why is this needed?

Some s3 customization entries don't have a deprecated message. Might be that the deprecated message key didn't exist when we did the customization for s3. To get around that I added this check only for s3 where if the message is null we don't blow up.

GarrettBeatty · 2025-06-02T16:02:52Z

generator/ServiceClientGeneratorLib/Generators/Marshallers/BaseMarshaller.tt

            if (member.model.Customizations.SkipUriPropertyValidations.Contains(member.PropertyName))
            {
-				if(isGreedy)
+            if (this.Config.ServiceId == "S3" && marshallLocationName == "{Bucket}")
+                continue;


can you fix the indentation on this, or use brackets instead of the one line if statement? the reason being is i wasnt sure why we needed the second check for != sa3 on line 137, but thats a separate if statement and not nested. so maybe adding brackets/fix indentation will help

peterrsongg added 9 commits May 22, 2025 13:08

generator updates to run generator successfully for s3

3b35c81

commit generator changes

1db9208

add custom tt files update other tt files

d2df562

updates to restxmlresponseunmarshaller.tt

1314f40

Add custom logic for s3

fd46027

Generate AbortMultipartUpload and CreateBucketMetadataTableConfigurat…

9ba0e7c

…ion from model

Remove greedy trim start for s3

118a58f

Add DeleteBucket to generated operation and other fixes

29f5ec2

minor tweaks for pr readiness

d1e5605

peterrsongg commented May 30, 2025

View reviewed changes

peterrsongg added the v4 label May 30, 2025

GarrettBeatty self-requested a review May 30, 2025 13:39

GarrettBeatty reviewed May 30, 2025

View reviewed changes

peterrsongg mentioned this pull request May 30, 2025

[merge 3rd]Generate S3 - More delete operations #3848

Open

10 tasks

peterrsongg requested a review from dscpinheiro May 30, 2025 17:29

dscpinheiro requested a review from normj May 30, 2025 17:30

peterrsongg mentioned this pull request May 30, 2025

[MERGE 1st] Generate ListBuckets and ListDirectoryBuckets #3850

Open

10 tasks

peterrsongg changed the title ~~Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model~~ [Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model May 30, 2025

GarrettBeatty reviewed Jun 2, 2025

View reviewed changes

GarrettBeatty approved these changes Jun 2, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model #3847

[Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model #3847

Uh oh!

peterrsongg commented May 30, 2025 •

edited

Loading

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

GarrettBeatty May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

GarrettBeatty May 30, 2025

Uh oh!

peterrsongg May 30, 2025

Uh oh!

GarrettBeatty Jun 2, 2025

Uh oh!

Uh oh!

[Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model #3847

Are you sure you want to change the base?

[Merge 2nd]Generate AbortMultipartUpload, CreateBucketMetadataTableConfiguration, DeleteBucket from the model #3847

Uh oh!

Conversation

peterrsongg commented May 30, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterrsongg commented May 30, 2025 •

edited

Loading