From 3fa3096fc1fe00e2c5a454278b24e2aaa1c2faec Mon Sep 17 00:00:00 2001 From: Pavel Guzenfeld Date: Sat, 4 Apr 2026 03:11:25 +0300 Subject: [PATCH] Auto-quote bare string parameters for DDS content filter expressions Fast DDS requires content filter expression parameters to be valid DDS SQL literals. Bare strings like "hello" fail to parse in DDSFilterParameter::set_value() because the DDSSQLFilter grammar expects string literals to be single-quoted ('hello'). This causes content filter parameter substitution (%0, %1, ...) to silently fail for string-typed fields when users pass unquoted values, which is the natural thing to do from a CLI or application perspective. Add ensure_dds_literal() that detects bare strings (not already a number, boolean, or quoted string) and wraps them in single quotes before passing to Fast DDS. Applied at both content filter creation and dynamic filter update call sites. References: https://github.com/eProsima/Fast-DDS/issues/4199 --- .../include/rmw_fastrtps_shared_cpp/utils.hpp | 55 +++++++++++++ .../src/rmw_subscription.cpp | 6 +- rmw_fastrtps_shared_cpp/src/utils.cpp | 5 +- rmw_fastrtps_shared_cpp/test/CMakeLists.txt | 5 ++ .../test/test_ensure_dds_literal.cpp | 79 +++++++++++++++++++ 5 files changed, 142 insertions(+), 8 deletions(-) create mode 100644 rmw_fastrtps_shared_cpp/test/test_ensure_dds_literal.cpp diff --git a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/utils.hpp b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/utils.hpp index 6a501c71da..ccc1ca2fc3 100644 --- a/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/utils.hpp +++ b/rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/utils.hpp @@ -15,8 +15,10 @@ #ifndef RMW_FASTRTPS_SHARED_CPP__UTILS_HPP_ #define RMW_FASTRTPS_SHARED_CPP__UTILS_HPP_ +#include #include #include +#include #include "fastdds/dds/topic/TopicDescription.hpp" #include "fastdds/dds/topic/TypeSupport.hpp" @@ -33,6 +35,59 @@ namespace rmw_fastrtps_shared_cpp { +/// Ensure a content filter parameter is a valid DDS SQL literal. +/// +/// Fast DDS requires content filter parameters to be parseable as DDS SQL +/// literals. Bare strings like "hello" must be wrapped in single quotes to +/// become valid string literals ('hello'), otherwise +/// DDSFilterParameter::set_value() fails with a parse error. +/// See https://github.com/eProsima/Fast-DDS/issues/4199 +inline std::string +ensure_dds_literal(const std::string & value) +{ + if (value.empty()) { + return "'" + value + "'"; + } + + // Already a quoted string + if ((value.front() == '\'' || value.front() == '`') && value.size() >= 2 && + value.back() == value.front()) + { + return value; + } + + // Boolean literals + if (value == "TRUE" || value == "FALSE") { + return value; + } + + // Numeric: optional leading sign, then digit or dot (covers int, float, hex) + const char * p = value.c_str(); + if (*p == '+' || *p == '-') { + ++p; + } + if (*p == '0' && (*(p + 1) == 'x' || *(p + 1) == 'X')) { + return value; // hex + } + if (std::isdigit(static_cast(*p)) || *p == '.') { + return value; // numeric + } + + // Not a recognized literal — wrap in single quotes + return "'" + value + "'"; +} + +/// Convert rmw content filter parameters, auto-quoting bare strings for Fast DDS. +inline std::vector +prepare_content_filter_parameters(const rmw_subscription_content_filter_options_t * options) +{ + std::vector expression_parameters; + for (size_t i = 0; i < options->expression_parameters.size; ++i) { + expression_parameters.push_back(ensure_dds_literal(options->expression_parameters.data[i])); + } + return expression_parameters; +} + /** * Convert a Fast DDS return code into the corresponding rmw_ret_t * \param[in] code The Fast DDS return code to be translated diff --git a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp index 98e471563c..72dee4bede 100644 --- a/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp +++ b/rmw_fastrtps_shared_cpp/src/rmw_subscription.cpp @@ -120,10 +120,8 @@ __rmw_subscription_set_content_filter( "current subscriber has no content filter topic"); return RMW_RET_ERROR; } else if (filtered_topic && !filter_expression_empty) { - std::vector expression_parameters; - for (size_t i = 0; i < options->expression_parameters.size; ++i) { - expression_parameters.push_back(options->expression_parameters.data[i]); - } + std::vector expression_parameters = + rmw_fastrtps_shared_cpp::prepare_content_filter_parameters(options); eprosima::fastdds::dds::ReturnCode_t ret = filtered_topic->set_filter_expression(options->filter_expression, expression_parameters); diff --git a/rmw_fastrtps_shared_cpp/src/utils.cpp b/rmw_fastrtps_shared_cpp/src/utils.cpp index a781020107..674487c2c3 100644 --- a/rmw_fastrtps_shared_cpp/src/utils.cpp +++ b/rmw_fastrtps_shared_cpp/src/utils.cpp @@ -102,10 +102,7 @@ create_content_filtered_topic( const rmw_subscription_content_filter_options_t * options, eprosima::fastdds::dds::ContentFilteredTopic ** content_filtered_topic) { - std::vector expression_parameters; - for (size_t i = 0; i < options->expression_parameters.size; ++i) { - expression_parameters.push_back(options->expression_parameters.data[i]); - } + std::vector expression_parameters = prepare_content_filter_parameters(options); auto topic = dynamic_cast(topic_desc); static std::atomic cft_counter{0}; diff --git a/rmw_fastrtps_shared_cpp/test/CMakeLists.txt b/rmw_fastrtps_shared_cpp/test/CMakeLists.txt index 33c38b28ee..b859ae8138 100644 --- a/rmw_fastrtps_shared_cpp/test/CMakeLists.txt +++ b/rmw_fastrtps_shared_cpp/test/CMakeLists.txt @@ -54,3 +54,8 @@ ament_add_gtest(test_logging test_logging.cpp) if(TARGET test_logging) target_link_libraries(test_logging ${PROJECT_NAME} rmw::rmw) endif() + +ament_add_gtest(test_ensure_dds_literal test_ensure_dds_literal.cpp) +if(TARGET test_ensure_dds_literal) + target_link_libraries(test_ensure_dds_literal ${PROJECT_NAME} rmw::rmw) +endif() diff --git a/rmw_fastrtps_shared_cpp/test/test_ensure_dds_literal.cpp b/rmw_fastrtps_shared_cpp/test/test_ensure_dds_literal.cpp new file mode 100644 index 0000000000..929499605f --- /dev/null +++ b/rmw_fastrtps_shared_cpp/test/test_ensure_dds_literal.cpp @@ -0,0 +1,79 @@ +// Copyright 2026 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include + +#include "rmw_fastrtps_shared_cpp/utils.hpp" + +using rmw_fastrtps_shared_cpp::ensure_dds_literal; + +// Bare strings should be wrapped in single quotes +TEST(EnsureDdsLiteral, bare_string_gets_quoted) +{ + EXPECT_EQ("'hello'", ensure_dds_literal("hello")); + EXPECT_EQ("'RED'", ensure_dds_literal("RED")); + EXPECT_EQ("'some value'", ensure_dds_literal("some value")); +} + +// Already single-quoted strings should pass through unchanged +TEST(EnsureDdsLiteral, already_single_quoted) +{ + EXPECT_EQ("'hello'", ensure_dds_literal("'hello'")); + EXPECT_EQ("'with spaces'", ensure_dds_literal("'with spaces'")); +} + +// Backtick-quoted strings should pass through unchanged +TEST(EnsureDdsLiteral, already_backtick_quoted) +{ + EXPECT_EQ("`hello`", ensure_dds_literal("`hello`")); +} + +// Boolean literals should pass through unchanged +TEST(EnsureDdsLiteral, boolean_literals) +{ + EXPECT_EQ("TRUE", ensure_dds_literal("TRUE")); + EXPECT_EQ("FALSE", ensure_dds_literal("FALSE")); +} + +// Integer literals should pass through unchanged +TEST(EnsureDdsLiteral, integer_literals) +{ + EXPECT_EQ("42", ensure_dds_literal("42")); + EXPECT_EQ("0", ensure_dds_literal("0")); + EXPECT_EQ("-1", ensure_dds_literal("-1")); + EXPECT_EQ("+5", ensure_dds_literal("+5")); +} + +// Float literals should pass through unchanged +TEST(EnsureDdsLiteral, float_literals) +{ + EXPECT_EQ("3.14", ensure_dds_literal("3.14")); + EXPECT_EQ(".5", ensure_dds_literal(".5")); + EXPECT_EQ("-0.1", ensure_dds_literal("-0.1")); +} + +// Hex literals should pass through unchanged +TEST(EnsureDdsLiteral, hex_literals) +{ + EXPECT_EQ("0xFF", ensure_dds_literal("0xFF")); + EXPECT_EQ("0X1A", ensure_dds_literal("0X1A")); +} + +// Empty string should be quoted +TEST(EnsureDdsLiteral, empty_string) +{ + EXPECT_EQ("''", ensure_dds_literal("")); +}