Skip to content

Nest x-medkit vendor fields on SOVD endpoint payloads#390

Open
bburda wants to merge 8 commits intomainfrom
feat/issue-385-nest-x-medkit-vendor-fields
Open

Nest x-medkit vendor fields on SOVD endpoint payloads#390
bburda wants to merge 8 commits intomainfrom
feat/issue-385-nest-x-medkit-vendor-fields

Conversation

@bburda
Copy link
Copy Markdown
Collaborator

@bburda bburda commented Apr 26, 2026

Summary

Migrates the two remaining SOVD-standard endpoint payloads that emit flat
x-medkit-* top-level vendor keys to the nested x-medkit: {...}
convention that every other endpoint already follows.

  • GET /updates/{id}/status -> x-medkit.phase (was flat x-medkit-phase)
  • GET /apps|components/{id}/configurations -> nested x-medkit.source
    on items, x-medkit.{source,node} on per-parameter entries (was flat
    x-medkit-source / x-medkit-node)

The OpenAPI schemas for UpdateStatus and ConfigurationMetaData now
declare the nested object so generated clients pick up accurate typing.

Adds test_openapi_response_drift - a runtime drift validator that
walks every GET endpoint declared in GET /docs, fetches a real
response, and validates it against the declared response schema. The
original x-medkit-phase divergence slipped past every typed client
because the schema never declared the field; this test catches that
class of regression. The compile-time emitter -> schema contract is
deferred to #338.


Issue


Type

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

Breaking on the wire format of two endpoints. Cross-repo grep showed
zero current consumers reading the legacy flat keys (web UI, MCP,
foxglove, demos, generated clients, commercial).


Testing

  • colcon build --packages-up-to ros2_medkit_gateway ros2_medkit_integration_tests (Jazzy, Release)
  • colcon test --packages-select ros2_medkit_gateway --ctest-args -L linter -> green
  • ctest -R test_update_manager -> 24/24 passing (UpdateStatusToJson + UpdateManagerFailureTest assertions updated)
  • Smoke: GET /api/v1/docs returns UpdateStatus.properties.x-medkit schema with phase enum
  • Drift integration test (test_openapi_response_drift) runs in CI - requires python3-jsonschema (added to ros2_medkit_integration_tests/package.xml)

Checklist

  • Breaking changes are clearly described
  • Tests were added or updated if needed
  • Docs were updated (docs/api/rest.rst example payload + vendor extension prose)

Per #385, vendor extensions on SOVD-standard endpoint payloads should be
nested under a single `x-medkit` object instead of flat top-level keys.
This was already the convention for most endpoints (faults, data, ops,
bulk-data, ...) but two sites still emitted flat keys:

- GET /updates/{id}/status emitted `x-medkit-phase` at the top level
- GET /apps|components/{id}/configurations emitted `x-medkit-source`
  on each item and `x-medkit-source`/`x-medkit-node` on each parameter

Both now emit a nested `x-medkit` object. The OpenAPI schemas for
UpdateStatus and ConfigurationMetaData declare the nested object
explicitly so generated clients (ros2_medkit_clients) get accurate
typing.

Adds an integration test (test_openapi_response_drift) that validates
runtime GET responses against the OpenAPI response schema served at
/api/v1/docs. The original drift slipped past every typed client because
the schema never declared `x-medkit-phase` even though the handler
emitted it; this test catches that class of regression at runtime.
The compile-time contract (template-based emitter-to-schema link) lands
later under #338.
Copilot AI review requested due to automatic review settings April 26, 2026 15:56
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

This PR standardizes remaining SOVD-standard payloads to use nested x-medkit: { ... } vendor extensions (instead of flat x-medkit-* keys) and strengthens spec/implementation consistency by adding an integration drift test that validates live GET responses against the runtime OpenAPI 3.1 schema served at /docs.

