Refactor with smithy client for json protocol non customized services [Work in Progress] #3208
Conversation
| @@ -55,6 +55,12 @@ namespace Aws | |||
| namespace Endpoint | |||
| { | |||
| class AWSEndpoint; | |||
|
|
|||
| struct AWSEndpointResolutionOverrides | |||
There was a problem hiding this comment.
there is a EndpointUpdateCallback&& endpointCallback, to insert the path segments and set this boolean variable within the generated code
| @@ -19,9 +19,11 @@ namespace smithy { | |||
| /* note: AuthSchemeOption is not connected with AuthScheme by type system, only by the String of schemeId, this is in accordance with SRA */ | |||
| public: | |||
| AuthSchemeOption(const char* id = nullptr): schemeId(id) {} | |||
| AuthSchemeOption(const char* id, bool isStreaming): schemeId(id),isEventStreaming{isStreaming} {} | |||
There was a problem hiding this comment.
please don't extended base interfaces with implementation details.
Just create a AuthScheme SigV4 EventStreaming.
There was a problem hiding this comment.
The way current code is designed otherwise needs way more changes. This minimizes that with default args
| @@ -131,12 +139,17 @@ namespace client | |||
| Aws::Http::HttpMethod method, | |||
| EndpointUpdateCallback&& endpointCallback, | |||
| ResponseHandlerFunc&& responseHandler, | |||
| std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor) const; | |||
| std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor, | |||
There was a problem hiding this comment.
the same here
I'd try to avoid leaking details to the base client class.
This API will be used by ~400 services, and having 2 extra arguments only for few of them is inconvenient in support.
There was a problem hiding this comment.
Same reasoning as above, my first attempt was decorator class which necessitated way more changes given the templated design. This was the way i could minimize it
| @@ -99,7 +98,10 @@ void AwsSmithyClientBase::MakeRequestAsync(Aws::AmazonWebServiceRequest const* c | |||
| Aws::Http::HttpMethod method, | |||
| EndpointUpdateCallback&& endpointCallback, | |||
| ResponseHandlerFunc&& responseHandler, | |||
| std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor) const | |||
| std::shared_ptr<Aws::Utils::Threading::Executor> pExecutor, | |||
| bool isEventStreamRequest, | |||
There was a problem hiding this comment.
the same here, I'd try to avoid placing detailed logic in the base Client class.
AwsClient became a god class nobody-knows-how-it-works and nobody-wants-to-touch-it and we would like to try to avoid repeating the same mistake.
| @@ -61,4 +61,51 @@ namespace smithy { | |||
| Aws::String m_region; | |||
| Aws::Client::AWSAuthV4Signer legacySigner; | |||
| }; | |||
|
|
|||
| class AWSAuthEventStreamV4Signer : public AwsSignerBase<AwsCredentialIdentityBase> { | |||
There was a problem hiding this comment.
place it in a separate file, so that 400+ service clients not-using-eventstreams won't have to compile this code.
|
closing this to open another PR with full set of changes + merges |
Issue #, if available:
Description of changes:
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.