-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Pre-requisite Changes Enable smithy query + rest-xml clients #3321
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
27ab711
to
e788a0f
Compare
c5622a1
to
80a1146
Compare
@@ -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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reuse base class function
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()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to use base class function to avoid duplicate logic
Template template; | ||
if (serviceModel.isUseSmithyClient()) | ||
{ | ||
template = velocityEngine.getTemplate("/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm", StandardCharsets.UTF_8.name()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactor to use base class function to avoid duplicate code and logic
@@ -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())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make API reusable, accommodate interface change
@@ -40,6 +40,9 @@ | |||
, "lexv2-runtime" | |||
, "qbusiness" | |||
, "transcribestreaming" | |||
, "s3-crt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In exclusion list because s3 was addressed in separate PR, and S3-crt,s3control pending
@@ -284,6 +284,49 @@ namespace client | |||
return {}; | |||
} | |||
|
|||
Aws::String GeneratePresignedUrl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a to duplicate this method body?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -100,6 +100,186 @@ namespace client | |||
} | |||
|
|||
AwsLegacyClientT() = default; | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah the joys of backwards compatibility
@@ -284,6 +284,49 @@ namespace client | |||
return {}; | |||
} | |||
|
|||
Aws::String GeneratePresignedUrl( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 .
else | ||
{ | ||
throw new RuntimeException(String.format("authSchemes '%s'",serviceModel.getAuthSchemes().stream().collect(Collectors.toList()) | ||
)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix these trailing params, put them on the same line as everything else, it reads weird
{ | ||
context.put("AuthSchemeResolver", ResolverMapping.get(firstAuthScheme.get())); | ||
context.put("AuthSchemeResolver", "SigV4MultiAuthSchemeResolver"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the third time this has been copied so likely it should be broken off. into its own function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if (serviceModel.isUseSmithyClient()) | ||
{ | ||
template = velocityEngine.getTemplate("/com/amazonaws/util/awsclientgenerator/velocity/cpp/smithy/SmithyClientHeader.vm", StandardCharsets.UTF_8.name()); | ||
if(serviceModel.getAuthSchemes().size() > 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this same block appears all of the place, we need to move it to a single place to avoid code duplicaiton
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, let me address this in next commit
9eb7cee
to
842fc92
Compare
@@ -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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Issue #, if available:
Description of changes:
This PR includes
Check all that applies:
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.