Changes:

  • Migrate update status payload vendor extension from x-medkit-phase to x-medkit.phase and update schema/docs/tests accordingly.
  • Migrate configurations payload vendor extension fields to nested x-medkit objects (per-item and per-parameter metadata).
  • Add test_openapi_response_drift integration test and the python3-jsonschema test dependency.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test validating live GET JSON responses against /docs OpenAPI response schemas.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema as a test dependency for the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates unit tests to assert nested x-medkit.phase.
src/ros2_medkit_gateway/src/updates/update_manager.cpp Comment updates to reflect nested vendor extension naming.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates OpenAPI schemas to declare nested x-medkit for UpdateStatus + ConfigurationMetaData.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Emits nested x-medkit metadata instead of flat x-medkit-source/x-medkit-node.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Switches UpdateStatus JSON serialization to nested x-medkit.phase.
docs/api/rest.rst Updates REST docs examples/prose for nested x-medkit.phase.

@bburda bburda marked this pull request as draft April 26, 2026 18:12
@bburda bburda self-assigned this Apr 26, 2026
bburda added 5 commits April 27, 2026 09:48
GET /apps/{id}/is-located-on returns a single-element collection
({items, x-medkit, _links}) per the handler in discovery_handlers.cpp,
but the spec was advertising it as EntityDetail (single object with
required id/name). Aligns the spec to the actual response shape so the
new drift test passes and generated clients get the right type.
ProcInfoHandler iteration yields ProcessExited events, not name keys -
indexing back into proc_info[proc] raised KeyError on every shutdown.
Mirrors the pattern used in test_openapi_callability.
Ubuntu 22.04 ships python3-jsonschema 3.2 which lacks Draft202012Validator
(added in 4.0). The validation surface we use (required, type, properties)
behaves identically across drafts, so falling back to Draft7 keeps the
drift test working on Humble while preferring Draft202012 on Jazzy/Rolling
for OpenAPI 3.1 alignment.
Cold-cache ASan builds take ~33 min on the GitHub-hosted runner, which
left ~12 min for the integration test pass. PR branches that miss the
ccache restore-keys lookup were getting cancelled at the last scenario
test before the actions/cache post step could persist the build, so
each subsequent run hit the same cold cache. 60 min provides clear
headroom to complete + save cache once, after which warm-cache runs
will return to the typical ~22 min.
SOVD does not require vendor extensions, and the existing codebase
convention (fault_detail_schema, entity_detail_schema) leaves x-medkit
out of the required list even when the gateway always emits it. Marking
it required in update_status_schema would imply a SOVD-incompatible
contract while only the gateway implementation actually depends on it.
The explicit handler guard (test_update_status_payload_uses_nested_x_medkit)
keeps regression risk covered.
@bburda bburda marked this pull request as ready for review April 27, 2026 13:58
@bburda bburda requested review from Copilot and mfaferek93 April 27, 2026 13:58
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

This PR standardizes remaining flat x-medkit-* vendor keys on SOVD-standard payloads into the existing nested "x-medkit": {...} convention, updates the runtime OpenAPI schemas accordingly, and adds an integration test intended to detect handler-vs-schema response drift.

Changes:

  • Migrate update status vendor field from flat x-medkit-phase to nested x-medkit.phase (code, schema, docs, and tests).
  • Migrate configurations vendor fields to nested x-medkit objects (items and per-parameter entries) and update ConfigurationMetaData schema.
  • Add test_openapi_response_drift and add python3-jsonschema as an integration-test dependency.

Reviewed changes

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

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test that fetches /docs, calls GET endpoints, and validates 200 JSON bodies against response schemas.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema test dependency for the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates assertions for nested x-medkit.phase in update-status JSON.
src/ros2_medkit_gateway/src/updates/update_manager.cpp Updates comments to reflect nested x-medkit.phase convention.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates UpdateStatus and ConfigurationMetaData schemas to declare nested x-medkit objects.
src/ros2_medkit_gateway/src/http/rest_server.cpp Fixes spec response typing for GET /apps/{id}/is-located-on to match actual list payload shape.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Emits nested x-medkit objects for aggregated configuration metadata and per-parameter provenance.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Changes update status JSON serialization to emit nested x-medkit.phase.
docs/api/rest.rst Updates REST docs examples and prose to use nested x-medkit.phase.
.github/workflows/quality.yml Increases CI job timeout to accommodate cold-cache ASan builds.

Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp Outdated
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp Outdated
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp
Four review points addressed:

