Pushkal-Gupta opened a new pull request, #74027:
URL: https://github.com/apache/airflow/pull/74027
Follow-up to #71647, which added `check-openapi-exception-doc-in-sync` but
only inspected statuses a handler raised in its own body. Review on that PR
asked for nested resolution, since most route code reaches its error paths
through a helper:
```
route.handler -> common.validate -> common.not_found raises
HTTPException(404)
```
Those are exactly the statuses a client is most likely to hit and least
likely to have a model for. Following helper calls found six still undeclared:
| Endpoint | Undeclared | Raised by |
| --- | --- | --- |
| `PUT
/dags/{dag_id}/dagRuns/{dag_run_id}/taskInstances/{task_id}/state-store/{key}`
| `400` | `_resolve_expires_at` — a negative `default_retention_days` |
| `GET /ui/calendar/{dag_id}` | `404` | `get_latest_version_of_dag` — the
Dag does not exist |
| `GET /execution/hitlDetails/{task_instance_id}` | `404` |
`_check_hitl_detail_exists` |
| `PATCH /execution/hitlDetails/{task_instance_id}` | `404` |
`_check_hitl_detail_exists` |
| `GET /execution/task-instances/count` | `404` | `_get_group_tasks` —
unknown task group |
| `GET /execution/task-instances/states` | `404` | `_get_group_tasks` —
unknown task group |
The first one is worth calling out: `_resolve_expires_at`'s own docstring
already says "Negative values raise HTTP 400", so that response was documented
in prose and absent from the spec at the same time.
Two public/UI endpoints gain error models their clients had no type for; the
execution API changes have no committed spec artifact.
### Why plain `ast` and not LibCST
Review suggested LibCST's `FullyQualifiedNameProvider`. I measured its two
advantages against this package before adding the dependency, and neither
applies here:
- **Relative imports:** `grep -rE "^from \.+"
airflow-core/src/airflow/api_fastapi` returns nothing. There are none to
resolve.
- **Aliases:** present, but only for model classes (`TaskInstance as TI`),
never for functions that raise `HTTPException`. `ast.ImportFrom` already
exposes `asname`, which is what this resolver keys on.
Meanwhile `libcst.parse_module` is **17.8x slower** than `ast.parse` over
the 68 route files (599 ms vs 34 ms), before counting the imported modules a
call graph also has to parse, and `FullyQualifiedNameProvider` needs a
`MetadataWrapper` with repository-level resolution on top of that. For a hook
that runs on every commit that cost buys no additional resolution power in this
package.
Both approaches find the same six statuses. I went with `ast` and kept the
dependency surface unchanged. Happy to switch if relative imports or aliased
helpers show up here later — the resolver is one function, and the tests pin
the behaviour either way.
### About the check
The conservative posture from #71647 is unchanged — it gates CI, so it
under-reports rather than over-reports:
- Only calls in a handler's **body** are followed. This matters more than it
sounds: a route decorator's `dependencies=[Depends(requires_access_dag(...))]`
resolves to functions that raise `400` on an invalid identifier, and treating
those as the route's own would have produced nine false positives. The router
supplies those statuses, not the route.
- Imports are followed only from the route module itself; inside an imported
module only that module's own definitions are visible.
- Chains stop at `MAX_CALL_DEPTH` (3), and a repeated name terminates the
walk, so mutual recursion cannot loop.
- `401`, `403` and `422` are still never required, and anything unresolvable
is skipped rather than guessed at.
### Verification
- `uv run --project scripts pytest
scripts/tests/ci/prek/test_check_openapi_exception_doc_in_sync.py` — 36 passed
- `prek run check-openapi-exception-doc-in-sync --all-files` — Passed, 0
violations remaining
- `prek run --from-ref main --stage pre-commit` — exit 0 (mypy included)
- `pytest airflow-core/tests/unit/api_fastapi/execution_api` — 669 passed, 4
skipped (postgres/mysql-only)
- Calendar, HITL and task-state-store route tests — 158 passed
- Specs and the UI TypeScript client regenerated; insertions only, no churn
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Code (Opus 5)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
