[#142] Consolidate discovery handlers into unified class #143

bburda · 2026-01-25T21:38:43Z

Data endpoints unified across all entity types via DataHandlers.

Pull Request

Summary

Create unified DiscoveryHandlers class with entity-agnostic methods
Create unified DataHandlers class for data operations across entities
Add AggregatedData and TopicData structs to ThreadSafeEntityCache
Remove handlers/discovery/ folder (8 files, ~1600 lines)
Update rest_server to use discovery_handlers_ and data_handlers_
~55% code reduction through handler consolidation

Discovery endpoints now handled by single class:

Areas: list, get, components, subareas, contains
Components: list, get, subcomponents, hosts, depends-on
Apps: list, get, depends-on
Functions: list, get, hosts

Issue

Link the related issue (required):

closes Consolidate discovery handlers into a single unified DiscoveryHandlers #142

Type

Bug fix
New feature or tests
Breaking change
Documentation only

Testing

How was this tested / how should reviewers verify it?

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

- Create unified DiscoveryHandlers class with entity-agnostic methods - Create unified DataHandlers class for data operations across entities - Add AggregatedData and TopicData structs to ThreadSafeEntityCache - Remove handlers/discovery/ folder (8 files, ~1600 lines) - Update rest_server to use discovery_handlers_ and data_handlers_ - ~55% code reduction through handler consolidation Discovery endpoints now handled by single class: - Areas: list, get, components, subareas, contains - Components: list, get, subcomponents, hosts, depends-on - Apps: list, get, depends-on - Functions: list, get, hosts Data endpoints unified across all entity types via DataHandlers.

Copilot

Pull request overview

Consolidates entity discovery and data endpoints by replacing per-entity handler classes with unified DiscoveryHandlers and DataHandlers, and adds cache-level data aggregation to support entity-agnostic /data operations across areas/components/apps/functions.

Changes:

Replaced per-entity discovery handlers with a unified DiscoveryHandlers.
Introduced unified DataHandlers plus new ThreadSafeEntityCache data aggregation APIs/structs to support /data across entity types.
Updated REST routing and build configuration; removed legacy handlers/discovery/* handler files.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp	Adds data aggregation implementation used by unified data endpoints.
src/ros2_medkit_gateway/src/http/rest_server.cpp	Rewires routes to unified handlers; adds new area/function data routes.
src/ros2_medkit_gateway/src/http/handlers/discovery_handlers.cpp	New consolidated discovery endpoint implementations.
src/ros2_medkit_gateway/src/http/handlers/discovery/function_handlers.cpp	Removed legacy function discovery handler implementation.
src/ros2_medkit_gateway/src/http/handlers/discovery/component_handlers.cpp	Removed legacy component discovery/data handler implementation.
src/ros2_medkit_gateway/src/http/handlers/discovery/area_handlers.cpp	Removed legacy area discovery handler implementation.
src/ros2_medkit_gateway/src/http/handlers/discovery/app_handlers.cpp	Removed legacy app discovery/data handler implementation.
src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp	New consolidated `/data` endpoint implementations across entity types.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/models/thread_safe_entity_cache.hpp	Adds `AggregatedData`/`TopicData` and new data aggregation APIs.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/rest_server.hpp	Updates handler member fields to unified handler classes.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handlers.hpp	Updates umbrella handler includes to unified discovery/data handlers.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery_handlers.hpp	New public header for consolidated discovery handlers.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/function_handlers.hpp	Removed legacy function handler header.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/component_handlers.hpp	Removed legacy component handler header.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/area_handlers.hpp	Removed legacy area handler header.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery/app_handlers.hpp	Removed legacy app handler header.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/data_handlers.hpp	New public header for consolidated data handlers.
src/ros2_medkit_gateway/CMakeLists.txt	Removes legacy discovery handler sources; adds new unified handler sources.

Copilot · 2026-01-25T21:52:08Z

src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp

+    // Build full topic path
+    std::string full_topic_path = "/" + topic_name;


handle_put_data_item() unconditionally builds full_topic_path as "/" + topic_name. If the client supplies an already-absolute topic (e.g. URL-encoded "%2Ffoo" -> "/foo"), this produces "//foo". Mirror the GET logic: only prefix '/' when topic_name is non-empty and does not already start with '/'.

Suggested change

// Build full topic path

std::string full_topic_path = "/" + topic_name;

// Build full topic path (mirror GET logic: only prefix '/' when needed)

std::string full_topic_path = topic_name;

if (!full_topic_path.empty() && full_topic_path.front() != '/') {

full_topic_path = "/" + full_topic_path;

}

Copilot · 2026-01-25T21:52:09Z

src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp

+
+#include "ros2_medkit_gateway/http/handlers/data_handlers.hpp"
+
+#include <map>


data_handlers.cpp uses std::count() (message type validation) but does not include . With strict builds this can fail to compile. Add the missing include (or remove the std::count usage).

Suggested change

#include <map>

#include <map>

#include <algorithm>

Copilot · 2026-01-25T21:52:09Z

src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp

+    if (seen_topics.insert(topic).second) {
+      result.topics.push_back({topic, "", "publish"});


collect_topics_from_component(): same issue as app aggregation—if a topic is first seen as a subscriber and later encountered as a publisher, the existing entry is never updated to "both". Handle the already-seen case in the publisher loop to keep direction accurate.

Suggested change

if (seen_topics.insert(topic).second) {

result.topics.push_back({topic, "", "publish"});

auto [_, inserted] = seen_topics.insert(topic);

if (inserted) {

result.topics.push_back({topic, "", "publish"});

} else {

// Topic already seen - might be both pub and sub, update direction

for (auto & t : result.topics) {

if (t.name == topic && t.direction == "subscribe") {

t.direction = "both";

break;

}

}

Copilot · 2026-01-25T21:52:09Z

src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp

+
+  AggregatedData result;
+  result.aggregation_level = "area";
+  result.is_aggregated = true;


get_area_data() (and similarly get_function_data()) never adds the requested entity’s own ID to result.source_ids. If an area/function exists but has zero contributing topics (e.g., no components/apps), result.source_ids stays empty and the HTTP layer currently returns 404. Add area_id/function_id to source_ids when the entity exists so callers can distinguish "exists but empty" from "not found".

Suggested change

result.is_aggregated = true;

result.is_aggregated = true;

result.source_ids.push_back(area_id);

Copilot · 2026-01-25T21:52:10Z

src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/discovery_handlers.hpp

+#ifndef ROS2_MEDKIT_GATEWAY__HTTP__HANDLERS__DISCOVERY_HANDLERS_HPP_
+#define ROS2_MEDKIT_GATEWAY__HTTP__HANDLERS__DISCOVERY_HANDLERS_HPP_
+
+#include "ros2_medkit_gateway/http/handlers/handler_context.hpp"


This header uses a traditional #ifndef include guard, but the rest of the codebase consistently uses #pragma once (e.g., include/ros2_medkit_gateway/http/rest_server.hpp:15). Switch to #pragma once to match project conventions.

Copilot · 2026-01-25T21:52:10Z

src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp

+    // Determine the full ROS topic path
+    std::string full_topic_path;
+    if (topic_name.empty() || topic_name[0] == '/') {
+      full_topic_path = topic_name;
+    } else {


handle_get_data_item() samples the requested topic after validating entity_id, but it never verifies that the topic is actually associated with that entity’s /data collection. This allows reading arbitrary topics via any valid entity endpoint (and similarly affects PUT). Consider validating membership against the entity’s aggregated topic list and returning 404 when not associated.

Copilot · 2026-01-25T21:52:11Z

src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp

+  for (const auto & topic : app.topics.publishes) {
+    if (seen_topics.insert(topic).second) {
+      result.topics.push_back({topic, "", "publish"});
+    }
+  }


collect_topics_from_app(): if a topic is first seen as a subscriber topic and later encountered as a publisher topic (from another app), the publisher loop currently drops it without updating the existing entry’s direction to "both". This produces incorrect direction metadata in aggregated responses. Handle the already-seen case in the publisher loop.

Copilot · 2026-01-25T21:52:11Z

src/ros2_medkit_gateway/src/http/rest_server.cpp

+  // Area data item (specific topic) - register before /areas/{id}/data
+  srv->Get((api_path("/areas") + R"(/([^/]+)/data/(.+)$)"),
+           [this](const httplib::Request & req, httplib::Response & res) {
+             data_handlers_->handle_get_data_item(req, res);
+           });


New /areas/{id}/data (GET/PUT/item) routes are introduced here, but there are no corresponding integration tests covering area data endpoints. Extend the existing Python launch/integration tests to exercise these routes (including empty-data vs 404 behavior).

Copilot · 2026-01-25T21:52:11Z

src/ros2_medkit_gateway/src/http/rest_server.cpp

+  // Function data item (specific topic) - register before /functions/{id}/data
+  srv->Get((api_path("/functions") + R"(/([^/]+)/data/(.+)$)"),
+           [this](const httplib::Request & req, httplib::Response & res) {
+             data_handlers_->handle_get_data_item(req, res);
+           });


Function-specific /functions/{id}/data/{topic} (GET/PUT) routes are added here, but the current integration tests primarily cover function /data listing (and app/component data item/PUT). Add integration tests that exercise function data item read/write behavior and verify the response/error format.

Copilot · 2026-01-25T21:52:11Z

src/ros2_medkit_gateway/src/models/thread_safe_entity_cache.cpp

+  AggregatedData result;
+  result.aggregation_level = "function";
+  result.is_aggregated = true;
+
+  std::unordered_set<std::string> seen_topics;


get_function_data() does not add function_id to result.source_ids when the function exists. If a function has no host apps (or no topics), source_ids stays empty and the HTTP layer currently returns 404 even though the function exists. Add function_id to source_ids (and consider setting is_aggregated based on whether any hosts contributed).

bburda requested a review from mfaferek93 January 25, 2026 21:38

bburda self-assigned this Jan 25, 2026

Copilot AI review requested due to automatic review settings January 25, 2026 21:38

Copilot started reviewing on behalf of bburda January 25, 2026 21:38 View session

Copilot AI reviewed Jan 25, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#142] Consolidate discovery handlers into unified class #143

[#142] Consolidate discovery handlers into unified class #143

bburda commented Jan 25, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Copilot AI Jan 25, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Build full topic path
		std::string full_topic_path = "/" + topic_name;

-    // Build full topic path
-    std::string full_topic_path = "/" + topic_name;
+    // Build full topic path (mirror GET logic: only prefix '/' when needed)
+    std::string full_topic_path = topic_name;
+    if (!full_topic_path.empty() && full_topic_path.front() != '/') {
+      full_topic_path = "/" + full_topic_path;
+    }


		#include "ros2_medkit_gateway/http/handlers/data_handlers.hpp"

		#include <map>

		if (seen_topics.insert(topic).second) {
		result.topics.push_back({topic, "", "publish"});

-    if (seen_topics.insert(topic).second) {
-      result.topics.push_back({topic, "", "publish"});
+    auto [_, inserted] = seen_topics.insert(topic);
+    if (inserted) {
+      result.topics.push_back({topic, "", "publish"});
+    } else {
+      // Topic already seen - might be both pub and sub, update direction
+      for (auto & t : result.topics) {
+        if (t.name == topic && t.direction == "subscribe") {
+          t.direction = "both";
+          break;
+        }
+      }

	result.is_aggregated = true;
	result.is_aggregated = true;
	result.source_ids.push_back(area_id);

[#142] Consolidate discovery handlers into unified class #143

Are you sure you want to change the base?

[#142] Consolidate discovery handlers into unified class #143

Conversation

bburda commented Jan 25, 2026

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!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants