Generate DelegatingS3Client #3745

davidh44 · 2023-02-04T01:23:02Z

Part of S3 Encryption Client Integration

Motivation and Context

We have an existing DelegatingS3AsyncClient, but not one for Sync client

Modifications

Testing

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

Checklist

I have read the CONTRIBUTING document
Local run of mvn install succeeds
My code follows the code style of this project
My change requires a change to the Javadoc documentation
I have updated the Javadoc documentation accordingly
I have added tests to cover my changes
All new and existing tests passed
I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.
My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

I confirm that this pull request can be released under the Apache 2 license

millems · 2023-02-06T17:48:59Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingAsyncClientClass.java

@@ -53,7 +53,7 @@ public TypeSpec poetSpec() {
        TypeSpec.Builder result = PoetUtils.createClassBuilder(className);

        result.addSuperinterface(interfaceClass)
-              .addAnnotation(SdkInternalApi.class)
+              .addAnnotation(SdkProtectedApi.class)


This would need to be an SdkPublicApi if it's going to be used by someone other than our team (e.g. the crypto tools teams).

Updated to PublicApi

millems · 2023-02-06T17:50:21Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+        TypeSpec.Builder result = PoetUtils.createClassBuilder(className);
+
+        result.addSuperinterface(interfaceClass)
+              .addAnnotation(SdkProtectedApi.class)


Same here: SdkPublicApi?

millems · 2023-02-06T17:50:48Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+
+        result.addSuperinterface(interfaceClass)
+              .addAnnotation(SdkProtectedApi.class)
+              .addModifiers(Modifier.ABSTRACT, Modifier.PUBLIC)


Does it need to be abstract? Are there unimplemented methods?

I think all methods are implemented except serviceName. One benefit of using abstract is that people can't initialize it directly, which is not the intended use-case.

serviceName() in SdkClient is unimplemented

Any reason not to implement serviceName()? I feel like it shouldn't differ in subclasses?

Also, should we create a subclass of the delegating class in our codegen generated classes test module, so that if we fail to override a method, it'll fail compilation in our project instead of in our customer's?

implemented serviceName in DelegatingSyncClientClass

millems · 2023-02-06T17:52:48Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+              .addField(FieldSpec.builder(interfaceClass, "delegate")
+                                 .addModifiers(Modifier.PROTECTED, Modifier.FINAL)
+                                 .build())


Nit: Should we encapsulate this field and let them use a method for accessing it (e.g. delegate())? It's probably not a big deal for this class, since I don't expect internals of this one to change, but I'm being picky here so that we don't start thinking this is great practice in other contexts.

makes sense, encapsulated field and added delegate() method

millems · 2023-02-06T17:57:05Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+        if (model.getCustomizationConfig().getUtilitiesMethod() != null) {
+            result.addMethod(utilitiesMethod());
+        }


This pattern sets off some warning signs in my mind: If we add something like utilities or waiters in the future, how will we remember to add them here as well? I'm worried we'd have to wait for someone to report that we forgot to implement them. That seems likely.

Can we refactor things so that isn't possible? I can think of some patterns, but you know the code better, so you might have better ideas.

Refactored and made poetSpec final in SyncClientInterface as discussed. Created various methods that are called in poetSpec

millems · 2023-02-06T17:58:26Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+        return MethodSpec.constructorBuilder()
+                         .addModifiers(Modifier.PUBLIC)
+                         .addParameter(interfaceClass, "delegate")
+                         .addStatement("this.delegate = delegate")


Should we Validate.paramNotNull(delegate, "delegate")?

added Validate in constructor

millems · 2023-02-08T00:25:41Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingAsyncClientClass.java


 public class DelegatingAsyncClientClass extends AsyncClientInterface {

+    private static final String DELEGATE = "delegate";


This feels kind of silly. Should we just inline it? I think the principal of not having magic constants applies to where the constants aren't self-describing. E.g. 24 instead of HOURS_PER_DAY. This particular case feels redundant.

reverted back to inline

millems · 2023-02-08T00:33:12Z

codegen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingSyncClientClass.java

+        if (opModel.hasStreamingInput() || opModel.hasStreamingOutput()) {
+            String variableName = opModel.hasStreamingInput() ? SYNC_STREAMING_INPUT_PARAM : SYNC_STREAMING_OUTPUT_PARAM;
+            return builder.addStatement("return delegate.$N($N, $N)",
+                                        opModel.getMethodName(),
+                                        opModel.getInput().getVariableName(),
+                                        variableName);
+        }
+
+        if (opModel.hasEventStreamInput() && opModel.hasEventStreamOutput()) {
+            return builder.addStatement("return delegate.$N($N, $N, $N)",
+                                        opModel.getMethodName(),
+                                        opModel.getInput().getVariableName(),
+                                        EVENT_PUBLISHER_PARAM_NAME,
+                                        SYNC_STREAMING_OUTPUT_PARAM);
+        }
+
+        if (opModel.hasEventStreamInput() || opModel.hasEventStreamOutput()) {
+            String variableName = opModel.hasEventStreamInput() ? EVENT_PUBLISHER_PARAM_NAME : EVENT_RESPONSE_HANDLER_PARAM_NAME;
+            return builder.addStatement("return delegate.$N($N, $N)",
+                                        opModel.getMethodName(),
+                                        opModel.getInput().getVariableName(),
+                                        variableName);
+        }


nit: any way to just copy the delegate method name and parameters from the incoming MethodSpec.Builder, since the caller already has it figured out?

Something like:

builder.addStatement("return delegate.$N($L)", opModel.getMethodName(), builder.parameters.stream().map(p -> p.name).collect(joining(", "));

(I haven't tested this, I could be way off).

that worked seamlessly! :)

millems · 2023-02-09T20:39:26Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingAsyncClientClass.java

+        MethodSpec delegate = MethodSpec.methodBuilder("delegate")
+                                        .addModifiers(PUBLIC)
+                                        .addStatement("return this.delegate")
+                                        .returns(SdkClient.class)
+                                        .build();


nit: should this and the constructor be in addAdditionalMethods?

moved to addAdditionalMethods, same for DelegatingSyncClientClass

millems · 2023-02-09T20:39:38Z

...gen/src/main/java/software/amazon/awssdk/codegen/poet/client/DelegatingAsyncClientClass.java

+            .addField(FieldSpec.builder(interfaceClass, "delegate")
+                               .addModifiers(PRIVATE, FINAL)
+                               .build())


nit: should this be in addFields?

moved to addFields, same for DelegatingSyncClientClass

millems · 2023-02-09T20:44:10Z

...t/resources/software/amazon/awssdk/codegen/poet/client/test-abstract-async-client-class.java

-        return delegate.streamingInputOutputOperation(streamingInputOutputOperationRequest, requestBody);
+        return delegate
+            .streamingInputOutputOperation(streamingInputOutputOperationRequest, requestBody, asyncResponseTransformer);


Oops, looks like some of these were broken before?

yup i noticed they were off in the diff

millems · 2023-02-09T20:44:38Z

...t/resources/software/amazon/awssdk/codegen/poet/client/test-abstract-async-client-class.java

-    public void close() {
-        delegate.close();
+    public final String serviceName() {
+        return SERVICE_NAME;


should we delegate the serviceName method?

updated to return delegate.serviceName()

millems · 2023-02-09T20:45:28Z

...st/resources/software/amazon/awssdk/codegen/poet/client/test-abstract-sync-client-class.java

+    /**
+     * <p>
+     * Performs a post operation to the query service and has no output
+     * </p>
+     *
+     * @param aPostOperationRequest
+     * @return Result of the APostOperation operation returned by the service.
+     * @throws InvalidInputException
+     *         The request was rejected because an invalid or out-of-range value was supplied for an input parameter.
+     * @throws SdkException
+     *         Base class for all exceptions that can be thrown by the SDK (both service and client). Can be used for
+     *         catch all scenarios.
+     * @throws SdkClientException
+     *         If any client side error occurs such as an IO related failure, failure to get credentials, etc.
+     * @throws JsonException
+     *         Base class for all service exceptions. Unknown exceptions will be thrown as an instance of this type.
+     * @sample JsonClient.APostOperation
+     * @see <a href="https://docs.aws.amazon.com/goto/WebAPI/json-service-2010-05-08/APostOperation" target="_top">AWS
+     *      API Documentation</a>
+     *
+     * @deprecated This API is deprecated, use something else
+     */


Is this changing the javadoc at all? If not, we can probably just rely on it being inherited?

looks like its inherited already

millems

Great work!

sonarqubecloud · 2023-02-10T01:06:01Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
54 Code Smells

92.0% Coverage
4.7% Duplication

davidh44 added 4 commits February 2, 2023 12:18

Change DelegatingS3AsyncClient from internal to protected API class

afa6fb4

Move DelegatingS3AsyncClient out of internal package

4fb8922

Generate DelegatingS3Client

f96abcd

Fix poet functional tests

491e414

davidh44 requested a review from a team as a code owner February 4, 2023 01:23

davidh44 added 2 commits February 4, 2023 15:18

Refactor add consumer method

d18e8b6

Fix sonarcloud bug - add override and make protected

2f856b4

millems reviewed Feb 6, 2023

View reviewed changes

davidh44 added 2 commits February 6, 2023 16:14

Refactoring - Address comments pt 1

a2ba743

Refactor poetSpec - addressing comments pt2

aeef3b7

millems reviewed Feb 8, 2023

View reviewed changes

davidh44 added 2 commits February 8, 2023 12:18

Refactoring - address comments pt3

881f357

Refactor async client classes

35cce5a

millems reviewed Feb 9, 2023

View reviewed changes

Refactoring - address comments pt4

23cc01c

millems approved these changes Feb 10, 2023

View reviewed changes

davidh44 enabled auto-merge (squash) February 10, 2023 00:53

davidh44 merged commit d1f46a6 into master Feb 10, 2023

davidh44 deleted the hdavidh/generate-delegating-s3-client branch February 10, 2023 01:23

davidh44 restored the hdavidh/generate-delegating-s3-client branch February 10, 2023 23:24

davidh44 deleted the hdavidh/generate-delegating-s3-client branch February 15, 2023 19:56

kessplas mentioned this pull request Feb 28, 2023

chore: bump AWS SDK v2 deps and make S3 non-optional aws/amazon-s3-encryption-client-java#93

Merged


		public class DelegatingAsyncClientClass extends AsyncClientInterface {

		private static final String DELEGATE = "delegate";

Generate DelegatingS3Client #3745

Generate DelegatingS3Client #3745

Uh oh!

Conversation

davidh44 commented Feb 4, 2023

Motivation and Context

Modifications

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!

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!

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!

millems left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Feb 10, 2023

Uh oh!

Uh oh!