Pre-requisite Changes Enable smithy query + rest-xml clients #3321

sbera87 · 2025-02-28T23:01:29Z

Issue #, if available:

Description of changes:
This PR includes

changes needed to enable xml, query protocol clients and special customized client like RDS
changes for multi auth support across all protocols (Refactor to minimize code/logic duplication in Java codegen classes)
backwards compatible changes for generate presign url APIs
Examples of changes on clients from different protocols.

Check all that applies:

Did a review by yourself.
Added proper tests to cover this PR. (If tests are not applicable, explain.)
Checked if this PR is a breaking (APIs have been changed) change.
Checked if this PR will not introduce cross-platform inconsistent behavior.
Checked if this PR would require a ReadMe/Wiki update.

Check which platforms you have built SDK on to verify the correctness of this PR.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

sbera87 · 2025-03-11T20:56:34Z

...c/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/JsonCppClientGenerator.java

@@ -176,7 +177,7 @@ protected List<SdkFileEntry> generateClientSourceFile( List<ServiceModel> servic
        {
            if(serviceModels.get(index).isUseSmithyClient() && !serviceModels.get(index).hasEventStreamingRequestShapes())
            {
-                return GenerateSmithyClientSourceFile(serviceModels.get(index), index);
+                return GenerateSmithyClientSourceFile(serviceModels.get(index), index, Optional.empty());


Reuse base class function

sbera87 · 2025-03-11T20:56:57Z

.../main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/QueryCppClientGenerator.java

-        for (int i = 0; i < serviceModels.size(); i++) {
-            Template template = velocityEngine.getTemplate("/com/amazonaws/util/awsclientgenerator/velocity/cpp/queryxml/QueryXmlServiceClientSource.vm", StandardCharsets.UTF_8.name());
+
+        List<Integer> serviceModelsIndices = IntStream.range(0, serviceModels.size()).boxed().collect(Collectors.toList());


Refactor to use base class function to avoid duplicate logic

sbera87 · 2025-03-11T20:57:29Z

...ain/java/com/amazonaws/util/awsclientgenerator/generators/cpp/RestXmlCppClientGenerator.java

+        Template template;
+        if (serviceModel.isUseSmithyClient())
+        {
+            template = velocityEngine.getTemplate("/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm", StandardCharsets.UTF_8.name());


Refactor to use base class function to avoid duplicate code and logic

sbera87 · 2025-03-12T14:51:55Z

...mazonaws/util/awsclientgenerator/generators/cpp/dynamodb/DynamoDBJsonCppClientGenerator.java

@@ -62,7 +63,7 @@ protected List<SdkFileEntry> generateClientSourceFile(List<ServiceModel> service
        serviceModelsIndices.stream().forEach(index -> {
            if(serviceModels.get(index).isUseSmithyClient())
            {
-                smithyClients.add(GenerateSmithyClientSourceFile(serviceModels.get(index), index));
+                smithyClients.add(GenerateSmithyClientSourceFile(serviceModels.get(index), index,Optional.empty()));


To make API reusable, accommodate interface change

sbera87 · 2025-03-12T14:53:41Z

tools/scripts/codegen/model_utils.py

@@ -40,6 +40,9 @@
    , "lexv2-runtime"
    , "qbusiness"
    , "transcribestreaming"
+    , "s3-crt"


In exclusion list because s3 was addressed in separate PR, and S3-crt,s3control pending

SergeyRyabinin · 2025-03-12T21:09:48Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

@@ -284,6 +284,49 @@ namespace client
            return {};
        }

+        Aws::String GeneratePresignedUrl(


is there a to duplicate this method body?

sbiscigl · 2025-03-12T20:57:14Z

src/aws-cpp-sdk-core/include/smithy/client/AwsLegacyClient.h

@@ -100,6 +100,186 @@ namespace client
        }

        AwsLegacyClientT() = default;
+


ah the joys of backwards compatibility

sbiscigl · 2025-03-12T21:05:30Z

src/aws-cpp-sdk-core/include/smithy/client/AwsSmithyClient.h

@@ -284,6 +284,49 @@ namespace client
            return {};
        }

+        Aws::String GeneratePresignedUrl(


having four generate presigned URL methods seems excessive and that we are creating bloat in this class. can we do something similar to how we did AwsLegacyClientT and move this into a PresignMixinT then invoke a single smithy client method?

In fact we already do this on the old client but have it as a signer to look up. lets just add it as a mix in.

sbiscigl · 2025-03-12T21:06:27Z

...r/src/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppClientGenerator.java

+            else
+            {
+                throw new RuntimeException(String.format("authSchemes '%s'",serviceModel.getAuthSchemes().stream().collect(Collectors.toList())
+                ));


fix these trailing params, put them on the same line as everything else, it reads weird

sbiscigl · 2025-03-12T21:10:12Z

...r/src/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppClientGenerator.java

        {
-            context.put("AuthSchemeResolver", ResolverMapping.get(firstAuthScheme.get()));
+            context.put("AuthSchemeResolver", "SigV4MultiAuthSchemeResolver");


why are we hard coding it to sigv4 and no the the first auth scheme? we should be using the first in the list, hard coding it seems wrong

sbiscigl · 2025-03-12T21:10:42Z

...r/src/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CppClientGenerator.java

        }
        else
        {
-            throw new RuntimeException(String.format("authSchemes '%s'",serviceModel.getAuthSchemes().stream().collect(Collectors.toList())
-            ));
+            Optional<String> firstAuthScheme = serviceModel.getAuthSchemes().stream().filter(entry->ResolverMapping.containsKey(entry)).findFirst();


this is the third time this has been copied so likely it should be broken off. into its own function

sbiscigl · 2025-03-12T21:11:38Z

...ain/java/com/amazonaws/util/awsclientgenerator/generators/cpp/RestXmlCppClientGenerator.java

+        if (serviceModel.isUseSmithyClient())
+        {
+            template = velocityEngine.getTemplate("/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm", StandardCharsets.UTF_8.name());
+            if(serviceModel.getAuthSchemes().size() > 1)


this same block appears all of the place, we need to move it to a single place to avoid code duplicaiton

sbera87 force-pushed the s3query branch 3 times, most recently from 27ab711 to e788a0f Compare March 6, 2025 16:51

Enable Smithy Query, rest xml clients

c4b6c51

sbera87 force-pushed the s3query branch 3 times, most recently from c5622a1 to 80a1146 Compare March 11, 2025 20:37

sbera87 commented Mar 11, 2025

View reviewed changes

backwards compatible changes

4b05793

sbera87 force-pushed the s3query branch from 80a1146 to 4b05793 Compare March 12, 2025 13:45

sbera87 changed the title ~~Enable S3 query + rest-xml clients [Work in Progress]~~ Pre-requisite Changes Enable S3 query + rest-xml clients Mar 12, 2025

sbera87 marked this pull request as ready for review March 12, 2025 14:50

sbera87 commented Mar 12, 2025

View reviewed changes

sbera87 requested review from sbiscigl and SergeyRyabinin March 12, 2025 15:08

sbera87 changed the title ~~Pre-requisite Changes Enable S3 query + rest-xml clients~~ Pre-requisite Changes Enable smithy query + Rest-xml clients Mar 12, 2025

sbera87 changed the title ~~Pre-requisite Changes Enable smithy query + Rest-xml clients~~ Pre-requisite Changes Enable smithy query + rest-xml clients Mar 12, 2025

SergeyRyabinin reviewed Mar 12, 2025

View reviewed changes

sbiscigl reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pre-requisite Changes Enable smithy query + rest-xml clients #3321

Pre-requisite Changes Enable smithy query + rest-xml clients #3321

sbera87 commented Feb 28, 2025 •

edited

Loading

sbera87 Mar 11, 2025

sbera87 Mar 11, 2025

sbera87 Mar 11, 2025

sbera87 Mar 12, 2025

sbera87 Mar 12, 2025

SergeyRyabinin Mar 12, 2025

sbiscigl Mar 12, 2025

sbiscigl Mar 12, 2025

sbiscigl Mar 12, 2025

sbiscigl Mar 12, 2025

sbiscigl Mar 12, 2025

sbiscigl Mar 12, 2025

		@@ -100,6 +100,186 @@ namespace client
		}

		AwsLegacyClientT() = default;

Pre-requisite Changes Enable smithy query + rest-xml clients #3321

Are you sure you want to change the base?

Pre-requisite Changes Enable smithy query + rest-xml clients #3321

Conversation

sbera87 commented Feb 28, 2025 • edited Loading

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

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

sbera87 commented Feb 28, 2025 •

edited

Loading