Pushkal-Gupta opened a new pull request, #74871:
URL: https://github.com/apache/airflow/pull/74871
Depends on #74654. That one is approved and green and needs nothing more
from me, it just needs a committer. This branch is stacked on it and touches
the same two scripts, so until it lands the diff here carries its commits too.
I'll rebase once it merges.
The generated API permission reference lists `PUT /api/v2/backfills`, which
is not a real endpoint, and leaves out six endpoints that are. It also prints
five paths with a `:path` suffix that no client ever calls.
### What's wrong
`airflow-core/docs/security/api_permissions_ref.rst` is generated by
`scripts/ci/prek/extract_permissions.py`, which statically reads the route
decorators under `api_fastapi/core_api/routes/public/`. I compared what it
produces against FastAPI's own `v2-rest-api-generated.yaml`, which covers the
same routers, so the two should agree exactly. The spec has 134 operations. 22
of them are missing from the doc, and 17 rows in the doc have no matching
operation.
Six of the missing ones are backfill endpoints:
```
POST /api/v2/backfills/dry_run
GET /api/v2/backfills/{backfill_id}
GET /api/v2/backfills/{backfill_id}/dag_runs
PUT /api/v2/backfills/{backfill_id}/cancel
PUT /api/v2/backfills/{backfill_id}/pause
PUT /api/v2/backfills/{backfill_id}/unpause
```
`backfills.py` is the one route file that passes the path as a keyword:
```python
@backfills_router.put(
path="/{backfill_id}/pause",
dependencies=[Depends(action_logging()),
Depends(requires_access_backfill(method="PUT"))],
)
```
The extractor only read `decorator.args[0]`, so for all eight routes in that
file the path came out empty and `full_path` ended up as just the router
prefix, `/api/v2/backfills`. Dedup then folded eight operations into three
rows, and one of those three is `PUT /api/v2/backfills`, which doesn't exist,
since PUT only appears on the sub-paths. 126 routes in the repo pass the path
positionally and 8 pass it as `path=`, which is probably why nobody hit this.
The other 16 rows are a smaller thing. Routes declared as
`/{pool_name:path}` get printed with the converter still attached. That's
Starlette's matching syntax rather than part of the URL, and the OpenAPI spec
and the real request path are both `/{pool_name}`. It affects pools, variables,
xcoms and the two state-store paths.
The FAB page comes out of the same extractor, so it has the same holes.
There it also means an operator looking up any of the six backfill endpoints
finds no row telling them which FAB permissions and minimum role it needs.
### The fix
Read the path whichever way the decorator passes it, and drop the converter
so the documented path is the one clients call. If a decorator has no path at
all, raise instead of quietly falling back to the prefix, since that fallback
is what turned a parsing gap into wrong documentation.
After the change the doc covers all 134 operations with nothing left over:
both the 22 missing and the 17 phantom rows go to 0, and the entry count goes
from 137 to 142.
I made the same two changes in `fab_permissions_doc.py`, and also let it
walk `async def` handlers the way `extract_permissions.py` already does.
Nothing there is broken today, since every FAB route is a sync `def` with a
positional path, but it would drop rows silently rather than fail if that
changed. I left its tolerant `continue` for a non-constant path alone rather
than making that raise too, since that part isn't what this PR is about.
### Tests
Unit tests for both helpers, including the `path=` form taken from
`backfills.py` and a decorator with no path at all, plus two end-to-end cases
on synthetic route files for the keyword path and the converter.
The guard I most wanted is an invariant rather than a snapshot: every route
decorator in the source has to produce its own row. Against the current code it
fails with `backfills.py declares 8 routes but the table holds 3 distinct
operations`, so it would have caught this when `backfills.py` was written.
There's a second check that no documented path contains a converter.
All four of those fail without the change. `scripts/tests/` goes from 1684
to 1698 passed, 1 skipped. ruff, mypy and the 43 prek checks that apply to
these five files are clean, and both hooks pass leaving the docs byte-identical
on a second run.
No newsfragment, since this is a generated doc and the tooling behind it,
with no user-facing code change.
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes (please specify the tool below)
--
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]
