From 2ed6b7b512c2262af0a832340c26dff8edef9f7b Mon Sep 17 00:00:00 2001 From: Sandro Wenzel Date: Thu, 25 Jun 2026 21:00:51 +0200 Subject: [PATCH] ConfigurableParam: reject malformed container fields, drop redundant parseSet Follow-up hardening on top of the std-container ConfigurableParam support. 1. split() no longer silently drops empty fields. A stray delimiter such as "[1,,3]" previously parsed to a 2-element vector with no error, and "key:" collapsed an empty map value away. Both now produce an empty token that is rejected by parseScalar / the map-syntax check, so malformed configuration surfaces as an error instead of corrupting element counts. The empty container case ("[]" / "{}") is still short-circuited by the callers before split() runs, so well-formed input is unaffected. 2. Removed the redundant parseSet() branch (and the now-unused HasPushBack concept). Every non-map Container already satisfies SequenceContainer, so the parseSet dispatch was reached for all sequence and set types and re-parsed the string into a throwaway std::vector before copying element-by-element. parseSequence inserts at end(), which std::set / std::unordered_set accept just as well, so all non-map containers now share a single, single-pass path. 3. Re-enabled cleanup of test_config.root in ConfigurableParam_Container_FileIO_ROOT (the std::remove was left commented out), restoring test isolation. Adds regression tests: set/unordered_set/list/deque parsing through the unified parseSequence path (incl. set de-duplication), and rejection of empty tokens in both sequence and map parsing. Note: container parse failures remain non-fatal (logged), matching the existing scalar setValue behaviour; this commit does not change that contract. Co-Authored-By: Claude Opus 4.8 --- .../include/CommonUtils/ConfigurableParam.h | 45 ++++++------------- Common/Utils/test/testConfigurableParam.cxx | 41 ++++++++++++++++- 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/Common/Utils/include/CommonUtils/ConfigurableParam.h b/Common/Utils/include/CommonUtils/ConfigurableParam.h index 6aa6a7009dc57..0a6da30aa39f5 100644 --- a/Common/Utils/include/CommonUtils/ConfigurableParam.h +++ b/Common/Utils/include/CommonUtils/ConfigurableParam.h @@ -162,11 +162,6 @@ concept MapLike = Container && requires { template concept SequenceContainer = Container && !MapLike; -template -concept HasPushBack = requires(T a, typename T::value_type v) { - { a.push_back(v) } -> std::same_as; -}; - template inline constexpr bool AlwaysFalse = false; @@ -178,9 +173,12 @@ class ContainerParser { if constexpr (MapLike) { return parseMap(str); - } else if constexpr (SequenceContainer) { - return parseSet(str); } else if constexpr (Container) { + // 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(str); } else { return parseScalar(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 static SequenceT parseSequence(const std::string& str) { @@ -251,23 +249,6 @@ class ContainerParser return result; } - // Parse set containers - template - static SetT parseSet(const std::string& str) - { - SetT result; - using ValueType = typename SetT::value_type; - auto vec = parseSequence>(str); - for (const auto& val : vec) { - if constexpr (HasPushBack) { - result.push_back(val); - } else { - result.insert(val); - } - } - return result; - } - // Parse scalar types template 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; } }; diff --git a/Common/Utils/test/testConfigurableParam.cxx b/Common/Utils/test/testConfigurableParam.cxx index ed9ad19f1c35e..f0c3c59c79c35 100644 --- a/Common/Utils/test/testConfigurableParam.cxx +++ b/Common/Utils/test/testConfigurableParam.cxx @@ -18,7 +18,12 @@ #include #include #include +#include #include +#include +#include +#include +#include #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>("[2,3,2,3,2,5]"); + BOOST_CHECK_EQUAL(se.size(), 3); + BOOST_CHECK(se == (std::set{2, 3, 5})); + + auto us = ContainerParser::parse>("[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>("[7,8]"); + BOOST_CHECK(li == (std::list{7, 8})); + + auto dq = ContainerParser::parse>("[9,10]"); + BOOST_CHECK(dq == (std::deque{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>("[1,,3]"), std::exception); + BOOST_CHECK_THROW(ContainerParser::parse>("[1,2,]"), std::exception); + // ... and on the map side, an empty value/entry is rejected too. + BOOST_CHECK_THROW((ContainerParser::parse>("{1:2,,3:4}")), std::exception); + BOOST_CHECK_THROW((ContainerParser::parse>("{1:}")), std::exception); + // Well-formed input is unaffected. + auto v = ContainerParser::parse>("[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()); }