Add check for nested optional members in key #2154
Conversation
Optional members as part of a key in a data type are not allowed. A check for optional members in nested types was missing, and is introduced in this commit. The checks is added in the IDL compiler and in the runtime type meta-data validator. Signed-off-by: Dennis Potman <[email protected]>
src/core/ddsi/src/ddsi_typewrap.c
Outdated
| @@ -953,25 +959,27 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_ | |||
| return ret; | |||
| for (uint32_t n = 0; n < t->_u.structure.members.length; n++) | |||
| { | |||
| if ((ret = xt_valid_member_flags (gv, t->_u.structure.members.seq[n].flags, MEMBER_FLAG_STRUCT_MEMBER))) | |||
| DDS_XTypes_StructMemberFlag flags = t->_u.structure.members.seq[n].flags; | |||
| bool key = in_key || (flags & DDS_XTypes_IS_KEY); | |||
There was a problem hiding this comment.
Would this not cause it to reject:
struct A { @key x; @optional y; };
struct B { @key A z; };
- validation starts with
B,in_key=false - field
z: key flag is set, so validation ofAstarts within_key=true - when it gets to field
y,xt_valid_member_flagsis called within_key=trueand so rejects it at line 873 (if (in_key && (flags & O)))
In my understanding, because A contains some members marked @key, the members not marked @key (i.e., y) are not key fields. It x hadn't marked as key, then of course rejecting it would've been correct.
I think it needs a separate run over the structure members to check for the presence of some @key annotations.
There was a problem hiding this comment.
Good point, I missed that. In 88cfe84 I've addressed this, and also take the members in the base type into account for this.
…he aggregated type is marked as key. Also take base type members into account. Signed-off-by: Dennis Potman <[email protected]>
| for (uint32_t n = 0; n < t->_u.structure.members.length; n++) | ||
| { | ||
| DDS_XTypes_StructMemberFlag flags = t->_u.structure.members.seq[n].flags; | ||
| bool key = in_key || (flags & DDS_XTypes_IS_KEY); | ||
| bool key = (in_key && !has_key_members) || (flags & DDS_XTypes_IS_KEY); |
There was a problem hiding this comment.
When I reviewed it the first time I completely forgot about base types 😱 Good thing you thought of it now 🙂
So ... on looking at this again ... in
struct A { x; };
struct B { @key A y; };
struct C { B z; };
x is not a key when considering C as the type of a topic, but it would be a key if considering B as the type of a topic. Should that make C fail verification if x were optional? What if no-one tries to make a topic out of B?
I can see arguments both ways: the one combination used has no problems, but then the IDL is nonsensical. So rejecting B and thus also C if x were made optional is reasonable.
I do think it would be wise to point out in a small comment that the key can be true here without the field actually acting as a key field in the topic type, and that we choose to reject cases like the above.
There was a problem hiding this comment.
Added the comment in 19a9679
…o implicit key members that have the optional flag set Signed-off-by: Dennis Potman <[email protected]>
Optional members as part of a key in a data type are not allowed. A check for optional members in nested types was missing, and is introduced in this commit. The checks is added in the IDL compiler and in the runtime type meta-data validator.
Fixes #2151