Merge pull request OpenDDS#4593 from jrw972/type-object-encoding

`Service_Participant::type_object_encoding` returns incorrect value
iguessthislldo · Apr 19, 2024 · 5ab002d · 5ab002d
2 parents f7165a7 + c66891c
commit 5ab002d
Show file tree

Hide file tree

Showing 5 changed files with 111 additions and 41 deletions.
diff --git a/dds/DCPS/ConfigStoreImpl.h b/dds/DCPS/ConfigStoreImpl.h
@@ -172,7 +172,6 @@ class OpenDDS_Dcps_Export ConfigStoreImpl : public ConfigStore {
         const EnumList<T> decoder[])
   {
     bool found = false;
-    // Encode the default.
     String value_as_string;
     for (size_t idx = 0; decoder[idx].name; ++idx) {
       if (decoder[idx].value == value) {
@@ -184,8 +183,8 @@ class OpenDDS_Dcps_Export ConfigStoreImpl : public ConfigStore {
 
     if (!found && log_level >= LogLevel::Warning) {
       ACE_ERROR((LM_WARNING,
-                 ACE_TEXT("(%P|%t) WARNING: ConfigStoreImpl::get: ")
-                 ACE_TEXT("failed to convert default value to string\n")));
+                 "(%P|%t) WARNING: ConfigStoreImpl::get: "
+                 "failed to convert default value to string\n"));
     }
 
     const String actual = get(key, value_as_string);
@@ -197,14 +196,66 @@ class OpenDDS_Dcps_Export ConfigStoreImpl : public ConfigStore {
 
     if (log_level >= LogLevel::Warning) {
       ACE_ERROR((LM_WARNING,
-                 ACE_TEXT("(%P|%t) WARNING: ConfigStoreImpl::get: ")
-                 ACE_TEXT("failed to encode (%C) to enumerated value, defaulting to (%C)\n"),
-                 actual.c_str(), value_as_string.c_str()));
+                 "(%P|%t) WARNING: ConfigStoreImpl::get: "
+                 "for %C, failed to encode (%C) to enumerated value, defaulting to (%C)\n",
+                 key, actual.c_str(), value_as_string.c_str()));
     }
 
     return value;
   }
 
+  template<typename T>
+  void set(const char* key,
+           T value,
+           const EnumList<T> decoder[])
+  {
+    bool found = false;
+    String value_as_string;
+    for (size_t idx = 0; decoder[idx].name; ++idx) {
+      if (decoder[idx].value == value) {
+        value_as_string = decoder[idx].name;
+        found = true;
+        break;
+      }
+    }
+
+    if (!found) {
+      if (log_level >= LogLevel::Warning) {
+        ACE_ERROR((LM_WARNING,
+                   "(%P|%t) WARNING: ConfigStoreImpl::set: "
+                   "for %C, failed to convert enum value to string\n",
+                   key));
+      }
+      return;
+    }
+
+    set(key, value_as_string);
+  }
+
+  template<typename T>
+  void set(const char* key,
+           const String& value,
+           const EnumList<T> decoder[])
+  {
+    bool found = false;
+    // Sanity check.
+    String value_as_string;
+    for (size_t idx = 0; decoder[idx].name; ++idx) {
+      if (value == decoder[idx].name) {
+        set(key, value);
+        found = true;
+        break;
+      }
+    }
+
+    if (!found && log_level >= LogLevel::Warning) {
+      ACE_ERROR((LM_WARNING,
+                 "(%P|%t) WARNING: ConfigStoreImpl::set: "
+                 "for %C, %C is not a valid enum value\n",
+                 key, value.c_str()));
+    }
+  }
+
   void set(const char* key,
            const TimeDuration& value,
            TimeFormat format);

diff --git a/dds/DCPS/Service_Participant.cpp b/dds/DCPS/Service_Participant.cpp
@@ -2283,51 +2283,31 @@ Service_Participant::get_type_object(DDS::DomainParticipant_ptr participant,
   return XTypes::TypeObject();
 }
 
