Add check for nested optional members in key (#2154)

* Add check for nested optional members in key 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]> * Allow optional flag on a non-key member in case any other member in the aggregated type is marked as key. Also take base type members into account. Signed-off-by: Dennis Potman <[email protected]> * Add comment explaining the behaviour of the validation with respect to implicit key members that have the optional flag set Signed-off-by: Dennis Potman <[email protected]> --------- Signed-off-by: Dennis Potman <[email protected]>
eclipse-cyclonedds · Jan 16, 2025 · 57ecc84 · 57ecc84
1 parent 7a67a31
commit 57ecc84
Show file tree

Hide file tree

Showing 4 changed files with 73 additions and 25 deletions.
diff --git a/src/core/ddsc/tests/dynamic_type.c b/src/core/ddsc/tests/dynamic_type.c
@@ -416,11 +416,10 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini
   dds_dynamic_type_set_autoid (&dstruct, DDS_DYNAMIC_TYPE_AUTOID_HASH);
   dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m1"));
   dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m2"));
+  dds_dynamic_type_add_member (&dstruct, DDS_DYNAMIC_MEMBER_PRIM(DDS_DYNAMIC_UINT16, "m3"));
 
   dds_return_t ret = dds_dynamic_member_set_key (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
   CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
-  ret = dds_dynamic_member_set_optional (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
-  CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
   ret = dds_dynamic_member_set_external (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), true);
   CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
   ret = dds_dynamic_member_set_hashid (&dstruct, ddsi_dynamic_type_member_hashid ("m2"), "m2_name");
@@ -429,8 +428,12 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini
   ret = dds_dynamic_member_set_must_understand (&dstruct, ddsi_dynamic_type_member_hashid ("m2_name"), true);
   CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
 
+  // Optional and key can't be set to the same member
+  ret = dds_dynamic_member_set_optional (&dstruct, ddsi_dynamic_type_member_hashid ("m3"), true);
+  CU_ASSERT_EQUAL_FATAL (ret, DDS_RETCODE_OK);
+
   struct ddsi_type *type = get_ddsi_type (&dstruct);
-  CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.length, 2);
+  CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.length, 3);
 
   CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[0].id, ddsi_dynamic_type_member_hashid ("m1"));
   CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[0].flags & DDS_XTypes_IS_KEY));
@@ -440,10 +443,16 @@ CU_Test (ddsc_dynamic_type, struct_member_prop, .init = dynamic_type_init, .fini
 
   CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[1].id, ddsi_dynamic_type_member_hashid ("m2_name"));
   CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_KEY);
-  CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_OPTIONAL);
+  CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_OPTIONAL));
   CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_EXTERNAL);
   CU_ASSERT_FATAL (type->xt._u.structure.members.seq[1].flags & DDS_XTypes_IS_MUST_UNDERSTAND);
 
+  CU_ASSERT_EQUAL_FATAL (type->xt._u.structure.members.seq[2].id, ddsi_dynamic_type_member_hashid ("m3"));
+  CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_KEY));
+  CU_ASSERT_FATAL (type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_OPTIONAL);
+  CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_EXTERNAL));
+  CU_ASSERT_FATAL (!(type->xt._u.structure.members.seq[2].flags & DDS_XTypes_IS_MUST_UNDERSTAND));
+
   dds_dynamic_type_unref (&dstruct);
 }
 

diff --git a/src/core/ddsi/src/ddsi_typewrap.c b/src/core/ddsi/src/ddsi_typewrap.c
@@ -853,7 +853,7 @@ static dds_return_t xt_valid_type_flags (struct ddsi_domaingv *gv, uint16_t flag
 #define M DDS_XTypes_IS_MUST_UNDERSTAND
 #define K DDS_XTypes_IS_KEY
 #define D DDS_XTypes_IS_DEFAULT
-static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t flags, uint8_t member_flag_kind)
+static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t flags, uint8_t member_flag_kind, bool in_key)
 {
   dds_return_t ret = DDS_RETCODE_OK;
 
@@ -870,6 +870,10 @@ static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t fl
     case MEMBER_FLAG_STRUCT_MEMBER:
       if (flags & ~(T1|T2|O|M|K|X))
         ret = DDS_RETCODE_BAD_PARAMETER;
+      if (in_key && (flags & O))
+        ret = DDS_RETCODE_BAD_PARAMETER;
+      if ((flags & O) && (flags & K))
+        ret = DDS_RETCODE_BAD_PARAMETER;
       break;
     case MEMBER_FLAG_UNION_MEMBER:
       if (flags & ~(T1|T2|D|X))
@@ -879,6 +883,8 @@ static dds_return_t xt_valid_member_flags (struct ddsi_domaingv *gv, uint16_t fl
       // must-understand not in spec
       if (flags & ~(T1|T2|M|K))
         ret = DDS_RETCODE_BAD_PARAMETER;
+      if (in_key && (flags & O))
+        ret = DDS_RETCODE_BAD_PARAMETER;
       break;
     case MEMBER_FLAG_ENUM_LITERAL:
       if (flags & ~(D))
@@ -932,11 +938,11 @@ static dds_return_t xt_valid_array_bounds (struct ddsi_domaingv *gv, const struc
   return DDS_RETCODE_OK;
 }
 
-static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_type *t, bool validate_hash_type)
+static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_type *t, bool validate_hash_type, bool in_key)
 {
   dds_return_t ret;
 
-  if (!validate_hash_type && !xt_is_non_hash (t))
+  if (!in_key && !validate_hash_type && !xt_is_non_hash (t))
     return DDS_RETCODE_OK;
 
   if (ddsi_xt_is_unresolved (t))
@@ -951,27 +957,48 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
           || (ret = xt_valid_struct_member_ids (gv, t))
           || (ret = xt_valid_type_flags (gv, t->_u.structure.flags, t->_d)))
         return ret;
