Skip to content

fix(macros): direction-aware NULL fallback in cursor condition for nullable sort columns#137

Merged
vindard merged 1 commit into
mainfrom
fix/desc-nulls-last-cursor-condition
May 26, 2026
Merged

fix(macros): direction-aware NULL fallback in cursor condition for nullable sort columns#137
vindard merged 1 commit into
mainfrom
fix/desc-nulls-last-cursor-condition

Conversation

@vindard
Copy link
Copy Markdown
Contributor

@vindard vindard commented May 19, 2026

Summary

CursorStruct::condition for is_optional() columns emits a fallback col IS NOT NULL that's correct for ASC NULLS FIRST but silently drops NULL rows on page 2+ of DESC NULLS LAST pagination. Flagged by Cursor Bugbot on a downstream PR (GaloyMoney/lana-bank#5898) where Desc is the default SortDirection in the public GraphQL — making the broken path the common one.

Root cause

The OR-clause's COALESCE fires whenever col <op> $cursor is NULL, which happens when either side is NULL. The fallback decides whether to include such rows — and the correct value depends on direction, which-side-is-NULL, and whether we're on page 1 vs. paginating from a NULL-row cursor.

For ASC NULLS FIRST the fallback col IS NOT NULL happens to cover all sub-cases because NULL sorts first: a non-NULL row is "after" both page-1 and cursor-on-NULL sentinels, while a NULL row with a non-NULL cursor was on page 1 already.

For DESC NULLS LAST the cases diverge:

Case Cursor $col Cursor $id Row col Expected Old fallback Verdict
Page 1 NULL NULL non-NULL include (no cursor, everything is after) FALSE ✗ BUG
Cursor non-NULL, row NULL non-NULL non-NULL NULL include (NULL sorts last in DESC) FALSE ✗ BUG
Cursor NULL, row non-NULL NULL non-NULL non-NULL exclude (already shown before cursor) TRUE ✗ BUG

Fix

Direction-aware fallback:

  • ASC: col IS NOT NULL (unchanged).
  • DESC: $id_cursor IS NULL OR (col IS NULL AND $cursor IS NOT NULL).

