From 454c7395a32528695ec5ccafbe68932e5cc8430e Mon Sep 17 00:00:00 2001 From: Michal Faferek Date: Sat, 18 Jul 2026 22:10:47 +0200 Subject: [PATCH 1/3] fix(gateway): consistent parameter-config error surface and drop dead classify fallback List configurations now flags itself partial and names unavailable backing nodes instead of silently returning a shrunken 200, matching the 503 the single-parameter GET returns for the same node outage (#541). The dead string-matching fallback in the parameter-error classifier is removed and extracted into a testable unit; a NONE-coded failure now maps to 500, never a 400 misclassification (#542). Closes #541 Closes #542 --- docs/api/rest.rst | 20 ++++ src/ros2_medkit_gateway/CMakeLists.txt | 5 + .../http/parameter_error_classification.hpp | 38 ++++++ .../ros2_medkit_gateway/dto/config.hpp | 4 +- .../src/http/handlers/config_handlers.cpp | 107 ++++------------- .../http/parameter_error_classification.cpp | 76 ++++++++++++ .../test_parameter_error_classification.cpp | 111 ++++++++++++++++++ .../test/test_parameter_error_precedence.cpp | 75 ++++++++++-- 8 files changed, 341 insertions(+), 95 deletions(-) create mode 100644 src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/parameter_error_classification.hpp create mode 100644 src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp create mode 100644 src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp diff --git a/docs/api/rest.rst b/docs/api/rest.rst index e5097da9d..e62ebdba8 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -2530,6 +2530,26 @@ response), the response includes vendor metadata indicating partial results: When all peers respond successfully, these fields are omitted. See the :doc:`aggregation configuration guide ` for setup details. +``GET /{entity}/configurations`` applies the same honesty to its *local* +backing nodes. When an entity is backed by several ROS 2 nodes and some are +reachable while others are down, the response stays ``200`` but flags itself +``partial`` and names the unreachable nodes in ``unavailable_nodes`` rather than +silently returning a shrunken list: + +.. code-block:: json + + { + "items": [], + "x-medkit": { + "partial": true, + "unavailable_nodes": ["/down_node"] + } + } + +This matches ``GET /{entity}/configurations/{param}``, which returns ``503`` +``x-medkit-ros2-node-unavailable`` for the same backing-node outage. When every +backing node responds, both fields are omitted. + Capability Description (OpenAPI Docs) -------------------------------------- diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index bd1ce6729..491bf77d3 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -175,6 +175,7 @@ add_library(gateway_ros2 STATIC src/fault_manager_paths.cpp src/gateway_node.cpp src/http/fan_out_helpers.cpp + src/http/parameter_error_classification.cpp src/http/handlers/auth_handlers.cpp src/http/handlers/bulkdata_handlers.cpp src/http/handlers/config_handlers.cpp @@ -593,6 +594,10 @@ if(BUILD_TESTING) ament_add_gtest(test_error_info test/test_error_info.cpp) target_link_libraries(test_error_info gateway_ros2) + # Parameter error classification unit tests (pure C++, no rclcpp) + ament_add_gtest(test_parameter_error_classification test/test_parameter_error_classification.cpp) + target_link_libraries(test_parameter_error_classification 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) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/parameter_error_classification.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/parameter_error_classification.hpp new file mode 100644 index 000000000..b1e3aa115 --- /dev/null +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/parameter_error_classification.hpp @@ -0,0 +1,38 @@ +// 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. + +#pragma once + +#include + +#include "ros2_medkit_gateway/core/configuration/parameter_types.hpp" + +namespace ros2_medkit_gateway { +namespace handlers { + +/// HTTP status + SOVD error code for a failed parameter operation. +struct ParameterErrorClassification { + int status_code = 500; + std::string error_code; +}; + +/// Map a failed `ParameterResult` to an HTTP status + SOVD error code from its +/// structured `error_code`. Every failure path on the shipped +/// `Ros2ParameterTransport` sets a structured code, so a failure that still +/// carries `ParameterErrorCode::NONE` is a gateway-side defect and maps to 500 +/// internal-error - it is never guessed from the free-form message text. +ParameterErrorClassification classify_parameter_error(const ParameterResult & result); + +} // namespace handlers +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp index 605e38fee..8d592043a 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp @@ -101,6 +101,7 @@ struct ConfigListXMedkit { std::optional parameters; // free-form: array of raw param JSON std::optional> source_ids; std::optional> queried_nodes; + std::optional> unavailable_nodes; std::optional partial; std::optional> failed_peers; std::optional> peer_dropped_items; @@ -112,7 +113,8 @@ inline constexpr auto dto_fields = std::make_tuple( field("aggregation_level", &ConfigListXMedkit::aggregation_level), field("is_aggregated", &ConfigListXMedkit::is_aggregated), field("parameters", &ConfigListXMedkit::parameters), field("source_ids", &ConfigListXMedkit::source_ids), field("queried_nodes", &ConfigListXMedkit::queried_nodes), - field("partial", &ConfigListXMedkit::partial), field("failed_peers", &ConfigListXMedkit::failed_peers), + field("unavailable_nodes", &ConfigListXMedkit::unavailable_nodes), field("partial", &ConfigListXMedkit::partial), + field("failed_peers", &ConfigListXMedkit::failed_peers), field("peer_dropped_items", &ConfigListXMedkit::peer_dropped_items)); template <> 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 097874400..30be860fd 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -27,6 +27,7 @@ #include "ros2_medkit_gateway/core/http/error_codes.hpp" #include "ros2_medkit_gateway/core/http/fan_out_helpers.hpp" #include "ros2_medkit_gateway/core/http/http_utils.hpp" +#include "ros2_medkit_gateway/core/http/parameter_error_classification.hpp" #include "ros2_medkit_gateway/dto/json_reader.hpp" #include "ros2_medkit_gateway/dto/json_writer.hpp" #include "ros2_medkit_gateway/gateway_node.hpp" @@ -114,90 +115,6 @@ const NodeConfigInfo * find_node_for_app(const std::vector & nod return nullptr; } -/// 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. -ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) { - ParameterErrorClassification result; - - switch (error_code) { - case ParameterErrorCode::NOT_FOUND: - case ParameterErrorCode::NO_DEFAULTS_CACHED: - result.status_code = 404; - result.error_code = ERR_RESOURCE_NOT_FOUND; - break; - case ParameterErrorCode::READ_ONLY: - result.status_code = 403; - result.error_code = ERR_X_MEDKIT_ROS2_PARAMETER_READ_ONLY; - break; - case ParameterErrorCode::SERVICE_UNAVAILABLE: - case ParameterErrorCode::TIMEOUT: - result.status_code = 503; - result.error_code = ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; - break; - case ParameterErrorCode::TYPE_MISMATCH: - case ParameterErrorCode::INVALID_VALUE: - result.status_code = 400; - result.error_code = ERR_INVALID_PARAMETER; - break; - case ParameterErrorCode::NONE: - // NONE means "no error" - caller (classify_parameter_error) pre-filters - // this case, so reaching here indicates a programming error. - result.status_code = 500; - result.error_code = ERR_INTERNAL_ERROR; - break; - case ParameterErrorCode::SHUT_DOWN: - case ParameterErrorCode::INTERNAL_ERROR: - default: - result.status_code = 500; - result.error_code = ERR_INTERNAL_ERROR; - break; - } - - return result; -} - -/// 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. -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 - ParameterErrorClassification classification; - const auto & error_message = result.error_message; - - if (error_message.find("not found") != std::string::npos || - error_message.find("Parameter not found") != std::string::npos) { - classification.status_code = 404; - classification.error_code = ERR_RESOURCE_NOT_FOUND; - } else if (error_message.find("read-only") != std::string::npos || - error_message.find("read only") != std::string::npos || - error_message.find("is read_only") != std::string::npos) { - classification.status_code = 403; - classification.error_code = ERR_X_MEDKIT_ROS2_PARAMETER_READ_ONLY; - } else if (error_message.find("not available") != std::string::npos || - error_message.find("service not available") != std::string::npos || - error_message.find("timed out") != std::string::npos || - error_message.find("timeout") != std::string::npos) { - classification.status_code = 503; - classification.error_code = ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; - } else { - classification.status_code = 400; - classification.error_code = ERR_INVALID_REQUEST; - } - - return classification; -} - /// Build a typed `ErrorInfo` for a failed parameter operation. Mirrors the /// legacy `send_parameter_error` helper's wire shape (params: details + /// entity_id + id, message: "Failed to parameter"). @@ -411,6 +328,7 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { bool any_success = false; std::string first_error; std::string first_error_node; // Track which node failed for better diagnostics + std::vector unavailable_nodes; for (const auto & qr : node_results) { const auto & node_info = qr.node_info; @@ -435,9 +353,14 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { all_parameters.push_back(std::move(param_with_source)); } } - } else if (first_error.empty()) { - first_error = result.error_message; - first_error_node = node_info.node_fqn; + } else { + // Record every failed backing node so a partial list can name what is + // missing, and keep the first failure for the all-nodes-down 503. + unavailable_nodes.push_back(node_info.node_fqn); + if (first_error.empty()) { + first_error = result.error_message; + first_error_node = node_info.node_fqn; + } } } @@ -454,6 +377,16 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { if (!queried_nodes.empty()) { xm.queried_nodes = queried_nodes; } + // Some (but not all) backing nodes were unreachable. Mirror the peer + // fan-out honesty convention (partial + failed_peers) for local nodes: + // flag the response partial and name the unavailable nodes instead of + // silently returning a shrunken 200. This keeps the list surface + // consistent with GET /{entity}/configurations/{param}, which returns 503 + // ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE for the same backing-node outage. + if (!unavailable_nodes.empty()) { + xm.partial = true; + xm.unavailable_nodes = std::move(unavailable_nodes); + } } // Typed fan-out for collection endpoints. Replaces the legacy raw-JSON diff --git a/src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp b/src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp new file mode 100644 index 000000000..5d4d461fd --- /dev/null +++ b/src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp @@ -0,0 +1,76 @@ +// 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 "ros2_medkit_gateway/core/http/parameter_error_classification.hpp" + +#include "ros2_medkit_gateway/core/http/error_codes.hpp" + +namespace ros2_medkit_gateway { +namespace handlers { + +namespace { + +/// Map a structured `ParameterErrorCode` to an HTTP status + SOVD error code. +ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) { + ParameterErrorClassification result; + + switch (error_code) { + case ParameterErrorCode::NOT_FOUND: + case ParameterErrorCode::NO_DEFAULTS_CACHED: + result.status_code = 404; + result.error_code = ERR_RESOURCE_NOT_FOUND; + break; + case ParameterErrorCode::READ_ONLY: + result.status_code = 403; + result.error_code = ERR_X_MEDKIT_ROS2_PARAMETER_READ_ONLY; + break; + case ParameterErrorCode::SERVICE_UNAVAILABLE: + case ParameterErrorCode::TIMEOUT: + result.status_code = 503; + result.error_code = ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; + break; + case ParameterErrorCode::TYPE_MISMATCH: + case ParameterErrorCode::INVALID_VALUE: + result.status_code = 400; + result.error_code = ERR_INVALID_PARAMETER; + break; + case ParameterErrorCode::NONE: + // A failed ParameterResult that still carries NONE cannot occur on the + // shipped transport (every failure path sets a structured code), so a + // NONE failure is a gateway-side defect: surface it as 500 internal-error. + // The removed legacy string-matching fallback misrouted these to 400 + // invalid-request because its substrings never matched the real transport + // messages ("did not respond", "not currently set"). + result.status_code = 500; + result.error_code = ERR_INTERNAL_ERROR; + break; + case ParameterErrorCode::SHUT_DOWN: + case ParameterErrorCode::INTERNAL_ERROR: + default: + result.status_code = 500; + result.error_code = ERR_INTERNAL_ERROR; + break; + } + + return result; +} + +} // namespace + +ParameterErrorClassification classify_parameter_error(const ParameterResult & result) { + return classify_error_code(result.error_code); +} + +} // namespace handlers +} // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp new file mode 100644 index 000000000..38a010565 --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp @@ -0,0 +1,111 @@ +// 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 "ros2_medkit_gateway/core/configuration/parameter_types.hpp" +#include "ros2_medkit_gateway/core/http/error_codes.hpp" +#include "ros2_medkit_gateway/core/http/parameter_error_classification.hpp" + +using ros2_medkit_gateway::ERR_INTERNAL_ERROR; +using ros2_medkit_gateway::ERR_INVALID_PARAMETER; +using ros2_medkit_gateway::ERR_INVALID_REQUEST; +using ros2_medkit_gateway::ERR_RESOURCE_NOT_FOUND; +using ros2_medkit_gateway::ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; +using ros2_medkit_gateway::ERR_X_MEDKIT_ROS2_PARAMETER_READ_ONLY; +using ros2_medkit_gateway::ParameterErrorCode; +using ros2_medkit_gateway::ParameterResult; +using ros2_medkit_gateway::handlers::classify_parameter_error; + +namespace { + +ParameterResult failure(ParameterErrorCode code, const std::string & message) { + ParameterResult result; + result.success = false; + result.error_message = message; + result.error_code = code; + return result; +} + +} // namespace + +// ============================================================================= +// classify_parameter_error maps the structured ParameterErrorCode set by the +// transport to an HTTP status + SOVD error code. These pin the contract so the +// mapping cannot silently drift. +// ============================================================================= + +TEST(ParameterErrorClassification, TimeoutIs503) { + auto c = + classify_parameter_error(failure(ParameterErrorCode::TIMEOUT, "Parameter service did not respond for node: /x")); + EXPECT_EQ(c.status_code, 503); + EXPECT_EQ(c.error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); +} + +TEST(ParameterErrorClassification, ServiceUnavailableIs503) { + auto c = classify_parameter_error( + failure(ParameterErrorCode::SERVICE_UNAVAILABLE, "Parameter service not available for node: /x")); + EXPECT_EQ(c.status_code, 503); + EXPECT_EQ(c.error_code, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE); +} + +TEST(ParameterErrorClassification, NotFoundIs404) { + auto c = classify_parameter_error(failure(ParameterErrorCode::NOT_FOUND, "Parameter not currently set: foo")); + EXPECT_EQ(c.status_code, 404); + EXPECT_EQ(c.error_code, ERR_RESOURCE_NOT_FOUND); +} + +TEST(ParameterErrorClassification, ReadOnlyIs403) { + auto c = classify_parameter_error(failure(ParameterErrorCode::READ_ONLY, "read-only")); + EXPECT_EQ(c.status_code, 403); + EXPECT_EQ(c.error_code, ERR_X_MEDKIT_ROS2_PARAMETER_READ_ONLY); +} + +TEST(ParameterErrorClassification, InvalidValueIs400) { + auto c = classify_parameter_error(failure(ParameterErrorCode::INVALID_VALUE, "bad value")); + EXPECT_EQ(c.status_code, 400); + EXPECT_EQ(c.error_code, ERR_INVALID_PARAMETER); +} + +TEST(ParameterErrorClassification, InternalErrorIs500) { + auto c = classify_parameter_error(failure(ParameterErrorCode::INTERNAL_ERROR, "boom")); + EXPECT_EQ(c.status_code, 500); + EXPECT_EQ(c.error_code, ERR_INTERNAL_ERROR); +} + +// Regression pin for issue #542: the removed legacy string-matching fallback +// routed a failure carrying ParameterErrorCode::NONE to 400 ERR_INVALID_REQUEST +// whenever its substrings failed to match the real transport messages (which is +// always - the shipped transport uses "did not respond" / "not currently set", +// none of which the fallback recognised). Such a NONE failure cannot occur on +// the shipped transport, so if one ever does it is a gateway-side defect and +// must surface as 500 internal-error, never as a 400 that blames the client. +TEST(ParameterErrorClassification, NoneFailureIsInternalErrorNot400) { + // A message the old fallback would have misrouted to 503, still NONE-coded. + auto did_not_respond = + classify_parameter_error(failure(ParameterErrorCode::NONE, "Parameter service did not respond for node: /x")); + EXPECT_EQ(did_not_respond.status_code, 500); + EXPECT_EQ(did_not_respond.error_code, ERR_INTERNAL_ERROR); + EXPECT_NE(did_not_respond.status_code, 400); + EXPECT_NE(did_not_respond.error_code, ERR_INVALID_REQUEST); + + // A message the old fallback would have misrouted to 400 invalid-request. + auto not_set = classify_parameter_error(failure(ParameterErrorCode::NONE, "Parameter not currently set: foo")); + EXPECT_EQ(not_set.status_code, 500); + EXPECT_EQ(not_set.error_code, ERR_INTERNAL_ERROR); + EXPECT_NE(not_set.status_code, 400); + EXPECT_NE(not_set.error_code, ERR_INVALID_REQUEST); +} diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp index ddb164984..40aebc005 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -17,6 +17,7 @@ #include #include +#include #include #include #include @@ -45,11 +46,14 @@ using ros2_medkit_gateway::TlsConfig; using ros2_medkit_gateway::handlers::ConfigHandlers; using ros2_medkit_gateway::handlers::HandlerContext; namespace http = ros2_medkit_gateway::http; +namespace dto = ros2_medkit_gateway::dto; namespace { constexpr const char * kMissingParam = "definitely_missing_param"; constexpr const char * kGetConfigPattern = R"(/api/v1/components/([^/]+)/configurations/([^/]+))"; +constexpr const char * kListConfigPattern = R"(/api/v1/components/([^/]+)/configurations)"; +constexpr const char * kGhostFqn = "/param_precedence_ghost"; httplib::Request make_request_with_match(const std::string & path, const std::string & pattern) { httplib::Request req; @@ -62,16 +66,22 @@ httplib::Request make_request_with_match(const std::string & path, const std::st } // 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 +// Handler-level regression tests for the multi-node config endpoints, exercised +// end-to-end through ConfigHandlers 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 +// +// 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). +// +// GET /{entity}/configurations (list, issue #541): when some backing nodes are +// up and others are down, the 200 response must flag itself partial and name +// the unavailable nodes rather than silently dropping them - matching the +// single-parameter GET's 503 honesty for the same outage. // ============================================================================= class ParameterErrorPrecedenceTest : public ::testing::Test { @@ -170,7 +180,7 @@ class ParameterErrorPrecedenceTest : public ::testing::Test { /// 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"; + const std::string ghost_fqn = kGhostFqn; std::vector components; std::vector apps; @@ -211,6 +221,21 @@ class ParameterErrorPrecedenceTest : public ::testing::Test { return result.has_value() ? ErrorInfo{} : result.error(); } + /// Run GET /{entity}/configurations and return the parsed x-medkit block. + /// The call must succeed (list is a 200 even when a backing node is down). + dto::ConfigListXMedkit list_xmedkit(const std::string & component_id) { + const std::string path = "/api/v1/components/" + component_id + "/configurations"; + auto raw = make_request_with_match(path, kListConfigPattern); + http::TypedRequest req(raw); + + auto result = handlers_->list_configurations(req); + EXPECT_TRUE(result.has_value()) << "list_configurations should return 200 for " << path; + if (!result.has_value() || !result.value().x_medkit.has_value()) { + return dto::ConfigListXMedkit{}; + } + return *result.value().x_medkit; + } + CorsConfig cors_{}; AuthConfig auth_{}; TlsConfig tls_{}; @@ -271,3 +296,39 @@ TEST_F(ParameterErrorPrecedenceTest, AllNodesMissingParameterReturns404) { EXPECT_EQ(err.http_status, 404); EXPECT_EQ(err.code, ERR_RESOURCE_NOT_FOUND); } + +// Issue #541: one backing node down and one up. The list endpoint must stay a +// 200 but flag itself partial and name the unavailable node, instead of +// silently returning a shrunken list. Both node orders are covered. +TEST_F(ParameterErrorPrecedenceTest, ListWithUnavailableNodeFirstIsPartial) { + auto xm = list_xmedkit("stack_unavail_first"); + + ASSERT_TRUE(xm.partial.has_value()); + EXPECT_TRUE(*xm.partial); + ASSERT_TRUE(xm.unavailable_nodes.has_value()); + EXPECT_NE(std::find(xm.unavailable_nodes->begin(), xm.unavailable_nodes->end(), kGhostFqn), + xm.unavailable_nodes->end()) + << "unavailable node list must name the down node"; + // The reachable node's parameters are still listed (queried_nodes non-empty). + ASSERT_TRUE(xm.queried_nodes.has_value()); + EXPECT_FALSE(xm.queried_nodes->empty()); +} + +TEST_F(ParameterErrorPrecedenceTest, ListWithUnavailableNodeLastIsPartial) { + auto xm = list_xmedkit("stack_unavail_last"); + + ASSERT_TRUE(xm.partial.has_value()); + EXPECT_TRUE(*xm.partial); + ASSERT_TRUE(xm.unavailable_nodes.has_value()); + EXPECT_NE(std::find(xm.unavailable_nodes->begin(), xm.unavailable_nodes->end(), kGhostFqn), + xm.unavailable_nodes->end()); +} + +// Control: when every backing node is reachable, the list is not flagged +// partial and no unavailable nodes are reported. +TEST_F(ParameterErrorPrecedenceTest, ListWithAllNodesReachableIsNotPartial) { + auto xm = list_xmedkit("stack_all_missing"); + + EXPECT_FALSE(xm.partial.has_value() && *xm.partial); + EXPECT_FALSE(xm.unavailable_nodes.has_value()); +} From c782a5bbfcf0170f41dabf28e5229c2a22e8613d Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 19 Jul 2026 07:51:18 +0200 Subject: [PATCH 2/3] fix(gateway): scope list unavailable_nodes to 503, home classifier in core Only a genuine 503 availability failure names a backing node "unavailable" and keeps the list partial; a hard shutdown/internal failure now fails the request instead of mislabeling the node behind a partial 200 (matches GET). Move parameter_error_classification.cpp into gateway_core so its neutral core header's symbol comes from the core lib, not the ROS adapter layer. --- src/ros2_medkit_gateway/CMakeLists.txt | 1 - .../http/parameter_error_classification.cpp | 0 .../src/http/handlers/config_handlers.cpp | 28 +++++++++++++++---- .../test/test_parameter_error_precedence.cpp | 18 ++++++++++++ 4 files changed, 40 insertions(+), 7 deletions(-) rename src/ros2_medkit_gateway/src/{ => core}/http/parameter_error_classification.cpp (100%) diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index 491bf77d3..1ca4ac674 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -175,7 +175,6 @@ add_library(gateway_ros2 STATIC src/fault_manager_paths.cpp src/gateway_node.cpp src/http/fan_out_helpers.cpp - src/http/parameter_error_classification.cpp src/http/handlers/auth_handlers.cpp src/http/handlers/bulkdata_handlers.cpp src/http/handlers/config_handlers.cpp diff --git a/src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp b/src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp similarity index 100% rename from src/ros2_medkit_gateway/src/http/parameter_error_classification.cpp rename to src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp 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 30be860fd..3a24683f0 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -329,6 +329,7 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { std::string first_error; std::string first_error_node; // Track which node failed for better diagnostics std::vector unavailable_nodes; + std::optional hard_error; // non-availability failure -> fail the whole list for (const auto & qr : node_results) { const auto & node_info = qr.node_info; @@ -354,16 +355,31 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { } } } else { - // Record every failed backing node so a partial list can name what is - // missing, and keep the first failure for the all-nodes-down 503. - unavailable_nodes.push_back(node_info.node_fqn); - if (first_error.empty()) { - first_error = result.error_message; - first_error_node = node_info.node_fqn; + // Only a genuine availability failure (503: node down/unresponsive) + // makes the node "unavailable" and the list merely partial, mirroring + // the 503 the single-parameter GET returns for the same outage. A + // non-503 failure (shutdown/internal) is a hard error, not an + // availability gap: surface it rather than mislabel the node + // unavailable and mask it behind a partial 200. + auto cls = classify_parameter_error(result); + if (cls.status_code == 503) { + unavailable_nodes.push_back(node_info.node_fqn); + if (first_error.empty()) { + first_error = result.error_message; + first_error_node = node_info.node_fqn; + } + } else if (!hard_error) { + hard_error = make_error( + cls.status_code, cls.error_code, "Failed to list parameters", + json{{"details", result.error_message}, {"entity_id", entity_id}, {"failed_node", node_info.node_fqn}}); } } } + if (hard_error) { + return tl::unexpected(std::move(*hard_error)); + } + if (!any_success) { return tl::unexpected( make_error(503, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE, "Failed to list parameters from any node", diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp index 40aebc005..b96984276 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -37,6 +37,7 @@ 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_INTERNAL_ERROR; using ros2_medkit_gateway::ERR_RESOURCE_NOT_FOUND; using ros2_medkit_gateway::ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE; using ros2_medkit_gateway::ErrorInfo; @@ -332,3 +333,20 @@ TEST_F(ParameterErrorPrecedenceTest, ListWithAllNodesReachableIsNotPartial) { EXPECT_FALSE(xm.partial.has_value() && *xm.partial); EXPECT_FALSE(xm.unavailable_nodes.has_value()); } + +// A non-availability hard failure (manager shut down -> SHUT_DOWN on every +// node) must fail the list with a 500 internal-error, not mislabel the nodes +// "unavailable" behind a partial 200. Guards the list-vs-GET consistency: +// GET surfaces 500 for the same code, so the list must not swallow it. +TEST_F(ParameterErrorPrecedenceTest, ListWithHardErrorFailsRequestNotPartial) { + gateway_node_->get_configuration_manager()->shutdown(); + + const std::string path = "/api/v1/components/stack_all_missing/configurations"; + auto raw = make_request_with_match(path, kListConfigPattern); + http::TypedRequest req(raw); + + auto result = handlers_->list_configurations(req); + ASSERT_FALSE(result.has_value()) << "hard per-node error must fail the list, not return a partial 200"; + EXPECT_EQ(result.error().http_status, 500); + EXPECT_EQ(result.error().code, ERR_INTERNAL_ERROR); +} From a00898efb3a951cd48b3a25890563864f5cfd47e Mon Sep 17 00:00:00 2001 From: mfaferek93 Date: Sun, 19 Jul 2026 10:47:48 +0200 Subject: [PATCH 3/3] fix(gateway): keep answering nodes on partial config list, rank all-down failure A non-503 backing-node failure no longer fails the whole list and discards the nodes that answered; the list stays 200 partial, keeps their parameters, and names the failed node (503-class in unavailable_nodes, others in failed_nodes). Only when no node answers does the request fail, folding through the severity accumulator so a mixed down+internal outage returns 503 to match GET. Docs, DTO comment and the classification/precedence test comments corrected accordingly. --- docs/api/rest.rst | 30 ++++++-- .../ros2_medkit_gateway/dto/config.hpp | 14 ++-- .../http/parameter_error_classification.cpp | 6 +- .../src/http/handlers/config_handlers.cpp | 70 ++++++++++--------- .../test_parameter_error_classification.cpp | 32 +++++---- .../test/test_parameter_error_precedence.cpp | 48 +++++++++---- 6 files changed, 123 insertions(+), 77 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index e62ebdba8..185533bfa 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -2531,15 +2531,21 @@ When all peers respond successfully, these fields are omitted. See the :doc:`aggregation configuration guide ` for setup details. ``GET /{entity}/configurations`` applies the same honesty to its *local* -backing nodes. When an entity is backed by several ROS 2 nodes and some are -reachable while others are down, the response stays ``200`` but flags itself -``partial`` and names the unreachable nodes in ``unavailable_nodes`` rather than -silently returning a shrunken list: +backing nodes. This only arises for entities backed by more than one ROS 2 +node; a single-node entity has no partial state, since its one node either +answers or the whole request fails. When several backing nodes contribute and +some answer while others do not, the response stays ``200`` but flags itself +``partial`` and names the failed nodes: nodes that were down or unresponsive +(``503``-class) in ``unavailable_nodes``, any other per-node failure in +``failed_nodes``. The parameters of the nodes that *did* answer are still +returned - a partial list never discards them: .. code-block:: json { - "items": [], + "items": [ + {"id": "use_sim_time", "name": "use_sim_time", "type": "parameter"} + ], "x-medkit": { "partial": true, "unavailable_nodes": ["/down_node"] @@ -2547,8 +2553,18 @@ silently returning a shrunken list: } This matches ``GET /{entity}/configurations/{param}``, which returns ``503`` -``x-medkit-ros2-node-unavailable`` for the same backing-node outage. When every -backing node responds, both fields are omitted. +``x-medkit-ros2-node-unavailable`` for the same backing-node outage. + +When every backing node responds, ``unavailable_nodes`` and ``failed_nodes`` are +omitted. Note that ``partial`` may still appear on its own from the peer +fan-out described above, without any local unavailable node. + +When *no* backing node answers, the route fails rather than returning an empty +partial list: it surfaces the highest-severity per-node failure, so a +node-unavailability outage returns ``503`` while a purely internal failure - +for example every backing node reporting shutdown - returns ``500``. This route +could not return ``500`` before; clients that previously saw ``503`` for a +manager shutdown now see ``500``. Capability Description (OpenAPI Docs) -------------------------------------- diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp index 8d592043a..97196555d 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/config.hpp @@ -87,8 +87,11 @@ inline constexpr std::string_view dto_name = "Configurati // parameters - full parameter details including value/type/read_only // (free-form: raw ConfigurationManager output + x-medkit) // source_ids - namespace/FQN strings used for node lookup -// queried_nodes - list of node FQNs successfully queried -// partial - true when a fan-out peer request failed +// queried_nodes - list of node FQNs successfully queried +// unavailable_nodes - backing node FQNs that were down/unresponsive (503-class) +// failed_nodes - backing node FQNs that failed for a non-availability reason +// partial - true when a fan-out peer request failed OR a local backing +// node did not answer (see unavailable_nodes/failed_nodes) // failed_peers - list of peer addresses that returned errors // peer_dropped_items - per-peer items dropped due to malformed JSON // (observability for invisible drift) @@ -102,6 +105,7 @@ struct ConfigListXMedkit { std::optional> source_ids; std::optional> queried_nodes; std::optional> unavailable_nodes; + std::optional> failed_nodes; std::optional partial; std::optional> failed_peers; std::optional> peer_dropped_items; @@ -113,7 +117,8 @@ inline constexpr auto dto_fields = std::make_tuple( field("aggregation_level", &ConfigListXMedkit::aggregation_level), field("is_aggregated", &ConfigListXMedkit::is_aggregated), field("parameters", &ConfigListXMedkit::parameters), field("source_ids", &ConfigListXMedkit::source_ids), field("queried_nodes", &ConfigListXMedkit::queried_nodes), - field("unavailable_nodes", &ConfigListXMedkit::unavailable_nodes), field("partial", &ConfigListXMedkit::partial), + field("unavailable_nodes", &ConfigListXMedkit::unavailable_nodes), + field("failed_nodes", &ConfigListXMedkit::failed_nodes), field("partial", &ConfigListXMedkit::partial), field("failed_peers", &ConfigListXMedkit::failed_peers), field("peer_dropped_items", &ConfigListXMedkit::peer_dropped_items)); @@ -250,7 +255,8 @@ inline constexpr std::string_view dto_name = "Co // Same wire shape as the legacy `Collection` named // `ConfigurationList`, but the `x-medkit` payload carries the rich // `ConfigListXMedkit` fields (entity_id, source, parameters, aggregation_level, -// is_aggregated, source_ids, queried_nodes, partial, failed_peers, +// is_aggregated, source_ids, queried_nodes, unavailable_nodes, failed_nodes, +// partial, failed_peers, // peer_dropped_items) instead of the generic `XMedkitCollection`. The schema // name is kept identical to the legacy `ConfigurationList` $ref so existing // OpenAPI clients are not affected; the difference is purely server-side diff --git a/src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp b/src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp index 5d4d461fd..f6e25af0c 100644 --- a/src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp +++ b/src/ros2_medkit_gateway/src/core/http/parameter_error_classification.cpp @@ -49,9 +49,9 @@ ParameterErrorClassification classify_error_code(ParameterErrorCode error_code) // A failed ParameterResult that still carries NONE cannot occur on the // shipped transport (every failure path sets a structured code), so a // NONE failure is a gateway-side defect: surface it as 500 internal-error. - // The removed legacy string-matching fallback misrouted these to 400 - // invalid-request because its substrings never matched the real transport - // messages ("did not respond", "not currently set"). + // The removed legacy string-matching fallback was unreachable, and would + // have misrouted messages it did not recognise (e.g. "did not respond", + // "not currently set") to 400 invalid-request had it ever run. result.status_code = 500; result.error_code = ERR_INTERNAL_ERROR; break; 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 3a24683f0..c90467ce6 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp @@ -326,10 +326,9 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { const auto node_results = query_nodes_in_parallel(config_mgr, agg_configs); bool any_success = false; - std::string first_error; - std::string first_error_node; // Track which node failed for better diagnostics - std::vector unavailable_nodes; - std::optional hard_error; // non-availability failure -> fail the whole list + ParameterErrorAccumulator errors; // worst failure to surface if no node answers + std::vector unavailable_nodes; // 503-class: node down/unresponsive + std::vector failed_nodes; // other per-node failures for (const auto & qr : node_results) { const auto & node_info = qr.node_info; @@ -355,35 +354,34 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { } } } else { - // Only a genuine availability failure (503: node down/unresponsive) - // makes the node "unavailable" and the list merely partial, mirroring - // the 503 the single-parameter GET returns for the same outage. A - // non-503 failure (shutdown/internal) is a hard error, not an - // availability gap: surface it rather than mislabel the node - // unavailable and mask it behind a partial 200. - auto cls = classify_parameter_error(result); - if (cls.status_code == 503) { + errors.add(result); + // A per-node failure never discards the nodes that answered. A 503-class + // failure (node down/unresponsive) is an availability gap named in + // `unavailable_nodes`; any other failure is named in `failed_nodes`. + // Both keep the answering nodes' parameters and flag the list partial. + if (classify_parameter_error(result).status_code == 503) { unavailable_nodes.push_back(node_info.node_fqn); - if (first_error.empty()) { - first_error = result.error_message; - first_error_node = node_info.node_fqn; - } - } else if (!hard_error) { - hard_error = make_error( - cls.status_code, cls.error_code, "Failed to list parameters", - json{{"details", result.error_message}, {"entity_id", entity_id}, {"failed_node", node_info.node_fqn}}); + } else { + failed_nodes.push_back(node_info.node_fqn); } } } - if (hard_error) { - return tl::unexpected(std::move(*hard_error)); - } - if (!any_success) { + // No backing node answered. Surface the worst failure order-independently + // (`severity_rank`: 503 outranks other 5xx), so a mixed down + internal + // outage returns 503 to match GET /{entity}/configurations/{param}; name + // every failed node so the outage is diagnosable. + const auto & cls = errors.classification(); + json details{{"details", errors.worst().error_message}, {"entity_id", entity_id}}; + if (!unavailable_nodes.empty()) { + details["unavailable_nodes"] = unavailable_nodes; + } + if (!failed_nodes.empty()) { + details["failed_nodes"] = failed_nodes; + } return tl::unexpected( - make_error(503, ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE, "Failed to list parameters from any node", - json{{"details", first_error}, {"entity_id", entity_id}, {"failed_node", first_error_node}})); + make_error(cls.status_code, cls.error_code, "Failed to list parameters from any node", std::move(details))); } xm.parameters = all_parameters; @@ -393,16 +391,22 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { if (!queried_nodes.empty()) { xm.queried_nodes = queried_nodes; } - // Some (but not all) backing nodes were unreachable. Mirror the peer - // fan-out honesty convention (partial + failed_peers) for local nodes: - // flag the response partial and name the unavailable nodes instead of - // silently returning a shrunken 200. This keeps the list surface - // consistent with GET /{entity}/configurations/{param}, which returns 503 - // ERR_X_MEDKIT_ROS2_NODE_UNAVAILABLE for the same backing-node outage. - if (!unavailable_nodes.empty()) { + // Some (but not all) backing nodes failed. Mirror the peer fan-out honesty + // convention (partial + failed_peers) for local nodes: keep the answering + // nodes' parameters, flag the response partial, and name what failed - + // 503-class nodes in `unavailable_nodes`, other failures in `failed_nodes` - + // instead of silently returning a shrunken 200. Keeps the list surface + // consistent with GET /{entity}/configurations/{param}, which returns the + // same 503 for a backing-node outage. + if (!unavailable_nodes.empty() || !failed_nodes.empty()) { xm.partial = true; + } + if (!unavailable_nodes.empty()) { xm.unavailable_nodes = std::move(unavailable_nodes); } + if (!failed_nodes.empty()) { + xm.failed_nodes = std::move(failed_nodes); + } } // Typed fan-out for collection endpoints. Replaces the legacy raw-JSON diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp index 38a010565..9a2e3bbec 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp @@ -87,22 +87,24 @@ TEST(ParameterErrorClassification, InternalErrorIs500) { } // Regression pin for issue #542: the removed legacy string-matching fallback -// routed a failure carrying ParameterErrorCode::NONE to 400 ERR_INVALID_REQUEST -// whenever its substrings failed to match the real transport messages (which is -// always - the shipped transport uses "did not respond" / "not currently set", -// none of which the fallback recognised). Such a NONE failure cannot occur on -// the shipped transport, so if one ever does it is a gateway-side defect and -// must surface as 500 internal-error, never as a 400 that blames the client. +// only ran on a failure carrying ParameterErrorCode::NONE, which the shipped +// transport never emits, so it was dead code. Had it run it would have mapped +// by message substrings - agreeing with the structured mapping for some (e.g. +// "not available" -> 503) and misrouting the rest to 400 ERR_INVALID_REQUEST. +// A NONE failure is a gateway-side defect, so it must surface as 500 +// internal-error regardless of message, never as a 400 that blames the client. TEST(ParameterErrorClassification, NoneFailureIsInternalErrorNot400) { - // A message the old fallback would have misrouted to 503, still NONE-coded. - auto did_not_respond = - classify_parameter_error(failure(ParameterErrorCode::NONE, "Parameter service did not respond for node: /x")); - EXPECT_EQ(did_not_respond.status_code, 500); - EXPECT_EQ(did_not_respond.error_code, ERR_INTERNAL_ERROR); - EXPECT_NE(did_not_respond.status_code, 400); - EXPECT_NE(did_not_respond.error_code, ERR_INVALID_REQUEST); - - // A message the old fallback would have misrouted to 400 invalid-request. + // A message the old fallback matched and would have misrouted to 503 (its + // "not available" substring), still NONE-coded. + auto not_available = + classify_parameter_error(failure(ParameterErrorCode::NONE, "Parameter service not available for node: /x")); + EXPECT_EQ(not_available.status_code, 500); + EXPECT_EQ(not_available.error_code, ERR_INTERNAL_ERROR); + EXPECT_NE(not_available.status_code, 400); + EXPECT_NE(not_available.error_code, ERR_INVALID_REQUEST); + + // A message the old fallback did not match, so it fell through to 400 + // invalid-request. auto not_set = classify_parameter_error(failure(ParameterErrorCode::NONE, "Parameter not currently set: foo")); EXPECT_EQ(not_set.status_code, 500); EXPECT_EQ(not_set.error_code, ERR_INTERNAL_ERROR); diff --git a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp index b96984276..bb6b0ef02 100644 --- a/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp +++ b/src/ros2_medkit_gateway/test/test_parameter_error_precedence.cpp @@ -222,19 +222,24 @@ class ParameterErrorPrecedenceTest : public ::testing::Test { return result.has_value() ? ErrorInfo{} : result.error(); } - /// Run GET /{entity}/configurations and return the parsed x-medkit block. + using ConfigListCollection = dto::Collection; + + /// Run GET /{entity}/configurations and return the full typed collection. /// The call must succeed (list is a 200 even when a backing node is down). - dto::ConfigListXMedkit list_xmedkit(const std::string & component_id) { + ConfigListCollection list_collection(const std::string & component_id) { const std::string path = "/api/v1/components/" + component_id + "/configurations"; auto raw = make_request_with_match(path, kListConfigPattern); http::TypedRequest req(raw); auto result = handlers_->list_configurations(req); EXPECT_TRUE(result.has_value()) << "list_configurations should return 200 for " << path; - if (!result.has_value() || !result.value().x_medkit.has_value()) { - return dto::ConfigListXMedkit{}; - } - return *result.value().x_medkit; + return result.has_value() ? result.value() : ConfigListCollection{}; + } + + /// Convenience: just the parsed x-medkit block of the list response. + dto::ConfigListXMedkit list_xmedkit(const std::string & component_id) { + auto collection = list_collection(component_id); + return collection.x_medkit.has_value() ? *collection.x_medkit : dto::ConfigListXMedkit{}; } CorsConfig cors_{}; @@ -302,7 +307,8 @@ TEST_F(ParameterErrorPrecedenceTest, AllNodesMissingParameterReturns404) { // 200 but flag itself partial and name the unavailable node, instead of // silently returning a shrunken list. Both node orders are covered. TEST_F(ParameterErrorPrecedenceTest, ListWithUnavailableNodeFirstIsPartial) { - auto xm = list_xmedkit("stack_unavail_first"); + auto collection = list_collection("stack_unavail_first"); + const auto & xm = collection.x_medkit.value(); ASSERT_TRUE(xm.partial.has_value()); EXPECT_TRUE(*xm.partial); @@ -310,19 +316,23 @@ TEST_F(ParameterErrorPrecedenceTest, ListWithUnavailableNodeFirstIsPartial) { EXPECT_NE(std::find(xm.unavailable_nodes->begin(), xm.unavailable_nodes->end(), kGhostFqn), xm.unavailable_nodes->end()) << "unavailable node list must name the down node"; - // The reachable node's parameters are still listed (queried_nodes non-empty). - ASSERT_TRUE(xm.queried_nodes.has_value()); - EXPECT_FALSE(xm.queried_nodes->empty()); + // The reachable node's parameters are still merged into the returned list - + // a partial result must not throw away the nodes that answered. + EXPECT_FALSE(collection.items.empty()) << "answering node's parameters must survive the partial"; } TEST_F(ParameterErrorPrecedenceTest, ListWithUnavailableNodeLastIsPartial) { - auto xm = list_xmedkit("stack_unavail_last"); + auto collection = list_collection("stack_unavail_last"); + const auto & xm = collection.x_medkit.value(); ASSERT_TRUE(xm.partial.has_value()); EXPECT_TRUE(*xm.partial); ASSERT_TRUE(xm.unavailable_nodes.has_value()); EXPECT_NE(std::find(xm.unavailable_nodes->begin(), xm.unavailable_nodes->end(), kGhostFqn), xm.unavailable_nodes->end()); + // Reachable node comes first in this order, so it is the one that stresses + // accumulation: assert its parameters survive here too. + EXPECT_FALSE(collection.items.empty()) << "answering node's parameters must survive the partial"; } // Control: when every backing node is reachable, the list is not flagged @@ -334,10 +344,18 @@ TEST_F(ParameterErrorPrecedenceTest, ListWithAllNodesReachableIsNotPartial) { EXPECT_FALSE(xm.unavailable_nodes.has_value()); } -// A non-availability hard failure (manager shut down -> SHUT_DOWN on every -// node) must fail the list with a 500 internal-error, not mislabel the nodes -// "unavailable" behind a partial 200. Guards the list-vs-GET consistency: -// GET surfaces 500 for the same code, so the list must not swallow it. +// When NO backing node answers, the list fails with the worst per-node +// verdict instead of returning an empty partial 200. Here the manager is shut +// down so every node returns SHUT_DOWN and the surfaced status is 500 +// internal-error, mirroring GET, rather than mislabelling the nodes +// "unavailable" behind a partial 200. +// +// The complementary case - one node failing while another answers - now keeps +// the answering node's parameters and stays a 200 partial (asserted by the +// ListWith*NodeIsPartial cases above via collection.items), so the old +// 200->500 loud-drop no longer exists. That mixed case cannot be pinned more +// tightly here because the live-node harness has no per-node non-503 fault +// hook (shutdown is manager-wide; the ghost/unspun nodes only yield 503). TEST_F(ParameterErrorPrecedenceTest, ListWithHardErrorFailsRequestNotPartial) { gateway_node_->get_configuration_manager()->shutdown();