Skip to content

Commit 35e4c7f

Browse files
committed
fix(run-storage): validate cursor order_by column
## Summary & Motivation Cursor pagination is invalid when the `order_by` doesn't match the `cursor` and/or the cursor doesn't completely cover the index. Validate that this doesn't happen, and raise if it ever does happen. `_add_cursor_limit_to_query` in `SqlRunStorage` previously allowed semantically invalid cursor pagination with a non-`id` `order_by` column. This would silently apply `id`-based cursor filtering while sorting by the requested column — producing incorrect, page-skipping results with no error. This change adds explicit guards: - If `cursor` is provided and `order_by` maps to a **non-unique column** (e.g. `status`), a `ParameterCheckError` is raised immediately, since cursor pagination on a non-unique column is undefined. - If `cursor` is provided and `order_by` maps to a **unique non-`id` column** (e.g. `run_id`), a `NotImplementedCheckError` is raised, since stable single-column cursor pagination over an arbitrary unique key is not yet implemented. - The existing `id`-based cursor path is unchanged. ## How I Tested These Changes Two new test cases in `TestRunStorage` cover both error branches: `test_cursor_pagination_with_non_unique_order_by_raises` and `test_cursor_pagination_with_unique_non_id_order_by_raises`. ## Changelog `DagsterInstance.get_run_records`, `RunStorage.get_runs`, and `RunStorage.get_run_records`: passing `cursor` together with an `order_by` column other than `id` now raises an explicit error instead of silently returning wrong results.
1 parent b8b7f3f commit 35e4c7f

2 files changed

Lines changed: 47 additions & 6 deletions

File tree

python_modules/dagster/dagster/_core/storage/runs/sql_run_storage.py

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -277,18 +277,40 @@ def _add_cursor_limit_to_query(
277277
order_by: str | None,
278278
ascending: bool | None,
279279
) -> SqlAlchemyQuery:
280-
"""Helper function to deal with cursor/limit pagination args."""
280+
"""Helper function to deal with cursor/limit/ordering pagination args for a RunsTable query.
281+
282+
Cursor pagination is only supported when sorting by ``RunsTable.c.id`` (the default). If
283+
``order_by`` names a different column and ``cursor`` is also provided, this method raises.
284+
"""
285+
sorting_column: db.Column = getattr(RunsTable.c, order_by) if order_by else RunsTable.c.id
281286
if cursor:
282-
cursor_query = db_select([RunsTable.c.id]).where(RunsTable.c.run_id == cursor)
283-
if ascending:
284-
query = query.where(RunsTable.c.id > db_scalar_subquery(cursor_query))
287+
if sorting_column is RunsTable.c.id:
288+
# `cursor_query` and `query.where(RunsTable.c.id ....)` both assume that `id` is
289+
# the sorting_column.
290+
cursor_query = db_select([RunsTable.c.id]).where(RunsTable.c.run_id == cursor)
291+
if ascending:
292+
query = query.where(RunsTable.c.id > db_scalar_subquery(cursor_query))
293+
else:
294+
query = query.where(RunsTable.c.id < db_scalar_subquery(cursor_query))
295+
elif sorting_column.unique:
296+
# Single-column cursor pagination requires a unique sort column to identify the last
297+
# seen row unambiguously. This branch means the column is unique but is not `id`.
298+
# The only other unique column today is `run_id`, which is a random uuid4 and not a
299+
# useful sort key. Stable, multi-column cursor pagination over a composite index
300+
# is possible in general but not currently implemented.
301+
check.not_implemented(
302+
f"Cursor pagination for this RunsTable unique column: {order_by}"
303+
)
285304
else:
286-
query = query.where(RunsTable.c.id < db_scalar_subquery(cursor_query))
305+
check.param_invariant(
306+
False,
307+
"sorting_column",
308+
f"Cursor pagination requires a unique sort column, but RunsTable.c.{order_by} is not unique.",
309+
)
287310

288311
if limit:
289312
query = query.limit(limit)
290313

291-
sorting_column = getattr(RunsTable.c, order_by) if order_by else RunsTable.c.id
292314
direction = db.asc if ascending else db.desc
293315
query = query.order_by(direction(sorting_column))
294316

python_modules/dagster/dagster_tests/storage_tests/utils/run_storage.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@
4242
from dagster._serdes import serialize_pp
4343
from dagster._time import create_datetime, datetime_from_timestamp
4444
from dagster_shared import seven
45+
from dagster_shared.check.functions import NotImplementedCheckError, ParameterCheckError
4546
from dagster_test.utils.data_factory import dagster_run as create_dagster_run
4647

4748
win_py36 = seven.IS_WINDOWS and sys.version_info[0] == 3 and sys.version_info[1] == 6
@@ -940,6 +941,24 @@ def test_fetch_records_by_update_timestamp(self, storage, instance):
940941
)
941942
] == [two]
942943

944+
def test_cursor_pagination_with_non_unique_order_by_raises(self, storage):
945+
"""Cursor pagination on a non-unique column is undefined and must raise ParameterCheckError."""
946+
assert storage
947+
self._skip_in_memory(storage)
948+
949+
with pytest.raises(ParameterCheckError, match="requires a unique sort column"):
950+
storage.get_run_records(cursor=str(uuid4()), order_by="status")
951+
952+
def test_cursor_pagination_with_unique_non_id_order_by_raises(self, storage):
953+
"""Cursor pagination on a unique non-id column is not yet implemented."""
954+
assert storage
955+
self._skip_in_memory(storage)
956+
957+
with pytest.raises(
958+
NotImplementedCheckError, match="Cursor pagination for this RunsTable unique column"
959+
):
960+
storage.get_run_records(cursor=str(uuid4()), order_by="run_id")
961+
943962
def test_fetch_records_by_create_timestamp(self, storage):
944963
assert storage
945964
self._skip_in_memory(storage)

0 commit comments

Comments
 (0)