diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index d4cad2a6..04386729 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -592,6 +592,12 @@ if(BUILD_TESTING) ament_add_gtest(test_error_info test/test_error_info.cpp) target_link_libraries(test_error_info gateway_ros2) + # Parameter error precedence tests (multi-node GET aggregation, live GatewayNode) + ament_add_gtest(test_parameter_error_precedence test/test_parameter_error_precedence.cpp) + target_link_libraries(test_parameter_error_precedence gateway_ros2) + medkit_target_dependencies(test_parameter_error_precedence rclcpp) + medkit_set_test_domain(test_parameter_error_precedence) + # Add ROS 2 subscription executor tests ament_add_gtest(test_ros2_subscription_executor test/test_ros2_subscription_executor.cpp) target_link_libraries(test_ros2_subscription_executor gateway_ros2) @@ -1051,6 +1057,7 @@ if(BUILD_TESTING) test_tls_config test_fault_manager test_error_info + test_parameter_error_precedence test_lifecycle_status_helpers test_ros2_lifecycle_state_reader test_ros2_subscription_executor diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index cd4534d3..09787440 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -114,16 +114,16 @@ const NodeConfigInfo * find_node_for_app(const std::vector & nod return nullptr; } -/// Error classification result for parameter operations. -struct ErrorClassification { - int status_code; +/// HTTP status + SOVD error code for a failed parameter operation. +struct ParameterErrorClassification { + int status_code = 500; std::string error_code; }; /// Map a structured `ParameterErrorCode` to an HTTP status + SOVD error code. /// Identical mapping to the legacy `classify_error_code` helper. -ErrorClassification classify_error_code(ParameterErrorCode error_code) { - ErrorClassification result; +ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) { + ParameterErrorClassification result; switch (error_code) { case ParameterErrorCode::NOT_FOUND: @@ -165,14 +165,14 @@ ErrorClassification classify_error_code(ParameterErrorCode error_code) { /// Classify a ParameterResult into HTTP status + error code. Prefers the /// structured `error_code` field and falls back to string-matching the legacy /// error messages for backward compatibility with older transports. -ErrorClassification classify_parameter_error(const ParameterResult & result) { +ParameterErrorClassification classify_parameter_error(const ParameterResult & result) { // Prefer structured error code if (result.error_code != ParameterErrorCode::NONE) { return classify_error_code(result.error_code); } // Fallback to string parsing for backward compatibility - ErrorClassification classification; + ParameterErrorClassification classification; const auto & error_message = result.error_message; if (error_message.find("not found") != std::string::npos || @@ -312,6 +312,66 @@ void apply_fan_out_observability(dto::ConfigListXMedkit & xm, } } +/// Severity rank for order-independent folding: node-unavailable (503) is the +/// most actionable verdict (retry), then other server errors, then non-404 +/// client errors; NOT_FOUND ranks lowest so it can never mask a real failure. +int severity_rank(int status_code) { + if (status_code == 404) { + return 0; + } + if (status_code == 503) { + return 3; + } + if (status_code >= 500) { + return 2; + } + return 1; // non-404 4xx +} + +/// Folds per-node parameter lookup failures so the error surfaced when no +/// node succeeds does not depend on node iteration order. Failures are +/// ranked by `severity_rank`; the highest rank folded so far is kept, and +/// within a rank the first failure folded wins. +class ParameterErrorAccumulator { + public: + /// Fold one failed ParameterResult. + void add(const ParameterResult & result) { + auto err = classify_parameter_error(result); + const int rank = severity_rank(err.status_code); + if (rank > 0) { + all_not_found_ = false; + } + // Strictly-greater keeps the first failure folded at the winning rank, so + // the surfaced status never depends on node iteration order. + if (rank > worst_rank_) { + worst_ = result; + classification_ = std::move(err); + worst_rank_ = rank; + } + } + + /// True when every folded failure classified as 404. + bool all_not_found() const { + return all_not_found_; + } + + /// The failure to surface: the highest-severity failure folded so far. + const ParameterResult & worst() const { + return worst_; + } + + /// Classification of worst(). + const ParameterErrorClassification & classification() const { + return classification_; + } + + private: + ParameterResult worst_; + ParameterErrorClassification classification_; + int worst_rank_ = -1; + bool all_not_found_ = true; +}; + } // namespace // =========================================================================== @@ -476,10 +536,10 @@ http::Result ConfigHandlers::get_configuration(cons // For non-aggregated entities (or aggregated entities without a prefix) we // probe every backing node for the parameter and return the first success. - // Track errors so a non-404 from any node (e.g. unavailable) wins over a - // pure "not found" verdict in the no-success branch. - ParameterResult last_result; - bool all_not_found = true; + // Failures fold through ParameterErrorAccumulator, which ranks them by + // severity (503 > other 5xx > non-404 4xx > 404), so the surfaced error is + // the same regardless of node iteration order. + ParameterErrorAccumulator errors; for (const auto & node_info : agg_configs.nodes) { auto result = config_mgr->get_parameter(node_info.node_fqn, parsed.param_name); @@ -491,23 +551,19 @@ http::Result ConfigHandlers::get_configuration(cons return make_read_value(entity_id, node_info.node_fqn, parsed.param_name, source_app, result.data); } - last_result = result; - auto err = classify_parameter_error(result); - if (err.status_code != 404) { - all_not_found = false; - } + errors.add(result); } - if (all_not_found) { + if (errors.all_not_found()) { return tl::unexpected(make_error(404, ERR_RESOURCE_NOT_FOUND, "Parameter not found", json{{"entity_id", entity_id}, {"id", param_id}})); } // Some node reported a non-404 (e.g. unavailable); surface that. - auto err = classify_parameter_error(last_result); + const auto & err = errors.classification(); return tl::unexpected( make_error(err.status_code, err.error_code, "Failed to get parameter from any node", - json{{"details", last_result.error_message}, {"entity_id", entity_id}, {"id", param_id}})); + json{{"details", errors.worst().error_message}, {"entity_id", entity_id}, {"id", param_id}})); } // =========================================================================== diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp new file mode 100644 index 00000000..ddb16498 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -0,0 +1,273 @@ +// Copyright 2026 bburda +// +// 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 + +#include +#include +#include +#include +#include +#include + +#include "ros2_medkit_gateway/core/discovery/models/app.hpp" +#include "ros2_medkit_gateway/core/discovery/models/component.hpp" +#include "ros2_medkit_gateway/core/http/error_codes.hpp" +#include "ros2_medkit_gateway/core/http/handlers/config_handlers.hpp" +#include "ros2_medkit_gateway/core/models/thread_safe_entity_cache.hpp" +#include "ros2_medkit_gateway/gateway_node.hpp" +#include "ros2_medkit_gateway/http/typed_router.hpp" + +using ros2_medkit_gateway::App; +using ros2_medkit_gateway::AuthConfig; +using ros2_medkit_gateway::Component; +using ros2_medkit_gateway::CorsConfig; +using ros2_medkit_gateway::ERR_RESOURCE_NOT_FOUND; +using ros2_medkit_gateway::ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; +using ros2_medkit_gateway::ErrorInfo; +using ros2_medkit_gateway::GatewayNode; +using ros2_medkit_gateway::ThreadSafeEntityCache; +using ros2_medkit_gateway::TlsConfig; +using ros2_medkit_gateway::handlers::ConfigHandlers; +using ros2_medkit_gateway::handlers::HandlerContext; +namespace http = ros2_medkit_gateway::http; + +namespace { + +constexpr const char * kMissingParam = "definitely_missing_param"; +constexpr const char * kGetConfigPattern = R"(/api/v1/components/([^/]+)/configurations/([^/]+))"; + +httplib::Request make_request_with_match(const std::string & path, const std::string & pattern) { + httplib::Request req; + req.path = path; + std::regex re(pattern); + std::regex_match(req.path, req.matches, re); + return req; +} + +} // namespace + +// ============================================================================= +// Handler-level regression tests for the multi-node probe loop in +// GET /{entity}/configurations/{param}: when no backing node succeeds, the +// surfaced error must be the highest-severity per-node failure regardless of +// node iteration order (a NOT_FOUND from one node must never mask an +// unavailable/timeout from another). Exercised end-to-end through +// ConfigHandlers::get_configuration against a live GatewayNode with real +// per-node failures: +// - "ghost" node FQN with no ROS node behind it -> SERVICE_UNAVAILABLE +// - unspun node (parameter services exist, never answered) -> TIMEOUT +// - the gateway's own node missing the parameter -> NOT_FOUND +// ============================================================================= + +class ParameterErrorPrecedenceTest : public ::testing::Test { + protected: + static void SetUpTestSuite() { + rclcpp::init(0, nullptr); + } + + static void TearDownTestSuite() { + if (rclcpp::ok()) { + rclcpp::shutdown(); + } + } + + void SetUp() override { + rclcpp::NodeOptions options; + options.parameter_overrides({ + // Fail fast on the ghost node's wait_for_service and keep the test + // runtime bounded. + rclcpp::Parameter("parameter_service_timeout_sec", 1.0), + // Disable the negative cache so every probe classifies fresh + // (TIMEOUT stays TIMEOUT instead of a cached SERVICE_UNAVAILABLE). + rclcpp::Parameter("parameter_service_negative_cache_sec", 0.0), + }); + gateway_node_ = std::make_shared(options); + ASSERT_NE(gateway_node_, nullptr); + // The cache is injected below; a graph-driven refresh would wipe it. + gateway_node_->stop_discovery_refresh_for_testing(); + + // A node whose parameter services exist in the DDS graph but are never + // spun: parameter requests to it time out instead of failing fast. + unspun_node_ = std::make_shared("param_precedence_unspun"); + + executor_ = std::make_unique(); + executor_->add_node(gateway_node_); + spin_thread_ = std::thread([this]() { + executor_->spin(); + }); + + ctx_ = std::make_unique(gateway_node_.get(), cors_, auth_, tls_, nullptr); + handlers_ = std::make_unique(*ctx_); + + wait_for_unspun_node_discovery(); + seed_cache(); + } + + void TearDown() override { + if (executor_) { + executor_->cancel(); + } + if (spin_thread_.joinable()) { + spin_thread_.join(); + } + handlers_.reset(); + ctx_.reset(); + executor_.reset(); + unspun_node_.reset(); + gateway_node_.reset(); + } + + /// Wait until the gateway sees the unspun node's parameter services in the + /// graph, so its probes deterministically reach the TIMEOUT path (service + /// discovered, request never answered) instead of SERVICE_UNAVAILABLE. + void wait_for_unspun_node_discovery() { + const std::string service = unspun_fqn() + "/get_parameters"; + const auto deadline = std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (std::chrono::steady_clock::now() < deadline) { + auto services = gateway_node_->get_service_names_and_types(); + if (services.count(service) > 0) { + return; + } + std::this_thread::sleep_for(std::chrono::milliseconds(50)); + } + FAIL() << "unspun node parameter service not discovered: " << service; + } + + std::string self_fqn() const { + return gateway_node_->get_fully_qualified_name(); + } + + std::string unspun_fqn() const { + return unspun_node_->get_fully_qualified_name(); + } + + static App make_app(const std::string & id, const std::string & component_id, const std::string & fqn) { + App app; + app.id = id; + app.name = id; + app.component_id = component_id; + app.bound_fqn = fqn; + app.is_online = true; + return app; + } + + /// Seed aggregated components whose backing-node order is fixed by app + /// insertion order (get_component_configurations preserves it), covering + /// both iteration orders for every scenario. + void seed_cache() { + const std::string ghost_fqn = "/param_precedence_ghost"; + + std::vector components; + std::vector apps; + + auto add_component = [&](const std::string & id, const std::vector & fqns) { + Component comp; + comp.id = id; + comp.name = id; + components.push_back(comp); + for (size_t i = 0; i < fqns.size(); ++i) { + apps.push_back(make_app(id + "_app" + std::to_string(i), id, fqns[i])); + } + }; + + // Unavailable node first / last, plus a node that lacks the parameter. + add_component("stack_unavail_first", {ghost_fqn, self_fqn()}); + add_component("stack_unavail_last", {self_fqn(), ghost_fqn}); + + // Three distinct verdicts (TIMEOUT, SERVICE_UNAVAILABLE, NOT_FOUND) in + // both orders. + add_component("stack_mixed_timeout_first", {unspun_fqn(), ghost_fqn, self_fqn()}); + add_component("stack_mixed_timeout_last", {self_fqn(), ghost_fqn, unspun_fqn()}); + + // Every node reachable but none has the parameter. + add_component("stack_all_missing", {self_fqn(), self_fqn()}); + + auto & cache = const_cast(gateway_node_->get_thread_safe_cache()); + cache.update_all({}, components, apps, {}); + } + + ErrorInfo get_missing_param_error(const std::string & component_id) { + const std::string path = "/api/v1/components/" + component_id + "/configurations/" + kMissingParam; + auto raw = make_request_with_match(path, kGetConfigPattern); + http::TypedRequest req(raw); + + auto result = handlers_->get_configuration(req); + EXPECT_FALSE(result.has_value()) << "expected an error for " << path; + return result.has_value() ? ErrorInfo{} : result.error(); + } + + CorsConfig cors_{}; + AuthConfig auth_{}; + TlsConfig tls_{}; + std::shared_ptr gateway_node_; + std::shared_ptr unspun_node_; + std::unique_ptr executor_; + std::thread spin_thread_; + std::unique_ptr ctx_; + std::unique_ptr handlers_; +}; + +// Regression order for the pre-fix overwrite bug: the unavailable node is +// probed first, then the reachable node reports NOT_FOUND last. The old code +// classified only the LAST failure, so this surfaced 404 instead of 503. +TEST_F(ParameterErrorPrecedenceTest, UnavailableNodeFirstNotFoundLastReturns503) { + auto err = get_missing_param_error("stack_unavail_first"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); +} + +// Opposite iteration order must surface the same verdict. +TEST_F(ParameterErrorPrecedenceTest, NotFoundFirstUnavailableNodeLastReturns503) { + auto err = get_missing_param_error("stack_unavail_last"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); +} + +// Three distinct per-node verdicts (TIMEOUT, SERVICE_UNAVAILABLE, NOT_FOUND): +// a 503-class failure must win over NOT_FOUND, and within the 503 rank the +// first failure probed is the one surfaced in details. +TEST_F(ParameterErrorPrecedenceTest, MixedThreeNodeFailuresTimeoutFirstReturns503) { + auto err = get_missing_param_error("stack_mixed_timeout_first"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + // First 503-rank failure probed: the unspun node's TIMEOUT. + ASSERT_TRUE(err.params.contains("details")); + EXPECT_NE(err.params["details"].get().find("did not respond"), std::string::npos) << err.params.dump(); +} + +TEST_F(ParameterErrorPrecedenceTest, MixedThreeNodeFailuresTimeoutLastReturns503) { + auto err = get_missing_param_error("stack_mixed_timeout_last"); + + EXPECT_EQ(err.http_status, 503); + EXPECT_EQ(err.code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); + // First 503-rank failure probed: the ghost node's SERVICE_UNAVAILABLE. + ASSERT_TRUE(err.params.contains("details")); + EXPECT_NE(err.params["details"].get().find("not available"), std::string::npos) << err.params.dump(); +} + +// When every backing node is reachable and reports NOT_FOUND, the aggregate +// verdict stays a plain 404. +TEST_F(ParameterErrorPrecedenceTest, AllNodesMissingParameterReturns404) { + auto err = get_missing_param_error("stack_all_missing"); + + EXPECT_EQ(err.http_status, 404); + EXPECT_EQ(err.code, ERR_RESOURCE_NOT_FOUND); +}