Skip to content

[#158] Fixes the dual-path routing bug #160

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
mfaferek93 merged 4 commits into from
Feb 1, 2026
Merged

[#158] Fixes the dual-path routing bug #160

mfaferek93 merged 4 commits into from
Feb 1, 2026
+558 −225

Conversation

@bburda
Copy link
Collaborator

@bburda bburda commented Feb 1, 2026 •
edited
Loading

Pull Request

Summary

Fixes the dual-path routing bug (#158) by enforcing semantic entity-type validation so /components/{id} no longer accepts App IDs (and similarly for other collection routes).

Changes:

  • Add route-path-based entity type extraction and use it to reject mismatched entity IDs with a 400 error.
  • Update handler entity lookup to support type-restricted lookups (avoids cross-collection ID collisions).
  • Extend unit + integration tests (including polling improvements for action execution status).

Issue

Link the related issue (required):

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

@bburda bburda requested a review from mfaferek93 February 1, 2026 19:17
@bburda bburda self-assigned this Feb 1, 2026
Copilot AI review requested due to automatic review settings February 1, 2026 19:17
@bburda bburda added the bug Something isn't working label Feb 1, 2026
@bburda bburda force-pushed the fix/dual-path-routing branch from 30f4fd8 to 42f2e60 Compare February 1, 2026 19:18
Copilot AI reviewed Feb 1, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes the dual-path routing bug (#158) by enforcing semantic entity-type validation so /components/{id} no longer accepts App IDs (and similarly for other collection routes).

Changes:

  • Add route-path-based entity type extraction and use it to reject mismatched entity IDs with a 400 error.
  • Update handler entity lookup to support type-restricted lookups (avoids cross-collection ID collisions).
  • Extend unit + integration tests (including polling improvements for action execution status).

Reviewed changes

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

Show a summary per file
File Description
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/http_utils.hpp Adds extract_entity_type_from_path helper used to infer expected entity type from the URL path.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/handlers/handler_context.hpp Extends get_entity_info API and adds validate_entity_type helper declaration.
src/ros2_medkit_gateway/src/http/handlers/handler_context.cpp Implements type-restricted get_entity_info lookup and validate_entity_type.
src/ros2_medkit_gateway/src/http/handlers/data_handlers.cpp Enforces entity-type correctness for /.../{entity}/{id}/data routes and improves 400 vs 404 reporting.
src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp Enforces entity-type correctness for /.../{entity}/{id}/faults routes with improved error reporting.
src/ros2_medkit_gateway/src/http/handlers/operation_handlers.cpp Adds entity-type validation for operations routes to prevent component/app ID interchangeability.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Adds entity-type validation for configurations routes to prevent component/app ID interchangeability.
src/ros2_medkit_gateway/src/http/rest_server.cpp Removes the TODO comment now that entity-type validation is implemented.
src/ros2_medkit_gateway/test/test_gateway_node.cpp Adds unit tests for entity-type extraction from request paths.
src/ros2_medkit_gateway/test/test_integration.test.py Improves action execution polling stability and adds integration tests asserting /components/{app_id}/... is rejected.
docs/getting_started.rst Adjusts JSON examples ({} instead of {...}) for clarity/validity in docs.

@bburda bburda changed the title Fix/dual path routing [#158] Fixes the dual-path routing bug Feb 1, 2026
bburda added a commit that referenced this pull request Feb 1, 2026
@bburda
fix(gateway): address PR #160 review comments
8dafac9 
- Fix collection matching in extract_entity_type_from_path to require
  segment boundaries (prevents /componentship matching as COMPONENT)
- Remove unused validate_entity_type helper from HandlerContext
- Fix integration tests 116-119 to avoid app/component ID collisions
  by selecting app IDs that don't exist in the components list
- Add unit tests for segment-boundary matching edge cases
mfaferek93
mfaferek93 reviewed Feb 1, 2026
View reviewed changes
Base automatically changed from docs/add-more-docs to main February 1, 2026 19:37
bburda added a commit that referenced this pull request Feb 1, 2026
@bburda
fix(gateway): unify entity validation across handlers
d58db39 
- Add validate_entity_for_route() helper in HandlerContext that combines
  entity_id validation, path-based type extraction, and entity lookup
- Refactor data_handlers.cpp (3 handlers) to use unified validation
- Refactor config_handlers.cpp (7 handlers) to use unified validation
- Refactor fault_handlers.cpp (5 handlers) to use unified validation
- Refactor operation_handlers.cpp (3 handlers) to use unified validation
- Net reduction of ~270 lines of duplicated validation code

Addresses review comments from mfaferek93 on PR #160
bburda added 3 commits February 1, 2026 19:55
@bburda
fix(gateway): Validate entity type matches route path for dual-path e…
e62a710 
…ndpoints

- Add expected_type parameter to HandlerContext::get_entity_info()
- Update data, fault, config, and operation handlers to validate entity type
- Return 400 Bad Request when App ID is used on Component route (and vice versa)
- Add unit tests for extract_entity_type_from_path() utility

This fixes the issue where /components/{id}/data incorrectly accepted App IDs
in runtime-only discovery mode, where entity IDs can collide between entity types.
@bburda
fix(gateway): address PR #160 review comments
1753772 
- Fix collection matching in extract_entity_type_from_path to require
  segment boundaries (prevents /componentship matching as COMPONENT)
- Remove unused validate_entity_type helper from HandlerContext
- Fix integration tests 116-119 to avoid app/component ID collisions
  by selecting app IDs that don't exist in the components list
- Add unit tests for segment-boundary matching edge cases
@bburda
fix(gateway): unify entity validation across handlers
fa4279d 
- Add validate_entity_for_route() helper in HandlerContext that combines
  entity_id validation, path-based type extraction, and entity lookup
- Refactor data_handlers.cpp (3 handlers) to use unified validation
- Refactor config_handlers.cpp (7 handlers) to use unified validation
- Refactor fault_handlers.cpp (5 handlers) to use unified validation
- Refactor operation_handlers.cpp (3 handlers) to use unified validation
- Net reduction of ~270 lines of duplicated validation code

Addresses review comments from mfaferek93 on PR #160
@bburda bburda force-pushed the fix/dual-path-routing branch from d58db39 to fa4279d Compare February 1, 2026 19:55
@bburda
fix(gateway): preserve JSON validation before entity existence check
0a2c136 
In handle_set_configuration, validate JSON body format before checking
entity existence. This ensures BadRequest (400) is returned for invalid
JSON instead of NotFound (404), matching the expected error priority.

Fixes test_set_configuration_invalid_json and
test_set_configuration_missing_value_field tests.
@mfaferek93 mfaferek93 self-requested a review February 1, 2026 20:43
@mfaferek93 mfaferek93 merged commit 1dcf6bf into main Feb 1, 2026
4 checks passed
mfaferek93 pushed a commit that referenced this pull request Feb 1, 2026
@bburda @mfaferek93
fix(gateway): address PR #160 review comments
4e8778a 
- Fix collection matching in extract_entity_type_from_path to require
  segment boundaries (prevents /componentship matching as COMPONENT)
- Remove unused validate_entity_type helper from HandlerContext
- Fix integration tests 116-119 to avoid app/component ID collisions
  by selecting app IDs that don't exist in the components list
- Add unit tests for segment-boundary matching edge cases
mfaferek93 pushed a commit that referenced this pull request Feb 1, 2026
@bburda @mfaferek93
fix(gateway): unify entity validation across handlers
06d02dc 
- Add validate_entity_for_route() helper in HandlerContext that combines
  entity_id validation, path-based type extraction, and entity lookup
- Refactor data_handlers.cpp (3 handlers) to use unified validation
- Refactor config_handlers.cpp (7 handlers) to use unified validation
- Refactor fault_handlers.cpp (5 handlers) to use unified validation
- Refactor operation_handlers.cpp (3 handlers) to use unified validation
- Net reduction of ~270 lines of duplicated validation code

Addresses review comments from mfaferek93 on PR #160
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@mfaferek93 mfaferek93 mfaferek93 approved these changes

Assignees

@bburda bburda

Labels

bug Something isn't working

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Dual-path routing bug: App IDs resolved via /components/{id}

3 participants

@bburda @mfaferek93