diff --git a/docs/api/rest.rst b/docs/api/rest.rst index e5097da9d..185533bfa 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -2530,6 +2530,42 @@ 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. 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": [ + {"id": "use_sim_time", "name": "use_sim_time", "type": "parameter"} + ], + "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, ``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/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index bd1ce6729..1ca4ac674 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -593,6 +593,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..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) @@ -101,6 +104,8 @@ 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> failed_nodes; std::optional partial; std::optional> failed_peers; std::optional> peer_dropped_items; @@ -112,7 +117,9 @@ 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("failed_nodes", &ConfigListXMedkit::failed_nodes), field("partial", &ConfigListXMedkit::partial), + field("failed_peers", &ConfigListXMedkit::failed_peers), field("peer_dropped_items", &ConfigListXMedkit::peer_dropped_items)); template <> @@ -248,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 new file mode 100644 index 000000000..f6e25af0c --- /dev/null +++ b/src/ros2_medkit_gateway/src/core/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 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; + 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/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp index 097874400..c90467ce6 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"). @@ -409,8 +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 + 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; @@ -435,16 +353,35 @@ 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 { + 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); + } else { + failed_nodes.push_back(node_info.node_fqn); + } } } 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; @@ -454,6 +391,22 @@ ConfigHandlers::list_configurations(const http::TypedRequest & req) { if (!queried_nodes.empty()) { xm.queried_nodes = queried_nodes; } + // 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 new file mode 100644 index 000000000..9a2e3bbec --- /dev/null +++ b/src/ros2_medkit_gateway/test/test_parameter_error_classification.cpp @@ -0,0 +1,113 @@ +// 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 +// 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 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); + 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..bb6b0ef02 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 @@ -36,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; @@ -45,11 +47,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 +67,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 +181,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 +222,26 @@ class ParameterErrorPrecedenceTest : public ::testing::Test { return result.has_value() ? ErrorInfo{} : result.error(); } + 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). + 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; + 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_{}; AuthConfig auth_{}; TlsConfig tls_{}; @@ -271,3 +302,69 @@ 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 collection = list_collection("stack_unavail_first"); + 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()) + << "unavailable node list must name the down node"; + // 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 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 +// 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()); +} + +// 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(); + + 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); +}