Support all SOVD resource collections in cyclic subscriptions by bburda · Pull Request #254 · selfpatch/ros2_medkit

bburda · 2026-03-05T20:31:18Z

Summary

Make cyclic subscriptions work with any SOVD resource collection (data, faults, configurations, communication-logs) and vendor extensions (x-* prefix), instead of being hard-coded to /data/ only.

Key changes:

ResourceSamplerRegistry - thread-safe registry mapping collection names to sampler functions. Built-in samplers (data, faults, configurations, communication-logs) use last-write-wins; plugins must use x- prefix
SubscriptionTransportProvider / TransportRegistry - pluggable transport interface (SSE built-in, extensible to MQTT/WebSocket/Zenoh) with protocol-keyed registry
SseTransportProvider - SSE streaming logic extracted from handler into a standalone transport provider with exception-safe sampler invocation
Generalized parse_resource_uri() - extracts entity type, entity ID, collection, and resource path from any valid SOVD resource URI with path traversal protection
CyclicSubscriptionInfo - replaced topic_name with collection + resource_path fields
on_removed callback - SubscriptionManager lifecycle hook for transport cleanup on remove/expire/shutdown
Proper shutdown ordering - REST stop -> transport stop -> sub_mgr shutdown -> plugin shutdown -> destruction
PluginManager extensions - register_resource_sampler() and register_transport() for plugin-provided collections and transports

Issue

closes cyclic-subscriptions/ should support all SOVD resource collections and vendor extensions #253

Type

Bug fix
New feature or tests
Breaking change
Documentation only

Testing

4 new unit test files (22 new test cases):
- test_resource_sampler_registry.cpp - 7 tests (register, lookup, builtin overwrite, plugin reject, concurrent access)
- test_transport_registry.cpp - 3 tests (register/lookup, missing transport, shutdown_all)
- test_cyclic_subscription_handlers.cpp - 11 new ParseResourceUri tests (all collections, vendor extensions, entity types, path traversal, invalid URIs)
- test_subscription_manager.cpp - 3 new on_removed callback tests (remove, expire, shutdown)
1 new integration test file:
- test_multi_collection_subscriptions.test.py - 8 tests (data/faults/configurations create, unsupported collection, invalid URI, entity mismatch, unsupported protocol, path traversal)
All 870 unit tests pass, all linters pass (clang-format, clang-tidy, ament)

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

Copilot

Pull request overview

Adds generalized cyclic subscription support across all SOVD resource collections (plus x-* vendor extensions) by introducing registries for resource samplers and transport providers, and refactoring SSE streaming into a dedicated transport provider.

Changes:

Introduces ResourceSamplerRegistry (thread-safe) and TransportRegistry (protocol-keyed) with plugin registration hooks.
Refactors cyclic subscription creation to accept {collection, resource_path} from generalized parse_resource_uri() (with traversal protection) and to start delivery via transport providers.
Adds unit + integration tests covering registries, URI parsing, on-removed lifecycle hook, and multi-collection subscription flows.

Reviewed changes

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

Show a summary per file

File	Description
src/ros2_medkit_integration_tests/test/features/test_multi_collection_subscriptions.test.py	New integration tests validating create/error cases across multiple collections and protocols.
src/ros2_medkit_gateway/test/test_transport_registry.cpp	New unit tests for transport registry lookup and shutdown behavior.
src/ros2_medkit_gateway/test/test_subscription_manager.cpp	Updates tests for new create() signature; adds on_removed callback tests.
src/ros2_medkit_gateway/test/test_resource_sampler_registry.cpp	New unit tests for sampler registration rules and concurrency.
src/ros2_medkit_gateway/test/test_cyclic_subscription_handlers.cpp	Adds parse_resource_uri test coverage for multiple collections and traversal rejection.
src/ros2_medkit_gateway/src/subscription_transport.cpp	Implements basic transport registry and shutdown_all delegating to SubscriptionManager.
src/ros2_medkit_gateway/src/subscription_manager.cpp	Extends subscription info fields; adds on_removed hook; updates remove/expiry/shutdown lifecycle.
src/ros2_medkit_gateway/src/resource_sampler.cpp	Implements thread-safe sampler registry with builtin/plugin registration policies.
src/ros2_medkit_gateway/src/plugins/plugin_manager.cpp	Adds plugin APIs to register samplers/transports and wires registries into PluginManager.
src/ros2_medkit_gateway/src/http/rest_server.cpp	Injects sampler/transport registries into cyclic subscription handlers; uses node-provided SSE tracker.
src/ros2_medkit_gateway/src/http/handlers/sse_transport_provider.cpp	New SSE transport provider extracting former handler streaming logic and invoking samplers safely.
src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp	Generalizes resource URI parsing; validates collection/protocol; starts delivery via transport provider.
src/ros2_medkit_gateway/src/gateway_node.cpp	Initializes registries, built-in samplers/transports, lifecycle wiring, and shutdown ordering.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/subscription_transport.hpp	New transport provider interface and registry.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/subscription_manager.hpp	Updates CyclicSubscriptionInfo; new create() signature; adds on_removed callback API.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/resource_sampler.hpp	New sampler function type + thread-safe sampler registry interface.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/plugins/plugin_manager.hpp	Exposes plugin APIs for sampler/transport registration + registry wiring.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/sse_transport_provider.hpp	Declares SSE transport provider implementation.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/cyclic_subscription_handlers.hpp	Adds ParsedResourceUri and parse_resource_uri() API; injects registries.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/error_codes.hpp	Adds vendor-specific error codes for URI/collection/protocol validation.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/gateway_node.hpp	Exposes registries and shared SSE client tracker from GatewayNode.
src/ros2_medkit_gateway/CMakeLists.txt	Adds new sources and unit tests to build/test targets.

