-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Cbor Serialization/Deserialization & Protocol Tests #3538
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
base: main
Are you sure you want to change the base?
Conversation
…tor, Datetime constructor, AwsProtocolTestHelper, protocol_tests_gen.py))
…lientGenerator class
…recursive variables
… string/int, add indefinite blob logic
...rc/main/java/com/amazonaws/util/awsclientgenerator/domainmodels/codegeneration/Metadata.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/com/amazonaws/util/awsclientgenerator/domainmodels/codegeneration/Shape.java
Show resolved
Hide resolved
...ava/com/amazonaws/util/awsclientgenerator/domainmodels/codegeneration/cpp/CppViewHelper.java
Show resolved
Hide resolved
...c/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CborCppClientGenerator.java
Show resolved
Hide resolved
...c/main/java/com/amazonaws/util/awsclientgenerator/generators/cpp/CborCppClientGenerator.java
Show resolved
Hide resolved
generated/protocol-tests/test-clients/aws-cpp-sdk-rpcv2protocol/source/model/ComplexError.cpp
Outdated
Show resolved
Hide resolved
...rotocol-tests/test-clients/aws-cpp-sdk-rpcv2protocol/source/model/ComplexNestedErrorData.cpp
Outdated
Show resolved
Hide resolved
tests/testing-resources/include/aws/testing/AwsProtocolTestHelpers.h
Outdated
Show resolved
Hide resolved
tests/testing-resources/include/aws/testing/AwsProtocolTestHelpers.h
Outdated
Show resolved
Hide resolved
…bject compairsons are scoped to AwsProtocolTestHelpers
const RecursiveShapesResult& result = outcome.GetResult(); | ||
ValidateRequestSent([&result](const ExpectedRequest&, const Aws::ProtocolMock::Model::Request&) -> void { | ||
/* expectedResult = R"( {"nested":{"foo":"Foo1","nested":{"bar":"Bar1","recursiveMember":{"foo":"Foo2","nested":{"bar":"Bar2"}}}}} )" */ | ||
{ |
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.
nit: {{
unneeded but it also sounds like it might be a codegen nightmare
const auto& cborValue = result.GetPayload(); | ||
const auto decoder = cborValue.GetDecoder(); | ||
|
||
if (decoder != nullptr) { |
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 thought a lot about this factoring of it and tried to come up with a better was to do this to avoid this nesting logic, and right now I think its OK given the circumstances and that its a two way door given that this will be in compiled code and we can change the implementation anytime we want.
At a high level though we should try to decompose this code into logical parts that can be re-used in code gen instead of code genning the entire implementation i.e. logically this is:
if (known map size) {
deserialize_float
deserialize_string
} else {
deserialize_float
deserialize_string
}
this could roughly become in templated code
#pragma once
#include <aws/core/AmazonWebServiceResult.h>
#include <aws/core/utils/memory/stl/AWSString.h>
#include <aws/core/utils/cbor/CborValue.h>
#include <type_traits>
namespace Aws {
namespace Utils {
namespace Cbor {
class CborDecoderUtil final {
public:
CborDecoderUtil() = delete;
template <typename T, typename std::enable_if<std::is_same<T, Aws::String>::value, int>::type = 0>
static T Decode(const AmazonWebServiceResult<Aws::Utils::Cbor::CborValue>& result) {
auto decoder = result.GetPayload().GetDecoder();
auto peekType = decoder->PeekType();
if (peekType.has_value()) {
if (peekType.value() == Aws::Crt::Cbor::CborType::Text) {
auto val = decoder->PopNextTextVal();
if (val.has_value()) {
return Aws::String(reinterpret_cast<const char*>(val.value().ptr), val.value().len);
}
} else {
decoder->ConsumeNextSingleElement();
Aws::StringStream ss;
while (decoder->LastError() == AWS_ERROR_UNKNOWN) {
auto nextType = decoder->PeekType();
if (!nextType.has_value() || nextType.value() == CborType::Break) {
if (nextType.has_value()) {
decoder->ConsumeNextSingleElement(); // consume the Break
}
break;
}
auto val = decoder->PopNextTextVal();
if (val.has_value()) {
ss << Aws::String(reinterpret_cast<const char*>(val.value().ptr), val.value().len);
}
}
return ss.str();
}
}
// likely log an error, we're in a weird state.
return T{};
}
template <typename T, typename std::enable_if<std::is_same<T, double>::value, int>::type = 0>
static T Decode(const AmazonWebServiceResult<Aws::Utils::Cbor::CborValue>& result) {
auto decoder = result.GetPayload().GetDecoder();
auto val = decoder->PopNextFloatVal();
if (val.has_value()) {
return val.value();
}
// likely log an error, we're in a weird state.
return T{};
}
// TODO: do this for every type
};
}
}
}
where the trouble starts then is
TODO: do this for every type
specifically where this gets into trouble is Map
-> Object
types. This is because C++ does not have reflection. for primative types this is tivial, but with complex types we need to actually map between a cbor "map" which is actually closer to a object and a c++ "object".
The way other C++/C parsers accomplish this is by using a intermediate object i.e.
#include <unordered_map>
#include <vector>
class cbor_float {};
class cbor_string {};
//other types
class cbor_value {
public:
cbor_value(char* bytes) {
//...create underlying type
}
private:
std::unordered_map<
std::string,
std::variant<cbor_float, cbor_string, cbor_value>> data;
};
class cbor_constructible {
public:
cbor_constructible(cbor_value value) {
//...construct from free from objec
}
};
the issue there though is then you have to construct a intermediate object, which is also not ideal.
so given the open choices:
-
use reflection
pros:
It is the correct choice
cons:
doesnt exist yet -
create a temporary object
pros:
cleaner code
cons:
temporary object -
use code gen to create cbor parsing
pros:
no temp object
no implementaion specific doorway
cons:
messy code
i think we can move forward with codegen with the idea to revisit it if we want to use temporary objects or a better solutions presents itself
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.