The first disjunct of the DESC fallback short-circuits page 1 to TRUE (no cursor → include everything). The second covers the "row is NULL after a non-NULL cursor" case. Both correctly evaluate to FALSE when the cursor sits on a NULL row and the row is non-NULL (already shown), or when both are NULL (handled by AND-clause's IS NOT DISTINCT FROM).

Truth-table verification (DESC NULLS LAST, post-fix)

$cursor $id_cursor Row col Row id vs $id AND OR fallback Final Expected
NULL NULL non-NULL any F T ($id IS NULL short-circuit) T T ✓ (page 1)
NULL NULL NULL any T T T ✓ (page 1)
non-NULL non-NULL non-NULL < $c any F T (< definite) T T ✓
non-NULL non-NULL non-NULL = $c id < $id T T T ✓
non-NULL non-NULL non-NULL = $c id ≥ $id F F (< definite) F F ✓
non-NULL non-NULL non-NULL > $c any F F (< definite) F F ✓
non-NULL non-NULL NULL any F T (NULL IS NULL AND $c IS NOT NULL) T T ✓
NULL non-NULL non-NULL any F F (both disjuncts F) F F ✓
NULL non-NULL NULL id < $id T T T ✓
NULL non-NULL NULL id ≥ $id F F (same) F F ✓

ASC NULLS FIRST is unchanged — col IS NOT NULL already satisfies the analogous truth table for ASC.

Single point of change

condition() is shared by list_by, list_for, and list_for_filters macros (referenced from list_for_fn.rs:102,115 and list_for_filters_fn.rs:314,328). One fix propagates to all three.

Test plan

  • Updated SQL-string assertion in list_by_fn.rs for the DESC NULLS LAST case.
  • All 82 macro tests pass.
  • Runtime regression verification lives in the consuming repo (GaloyMoney/lana-bank#5898's core/credit/tests/loan_to_collateral_ratio_pagination.rs); the DESC half becomes reachable from there after this lands and es-entity is bumped.

🤖 Generated with Claude Code

…llable sort columns

`CursorStruct::condition` for `is_optional()` columns emits a fallback
clause `col IS NOT NULL` that works for `ASC NULLS FIRST` but silently
drops NULL rows on page 2+ of `DESC NULLS LAST` pagination. Flagged by
Cursor Bugbot on a downstream PR (GaloyMoney/lana-bank#5898) where
`Desc` is the default sort direction in the public GraphQL.

## Root cause

For a nullable column, the macro at `list_by_fn.rs::condition` emitted:

    ((col IS NOT DISTINCT FROM $cursor) AND COALESCE(id <op> $id_cursor, true)
     OR COALESCE(col <op> $cursor, col IS NOT NULL))

with `<op>` flipping between `>` (ASC) and `<` (DESC). The fallback
`col IS NOT NULL` was applied to both directions. It is semantically
correct only for ASC NULLS FIRST.

The OR-clause's `COALESCE` fires whenever `col <op> $cursor` is NULL,
which happens when either side is NULL. The fallback decides whether
to include the row — and the correct value depends on (a) where NULL
rows sort, (b) which side is NULL, AND (c) whether we are on page 1
(no cursor) vs. paginating from a cursor that happens to be NULL.

For ASC NULLS FIRST the fallback `col IS NOT NULL` happens to cover
all sub-cases because NULL sorts first: a non-NULL row is "after" both
page-1 and cursor-on-NULL sentinels, while a NULL row with a non-NULL
cursor was on page 1 already.

For DESC NULLS LAST the cases diverge:

  - Page 1 (no cursor): include all rows.
  - Cursor on non-NULL row: include NULL rows (sort last).
  - Cursor on NULL row: exclude non-NULL rows (already shown).

The first and third cases both have `$cursor IS NULL` but require
opposite answers. The discriminator is `$id_cursor`: NULL on page 1,
non-NULL when paginating from a NULL cursor row.

## Fix

Direction-aware fallback:

  - ASC: `col IS NOT NULL` (unchanged).
  - DESC: `$id_cursor IS NULL OR (col IS NULL AND $cursor IS NOT NULL)`.

The first disjunct of the DESC fallback short-circuits page 1 to TRUE
(no cursor → include everything). The second covers the "row is NULL
after a non-NULL cursor" case. Both correctly evaluate to FALSE when
the cursor sits on a NULL row and the row is non-NULL (already shown),
or when both are NULL (handled by AND-clause's IS NOT DISTINCT FROM).

## Truth-table verification (DESC NULLS LAST, post-fix)

Tracing every distinct case the WHERE clause can encounter:

| `$cursor` | `$id_cursor` | Row `col` | Row id vs `$id` | AND | OR fallback | Final | Expected |
|---|---|---|---|---|---|---|---|
| NULL | NULL | non-NULL | any | F | T (`$id IS NULL` short-circuit) | T | T ✓ (page 1) |
| NULL | NULL | NULL     | any | T | —                                | T | T ✓ (page 1) |
| non-NULL | non-NULL | non-NULL `< $c` | any        | F | T (`<` definite)    | T | T ✓ |
| non-NULL | non-NULL | non-NULL `= $c` | id < $id   | T | —                   | T | T ✓ |
| non-NULL | non-NULL | non-NULL `= $c` | id ≥ $id   | F | F (`<` definite)    | F | F ✓ |
| non-NULL | non-NULL | non-NULL `> $c` | any        | F | F (`<` definite)    | F | F ✓ |
| non-NULL | non-NULL | NULL            | any        | F | T (`NULL IS NULL AND $c IS NOT NULL`) | T | T ✓ |
| NULL | non-NULL | non-NULL       | any        | F | F (`$id` set AND `$c` NULL → both disjuncts F) | F | F ✓ |
| NULL | non-NULL | NULL           | id < $id   | T | —                   | T | T ✓ |
| NULL | non-NULL | NULL           | id ≥ $id   | F | F (same)            | F | F ✓ |

ASC NULLS FIRST is unchanged — `col IS NOT NULL` already satisfies the
analogous truth table for ASC (the relative position of NULL in the
sort order makes page-1 and cursor-on-NULL behave identically for
non-NULL rows).

## Single point of change

`condition()` is shared by `list_by`, `list_for`, and
`list_for_filters` macros (referenced from `list_for_fn.rs:102,115`
and `list_for_filters_fn.rs:314,328`). One fix propagates to all
three.

## Tests

  - Updated SQL-string assertion in `list_by_fn.rs` for the DESC
    NULLS LAST case.
  - 82 macro tests pass.
  - Runtime regression verification lives in the consuming repo
    (GaloyMoney/lana-bank#5898's
    `core/credit/tests/loan_to_collateral_ratio_pagination.rs`); the
    DESC half will be reachable from there after es-entity is bumped.
@vindard vindard merged commit 68a4825 into main May 26, 2026
6 checks passed
vindard added a commit to GaloyMoney/cala that referenced this pull request May 27, 2026
es-entity 0.10.37 contains the direction-aware NULL fallback in cursor
WHERE clauses for nullable sort columns (GaloyMoney/es-entity#137) and
the opt-in `nullable` column attribute for non-Option<T> Rust types
(GaloyMoney/es-entity#138).

The macro change in #137 alters the DESC SQL emitted for `Option<...>`
columns — for cala-ledger this affects `Account.external_id` and
`AccountSet.external_id` pagination. New fingerprints replace the old
ones in `cala-ledger/.sqlx/`; semantics are unchanged for non-NULL rows
and now also include NULL rows on page 2+ for DESC (previously dropped).

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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