Skip to content

feat(gateway): typed DTO contract for HTTP responses and OpenAPI schema#403

Open
bburda wants to merge 4 commits into
mainfrom
feat/338-openapi-dto-contract
Open

feat(gateway): typed DTO contract for HTTP responses and OpenAPI schema#403
bburda wants to merge 4 commits into
mainfrom
feat/338-openapi-dto-contract

Conversation

@bburda
Copy link
Copy Markdown
Collaborator

@bburda bburda commented May 18, 2026

Pull Request

Summary

The gateway's HTTP payloads were maintained as three independent, hand-written representations - the handler's JSON construction, the OpenAPI schema factories, and the XMedkit extension builder - with nothing keeping them in sync. They drifted: entity list responses advertised a minimal schema while the gateway actually emitted rich x-medkit metadata, so generated clients could not see those fields.

This introduces a typed DTO contract. Each payload is a plain C++17 struct described once by a constexpr field list. Three visitors fold over that single description:

  • JsonWriter - struct to wire JSON
  • SchemaWriter - type to OpenAPI schema
  • JsonReader - request body to struct, with validation

Wire output and OpenAPI schema are therefore generated from one source and cannot drift. All HTTP handler domains (entities, faults, operations, configurations, data, locks, triggers, cyclic-subscriptions, bulk-data, logs, scripts, updates, auth, health) were migrated to the contract, and components/schemas is now generated from the DTO registry. The legacy XMedkit fluent builder and the ~50 hand-written schema factories were removed.

Client-observable changes

  • The OpenAPI components/schemas is regenerated from the DTOs; several schema names changed (for example the entity list/detail schemas). Clients generated from the previous spec need to be regenerated.
  • Entity responses now always include the type discriminator (previously absent on list items).
  • Optional fields previously emitted as empty strings or empty arrays are now omitted when empty.
  • Request-body validation failures on some PUT/POST endpoints now return the error code invalid-request (HTTP status 400 is unchanged).

Known limitation

The fault/config/data/log list endpoints emit a richer collection-level x-medkit than the generic Collection<T> wrapper schema types it as. The wire payload stays a valid instance of the published schema; this is a schema-precision gap, documented in design/dto_contract.rst.


Issue

Part of #338.

This PR makes the gateway spec accurate. #338 stays open until generated clients are regenerated against the corrected spec and downstream consumers updated, which is follow-up work in separate repositories.


Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Testing

  • New test_dto_contract unit suite round-trips every registered DTO through all three visitors and validates each generated schema.
  • Integration tests extended: live endpoint responses are validated against the served OpenAPI spec, and the typed x-medkit sub-schemas are asserted present in the spec.
  • Full ros2_medkit_gateway and ros2_medkit_integration_tests suites pass (unit, integration, linters); clang-tidy is clean.

Reviewers can verify by building ros2_medkit_gateway, running colcon test, and hitting GET /api/v1/docs to confirm components/schemas contains the typed entity and x-medkit schemas.


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 marked this pull request as ready for review May 19, 2026 10:15
Copilot AI review requested due to automatic review settings May 19, 2026 10:15
Copy link
Copy Markdown
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

Introduces a typed DTO contract for the gateway's HTTP responses so that wire JSON, OpenAPI schemas, and x-medkit extensions are all generated from a single C++17 source-of-truth, eliminating drift between handler output and the published spec. All HTTP handler domains were migrated, the legacy XMedkit fluent builder and ~50 hand-written schema factories were removed, and integration tests now validate live responses against the served spec.

Changes:

  • Add Draft202012/Draft7 jsonschema validator import with Humble fallback and new skip patterns in test_openapi_callability.test.py for schema limitations (Configuration/Trigger/CyclicSubscription).
  • Add new integration test methods to assert typed x-medkit sub-schemas (XMedkitArea, XMedkitRos2, AreaListItem) are present and that live entity GET responses conform to the served OpenAPI spec (with $ref-inlining helper).
  • Delete test_x_medkit.cpp since the XMedkit fluent builder is replaced by the DTO/visitor contract.

Reviewed changes

Copilot reviewed 70 out of 70 changed files in this pull request and generated no comments.

File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_callability.test.py Adds DTO/schema presence checks, live-response-vs-spec conformance test, and skip patterns for known schema-precision gaps.
src/ros2_medkit_gateway/test/test_x_medkit.cpp Removed; superseded by the new DTO contract test suite (test_dto_contract).

Note: the PR description states the change touches all handler domains, OpenAPI builder, and removes ~50 schema factories, but only two files are visible in this diff slice. The bulk of the migration (handlers, DTO/visitor framework, OpenAPI builder rewiring, test_dto_contract) is outside the reviewed hunks and cannot be assessed here.

