feat(openapi): derive query parameters from a typed query contract#417
Open
bburda wants to merge 1 commit into
Open
feat(openapi): derive query parameters from a typed query contract#417bburda wants to merge 1 commit into
bburda wants to merge 1 commit into
Conversation
Query parameters were the one part of the HTTP contract not derived from a
descriptor. Handlers read them ad-hoc via req.query_param(), and the OpenAPI
generator had no way to see them, so the exported spec - and every client
generated from it - carried zero query parameters even though the gateway reads
them at runtime.
Make query parameters a typed, descriptor-driven contract, like request and
response bodies:
- dto::QueryParamWriter<T> emits the OpenAPI `parameters` (all in: query) for a
query DTO from its dto_fields, mirroring SchemaWriter.
- TypedRequest::query<T>() parses the request query string into a typed query
DTO via the same descriptor.
- RouteEntry::query<T>() declares those parameters on the route.
A handler can only read fields that exist on its query DTO, and those fields are
exactly what the route advertises, so the parsed object and the spec cannot
drift. Adding a parameter means adding a member - which appears in the spec
automatically.
Applied to the endpoints whose handlers read query parameters:
- GET /faults and GET /{entity}/faults: status, include_muted, include_clusters
- DELETE /faults: status
- GET /{entity}/logs: severity, context
- GET /updates: origin, target-version
A pre-commit guard (check_handlers_typed_query.sh) bans raw query reads in
handlers so the typed path stays the only way in. Adds RouteRegistry and
TypedRequest tests covering the writer and reader.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes missing query parameters in the generated OpenAPI spec by introducing a typed, descriptor-driven “query DTO” contract that drives both OpenAPI parameters emission and handler-side query parsing, preventing drift between runtime behavior and the spec.
Changes:
- Add
dto::QueryParamWriter<T>+TypedRequest::query<T>()andRouteEntry::query<T>()to derive/query parameters fromdto_fields<T>. - Migrate faults/logs/updates handlers and routes to the typed query DTOs, and update the OpenAPI route registry to accept bulk query parameters.
- Add unit tests for typed query parsing and OpenAPI query-parameter emission, plus a pre-commit guard to block raw query reads in handlers.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_gateway/test/test_typed_router.cpp | Adds unit tests for TypedRequest::query<T>() parsing into a query DTO. |
| src/ros2_medkit_gateway/test/test_route_registry.cpp | Adds unit tests asserting OpenAPI query parameters are emitted (manual + typed DTO derived). |
| src/ros2_medkit_gateway/src/openapi/route_registry.hpp | Introduces RouteEntry::add_query_parameters() and RouteEntry::query<T>() for typed query declaration. |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Declares typed query DTOs on affected routes via .query<dto::…>(). |
| src/ros2_medkit_gateway/src/http/handlers/update_handlers.cpp | Switches update filtering to req.query<dto::UpdateListQuery>(). |
| src/ros2_medkit_gateway/src/http/handlers/log_handlers.cpp | Switches log query parsing to req.query<dto::LogQuery>(). |
| src/ros2_medkit_gateway/src/http/handlers/fault_handlers.cpp | Switches faults filtering to typed DTOs; refactors status parsing away from raw request access. |
| src/ros2_medkit_gateway/src/core/openapi/route_registry.cpp | Implements RouteEntry::add_query_parameters() to append generated parameter objects. |
| src/ros2_medkit_gateway/scripts/check_handlers_typed_query.sh | Adds a pre-commit guard intended to ban raw query reads in handlers. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/http/typed_router.hpp | Adds TypedRequest::query<T>() for descriptor-driven typed query parsing. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/updates.hpp | Adds UpdateListQuery DTO + dto_fields for /updates query parameters. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/query.hpp | Adds query DTO contract utilities (QueryParamWriter, assign_query_field, optional-unwrapping). |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/logs.hpp | Adds LogQuery DTO + dto_fields for /{entity}/logs query parameters. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/dto/faults.hpp | Adds FaultListQuery / FaultClearQuery DTOs + dto_fields for faults endpoints. |
| src/ros2_medkit_gateway/include/ros2_medkit_gateway/core/http/http_utils.hpp | Refactors fault status parsing utility to take an optional string (decoupled from httplib request). |
| .pre-commit-config.yaml | Registers the new typed-query guard hook. |
Comment on lines
+33
to
+40
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| HANDLERS_DIR="$(cd "${SCRIPT_DIR}/../src/http/handlers" && pwd)" | ||
|
|
||
| # Path reads (req.path), header reads (req.header), and fan-out (raw_for_framework | ||
| # for path/Authorization) are legitimate; only query-string reads are banned. | ||
| pattern='\.query_param\(|->query_param\(|get_param_value\(' | ||
|
|
||
| hits="$(grep -rnE "${pattern}" "${HANDLERS_DIR}" --include='*.cpp' || true)" |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Make query parameters a typed, descriptor-driven contract like request and response bodies, fixing the generated spec which carried zero query parameters.
Handlers read query parameters ad-hoc via
req.query_param(), so the OpenAPI generator never saw them and the exported spec - and every client generated from it - had no query parameters even though the gateway reads them at runtime.dto::QueryParamWriter<T>emits the OpenAPIparameters(allin: query) for a query DTO from itsdto_fields, mirroringSchemaWriter.TypedRequest::query<T>()parses the request query string into a typed query DTO via the same descriptor.RouteEntry::query<T>()declares those parameters on the route.A handler can only read fields that exist on its query DTO, and those fields are exactly what the route advertises, so the parsed object and the spec cannot drift. Adding a parameter means adding a member - which appears in the spec automatically. A pre-commit guard bans raw query reads in handlers so the typed path is the only way in.
Applied to the endpoints whose handlers read query parameters:
GET /faults,GET /{entity}/faults:status,include_muted,include_clustersDELETE /faults:statusGET /{entity}/logs:severity,contextGET /updates:origin,target-versionThe exported spec now declares 26 query parameters (was 0).
Issue
Type
Testing
colcon buildclean; gateway unit suite green, including newRouteRegistrytest (typed.query<T>()emits the parameters) andTypedRequesttests (typedquery<T>()parsing of present/absent params)./api/v1/docs; the spec now declares 26in: queryparameters across the faults/logs/updates endpoints with the correct schema types (string/boolean) andrequired: false.Checklist