Split GetACL into GetBucketACL and GetObject ACL and the same for PutACL #3408

peterrsongg · 2024-07-30T21:14:16Z

Description

GetACL and PutACL in the SDK is a combination of GetBucketACL and PutBucketACL and there is a customization made to rename the method and exclude the GetObjectACL methods. This PR obsoletes the existing GetACL/PutACL operations and adds the GetBucketACL and GetObjectACL / PutBucketACL and PutObjectACL operations. It also adds the handwritten custom marshallers/request classes etc. and refactors any existing unit/integ test to use the new operations.

I put the GetACL and PutACL into a custom partial AmazonS3Client and did the same thing for IAmazonS3.

Motivation and Context

Currently GetACL and PutACL do not read the contextParams from the model, so it is possible that in the future if these contextParams are used in the endpoint ruleset then we could resolve an incorrect endpoint. Splitting the operations ensures that the contextParams are used in the EndpointResolver.
Parity with other SDKs + maintanability.

Testing

Dry run passes.
Added an integ test.
Also did a manual test and made sure that the old method is still callable.

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

muhammad-othman

This approach seems a bit complicated IMO. What about removing GetACL and PutACL from the generator and the models, and add them and add them manually?

peterrsongg · 2024-08-07T17:13:19Z

This approach seems a bit complicated IMO. What about removing GetACL and PutACL from the generator and the models, and add them and add them manually?

I agree that it is a bit complicated, but unfortunately it is just the way that customizations work in the generator, especially renames. All it does is point to the original json object in the model but renames the operation. We need to create a copy of this operation basically with the original name to be able to generate both GetACL and GetBucketACL in the client file.

I was thinking maybe we could use this functionality in the future? but you're probably right, it is better to manually add them, and delete this added complexity to the generator. I will see what the team thinks

dscpinheiro · 2024-08-09T16:33:02Z

sdk/test/Services/S3/IntegrationTests/PutObjectTests.cs