@bburda bburda self-assigned this May 19, 2026
@bburda bburda requested a review from mfaferek93 May 19, 2026 10:51
@bburda bburda force-pushed the feat/338-openapi-dto-contract branch from 1bed0f8 to 0d50544 Compare May 27, 2026 13:45
mfaferek93

This comment was marked as outdated.

mfaferek93

This comment was marked as outdated.

auto & member = obj.*(f.ptr);
using MemberT = std::decay_t<decltype(member)>;
const auto it = j.find(key);
if (it == j.end() || it->is_null()) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Treating null as absent here means {"data": null} on PUT configurations/{name} and PUT data/{name} now 400s. The old handlers accepted data:null as a value (body.contains("data") is true for null). Setting a parameter/value to null is legitimate and now breaks, and it isn't in the documented change list. Worth distinguishing present-but-null from absent for the json / optional value members.


template <>
inline constexpr auto dto_fields<ScriptControlRequest> =
std::make_tuple(field_enum("action", &ScriptControlRequest::action, kScriptControlActionValues));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

field_enum on action rejects anything outside {stop, forced_termination} at parse time, but control_execution is a virtual ScriptProvider - a plugin backend with custom actions (pause/resume/...) is now blocked at the gateway. Same pattern you reverted for ExecutionUpdateRequest.capability (71b51ae); worth a plain field() here too and letting the provider validate.

