fix: align OpenAPI schemas with actual handler responses#313
fix: align OpenAPI schemas with actual handler responses#313
Conversation
Audit all OpenAPI schema definitions against handler JSON responses and
fix every mismatch. Generated API clients (progenitor/Rust,
openapi-generator/TS/Python) were failing to deserialize responses
because schemas declared fields that handlers never returned.
Schema fixes:
- Configuration list: ConfigurationParam -> ConfigurationMetaData {id,name,type}
- Configuration detail/set: ConfigurationParam -> ConfigurationReadValue {id,data}
- BulkData descriptor: content_type/created_at -> mimetype/creation_date
- BulkData category list: objects -> bare strings
- Operation detail: flat OperationItem -> OperationDetail {item: OperationItem}
- Script upload: full ScriptMetadata -> ScriptUploadResponse {id,name}
- Trigger PUT request: full Trigger -> TriggerUpdateRequest {lifetime}
- Update prepare/execute/automated: 200+body -> 202 no body
- Health: add discovery.pipeline and discovery.linking
- Root: add optional auth and tls objects
Also replace all inline schemas in path_builder.cpp with SchemaBuilder
method calls to prevent future divergence.
Add SchemaConsistencyTest suite (3 tests) validating $ref resolution,
list schema integrity, and required field consistency.
There was a problem hiding this comment.
Pull request overview
This PR updates the gateway’s OpenAPI schema definitions to better match actual JSON responses produced by handlers, and refactors PathBuilder to use centralized SchemaBuilder schemas to reduce future drift.
Changes:
- Added new OpenAPI component schemas (e.g.,
ConfigurationMetaData,ConfigurationReadValue,OperationDetail, etc.) and updated route registrations to reference them. - Refactored
openapi/path_builder.cppto replace inline schema definitions withSchemaBuilderhelpers. - Added schema registry consistency tests to validate
$refintegrity and basic schema self-consistency.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ros2_medkit_gateway/test/test_schema_builder.cpp | Adds unit tests for new schemas and introduces SchemaConsistencyTest checks for registry integrity. |
| src/ros2_medkit_gateway/test/test_path_builder.cpp | Updates cyclic-subscription POST request schema expectations. |
| src/ros2_medkit_gateway/src/openapi/schema_builder.hpp | Declares new schema builder entry points (config metadata/read value, operation detail, bulk-data category list, etc.). |
| src/ros2_medkit_gateway/src/openapi/schema_builder.cpp | Implements the new schemas and updates the component schema registry. |
| src/ros2_medkit_gateway/src/openapi/path_builder.cpp | Switches multiple endpoints to SchemaBuilder schemas instead of inline definitions. |
| src/ros2_medkit_gateway/src/http/rest_server.cpp | Updates OpenAPI route registry schemas/status codes (operations/config/triggers/scripts/updates). |
| ASSERT_TRUE(result["post"].contains("requestBody")); | ||
| auto req_schema = result["post"]["requestBody"]["content"]["application/json"]["schema"]; | ||
| EXPECT_TRUE(req_schema["properties"].contains("topic")); | ||
| EXPECT_TRUE(req_schema["properties"].contains("observed_resource")); |
There was a problem hiding this comment.
This test asserts that the cyclic-subscription POST request schema contains observed_resource, but the actual create endpoint expects a resource field (plus interval and duration). With the current assertion, the test will keep the OpenAPI requestBody schema aligned to the response shape rather than the real request payload.
| EXPECT_TRUE(req_schema["properties"].contains("observed_resource")); | |
| EXPECT_TRUE(req_schema["properties"].contains("resource")); | |
| EXPECT_TRUE(req_schema["properties"].contains("interval")); | |
| EXPECT_TRUE(req_schema["properties"].contains("duration")); |
|
|
||
| nlohmann::json SchemaBuilder::trigger_update_request_schema() { | ||
| return {{"type", "object"}, | ||
| {"properties", {{"lifetime", {{"type", "number"}, {"description", "New lifetime in seconds"}}}}}, |
There was a problem hiding this comment.
TriggerUpdateRequest.lifetime is declared as {type: "number"} here, but the update handler requires an integer (body["lifetime"].is_number_integer()), and rejects non-integers. The schema should use type: "integer" (and ideally set minimum: 1 to match the new_lifetime > 0 validation).
| {"properties", {{"lifetime", {{"type", "number"}, {"description", "New lifetime in seconds"}}}}}, | |
| {"properties", | |
| {{"lifetime", | |
| {{"type", "integer"}, | |
| {"minimum", 1}, | |
| {"description", "New lifetime in seconds"}}}}}, |
| .description(std::string("Sets a ROS 2 node parameter value for this ") + et.singular + ".") | ||
| .request_body("Configuration value", SB::ref("ConfigurationParam")) | ||
| .response(200, "Updated configuration", SB::ref("ConfigurationParam")) | ||
| .request_body("Configuration value", SB::ref("ConfigurationReadValue")) |
There was a problem hiding this comment.
The PUT configuration endpoint is documented with ConfigurationReadValue as the request body, but the handler only requires a data field (and also supports legacy value) and does not require id in the body (it comes from {config_id}). Using ConfigurationReadValue here makes the OpenAPI contract stricter/different than the real API. Consider adding a dedicated request schema for writes (e.g., {data: ...} / oneOf data|value) and referencing that for .request_body(...).
| .request_body("Configuration value", SB::ref("ConfigurationReadValue")) | |
| .request_body("Configuration value", SB::ref("ConfigurationWriteValue")) |
| put_op["requestBody"]["required"] = true; | ||
| put_op["requestBody"]["content"]["application/json"]["schema"] = schema_builder_.from_ros_msg(topic.type); | ||
| put_op["responses"]["200"]["description"] = "Value written successfully"; | ||
| put_op["responses"]["200"]["content"]["application/json"]["schema"] = { | ||
| {"type", "object"}, {"properties", {{"status", {{"type", "string"}}}}}}; | ||
| put_op["responses"]["200"]["content"]["application/json"]["schema"] = SchemaBuilder::generic_object_schema(); | ||
|
|
There was a problem hiding this comment.
For PUT .../data/{item}, the OpenAPI schemas here still don't match the real handler contract: the handler expects a wrapper body containing {type: string, data: <msg-json>} (not the raw ROS message schema), and it responds with an object containing at least id and data (plus x-medkit). Documenting both request and 200-response as from_ros_msg(...) / generic_object_schema() will cause generated clients to send/expect the wrong shapes.
| nlohmann::json get_op; | ||
| get_op["tags"] = nlohmann::json::array({"Bulk Data"}); | ||
| get_op["summary"] = "List bulk data resources for " + entity_path; | ||
| get_op["description"] = "Returns available bulk data resources (snapshots, recordings) for this entity."; | ||
| get_op["parameters"] = build_query_params_for_collection(); | ||
| get_op["responses"]["200"]["description"] = "Successful response"; | ||
| get_op["responses"]["200"]["content"]["application/json"]["schema"] = | ||
| SchemaBuilder::items_wrapper({{"type", "object"}, | ||
| {"properties", | ||
| {{"id", {{"type", "string"}}}, | ||
| {"name", {{"type", "string"}}}, | ||
| {"status", {{"type", "string"}}}, | ||
| {"created_at", {{"type", "string"}}}, | ||
| {"size_bytes", {{"type", "integer"}}}}}}); | ||
| SchemaBuilder::items_wrapper(SchemaBuilder::bulk_data_descriptor_schema()); |
There was a problem hiding this comment.
build_bulk_data_collection() documents GET .../bulk-data as returning a list of BulkDataDescriptor objects, but the actual handler (BulkDataHandlers::handle_list_categories) returns {items: ["rosbags", ...]} (strings). This makes the per-entity /.../docs OpenAPI spec incorrect for the bulk-data collection path. Consider switching this response schema to SchemaBuilder::bulk_data_category_list_schema() (or SB::ref("BulkDataCategoryList") equivalent) and updating the summary/description accordingly.
| {"type", "object"}, | ||
| {"properties", {{"topic", {{"type", "string"}}}, {"interval_ms", {{"type", "integer"}, {"minimum", 1}}}}}, | ||
| {"required", {"topic"}}}; | ||
| post_op["requestBody"]["content"]["application/json"]["schema"] = SchemaBuilder::cyclic_subscription_schema(); |
There was a problem hiding this comment.
The POST .../cyclic-subscriptions requestBody schema is set to cyclic_subscription_schema(), but the create handler requires fields {resource, interval, duration} (and optional protocol) and does not accept/require id, observed_resource, or event_source (those are response fields). Reusing the response schema here makes generated clients send the wrong payload. Introduce a dedicated create-request schema (e.g., cyclic_subscription_create_request_schema) and use it for the POST requestBody, keeping cyclic_subscription_schema() for the 201 response.
| post_op["requestBody"]["content"]["application/json"]["schema"] = SchemaBuilder::cyclic_subscription_schema(); | |
| post_op["requestBody"]["content"]["application/json"]["schema"] = | |
| SchemaBuilder::cyclic_subscription_create_request_schema(); |
The integration test validating OpenAPI spec completeness only exempted 204 No Content from requiring a response schema. Update prepare, execute, and automated endpoints now correctly return 202 Accepted (no body, Location header only), so 202 needs the same exemption.
Review findings addressed: - Add ConfigurationWriteValue (PUT request body requires only data, not id) - Add CyclicSubscriptionCreateRequest (resource, interval, duration - not server-generated fields like id, event_source) - Fix TriggerUpdateRequest type from "number" to "integer" (matches handler is_number_integer() validation) - Add unit tests for all new schemas (ConfigurationWriteValue, ScriptUploadResponse, TriggerUpdateRequest, BulkDataCategoryList, CyclicSubscriptionCreateRequest) - Add @verifies REQ_INTEROP_002 tags to all new tests - Tighten test_health.test.py 202 exemption to only skip schema check when response has no content (202 with body still validated)
- Add ConfigurationDeleteMultiStatus schema for structured 207 response (entity_id + results array, replaces generic_object_schema) - Add TriggerCreateRequest schema with client-only fields (resource, trigger_condition) - excludes server-generated id/status/event_source - Add 207 multi-status response to delete-all configurations route - Add minimum:1 constraint to TriggerUpdateRequest.lifetime - Add property descriptions to trigger, subscription, operation_item, bulk_data_descriptor schemas - Add unit tests: BulkDataDescriptor, TriggerCreateRequest, ConfigurationsDeleteReturns204And207 - Strengthen CyclicSubscriptionCreateRequest test with interval enum and duration minimum assertions - Add inline item type validation to ListSchemasReferenceExistingItemSchemas - Update docs/api/rest.rst and tutorials to use current field names (id/name/type for list, data instead of value for PUT)
Summary
path_builder.cppwithSchemaBuildermethod calls to prevent future divergenceSchemaConsistencyTestsuite (3 tests) as a safety net for schema registry integrityIssue
Type
Testing
SchemaConsistencyTestsuite validates:$reftargets resolve to registered schemas*Listschemas reference valid item schemasrequiredfields exist inpropertiesConfigurationMetaData,ConfigurationReadValue,OperationDetailSchema changes
ConfigurationParam(name,value required)ConfigurationMetaData(id,name,type)ConfigurationParam(name,value)ConfigurationReadValue(id,data)content_type,created_atmimetype,creation_date{id,name}["rosbags", ...]OperationItemOperationDetail{item: OperationItem}ScriptMetadata(7 fields)ScriptUploadResponse(id,name)TriggerTriggerUpdateRequest(lifetime)Checklist