Skip to content

Commit 7ab6f8a

Browse files
Gayathri Srividya RajavarapuGayathri Srividya Rajavarapu
authored andcommitted
Fix cursor encoding for column-form SortParam to_replace
1 parent b311e6f commit 7ab6f8a

4 files changed

Lines changed: 112 additions & 19 deletions

File tree

airflow-core/src/airflow/api_fastapi/common/parameters.py

Lines changed: 17 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -639,24 +639,29 @@ def row_value(self, row: Any, name: str) -> Any:
639639
Extract the sort-key value for ``name`` from a result row.
640640
641641
Resolves the accessor through ``to_replace`` for string aliases
642-
(e.g. ``{"dag_run_id": "run_id"}``); otherwise reads ``name`` directly.
642+
(e.g. ``{"dag_run_id": "run_id"}``). For column-form mappings
643+
(e.g. ``{"run_after": DagRun.run_after}``), resolves through the
644+
primary model's attribute so association proxies can still be used
645+
for cursor values. Raises ``NotImplementedError`` when the model
646+
exposes no such attribute rather than emitting a ``None`` cursor token.
643647
"""
644648
if self.to_replace:
645649
replacement = self.to_replace.get(name)
646650
if isinstance(replacement, str):
647651
return getattr(row, replacement, None)
648652
if replacement is not None and not isinstance(replacement, list):
649-
# TODO: Column-form ``to_replace`` (e.g. ``{"last_run_state": DagRun.state}``)
650-
# isn't supported for cursor pagination — no endpoint that uses cursor
651-
# pagination needs it today. When one does, decide how the row exposes the
652-
# value (projected label on the SELECT, eagerly loaded relationship, etc.)
653-
# and wire it up here. Raising loudly so a future caller doesn't silently
654-
# get ``None`` cursor tokens.
655-
raise NotImplementedError(
656-
f"Cursor pagination does not support column-form ``to_replace`` mapping for "
657-
f"``{name}``. Use a string alias in ``to_replace`` or sort by a primary-model "
658-
f"attribute."
659-
)
653+
# Column-form mapping resolves through the primary model's attribute,
654+
# often an association proxy onto the joined entity
655+
# (``TaskInstance.run_after`` -> ``dag_run.run_after``). Fail loudly if the
656+
# model exposes no such attribute, rather than emitting a ``None`` cursor token.
657+
try:
658+
return getattr(row, name)
659+
except AttributeError:
660+
raise NotImplementedError(
661+
f"Cursor pagination cannot resolve column-form ``to_replace`` for "
662+
f"``{name}``: the primary model exposes no such attribute. Add an "
663+
f"association proxy, use a string alias, or sort by a primary-model column."
664+
)
660665
# List-form replacements are expanded in _resolve() into individual entries
661666
# each using the column's own ORM key as attr_name, so ``name`` at this point
662667
# is already a concrete model attribute (e.g. ``_rendered_map_index`` or

airflow-core/tests/unit/api_fastapi/common/test_cursors.py

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import base64
2121
import uuid
2222
from datetime import datetime, timezone
23+
from types import SimpleNamespace
2324
from unittest.mock import MagicMock
2425

2526
import msgspec
@@ -29,6 +30,7 @@
2930

3031
from airflow.api_fastapi.common.cursors import apply_cursor_filter, decode_cursor, encode_cursor
3132
from airflow.api_fastapi.common.parameters import SortParam
33+
from airflow.models.dagrun import DagRun
3234
from airflow.models.taskinstance import TaskInstance
3335

3436

@@ -85,6 +87,40 @@ def test_encode_cursor_works_without_prior_to_orm(self):
8587
decoded = decode_cursor(token)
8688
assert decoded == ["019462ab-1234-5678-9abc-def012345678"]
8789

90+
def test_encode_cursor_with_column_form_to_replace_falls_back_to_row_attr(self):
91+
"""Column-form ``to_replace`` should still allow cursor encoding via row attribute access."""
92+
sp = SortParam(["id", "run_after"], TaskInstance, {"run_after": DagRun.run_after})
93+
sp.set_value(["run_after"])
94+
95+
row = SimpleNamespace(
96+
run_after="2026-06-04T10:00:00+00:00",
97+
id="019462ab-1234-5678-9abc-def012345678",
98+
)
99+
100+
token = encode_cursor(row, sp)
101+
decoded = decode_cursor(token)
102+
assert decoded == [
103+
"2026-06-04T10:00:00+00:00",
104+
"019462ab-1234-5678-9abc-def012345678",
105+
]
106+
107+
def test_encode_cursor_column_form_to_replace_raises_when_attribute_absent(self):
108+
"""
109+
``encode_cursor`` must raise ``NotImplementedError`` (not silently encode ``None``)
110+
when a column-form ``to_replace`` key has no corresponding attribute on the row object.
111+
A ``None`` token would cause the next-page WHERE to compare against NULL and drop rows.
112+
"""
113+
sp = SortParam(
114+
["id", "data_interval_start"], TaskInstance, {"data_interval_start": DagRun.data_interval_start}
115+
)
116+
sp.set_value(["data_interval_start"])
117+
118+
# Row without data_interval_start — TaskInstance does not expose this as an attribute.
119+
row = SimpleNamespace(id="019462ab-1234-5678-9abc-def012345678")
120+
121+
with pytest.raises(NotImplementedError, match="data_interval_start"):
122+
encode_cursor(row, sp)
123+
88124
def test_apply_cursor_filter_wrong_value_count(self):
89125
sp = self._make_sort_param_with_resolved_columns(["start_date"])
90126
token = _msgpack_cursor_token(["only-one-value"])

airflow-core/tests/unit/api_fastapi/common/test_parameters.py

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,17 +135,29 @@ def test_aliased_sort_resolves_row_value_via_to_replace(self):
135135
assert param.row_value(row, "dag_run_id") == "manual__2026-04-22"
136136
assert param.row_value(row, "id") == 42
137137

138-
def test_row_value_raises_on_column_form_to_replace(self):
138+
def test_row_value_column_form_to_replace_resolves_via_row_attribute(self):
139139
"""
140-
Column-form ``to_replace`` is not supported by cursor encoding. The helper must
141-
fail loudly so a future endpoint doesn't silently ship ``None`` cursor tokens.
140+
Column-form ``to_replace`` resolves through the primary model's attribute so
141+
association proxies (e.g. ``TaskInstance.run_after``) are usable for cursor encoding.
142142
"""
143143
param = SortParam(["dag_id"], DagModel, {"last_run_state": DagRun.state}).set_value(
144144
["last_run_state"]
145145
)
146-
row = SimpleNamespace(id="test_dag")
147-
with pytest.raises(NotImplementedError, match="column-form ``to_replace``"):
148-
param.row_value(row, "last_run_state")
146+
row = SimpleNamespace(id="test_dag", last_run_state="success")
147+
assert param.row_value(row, "last_run_state") == "success"
148+
149+
def test_row_value_column_form_to_replace_raises_when_attribute_absent(self):
150+
"""
151+
Column-form ``to_replace`` must raise ``NotImplementedError`` (not return ``None``)
152+
when the primary model exposes no such attribute. A ``None`` cursor token would cause
153+
the next-page ``WHERE`` to compare against ``NULL`` and silently drop rows.
154+
"""
155+
param = SortParam(
156+
["dag_id"], DagModel, {"data_interval_start": DagRun.data_interval_start}
157+
).set_value(["data_interval_start"])
158+
row = SimpleNamespace(id="test_dag") # deliberately no data_interval_start attribute
159+
with pytest.raises(NotImplementedError, match="data_interval_start"):
160+
param.row_value(row, "data_interval_start")
149161

150162
def test_primary_key_is_not_duplicated_when_alias_maps_to_pk(self):
151163
"""Sorting by an alias that resolves to the PK must not append the PK a second time."""

airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_task_instances.py

Lines changed: 41 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2117,7 +2117,47 @@ def test_cursor_pagination_invalid_token(self, test_client, session):
21172117
)
21182118
assert response.status_code == 400
21192119

2120-
def test_task_group_filter_uses_run_version_not_latest(self, test_client, dag_maker, session):
2120+
def test_cursor_pagination_order_by_run_after_roundtrips(self, test_client, session):
2121+
"""
2122+
Sorting by ``run_after`` (a column-form ``to_replace`` backed by an association proxy)
2123+
must not raise a 500 when ``has_next=true``. Regression for
2124+
https://github.com/apache/airflow/issues/67970.
2125+
2126+
Verify the full cursor round-trip: the first page must include a ``next_cursor``,
2127+
and following that cursor must return the remaining TIs without overlap.
2128+
"""
2129+
dag_id = "example_python_operator"
2130+
total_tis = 5
2131+
self.create_task_instances(
2132+
session,
2133+
task_instances=[
2134+
{"start_date": DEFAULT_DATETIME_1 + dt.timedelta(minutes=(i + 1))} for i in range(total_tis)
2135+
],
2136+
dag_id=dag_id,
2137+
)
2138+
# First page — limit < total so next_cursor must be present
2139+
response1 = test_client.get(
2140+
"/dags/~/dagRuns/~/taskInstances",
2141+
params={"limit": 3, "order_by": ["-run_after"], "cursor": ""},
2142+
)
2143+
assert response1.status_code == 200, response1.json()
2144+
body1 = response1.json()
2145+
assert len(body1["task_instances"]) == 3
2146+
next_cursor = body1["next_cursor"]
2147+
assert next_cursor is not None, "next_cursor must be present when more rows exist"
2148+
2149+
# Second page — follow the cursor; must not 500 and must return remaining TIs
2150+
response2 = test_client.get(
2151+
"/dags/~/dagRuns/~/taskInstances",
2152+
params={"limit": 10, "order_by": ["-run_after"], "cursor": next_cursor},
2153+
)
2154+
assert response2.status_code == 200, response2.json()
2155+
body2 = response2.json()
2156+
ids1 = {ti["id"] for ti in body1["task_instances"]}
2157+
ids2 = {ti["id"] for ti in body2["task_instances"]}
2158+
assert ids1.isdisjoint(ids2), "Pages must not overlap"
2159+
assert len(ids1) + len(ids2) == total_tis
2160+
21212161
"""
21222162
Task group lookup should use the DAG version from the run, not the latest version.
21232163

0 commit comments

Comments
 (0)