+namespace {
+  const EnumList<Service_Participant::TypeObjectEncoding> type_object_encoding_kinds[] =
+    {
+      { Service_Participant::Encoding_Normal, "Normal" },
+      { Service_Participant::Encoding_WriteOldFormat, "WriteOldFormat" },
+      { Service_Participant::Encoding_ReadOldFormat, "ReadOldFormat" },
+      { Service_Participant::Encoding_Normal, 0 }
+    };
+}
+
 Service_Participant::TypeObjectEncoding
 Service_Participant::type_object_encoding() const
 {
-  String encoding_str = "Normal";
-  config_store_->get(COMMON_DCPS_TYPE_OBJECT_ENCODING, encoding_str);
-
-  struct NameValue {
-    const char* name;
-    TypeObjectEncoding value;
-  };
-  static const NameValue entries[] = {
-    {"Normal", Encoding_Normal},
-    {"WriteOldFormat", Encoding_WriteOldFormat},
-    {"ReadOldFormat", Encoding_ReadOldFormat},
-  };
-  for (size_t i = 0; i < sizeof entries / sizeof entries[0]; ++i) {
-    if (0 == std::strcmp(entries[i].name, encoding_str.c_str())) {
-      return entries[i].value;
-    }
-  }
-  ACE_ERROR((LM_ERROR, "(%P|%t) ERROR: Service_Participant::type_object_encoding: "
-             "invalid encoding %C\n", encoding_str.c_str()));
-
-  return Encoding_Normal;
+  return config_store_->get(COMMON_DCPS_TYPE_OBJECT_ENCODING, Encoding_Normal, type_object_encoding_kinds);
 }
 
 void Service_Participant::type_object_encoding(TypeObjectEncoding encoding)
 {
-  switch (encoding) {
-  case Encoding_Normal:
-    config_store_->set_string(COMMON_DCPS_TYPE_OBJECT_ENCODING, "Normal");
-    break;
-  case Encoding_WriteOldFormat:
-    config_store_->set_string(COMMON_DCPS_TYPE_OBJECT_ENCODING, "WriteOldFormat");
-    break;
-  case Encoding_ReadOldFormat:
-    config_store_->set_string(COMMON_DCPS_TYPE_OBJECT_ENCODING, "ReadOldFormat");
-    break;
-  }
+  config_store_->set(COMMON_DCPS_TYPE_OBJECT_ENCODING, encoding, type_object_encoding_kinds);
 }
 
 void
 Service_Participant::type_object_encoding(const char* encoding)
 {
-  config_store_->set_string(COMMON_DCPS_TYPE_OBJECT_ENCODING, encoding);
+  config_store_->set(COMMON_DCPS_TYPE_OBJECT_ENCODING, encoding, type_object_encoding_kinds);
 }
 
 unsigned int

diff --git a/docs/news.d/type-object-encoding.rst b/docs/news.d/type-object-encoding.rst
@@ -0,0 +1,5 @@
+.. news-prs: 4593
+
+.. news-start-section: Fixes
+- Fix bug where ``Service_Participant::type_object_encoding`` doesn't return configured value.
+.. news-end-section
diff --git a/tests/unit-tests/dds/DCPS/ConfigStoreImpl.cpp b/tests/unit-tests/dds/DCPS/ConfigStoreImpl.cpp
@@ -242,7 +242,8 @@ TEST(dds_DCPS_ConfigStoreImpl, set_get_StringList)
 enum MyConfigStoreEnum {
   ALPHA,
   BETA,
-  GAMMA
+  GAMMA,
+  DELTA
 };
 
 TEST(dds_DCPS_ConfigStoreImpl, set_get_Enum)
@@ -257,8 +258,26 @@ TEST(dds_DCPS_ConfigStoreImpl, set_get_Enum)
 
   ConfigTopic_rch topic = make_rch<ConfigTopic>();
   ConfigStoreImpl store(topic);
+  // Get the default if there is no entry.
   EXPECT_EQ(store.get("key", GAMMA, kinds), GAMMA);
-  store.set("key", "beta");
+
+  // Default not in helper works.
+  EXPECT_EQ(store.get("key", DELTA, kinds), DELTA);
+
+  // Setting with enum works.
+  store.set("key", ALPHA, kinds);
+  EXPECT_EQ(store.get("key", GAMMA, kinds), ALPHA);
+
+  // Setting with string works.
+  store.set("key", "beta", kinds);
+  EXPECT_EQ(store.get("key", GAMMA, kinds), BETA);
+
+  // Setting with enum that is not in helper does nothing.
+  store.set("key", DELTA, kinds);
+  EXPECT_EQ(store.get("key", GAMMA, kinds), BETA);
+
+  // Setting with enum that is not in helper does nothing.
+  store.set("key", "delta", kinds);
   EXPECT_EQ(store.get("key", GAMMA, kinds), BETA);
 }
 

diff --git a/tests/unit-tests/dds/DCPS/Service_Participant.cpp b/tests/unit-tests/dds/DCPS/Service_Participant.cpp
@@ -0,0 +1,15 @@
+#include <dds/DCPS/Service_Participant.h>
+
+#include <gtestWrapper.h>
+
+using namespace OpenDDS::DCPS;
+
+TEST(dds_DCPS_Service_Participant, type_object_encoding) {
+  Service_Participant sp;
+
+  EXPECT_EQ(sp.type_object_encoding(), Service_Participant::Encoding_Normal);
+  sp.type_object_encoding(Service_Participant::Encoding_WriteOldFormat);
+  EXPECT_EQ(sp.type_object_encoding(), Service_Participant::Encoding_WriteOldFormat);
+  sp.type_object_encoding("ReadOldFormat");
+  EXPECT_EQ(sp.type_object_encoding(), Service_Participant::Encoding_ReadOldFormat);
+}