Skip to content

fix: keep null rows when paginating past a non-null cursor - #654

Closed
breken-ai wants to merge 1 commit into
supabase:masterfrom
breken-ai:fix/pagination-cursor-nulls-last
Closed

breken-ai wants to merge 1 commit into
supabase:masterfrom
breken-ai:fix/pagination-cursor-nulls-last

Conversation

@breken-ai

Copy link
Copy Markdown

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

Keyset pagination never returns rows whose sort column is null once the cursor holds a non-null value, whenever those nulls sort after the cursor. That covers first/after with AscNullsLast or DescNullsLast, and last/before with AscNullsFirst or DescNullsFirst. hasNextPage also turns false too early, so a client paging through the collection stops before the null rows.

Table::to_pagination_clause builds, per order column:

col > x  or (col is not null and x is null and <nulls_first>)

This handles a null cursor under NullsFirst. The mirror case is missing: the column is null, the cursor is not, and nulls sort last.

Repro (account(id int primary key, score int), rows (1,10) (2,null) (3,20) (4,null)):

{ accountCollection(first: 2, orderBy: [{score: AscNullsLast}]) { pageInfo { hasNextPage endCursor } edges { node { id } } } }
# ids 1, 3 · hasNextPage true · endCursor "WzIwLCAzXQ==" ([20, 3])

{ accountCollection(first: 2, after: "WzIwLCAzXQ==", orderBy: [{score: AscNullsLast}]) { pageInfo { hasNextPage } edges { node { id } } } }
# master: no edges. Expected: ids 2, 4

What is the new behavior?

The clause adds or (col is null and x is not null and <nulls_last>), so rows with null values follow a non-null cursor wherever the effective direction puts nulls last. Pages come back in the same order as the unpaginated query.

One existing expectation changes. In resolve_connection_pagination_args, "Last before w/ complex order" (orderBy: [{reversed: AscNullsLast}, {title: AscNullsFirst}], before: [3, "a"]) left out id 12 (reversed = 3, title = null). Under title AscNullsFirst, (3, null) comes before (3, "a"), so id 12 is the row right before the cursor. The last 5 rows before the cursor in forward order are ids 3, 18, 13, 8 and 12, which is what the query now returns.

Additional context

  • New regression test test/sql/pagination_cursor_nulls.sql covers AscNullsLast after (plus hasNextPage), DescNullsLast after, AscNullsFirst before, and the null-cursor case that already worked.
  • Proof, run with the repo's dockerfiles/db/Dockerfile toolchain (PG 17, pgrx 0.19.2):
    • On master, pagination_cursor_nulls fails. The pages after the cursor are empty and hasNextPage is false.
    • With the fix, ./bin/installcheck passes all 123 tests.
    • cargo fmt --check and cargo clippy --features pg17 -- -D warnings are clean.
  • The clause has been in this shape since the Rust port.

This PR was prepared with AI assistance (Claude). I reviewed and tested the change as described above.

🤖 Generated with Claude Code

The keyset pagination clause only matched rows whose sort column
compared greater (or less) than the cursor value, plus non-null rows
after a null cursor under NullsFirst. It never matched null rows after
a non-null cursor, so with NullsLast ordering (or NullsFirst with
`before`) every row with a null sort value was unreachable by paging,
and hasNextPage reported false early.

Add the missing case: the column is null, the cursor value is not, and
nulls sort last in the effective direction.

The existing "last before w/ complex order" expectation in
resolve_connection_pagination_args left out id 12 (reversed 3, title
null), which sorts before the cursor [3, "a"] under title
AscNullsFirst; it now appears.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@imor

imor commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

Closing as this is 100% AI generated.

@imor imor closed this Sep 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants