pierrejeambrun commented on code in PR #47440:
URL: https://github.com/apache/airflow/pull/47440#discussion_r1999385213
##########
tests/api_fastapi/core_api/routes/public/test_dags.py:
##########
@@ -228,6 +228,17 @@ class TestGetDags(TestDagEndpoint):
3,
[DAG3_ID, DAG1_ID, DAG2_ID],
),
+ ({"order_by": ["-dag_id", "dag_display_name"]}, 2, [DAG2_ID,
DAG1_ID]),
+ ({"order_by": ["dag_display_name", "-next_dagrun"]}, 2, [DAG1_ID,
DAG2_ID]),
+ ({"order_by": ["last_run_state", "-dag_display_name", "dag_id"]},
2, [DAG1_ID, DAG2_ID]),
+ ({"order_by": ["-last_run_start_date", "dag_display_name",
"next_dagrun", "dag_id"]}, 2, [ DAG1_ID, DAG2_ID]),
+ ({"order_by": ["dag_display_name", "-last_run_state",
"next_dagrun", "dag_id", "last_run_start_date"]}, 2, [DAG1_ID, DAG2_ID]),
+ ({"order_by": ["dag_display_name", "dag_id"]}, 2, [DAG1_ID,
DAG2_ID]),
+ ({"order_by": ["-dag_display_name", "-dag_id"]}, 2, [DAG2_ID,
DAG1_ID]),
+ ({"order_by": ["last_run_state", "dag_id"], "only_active":
False},3, [DAG1_ID, DAG3_ID, DAG2_ID]),
+ ({"order_by": ["-last_run_state", "-dag_id"], "only_active":
False},3, [DAG3_ID, DAG1_ID, DAG2_ID]),
+ ({"order_by": ["-last_run_start_date", "dag_id"], "only_active":
False},3, [DAG3_ID, DAG1_ID, DAG2_ID]),
+ ({"order_by": ["last_run_start_date", "-dag_id"], "only_active":
False},3, [DAG1_ID, DAG3_ID, DAG2_ID]),
Review Comment:
Can you reorganize this and put tests that sort on the same criteria next to
each other. For instance `criteria1` is 'last_run_state', then `criteria2` is
"display_name" so we can more easily compare.
Also for the second sort to take effect you need data where the first
criteria is equal. I'm not sure we have this at the moment.
Basically we need two tests where:
`{"order_by": ["criteria1", "criteria2"]}` will yield a different result
than `{"order_by": ["criteria1", "-criteria2"]}` to highlight that `criteria2`
sorting is actually doing something and taken into account.
##########
airflow/api_fastapi/common/parameters.py:
##########
@@ -171,43 +171,62 @@ def __init__(
self.to_replace = to_replace
def to_orm(self, select: Select) -> Select:
+ MAX_SORT_PARAMS = 5
if self.skip_none is False:
raise ValueError(f"Cannot set 'skip_none' to False on a
{type(self)}")
if self.value is None:
self.value = self.get_primary_key_string()
-
- lstriped_orderby = self.value.lstrip("-")
- column: Column | None = None
- if self.to_replace:
- replacement = self.to_replace.get(lstriped_orderby,
lstriped_orderby)
- if isinstance(replacement, str):
- lstriped_orderby = replacement
- else:
- column = replacement
-
- if (self.allowed_attrs and lstriped_orderby not in self.allowed_attrs)
and column is None:
+
+ order_by_list = self.value
+ if len(order_by_list) > MAX_SORT_PARAMS:
raise HTTPException(
- 400,
- f"Ordering with '{lstriped_orderby}' is disallowed or "
- f"the attribute does not exist on the model",
+ 400,
+ f"Ordering with more than {MAX_SORT_PARAMS} parameters is not
allowed. Provided: {order_by_list}"
)
- if column is None:
- column = getattr(self.model, lstriped_orderby)
-
- # MySQL does not support `nullslast`, and True/False ordering depends
on the
- # database implementation.
- nullscheck = case((column.isnot(None), 0), else_=1)
+ order_by_columns = []
+
+ for order_by in order_by_list:
+ lstriped_orderby = order_by.lstrip("-")
+ column: Column | None = None
+ if self.to_replace:
+ replacement = self.to_replace.get(lstriped_orderby,
lstriped_orderby)
+ if isinstance(replacement, str):
+ lstriped_orderby = replacement
+ else:
+ column = replacement
+
+ if (self.allowed_attrs and lstriped_orderby not in
self.allowed_attrs) and column is None:
+ raise HTTPException(
+ 400,
+ f"Ordering with '{lstriped_orderby}' is disallowed or "
+ f"the attribute does not exist on the model",
+ )
+ if column is None:
+ column = getattr(self.model, lstriped_orderby)
+
+ # MySQL does not support nullslast, and True/False ordering
depends on the
+ # database implementation.
+ nullscheck = case((column.isnot(None), 0), else_=1)
+
+ if order_by[0] == "-":
+ order_by_columns.append((nullscheck, column.desc()))
+ else:
+ order_by_columns.append((nullscheck, column.asc()))
# Reset default sorting
select = select.order_by(None)
- primary_key_column = self.get_primary_key_column()
-
- if self.value[0] == "-":
- return select.order_by(nullscheck, column.desc(),
primary_key_column.desc())
+ if order_by_columns:
+ primary_key_column = self.get_primary_key_column()
+ primary_key_sort = primary_key_column.asc()
+ order_by_columns.append((case((primary_key_column.isnot(None), 0),
else_=1), primary_key_sort))
Review Comment:
This is done for `order_by_columns` but not in the other `if` clause. Also
primary key shouldn't be nullable, so that's not needed to add nullable
handling for this column.
##########
airflow/api_fastapi/common/parameters.py:
##########
@@ -171,43 +171,62 @@ def __init__(
self.to_replace = to_replace
def to_orm(self, select: Select) -> Select:
+ MAX_SORT_PARAMS = 5
if self.skip_none is False:
raise ValueError(f"Cannot set 'skip_none' to False on a
{type(self)}")
if self.value is None:
self.value = self.get_primary_key_string()
-
- lstriped_orderby = self.value.lstrip("-")
- column: Column | None = None
- if self.to_replace:
- replacement = self.to_replace.get(lstriped_orderby,
lstriped_orderby)
- if isinstance(replacement, str):
- lstriped_orderby = replacement
- else:
- column = replacement
-
- if (self.allowed_attrs and lstriped_orderby not in self.allowed_attrs)
and column is None:
+
+ order_by_list = self.value
+ if len(order_by_list) > MAX_SORT_PARAMS:
raise HTTPException(
- 400,
- f"Ordering with '{lstriped_orderby}' is disallowed or "
- f"the attribute does not exist on the model",
+ 400,
+ f"Ordering with more than {MAX_SORT_PARAMS} parameters is not
allowed. Provided: {order_by_list}"
)
- if column is None:
- column = getattr(self.model, lstriped_orderby)
-
- # MySQL does not support `nullslast`, and True/False ordering depends
on the
- # database implementation.
- nullscheck = case((column.isnot(None), 0), else_=1)
+ order_by_columns = []
+
+ for order_by in order_by_list:
+ lstriped_orderby = order_by.lstrip("-")
+ column: Column | None = None
+ if self.to_replace:
+ replacement = self.to_replace.get(lstriped_orderby,
lstriped_orderby)
+ if isinstance(replacement, str):
+ lstriped_orderby = replacement
+ else:
+ column = replacement
+
+ if (self.allowed_attrs and lstriped_orderby not in
self.allowed_attrs) and column is None:
+ raise HTTPException(
+ 400,
+ f"Ordering with '{lstriped_orderby}' is disallowed or "
+ f"the attribute does not exist on the model",
+ )
+ if column is None:
+ column = getattr(self.model, lstriped_orderby)
+
+ # MySQL does not support nullslast, and True/False ordering
depends on the
+ # database implementation.
+ nullscheck = case((column.isnot(None), 0), else_=1)
+
+ if order_by[0] == "-":
+ order_by_columns.append((nullscheck, column.desc()))
+ else:
+ order_by_columns.append((nullscheck, column.asc()))
# Reset default sorting
select = select.order_by(None)
- primary_key_column = self.get_primary_key_column()
-
- if self.value[0] == "-":
- return select.order_by(nullscheck, column.desc(),
primary_key_column.desc())
+ if order_by_columns:
+ primary_key_column = self.get_primary_key_column()
+ primary_key_sort = primary_key_column.asc()
+ order_by_columns.append((case((primary_key_column.isnot(None), 0),
else_=1), primary_key_sort))
Review Comment:
Also maybe we should respect the primary sort criteria order:
- If primary sort is ascending then the last sort on primary_key_column
should be ascending.
- If primary sort is descending then the last sort on primary_key_column
should also be descending. (this will fix some tests I believe)
(here we always to `asc` for the primary_key_column, even if the primary
sort is descending, which can feel a little weird)
--
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]
