ReviewOS

also looking at this

stacks/bun-query-builder

fix(client): the cursor predicate is a where term, not a second WHERE (#1090)

#1098
Merged glennmichael123 wants to merge fix/cursor-paginate-double-where into main
2 files +213 -18
10 days ago

Closes #1090.

The bug

cursorPaginate composed its cursor predicate with an unconditional WHERE keyword:

q = sql`${q} WHERE ${sql(String(column))} > ${cursor}`

So any filter already on the builder produced two top-level WHEREs:

await db.selectFrom('t').selectAll().where('b', '=', 1).cursorPaginate(3, 5, 'id')
// SQLiteError: near "WHERE": syntax error

chunkById and eachById both delegate to cursorPaginate, so .where(x).chunkById(...) — the natural way to walk a filtered table in batches — had never worked. This is not the SQLite-wrapper class of bug; it reproduces on every dialect.

Why nothing caught it

Page one is fetched with cursor === undefined, so no predicate is emitted and it succeeds. The failure only lands on page two. A test fixture that fits in a single chunk passes against the bug — which is exactly what the existing pagination tests do.

Every test added here either forces a non-null cursor or spans more than one chunk. The chunkById case deliberately spans four.

The fix

The predicate is now a WHERE term, rendered through currentSql() (the term-list renderer from #1083), so it is ANDed onto whatever the builder already has and spliced ahead of the trailing clauses rather than appended past them.

currentSql() gained an optional extra term for this. The cursor changes on every page, so pushing it onto whereTerms would accumulate one stale predicate per iteration — the extra term applies to a single render and is not recorded.

A test pins the interaction with #1083's grouping: .where(a).orWhere(b).cursorPaginate(...) must AND the cursor against the whole OR-group, not against its last term.

⚠️ One extra defect fixed, because it was in the same lines

cursorPaginate combined with orderBy emitted a second ORDER BY and failed to parse. It was invisible until now because the duplicate-WHERE error fired first — fixing that one revealed this one, and shipping the fix without handling it would have looked like this PR caused it.

Cursor pagination owns the ordering by necessity: col > ? only means "the rows after this one" if the rows are sorted by that column, so a second ordering makes the cursor meaningless — it silently skips or repeats rows. It now throws:

[query-builder] cursorPaginate() cannot be combined with orderBy() — it orders by id ASC
itself, and a cursor is only meaningful in that order. Remove the orderBy(), or pass the
column and direction to cursorPaginate().

No working code changes: that combination already crashed with a driver syntax error. This replaces an opaque near "ORDER": syntax error with an explanation. The check is paren-depth aware, so a subquery's own ORDER BY doesn't trip it.

Also tightened while rewriting

perPage is validated as a positive integer, and the cursor column goes through validateIdentifier — neither of which it did before. cursorPaginate(3, 1, 'id; DROP TABLE t') was previously interpolated; a test now asserts it is rejected and the table survives.

Verification

casebeforeafter
.where(b=1).cursorPaginate(3, 5, 'id')near "WHERE": syntax error[7, 9, 11]
.where(b=1).chunkById(3, 'id', …)throws on chunk 2[1,3,5,7,9,11,13,15,17,19]
.where(b=1).cursorPaginate(3, 15, 'id', 'desc')throws[13, 11, 9]
.where(b=1).cursorPaginate(3, [2,7], ['g','id'])throws[9, 11, 13]
.cursorPaginate(3, 5, 'id') (no filter)[6, 7, 8]unchanged

Run against both SQLite and a live Postgres — identical results on each.

1789 pass / 4 skip / 0 fail, typecheck clean, 0 lint errors.

🤖 Generated with Claude Code

2 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.