// Uses field_enum: phase is a RESPONSE-side field; the handler does NOT perform
// bespoke validation of the phase value in any request.
// =============================================================================
struct XMedkitUpdate {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This nests phase under x-medkit (x-medkit.phase), but the old update_status_to_json emitted a flat top-level x-medkit-phase, so clients reading x-medkit-phase break silently. Consistent with the nested-x-medkit convention, but it's a breaking wire change not in the client-observable list - same as config list items moving x-medkit-source/x-medkit-node into a nested x-medkit object. Worth adding both to the change list.

// fault_code, severity, description, first_occurred, last_occurred,
// occurrence_count, status, reporting_sources, severity_label
// =============================================================================
struct FaultListItem {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

FaultListItem is schema-only: the fault list handlers emit items verbatim from the transport (fault_handlers.cpp:339), never through JsonWriter, so this schema is an unverified claim about that wire - the drift the contract was meant to remove. It matches fault_msg_conversions today but nothing enforces it. Also several always-present fields are marked optional here, so the schema is looser than the wire. Route items through the DTO, or add a test pinning the verbatim items to this schema.

v);
} else {
// string / bool / integral / floating / nlohmann::json passthrough
return nlohmann::json(v);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The terminal else has no static_assert, unlike decode_value (json_reader.hpp:88) and schema_of (schema_writer.hpp:61) which both end in static_assert(sizeof(U)==0). If a DTO sentinel ever leaks (a nested DTO used in a TU that didn't see its dto_fields specialization), is_dto_v is false and this silently takes the scalar branch instead of failing to compile. Defused today by include discipline + the registry-wide instantiation, but a matching else static_assert here would make the writer fail loud like the other two visitors.

field("description", &FaultListItem::description), field("first_occurred", &FaultListItem::first_occurred),
field("last_occurred", &FaultListItem::last_occurred), field("occurrence_count", &FaultListItem::occurrence_count),
field("status", &FaultListItem::status), field("reporting_sources", &FaultListItem::reporting_sources),
field("severity_label", &FaultListItem::severity_label));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

severity_label is a plain field() so no enum reaches the schema, but the comment on line 49 and the old OpenAPI spec constrained it to INFO|WARN|ERROR|CRITICAL|UNKNOWN (same for severity_filter in logs). On a PR about schema accuracy this drops a constraint the previous spec advertised. kFaultSeverityLabelValues exists but is unused - field_enum here would restore it (response-side enum is schema-only, no request-rejection risk).

bburda added a commit that referenced this pull request May 28, 2026
…error renderer

3 auth routes migrate to typed handlers. Per-route .error_renderer(
kOAuth2Error) on RouteEntry wires errors through the framework's
write_oauth2_error primitive (RFC 6749 §5.2 shape: {error, error_description})
instead of SOVD GenericError.

Adds dto::TokenRequest, TokenResponse, TokenRevokeRequest,
TokenRevokeResponse for the auth endpoints (revoke was a missed
migration in PR #403). Wire format unchanged.
bburda added a commit that referenced this pull request May 28, 2026
7 operation routes migrate to typed handlers. create_execution uses
post_alternates<OperationExecutionResult, ExecutionCreateAsync>
for sync 200 / async 202 dispatch via dto_alternate_status trait.

Adds dto::ExecutionId + Collection<ExecutionId> for the previously
hand-built list_executions endpoint (missed migration from PR #403).
Removes the legacy pragma block now that all calls go through the
typed validators. Wire format unchanged.
bburda added a commit that referenced this pull request May 28, 2026
Updates design/dto_contract.rst with:
- Typed router section (reg.get<T>, Result<T>, static_assert is_dto_v)
- Escape hatches enumerated (sse, binary_download, multipart_upload,
  static_asset, docs_endpoint, alternates, plugin passthrough)
- Provider ABI typed-only policy
- Fan-out observability via peer_dropped_items
- Opaque object policy for runtime-shape fields
- OpenAPI generation pipeline now single-source from AllDtos

CHANGELOG.rst BREAKING entries:
- HandlerContext::send_*/parse_body removed
- Provider ABI returns typed DTOs
- RouteRegistry raw overloads deleted
- SchemaWriter optional fields emit anyOf+null (OpenAPI 3.1 idiom)
- Plugin ABI unchanged (test_plugin_abi_conformance locks it in)
- rtmaps_medkit not supported by this PR
bburda added 4 commits May 28, 2026 19:25
Introduces the typed DTO contract that lets a single C++ struct describe
each HTTP payload once, with three visitors folding over that description:
JsonWriter (DTO -> wire JSON), JsonReader (request body -> DTO with
validation), and SchemaWriter (type -> OpenAPI schema). Wire output and
OpenAPI schema therefore cannot drift because both are generated from the
same dto_fields<T> tuple.

Adds opaque_object(...) for fields whose runtime shape depends on context
(live ROS messages, plugin-defined payloads). Schema emits
{type:object, additionalProperties:true, x-medkit-opaque:true}; the
envelope around the opaque value stays typed.

Adds the typed RouteRegistry layer on top of cpp-httplib:
- reg.get<TResponse>(path, handler) where handler is
  Result<TResponse>(TypedRequest); the framework writes the response,
  handlers never see httplib::Response.
- static_assert(dto::is_dto_v<TResponse>) makes the typed path the only
  way to register a route.
- Named escape hatches: sse<TEvent>, binary_download, multipart_upload<T>,
  static_asset, docs_endpoint/docs_subtree, post_alternates<TBody,TAlt...>,
  del_alternates<TAlt...> for routes that return one of several typed
  shapes (200/202, 204/207). Per-route ErrorRenderer knob (SOVD generic
  error vs OAuth2 RFC 6749).

Framework primitives (write_json_body / write_generic_error /
write_oauth2_error) live behind a FrameworkOrPluginAccess friend gate so
handler code cannot reach them directly. The HandlerContext validator
surface (validate_entity_for_route, validate_collection_access,
validate_lock_access) returns tl::expected<T, ErrorInfo>; the typed
router wrapper writes the error and the framework owns response writing.

Closes the legacy invisible-drift gap on aggregation fan-out via
fan_out_collection<T>: peer items are parsed through JsonReader<T>,
malformed items are dropped into x-medkit.peer_dropped_items with a
WARN log so operators can see version skew in a 2-version cluster.

Collection<T, XMedkit = XMedkitCollection> 2-parameter template lets
per-domain list endpoints plug their richer XMedkit type directly into
the wrapper; previously the precision gap was documented and accepted,
now it's resolved.

SchemaWriter emits OpenAPI 3.1 anyOf+null idiom for std::optional<T>
fields so generated clients can express T | null instead of degrading
to T | undefined.

DTO sentinel trap is now a static_assert with an actionable message
pointing the developer at the right dto/<domain>.hpp header.
Provider virtual methods now return tl::expected<TypedDto, ErrorInfo>
instead of tl::expected<nlohmann::json, ErrorInfo>. The four provider
interfaces that previously returned raw JSON are migrated:

- UpdateProvider::get_update -> tl::expected<UpdateDetail, ...>
- OperationProvider::list_operations / get_operation / execute_operation
  -> typed Collection<OperationItem> / OperationItem /
  OperationExecutionResult. The execution result envelope is typed but
  carries an opaque ROS action payload (opaque_object marker).
- DataProvider::list_data / read_data / write_data -> opaque DataList /
  DataValue / DataWriteResult envelopes. DataValue's ros payload field
  is opaque because the shape depends on the ROS message type resolved
  at runtime.
- FaultProvider::list_faults / get_fault / clear_fault -> opaque
  FaultListResult / FaultDetailResult / FaultClearResult envelopes.
  Plugins frequently extend fault items with vendor-specific keys (UDS
  DTC environment records, OPC-UA alarm metadata); the opaque envelope
  preserves byte-for-byte plugin extensibility while exposing the typed
  C++ ABI.

Opaque envelope rationale: where plugin implementations need to attach
keys outside the SOVD vocabulary, the C++ ABI returns a typed wrapper
whose JsonWriter/JsonReader/SchemaWriter are specialised to pass the
inner json verbatim, with the schema marked x-medkit-opaque:true so
generated clients see "object whose runtime shape is implementation-
defined".

Wire format unchanged for every existing provider response.

In-tree open-source ros2_medkit_opcua plugin and the in-tree test
plugins are migrated to the typed signatures. Commercial plugin .so
files (UDS, OPC-UA gateway plugin, Uptane OTA, OTA) need parallel
migration PRs in their own repositories - their handlers compile
against the new typed signatures, returning typed wrappers around
their existing json payloads.

Tutorial / plugin-system.rst updated to show the typed signatures.
All 14 handler files migrate from raw void(httplib::Request, Response)
signatures to typed Result<TResponse>(TypedRequest [, TBody]):

- health, discovery (18 routes), lock (5 + 201/Location), trigger
  (6 incl. SSE), cyclic_subscription (6 incl. SSE), auth (3 + OAuth2
  error renderer), update (8 incl. 202/Location), log (3 typed
  fan-out), script (8 incl. multipart upload, HATEOAS _links typed),
  bulkdata (11 incl. binary_download + multipart_upload), config (5
  incl. del_alternates<NoContent, ConfigurationDeleteMultiStatus> for
  204/207), operation (7 incl. post_alternates for sync/async, plugin
  passthrough for free-form body), data (5 incl. opaque DataValue),
  fault (6 + SSE incl. del_alternates and X-Medkit-Local-Only
  attachment).

rest_server.cpp setup_routes is now exclusively typed reg.get<T>/
post<TB,T>/put<TB,T>/del<T>/patch<TB,T>/sse<TEvent>/binary_download/
multipart_upload<T>/static_asset/docs_endpoint. The deprecated raw
overloads on RouteRegistry are removed.

Issue #338 final compliance: per-item wire shape for fault list and
configuration list endpoints is now enforced by JsonReader<T> (closes
the "schema declares Items[T] but wire is raw json" gap).

Forwarded path through aggregation: validators that proxy to a peer
return Forwarded; the framework wrapper detects an internal sentinel
ErrorInfo and skips the response write so the proxied response stays
intact. Wire format byte-identical with the legacy direct-write
forwarded path.

operation_handlers plugin branch passes the raw request body to
OperationProvider::execute_operation rather than the typed
ExecutionCreateRequest envelope, preserving the pre-migration ABI for
plugin operations that read vendor-specific top-level keys (UDS
session_type / reset_type, OPC-UA acknowledge_fault arguments).

Tests:
- test_*_handlers.cpp rewritten to assert against Result<T> return
  values rather than http response state.
- Integration tests run unchanged - wire format byte-identical.
…HANGELOG

- test_plugin_abi_conformance.cpp: loads test_gateway_plugin.so via the
  production PluginLoader and verifies the plugin ABI is byte-stable.
  Twelve compile-time static_asserts guard the public layout of
  GatewayPlugin, PluginRoute, PluginRequest, PluginResponse; six runtime
  tests cover route registration, send_json content-type/indent, send_error
  vendor-error remapping and status clamping. If a future change breaks the
  plugin contract this test fails before commercial plugin builds catch it.

- typed_test_fixture.hpp: shared TypedRequest synthesis helper for the
  handler test suites that exercise the typed handlers directly.

- tsan_suppressions.txt: narrowly-scoped suppressions for libstdc++ future
  internals, yaml-cpp, vendored dynmsg, and the libyaml-cpp shared object.
  These races trace into third-party code reached via rclcpp GenericClient
  / dynamic ROS type introspection; our own JSON/transport paths remain
  visible to TSan.

- design/dto_contract.rst: typed router section, escape hatch enumeration,
  provider ABI typed-only policy, fan-out observability via
  peer_dropped_items, opaque object policy, OpenAPI generation pipeline.

- CHANGELOG.rst, README.md, docs/tutorials/openapi.rst: BREAKING entries
  on HandlerContext::send_* removal, provider ABI typed returns,
  RouteRegistry raw overload deletion, and the OpenAPI 3.1 anyOf+null
  representation for optional fields.

- test_openapi_callability.test.py: parametrized x-medkit presence test
  across all four entity types (Area / Component / App / Function) and
  updated optional-field probes to walk the new anyOf shape.
@bburda bburda force-pushed the feat/338-openapi-dto-contract branch from 86015c4 to 3048b43 Compare May 28, 2026 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants