fix(discovery): add input validation and improve test reliability

bburda · bburda · commit 4aeadc5326a2 · 2026-01-21T20:39:11.000+01:00
- Add empty area/pattern validation in apply_component_name_pattern()
  with WARN logging and safe fallbacks
- Add DEBUG logging for duplicate node detection in discover_node_components()
- Refactor config_handlers.cpp to use unified get_entity_info() lookup,
  reducing code duplication (~80 lines)
- Increase action test timeout to 30s and use shared ACTION_TIMEOUT constant
- Add documentation for expected entities in integration tests
diff --git a/src/ros2_medkit_gateway/src/discovery/runtime_discovery.cpp b/src/ros2_medkit_gateway/src/discovery/runtime_discovery.cpp
@@ -128,6 +128,7 @@ std::vector<Component> RuntimeDiscoveryStrategy::discover_node_components() {
 
     // Skip duplicate nodes - ROS 2 RMW may report same node multiple times
     if (seen_fqns.count(fqn) > 0) {
+      RCLCPP_DEBUG(node_->get_logger(), "Skipping duplicate node: %s", fqn.c_str());
       continue;
     }
     seen_fqns.insert(fqn);
@@ -611,6 +612,17 @@ std::string RuntimeDiscoveryStrategy::derive_component_id(const Component & node
 }
 
 std::string RuntimeDiscoveryStrategy::apply_component_name_pattern(const std::string & area) {
+  // Validate inputs to prevent unexpected empty component IDs
+  if (area.empty()) {
+    RCLCPP_WARN(node_->get_logger(), "apply_component_name_pattern called with empty area, using 'unknown'");
+    return apply_component_name_pattern("unknown");
+  }
+
+  if (config_.synthetic_component_name_pattern.empty()) {
+    RCLCPP_WARN(node_->get_logger(), "Empty synthetic_component_name_pattern, using area directly: %s", area.c_str());
+    return area;
+  }
+
   std::string result = config_.synthetic_component_name_pattern;
 
   // Replace {area} placeholder
diff --git a/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp b/src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp
@@ -96,56 +96,36 @@ void ConfigHandlers::handle_get_configuration(const httplib::Request & req, http
       return;
     }
 
-    const auto cache = ctx_.node()->get_entity_cache();
-
-    std::string node_name;
-    bool entity_found = false;
-    std::string id_field = "component_id";
-
-    // Try components first
-    for (const auto & component : cache.components) {
-      if (component.id == entity_id) {
-        node_name = component.fqn;
-        entity_found = true;
-        break;
-      }
-    }
-
-    // If not found in components, try apps
-    if (!entity_found) {
-      for (const auto & app : cache.apps) {
-        if (app.id == entity_id) {
-          node_name = app.bound_fqn.value_or("/" + app.id);
-          entity_found = true;
-          id_field = "app_id";
-          break;
-        }
-      }
+    // Use unified entity lookup
+    auto entity_info = ctx_.get_entity_info(entity_id);
+    if (entity_info.type == EntityType::UNKNOWN) {
+      HandlerContext::send_error(res, StatusCode::NotFound_404, "Entity not found", {{"entity_id", entity_id}});
+      return;
     }
 
-    if (!entity_found) {
-      std::string error_msg = (id_field == "app_id") ? "App not found" : "Component not found";
-      HandlerContext::send_error(res, StatusCode::NotFound_404, error_msg, {{id_field, entity_id}});
-      return;
+    // Get node name for parameter access
+    std::string node_name = entity_info.fqn;
+    if (node_name.empty()) {
+      node_name = "/" + entity_id;
     }
 
     auto config_mgr = ctx_.node()->get_configuration_manager();
     auto result = config_mgr->get_parameter(node_name, param_name);
 
     if (result.success) {
-      json response = {{id_field, entity_id}, {"parameter", result.data}};
+      json response = {{entity_info.id_field, entity_id}, {"parameter", result.data}};
       HandlerContext::send_json(res, response);
     } else {
       // Check if it's a "not found" error
       if (result.error_message.find("not found") != std::string::npos ||
           result.error_message.find("Parameter not found") != std::string::npos) {
         HandlerContext::send_error(
             res, StatusCode::NotFound_404, "Failed to get parameter",
-            {{"details", result.error_message}, {id_field, entity_id}, {"param_name", param_name}});
+            {{"details", result.error_message}, {entity_info.id_field, entity_id}, {"param_name", param_name}});
       } else {
         HandlerContext::send_error(
             res, StatusCode::ServiceUnavailable_503, "Failed to get parameter",
-            {{"details", result.error_message}, {id_field, entity_id}, {"param_name", param_name}});
+            {{"details", result.error_message}, {entity_info.id_field, entity_id}, {"param_name", param_name}});
       }
     }
   } catch (const std::exception & e) {
@@ -200,44 +180,24 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http
 
     json value = body["value"];
 
-    const auto cache = ctx_.node()->get_entity_cache();
-
-    std::string node_name;
-    bool entity_found = false;
-    std::string id_field = "component_id";
-
-    // Try components first
-    for (const auto & component : cache.components) {
-      if (component.id == entity_id) {
-        node_name = component.fqn;
-        entity_found = true;
-        break;
-      }
-    }
-
-    // If not found in components, try apps
-    if (!entity_found) {
-      for (const auto & app : cache.apps) {
-        if (app.id == entity_id) {
-          node_name = app.bound_fqn.value_or("/" + app.id);
-          entity_found = true;
-          id_field = "app_id";
-          break;
-        }
-      }
+    // Use unified entity lookup
+    auto entity_info = ctx_.get_entity_info(entity_id);
+    if (entity_info.type == EntityType::UNKNOWN) {
+      HandlerContext::send_error(res, StatusCode::NotFound_404, "Entity not found", {{"entity_id", entity_id}});
+      return;
     }
 
-    if (!entity_found) {
-      std::string error_msg = (id_field == "app_id") ? "App not found" : "Component not found";
-      HandlerContext::send_error(res, StatusCode::NotFound_404, error_msg, {{id_field, entity_id}});
-      return;
+    // Get node name for parameter access
+    std::string node_name = entity_info.fqn;
+    if (node_name.empty()) {
+      node_name = "/" + entity_id;
     }
 
     auto config_mgr = ctx_.node()->get_configuration_manager();
     auto result = config_mgr->set_parameter(node_name, param_name, value);
 
     if (result.success) {
-      json response = {{"status", "success"}, {id_field, entity_id}, {"parameter", result.data}};
+      json response = {{"status", "success"}, {entity_info.id_field, entity_id}, {"parameter", result.data}};
       HandlerContext::send_json(res, response);
     } else {
       // Check if it's a read-only, not found, or service unavailable error
@@ -257,7 +217,7 @@ void ConfigHandlers::handle_set_configuration(const httplib::Request & req, http
       }
       HandlerContext::send_error(
           res, status_code, "Failed to set parameter",
-          {{"details", result.error_message}, {id_field, entity_id}, {"param_name", param_name}});
+          {{"details", result.error_message}, {entity_info.id_field, entity_id}, {"param_name", param_name}});
     }
   } catch (const std::exception & e) {
     HandlerContext::send_error(res, StatusCode::InternalServerError_500, "Failed to set configuration",
@@ -287,38 +247,17 @@ void ConfigHandlers::handle_delete_configuration(const httplib::Request & req, h
       return;
     }
 
-    const auto cache = ctx_.node()->get_entity_cache();
-
-    // Find entity to get its namespace and node name
-    std::string node_name;
-    bool entity_found = false;
-    std::string id_field = "component_id";
-
-    // Try components first
-    for (const auto & component : cache.components) {
-      if (component.id == entity_id) {
-        node_name = component.fqn;
-        entity_found = true;
-        break;
-      }
-    }
-
-    // If not found in components, try apps
-    if (!entity_found) {
-      for (const auto & app : cache.apps) {
-        if (app.id == entity_id) {
-          node_name = app.bound_fqn.value_or("/" + app.id);
-          entity_found = true;
-          id_field = "app_id";
-          break;
-        }
-      }
+    // Use unified entity lookup
+    auto entity_info = ctx_.get_entity_info(entity_id);
+    if (entity_info.type == EntityType::UNKNOWN) {
+      HandlerContext::send_error(res, StatusCode::NotFound_404, "Entity not found", {{"entity_id", entity_id}});
+      return;
     }
 
-    if (!entity_found) {
-      std::string error_msg = (id_field == "app_id") ? "App not found" : "Component not found";
-      HandlerContext::send_error(res, StatusCode::NotFound_404, error_msg, {{id_field, entity_id}});
-      return;
+    // Get node name for parameter access
+    std::string node_name = entity_info.fqn;
+    if (node_name.empty()) {
+      node_name = "/" + entity_id;
     }
 
     auto config_mgr = ctx_.node()->get_configuration_manager();
@@ -356,38 +295,17 @@ void ConfigHandlers::handle_delete_all_configurations(const httplib::Request & r
       return;
     }
 
-    const auto cache = ctx_.node()->get_entity_cache();
-
-    // Find entity to get its namespace and node name
-    std::string node_name;
-    bool entity_found = false;
-    std::string id_field = "component_id";
-
-    // Try components first
-    for (const auto & component : cache.components) {
-      if (component.id == entity_id) {
-        node_name = component.fqn;
-        entity_found = true;
-        break;
-      }
-    }
-
-    // If not found in components, try apps
-    if (!entity_found) {
-      for (const auto & app : cache.apps) {
-        if (app.id == entity_id) {
-          node_name = app.bound_fqn.value_or("/" + app.id);
-          entity_found = true;
-          id_field = "app_id";
-          break;
-        }
-      }
+    // Use unified entity lookup
+    auto entity_info = ctx_.get_entity_info(entity_id);
+    if (entity_info.type == EntityType::UNKNOWN) {
+      HandlerContext::send_error(res, StatusCode::NotFound_404, "Entity not found", {{"entity_id", entity_id}});
+      return;
     }
 
-    if (!entity_found) {
-      std::string error_msg = (id_field == "app_id") ? "App not found" : "Component not found";
-      HandlerContext::send_error(res, StatusCode::NotFound_404, error_msg, {{id_field, entity_id}});
-      return;
+    // Get node name for parameter access
+    std::string node_name = entity_info.fqn;
+    if (node_name.empty()) {
+      node_name = "/" + entity_id;
     }
 
     auto config_mgr = ctx_.node()->get_configuration_manager();
diff --git a/src/ros2_medkit_gateway/test/test_integration.test.py b/src/ros2_medkit_gateway/test/test_integration.test.py
@@ -257,18 +257,30 @@ def generate_test_description():
 # API version prefix - must match rest_server.cpp
 API_BASE_PATH = '/api/v1'
 
+# Action timeout - 30s should be sufficient, if still flaky then code has performance issues
+ACTION_TIMEOUT = 30.0
+
 
 class TestROS2MedkitGatewayIntegration(unittest.TestCase):
     """Integration tests for ROS 2 Medkit Gateway REST API and discovery."""
 
     BASE_URL = f'http://localhost:8080{API_BASE_PATH}'
+
+    # Expected entities from demo_nodes.launch.py:
+    # - Apps: temp_sensor, rpm_sensor, pressure_sensor, status_sensor, actuator,
+    #         controller, calibration, long_calibration, lidar_sensor (9 total)
+    # - Areas: powertrain, chassis, body, perception, root (5 total)
+    # - Components: Synthetic groupings by area (powertrain, chassis, body, perception)
+    #               created from nodes in those namespaces
+    #
     # Minimum expected components (synthetic, grouped by top-level namespace)
-    # With default config: powertrain, chassis, body, perception, root
+    # With default config: powertrain, chassis, body, perception (root may not have synthetic)
     MIN_EXPECTED_COMPONENTS = 4
-    # Minimum expected apps (ROS 2 nodes)
+    # Minimum expected apps (ROS 2 nodes from demo launch)
     MIN_EXPECTED_APPS = 8
-    # Minimum expected areas (powertrain, chassis, body + root)
+    # Minimum expected areas (powertrain, chassis, body, perception + root)
     MIN_EXPECTED_AREAS = 4
+
     # Maximum time to wait for discovery (seconds)
     MAX_DISCOVERY_WAIT = 60.0
     # Interval between discovery checks (seconds)
@@ -321,7 +333,7 @@ def _get_json(self, endpoint: str, timeout: int = 10):
         return response.json()
 
     def _wait_for_action_status(
-        self, goal_id: str, target_statuses: list, max_wait: float = 15.0
+        self, goal_id: str, target_statuses: list, max_wait: float = None
     ) -> dict:
         """
         Poll action status until it reaches one of the target statuses.
@@ -333,7 +345,7 @@ def _wait_for_action_status(
         target_statuses : list
             List of status strings to wait for (e.g., ['succeeded', 'aborted']).
         max_wait : float
-            Maximum time to wait in seconds (default: 15.0).
+            Maximum time to wait in seconds. Defaults to ACTION_TIMEOUT (30s).
 
         Returns
         -------
@@ -346,6 +358,8 @@ def _wait_for_action_status(
             If target status is not reached within max_wait.
 
         """
+        if max_wait is None:
+            max_wait = ACTION_TIMEOUT
         start_time = time.time()
         last_status = None
         while time.time() - start_time < max_wait:
@@ -1483,7 +1497,7 @@ def test_41_action_status_after_completion(self):
 
         # Poll for completion instead of fixed sleep (handles CI timing variance)
         data = self._wait_for_action_status(
-            goal_id, ['succeeded', 'aborted'], max_wait=15.0
+            goal_id, ['succeeded', 'aborted'], max_wait=ACTION_TIMEOUT
         )
 
         self.assertIn('goal_id', data)
@@ -1511,7 +1525,7 @@ def test_42_action_cancel_endpoint(self):
         # Poll until action is executing (handles CI timing variance)
         try:
             self._wait_for_action_status(
-                goal_id, ['executing'], max_wait=10.0
+                goal_id, ['executing'], max_wait=ACTION_TIMEOUT
             )
         except AssertionError:
             # If action already completed or is still in accepted, try cancel anyway
