[c++] Fix base to derived deserialization #745
Conversation
ara-ayvazyan
force-pushed
the
fix_base_deserialize
branch
from
f24941e to
8566a07
Compare
cpp/inc/bond/core/transforms.h
Outdated
| @@ -388,6 +388,12 @@ class RequiredFieldValiadator | |||
| _required = next_required_field<typename schema<T>::type::fields>::value; | |||
| } | |||
|
|
|||
| void Base(bool done) const | |||
There was a problem hiding this comment.
Technically, this is a breaking change. Not sure if this needs to be mentioned in changelog.
There was a problem hiding this comment.
Yes, this should be mentioned in the change log.
CHANGELOG.md
Outdated
| @@ -48,6 +48,8 @@ different versioning scheme, following the Haskell community's | |||
| * gRPC v1.7.1 is now required to use Bond-over-gRPC. | |||
| * Fixed includes for gRPC services with events or parameterless methods. | |||
| [Issue #735](https://github.com/Microsoft/bond/issues/735) | |||
| * Fixed a bug which would read unrelated struct's field(s) when deserializing base struct | |||
There was a problem hiding this comment.
Minor: a few words missing "...would read an unrelated struct's... deserializing a base struct." and move the period to the end of this sentence have have the "Issue #742" float at the end.
cpp/inc/bond/core/transforms.h
Outdated
| @@ -388,6 +388,12 @@ class RequiredFieldValiadator | |||
| _required = next_required_field<typename schema<T>::type::fields>::value; | |||
| } | |||
|
|
|||
| void Base(bool done) const | |||
| { | |||
| if (done) | |||
There was a problem hiding this comment.
Minor: slight preference for curly braces always.
There was a problem hiding this comment.
Was trying to be consistent here.
cpp/inc/bond/core/transforms.h
Outdated
| mutable uint16_t _required; | ||
| }; | ||
|
|
||
| template <typename T> | ||
| void RequiredFieldValiadator<T>::MissingFieldException() const | ||
| { | ||
| (void)typename schema<T>::type(); |
There was a problem hiding this comment.
Why do we need to construct an instance of the field?
There was a problem hiding this comment.
To force-instantiate a possibly template static data member metadata (see this link error, even though it happens for the newly added exception function, but the issue is the same).
cpp/inc/bond/core/transforms.h
Outdated
| (void)typename schema<T>::type(); | ||
|
|
||
| BOND_THROW(CoreException, | ||
| "De-serialization failed: unexpected struct stop is encountered for " |
There was a problem hiding this comment.
Minor: "unexpected struct stop encountered for..."
cpp/inc/bond/core/transforms.h
Outdated
| @@ -388,6 +388,12 @@ class RequiredFieldValiadator | |||
| _required = next_required_field<typename schema<T>::type::fields>::value; | |||
| } | |||
|
|
|||
| void Base(bool done) const | |||
There was a problem hiding this comment.
It feels confusing to be doing this in something called "RequiredFieldValidator", as this doesn't have to do with required fields.
Should/can this be in To directly?
There was a problem hiding this comment.
In that case the user will not be able to customize that behavior. If we treat this as an incorrect payload detection, then I guess should be OK to move it to To<T> transform.
There was a problem hiding this comment.
I'm fine having this apply to all cases of deserialization and not be customizable. This is about detecting an invalid payload/bad downcast.
To keep it customizable, we could have a different validator that composes with RFV. PayloadIsntBeingDowncastIncorrectlyValidator<T> : T or something.
With this change deserialization will now fail by throwing
bond::CoreExceptionwhenBT_STOPis encountered while deserializing a base struct.Fixes #742