Keep OpenAPI error responses in sync with the statuses routes raise - #71647
Keep OpenAPI error responses in sync with the statuses routes raise#71647Pushkal-Gupta wants to merge 2 commits into
Conversation
`create_openapi_http_exception_doc(...)` feeds the `responses=` block that the generated spec — and every client built from it — uses to model error responses, but nothing ties that list to the statuses a handler actually raises. The two drift apart silently, and the same drift has had to be found and patched by hand four times (apache#67570, apache#67571, apache#70992, apache#71011). A static check keeps them together, so the next divergence fails in CI instead of shipping a spec that omits a response the API really returns.
boring-cyborg
Bot
added
area:API
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide
|
pierrejeambrun
left a comment
pierrejeambrun
left a comment
There was a problem hiding this comment.
LGTM, a couple of suggestions before we can merge.
jason810496
left a comment
jason810496
left a comment
There was a problem hiding this comment.
Thanks for the improvement.
| print(message) | ||
| return 1 | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
It think we should leverge LibCST to support the nested fastapi.HTTPException resolution.
The current implementation can't handle the following example:
route.handler -> common.validate -> common.not_found raises HTTPException(404)
More context in:
LibCST — not a complete call-graph package, but probably the best building block here. Its FullyQualifiedNameProvider resolves aliases and relative imports across a repository, and Airflow already depends on LibCST for scripts. LibCST name resolution
(https://libcst.readthedocs.io/en/latest/_modules/libcst/metadata/name_provider.html#FullyQualifiedNameProvider)
There was a problem hiding this comment.
Agreed this is the real limitation, and the LibCST pointer is the right one —
FullyQualifiedNameProvider is the piece that makes cross-module resolution
tractable, and the dependency is already there.
I'd like to propose it as a follow-up PR rather than folding it in here, for two
reasons:
- Scope. It changes the check from a single-file AST pass to inter-procedural
analysis across the repo, which brings a call graph, cycle handling, a depth
bound, and a per-file cost that has to stay inside the pre-commit budget. That
deserves its own review rather than riding along with a set of declaration
fixes. - It interacts with the router fix above. Now that inheritance works on
VersionedAPIRouter, following calls will move the finding set in both
directions — new true positives from helpers like_resolve_asset_id_by_name,
but also statuses that turn out to be already covered by a router. Landing the
corrected baseline first makes that delta measurable.
The current behaviour is documented as under-reporting rather than over-reporting,
so this is strictly better than the status quo today and does not block the deeper
version.
Happy to open the follow-up issue and take it, if that split works for you.
ROUTER_CLASSES omitted VersionedAPIRouter, so router-level `responses=` was invisible on every Cadwyn-based execution API route, and three statuses those routers already declare were reported as undeclared. prek executes hook scripts directly, so the script needs its executable bit to run at all.
3 participants
Follow-up to #67570, #67571, #70992 and #71011, which each found and patched instances of
the same drift by hand: a route handler raises an HTTP status that
create_openapi_http_exception_doc(...)never declares, so the generated spec — and everyclient built from it — has no model for a response the API really returns.
The helper's own docstring names the problem:
This adds the check that makes it automatic, so the next divergence fails in CI instead of
shipping. Running it over
api_fastapi/**/routes/found 15 statuses still undeclared afterthe four manual passes:
POST /connections/test400GET /dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/logs/{try_number}400POST /variables404GET /ui/next_run_assets/{dag_id}404GET /ui/partitioned_dag_runs404dag_idthat does not existGET /ui/pending_partitioned_dag_run/{dag_id}404GET /ui/teams403GET /execution/asset-events/by-asset400namenoruriis suppliedGET /execution/store/asset/by-name/value404GET /execution/store/asset/by-uri/value404PATCH /execution/hitlDetails/{task_instance_id}409PATCH /execution/task-instances/{task_instance_id}/run500GET /execution/store/ti/{task_instance_id}/{key}404HEAD /execution/xcoms/{dag_id}/{run_id}/{task_id}/{key}400map_indexis passed to a HEAD requestPOST /execution/xcoms/{dag_id}/{run_id}/{task_id}/{key}400The regenerated spec and UI client show the effect: seven public/UI endpoints gain error
models their clients previously had no type for.
About the check
It is deliberately conservative — it is meant to be trustworthy enough to gate CI, so it
under-reports rather than over-reports, and stays silent whenever it cannot see the whole
picture:
HTTPException(...)raised in the handler's own body counts. Statuses raised by ashared dependency or a service helper are not required to be declared.
422is never required (FastAPI documents validation errors itself), and neither are401/403— routers contribute those wholesale via their auth dependencies, and therouter that does so is often built in another module (
routes/public/__init__.pydeclares both for every public route).
responses=onAPIRouter(...)(as inexecution_api/routes/xcoms.py) counts as declared.responses=blocks that are not aliteral
create_openapi_http_exception_doc([...])call or mapping, are skipped ratherthan guessed at.
The
GET /ui/teams403above sits outside what the check enforces, for the401/403reason just described; it is included because it is a real gap in
_private_ui.yamlthatthe same audit turned up.
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines