Copilot commented on code in PR #64989:
URL: https://github.com/apache/airflow/pull/64989#discussion_r3066482195
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py:
##########
@@ -505,6 +515,63 @@ def test_should_respond_403(self,
unauthorized_test_client):
assert response.status_code == 403
+class TestGetXComEntriesOrderBy(TestXComEndpoint):
+ @pytest.mark.parametrize(
+ ("query_params", "expected_keys"),
+ [
+ # Ascending key order
+ (
+ {"order_by": "key"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ # Descending key order
+ (
+ {"order_by": "-key"},
+ ["gamma_key", "beta_key", "alpha_key"],
+ ),
+ # Ascending timestamp (insertion order)
+ (
+ {"order_by": "timestamp"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ # Descending timestamp (default behavior, reverse insertion)
+ (
+ {"order_by": "-timestamp"},
+ ["gamma_key", "beta_key", "alpha_key"],
+ ),
+ # run_after ordering via joined DagRun column (single run, no
error)
+ (
+ {"order_by": "run_after"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
Review Comment:
The `timestamp` assertions rely on insertion order matching timestamp order;
if timestamps are generated with coarse resolution (or DB truncation), multiple
entries may share the same timestamp and ordering becomes
nondeterministic/flaky. Also, the `run_after` case uses a single DagRun, so it
doesn’t actually validate ordering semantics for `run_after` (it only checks
the endpoint doesn’t error). Consider setting explicit, distinct timestamps (or
freezing time per insert), and for `run_after` create entries across multiple
DagRuns with different `run_after` values so the sort order is meaningfully
exercised.
##########
airflow-core/src/airflow/ui/src/pages/XCom/XCom.tsx:
##########
@@ -152,7 +152,8 @@ export const XCom = () => {
const { dagId = "~", mapIndex = "-1", runId = "~", taskId = "~" } =
useParams();
const { t: translate } = useTranslation(["browse", "common"]);
const { setTableURLState, tableURLState } = useTableURLState();
- const { pagination } = tableURLState;
+ const { pagination, sorting } = tableURLState;
Review Comment:
`const [sort] = sorting;` will throw if `tableURLState.sorting` is
`undefined` (or `null`). To avoid a runtime crash on initial load / older
persisted state, default `sorting` to an empty array before destructuring (e.g.
`const sorting = tableURLState.sorting ?? []`).
```suggestion
const { pagination } = tableURLState;
const sorting = tableURLState.sorting ?? [];
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py:
##########
@@ -559,6 +626,7 @@ class TestPaginationGetXComEntries(TestXComEndpoint):
def test_handle_limit_offset(self, query_params, expected_xcom_ids,
test_client):
for i in range(10):
self._create_xcom(f"TEST_XCOM_KEY{i}", TEST_XCOM_VALUE)
+ query_params["order_by"] = "key"
response =
test_client.get("/dags/~/dagRuns/~/taskInstances/~/xcomEntries",
params=query_params)
Review Comment:
This mutates the parametrized `query_params` dict in-place; pytest reuses
the same object across parametrized invocations, so this change can leak
between cases and make future edits brittle. Prefer creating a new dict
(copy/merge) when adding `order_by`.
```suggestion
request_params = {**query_params, "order_by": "key"}
response =
test_client.get("/dags/~/dagRuns/~/taskInstances/~/xcomEntries",
params=request_params)
```
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py:
##########
@@ -505,6 +515,63 @@ def test_should_respond_403(self,
unauthorized_test_client):
assert response.status_code == 403
+class TestGetXComEntriesOrderBy(TestXComEndpoint):
+ @pytest.mark.parametrize(
+ ("query_params", "expected_keys"),
+ [
Review Comment:
The `timestamp` assertions rely on insertion order matching timestamp order;
if timestamps are generated with coarse resolution (or DB truncation), multiple
entries may share the same timestamp and ordering becomes
nondeterministic/flaky. Also, the `run_after` case uses a single DagRun, so it
doesn’t actually validate ordering semantics for `run_after` (it only checks
the endpoint doesn’t error). Consider setting explicit, distinct timestamps (or
freezing time per insert), and for `run_after` create entries across multiple
DagRuns with different `run_after` values so the sort order is meaningfully
exercised.
##########
airflow-core/src/airflow/ui/src/pages/XCom/XCom.tsx:
##########
@@ -168,6 +169,8 @@ export const XCom = () => {
const runAfterGte = searchParams.get(RUN_AFTER_GTE);
const runAfterLte = searchParams.get(RUN_AFTER_LTE);
+ const orderBy = sort ? [`${sort.desc ? "-" : ""}${sort.id}`] :
["-timestamp"];
Review Comment:
`const [sort] = sorting;` will throw if `tableURLState.sorting` is
`undefined` (or `null`). To avoid a runtime crash on initial load / older
persisted state, default `sorting` to an empty array before destructuring (e.g.
`const sorting = tableURLState.sorting ?? []`).
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_xcom.py:
##########
@@ -505,6 +515,63 @@ def test_should_respond_403(self,
unauthorized_test_client):
assert response.status_code == 403
+class TestGetXComEntriesOrderBy(TestXComEndpoint):
+ @pytest.mark.parametrize(
+ ("query_params", "expected_keys"),
+ [
+ # Ascending key order
+ (
+ {"order_by": "key"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ # Descending key order
+ (
+ {"order_by": "-key"},
+ ["gamma_key", "beta_key", "alpha_key"],
+ ),
+ # Ascending timestamp (insertion order)
+ (
+ {"order_by": "timestamp"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ # Descending timestamp (default behavior, reverse insertion)
+ (
+ {"order_by": "-timestamp"},
+ ["gamma_key", "beta_key", "alpha_key"],
+ ),
+ # run_after ordering via joined DagRun column (single run, no
error)
+ (
+ {"order_by": "run_after"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ # task_display_name ordering (maps to task_id)
+ (
+ {"order_by": "task_display_name"},
+ ["alpha_key", "beta_key", "gamma_key"],
+ ),
+ ],
+ )
+ def test_order_by(self, query_params, expected_keys, test_client):
+ for key in ["alpha_key", "beta_key", "gamma_key"]:
+ self._create_xcom(key, TEST_XCOM_VALUE)
Review Comment:
The `timestamp` assertions rely on insertion order matching timestamp order;
if timestamps are generated with coarse resolution (or DB truncation), multiple
entries may share the same timestamp and ordering becomes
nondeterministic/flaky. Also, the `run_after` case uses a single DagRun, so it
doesn’t actually validate ordering semantics for `run_after` (it only checks
the endpoint doesn’t error). Consider setting explicit, distinct timestamps (or
freezing time per insert), and for `run_after` create entries across multiple
DagRuns with different `run_after` values so the sort order is meaningfully
exercised.
--
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]
