You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Pushkal-Gupta (via GitHub)" <gi...@apache.org> on 2026/10/01 13:08:55 UTC
[PR] Document HTTP statuses that route helpers raise [airflow]
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "Pushkal-Gupta (via GitHub)" <gi...@apache.org>.
Pushkal-Gupta commented on PR #74027:
URL: https://github.com/apache/airflow/pull/74027#issuecomment-5950491115
Both red checks look unrelated to this PR. Details in case it saves anyone a dig:
**`Static checks` — `check-go-sdk-generated-drift`**
```
ERROR: out of date: go-sdk/pkg/execution/genmodels/models.gen.go,
discriminators.gen.go, defaults.gen.go
```
That hook's `files:` pattern covers the serialization schemas and `go-sdk/**` generated files, and it runs with `pass_filenames: false`, so it regenerates from the committed schema regardless of what a PR changes. This PR touches none of those paths — the drift is in `ImportErrors`, which came in with #73853. #74046 fails the same hook, so it needs a regeneration on main rather than anything here.
**`WebKit UI e2e tests`**
Chromium and Firefox both pass on this same commit; only WebKit fails, on a backfill column lookup and two menu-click timeouts:
```
Error: Column "Reprocess Behavior" not found.
Available columns: ..., backfill.reprocessBehavior, ...
Error: locator.click: Timeout 3000ms exceeded
```
There is no UI, i18n or backfill code in this PR — the only UI files are the generated client, whose whole diff is the 400/404 response types for the two endpoints above. Happy to rebase if that helps it re-run.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "Pushkal-Gupta (via GitHub)" <gi...@apache.org>.
Pushkal-Gupta commented on PR #74027:
URL: https://github.com/apache/airflow/pull/74027#issuecomment-6034771517
Thanks for the approval!
I had a dig before re-running, and the static check turned out not to be a UI thing at the time. The only hook failing was `check-go-sdk-generated-drift`, on the generated Go models, which came in with the schema change in #73853 and had already been fixed on main by #74028.
A re-run wouldn't have picked that up either, which took me a bit to work out. Re-runs reuse the merge commit from the original event, so it would just rebuild the same tree. Our run started about ten hours before #74028 landed, so the fix wasn't there yet whichever way we ran it. Rebasing was the only way in.
So: rebased onto main. The go-sdk drift is gone and WebKit passed this time.
Static checks is still red, but it's a different hook now and still not from this PR. `ts-compile-lint-ui` reports two `unicorn/prefer-logical-operator-over-ternary` errors in `useAdvancedSearch.ts` and `useRecentTaskStateCounts.tsx`, both of which landed on main yesterday in #74289 and #69619. Neither file is in this diff. I've put a fix up separately in #74402 so it isn't tangled up with the OpenAPI change — one of the two rewrites reads worse than the ternary it replaces, so it could use a UI opinion.
This PR itself is green locally on the new base: 0 violations, 36 hook tests passing, mypy clean, and regenerating the specs and UI client produced no diff.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "Pushkal-Gupta (via GitHub)" <gi...@apache.org>.
Pushkal-Gupta commented on PR #74027:
URL: https://github.com/apache/airflow/pull/74027#issuecomment-6040714596
I don't have merge permissions on the repo, so I can't take it from here. Would one of you be able to merge when you get a chance? It's approved, and the lint fix it was waiting on went in as #74402.
Thanks for the reviews, and for the branch update earlier.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi merged PR #74027:
URL: https://github.com/apache/airflow/pull/74027
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "bbovenzi (via GitHub)" <gi...@apache.org>.
bbovenzi commented on PR #74027:
URL: https://github.com/apache/airflow/pull/74027#issuecomment-6024542366
Let's just re-run the UI formatting checks to fix the static checks CI
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Document HTTP statuses that route helpers raise [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #74027:
URL: https://github.com/apache/airflow/pull/74027#issuecomment-6037081237
Main CI fix was merged. Updated to branch to restart the CI with that fix.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org