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?

There are overlaps in the message body for the auth scheme selection bit, but differs in processing uri with request. I will push a commit with an attempt to reuse the overlapping block.

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.

Made the changes to have all legacy presigners in legacy client and smithy client have only one implementation. Under the hood, they all reuse a private implementation .

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

If you think about it, the existing ResolverMapping maps scheme id to an Auth scheme resolver, but in case there are multiple auth schemes detected for a service across different operations and endpoint rules, there is not a single scheme id to map against any resolver, hence it is handled separately to map explicitly to a MultiAuthSchemeResolver class. Using a first in the list wont work for this case.

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

There were 3 places where that lines you referred to existed: generateClientHeaderFile, generateSmithyClientSourceFile in the base class (CppClientGenerator) and one instance in derived class (which I missed on pointing to the base class implementation) . I have made changes in next commit to optimize this further

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

Sure, let me address this in next commit

sbiscigl · 2025-03-14T20:18:48Z

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

@@ -222,7 +222,7 @@ namespace client
            auto httpResponseOutcome = MakeRequestSync(request, requestName, method, std::move(endpointCallback));
            return m_serializer->Deserialize(std::move(httpResponseOutcome), GetServiceClientName(), requestName);
        }
-
+        
        Aws::String GeneratePresignedUrl(


so fan of having on presigner now, only question i really have is why is this in the the Smithy client and the others legcy. I think something that we could talk about is "what the public interface of a pre signer should be". but since this is already here, im open to moving forward without having a answer for that for now

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

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

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

sbera87 force-pushed the s3query branch 2 times, most recently from 9eb7cee to 842fc92 Compare March 13, 2025 17:52

sbera87 requested review from SergeyRyabinin and sbiscigl March 13, 2025 17:56

sbiscigl approved these changes Mar 14, 2025

View reviewed changes

sbera87 force-pushed the s3query branch from 842fc92 to da64e5b Compare March 17, 2025 15:13

SergeyRyabinin approved these changes Mar 17, 2025

View reviewed changes

sbera87 added 3 commits March 17, 2025 16:52

Enable Smithy Query, rest xml clients

511f0b7

backwards compatible changes

92ed8ad

reuse code

510c06a

sbera87 added 2 commits March 17, 2025 16:52

move all legacy presigners

89be26a

disable smithy client generation

0122fd4

sbera87 force-pushed the s3query branch from d782f71 to 0122fd4 Compare March 17, 2025 20:52

SergeyRyabinin approved these changes Mar 17, 2025

View reviewed changes

sbera87 merged commit d83d567 into main Mar 18, 2025
3 of 4 checks passed

sbera87 deleted the s3query branch March 18, 2025 13:03

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

		AwsLegacyClientT() = default;

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

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

Uh oh!

Conversation

sbera87 commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

sbera87 Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

sbera87 commented Feb 28, 2025 •

edited

Loading

sbera87 Mar 13, 2025 •

edited

Loading