pierrejeambrun commented on code in PR #67973:
URL: https://github.com/apache/airflow/pull/67973#discussion_r3374601697
##########
airflow-core/src/airflow/api_fastapi/common/parameters.py:
##########
@@ -626,24 +626,17 @@ def row_value(self, row: Any, name: str) -> Any:
Extract the sort-key value for ``name`` from a result row.
Resolves the accessor through ``to_replace`` for string aliases
- (e.g. ``{"dag_run_id": "run_id"}``); otherwise reads ``name`` directly.
+ (e.g. ``{"dag_run_id": "run_id"}``). For column-form mappings
+ (e.g. ``{"run_after": DagRun.run_after}``), we fall back to the
+ original attribute name so association proxies on the primary model can
+ still be used for cursor values.
"""
if self.to_replace:
replacement = self.to_replace.get(name)
if isinstance(replacement, str):
return getattr(row, replacement, None)
if replacement is not None:
- # TODO: Column-form ``to_replace`` (e.g. ``{"last_run_state":
DagRun.state}``)
- # isn't supported for cursor pagination — no endpoint that
uses cursor
- # pagination needs it today. When one does, decide how the row
exposes the
- # value (projected label on the SELECT, eagerly loaded
relationship, etc.)
- # and wire it up here. Raising loudly so a future caller
doesn't silently
- # get ``None`` cursor tokens.
- raise NotImplementedError(
- f"Cursor pagination does not support column-form
``to_replace`` mapping for "
- f"``{name}``. Use a string alias in ``to_replace`` or sort
by a primary-model "
- f"attribute."
- )
+ return getattr(row, name, None)
Review Comment:
Resolve through the attribute so association proxies like
`TaskInstance.run_after` work, but fail loud when the model exposes no such
attribute instead of emitting a `None` cursor. Catching `AttributeError`
(rather than checking `is None`) also avoids 500-ing on a legitimately null
`logical_date`.
```suggestion
if replacement is not None:
# Column-form mapping resolves through the primary model's
attribute,
# often an association proxy onto the joined entity
# (``TaskInstance.run_after`` -> ``dag_run.run_after``).
Fail loudly if the
# model exposes no such attribute, rather than emitting a
``None`` cursor token.
try:
return getattr(row, name)
except AttributeError:
raise NotImplementedError(
f"Cursor pagination cannot resolve column-form
``to_replace`` for "
f"``{name}``: the primary model exposes no such
attribute. Add an "
f"association proxy, use a string alias, or sort by
a primary-model column."
)
```
Worth tweaking the docstring above too — it now fails loud on unexposed keys
rather than always falling back.
##########
airflow-core/tests/unit/api_fastapi/common/test_parameters.py:
##########
@@ -135,17 +135,16 @@ def
test_aliased_sort_resolves_row_value_via_to_replace(self):
assert param.row_value(row, "dag_run_id") == "manual__2026-04-22"
assert param.row_value(row, "id") == 42
- def test_row_value_raises_on_column_form_to_replace(self):
+ def test_row_value_falls_back_on_column_form_to_replace(self):
"""
- Column-form ``to_replace`` is not supported by cursor encoding. The
helper must
- fail loudly so a future endpoint doesn't silently ship ``None`` cursor
tokens.
+ Column-form ``to_replace`` should fall back to ``getattr(row, name)``
so
+ association proxies on the primary model remain usable for cursor
encoding.
"""
param = SortParam(["dag_id"], DagModel, {"last_run_state":
DagRun.state}).set_value(
["last_run_state"]
)
- row = SimpleNamespace(id="test_dag")
- with pytest.raises(NotImplementedError, match="column-form
``to_replace``"):
- param.row_value(row, "last_run_state")
+ row = SimpleNamespace(id="test_dag", last_run_state="success")
+ assert param.row_value(row, "last_run_state") == "success"
Review Comment:
This (and the `test_cursors.py` case) uses a `SimpleNamespace` row that
always carries the attribute, so it'd pass even for `data_interval_start` — the
actual broken key. Could we cover both branches: one where the attribute
resolves (e.g. `run_after`) and one where it's absent and `row_value` raises
(mirroring `data_interval_start`)? An endpoint-level test
(`order_by=-run_after` with more than `limit` rows, asserting the returned
cursor round-trips) would lock in the real path.
--
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]
