From 46b81b88ae32123d074b862905e5b9897ce6299f Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Apr 2026 15:52:20 +0200 Subject: [PATCH 1/8] fix(gateway/logs): aggregate component logs from hosted apps' fqns Synthetic / runtime-discovered components have an empty fqn (no manifest namespace), so the existing GET /components/{id}/logs path - which fed entity.fqn to LogManager::get_logs with prefix_match=true - silently returned zero items even when 20+ active nodes were mapped to that component. Manifest-defined components with a real fqn worked because their fqn matched the per-node logger name buffers. Mirror the AREA / FUNCTION semantics already in this handler: look up hosted apps via cache.get_apps_for_component(), build their effective fqns, and call get_logs(host_fqns, prefix_match=false). Fall through to the original namespace prefix path only when the component has no hosted apps but still has a non-empty fqn (manifest deployments where the component groups topics rather than nodes). Response now also carries x-medkit.aggregation_level=component plus aggregation_sources / app_count, parity with how AREA and FUNCTION already advertise their aggregation. Adds an integration test verifying component logs aggregate child app entries (verifies REQ_INTEROP_061). --- .../src/http/handlers/log_handlers.cpp | 66 ++++++++++++++++++- .../test/features/test_logging_api.test.py | 29 ++++++++ 2 files changed, 92 insertions(+), 3 deletions(-) diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 3465f01b7..223350e41 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -177,11 +177,71 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons } // ----------------------------------------------------------------------- - // COMPONENT / APP - local query + fan-out to peers + // COMPONENT - aggregate from hosted apps' fqns (mirrors AREA / FUNCTION + // semantics) and fall through to the entity.fqn prefix path only when + // the component has no hosted apps. Synthetic / runtime-discovered + // components have an empty fqn, so the bare prefix-match query that + // worked for manifest components silently returned zero items even + // when the component grouped 20+ active nodes. // ----------------------------------------------------------------------- - const bool prefix_match = (entity.type == EntityType::COMPONENT); + if (entity.type == EntityType::COMPONENT) { + const auto & cache = ctx_.node()->get_thread_safe_cache(); + const auto app_ids = cache.get_apps_for_component(entity_id); + + std::vector host_fqns; + host_fqns.reserve(app_ids.size()); + for (const auto & app_id : app_ids) { + auto app = cache.get_app(app_id); + if (!app) { + continue; + } + auto fqn = app->effective_fqn(); + if (!fqn.empty()) { + host_fqns.push_back(std::move(fqn)); + } + } + + json result; + result["items"] = json::array(); + XMedkit ext; + ext.entity_id(entity_id); + ext.add("aggregation_level", "component"); + ext.add("aggregated", true); + + if (!host_fqns.empty()) { + auto logs = log_mgr->get_logs(host_fqns, false, min_severity, context_filter, entity_id); + if (!logs) { + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, logs.error()); + return; + } + result["items"] = std::move(*logs); + ext.add("app_count", host_fqns.size()); + nlohmann::json comp_log_source_fqns = nlohmann::json::array(); + for (const auto & fqn : host_fqns) { + comp_log_source_fqns.push_back(fqn); + } + ext.add("aggregation_sources", comp_log_source_fqns); + } else if (!entity.fqn.empty()) { + // Manifest component without hosted apps - keep the original + // namespace prefix path so manifest-only deployments still work. + auto logs = log_mgr->get_logs({entity.fqn}, true, min_severity, context_filter, entity_id); + if (!logs) { + HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, logs.error()); + return; + } + result["items"] = std::move(*logs); + } - auto logs = log_mgr->get_logs({entity.fqn}, prefix_match, min_severity, context_filter, entity_id); + merge_peer_items(ctx_.aggregation_manager(), req, result, ext); + result["x-medkit"] = ext.build(); + HandlerContext::send_json(res, result); + return; + } + + // ----------------------------------------------------------------------- + // APP - local query + fan-out to peers + // ----------------------------------------------------------------------- + auto logs = log_mgr->get_logs({entity.fqn}, false, min_severity, context_filter, entity_id); if (!logs) { HandlerContext::send_error(res, 503, ERR_SERVICE_UNAVAILABLE, logs.error()); return; diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index df537f13e..de5683ab0 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -229,6 +229,35 @@ def test_component_get_logs_returns_200(self): self.assertIn('items', data) self.assertIsInstance(data['items'], list) + def test_component_get_logs_aggregates_child_apps(self): + """GET /components/{id}/logs aggregates from hosted apps. + + Synthetic / runtime-discovered components have an empty fqn, so + the legacy prefix-match path silently returned zero items. The + handler now mirrors the AREA / FUNCTION pattern: look up child + apps via the entity cache, build their host fqns, and merge. + + # @verifies REQ_INTEROP_061 + """ + components = self.get_json('/components')['items'] + self.assertGreater(len(components), 0, 'At least one component required') + # Pick the component that hosts temp_sensor (the only demo node). + comp_id = None + for c in components: + hosts = self.get_json(f"/components/{c['id']}/hosts").get('items', []) + if any(h.get('id') == 'temp_sensor' for h in hosts): + comp_id = c['id'] + break + self.assertIsNotNone(comp_id, 'No component hosts temp_sensor') + + data = self.get_json(f'/components/{comp_id}/logs?severity=debug') + self.assertIn('items', data) + ext = data.get('x-medkit', {}) + self.assertEqual(ext.get('aggregation_level'), 'component') + self.assertEqual(ext.get('aggregated'), True) + self.assertGreaterEqual(ext.get('app_count', 0), 1) + self.assertIn('temp_sensor', ext.get('aggregation_sources', [])) + def test_component_get_logs_configuration_returns_200(self): """GET /components/{id}/logs/configuration returns 200 with config. From 03f2c37f6bdee9b3b2f17ed7e2b8898c4dc5f51d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Apr 2026 17:01:27 +0200 Subject: [PATCH 2/8] fix(gateway/bulkdata): aggregate component sources from hosted apps' fqns The same synthetic-component bug already fixed for /components/{id}/logs also applies to BulkDataHandlers::get_source_filters. Synthetic / runtime-discovered components have empty fqn AND empty namespace_path, so the FUNCTION-only aggregation arm was not enough: components silently returned zero source filters, producing empty rosbag descriptor lists and failing ownership checks on download. Extend the aggregation arm to handle COMPONENT identically - resolve hosted apps via cache.get_apps_for_component() and build their effective_fqn() list. Manifest deployments where a component groups topics fall through to the existing namespace-prefix path. Strengthen the integration test for the original logs fix: assert items is non-empty (the bug returned 0 items with valid x-medkit metadata, so app_count alone is insufficient) and match aggregation_sources by FQN substring instead of bare app id. Add an integration test for component bulk-data aggregation, and update docs/api/rest.rst to describe the actual aggregation behavior for /components/{id}/logs. --- docs/api/rest.rst | 9 +++-- .../src/http/handlers/bulkdata_handlers.cpp | 16 ++++++--- .../test/features/test_bulk_data_api.test.py | 35 +++++++++++++++++++ .../test/features/test_logging_api.test.py | 10 +++++- 4 files changed, 63 insertions(+), 7 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index ac33489b3..3365fbbac 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -737,7 +737,8 @@ Logs Endpoints -------------- Query and configure the /rosout ring buffer for an entity. Supported entity types: -**areas** (namespace prefix match), **components** (namespace prefix match), **apps** (exact FQN match), +**areas** (aggregated from hosted apps, namespace prefix fallback), **components** (aggregated from +hosted apps, namespace prefix fallback for manifest-only deployments), **apps** (exact FQN match), and **functions** (aggregated from hosted apps). .. note:: @@ -748,7 +749,11 @@ and **functions** (aggregated from hosted apps). storage backend or take full ownership of the log pipeline (see plugin development docs). ``GET /api/v1/components/{id}/logs`` - Query log entries for all nodes in the component namespace (prefix match). + Query log entries aggregated from the component's hosted apps. Resolves child apps via + the entity cache and queries each by exact FQN. Falls back to namespace prefix match only + when the component has no hosted apps but declares a non-empty namespace (manifest-only + deployments where the component groups topics rather than nodes). The response carries + ``x-medkit.aggregation_level=component``, ``app_count``, and ``aggregation_sources``. ``GET /api/v1/apps/{id}/logs`` Query log entries for the specific app node (exact match). diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index a85446aff..b43716e2f 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -539,10 +539,14 @@ std::string BulkDataHandlers::get_rosbag_mimetype(const std::string & format) { } std::vector BulkDataHandlers::get_source_filters(const EntityInfo & entity) const { - if (entity.type == EntityType::FUNCTION) { - // Functions aggregate rosbags from all hosting apps + // FUNCTION and COMPONENT both aggregate rosbags from their hosting apps. + // Synthetic / runtime-discovered components have an empty fqn / namespace_path, + // so falling through to the bare-fqn path silently returned zero source filters + // and produced empty descriptor lists plus failed ownership checks on download. + if (entity.type == EntityType::FUNCTION || entity.type == EntityType::COMPONENT) { const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto host_app_ids = cache.get_apps_for_function(entity.id); + auto host_app_ids = (entity.type == EntityType::FUNCTION) ? cache.get_apps_for_function(entity.id) + : cache.get_apps_for_component(entity.id); std::vector filters; filters.reserve(host_app_ids.size()); for (const auto & app_id : host_app_ids) { @@ -554,7 +558,11 @@ std::vector BulkDataHandlers::get_source_filters(const EntityInfo & } } } - return filters; + if (!filters.empty()) { + return filters; + } + // Manifest deployments where a component groups topics rather than nodes + // still need the namespace prefix path to work, so fall through. } // For other entity types, use FQN or namespace_path diff --git a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py index 9a9ee33fe..0e4ba72a5 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_bulk_data_api.test.py @@ -158,6 +158,41 @@ def test_bulk_data_list_descriptors_empty_result(self): self.assertIn('items', data) self.assertIsInstance(data['items'], list) + def test_bulk_data_component_aggregates_child_apps(self): + """GET /components/{id}/bulk-data/rosbags aggregates from hosted apps. + + Synthetic / runtime-discovered components have an empty fqn / + namespace_path, so the legacy fall-through path returned zero source + filters and produced empty descriptor lists. The handler now resolves + hosted apps via the entity cache (mirrors the FUNCTION branch). + + @verifies REQ_INTEROP_072 + """ + comp_data = self.get_json('/components') + components = comp_data.get('items', []) + # Find the component that hosts lidar_sensor (the only demo app here). + comp_id = None + for c in components: + hosts = self.get_json( + f"/components/{c['id']}/hosts").get('items', []) + if any(h.get('id') == 'lidar_sensor' for h in hosts): + comp_id = c['id'] + break + self.assertIsNotNone( + comp_id, 'No component hosts lidar_sensor') + + # Poll - rosbag capture is fault-triggered and runs after launch. + data = self.poll_endpoint_until( + f'/components/{comp_id}/bulk-data/rosbags', + lambda d: d if d.get('items') else None, + timeout=15.0, + interval=1.0, + ) + self.assertGreater( + len(data['items']), 0, + 'Component bulk-data aggregation returned zero descriptors', + ) + def test_bulk_data_unknown_category_returns_404(self): """Bulk-data returns 404 for unknown category. diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index de5683ab0..256f983bb 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -252,11 +252,19 @@ def test_component_get_logs_aggregates_child_apps(self): data = self.get_json(f'/components/{comp_id}/logs?severity=debug') self.assertIn('items', data) + # Items must be non-empty - the original bug was a silent empty list + # for synthetic components, so app_count alone is not sufficient. + self.assertGreater(len(data['items']), 0, + 'Component logs aggregation returned zero items') ext = data.get('x-medkit', {}) self.assertEqual(ext.get('aggregation_level'), 'component') self.assertEqual(ext.get('aggregated'), True) self.assertGreaterEqual(ext.get('app_count', 0), 1) - self.assertIn('temp_sensor', ext.get('aggregation_sources', [])) + sources = ext.get('aggregation_sources', []) + self.assertTrue( + any('temp_sensor' in src for src in sources), + f"Expected aggregation_sources to contain a temp_sensor fqn, got: {sources}", + ) def test_component_get_logs_configuration_returns_200(self): """GET /components/{id}/logs/configuration returns 200 with config. From 778dd0f6708634cd42de1861186bcd9d6e5bc00d Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Tue, 28 Apr 2026 19:12:43 +0200 Subject: [PATCH 3/8] test+refactor(gateway): unit-test component aggregation + extract resolve_app_host_fqns Address review feedback on the component log/bulk-data aggregation fix: - Extract the per-app FQN resolution loop (cache lookup + effective_fqn + empty-skip) to HandlerContext::resolve_app_host_fqns and call it from log_handlers (FUNCTION/AREA/COMPONENT branches) and bulkdata_handlers (FUNCTION/COMPONENT). Removes three duplicate copies of the same loop and gives both handlers a single unit-tested helper. - Split FUNCTION and COMPONENT branches in BulkDataHandlers::get_source_filters. The previous combined arm fell through to entity.fqn / namespace_path for both types when host_fqns was empty, which silently changed FUNCTION semantics: functions are pure aggregated views and must return an empty filter list when no apps host them. COMPONENT keeps the fall-through for manifest-only deployments. - Make BulkDataHandlers::get_source_filters public (was private). The branching logic is non-trivial and needs unit coverage; calling it through the REST surface forces a real FaultManager service which is not viable in unit tests. - Add unit tests: * 7 ResolveAppHostFqnsTest cases pinning the silent-skip semantics (missing apps, empty effective_fqn, bound_fqn precedence). * 9 BulkDataSourceFiltersTest cases covering APP / AREA / FUNCTION / COMPONENT branching, including the regression guard that FUNCTION without hosts returns empty even if the entity carried a fqn. * 3 ComponentLogs* cases on the existing AreaAggregationTest fixture that exercise the COMPONENT branch in handle_get_logs end-to-end via the REST server: synthetic component aggregates from hosted apps, manifest-only component falls through, empty component returns empty items without crashing. - Set ROS_DOMAIN_ID for test_bulkdata_handlers (now stands up a real GatewayNode, needs domain isolation per CMakeLists.txt convention). - Strengthen the integration test for component logs aggregation: * Wrap the assertion in poll_endpoint_until - the /rosout buffer fills asynchronously after node startup, the prior immediate assertion was timing-dependent. * Match aggregation_sources by FQN substring instead of bare app id (the response carries full FQNs like /powertrain/engine/temp_sensor). - Clarify docs/api/rest.rst: app_count and aggregation_sources are populated only when hosted-app aggregation is active and are omitted under the namespace-prefix fallback. --- docs/api/rest.rst | 6 +- src/ros2_medkit_gateway/CMakeLists.txt | 1 + .../http/handlers/bulkdata_handlers.hpp | 19 +- .../http/handlers/handler_context.hpp | 21 ++ .../src/http/handlers/bulkdata_handlers.cpp | 37 ++- .../src/http/handlers/handler_context.cpp | 17 ++ .../src/http/handlers/log_handlers.cpp | 41 +-- .../test/test_bulkdata_handlers.cpp | 207 +++++++++++++++ .../test/test_handler_context.cpp | 243 +++++++++++++++++- .../test/features/test_logging_api.test.py | 10 +- 10 files changed, 534 insertions(+), 68 deletions(-) diff --git a/docs/api/rest.rst b/docs/api/rest.rst index 3365fbbac..f2b1bcfbf 100644 --- a/docs/api/rest.rst +++ b/docs/api/rest.rst @@ -752,8 +752,10 @@ and **functions** (aggregated from hosted apps). Query log entries aggregated from the component's hosted apps. Resolves child apps via the entity cache and queries each by exact FQN. Falls back to namespace prefix match only when the component has no hosted apps but declares a non-empty namespace (manifest-only - deployments where the component groups topics rather than nodes). The response carries - ``x-medkit.aggregation_level=component``, ``app_count``, and ``aggregation_sources``. + deployments where the component groups topics rather than nodes). The response always + carries ``x-medkit.aggregation_level=component`` and ``aggregated=true``; the + ``app_count`` and ``aggregation_sources`` fields are populated only when hosted-app + aggregation is active and are omitted under the namespace-prefix fallback. ``GET /api/v1/apps/{id}/logs`` Query log entries for the specific app node (exact match). diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index ccfbedb2d..16aac8b54 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -514,6 +514,7 @@ if(BUILD_TESTING) # Add bulk data handlers tests ament_add_gtest(test_bulkdata_handlers test/test_bulkdata_handlers.cpp) target_link_libraries(test_bulkdata_handlers gateway_lib) + medkit_set_test_domain(test_bulkdata_handlers) # Add subscription manager tests ament_add_gtest(test_subscription_manager test/test_subscription_manager.cpp) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp index ad2f5641e..eb45e2b20 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp @@ -17,6 +17,7 @@ #include #include +#include #include "ros2_medkit_gateway/http/handlers/handler_context.hpp" @@ -111,21 +112,27 @@ class BulkDataHandlers { */ static std::string get_rosbag_mimetype(const std::string & format); - private: - HandlerContext & ctx_; - /** * @brief Get source filters for rosbag queries based on entity type. * - * For apps/components/areas: returns the entity's FQN or namespace path. - * For functions: aggregates FQNs from all hosting apps (read-only - * aggregated view - upload/delete are blocked at the route level). + * - APP / AREA: returns the entity's FQN or namespace path (single filter). + * - FUNCTION: aggregates non-empty effective_fqn() values across all hosted apps + * (no fallback - functions are pure aggregated views). + * - COMPONENT: aggregates from hosted apps; falls back to FQN/namespace_path only + * when the component has no hosted apps (manifest deployments where the component + * groups topics rather than nodes). This avoids the synthetic-component bug where + * empty fqn + empty namespace_path produced zero source filters. + * + * Public to enable unit testing the entity-type branching directly. * * @param entity Entity information * @return Vector of source filter strings (empty if no valid filters) */ std::vector get_source_filters(const EntityInfo & entity) const; + private: + HandlerContext & ctx_; + /** * @brief Stream file contents to HTTP response. * @param res HTTP response to write to diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp index 987d0717f..84afaee9c 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp @@ -25,6 +25,8 @@ #include #include +#include + #include "ros2_medkit_gateway/auth/auth_config.hpp" #include "ros2_medkit_gateway/auth/auth_manager.hpp" #include "ros2_medkit_gateway/config.hpp" @@ -32,6 +34,7 @@ #include "ros2_medkit_gateway/http/http_utils.hpp" #include "ros2_medkit_gateway/models/entity_capabilities.hpp" #include "ros2_medkit_gateway/models/entity_types.hpp" +#include "ros2_medkit_gateway/models/thread_safe_entity_cache.hpp" namespace ros2_medkit_gateway { @@ -316,6 +319,24 @@ class HandlerContext { return rclcpp::get_logger("rest_server"); } + /** + * @brief Resolve a list of app IDs to their non-empty effective FQNs. + * + * Apps that are missing from the cache or that have an empty effective_fqn() + * are skipped silently. The returned vector preserves the input app_ids order + * (minus skipped entries) and may be empty. + * + * Used by log_handlers and bulkdata_handlers to aggregate per-component / + * per-function resource queries from the entity's hosted apps. Static + public + * to enable direct unit testing without standing up a full GatewayNode fixture. + * + * @param cache Entity cache to look up apps in + * @param app_ids App IDs to resolve + * @return Effective FQNs for the apps that resolved + */ + static std::vector resolve_app_host_fqns(const ThreadSafeEntityCache & cache, + const std::vector & app_ids); + private: GatewayNode * node_; CorsConfig cors_config_; diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index b43716e2f..475eb64bb 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -539,33 +539,28 @@ std::string BulkDataHandlers::get_rosbag_mimetype(const std::string & format) { } std::vector BulkDataHandlers::get_source_filters(const EntityInfo & entity) const { - // FUNCTION and COMPONENT both aggregate rosbags from their hosting apps. - // Synthetic / runtime-discovered components have an empty fqn / namespace_path, - // so falling through to the bare-fqn path silently returned zero source filters - // and produced empty descriptor lists plus failed ownership checks on download. - if (entity.type == EntityType::FUNCTION || entity.type == EntityType::COMPONENT) { + if (entity.type == EntityType::FUNCTION) { + // Functions are pure aggregated views over hosted apps - if no apps host the function, + // there is nothing to query. No fall-through to fqn/namespace_path. const auto & cache = ctx_.node()->get_thread_safe_cache(); - auto host_app_ids = (entity.type == EntityType::FUNCTION) ? cache.get_apps_for_function(entity.id) - : cache.get_apps_for_component(entity.id); - std::vector filters; - filters.reserve(host_app_ids.size()); - for (const auto & app_id : host_app_ids) { - auto app = cache.get_app(app_id); - if (app) { - auto fqn = app->effective_fqn(); - if (!fqn.empty()) { - filters.push_back(fqn); - } - } - } + return HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_function(entity.id)); + } + + if (entity.type == EntityType::COMPONENT) { + // Synthetic / runtime-discovered components have an empty fqn / namespace_path, + // so the bare-fqn path used to silently return zero source filters and produce + // empty descriptor lists plus failed ownership checks on download. Resolve hosted + // apps first; manifest deployments where the component groups topics rather than + // nodes still need the namespace prefix path, so fall through if no apps host it. + const auto & cache = ctx_.node()->get_thread_safe_cache(); + auto filters = HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_component(entity.id)); if (!filters.empty()) { return filters; } - // Manifest deployments where a component groups topics rather than nodes - // still need the namespace prefix path to work, so fall through. + // fall through to fqn/namespace_path } - // For other entity types, use FQN or namespace_path + // For other entity types and manifest-only components, use FQN or namespace_path std::string filter = entity.fqn.empty() ? entity.namespace_path : entity.fqn; if (filter.empty()) { return {}; diff --git a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp index 87cd77605..8f38e854d 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp @@ -346,5 +346,22 @@ void HandlerContext::send_json(httplib::Response & res, const json & data) { res.set_content(data.dump(2), "application/json"); } +std::vector HandlerContext::resolve_app_host_fqns(const ThreadSafeEntityCache & cache, + const std::vector & app_ids) { + std::vector fqns; + fqns.reserve(app_ids.size()); + for (const auto & app_id : app_ids) { + auto app = cache.get_app(app_id); + if (!app) { + continue; + } + auto fqn = app->effective_fqn(); + if (!fqn.empty()) { + fqns.push_back(std::move(fqn)); + } + } + return fqns; +} + } // namespace handlers } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 223350e41..7d986f6a0 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -85,17 +85,7 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons ext.add("aggregated", true); if (func && !func->hosts.empty()) { - std::vector host_fqns; - for (const auto & app_id : func->hosts) { - auto app = cache.get_app(app_id); - if (!app) { - continue; - } - auto fqn = app->effective_fqn(); - if (!fqn.empty()) { - host_fqns.push_back(std::move(fqn)); - } - } + auto host_fqns = HandlerContext::resolve_app_host_fqns(cache, func->hosts); if (!host_fqns.empty()) { auto logs = log_mgr->get_logs(host_fqns, false, min_severity, context_filter, entity_id); @@ -128,17 +118,9 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons std::vector host_fqns; for (const auto & comp_id : comp_ids) { - auto app_ids = cache.get_apps_for_component(comp_id); - for (const auto & app_id : app_ids) { - auto app = cache.get_app(app_id); - if (!app) { - continue; - } - auto fqn = app->effective_fqn(); - if (!fqn.empty()) { - host_fqns.push_back(std::move(fqn)); - } - } + auto comp_fqns = HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_component(comp_id)); + host_fqns.insert(host_fqns.end(), std::make_move_iterator(comp_fqns.begin()), + std::make_move_iterator(comp_fqns.end())); } json result; @@ -186,20 +168,7 @@ void LogHandlers::handle_get_logs(const httplib::Request & req, httplib::Respons // ----------------------------------------------------------------------- if (entity.type == EntityType::COMPONENT) { const auto & cache = ctx_.node()->get_thread_safe_cache(); - const auto app_ids = cache.get_apps_for_component(entity_id); - - std::vector host_fqns; - host_fqns.reserve(app_ids.size()); - for (const auto & app_id : app_ids) { - auto app = cache.get_app(app_id); - if (!app) { - continue; - } - auto fqn = app->effective_fqn(); - if (!fqn.empty()) { - host_fqns.push_back(std::move(fqn)); - } - } + auto host_fqns = HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_component(entity_id)); json result; result["items"] = json::array(); diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index f0a304cd0..c6a941e0e 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -14,14 +14,25 @@ #include +#include #include +#include #include "ros2_medkit_gateway/bulk_data_store.hpp" +#include "ros2_medkit_gateway/discovery/models/app.hpp" +#include "ros2_medkit_gateway/discovery/models/component.hpp" +#include "ros2_medkit_gateway/discovery/models/function.hpp" +#include "ros2_medkit_gateway/gateway_node.hpp" #include "ros2_medkit_gateway/http/error_codes.hpp" #include "ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp" +#include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/http_utils.hpp" +#include "ros2_medkit_gateway/models/entity_types.hpp" +#include "ros2_medkit_gateway/models/thread_safe_entity_cache.hpp" +using namespace ros2_medkit_gateway; using ros2_medkit_gateway::handlers::BulkDataHandlers; +using ros2_medkit_gateway::handlers::HandlerContext; class BulkDataHandlersTest : public ::testing::Test { protected: @@ -189,3 +200,199 @@ TEST_F(BulkDataHandlersTest, PayloadTooLargeErrorCodeDefined) { EXPECT_NE(ros2_medkit_gateway::ERR_PAYLOAD_TOO_LARGE, nullptr); EXPECT_STREQ(ros2_medkit_gateway::ERR_PAYLOAD_TOO_LARGE, "payload-too-large"); } + +// ============================================================================= +// get_source_filters tests +// +// Pin the entity-type branching that drives rosbag descriptor lookups + the +// download ownership check. Crucial because synthetic / runtime-discovered +// components have empty fqn AND empty namespace_path: without aggregation +// from hosted apps the handler used to silently return zero source filters. +// ============================================================================= + +namespace { + +class RclcppEnvironment : public ::testing::Environment { + public: + void SetUp() override { + if (!rclcpp::ok()) { + rclcpp::init(0, nullptr); + } + } + void TearDown() override { + rclcpp::shutdown(); + } +}; + +::testing::Environment * const rclcpp_env = ::testing::AddGlobalTestEnvironment(new RclcppEnvironment); + +App make_test_app(const std::string & id, const std::string & node_name, const std::string & ns, + const std::string & component_id) { + App a; + a.id = id; + a.name = id; + a.component_id = component_id; + App::RosBinding rb; + rb.node_name = node_name; + rb.namespace_pattern = ns; + a.ros_binding = rb; + return a; +} + +EntityInfo make_entity_info(EntityType type, const std::string & id, const std::string & namespace_path, + const std::string & fqn) { + EntityInfo info; + info.type = type; + info.id = id; + info.namespace_path = namespace_path; + info.fqn = fqn; + return info; +} + +} // namespace + +class BulkDataSourceFiltersTest : public ::testing::Test { + protected: + static inline std::shared_ptr suite_node_; + + static void SetUpTestSuite() { + rclcpp::NodeOptions options; + options.append_parameter_override("server.host", std::string("127.0.0.1")); + options.append_parameter_override("server.port", 0); // disabled + options.append_parameter_override("refresh_interval_ms", 60000); + suite_node_ = std::make_shared(options); + ASSERT_NE(suite_node_, nullptr); + } + + static void TearDownTestSuite() { + suite_node_.reset(); + } + + void SetUp() override { + ASSERT_NE(suite_node_, nullptr); + + // Synthetic component: empty fqn AND empty namespace_path. + Component synthetic; + synthetic.id = "runtime_engine"; + synthetic.name = "Runtime Engine"; + synthetic.namespace_path = ""; + synthetic.fqn = ""; + + // Manifest-only component: declares namespace but groups topics, not nodes. + Component manifest_only; + manifest_only.id = "topics_group"; + manifest_only.name = "Topics Group"; + manifest_only.namespace_path = "/topics/group"; + manifest_only.fqn = "/topics/group"; + + auto app1 = make_test_app("temp_sensor", "temp_sensor", "/powertrain/engine", "runtime_engine"); + auto app2 = make_test_app("rpm_sensor", "rpm_sensor", "/powertrain/engine", "runtime_engine"); + + Function func; + func.id = "powertrain_diag"; + func.name = "Powertrain Diagnostics"; + func.hosts = {"temp_sensor", "rpm_sensor"}; + + Function empty_func; + empty_func.id = "empty_func"; + empty_func.name = "Empty Function"; + + auto & cache = const_cast(suite_node_->get_thread_safe_cache()); + cache.update_all({}, {synthetic, manifest_only}, {app1, app2}, {func, empty_func}); + + ctx_ = std::make_unique(suite_node_.get(), cors_, auth_, tls_, nullptr); + handlers_ = std::make_unique(*ctx_); + } + + void TearDown() override { + handlers_.reset(); + ctx_.reset(); + } + + CorsConfig cors_{}; + AuthConfig auth_{}; + TlsConfig tls_{}; + std::unique_ptr ctx_; + std::unique_ptr handlers_; +}; + +// COMPONENT with hosted apps - returns app effective FQNs (synthetic component +// has empty fqn / namespace_path; without aggregation this would be {}). +TEST_F(BulkDataSourceFiltersTest, ComponentWithHostedAppsReturnsAppFqns) { + auto entity = make_entity_info(EntityType::COMPONENT, "runtime_engine", "", ""); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 2u); + std::set as_set(filters.begin(), filters.end()); + EXPECT_TRUE(as_set.count("/powertrain/engine/temp_sensor")); + EXPECT_TRUE(as_set.count("/powertrain/engine/rpm_sensor")); +} + +// COMPONENT with no hosted apps but non-empty fqn falls through to fqn path +// (manifest deployment grouping topics rather than nodes). +TEST_F(BulkDataSourceFiltersTest, ComponentManifestOnlyFallsThroughToFqn) { + auto entity = make_entity_info(EntityType::COMPONENT, "topics_group", "/topics/group", "/topics/group"); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 1u); + EXPECT_EQ(filters[0], "/topics/group"); +} + +// COMPONENT with no hosted apps AND no fqn / namespace_path returns empty - +// nothing to query. +TEST_F(BulkDataSourceFiltersTest, ComponentSyntheticWithoutAppsReturnsEmpty) { + auto entity = make_entity_info(EntityType::COMPONENT, "nonexistent_comp", "", ""); + auto filters = handlers_->get_source_filters(entity); + EXPECT_TRUE(filters.empty()); +} + +// FUNCTION with hosted apps - returns app effective FQNs. Crucially, FUNCTION +// must NOT fall through to namespace_path/fqn even when the host list is +// non-empty - functions are pure aggregated views. +TEST_F(BulkDataSourceFiltersTest, FunctionWithHostsReturnsAppFqns) { + auto entity = make_entity_info(EntityType::FUNCTION, "powertrain_diag", "", ""); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 2u); + std::set as_set(filters.begin(), filters.end()); + EXPECT_TRUE(as_set.count("/powertrain/engine/temp_sensor")); + EXPECT_TRUE(as_set.count("/powertrain/engine/rpm_sensor")); +} + +// FUNCTION without hosted apps returns empty - no fall-through to fqn even if +// the entity carried one (regression guard for the original FUNCTION semantics +// after the COMPONENT/FUNCTION split). +TEST_F(BulkDataSourceFiltersTest, FunctionWithoutHostsReturnsEmptyEvenIfFqnSet) { + auto entity = make_entity_info(EntityType::FUNCTION, "empty_func", "/some/ns", "/some/fqn"); + auto filters = handlers_->get_source_filters(entity); + EXPECT_TRUE(filters.empty()); +} + +// APP entity returns its own fqn as the single filter - no aggregation. +TEST_F(BulkDataSourceFiltersTest, AppReturnsSingleFqnFilter) { + auto entity = + make_entity_info(EntityType::APP, "temp_sensor", "/powertrain/engine", "/powertrain/engine/temp_sensor"); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 1u); + EXPECT_EQ(filters[0], "/powertrain/engine/temp_sensor"); +} + +// APP without fqn falls through to namespace_path filter. +TEST_F(BulkDataSourceFiltersTest, AppWithEmptyFqnFallsThroughToNamespacePath) { + auto entity = make_entity_info(EntityType::APP, "some_app", "/the/namespace", ""); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 1u); + EXPECT_EQ(filters[0], "/the/namespace"); +} + +// APP with neither fqn nor namespace_path returns empty. +TEST_F(BulkDataSourceFiltersTest, AppWithEmptyFqnAndNamespaceReturnsEmpty) { + auto entity = make_entity_info(EntityType::APP, "some_app", "", ""); + auto filters = handlers_->get_source_filters(entity); + EXPECT_TRUE(filters.empty()); +} + +// AREA entity uses fqn directly - no aggregation in this helper. +TEST_F(BulkDataSourceFiltersTest, AreaReturnsFqnAsFilter) { + auto entity = make_entity_info(EntityType::AREA, "powertrain", "/powertrain", "/powertrain"); + auto filters = handlers_->get_source_filters(entity); + ASSERT_EQ(filters.size(), 1u); + EXPECT_EQ(filters[0], "/powertrain"); +} diff --git a/src/ros2_medkit_gateway/test/test_handler_context.cpp b/src/ros2_medkit_gateway/test/test_handler_context.cpp index 7dd4e3de0..bcc8f7c78 100644 --- a/src/ros2_medkit_gateway/test/test_handler_context.cpp +++ b/src/ros2_medkit_gateway/test/test_handler_context.cpp @@ -39,7 +39,6 @@ using namespace ros2_medkit_gateway; using namespace ros2_medkit_gateway::handlers; -using json = nlohmann::json; // ============================================================================= // HandlerContext static method tests (don't require GatewayNode) @@ -931,6 +930,153 @@ TEST_F(AreaAggregationTest, AreaLogsReturnsAggregatedResult) { EXPECT_EQ(sources.size(), 2); } +// ============================================================================= +// Component aggregation tests for synthetic / runtime-discovered components. +// +// Synthetic components have empty fqn AND empty namespace_path. The COMPONENT +// branch in handle_get_logs must resolve hosted apps via the entity cache and +// emit aggregation metadata; otherwise the response was silently empty for +// every runtime-discovered component (the original bug fixed by this branch). +// ============================================================================= + +// Component with hosted apps but empty fqn/namespace_path returns aggregated +// metadata + items list. With no log buffer entries the items list is empty, +// but x-medkit must still report aggregation_level=component plus app_count +// and aggregation_sources covering both hosted apps. +TEST_F(AreaAggregationTest, ComponentLogsAggregatesFromHostedAppsForSyntheticComponent) { + auto & cache = const_cast(suite_node_->get_thread_safe_cache()); + + // Synthetic component: empty fqn AND empty namespace_path - mirrors what + // the runtime discovery strategy produces for components grouping nodes by + // namespace. + Component synthetic; + synthetic.id = "runtime_engine"; + synthetic.name = "Runtime Engine"; + synthetic.area = "powertrain"; + synthetic.namespace_path = ""; + synthetic.fqn = ""; + + // Synthetic components store runtime-discovered apps with bound_fqn populated + // by the discovery layer (manifest-only path uses ros_binding instead). + App app1; + app1.id = "temp_sensor"; + app1.name = "Temperature Sensor"; + app1.component_id = "runtime_engine"; + app1.bound_fqn = "/powertrain/engine/temp_sensor"; + + App app2; + app2.id = "rpm_sensor"; + app2.name = "RPM Sensor"; + app2.component_id = "runtime_engine"; + app2.bound_fqn = "/powertrain/engine/rpm_sensor"; + + Area area; + area.id = "powertrain"; + area.name = "Powertrain"; + area.namespace_path = "/powertrain"; + cache.update_all({area}, {synthetic}, {app1, app2}, {}); + + auto res = client_->Get("/api/v1/components/runtime_engine/logs"); + ASSERT_NE(res, nullptr) << "HTTP request failed"; + EXPECT_EQ(res->status, 200); + + auto body = json::parse(res->body); + ASSERT_TRUE(body.contains("items")); + EXPECT_TRUE(body["items"].is_array()); + + ASSERT_TRUE(body.contains("x-medkit")); + auto xmedkit = body["x-medkit"]; + EXPECT_EQ(xmedkit["entity_id"], "runtime_engine"); + EXPECT_EQ(xmedkit["aggregation_level"], "component"); + EXPECT_TRUE(xmedkit["aggregated"].get()); + EXPECT_EQ(xmedkit["app_count"], 2); + + ASSERT_TRUE(xmedkit.contains("aggregation_sources")); + auto sources = xmedkit["aggregation_sources"]; + ASSERT_EQ(sources.size(), 2); + std::set source_set; + for (const auto & s : sources) { + source_set.insert(s.get()); + } + EXPECT_TRUE(source_set.count("/powertrain/engine/temp_sensor")); + EXPECT_TRUE(source_set.count("/powertrain/engine/rpm_sensor")); +} + +// Manifest component without hosted apps (component groups topics rather than +// nodes) falls through to the namespace-prefix path. Aggregation metadata +// reports level=component + aggregated=true but omits app_count and +// aggregation_sources because hosted-app aggregation was not active. +TEST_F(AreaAggregationTest, ComponentLogsManifestOnlyFallsThroughToNamespacePrefix) { + auto & cache = const_cast(suite_node_->get_thread_safe_cache()); + + // Manifest component with non-empty fqn but no hosted apps. + Component manifest_comp; + manifest_comp.id = "topic_only_component"; + manifest_comp.name = "Topic-Only Component"; + manifest_comp.area = "powertrain"; + manifest_comp.namespace_path = "/topics/group"; + manifest_comp.fqn = "/topics/group"; + + Area area; + area.id = "powertrain"; + area.name = "Powertrain"; + area.namespace_path = "/powertrain"; + cache.update_all({area}, {manifest_comp}, {}, {}); + + auto res = client_->Get("/api/v1/components/topic_only_component/logs"); + ASSERT_NE(res, nullptr) << "HTTP request failed"; + EXPECT_EQ(res->status, 200); + + auto body = json::parse(res->body); + ASSERT_TRUE(body.contains("items")); + EXPECT_TRUE(body["items"].is_array()); + + ASSERT_TRUE(body.contains("x-medkit")); + auto xmedkit = body["x-medkit"]; + EXPECT_EQ(xmedkit["aggregation_level"], "component"); + EXPECT_TRUE(xmedkit["aggregated"].get()); + // Hosted-app aggregation was not active, so these conditional fields must + // be omitted (per docs/api/rest.rst contract). + EXPECT_FALSE(xmedkit.contains("app_count")); + EXPECT_FALSE(xmedkit.contains("aggregation_sources")); +} + +// Component without hosted apps AND without fqn / namespace_path returns an +// empty items list - there is no source to query. Metadata still reports +// aggregation_level=component (the handler classified it as a component +// request) but no source metadata is emitted. +TEST_F(AreaAggregationTest, ComponentLogsEmptyComponentReturnsEmptyItemsAndNoSources) { + auto & cache = const_cast(suite_node_->get_thread_safe_cache()); + + Component empty_comp; + empty_comp.id = "empty_runtime_comp"; + empty_comp.name = "Empty Runtime Component"; + empty_comp.area = "powertrain"; + empty_comp.namespace_path = ""; + empty_comp.fqn = ""; + + Area area; + area.id = "powertrain"; + area.name = "Powertrain"; + area.namespace_path = "/powertrain"; + cache.update_all({area}, {empty_comp}, {}, {}); + + auto res = client_->Get("/api/v1/components/empty_runtime_comp/logs"); + ASSERT_NE(res, nullptr) << "HTTP request failed"; + EXPECT_EQ(res->status, 200); + + auto body = json::parse(res->body); + ASSERT_TRUE(body.contains("items")); + EXPECT_TRUE(body["items"].is_array()); + EXPECT_EQ(body["items"].size(), 0u); + + ASSERT_TRUE(body.contains("x-medkit")); + auto xmedkit = body["x-medkit"]; + EXPECT_EQ(xmedkit["aggregation_level"], "component"); + EXPECT_FALSE(xmedkit.contains("app_count")); + EXPECT_FALSE(xmedkit.contains("aggregation_sources")); +} + // Area with no components falls through to namespace prefix matching for logs. // With no matching logs, returns empty items. TEST_F(AreaAggregationTest, AreaLogsWithNoComponentsFallsThrough) { @@ -970,6 +1116,101 @@ TEST_F(AreaAggregationTest, AreaLogsWithNoComponentsFallsThrough) { EXPECT_TRUE(body["items"].is_array()); } +// ============================================================================= +// resolve_app_host_fqns tests (no GatewayNode required) +// +// Used by log_handlers and bulkdata_handlers to aggregate per-component / +// per-function resource queries from the entity's hosted apps. These tests +// pin the silent-skip semantics (missing apps, empty effective_fqn) that +// downstream callers rely on for the "synthetic component" fallback. +// ============================================================================= + +namespace { + +App make_app_with_binding(const std::string & id, const std::string & node_name, const std::string & ns) { + App a; + a.id = id; + a.name = id; + App::RosBinding rb; + rb.node_name = node_name; + rb.namespace_pattern = ns; + a.ros_binding = rb; + return a; +} + +} // namespace + +TEST(ResolveAppHostFqnsTest, EmptyAppListReturnsEmpty) { + ThreadSafeEntityCache cache; + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {}); + EXPECT_TRUE(fqns.empty()); +} + +TEST(ResolveAppHostFqnsTest, ResolvesSingleAppByEffectiveFqn) { + ThreadSafeEntityCache cache; + cache.update_apps({make_app_with_binding("temp_sensor", "temp_sensor", "/powertrain/engine")}); + + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"temp_sensor"}); + ASSERT_EQ(fqns.size(), 1u); + EXPECT_EQ(fqns[0], "/powertrain/engine/temp_sensor"); +} + +TEST(ResolveAppHostFqnsTest, ResolvesMultipleAppsPreservesInputOrder) { + ThreadSafeEntityCache cache; + cache.update_apps({ + make_app_with_binding("temp_sensor", "temp_sensor", "/powertrain/engine"), + make_app_with_binding("rpm_sensor", "rpm_sensor", "/powertrain/engine"), + make_app_with_binding("lidar", "lidar_sensor", "/perception/lidar"), + }); + + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"temp_sensor", "lidar", "rpm_sensor"}); + ASSERT_EQ(fqns.size(), 3u); + EXPECT_EQ(fqns[0], "/powertrain/engine/temp_sensor"); + EXPECT_EQ(fqns[1], "/perception/lidar/lidar_sensor"); + EXPECT_EQ(fqns[2], "/powertrain/engine/rpm_sensor"); +} + +TEST(ResolveAppHostFqnsTest, SkipsAppIdsMissingFromCache) { + ThreadSafeEntityCache cache; + cache.update_apps({make_app_with_binding("temp_sensor", "temp_sensor", "/powertrain/engine")}); + + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"missing_app", "temp_sensor", "also_missing"}); + ASSERT_EQ(fqns.size(), 1u); + EXPECT_EQ(fqns[0], "/powertrain/engine/temp_sensor"); +} + +TEST(ResolveAppHostFqnsTest, SkipsAppsWithEmptyEffectiveFqn) { + // App without ros_binding and without bound_fqn yields empty effective_fqn(). + ThreadSafeEntityCache cache; + App empty_fqn_app; + empty_fqn_app.id = "no_binding"; + empty_fqn_app.name = "no_binding"; + cache.update_apps({empty_fqn_app, make_app_with_binding("temp_sensor", "temp_sensor", "/powertrain/engine")}); + + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"no_binding", "temp_sensor"}); + ASSERT_EQ(fqns.size(), 1u); + EXPECT_EQ(fqns[0], "/powertrain/engine/temp_sensor"); +} + +TEST(ResolveAppHostFqnsTest, AllAppsMissingReturnsEmpty) { + ThreadSafeEntityCache cache; + // Cache is empty - none of these IDs exist + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"a", "b", "c"}); + EXPECT_TRUE(fqns.empty()); +} + +TEST(ResolveAppHostFqnsTest, PrefersBoundFqnOverRosBinding) { + // bound_fqn (set by runtime linking) wins over ros_binding-derived FQN. + ThreadSafeEntityCache cache; + App linked = make_app_with_binding("temp_sensor", "temp_sensor", "/powertrain/engine"); + linked.bound_fqn = "/runtime/discovered/path"; + cache.update_apps({linked}); + + auto fqns = HandlerContext::resolve_app_host_fqns(cache, {"temp_sensor"}); + ASSERT_EQ(fqns.size(), 1u); + EXPECT_EQ(fqns[0], "/runtime/discovered/path"); +} + int main(int argc, char ** argv) { testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS(); diff --git a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py index 256f983bb..25fd764f5 100644 --- a/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py +++ b/src/ros2_medkit_integration_tests/test/features/test_logging_api.test.py @@ -250,8 +250,14 @@ def test_component_get_logs_aggregates_child_apps(self): break self.assertIsNotNone(comp_id, 'No component hosts temp_sensor') - data = self.get_json(f'/components/{comp_id}/logs?severity=debug') - self.assertIn('items', data) + # Poll - /rosout buffer fills asynchronously after node startup, so + # the items list may be briefly empty. Without polling this assertion + # is timing-dependent in CI. + data = self.poll_endpoint_until( + f'/components/{comp_id}/logs?severity=debug', + condition=lambda d: d if d.get('items') else None, + timeout=15.0, + ) # Items must be non-empty - the original bug was a silent empty list # for synthetic components, so app_count alone is not sufficient. self.assertGreater(len(data['items']), 0, From 98c06e5c2b9be9c3391a01222b7bead0668921b6 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Apr 2026 13:01:29 +0200 Subject: [PATCH 4/8] fix(gateway): keep get_source_filters private, extract logic to detail:: Address two review findings: - Move ``get_source_filters`` back to ``private``. Extract the pure entity-type branching to ``handlers::detail::compute_bulkdata_source_filters``, a free function that takes a ``ThreadSafeEntityCache`` reference plus an ``EntityInfo``. The instance method becomes a one-line wrapper that fetches the cache from ``ctx_`` and delegates. This keeps the handler's public API unchanged while letting the unit tests target the helper directly without spinning up a ``GatewayNode`` (the previous fixture stood up a real ``GatewayNode`` purely to obtain a cache reference). - Add ``#include `` to ``log_handlers.cpp`` for ``std::make_move_iterator`` (was relying on transitive inclusion). Test rewrite: ``BulkDataSourceFiltersTest`` now constructs a bare ``ThreadSafeEntityCache``, calls ``cache_.update_all(...)``, and invokes ``handlers::detail::compute_bulkdata_source_filters`` directly. No DDS context, no rclcpp init, no ``ros2_medkit_test_utils`` shared library required. Drops the ``medkit_set_test_domain`` line for the same reason - this fixture no longer creates a ROS node, so domain isolation is moot. All 9 entity-type cases (synthetic component, manifest fallback, function without hosts regression guard, etc.) still cover the same scenarios. --- src/ros2_medkit_gateway/CMakeLists.txt | 1 - .../http/handlers/bulkdata_handlers.hpp | 50 ++++++++---- .../src/http/handlers/bulkdata_handlers.cpp | 11 ++- .../src/http/handlers/log_handlers.cpp | 1 + .../test/test_bulkdata_handlers.cpp | 79 ++++--------------- 5 files changed, 62 insertions(+), 80 deletions(-) diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index 16aac8b54..ccfbedb2d 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -514,7 +514,6 @@ if(BUILD_TESTING) # Add bulk data handlers tests ament_add_gtest(test_bulkdata_handlers test/test_bulkdata_handlers.cpp) target_link_libraries(test_bulkdata_handlers gateway_lib) - medkit_set_test_domain(test_bulkdata_handlers) # Add subscription manager tests ament_add_gtest(test_subscription_manager test/test_subscription_manager.cpp) diff --git a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp index eb45e2b20..87bd9c6be 100644 --- a/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp +++ b/src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp @@ -112,27 +112,19 @@ class BulkDataHandlers { */ static std::string get_rosbag_mimetype(const std::string & format); + private: + HandlerContext & ctx_; + /** * @brief Get source filters for rosbag queries based on entity type. * - * - APP / AREA: returns the entity's FQN or namespace path (single filter). - * - FUNCTION: aggregates non-empty effective_fqn() values across all hosted apps - * (no fallback - functions are pure aggregated views). - * - COMPONENT: aggregates from hosted apps; falls back to FQN/namespace_path only - * when the component has no hosted apps (manifest deployments where the component - * groups topics rather than nodes). This avoids the synthetic-component bug where - * empty fqn + empty namespace_path produced zero source filters. - * - * Public to enable unit testing the entity-type branching directly. - * - * @param entity Entity information - * @return Vector of source filter strings (empty if no valid filters) + * Thin instance wrapper that fetches the cache from ctx_ and delegates to + * detail::compute_bulkdata_source_filters. The pure logic (entity-type + * branching) is unit-tested via the free function instead of the member + * to keep the handler's public surface unchanged. */ std::vector get_source_filters(const EntityInfo & entity) const; - private: - HandlerContext & ctx_; - /** * @brief Stream file contents to HTTP response. * @param res HTTP response to write to @@ -155,5 +147,33 @@ class BulkDataHandlers { static std::string resolve_rosbag_file_path(const std::string & path); }; +namespace detail { + +/** + * @brief Compute rosbag source filters for an entity based on its type. + * + * Pure helper that drives ``BulkDataHandlers::get_source_filters``. Lives in + * a ``detail`` namespace to signal "not part of the public API" while still + * being directly unit-testable without spinning up a ``GatewayNode``. + * + * - APP / AREA: returns the entity's FQN or namespace path (single filter). + * - FUNCTION: aggregates non-empty ``effective_fqn()`` values across all + * hosted apps (no fallback - functions are pure aggregated views). + * - COMPONENT: aggregates from hosted apps; falls back to FQN/namespace_path + * only when the component has no hosted apps (manifest deployments where + * the component groups topics rather than nodes). This avoids the + * synthetic-component bug where empty fqn + empty namespace_path produced + * zero source filters. + * + * @param cache Entity cache to resolve hosted apps in (used for FUNCTION / + * COMPONENT only) + * @param entity Entity information + * @return Vector of source filter strings (empty if no valid filters) + */ +std::vector compute_bulkdata_source_filters(const ThreadSafeEntityCache & cache, + const EntityInfo & entity); + +} // namespace detail + } // namespace handlers } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp index 475eb64bb..bd745780c 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/bulkdata_handlers.cpp @@ -539,10 +539,16 @@ std::string BulkDataHandlers::get_rosbag_mimetype(const std::string & format) { } std::vector BulkDataHandlers::get_source_filters(const EntityInfo & entity) const { + return detail::compute_bulkdata_source_filters(ctx_.node()->get_thread_safe_cache(), entity); +} + +namespace detail { + +std::vector compute_bulkdata_source_filters(const ThreadSafeEntityCache & cache, + const EntityInfo & entity) { if (entity.type == EntityType::FUNCTION) { // Functions are pure aggregated views over hosted apps - if no apps host the function, // there is nothing to query. No fall-through to fqn/namespace_path. - const auto & cache = ctx_.node()->get_thread_safe_cache(); return HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_function(entity.id)); } @@ -552,7 +558,6 @@ std::vector BulkDataHandlers::get_source_filters(const EntityInfo & // empty descriptor lists plus failed ownership checks on download. Resolve hosted // apps first; manifest deployments where the component groups topics rather than // nodes still need the namespace prefix path, so fall through if no apps host it. - const auto & cache = ctx_.node()->get_thread_safe_cache(); auto filters = HandlerContext::resolve_app_host_fqns(cache, cache.get_apps_for_component(entity.id)); if (!filters.empty()) { return filters; @@ -568,5 +573,7 @@ std::vector BulkDataHandlers::get_source_filters(const EntityInfo & return {filter}; } +} // namespace detail + } // namespace handlers } // namespace ros2_medkit_gateway diff --git a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp index 7d986f6a0..44babfdc1 100644 --- a/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp +++ b/src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp @@ -14,6 +14,7 @@ #include "ros2_medkit_gateway/http/handlers/log_handlers.hpp" +#include #include #include #include diff --git a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp index c6a941e0e..10f4625fb 100644 --- a/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp +++ b/src/ros2_medkit_gateway/test/test_bulkdata_handlers.cpp @@ -14,25 +14,21 @@ #include -#include #include -#include +#include +#include #include "ros2_medkit_gateway/bulk_data_store.hpp" #include "ros2_medkit_gateway/discovery/models/app.hpp" #include "ros2_medkit_gateway/discovery/models/component.hpp" #include "ros2_medkit_gateway/discovery/models/function.hpp" -#include "ros2_medkit_gateway/gateway_node.hpp" #include "ros2_medkit_gateway/http/error_codes.hpp" #include "ros2_medkit_gateway/http/handlers/bulkdata_handlers.hpp" -#include "ros2_medkit_gateway/http/handlers/handler_context.hpp" #include "ros2_medkit_gateway/http/http_utils.hpp" -#include "ros2_medkit_gateway/models/entity_types.hpp" #include "ros2_medkit_gateway/models/thread_safe_entity_cache.hpp" using namespace ros2_medkit_gateway; using ros2_medkit_gateway::handlers::BulkDataHandlers; -using ros2_medkit_gateway::handlers::HandlerContext; class BulkDataHandlersTest : public ::testing::Test { protected: @@ -202,30 +198,19 @@ TEST_F(BulkDataHandlersTest, PayloadTooLargeErrorCodeDefined) { } // ============================================================================= -// get_source_filters tests +// compute_bulkdata_source_filters tests // // Pin the entity-type branching that drives rosbag descriptor lookups + the // download ownership check. Crucial because synthetic / runtime-discovered // components have empty fqn AND empty namespace_path: without aggregation // from hosted apps the handler used to silently return zero source filters. +// +// Tested as a pure free function in detail:: against a directly-constructed +// ThreadSafeEntityCache so no GatewayNode / DDS context is needed. // ============================================================================= namespace { -class RclcppEnvironment : public ::testing::Environment { - public: - void SetUp() override { - if (!rclcpp::ok()) { - rclcpp::init(0, nullptr); - } - } - void TearDown() override { - rclcpp::shutdown(); - } -}; - -::testing::Environment * const rclcpp_env = ::testing::AddGlobalTestEnvironment(new RclcppEnvironment); - App make_test_app(const std::string & id, const std::string & node_name, const std::string & ns, const std::string & component_id) { App a; @@ -253,24 +238,7 @@ EntityInfo make_entity_info(EntityType type, const std::string & id, const std:: class BulkDataSourceFiltersTest : public ::testing::Test { protected: - static inline std::shared_ptr suite_node_; - - static void SetUpTestSuite() { - rclcpp::NodeOptions options; - options.append_parameter_override("server.host", std::string("127.0.0.1")); - options.append_parameter_override("server.port", 0); // disabled - options.append_parameter_override("refresh_interval_ms", 60000); - suite_node_ = std::make_shared(options); - ASSERT_NE(suite_node_, nullptr); - } - - static void TearDownTestSuite() { - suite_node_.reset(); - } - void SetUp() override { - ASSERT_NE(suite_node_, nullptr); - // Synthetic component: empty fqn AND empty namespace_path. Component synthetic; synthetic.id = "runtime_engine"; @@ -297,30 +265,17 @@ class BulkDataSourceFiltersTest : public ::testing::Test { empty_func.id = "empty_func"; empty_func.name = "Empty Function"; - auto & cache = const_cast(suite_node_->get_thread_safe_cache()); - cache.update_all({}, {synthetic, manifest_only}, {app1, app2}, {func, empty_func}); - - ctx_ = std::make_unique(suite_node_.get(), cors_, auth_, tls_, nullptr); - handlers_ = std::make_unique(*ctx_); - } - - void TearDown() override { - handlers_.reset(); - ctx_.reset(); + cache_.update_all({}, {synthetic, manifest_only}, {app1, app2}, {func, empty_func}); } - CorsConfig cors_{}; - AuthConfig auth_{}; - TlsConfig tls_{}; - std::unique_ptr ctx_; - std::unique_ptr handlers_; + ThreadSafeEntityCache cache_; }; // COMPONENT with hosted apps - returns app effective FQNs (synthetic component // has empty fqn / namespace_path; without aggregation this would be {}). TEST_F(BulkDataSourceFiltersTest, ComponentWithHostedAppsReturnsAppFqns) { auto entity = make_entity_info(EntityType::COMPONENT, "runtime_engine", "", ""); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 2u); std::set as_set(filters.begin(), filters.end()); EXPECT_TRUE(as_set.count("/powertrain/engine/temp_sensor")); @@ -331,7 +286,7 @@ TEST_F(BulkDataSourceFiltersTest, ComponentWithHostedAppsReturnsAppFqns) { // (manifest deployment grouping topics rather than nodes). TEST_F(BulkDataSourceFiltersTest, ComponentManifestOnlyFallsThroughToFqn) { auto entity = make_entity_info(EntityType::COMPONENT, "topics_group", "/topics/group", "/topics/group"); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 1u); EXPECT_EQ(filters[0], "/topics/group"); } @@ -340,7 +295,7 @@ TEST_F(BulkDataSourceFiltersTest, ComponentManifestOnlyFallsThroughToFqn) { // nothing to query. TEST_F(BulkDataSourceFiltersTest, ComponentSyntheticWithoutAppsReturnsEmpty) { auto entity = make_entity_info(EntityType::COMPONENT, "nonexistent_comp", "", ""); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); EXPECT_TRUE(filters.empty()); } @@ -349,7 +304,7 @@ TEST_F(BulkDataSourceFiltersTest, ComponentSyntheticWithoutAppsReturnsEmpty) { // non-empty - functions are pure aggregated views. TEST_F(BulkDataSourceFiltersTest, FunctionWithHostsReturnsAppFqns) { auto entity = make_entity_info(EntityType::FUNCTION, "powertrain_diag", "", ""); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 2u); std::set as_set(filters.begin(), filters.end()); EXPECT_TRUE(as_set.count("/powertrain/engine/temp_sensor")); @@ -361,7 +316,7 @@ TEST_F(BulkDataSourceFiltersTest, FunctionWithHostsReturnsAppFqns) { // after the COMPONENT/FUNCTION split). TEST_F(BulkDataSourceFiltersTest, FunctionWithoutHostsReturnsEmptyEvenIfFqnSet) { auto entity = make_entity_info(EntityType::FUNCTION, "empty_func", "/some/ns", "/some/fqn"); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); EXPECT_TRUE(filters.empty()); } @@ -369,7 +324,7 @@ TEST_F(BulkDataSourceFiltersTest, FunctionWithoutHostsReturnsEmptyEvenIfFqnSet) TEST_F(BulkDataSourceFiltersTest, AppReturnsSingleFqnFilter) { auto entity = make_entity_info(EntityType::APP, "temp_sensor", "/powertrain/engine", "/powertrain/engine/temp_sensor"); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 1u); EXPECT_EQ(filters[0], "/powertrain/engine/temp_sensor"); } @@ -377,7 +332,7 @@ TEST_F(BulkDataSourceFiltersTest, AppReturnsSingleFqnFilter) { // APP without fqn falls through to namespace_path filter. TEST_F(BulkDataSourceFiltersTest, AppWithEmptyFqnFallsThroughToNamespacePath) { auto entity = make_entity_info(EntityType::APP, "some_app", "/the/namespace", ""); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 1u); EXPECT_EQ(filters[0], "/the/namespace"); } @@ -385,14 +340,14 @@ TEST_F(BulkDataSourceFiltersTest, AppWithEmptyFqnFallsThroughToNamespacePath) { // APP with neither fqn nor namespace_path returns empty. TEST_F(BulkDataSourceFiltersTest, AppWithEmptyFqnAndNamespaceReturnsEmpty) { auto entity = make_entity_info(EntityType::APP, "some_app", "", ""); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); EXPECT_TRUE(filters.empty()); } // AREA entity uses fqn directly - no aggregation in this helper. TEST_F(BulkDataSourceFiltersTest, AreaReturnsFqnAsFilter) { auto entity = make_entity_info(EntityType::AREA, "powertrain", "/powertrain", "/powertrain"); - auto filters = handlers_->get_source_filters(entity); + auto filters = handlers::detail::compute_bulkdata_source_filters(cache_, entity); ASSERT_EQ(filters.size(), 1u); EXPECT_EQ(filters[0], "/powertrain"); } From 793edfa1881f3a2c3eaffdcbd31af8e94efff1af Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Apr 2026 14:10:05 +0200 Subject: [PATCH 5/8] ci: skip ament_cmake_clang_tidy/format keys, drop rolling continue-on-error The rosdep keys for ``ament_cmake_clang_tidy`` and ``ament_cmake_clang_format`` are not registered for noble (the Rolling runner image), so ``rosdep install`` hard-fails before the build can start. Linters / clang-tidy already run only in the Jazzy job in ``quality.yml`` (gated by ``ENABLE_CLANG_TIDY`` in ``ROS2MedkitLinting.cmake``), so the build-and-test matrix on Humble + Rolling does not need those CMake packages installed - skip the keys instead of trying to resolve them. ``opcua-plugin.yml`` already follows the same pattern. Drop ``continue-on-error`` for the Rolling job. Rolling is now treated as a required-green distro per CLAUDE.md - any Rolling-specific failure is a real breakage that needs a code-side fix or an explicit owner sign-off, not a silent soft-fail. --- .github/workflows/ci.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5cc25ea68..ca4c7ad4b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -17,7 +17,6 @@ jobs: os_image: ubuntu:jammy - ros_distro: rolling os_image: ubuntu:noble - continue-on-error: ${{ matrix.ros_distro == 'rolling' }} container: image: ${{ matrix.os_image }} timeout-minutes: 60 @@ -59,7 +58,13 @@ jobs: fi source /opt/ros/${{ matrix.ros_distro }}/setup.bash rosdep update - rosdep install --from-paths src --ignore-src -y + # Linters (clang-tidy / clang-format) are gated to the Jazzy lint job + # in quality.yml, but multiple package.xml files still list them as + # . Their rosdep keys are not registered for noble + # (Rolling), which breaks rosdep install on that runner. Skip them + # on every distro this workflow targets - the linters never run here. + rosdep install --from-paths src --ignore-src -y \ + --skip-keys "ament_cmake_clang_tidy ament_cmake_clang_format" - name: Build packages env: From c332a1995c74065a274d687fb9c827db2750b204 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Apr 2026 14:47:20 +0200 Subject: [PATCH 6/8] build(cmake): gate ament_cmake_clang_format find_package by FOUND check ci.yml skips the ``ament_cmake_clang_format`` rosdep key on Humble + Rolling (linters only run in the Jazzy quality job, the rosdep key isn't even registered for noble), so ``find_package(... REQUIRED)`` hard-fails the configure step on those runners. Switch all 8 affected packages to ``find_package(ament_cmake_clang_format QUIET)`` plus an ``if(_FOUND)`` guard around the corresponding ``ament_clang_format(...)`` call. On Jazzy (where the package is installed in the quality job's rosdep step) the behavior is unchanged - clang-format still runs and passes 310 tests on ``ros2_medkit_gateway``. On Humble + Rolling the configure step now proceeds without the package and the linter is silently skipped, which matches the project policy that linters only run on Jazzy. The companion ``ament_cmake_clang_tidy`` find_package in ``ROS2MedkitLinting.cmake`` is already gated by the ``ENABLE_CLANG_TIDY`` option (default OFF; only set ON in the Jazzy clang-tidy job in quality.yml), so no change needed there. --- .../ros2_medkit_beacon_common/CMakeLists.txt | 17 +++++--- .../ros2_medkit_param_beacon/CMakeLists.txt | 17 +++++--- .../ros2_medkit_topic_beacon/CMakeLists.txt | 17 +++++--- src/ros2_medkit_gateway/CMakeLists.txt | 28 ++++++++----- .../ros2_medkit_graph_provider/CMakeLists.txt | 17 +++++--- .../ros2_medkit_opcua/CMakeLists.txt | 17 +++++--- .../CMakeLists.txt | 17 +++++--- src/ros2_medkit_serialization/CMakeLists.txt | 42 +++++++++++-------- 8 files changed, 107 insertions(+), 65 deletions(-) diff --git a/src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt b/src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt index 2668e741f..b30e4190a 100644 --- a/src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt +++ b/src/ros2_medkit_discovery_plugins/ros2_medkit_beacon_common/CMakeLists.txt @@ -79,12 +79,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gtest REQUIRED) diff --git a/src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt b/src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt index 69c2db9ad..556edce5f 100644 --- a/src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt +++ b/src/ros2_medkit_discovery_plugins/ros2_medkit_param_beacon/CMakeLists.txt @@ -65,12 +65,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gmock REQUIRED) diff --git a/src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt b/src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt index a08de3876..9be4e4c21 100644 --- a/src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt +++ b/src/ros2_medkit_discovery_plugins/ros2_medkit_topic_beacon/CMakeLists.txt @@ -70,12 +70,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gtest REQUIRED) diff --git a/src/ros2_medkit_gateway/CMakeLists.txt b/src/ros2_medkit_gateway/CMakeLists.txt index ccfbedb2d..2de8ef76a 100644 --- a/src/ros2_medkit_gateway/CMakeLists.txt +++ b/src/ros2_medkit_gateway/CMakeLists.txt @@ -327,17 +327,23 @@ if(BUILD_TESTING) ) ament_copyright(EXCLUDE ${VENDORED_FILES}) - # Configure clang-format to only check our source files (not vendored) - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.h" "include/*.hpp" - "src/*.cpp" "src/*.h" "src/*.hpp" - "test/*.cpp" "test/*.h" "test/*.hpp" - ) - list(FILTER _format_files EXCLUDE REGEX ".*/vendored/.*") - ament_clang_format(${_format_files} - CONFIG_FILE "${ament_cmake_clang_format_CONFIG_FILE}" - ) + # Configure clang-format to only check our source files (not vendored). + # ament_cmake_clang_format is intentionally only installed in the Jazzy + # quality job (ci.yml skips its rosdep key on Humble + Rolling), so use + # QUIET + FOUND check instead of REQUIRED to gracefully degrade on the + # build-and-test runners without breaking the build. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.h" "include/*.hpp" + "src/*.cpp" "src/*.h" "src/*.hpp" + "test/*.cpp" "test/*.h" "test/*.hpp" + ) + list(FILTER _format_files EXCLUDE REGEX ".*/vendored/.*") + ament_clang_format(${_format_files} + CONFIG_FILE "${ament_cmake_clang_format_CONFIG_FILE}" + ) + endif() ros2_medkit_clang_tidy( HEADER_FILTER "^${CMAKE_CURRENT_SOURCE_DIR}/(include|src|test)/" diff --git a/src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt index 7129048e5..53381d377 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_graph_provider/CMakeLists.txt @@ -73,12 +73,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gtest REQUIRED) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt index 92b4e2bf7..5d9f7eda5 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt @@ -133,12 +133,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "include/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "include/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gtest REQUIRED) diff --git a/src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/CMakeLists.txt index df09dc63d..da43c2bac 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_sovd_service_interface/CMakeLists.txt @@ -72,12 +72,17 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - find_package(ament_cmake_clang_format REQUIRED) - file(GLOB_RECURSE _format_files - "src/*.hpp" "src/*.cpp" "test/*.cpp" - ) - ament_clang_format(${_format_files} - CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + # ci.yml skips the ament_cmake_clang_format rosdep key on Humble + Rolling + # (linters only run in the Jazzy quality job), so QUIET + FOUND check + # avoids hard-failing on those runners. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + file(GLOB_RECURSE _format_files + "src/*.hpp" "src/*.cpp" "test/*.cpp" + ) + ament_clang_format(${_format_files} + CONFIG_FILE "${CMAKE_CURRENT_SOURCE_DIR}/../../../.clang-format") + endif() find_package(ament_cmake_gtest REQUIRED) diff --git a/src/ros2_medkit_serialization/CMakeLists.txt b/src/ros2_medkit_serialization/CMakeLists.txt index 133267077..5b2c4c7dd 100644 --- a/src/ros2_medkit_serialization/CMakeLists.txt +++ b/src/ros2_medkit_serialization/CMakeLists.txt @@ -135,24 +135,30 @@ if(BUILD_TESTING) ) ament_lint_auto_find_test_dependencies() - # Configure clang-format manually for non-vendored files only - find_package(ament_cmake_clang_format REQUIRED) - set(_clang_format_config "${CMAKE_CURRENT_SOURCE_DIR}/../../.clang-format") - ament_clang_format( - CONFIG_FILE "${_clang_format_config}" - "include/ros2_medkit_serialization/json_serializer.hpp" - "include/ros2_medkit_serialization/message_cleanup.hpp" - "include/ros2_medkit_serialization/serialization_error.hpp" - "include/ros2_medkit_serialization/service_action_types.hpp" - "include/ros2_medkit_serialization/type_cache.hpp" - "src/json_serializer.cpp" - "src/message_cleanup.cpp" - "src/service_action_types.cpp" - "src/type_cache.cpp" - "test/test_json_serializer.cpp" - "test/test_service_action_types.cpp" - "test/test_type_cache.cpp" - ) + # Configure clang-format manually for non-vendored files only. + # ament_cmake_clang_format is intentionally only installed in the Jazzy + # quality job (ci.yml skips its rosdep key on Humble + Rolling), so use + # QUIET + FOUND check instead of REQUIRED to gracefully degrade on the + # build-and-test runners without breaking the build. + find_package(ament_cmake_clang_format QUIET) + if(ament_cmake_clang_format_FOUND) + set(_clang_format_config "${CMAKE_CURRENT_SOURCE_DIR}/../../.clang-format") + ament_clang_format( + CONFIG_FILE "${_clang_format_config}" + "include/ros2_medkit_serialization/json_serializer.hpp" + "include/ros2_medkit_serialization/message_cleanup.hpp" + "include/ros2_medkit_serialization/serialization_error.hpp" + "include/ros2_medkit_serialization/service_action_types.hpp" + "include/ros2_medkit_serialization/type_cache.hpp" + "src/json_serializer.cpp" + "src/message_cleanup.cpp" + "src/service_action_types.cpp" + "src/type_cache.cpp" + "test/test_json_serializer.cpp" + "test/test_service_action_types.cpp" + "test/test_type_cache.cpp" + ) + endif() ros2_medkit_clang_tidy( HEADER_FILTER "^${CMAKE_CURRENT_SOURCE_DIR}/include/ros2_medkit_serialization/[^v].*" From e278fcdde649dcac0656c19dac38bbcba1d42de1 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Apr 2026 15:49:27 +0200 Subject: [PATCH 7/8] ci(opcua): skip clang_format/clang_tidy rosdep keys for cross-distro test job The opcua-plugin Unit tests matrix runs on humble + jazzy + rolling. On noble (Rolling) the ament_cmake_clang_format / ament_cmake_clang_tidy rosdep keys are not registered, so the previous skip-keys list (which covered only nav2_msgs) caused rosdep install to fail before any test could run. Add the linter keys to skip-keys, mirroring the change just made in ci.yml. The upstream medkit packages have already been switched to QUIET + FOUND for those find_package calls so the build degrades gracefully when the linter packages aren't installed. --- .github/workflows/opcua-plugin.yml | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/.github/workflows/opcua-plugin.yml b/.github/workflows/opcua-plugin.yml index 5c21bc608..9c321ad99 100644 --- a/.github/workflows/opcua-plugin.yml +++ b/.github/workflows/opcua-plugin.yml @@ -83,13 +83,14 @@ jobs: apt-get install -y ros-${{ matrix.ros_distro }}-test-msgs libyaml-cpp-dev libssl-dev source /opt/ros/${{ matrix.ros_distro }}/setup.bash rosdep update - # Only skip nav2_msgs because vda5050_agent declares it as a dep but - # the apt package is not available on all distros. We do NOT skip - # ament_cmake_clang_format or test_msgs here: upstream medkit - # packages (ros2_medkit_serialization, gateway) require them at - # configure time via find_package. + # Skip nav2_msgs (vda5050_agent declares it; not available on all + # distros) and the linter rosdep keys. The clang_format / clang_tidy + # keys are not registered for noble (Rolling), and the upstream + # medkit packages have already been switched to QUIET + FOUND for + # those find_package calls so the build degrades gracefully without + # the linter packages installed. rosdep install --from-paths src --ignore-src -y \ - --skip-keys='nav2_msgs' + --skip-keys='nav2_msgs ament_cmake_clang_format ament_cmake_clang_tidy' - name: Build ros2_medkit_opcua (and upstream deps) env: From 245123d447c54401d5e99c41e33ea3192eabc5f9 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Wed, 29 Apr 2026 16:17:06 +0200 Subject: [PATCH 8/8] fix(opcua/docker): rerun apt-get update before rosdep install The first RUN clears ``/var/lib/apt/lists/*`` to keep the builder image small. The next RUN then calls ``rosdep install``, which in turn invokes ``apt-get install`` under the hood without first refreshing the apt cache. On cold Docker builds this fails with ``E: Unable to locate package python3-jsonschema`` (declared as a ```` by ``ros2_medkit_integration_tests`` and pulled in by rosdep even with ``BUILD_TESTING=OFF``). The OPC-UA plugin Integration jobs were masking this on main via warm BuildKit layer caches and only blew up after the cache was evicted. Add an explicit ``apt-get update`` at the start of the rosdep RUN, and move the ``rm -rf /var/lib/apt/lists/*`` cleanup to the end of the same layer so the runtime image still inherits no apt cache. --- .../ros2_medkit_opcua/docker/Dockerfile.gateway | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway index 2c3a620c4..bb8d68a78 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway @@ -20,10 +20,21 @@ WORKDIR ${COLCON_WS} # Skip VDA 5050 packages in base build: they pull in nav2_msgs which is not # yet available as a jazzy apt package. The OPC-UA plugin has no dependency # on VDA 5050, so this does not affect our target. +# +# apt-get update has to be re-run here: the previous RUN cleared +# /var/lib/apt/lists, so without a fresh fetch rosdep's internal +# apt-get install calls fail with "Unable to locate package" for anything +# that wasn't pulled in by the first RUN's apt-get install (notably +# python3-jsonschema, which the integration_tests package declares as a +# test_depend and which rosdep tries to install even with BUILD_TESTING=OFF). +# This was previously masked by Docker layer cache hits on CI; cold builds +# always failed. RUN bash -c "source /opt/ros/jazzy/setup.bash && \ + apt-get update && \ rosdep update && \ rosdep install --from-paths src --ignore-src -r -y \ --skip-keys='ament_cmake_clang_format ament_cmake_clang_tidy test_msgs sqlite3 ros2_medkit_graph_provider python3-requests launch_testing_ament_cmake launch_testing launch_ros ament_index_python ros2_medkit_param_beacon nav2_msgs' && \ + rm -rf /var/lib/apt/lists/* && \ colcon build --cmake-args -DBUILD_TESTING=OFF \ --packages-skip vda5050_agent ros2_medkit_vda5050_msgs ros2_medkit_opcua"