- test_update_manager.cpp: switch j["x-medkit"]["phase"] reads to
  contains() + at(). nlohmann's operator[] on a non-const json inserts
  missing keys, which can mask a regression where update_status_to_json
  stops emitting the vendor extension.
- test_openapi_response_drift: REQUIRED_APPS = {temp_sensor, calibration}
  forces deterministic discovery before setUpClass proceeds, instead of
  relying on MIN_EXPECTED_APPS=1 catching an arbitrary first node.
- _inline_refs: raise _RefResolutionError on unresolved or cyclic $refs
  rather than returning {} (which validates anything and hides drift).
  Caught in the validation loop so multiple spec issues surface in one
  run.
- Module docstring: spell out what the validator catches (required,
  type, enum, nested shape) and what it does not (extra undeclared
  fields, since most schemas don't set additionalProperties: false).
  Closing the additionalProperties gap belongs in the issue #338 rework.
@bburda bburda requested a review from Copilot April 27, 2026 16:00
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

This PR standardizes remaining SOVD endpoint vendor extensions to the nested "x-medkit": {...} convention (instead of flat x-medkit-* keys), updates the OpenAPI schemas accordingly, and adds an integration test to detect schema/handler response drift.

Changes:

  • Migrate update status vendor field from x-medkit-phase to nested x-medkit.phase (payload + schema + tests + docs).
  • Migrate configurations vendor fields to nested x-medkit (source on list items; source/node on per-parameter entries in the vendor extension payload).
  • Add an integration drift test that validates live GET responses against the runtime /docs JSON Schemas; extend CI timeout to accommodate slower cold-cache ASan runs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test that walks GET endpoints from /docs and validates 200 JSON responses against declared schemas; includes a targeted guard for nested x-medkit.phase on update status.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema as a test dependency to support the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates unit tests to assert nested x-medkit.phase using contains()/at() (non-mutating access).
src/ros2_medkit_gateway/src/updates/update_manager.cpp Updates comments to reflect the new nested vendor extension naming.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates schemas for UpdateStatus and ConfigurationMetaData to declare nested x-medkit vendor extensions.
src/ros2_medkit_gateway/src/http/rest_server.cpp Fixes the documented OpenAPI response type for GET /apps/{id}/is-located-on to EntityList (matches actual handler behavior).
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Migrates configuration vendor keys from flat x-medkit-* to nested x-medkit objects.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Changes update_status_to_json() to emit nested x-medkit.phase instead of flat x-medkit-phase.
docs/api/rest.rst Updates update status example payload and vendor extension prose to the nested x-medkit.phase format.
.github/workflows/quality.yml Increases CI timeout from 45 to 60 minutes to avoid cancellations on cold-cache ASan runs.
Comments suppressed due to low confidence (1)

docs/api/rest.rst:1190

  • The docs claim GET /updates/{id}/status includes an error object when status is failed, but the gateway currently serializes error as a string (see update_status_to_json() and the OpenAPI UpdateStatus schema). Please update the example/wording here to match the actual wire format so clients don’t implement the wrong type.
   When ``status`` is ``failed``, an ``error`` object is included:

   .. code-block:: json

      {
        "status": "failed",
        "error": {
          "error_code": "internal-error",
          "message": "Download failed: connection timeout"
        }
      }

Mirror of test_update_status_payload_uses_nested_x_medkit for the
configurations endpoint. The drift test cannot catch reintroduction of
the legacy flat x-medkit-source / x-medkit-node keys because the
ConfigurationMetaData schema is not closed (no additionalProperties:
false), so an explicit positive assertion locks in the new shape.

The test exercises temp_sensor's four declared ROS 2 parameters, which
covers the per-parameter emit path migrated by this PR.
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.

Standardize vendor fields on SOVD-endpoint payloads as nested x-medkit object

2 participants