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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
