[#139] fix(gateway): aggregate configurations for aggregate SOVD entities #150

bburda · 2026-01-27T16:35:37Z

Pull Request

Summary

Add configuration aggregation for Areas, Components, and Functions by collecting parameters from all child App nodes. Implements pattern used by data/operations endpoints. Parameter IDs use "app_id:param_name" format for disambiguation. Updates endpoint documentation and adds smoke test.

Issue

Link the related issue (required):

closes [BUG] Configuration endpoints return 503 for aggregate entities (Areas, Components, Functions) #149

Type

Bug fix
New feature or tests
Breaking change
Documentation only

Testing

How was this tested / how should reviewers verify it?
colcon test

Checklist

Breaking changes are clearly described (and announced in docs / changelog if needed)
Tests were added or updated if needed
Docs were updated if behavior or public API changed

Add configuration aggregation for Areas, Components, and Functions by collecting parameters from all child App nodes. Implements pattern used by data/operations endpoints. Parameter IDs use "app_id:param_name" format for disambiguation. Updates endpoint documentation and adds smoke test.

Copilot

Pull request overview

This PR fixes configuration handling for aggregate SOVD entities (Areas, Components, Functions) by aggregating parameters from child Apps, aligns the advertised root endpoints with the actual implementation, and adds integration tests (including a smoke test) to validate endpoint coverage.

Changes:

Introduces configuration aggregation in ThreadSafeEntityCache and updates ConfigHandlers to use an entity-agnostic aggregation API (with app_id:param_name IDs for disambiguation on aggregate entities).
Expands the root health endpoint’s advertised endpoint list and adds an integration smoke test that calls all advertised GET endpoints to ensure none return 5xx (except intentional 501).
Updates integration tests for manifest-only discovery mode to expect live configuration data for Apps with running demo nodes.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`src/ros2_medkit_gateway/test/test_integration.test.py`	Adjusts root endpoint expectation to use `{data_id}` and adds a smoke test that iterates over all advertised GET endpoints from `/`, performing path parameter substitution and asserting no 5xx responses.
`src/ros2_medkit_gateway/test/test_discovery_manifest.test.py`	Updates the manifest-mode app configuration test to expect a 200 response with `items` and `x-medkit` when the bound node is running, reflecting the new runtime configuration retrieval behavior.
`src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp`	Adds `NodeConfigInfo`-based configuration aggregation methods for Apps, Components, Areas, and Functions, returning the node FQNs and source IDs for parameter queries.
`src/ros2_medkit_gateway/include/ros2_medkit_gateway/models/thread_safe_entity_cache.hpp`	Declares the `NodeConfigInfo` and `AggregatedConfigurations` structs and exposes configuration aggregation APIs analogous to existing data/operations aggregation methods.
`src/ros2_medkit_gateway/src/http/handlers/health_handlers.cpp`	Expands the `/` root endpoint’s `endpoints` list to comprehensively document data, operations, configurations, and faults endpoints for Areas, Components, Apps, Functions, and global faults, matching the actual routes.
`src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp`	Refactors configuration handlers to work for any entity ID via `get_entity_configurations`, aggregate list results across multiple nodes with richer `x-medkit` metadata, support `app_id:param_name` identifiers on aggregate entities, and adjust error handling and multi-node reset semantics.

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp

- Add helper functions to eliminate code duplication: - parse_aggregated_param_id(): parses 'app_id:param_name' format - find_node_for_app(): O(n) lookup replacing duplicated loops - classify_parameter_error(): consistent error classification (404/403/503/400) - Fix error handling in aggregated entity GET without prefix: - Track errors across all nodes instead of silently ignoring failures - Return 404 only when all nodes report 'not found' - Return appropriate error (503/400) when nodes have other failures - Fix get_area_configurations: add missing app IDs to source_ids Addresses code review feedback on inconsistent error semantics.

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp

src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

- Add ParameterErrorCode enum for programmatic error handling - Parallelize list_configurations with std::async for large hierarchies - Fix is_aggregated: true only when multiple nodes exist - Unify entity lookup to cache.find_entity() pattern - Add send_parameter_error() helper and MAX_AGGREGATED_PARAM_ID_LENGTH constant

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp

Copilot · 2026-01-27T19:23:03Z

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp

+    // Query nodes in parallel for better performance with large hierarchies
+    // Each async task queries one node and returns its result
+    struct NodeQueryResult {
+      NodeConfigInfo node_info;
+      ParameterResult result;
+    };
+
+    std::vector<std::future<NodeQueryResult>> futures;
+    futures.reserve(agg_configs.nodes.size());
+
+    // Launch parallel queries
+    for (const auto & node_info : agg_configs.nodes) {
+      futures.push_back(std::async(std::launch::async, [config_mgr, node_info]() {
+        NodeQueryResult query_result;
+        query_result.node_info = node_info;
+        query_result.result = config_mgr->list_parameters(node_info.node_fqn);
+        return query_result;
+      }));
+    }


handle_list_configurations launches one std::async per node in agg_configs.nodes with std::launch::async and no upper bound (lines 265–283). In large deployments with many child apps per area/function, this can create an unbounded number of threads and put unnecessary pressure on the thread pool compared to the batched, max_parallel approach used in NativeTopicSampler::sample_topics_parallel. Consider introducing a configurable concurrency limit (e.g., process nodes in batches similar to topic sampling) to keep resource usage predictable while still benefiting from parallelism.

src/ros2_medkit_gateway/test/test_discovery_manifest.test.py

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp

bburda added this to the MS1: Full Discovery & Data Access milestone Jan 27, 2026

bburda self-assigned this Jan 27, 2026

Copilot AI review requested due to automatic review settings January 27, 2026 16:35

bburda added the bug Something isn't working label Jan 27, 2026

bburda requested a review from mfaferek93 January 27, 2026 16:35

Copilot started reviewing on behalf of bburda January 27, 2026 16:36 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Outdated Show resolved Hide resolved

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Show resolved Hide resolved

bburda requested a review from Copilot January 27, 2026 17:13

Copilot started reviewing on behalf of bburda January 27, 2026 17:14 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Outdated Show resolved Hide resolved

mfaferek93 reviewed Jan 27, 2026

View reviewed changes

Copilot AI reviewed Jan 27, 2026

View reviewed changes

bburda requested review from Copilot and mfaferek93 January 27, 2026 19:16

Copilot started reviewing on behalf of bburda January 27, 2026 19:16 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

mfaferek93 approved these changes Jan 27, 2026

View reviewed changes

bburda merged commit fd3e57c into main Jan 27, 2026
10 checks passed

bburda deleted the fix/configuration-endpoint branch January 27, 2026 21:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#139] fix(gateway): aggregate configurations for aggregate SOVD entities #150

[#139] fix(gateway): aggregate configurations for aggregate SOVD entities #150

bburda commented Jan 27, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI Jan 27, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[#139] fix(gateway): aggregate configurations for aggregate SOVD entities #150

[#139] fix(gateway): aggregate configurations for aggregate SOVD entities #150

Conversation

bburda commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request

Summary

Issue

Type

Testing

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 27, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bburda commented Jan 27, 2026 •

edited

Loading