+            }
+            finally 
+            {                 
+                AmazonS3Util.DeleteS3BucketWithObjects(Client, bucketName);


I don't think this finally is correct, the bucketName variable here doesn't refer to the bucket you created but the private static variable with the same name:

aws-sdk-net/sdk/test/Services/S3/IntegrationTests/PutObjectTests.cs

Line 33 in e83c9f2

private static string bucketName;

I'd either use a different variable name in this method or update it not to create a new bucket at all (you'd need to confirm changing the ACLs doesn't impact the other tests here - as they all share the same bucket).

thank you for pointing that out. I moved the variable outside the try block and renamed it. I also just ran the integ test and confirmed it is deleting the right bucket.

…CL into PutObjectACL and PutBucketACL. Obsolete PutACL and GetACL.

muhammad-othman · 2024-08-14T19:30:40Z

Do we have to create this new custom S3 client? What about using the existing extension S3 client file instead?

peterrsongg · 2024-08-14T21:08:27Z

Do we have to create this new custom S3 client? What about using the existing extension S3 client file instead?

Nope, I just moved them to the extension file instead

dscpinheiro

I have one general question too: Is there a reason why we're using lowercase for the operation names / request objects? For example, the changelog (and PR title) mention GetACL into GetBucketACL and GetObjectACL but then the classes are named GetBucketAcl and GetObjectAcl.

dscpinheiro · 2024-08-16T13:16:18Z

generator/ServiceClientGeneratorLib/Operation.cs

+
+

What are these extra lines for?

generator/ServiceClientGeneratorLib/Generators/Endpoints/EndpointResolver.tt

dscpinheiro · 2024-08-16T13:18:28Z

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

@@ -271,6 +271,7 @@ namespace Amazon.S3.Model
    /// </para>
    ///  </li> </ul>
    /// </summary>
+    /// 


Remove this extra line.

dscpinheiro · 2024-08-16T13:27:58Z

sdk/src/Services/S3/AWSSDK.S3.NetStandard.csproj

@@ -82,8 +82,6 @@
            <PrivateAssets>all</PrivateAssets>
        </PackageReference>
  </ItemGroup>
-  <ItemGroup>


This <ItemGroup> removal seems odd, I don't see any generator changes included in this PR that could've caused this.

this was a manual change I made on accident😅, rerunning the generator removed this.

dscpinheiro · 2024-08-16T13:30:05Z

sdk/src/Services/S3/Custom/_bcl/IAmazonS3.cs

+    {
+        #region GetACL
+        [Obsolete("GetACL combines both GetBucketAcl and GetObjectAcl and is deprecated. Please use the separated GetObjectAcl and GetBucketAcl operations.")]
+        Task<GetACLResponse> GetACLAsync(string bucketName, System.Threading.CancellationToken cancellationToken = default(CancellationToken));


This method is missing documentation. Also, since this is the bcl folder, shouldn't you also have to include the non-async methods?

good call, i did a manual test and the non-async methods were callable so I thought I was in the clear, but based on the pattern the other methods follow it looks like I missed adding it here.

dscpinheiro · 2024-08-16T13:33:32Z

generator/.DevConfigs/4f49cfc6-d4a8-477c-a685-6b96d52d73ab.json

+            "serviceName": "S3",
+            "type": "patch",
+            "changeLogMessages": [
+                "Split GetACL into GetBucketACL and GetObjectACL. Split PutACL into PutObjectACL and PutBucketACL. Obsolete PutACL and GetACL"


I'd expand this message a bit more (for example, if this goes into the V4 tracker issue, is there enough information for customers to understand why we're doing this?)

peterrsongg · 2024-08-16T21:31:33Z

I have one general question too: Is there a reason why we're using lowercase for the operation names / request objects? For example, the changelog (and PR title) mention GetACL into GetBucketACL and GetObjectACL but then the classes are named GetBucketAcl and GetObjectAcl.

Yeah so when we did the customization for GetACL and PutACL we renamed it to use capital ACL, but generating from the model the operation is generated as GetAcl and PutAcl. I think the name of the files should reflect what is modeled so I will change those to Acl

peterrsongg · 2024-08-18T02:20:46Z

Addressed all comments and the PR is ready for another pass (dry-run also succeeded)

dscpinheiro · 2024-08-20T12:30:47Z

generator/.DevConfigs/4f49cfc6-d4a8-477c-a685-6b96d52d73ab.json

+        {
+            "serviceName": "S3",
+            "type": "patch",
+            "changeLogMessages": [


No need to change, but since this is an array you could also split this into multiple messages so it's easier to read. Like this:

"changeLogMessages": [ "Split `GetACL` into `GetBucketAcl` and `GetObjectAcl`", "Split `PutACL` into `PutObjectACL` and `PutBucketACL`", "Obsolete `PutACL` and `GetACL`: These operations are being split ..." ]

peterrsongg added the v4 label Jul 30, 2024

ashovlin requested review from muhammad-othman and boblodgett July 31, 2024 17:28

muhammad-othman reviewed Aug 7, 2024

View reviewed changes

peterrsongg force-pushed the petesong/split-acl branch 2 times, most recently from 2eb83db to e83c9f2 Compare August 9, 2024 15:58

dscpinheiro reviewed Aug 9, 2024

View reviewed changes

refactor: split GetACL into GetBucketACL and GetObjectACL. Split PutA…

aa5ea9f

…CL into PutObjectACL and PutBucketACL. Obsolete PutACL and GetACL.

peterrsongg force-pushed the petesong/split-acl branch from e83c9f2 to aa5ea9f Compare August 9, 2024 17:03

peterrsongg requested review from dscpinheiro and muhammad-othman and removed request for boblodgett August 9, 2024 20:31

Move deprecated operations to AmazonS3Client.extensions

e43a521

muhammad-othman approved these changes Aug 15, 2024

View reviewed changes

dscpinheiro reviewed Aug 16, 2024

View reviewed changes

Address PR feedback

9fe943f

dscpinheiro reviewed Aug 20, 2024

View reviewed changes

dscpinheiro approved these changes Aug 20, 2024

View reviewed changes

Update DevConfig changelog message into array

b5cea36

peterrsongg merged commit 6c740df into v4-development Aug 20, 2024
1 check passed

peterrsongg deleted the petesong/split-acl branch August 20, 2024 16:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split GetACL into GetBucketACL and GetObject ACL and the same for PutACL #3408

Split GetACL into GetBucketACL and GetObject ACL and the same for PutACL #3408

peterrsongg commented Jul 30, 2024 •

edited

Loading

muhammad-othman left a comment

peterrsongg commented Aug 7, 2024

dscpinheiro Aug 9, 2024

peterrsongg Aug 9, 2024

muhammad-othman commented Aug 14, 2024

peterrsongg commented Aug 14, 2024

dscpinheiro left a comment

dscpinheiro Aug 16, 2024

dscpinheiro Aug 16, 2024

dscpinheiro Aug 16, 2024

peterrsongg Aug 19, 2024

dscpinheiro Aug 16, 2024

peterrsongg Aug 19, 2024

dscpinheiro Aug 16, 2024

peterrsongg commented Aug 16, 2024

peterrsongg commented Aug 18, 2024

dscpinheiro Aug 20, 2024

Split GetACL into GetBucketACL and GetObject ACL and the same for PutACL #3408

Split GetACL into GetBucketACL and GetObject ACL and the same for PutACL #3408

Conversation

peterrsongg commented Jul 30, 2024 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

License

muhammad-othman left a comment

Choose a reason for hiding this comment

peterrsongg commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muhammad-othman commented Aug 14, 2024

peterrsongg commented Aug 14, 2024

dscpinheiro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peterrsongg commented Aug 16, 2024

peterrsongg commented Aug 18, 2024

Choose a reason for hiding this comment

peterrsongg commented Jul 30, 2024 •

edited

Loading