You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "henry3260 (via GitHub)" <gi...@apache.org> on 2026/03/12 08:46:12 UTC
[PR] Add order_by parameter to GET /permissions for pagination consistency [airflow]
henry3260 opened a new pull request, #63418:
URL: https://github.com/apache/airflow/pull/63418
`GET /permissions` lacked the `order_by `parameter present on other list endpoints (e.g. GET /roles), causing non-deterministic pagination.
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [ ] Yes (please specify the tool below)
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Add order_by parameter to GET /permissions for pagination consistency [airflow]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #63418:
URL: https://github.com/apache/airflow/pull/63418#discussion_r2923138501
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py:
##########
@@ -140,9 +140,12 @@ def patch_role(
dependencies=[Depends(requires_fab_custom_view("GET", permissions.RESOURCE_ROLE))],
)
def get_permissions(
+ order_by: str = Query(
+ "id", description="Fields to order by, separated by comma. Prefix with '-' for descending."
Review Comment:
The `order_by` description says fields can be comma-separated, but `build_ordering()` only supports a single field name (optionally prefixed with `-`). This is misleading and will cause `order_by=id,action_id` (etc.) to fail with 400; update the description to reflect the actual supported format (single field).
```suggestion
"id", description="Field to order by. Prefix with '-' for descending."
```
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py:
##########
@@ -140,9 +140,12 @@ def patch_role(
dependencies=[Depends(requires_fab_custom_view("GET", permissions.RESOURCE_ROLE))],
)
def get_permissions(
+ order_by: str = Query(
+ "id", description="Fields to order by, separated by comma. Prefix with '-' for descending."
+ ),
limit: int = Depends(get_effective_limit()),
offset: int = Query(0, ge=0, description="Number of items to skip before starting to collect results."),
):
Review Comment:
For consistency with the other route handlers in this module (which have explicit return type annotations), consider annotating this handler as returning `PermissionCollectionResponse` (it already declares `response_model=PermissionCollectionResponse`). This improves typing and readability.
```suggestion
) -> PermissionCollectionResponse:
```
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/routes/roles.py:
##########
@@ -140,9 +140,12 @@ def patch_role(
dependencies=[Depends(requires_fab_custom_view("GET", permissions.RESOURCE_ROLE))],
)
def get_permissions(
+ order_by: str = Query(
+ "id", description="Fields to order by, separated by comma. Prefix with '-' for descending."
+ ),
Review Comment:
With the new `order_by` parameter, this endpoint can now return a 400 (invalid/disallowed ordering) via `build_ordering()`, but the route’s `responses=create_openapi_http_exception_doc(...)` list does not include 400. Please add 400 so the OpenAPI docs match actual behavior.
##########
providers/fab/src/airflow/providers/fab/auth_manager/api_fastapi/openapi/v2-fab-auth-manager-generated.yaml:
##########
@@ -409,6 +409,16 @@ paths:
- OAuth2PasswordBearer: []
- HTTPBearer: []
parameters:
+ - name: order_by
+ in: query
+ required: false
+ schema:
+ type: string
+ description: Fields to order by, separated by comma. Prefix with '-' for
+ descending.
+ default: id
+ title: Order By
+ description: Fields to order by, separated by comma. Prefix with '-' for descending.
Review Comment:
The OpenAPI spec describes `order_by` as supporting comma-separated fields, but the implementation only supports a single field (optionally prefixed with `-`). Update this description (likely via fixing the FastAPI `Query` description and regenerating this spec) so the contract matches behavior.
```suggestion
description: Field to order by. Prefix with '-' for descending.
default: id
title: Order By
description: Field to order by. Prefix with '-' for descending.
```
--
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] Add order_by parameter to GET /permissions for pagination consistency [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #63418:
URL: https://github.com/apache/airflow/pull/63418
--
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