From 5ae8d59e759e494a97efc802cbfb8047b700fac3 Mon Sep 17 00:00:00 2001 From: Chen Lihui Date: Thu, 9 Dec 2021 15:37:37 +0800 Subject: [PATCH] use regex for wildcard matching Co-authored-by: Aaron Lipinski Signed-off-by: Chen Lihui --- .../detail/resolve_parameter_overrides.cpp | 26 +++++-- .../node_interfaces/test_node_parameters.cpp | 69 +++++++++++++++++++ .../test_node_parameters/wildcards.yaml | 57 +++++++++++++++ 3 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 rclcpp/test/resources/test_node_parameters/wildcards.yaml diff --git a/rclcpp/src/rclcpp/detail/resolve_parameter_overrides.cpp b/rclcpp/src/rclcpp/detail/resolve_parameter_overrides.cpp index 15f567f7c5..010862703a 100644 --- a/rclcpp/src/rclcpp/detail/resolve_parameter_overrides.cpp +++ b/rclcpp/src/rclcpp/detail/resolve_parameter_overrides.cpp @@ -16,9 +16,11 @@ #include #include +#include #include #include "rcl_yaml_param_parser/parser.h" +#include "rcpputils/find_and_replace.hpp" #include "rcpputils/scope_exit.hpp" #include "rclcpp/parameter_map.hpp" @@ -51,20 +53,30 @@ rclcpp::detail::resolve_parameter_overrides( [params]() { rcl_yaml_node_struct_fini(params); }); + // TODO(iuhilnehc-ynos) use map instead of unordered_map for ParameterMap + // to set param with contents of parameter file by order rclcpp::ParameterMap initial_map = rclcpp::parameter_map_from(params); - // Enforce wildcard matching precedence - // TODO(cottsay) implement further wildcard matching - const std::array node_matching_names{"/**", node_fqn}; - for (const auto & node_name : node_matching_names) { - if (initial_map.count(node_name) > 0) { - // Combine parameter yaml files, overwriting values in older ones - for (const rclcpp::Parameter & param : initial_map.at(node_name)) { + for (auto & item : initial_map) { + if (item.first == node_fqn) { + continue; + } + // Update the regular expression ["/*" -> "(/\\w+)" and "/**" -> "(/\\w+)*"] + std::string regex = rcpputils::find_and_replace(item.first, "/*", "(/\\w+)"); + if (std::regex_match(node_fqn, std::regex(regex))) { + for (const rclcpp::Parameter & param : item.second) { result[param.get_name()] = rclcpp::ParameterValue(param.get_value_message()); } } } + if (initial_map.count(node_fqn) > 0) { + // Combine parameter yaml files, overwriting values in older ones + for (const rclcpp::Parameter & param : initial_map.at(node_fqn)) { + result[param.get_name()] = + rclcpp::ParameterValue(param.get_value_message()); + } + } } } diff --git a/rclcpp/test/rclcpp/node_interfaces/test_node_parameters.cpp b/rclcpp/test/rclcpp/node_interfaces/test_node_parameters.cpp index 31b755b4a7..43f48c4aca 100644 --- a/rclcpp/test/rclcpp/node_interfaces/test_node_parameters.cpp +++ b/rclcpp/test/rclcpp/node_interfaces/test_node_parameters.cpp @@ -31,6 +31,8 @@ #include "../../mocking_utils/patch.hpp" #include "../../utils/rclcpp_gtest_macros.hpp" +#include "rcpputils/filesystem_helper.hpp" + class TestNodeParameters : public ::testing::Test { public: @@ -47,6 +49,7 @@ class TestNodeParameters : public ::testing::Test dynamic_cast( node->get_node_parameters_interface().get()); ASSERT_NE(nullptr, node_parameters); + test_resources_path /= "test_node_parameters"; } void TearDown() @@ -57,6 +60,8 @@ class TestNodeParameters : public ::testing::Test protected: std::shared_ptr node; rclcpp::node_interfaces::NodeParameters * node_parameters; + + rcpputils::fs::path test_resources_path{TEST_RESOURCES_DIRECTORY}; }; TEST_F(TestNodeParameters, construct_destruct_rcl_errors) { @@ -199,3 +204,67 @@ TEST_F(TestNodeParameters, add_remove_parameters_callback) { node_parameters->remove_on_set_parameters_callback(handle.get()), std::runtime_error("Callback doesn't exist")); } + +TEST_F(TestNodeParameters, wildcard_with_namespace) +{ + rclcpp::NodeOptions opts; + opts.arguments( + { + "--ros-args", + "--params-file", (test_resources_path / "wildcards.yaml").string() + }); + + std::shared_ptr node = std::make_shared("node", "ns", opts); + + auto * node_parameters = + dynamic_cast( + node->get_node_parameters_interface().get()); + ASSERT_NE(nullptr, node_parameters); + + const auto & parameter_overrides = node_parameters->get_parameter_overrides(); + EXPECT_EQ(7u, parameter_overrides.size()); + EXPECT_EQ(parameter_overrides.at("full_wild").get(), "full_wild"); + EXPECT_EQ(parameter_overrides.at("namespace_wild").get(), "namespace_wild"); + EXPECT_EQ( + parameter_overrides.at("namespace_wild_another").get(), + "namespace_wild_another"); + EXPECT_EQ( + parameter_overrides.at("namespace_wild_one_star").get(), + "namespace_wild_one_star"); + EXPECT_EQ(parameter_overrides.at("node_wild_in_ns").get(), "node_wild_in_ns"); + EXPECT_EQ( + parameter_overrides.at("node_wild_in_ns_another").get(), + "node_wild_in_ns_another"); + EXPECT_EQ(parameter_overrides.at("explicit_in_ns").get(), "explicit_in_ns"); + EXPECT_EQ(parameter_overrides.count("should_not_appear"), 0u); +} + +TEST_F(TestNodeParameters, wildcard_no_namespace) +{ + rclcpp::NodeOptions opts; + opts.arguments( + { + "--ros-args", + "--params-file", (test_resources_path / "wildcards.yaml").string() + }); + + std::shared_ptr node = std::make_shared("node", opts); + + auto * node_parameters = + dynamic_cast( + node->get_node_parameters_interface().get()); + ASSERT_NE(nullptr, node_parameters); + + const auto & parameter_overrides = node_parameters->get_parameter_overrides(); + EXPECT_EQ(5u, parameter_overrides.size()); + EXPECT_EQ(parameter_overrides.at("full_wild").get(), "full_wild"); + EXPECT_EQ(parameter_overrides.at("namespace_wild").get(), "namespace_wild"); + EXPECT_EQ( + parameter_overrides.at("namespace_wild_another").get(), + "namespace_wild_another"); + EXPECT_EQ(parameter_overrides.at("node_wild_no_ns").get(), "node_wild_no_ns"); + EXPECT_EQ(parameter_overrides.at("explicit_no_ns").get(), "explicit_no_ns"); + EXPECT_EQ(parameter_overrides.count("should_not_appear"), 0u); + // "/*" match exactly one token, not expect to get `namespace_wild_one_star` + EXPECT_EQ(parameter_overrides.count("namespace_wild_one_star"), 0u); +} diff --git a/rclcpp/test/resources/test_node_parameters/wildcards.yaml b/rclcpp/test/resources/test_node_parameters/wildcards.yaml new file mode 100644 index 0000000000..46371435be --- /dev/null +++ b/rclcpp/test/resources/test_node_parameters/wildcards.yaml @@ -0,0 +1,57 @@ +/**: + ros__parameters: + full_wild: "full_wild" + +/**: + node: + ros__parameters: + namespace_wild: "namespace_wild" + +/**/node: + ros__parameters: + namespace_wild_another: "namespace_wild_another" + +/*: + node: + ros__parameters: + namespace_wild_one_star: "namespace_wild_one_star" + +ns: + "*": + ros__parameters: + node_wild_in_ns: "node_wild_in_ns" + +/ns/*: + ros__parameters: + node_wild_in_ns_another: "node_wild_in_ns_another" + +ns: + node: + ros__parameters: + explicit_in_ns: "explicit_in_ns" + +"*": + ros__parameters: + node_wild_no_ns: "node_wild_no_ns" + +node: + ros__parameters: + explicit_no_ns: "explicit_no_ns" + +ns: + nodeX: + ros__parameters: + should_not_appear: "incorrect_node_name" + +/**/nodeX: + ros__parameters: + should_not_appear: "incorrect_node_name" + +nsX: + node: + ros__parameters: + should_not_appear: "incorrect_namespace" + +/nsX/*: + ros__parameters: + should_not_appear: "incorrect_namespace"