pierrejeambrun commented on code in PR #64163:
URL: https://github.com/apache/airflow/pull/64163#discussion_r2989283899
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/tasks.py:
##########
@@ -51,10 +56,21 @@ def get_tasks(
order_by: str = "task_id",
) -> TaskCollectionResponse:
"""Get tasks for DAG."""
+ sort_key = order_by.lstrip("-")
+ if sort_key not in _VALID_TASK_ORDER_BY_FIELDS:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ f"Invalid order_by parameter: '{sort_key}'. "
+ f"Valid fields are: {',
'.join(sorted(_VALID_TASK_ORDER_BY_FIELDS))}",
Review Comment:
For other endpoints we return:
```
f"Ordering with '{lstriped_orderby}' is disallowed or "
f"the attribute does not exist on the model",
```
We should probably align here.
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/tasks.py:
##########
@@ -33,6 +32,12 @@
tasks_router = AirflowRouter(tags=["Task"], prefix="/dags/{dag_id}/tasks")
+_VALID_TASK_ORDER_BY_FIELDS = {
+ field_name
+ for field_name, field_info in TaskResponse.model_fields.items()
+ if field_name not in ("class_ref", "template_fields",
"downstream_task_ids", "params")
+}
+
Review Comment:
Similarly to other order_by params for other endpoint, I would manually
write the authorized values here:
```
order_by: Annotated[
SortParam,
Depends(
SortParam(["id", "version_number", "bundle_name",
"bundle_version"], DagVersion).dynamic_depends()
),
],
```
##########
airflow-core/src/airflow/api_fastapi/core_api/routes/public/tasks.py:
##########
@@ -51,10 +56,21 @@ def get_tasks(
order_by: str = "task_id",
) -> TaskCollectionResponse:
"""Get tasks for DAG."""
+ sort_key = order_by.lstrip("-")
+ if sort_key not in _VALID_TASK_ORDER_BY_FIELDS:
+ raise HTTPException(
+ status.HTTP_400_BAD_REQUEST,
+ f"Invalid order_by parameter: '{sort_key}'. "
+ f"Valid fields are: {',
'.join(sorted(_VALID_TASK_ORDER_BY_FIELDS))}",
+ )
dag = get_latest_version_of_dag(dag_bag, dag_id, session)
try:
- tasks = sorted(dag.tasks, key=attrgetter(order_by.lstrip("-")),
reverse=(order_by[0:1] == "-"))
- except AttributeError as err:
+ tasks = sorted(
+ dag.tasks,
+ key=lambda task: (getattr(task, sort_key) is None, getattr(task,
sort_key)),
+ reverse=(order_by[0:1] == "-"),
+ )
+ except (AttributeError, TypeError) as err:
Review Comment:
TypeError can never happen, because we compare None with None and not None
with not None I believe. (Thanks to the 'is None' first value in the tuples)
AttributeError cannot happen because of the previous check `if sort_key not
in _VALID_TASK_ORDER_BY_FIELDS:` valid order_by have existing members.
--
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]
