Derive the OpenAPI document from the code instead of declaring it alongside - #591
Draft
bburda wants to merge 17 commits into
Draft
Derive the OpenAPI document from the code instead of declaring it alongside#591bburda wants to merge 17 commits into
bburda wants to merge 17 commits into
Conversation
Document-wide invariants no single handler owns: unique operation identity, declared tags, resolvable refs, no malformed path keys. A violation here is a client-visible defect even when every endpoint behaves correctly.
…turn type 45 operations declared a 2xx they cannot return, because the status was attached by hand at the registration while the handler's return type said otherwise. Created<T> and Accepted<T> carry the status in the type, so the registry reads it from the signature and the document cannot drift from the wire. status_payload_t unwraps to the schema, the serializer and the static assertions. A 202 labelled "No content" is now unrepresentable rather than merely corrected.
28 routes could answer 501 and none of them said so, so a client met a refusal the document never mentioned. gated_on() ties the guard and the declaration to one expression: a route that can be gated off publishes its 501, and a gate with an empty predicate now fails closed instead of segfaulting.
Twelve sites attached a header no operation declared. response_header() records it where it is set; declare_derived_response static_asserts that a 201 or 202 carries its Location, so publishing the header without sending it stops compiling. validate_completeness() runs at startup and is asserted by the suite, so a missing declaration is reported rather than silently dropped.
…non-JSON route returns lock_guarded() marks the writes that answer 409 to a client without the lock, and the expected set is pinned so dropping a marker turns the suite red. Binary downloads stop emitting format: binary, which is not valid in OpenAPI 3.1, and the schema-less exemption now requires a non-JSON media type. The 416 cpp-httplib answers before routing is declared where it is reachable.
A created resource is answered with its Location, a fault-trigger route that cannot serve says 501 rather than a bare error, and a parameter conversion failure answers 400 instead of a status that told the caller nothing.
get_tracked_goal was scoped to no entity, so an execution started on one app was reachable from another, and list_executions resolved only two of the four entity types. Both now resolve the entity that owns the execution. Kept as its own commit: this changes behaviour, and reverting it must not take the documentation work with it.
…keep Location resolvable Script handlers guessed the collection from the request path instead of reading the segment the router matched. Separately, to_regex_path appends "/?$", so a request with a trailing slash routes successfully and req.path() + "/" + id then yields a double slash whose Location answers 404. Nine sites built a URI that way; canonical_request_path and child_resource_path are now the single place that knows about the anchor.
15 schemas were unreachable from any operation and 9 operations published a body with no content at all. Responses, request bodies, error bodies, SSE frames and the update payloads SOVD already defines are now bound to the types the handlers use, with success_schema() narrowing only the declared schema where a handler must keep returning free-form JSON so peer vendor keys survive. Two wire defects surfaced doing it: the vendor-error sentinel was emitted without its vendor_code on one path, so a client could not identify the error; and the error-code table claimed completeness while its check matched any mention anywhere in the guide. The check now reads the table itself, and the emitter scan covers headers and in-tree plugins.
…a client must send The document advertised entity resource collections the gateway does not serve and omitted several it does; the correspondence is now exact in both directions for all five types. FieldConstraints publishes the bounds the handlers enforce, every operation carries a description, and non-trivial request bodies carry an example. A published bound is only added where every route carrying it rejects an over-long value unconditionally - DELETE on a configuration did not, and now does.
…osing bags silently The document published a maximum of 256 while FaultManagerNode enforced 128, so a code in between produced a server error on a value the document calls valid. The bound is now 256 on both sides; every fault_code column is unconstrained TEXT and every IDL field an unbounded string, so nothing downstream caps it. Raising it exposed silent loss: fault_<code>_<timestamp> pushed the rosbag directory name past NAME_MAX, the exception was caught and only logged, and the bag was never written. The name is bounded and carries a digest of the whole code, so two codes sharing every kept byte still get separate directories. Separately, get_fault decided 404 against 503 by substring-matching the store's message, so a validation refusal from the fault manager reached the client as a server error. The transport now records whether it got a response at all, and the client-error side is the default so a 503 must be asked for.
…ion table, project the sub-documents The /docs routes and the graph provider's routes were served but undocumented. Folding them in needed OperationDesc to carry tag, operationId and role, and a gateway-stamped marker for the coverage sweep, whose recorder attaches at the registry mount point and structurally cannot see a plugin route. The RBAC table is now generated from the route registrations rather than maintained as a literal, with a short residual list for what is mounted outside the registry. Enforcement did not move: check_authorization still fails closed where it always did, because docs, Swagger UI and plugin routes would otherwise lose their rule. The <entity-path>/docs sub-documents are projected from the real document instead of rebuilt by hand. The hand-written producers had been shipping dangling $refs - twelve in a component's document - so a client generating code from one got references to nothing. The cache now stores the serialized form rather than a parsed tree, cutting resident growth by about 40% and the per-hit copy by 70%. Also documents the derivation rule and the tier each mechanism reaches, and adds a check resolving every test the design docs cite against the test tree.
…implicit Pre-existing clang-tidy findings in packages this work does not otherwise touch. The gate is mandatory, and "pre-existing" is not a reason to leave it red.
A whole-branch pass found what reviewing one slice at a time cannot: three surfaces sit outside RouteRegistry::to_openapi_paths() and each published something the gateway does not honour. Per-operation security and the 401/403 declarations were keyed on auth.enabled while enforcement asks the policy, so under require_auth_for: write the document told a client to hold a token for 136 GETs that admit everyone, and under none it asserted an RBAC posture the gateway does not have. Both now key on the policy. Plugin-served operations omitted the 401/403 the middleware does emit on them - the same pre-routing argument the code already made for 416. With locking off, 44 operations still advertised locks while the same gateway's root said locking: false. GET /api/v1 and /docs disagreed in both directions, and nothing compared them. A hand-written array's completeness is now a compiler check rather than a claim the design docs made for it. The cache-derived items in the sub-documents published a request body the handler rejects and a 200 body the gateway never returns; they now inherit both from the templated sibling that names the same route, and are discarded rather than published raw when that sibling cannot be found.
…inted The package set no CMAKE_EXPORT_COMPILE_COMMANDS, so it produced no compile_commands.json and clang-tidy run standalone could not resolve the gateway headers graph_provider_plugin_exports.cpp includes - reporting them as missing on a file that compiles clean. Nine other packages already set it; this one was the gap, and it only surfaced now because this work touches that file.
declare_parameter<int> yields int64_t, so narrowing to int before the range check let a value past INT_MAX truncate into range and pass it - severity_floor would have been accepted, and max_tracked_nodes could wrap to a small positive bound. Both now clamp as int64_t and narrow afterwards. The -Wconversion warnings these raised are pre-existing on main; this branch already touches the file, and the gate is zero warnings.
bburda
force-pushed
the
feat/openapi-derivation
branch
from
August 5, 2026 07:55
5e5b36e to
9429c5c
Compare
UpdateManager runs prepare/execute on their own std::async threads and each calls ResourceChangeNotifier::notify() once the backend returns, so a task can still be inside the backend when the gateway tears down. ~UpdateManager is what joins those tasks, but update_mgr_ is declared before resource_change_notifier_ and C++ destroys members in reverse declaration order, so the notifier was freed first. A task waking up afterwards wrote to freed memory - ASan reported heap-use-after-free on the fetch_add that opens notify(), from run_prepare() on a std::async thread. The counter is the function's own lifetime guard, so it cannot help here: it only protects a call already in progress, not one that starts after the object is gone. Give UpdateManager an explicit shutdown() that stops accepting work and joins every in-flight task, and call it from ~GatewayNode before the notifier is shut down. Draining at a defined point makes the ordering independent of member declaration order. The destructor delegates to shutdown() so standalone use keeps the previous join behaviour, and moving the futures out makes repeat calls a no-op. The stopped_ check in the three start_*() entry points moves under mutex_. Previously it was read before the lock, leaving a window where a call could pass the check and then launch after shutdown() had already snapshotted the futures, escaping the drain. Covered by test_openapi_response_drift under ASan, which is what first drove this path, plus unit tests pinning that shutdown() blocks while a task is in flight and is safe to call twice.
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
1 participant
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.
Pull Request
Summary
The generated OpenAPI document asserted a great deal the gateway does not honour, and omitted a great deal it does. The common cause was that facts about a route were declared separately from the handler implementing them, so the two drifted with nothing checking.
This derives what can be derived, and where it cannot be derived, declares it once at a seam that also does the work.
The governing rule, now stated in
design/openapi_derivation.rst: if a fact about a route can be derived from the handler, it must not be declared separately; where it cannot, the declaration lives at a seam that also does the work.What now derives itself
Created<T>,Accepted<T>,status_payload_t) rather than a hand-attached number.check_authorizationstill fails closed where it always did, because docs, Swagger UI and plugin routes are registered outside the registry and would lose their rule.<entity-path>/docssub-documents are projected from the real document instead of rebuilt by hand.securityis published only where the deployment actually checks it, keyed on the auth policy rather than onauth.enabled.Measured against a running gateway
format: binary(invalid in OpenAPI 3.1)$refin/docssub-documentsVerification probed all 239 documented operations against live gateways in five configurations - feature gates on and off, auth off and all three
require_auth_forvalues, plugin loaded and not - and found no status the gateway emits that the document does not declare.Defects found and fixed along the way
These were not documentation problems:
503, because the classification matched a substring of the error message. Replaced with a two-state outcome set at the transport's no-response paths, defaulting to the client-error side so a server error must be asked for.fault_codewas published with a maximum of 256 while the fault manager enforced 128; values in between produced a server error. Bounds reconciled.NAME_MAX, the exception was caught and only logged, and the bag was never written. The name is now bounded and carries a digest so truncated codes stay distinguishable.401/403/429answered before the request body was read stranded the payload and poisoned the keep-alive connection, so the next request on it received a spurious400.DELETE /{entity}/configurations/{config_id}did not enforce the length bound its siblings enforce and the document published./docssub-documents published a request body the handler rejects, and a 200 body the gateway never returns.Enforcement, so this does not come back
$refs, no phantom 2xx, every advertised collection served, every operation described, no role published where none is enforced.Issue
Type
Testing
Gateway unit suite and the integration suite, plus contract suites added here:
test_openapi_contract,test_openapi_callability,test_openapi_error_coverage,test_openapi_response_drift,test_rbac_contract,test_auth_policy_contract,test_locking_disabled_contract.Reviewers can reproduce the central claim directly:
Re-verified after rebasing onto current
main(25 commits had landed since thiswork started): gateway builds with zero warnings, 2678 unit tests and
137 contract cases pass, no orphaned processes left behind. The one conflict
was the per-target coverage list, which
mainreplaced with a directory-scopemodule that instruments every target automatically.
Two tests are red and are not caused by this branch:
test_external_app_fault_rollupfails roughly two runs in three on an idle machine. Diagnosed here:RosbagCapture::start()runs from theFaultManagerNodeconstructor while the node's graph cache is still empty, so nothing is buffered for the first 500 ms and a fault confirmed in that window gets no bag. Written up on [BUG] A fault confirmed right after a post-fault recording window gets no rosbag #574, which is a different entry path into the same sink; left unfixed by owner decision.test_opcua_securedlaunches the fault manager with no parameters, so storage defaults to/var/lib/ros2_medkit, which the test runner cannot create;create_storage()rethrows and the node aborts before its service appears. One launch parameter fixes it; out of scope here.Known limitations
Stated rather than left to be discovered:
<entity-path>/docscarry no payload schema on any gateway: all fourTopicDataconstruction sites push an empty type, so the schema-generating branch never runs in production. The items still carry the SOVD extensions and the correct write envelope./docsis silently unreachable - route registration order puts the docs catch-all first, with no warning at registration./docssub-document cache bounds entries rather than bytes; caching the serialized form rather than the parsed tree cut resident growth by about 40% and the per-hit copy by about 70%, but the entry bound is still the wrong unit.<entity-path>/docsanswers 404 for four collections that are served and advertised (locks,scripts,fault-triggers,status), and 200 with an empty document for two nested paths that are not served.A tooling gap this surfaced
The plugin packages do not set
CMAKE_EXPORT_COMPILE_COMMANDS, so they produceno
compile_commands.jsonand the pre-push clang-tidy hook cannot resolve thegateway headers they include - reporting them as missing on files that compile
clean. Nine other packages set it. It bites whoever next touches a file in
ros2_medkit_graph_providerorros2_medkit_graph_watchdog; both are outsidethis change's subject, so it is reported rather than fixed here.
Checklist