src/ros2_medkit_gateway/src/resource_sampler.cpp

src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp

src/ros2_medkit_gateway/include/ros2_medkit_gateway/subscription_manager.hpp

src/ros2_medkit_gateway/src/http/handlers/sse_transport_provider.cpp

src/ros2_medkit_gateway/src/subscription_transport.cpp

Copilot

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated 5 comments.

src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp

src/ros2_medkit_gateway/include/ros2_medkit_gateway/subscription_transport.hpp

src/ros2_medkit_gateway/src/subscription_transport.cpp

src/ros2_medkit_gateway/src/gateway_node.cpp

- Guard substr(0,2) against empty collection strings (Copilot #1) - Use segment-based path traversal check instead of substring matching, allowing benign paths like '/..foo' while still rejecting '/..' segments (Copilot #2,#7) - Add thread-safety to TransportRegistry via shared_mutex (Copilot #10) - Reject duplicate transport protocol registration (Copilot #6) - Validate data collection requires resource_path (topic name) (Copilot #8) - Document set_on_removed() init-only constraint for thread safety (Copilot #3,#4) - Explain SSE stop() behavior in comments (Copilot #5) - Remove redundant subscription_mgr_->shutdown() call in destructor (Copilot #11) - Fix shutdown_all() docstring to match delegation pattern (Copilot #9) - Fix clang-tidy: use const ref for captured_sub_id (CI failure)

Copilot

Pull request overview

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

docs/tutorials/plugin-system.rst

src/ros2_medkit_gateway/src/subscription_manager.cpp

src/ros2_medkit_gateway/src/http/handlers/cyclic_subscription_handlers.cpp

...s2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/cyclic_subscription_handlers.hpp

src/ros2_medkit_gateway/src/gateway_node.cpp

RuntimeLayer filtered areas and components by namespace blacklist but not apps - nodes in blacklisted namespaces still appeared as gap-fill apps. Extract namespace from bound_fqn and filter accordingly.

mfaferek93

Looks good, only a few comments

src/ros2_medkit_gateway/src/gateway_node.cpp

src/ros2_medkit_gateway/src/http/handlers/sse_transport_provider.cpp

src/ros2_medkit_gateway/include/ros2_medkit_gateway/models/entity_types.hpp

src/ros2_medkit_gateway/src/gateway_node.cpp

- Add null guard for native_sampler in data collection sampler - Add TOCTOU guard in fault sampler when entity vanishes between lookups - Add bounds validation for sse.max_duration_sec parameter - Clarify stop() contract in SubscriptionTransportProvider docstring to document shutdown ordering dependency for HTTP-based transports

Copilot AI review requested due to automatic review settings March 5, 2026 20:31

bburda self-assigned this Mar 5, 2026

bburda added the enhancement New feature or request label Mar 5, 2026

bburda marked this pull request as draft March 5, 2026 20:31

bburda changed the title ~~feat(gateway): support all SOVD resource collections in cyclic subscriptions~~ Support all SOVD resource collections in cyclic subscriptions Mar 5, 2026

Copilot AI reviewed Mar 5, 2026

View reviewed changes

bburda requested a review from Copilot March 5, 2026 20:36

Copilot AI reviewed Mar 5, 2026

View reviewed changes

bburda force-pushed the feature/253-cyclic-subscriptions-all-collections branch from 07177d0 to 869d745 Compare March 6, 2026 20:44

bburda marked this pull request as ready for review March 7, 2026 07:25

bburda requested a review from Copilot March 7, 2026 07:25

Copilot started reviewing on behalf of bburda March 7, 2026 07:25 View session

Copilot AI reviewed Mar 7, 2026

View reviewed changes

bburda requested a review from mfaferek93 March 7, 2026 19:18

bburda added 5 commits March 7, 2026 20:26

fix(discovery): apply namespace blacklist to apps in gap-fill

257d25c

RuntimeLayer filtered areas and components by namespace blacklist but not apps - nodes in blacklisted namespaces still appeared as gap-fill apps. Extract namespace from bound_fqn and filter accordingly.

feat(gateway): add multi-collection cyclic subscription support

6cc0f0a

fix(gateway): harden cyclic subscriptions from review findings

d3d1f5f

feat(gateway): add logs collection cyclic subscription support

6f42296

feat(gateway): add cyclic subscription support for update status

be593c9

bburda force-pushed the feature/253-cyclic-subscriptions-all-collections branch from f781c62 to be593c9 Compare March 7, 2026 19:26

mfaferek93 reviewed Mar 7, 2026

View reviewed changes

mfaferek93 self-requested a review March 8, 2026 06:59

mfaferek93 approved these changes Mar 8, 2026

View reviewed changes

bburda merged commit 9d9aac0 into main Mar 8, 2026
9 checks passed

bburda deleted the feature/253-cyclic-subscriptions-all-collections branch March 8, 2026 09:46

Conversation

bburda commented Mar 5, 2026

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!

Uh oh!

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

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfaferek93 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mfaferek93 left a comment •

edited

Loading