Merge pull request #63 from sawenzel/cfg/dynamic-improvements

ConfigurableParam: reject malformed container fields, drop redundant parseSet

Author: f3sch

Common/Utils/include/CommonUtils/ConfigurableParam.h

@@ -162,11 +162,6 @@ concept MapLike = Container<T> && requires {
 template <typename T>
 concept SequenceContainer = Container<T> && !MapLike<T>;
 
-template <typename T>
-concept HasPushBack = requires(T a, typename T::value_type v) {
-  { a.push_back(v) } -> std::same_as<void>;
-};
-
 template <typename>
 inline constexpr bool AlwaysFalse = false;
 
@@ -178,9 +173,12 @@ class ContainerParser
   {
     if constexpr (MapLike<T>) {
       return parseMap<T>(str);
-    } else if constexpr (SequenceContainer<T>) {
-      return parseSet<T>(str);
     } else if constexpr (Container<T>) {
+      // Covers vector/list/deque as well as set/unordered_set: parseSequence
+      // inserts at end(), which both sequence and associative-set containers
+      // accept. (Any non-map Container is a SequenceContainer, so the previous
+      // separate parseSet branch was unreachable and re-parsed into a temporary
+      // vector first.)
       return parseSequence<T>(str);
     } else {
       return parseScalar<T>(str);
@@ -198,7 +196,7 @@ class ContainerParser
   }
 
  private:
-  // Parse vector, list, deque, array
+  // Parse sequence and set containers (vector, list, deque, set, unordered_set)
   template <SequenceContainer SequenceT>
   static SequenceT parseSequence(const std::string& str)
   {
@@ -251,23 +249,6 @@ class ContainerParser
     return result;
   }
 
-  // Parse set containers
-  template <SequenceContainer SetT>
-  static SetT parseSet(const std::string& str)
-  {
-    SetT result;
-    using ValueType = typename SetT::value_type;
-    auto vec = parseSequence<std::vector<ValueType>>(str);
-    for (const auto& val : vec) {
-      if constexpr (HasPushBack<SetT>) {
-        result.push_back(val);
-      } else {
-        result.insert(val);
-      }
-    }
-    return result;
-  }
-
   // Parse scalar types
   template <typename T>
   static T parseScalar(const std::string& str)
@@ -357,17 +338,17 @@ class ContainerParser
       } else if (c == '}') {
         brace_depth--;
       } else if (c == delimiter && bracket_depth == 0 && brace_depth == 0) {
-        if (!current.empty()) {
-          tokens.push_back(current);
-          current.clear();
-        }
+        // Keep empty fields: a stray delimiter (e.g. "[1,,3]" or "key:") must
+        // surface as a parse error downstream rather than silently dropping an
+        // element. The empty-container case ("[]"/"{}") is handled by the
+        // callers before split() is ever reached.
+        tokens.push_back(current);
+        current.clear();
         continue;
       }
       current += c;
     }
-    if (!current.empty()) {
-      tokens.push_back(current);
-    }
+    tokens.push_back(current);
     return tokens;
   }
 };

Common/Utils/test/testConfigurableParam.cxx

@@ -18,7 +18,12 @@
 #include <boost/test/unit_test.hpp>
 #include <boost/property_tree/ptree.hpp>
 #include <cstdlib>
+#include <deque>
 #include <filesystem>
+#include <list>
+#include <set>
+#include <unordered_set>
+#include <vector>
 
 #include "CommonUtils/ConfigurableParamTest.h"
 
@@ -200,6 +205,40 @@ BOOST_AUTO_TEST_CASE(ConfigurableParam_ContainerParserMap)
   BOOST_CHECK_EQUAL(m["beta"], 0.3);
 }
 
+BOOST_AUTO_TEST_CASE(ConfigurableParam_ContainerParserSetAndSequenceTypes)
+{
+  // All non-map containers go through the single parseSequence path. Verify the
+  // set containers (which previously took a separate, doubly-parsing parseSet
+  // branch) still parse and de-duplicate correctly.
+  auto se = ContainerParser::parse<std::set<int>>("[2,3,2,3,2,5]");
+  BOOST_CHECK_EQUAL(se.size(), 3);
+  BOOST_CHECK(se == (std::set<int>{2, 3, 5}));
+
+  auto us = ContainerParser::parse<std::unordered_set<int>>("[1,2,3,3]");
+  BOOST_CHECK_EQUAL(us.size(), 3);
+  BOOST_CHECK(us.count(1) && us.count(2) && us.count(3));
+
+  auto li = ContainerParser::parse<std::list<int>>("[7,8]");
+  BOOST_CHECK(li == (std::list<int>{7, 8}));
+
+  auto dq = ContainerParser::parse<std::deque<int>>("[9,10]");
+  BOOST_CHECK(dq == (std::deque<int>{9, 10}));
+}
+
+BOOST_AUTO_TEST_CASE(ConfigurableParam_ContainerParserRejectsEmptyToken)
+{
+  // A stray delimiter must be reported, not silently swallowed: "[1,,3]" used to
+  // parse to a 2-element vector with no error, masking malformed configuration.
+  BOOST_CHECK_THROW(ContainerParser::parse<std::vector<int>>("[1,,3]"), std::exception);
+  BOOST_CHECK_THROW(ContainerParser::parse<std::vector<int>>("[1,2,]"), std::exception);
+  // ... and on the map side, an empty value/entry is rejected too.
+  BOOST_CHECK_THROW((ContainerParser::parse<std::map<int, int>>("{1:2,,3:4}")), std::exception);
+  BOOST_CHECK_THROW((ContainerParser::parse<std::map<int, int>>("{1:}")), std::exception);
+  // Well-formed input is unaffected.
+  auto v = ContainerParser::parse<std::vector<int>>("[1,2,3]");
+  BOOST_CHECK_EQUAL(v.size(), 3);
+}
+
 BOOST_AUTO_TEST_CASE(ConfigurableParam_DamerauLevenshteinDistance)
 {
   BOOST_CHECK_EQUAL(damerauLevenshteinDistance("TestParam.iValue", "TestParam.iValue"), 0);
@@ -255,5 +294,5 @@ BOOST_AUTO_TEST_CASE(ConfigurableParam_Container_FileIO_ROOT)
   for (const auto& s : set) {
     BOOST_CHECK(tp.set.contains(s));
   }
-  // std::remove(testFileName.c_str());
+  std::remove(testFileName.c_str());
 }