From 3786325071e6d07482827d0acc0d6930371b4e6e Mon Sep 17 00:00:00 2001 From: vindard <17693119+vindard@users.noreply.github.com> Date: Tue, 19 May 2026 11:12:56 -0400 Subject: [PATCH] fix(macros): direction-aware NULL fallback in cursor condition for nullable sort columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 $id_cursor, true) OR COALESCE(col $cursor, col IS NOT NULL)) with `` 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 $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. --- es-entity-macros/src/repo/list_by_fn.rs | 41 +++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/es-entity-macros/src/repo/list_by_fn.rs b/es-entity-macros/src/repo/list_by_fn.rs index 49e563a..a295a8c 100644 --- a/es-entity-macros/src/repo/list_by_fn.rs +++ b/es-entity-macros/src/repo/list_by_fn.rs @@ -60,8 +60,45 @@ impl CursorStruct<'_> { if self.column.is_id() { format!("COALESCE(id {comp} ${id_offset}, true)") } else if self.column.is_optional() { + // The OR-clause's COALESCE fires when `col {comp} ${cursor}` is NULL, + // which happens whenever either side of the comparison is NULL. The + // fallback decides whether the row is "after" the cursor in + // (col, id) ordering — and the correct answer depends on direction + // (NULLS FIRST for ASC, NULLS LAST for DESC) and on whether we are + // on page 1 vs. paginating from a cursor sitting on a NULL row. + // + // - ASC NULLS FIRST: a non-NULL row with a NULL cursor is "after" + // (non-NULL sorts after NULL), whether the NULL cursor represents + // page 1 (no cursor) or a cursor sitting on a NULL row. Fallback + // `col IS NOT NULL` covers both: TRUE for non-NULL rows, FALSE + // for NULL rows (NULL rows with a non-NULL cursor were on page 1 + // already). + // + // - DESC NULLS LAST: asymmetric. NULL rows sort *last*, so: + // • Page 1 (`$id_offset` IS NULL, no cursor): include all rows + // — equivalent to "the sentinel before everything". + // • Cursor on NULL row (`$id_offset` set, `${column_offset}` + // NULL): non-NULL rows already shown → exclude. + // • Cursor on non-NULL row, row NULL: NULL sorts after → include. + // The DESC fallback `${id_offset} IS NULL OR + // (col IS NULL AND ${column_offset} IS NOT NULL)` captures all + // three: `${id_offset} IS NULL` short-circuits to TRUE on page 1, + // while the second disjunct catches the "include NULL rows after + // non-NULL cursor" case without re-including already-shown rows. + // + // The cursor=NULL + row=NULL case is handled by the AND-clause's + // `IS NOT DISTINCT FROM` + the id comparison, so the OR-clause's + // fallback must NOT fire then. Verified by truth-table on PR. + let null_handling = if ascending { + format!("{0} IS NOT NULL", self.column.name()) + } else { + format!( + "${id_offset} IS NULL OR ({0} IS NULL AND ${column_offset} IS NOT NULL)", + self.column.name(), + ) + }; format!( - "({0} IS NOT DISTINCT FROM ${column_offset}) AND COALESCE(id {comp} ${id_offset}, true) OR COALESCE({0} {comp} ${column_offset}, {0} IS NOT NULL)", + "({0} IS NOT DISTINCT FROM ${column_offset}) AND COALESCE(id {comp} ${id_offset}, true) OR COALESCE({0} {comp} ${column_offset}, {null_handling})", self.column.name(), ) } else { @@ -824,7 +861,7 @@ mod tests { es_entity::ListDirection::Descending => { es_entity::es_query!( entity = Entity, - "SELECT value, id FROM entities WHERE ((value IS NOT DISTINCT FROM $3) AND COALESCE(id < $2, true) OR COALESCE(value < $3, value IS NOT NULL)) ORDER BY value DESC NULLS LAST, id DESC LIMIT $1", + "SELECT value, id FROM entities WHERE ((value IS NOT DISTINCT FROM $3) AND COALESCE(id < $2, true) OR COALESCE(value < $3, $2 IS NULL OR (value IS NULL AND $3 IS NOT NULL))) ORDER BY value DESC NULLS LAST, id DESC LIMIT $1", (first + 1) as i64, id as Option, value as Option,