Initial implementation of AWSRpcV2CborClient #3515
Conversation
| #include <aws/core/utils/event/EventStream.h> | ||
| #include <aws/core/utils/logging/LogMacros.h> | ||
| #include <aws/core/utils/memory/stl/AWSStringStream.h> | ||
| #include <aws/core/utils/xml/XmlSerializer.h> |
There was a problem hiding this comment.
dont think we need a xml serializer in here right?
| * AWSClient that handles marshalling cbor response bodies. You would inherit from this class | ||
| * to create a client that uses Cbor as its payload format. | ||
| */ | ||
| class AWS_CORE_API AWSRpcV2CborClient : public AWSClient |
There was a problem hiding this comment.
one high level comment i have is that this class and JsonClient is about 99 percent the same. is there a opportunity to re-use some code between the two? it feels like the only thing that is different is the outcome type. can we template the outcome type and keep the logic the same? i.e.
#include <type_traits>
class AwsOutcome {};
class AwsClient {
public:
AwsOutcome MakeRequest() { return AwsOutcome{}; }
};
template <typename OutcomeT,
typename std::enable_if<std::is_constructible<OutcomeT, AwsOutcome>::value, int>::type = 0>
class BaseClient : public AwsClient {
public:
OutcomeT MakeRequest() { return OutcomeT{AwsClient::MakeRequest()}; }
};
class JsonOutcome {
public:
explicit JsonOutcome(AwsOutcome outcome) {}
};
class CborOutcome {
public:
explicit CborOutcome(AwsOutcome outcome) {}
};
class JsonClient : public BaseClient<JsonOutcome> {
public:
JsonOutcome MakeRequest() { return BaseClient::MakeRequest(); }
};
class CborClient : public BaseClient<CborOutcome> {
public:
CborOutcome MakeRequest() { return BaseClient::MakeRequest(); }
};
int main() {
CborClient cbor_client{};
cbor_client.MakeRequest();
JsonClient json_client{};
json_client.MakeRequest();
return 0;
}using sfinae to ensure that OutcomeT is constructible from AwsOutcome which in this case is HttpResponseOutcome but that general idea.
There was a problem hiding this comment.
I agree that much of the code is the same, and this would be a perfect use case for a template-based approach. However, it would also increase the scope significantly, since that would be a change that could affect current client functionality. I think if we did want to do a refactor like this, it'd be best to do it in a separate PR after these changes are merged
There was a problem hiding this comment.
However, it would also increase the scope significantly, since that would be a change that could affect current client functionality.
I don't think thats true, if anything this reduces scope as it re-uses a ton of functionality we already have implemented for json. copy-ing and pasting here would create duplicates of the same code path creating maintainability concerns. we're here editing this now, so lets do it the right way the first time
…e (AWSProtocolClient)
| else | ||
| { | ||
| assert(httpResponse->GetResponseCode() != HttpResponseCode::OK); | ||
| error = GetErrorMarshaller()->Marshall(*httpResponse); |
There was a problem hiding this comment.
since GetErrorMarshaller is a function on AwsClient and Marshall is a pure virtual interface, cant function be moved to the base AWSProtocolClient class? also looking at the diff it looks like theres actually no difference between the impls right now.
…ocolClient with new changes.
Issue #, if available:
Description of changes:
Initial implementation of the core CBOR Client. Purpose of this PR is to bring in changes incrementally.
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.