+
+      bool has_key_members = false;
+      for (const struct xt_type *t1 = t; t1 && ddsi_xt_is_resolved (t1); t1 = t1->_u.structure.base_type ? &t1->_u.structure.base_type->xt : NULL)
+        for (uint32_t n = 0; !has_key_members && n < t1->_u.structure.members.length; n++)
+          has_key_members = (t1->_u.structure.members.seq[n].flags & DDS_XTypes_IS_KEY);
+
       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;
+        /* A member is considered a key-member (and therefore cannot be optional)
+           in case (1) the member has a key flag or (2) no member of the struct
+           and it's base types has a key flag and the 'parent' member (the one
+           that has this struct as it's member type) is a key (by either rule 1
+           or 2). As a result, a type can be valid or invalid based on the context
+           it is used in:
+              struct A { @optional long x; };
+              struct B { @key A y; };
+              struct C { B z; };
+           Type B is an invalid top-level type for a topic. Type C is a valid
+           top-level type, but will still be rejected because of the key flag
+           in B.
+        */
+        bool key = (in_key && !has_key_members) || (flags & DDS_XTypes_IS_KEY);
+        if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_STRUCT_MEMBER, key)))
           return ret;
-        if ((ret = xt_validate_impl (gv, &t->_u.structure.members.seq[n].type->xt, false)))
+        if ((ret = xt_validate_impl (gv, &t->_u.structure.members.seq[n].type->xt, false, key)))
           return ret;
       }
       break;
     case DDS_XTypes_TK_UNION: {
       if (((ret = xt_valid_union_disc_type (gv, t)))
           || (ret = xt_valid_union_member_ids (gv, t))
           || (ret = xt_valid_type_flags (gv, t->_u.union_type.flags, t->_d))
-          || (ret = xt_valid_member_flags (gv, t->_u.union_type.disc_flags, MEMBER_FLAG_UNION_DISC)))
+          || (ret = xt_valid_member_flags (gv, t->_u.union_type.disc_flags, MEMBER_FLAG_UNION_DISC, in_key)))
         return ret;
       bool has_default = false;
       for (uint32_t n = 0; n < t->_u.union_type.members.length; n++)
       {
         DDS_XTypes_UnionMemberFlag flags = t->_u.union_type.members.seq[n].flags;
-        if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_UNION_MEMBER)))
+        if ((ret = xt_valid_member_flags (gv, flags, MEMBER_FLAG_UNION_MEMBER, in_key)))
           return ret;
-        if ((ret = xt_validate_impl (gv, &t->_u.union_type.members.seq[n].type->xt, false)))
+        if ((ret = xt_validate_impl (gv, &t->_u.union_type.members.seq[n].type->xt, false, in_key)))
           return ret;
         if (flags & DDS_XTypes_IS_DEFAULT)
         {
@@ -992,7 +1019,7 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
       if (t->_u.enum_type.bit_bound > 32)
         return DDS_RETCODE_BAD_PARAMETER;
       for (uint32_t n = 0; n < t->_u.enum_type.literals.length; n++)
-        if ((ret = xt_valid_member_flags (gv, t->_u.enum_type.literals.seq[n].flags, MEMBER_FLAG_ENUM_LITERAL)))
+        if ((ret = xt_valid_member_flags (gv, t->_u.enum_type.literals.seq[n].flags, MEMBER_FLAG_ENUM_LITERAL, in_key)))
           return ret;
       break;
     case DDS_XTypes_TK_BITMASK:
@@ -1002,13 +1029,13 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
       if (t->_u.bitmask.bit_bound > 64)
         return DDS_RETCODE_BAD_PARAMETER;
       for (uint32_t n = 0; n < t->_u.bitmask.bitflags.length; n++)
-        if ((ret = xt_valid_member_flags (gv, t->_u.bitmask.bitflags.seq[n].flags, MEMBER_FLAG_BIT_FLAG)))
+        if ((ret = xt_valid_member_flags (gv, t->_u.bitmask.bitflags.seq[n].flags, MEMBER_FLAG_BIT_FLAG, in_key)))
           return ret;
       break;
     case DDS_XTypes_TK_ALIAS:
       if ((ret = xt_valid_type_flags (gv, t->_u.alias.flags, t->_d))
-          || (ret = xt_valid_member_flags (gv, t->_u.alias.related_flags, MEMBER_FLAG_ALIAS_MEMBER))
-          || (ret = xt_validate_impl (gv, &t->_u.alias.related_type->xt, false)))
+          || (ret = xt_valid_member_flags (gv, t->_u.alias.related_flags, MEMBER_FLAG_ALIAS_MEMBER, in_key))
+          || (ret = xt_validate_impl (gv, &t->_u.alias.related_type->xt, false, in_key)))
         return ret;
       break;
     case DDS_XTypes_TK_BITSET:
@@ -1021,22 +1048,22 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
       break;
     case DDS_XTypes_TK_SEQUENCE:
       if ((ret = xt_valid_type_flags (gv, t->_u.seq.c.flags, t->_d))
-          || (ret = xt_valid_member_flags (gv, t->_u.seq.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
-          || (ret = xt_validate_impl (gv, &t->_u.seq.c.element_type->xt, false)))
+          || (ret = xt_valid_member_flags (gv, t->_u.seq.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
+          || (ret = xt_validate_impl (gv, &t->_u.seq.c.element_type->xt, false, in_key)))
         return ret;
       break;
     case DDS_XTypes_TK_ARRAY:
       if ((ret = xt_valid_type_flags (gv, t->_u.array.c.flags, t->_d))
-          || (ret = xt_valid_member_flags (gv, t->_u.array.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
-          || (ret = xt_validate_impl (gv, &t->_u.array.c.element_type->xt, false))
+          || (ret = xt_valid_member_flags (gv, t->_u.array.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
+          || (ret = xt_validate_impl (gv, &t->_u.array.c.element_type->xt, false, in_key))
           || (ret = xt_valid_array_bounds (gv, t)))
         return ret;
       break;
     case DDS_XTypes_TK_MAP:
       if ((ret = xt_valid_type_flags (gv, t->_u.map.c.flags, t->_d))
-          || (ret = xt_valid_member_flags (gv, t->_u.map.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT))
-          || (ret = xt_validate_impl (gv, &t->_u.map.key_type->xt, false))
-          || (ret = xt_validate_impl (gv, &t->_u.map.c.element_type->xt, false)))
+          || (ret = xt_valid_member_flags (gv, t->_u.map.c.element_flags, MEMBER_FLAG_COLLECTION_ELEMENT, in_key))
+          || (ret = xt_validate_impl (gv, &t->_u.map.key_type->xt, false, in_key))
+          || (ret = xt_validate_impl (gv, &t->_u.map.c.element_type->xt, false, in_key)))
         return ret;
       break;
     case DDS_XTypes_TK_BOOLEAN: case DDS_XTypes_TK_BYTE:
@@ -1054,7 +1081,7 @@ static dds_return_t xt_validate_impl (struct ddsi_domaingv *gv, const struct xt_
 
 dds_return_t ddsi_xt_validate (struct ddsi_domaingv *gv, const struct xt_type *t)
 {
-  return xt_validate_impl (gv, t, true);
+  return xt_validate_impl (gv, t, true, false);
 }
 
 static dds_return_t add_minimal_typeobj (struct ddsi_domaingv *gv, struct xt_type *xt, const struct DDS_XTypes_TypeObject *to)

diff --git a/src/idl/src/tree.c b/src/idl/src/tree.c
@@ -3700,7 +3700,14 @@ static bool no_specific_key(const void *node)
 {
   /* @key(FALSE) is equivalent to missing @key(?) */
   if (idl_mask(node) & IDL_STRUCT) {
-    const idl_member_t *member = ((const idl_struct_t *)node)->members;
+    const idl_struct_t *_struct = (const idl_struct_t *)node;
+    if (_struct->inherit_spec)
+    {
+      if (!no_specific_key(_struct->inherit_spec->base))
+        return false;
+    }
+
+    const idl_member_t *member = _struct->members;
     for (; member; member = idl_next(member)) {
       if (member->key.value)
         return false;

diff --git a/src/tools/idlc/src/libidlc/libidlc__descriptor.c b/src/tools/idlc/src/libidlc/libidlc__descriptor.c
@@ -2119,6 +2119,11 @@ static idl_retcode_t get_ctype_keys(const idl_pstate_t *pstate, struct descripto
           if (parent_is_key && !ctype->has_key_member)
             inst->data.opcode.code |= DDS_OP_FLAG_KEY;
           if (inst->data.opcode.code & DDS_OP_FLAG_KEY) {
+            if (inst->data.opcode.code & DDS_OP_FLAG_OPT) {
+              idl_error (pstate, ctype->node, "A member that is part of a key (possibly nested inside a struct) cannot be optional");
+              ret = IDL_RETCODE_SYNTAX_ERROR;
+              goto err;
+            }
             if ((ret = get_ctype_keys_adr(pstate, descriptor, offs, base_type_ops_offs, inst, ctype, n_keys, &ctype_keys)) != IDL_RETCODE_OK)
               goto err;
           }