ReviewOS
Reload

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
packages/bun-query-builder/src/client.tsmodified+72-18
@@ -186,6 +186,26 @@ function renderSelectColumn(col: unknown): string {
186186 )
187187}
188188
189/**
190 * Does this SQL have an `ORDER BY` of its own, at paren depth 0?
191 *
192 * Depth-aware because a subquery in the FROM list or an `IN (SELECT … ORDER BY
193 * …)` carries its own, and matching that one would reject a query whose outer
194 * SELECT is unordered.
195 */
196function hasTopLevelOrderBy(s: string): boolean {
197 const re = /\(|\)|\bORDER\s+BY\b/gi
198 let depth = 0
199 let m: RegExpExecArray | null
200 // eslint-disable-next-line no-cond-assign
201 while ((m = re.exec(s))) {
202 if (m[0] === '(') { depth++ }
203 else if (m[0] === ')') { depth = Math.max(0, depth - 1) }
204 else if (depth === 0) { return true }
205 }
206 return false
207}
208
189209// Pre-compiled regex patterns for performance
190210const SQL_PATTERNS = {
191211 SELECT_STAR: /^SELECT\s+\*/i,
@@ -3297,9 +3317,14 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
32973317 *
32983318 * Splicing rather than appending also fixes `.where(a).orderBy(c).orWhere(b)`,
32993319 * which used to emit `… ORDER BY c ASC OR b` and fail to parse.
3320 *
3321 * `extra` appends one more AND-term for this render only, without recording
3322 * it. That is what cursorPaginate needs: its cursor predicate changes on
3323 * every page, so pushing it onto `whereTerms` would accumulate one stale
3324 * predicate per iteration.
33003325 */
3301 const currentSql = (): string => {
3302 const body = renderWhereTerms(whereTerms)
3326 const currentSql = (extra?: WhereTerm): string => {
3327 const body = renderWhereTerms(extra ? [...whereTerms, extra] : whereTerms)
33033328 if (!body)
33043329 return text
33053330 if (whereTail) {
@@ -5608,29 +5633,58 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
56085633 return { data: hasMore ? data.slice(0, perPage) : data, meta: { perPage, page: p, hasMore } }
56095634 },
56105635 async cursorPaginate(perPage: number, cursor?: any, column: string | string[] = 'id', direction: 'asc' | 'desc' = 'asc') {
5611 let q = ensureBuilt()
5636 if (!Number.isInteger(perPage) || perPage <= 0)
5637 throw new TypeError(`[query-builder] cursorPaginate(perPage): expected positive integer, got ${perPage}`)
5638
5639 const cols = (Array.isArray(column) ? column : [column]).map(String)
5640 for (const c of cols) validateIdentifier(c, 'cursorPaginate(column)')
5641 const cmp = direction === 'asc' ? '>' : '<'
5642 const dir = direction === 'asc' ? 'ASC' : 'DESC'
5643 const params = [...whereParams]
5644
5645 // The cursor predicate is a WHERE TERM, not a second WHERE keyword.
5646 // It used to be composed as `sql`${q} WHERE ...``, unconditionally — so
5647 // any filter already on the builder produced `... WHERE a WHERE b`, a
5648 // syntax error on every dialect. That is why `.where(x).chunkById(...)`
5649 // never worked: page one has no cursor and succeeds, and the failure
5650 // only lands on page two. See stacksjs/bun-query-builder#1090.
5651 let predicate = ''
56125652 if (cursor !== undefined && cursor !== null) {
56135653 if (Array.isArray(column)) {
5614 const cols = column.map(c => sql(String(c)))
5615 const comp = direction === 'asc' ? sql`>` : sql`<`
5616 const tupleCols = sql`(${sql(cols as any)})`
5617 const tupleVals = sql`(${sql(cursor as any)})`
5618 q = sql`${q} WHERE ${tupleCols} ${comp} ${tupleVals}`
5654 const phs = (cursor as any[]).map((v) => { params.push(v); return getPlaceholder(params.length) })
5655 predicate = `(${cols.join(', ')}) ${cmp} (${phs.join(', ')})`
56195656 }
56205657 else {
5621 q = direction === 'asc'
5622 ? sql`${q} WHERE ${sql(String(column))} > ${cursor}`
5623 : sql`${q} WHERE ${sql(String(column))} < ${cursor}`
5658 params.push(cursor)
5659 predicate = `${cols[0]} ${cmp} ${getPlaceholder(params.length)}`
56245660 }
56255661 }
5626 if (Array.isArray(column)) {
5627 const orderParts = column.map(c => sql`${sql(String(c))} ${direction === 'asc' ? sql`ASC` : sql`DESC`}`)
5628 const orderExpr = orderParts.reduce((acc, p, i) => (i === 0 ? p : sql`${acc}, ${p}`))
5629 q = sql`${q} ORDER BY ${orderExpr} LIMIT ${perPage + 1}`
5630 }
5631 else {
5632 q = sql`${q} ORDER BY ${sql(String(column))} ${direction === 'asc' ? sql`ASC` : sql`DESC`} LIMIT ${perPage + 1}`
5662
5663 // Rendered through currentSql() so the predicate is spliced ahead of any
5664 // trailing clause rather than appended past it, and so the builder's own
5665 // WHERE terms come along.
5666 const base = reorderSelectClauses(predicate ? currentSql({ conn: 'AND', sql: predicate }) : currentSql())
5667 const order = cols.map(c => `${c} ${dir}`).join(', ')
5668
5669 // Cursor pagination OWNS the ordering — the cursor predicate `col > ?`
5670 // only selects "the rows after this one" if the rows are sorted by that
5671 // same column, so a different ORDER BY makes the cursor meaningless and
5672 // silently skips or repeats rows.
5673 //
5674 // This case has always been broken: it emitted a second ORDER BY and
5675 // failed to parse. It was invisible until now only because the
5676 // duplicate-WHERE error above fired first. Refusing it says so, rather
5677 // than dropping the caller's ordering without mentioning it.
5678 if (hasTopLevelOrderBy(base)) {
5679 throw new TypeError(
5680 `[query-builder] cursorPaginate() cannot be combined with orderBy() — it orders by `
5681 + `${order} itself, and a cursor is only meaningful in that order. `
5682 + `Remove the orderBy(), or pass the column and direction to cursorPaginate().`,
5683 )
56335684 }
5685 const q = params.length > 0
5686 ? _sql.unsafe(`${base} ORDER BY ${order} LIMIT ${perPage + 1}`, params)
5687 : _sql.unsafe(`${base} ORDER BY ${order} LIMIT ${perPage + 1}`)
56345688 const rows = await runWithHooks<any[]>(q, 'select', { signal: abortSignal, timeoutMs })
56355689 // We fetch perPage+1 rows to detect whether more exist; the extra row
56365690 // is only a "has more?" probe and is NOT delivered. The next cursor
packages/bun-query-builder/test/client.cursor-paginate.test.tsadded+141-0
@@ -0,0 +1,141 @@
1/**
2 * Regression coverage for stacksjs/bun-query-builder#1090.
3 *
4 * `cursorPaginate` appended its cursor predicate with an unconditional `WHERE`
5 * keyword instead of continuing an existing one with `AND`. Any filter already
6 * on the builder produced two top-level `WHERE`s and a parse error — on every
7 * dialect, not just the SQLite wrapper.
8 *
9 * `chunkById` and `eachById` both delegate to it, so `.where(x).chunkById(...)`
10 * — the natural way to walk a filtered table in batches — had never worked.
11 *
12 * The timing is what kept this hidden: page one is fetched with no cursor, so
13 * no predicate is emitted and it succeeds. The failure only lands on page two.
14 * A fixture that fits in a single chunk passes, which is why the existing
15 * pagination tests never caught it. Every test here therefore either forces a
16 * non-null cursor or spans more than one chunk.
17 */
18
19import { SQL } from 'bun'
20import { afterAll, beforeAll, describe, expect, it } from 'bun:test'
21import { buildDatabaseSchema, createQueryBuilder } from '../src'
22
23const schema = buildDatabaseSchema({
24 Row: {
25 name: 'Row',
26 table: 'cp_rows',
27 primaryKey: 'id',
28 attributes: {
29 id: { validation: { rule: {} } },
30 b: { validation: { rule: {} } },
31 g: { validation: { rule: {} } },
32 },
33 },
34} as any)
35
36let sql: SQL
37
38beforeAll(async () => {
39 sql = new SQL('sqlite://:memory:')
40 await sql.unsafe('CREATE TABLE cp_rows (id INTEGER PRIMARY KEY, b INTEGER, g INTEGER)')
41 // ids 1-20; `b` alternates 1/0 so a filter halves the set; `g` groups by 5.
42 for (let i = 1; i <= 20; i++)
43 await sql.unsafe('INSERT INTO cp_rows VALUES (?, ?, ?)', [i, i % 2, Math.ceil(i / 5)])
44})
45
46afterAll(async () => {
47 await sql?.end()
48})
49
50const q = () => createQueryBuilder<typeof schema>({ schema, sql }).selectFrom('cp_rows').selectAll() as any
51
52describe('cursorPaginate continues an existing WHERE (#1090)', () => {
53 it('pages a filtered query instead of throwing', async () => {
54 const page = await q().where('b', '=', 1).cursorPaginate(3, 5, 'id')
55
56 // Was: SQLiteError: near "WHERE": syntax error.
57 expect(page.data.map((r: any) => r.id)).toEqual([7, 9, 11])
58 // The builder's own filter must still apply — a fix that merely stopped
59 // throwing by dropping the WHERE would return 6, 7, 8 here.
60 expect(page.data.every((r: any) => r.b === 1)).toBe(true)
61 })
62
63 it('pages an unfiltered query exactly as before', async () => {
64 const page = await q().cursorPaginate(3, 5, 'id')
65
66 expect(page.data.map((r: any) => r.id)).toEqual([6, 7, 8])
67 })
68
69 it('honours a descending cursor alongside a filter', async () => {
70 const page = await q().where('b', '=', 1).cursorPaginate(3, 15, 'id', 'desc')
71
72 expect(page.data.map((r: any) => r.id)).toEqual([13, 11, 9])
73 })
74
75 it('honours a composite cursor alongside a filter', async () => {
76 const page = await q().where('b', '=', 1).cursorPaginate(3, [2, 7], ['g', 'id'])
77
78 expect(page.data.map((r: any) => r.id)).toEqual([9, 11, 13])
79 })
80
81 it('carries a grouped OR filter into the cursor query', async () => {
82 // Guards the interaction with #1083: the builder's WHERE is rendered from
83 // the term list, so the cursor predicate has to be ANDed against the whole
84 // group rather than against its last term.
85 const page = await q().where('id', '<=', 4).orWhere('id', '>=', 18).cursorPaginate(10, 1, 'id')
86
87 expect(page.data.map((r: any) => r.id)).toEqual([2, 3, 4, 18, 19, 20])
88 })
89})
90
91describe('chunkById/eachById over a filtered query (#1090)', () => {
92 it('chunkById walks every matching row', async () => {
93 const seen: number[] = []
94 await q().where('b', '=', 1).chunkById(3, 'id', (rows: any[]) => {
95 seen.push(...rows.map(r => r.id))
96 })
97
98 // Was: threw on the second chunk. Note this spans four chunks — a fixture
99 // that fitted in one would have passed against the bug.
100 expect(seen).toEqual([1, 3, 5, 7, 9, 11, 13, 15, 17, 19])
101 })
102
103 it('eachById visits every matching row', async () => {
104 const seen: number[] = []
105 await q().where('b', '=', 1).eachById(3, 'id', (row: any) => {
106 seen.push(row.id)
107 })
108
109 expect(seen).toEqual([1, 3, 5, 7, 9, 11, 13, 15, 17, 19])
110 })
111
112 it('chunkById is unchanged on an unfiltered query', async () => {
113 const seen: number[] = []
114 await q().chunkById(3, 'id', (rows: any[]) => {
115 seen.push(...rows.map(r => r.id))
116 })
117
118 expect(seen).toEqual(Array.from({ length: 20 }, (_, i) => i + 1))
119 })
120})
121
122describe('cursorPaginate argument validation (#1090)', () => {
123 it('refuses to be combined with orderBy', async () => {
124 // A cursor predicate `col > ?` only means "the rows after this one" if the
125 // result is sorted by that column, so a second ordering makes the cursor
126 // meaningless. This always emitted a duplicate ORDER BY and failed to
127 // parse; it was invisible because the duplicate-WHERE error fired first.
128 await expect(q().where('b', '=', 1).orderBy('id').cursorPaginate(3, 5, 'id'))
129 .rejects.toThrow(/cannot be combined with orderBy/)
130 })
131
132 it('refuses a non-positive page size', async () => {
133 await expect(q().cursorPaginate(0, 1, 'id')).rejects.toThrow(/expected positive integer/)
134 })
135
136 it('refuses a column that is not an identifier', async () => {
137 await expect(q().cursorPaginate(3, 1, 'id; DROP TABLE cp_rows')).rejects.toThrow(/Invalid identifier/)
138 // And the table is still there.
139 expect((await sql.unsafe('SELECT count(*) as c FROM cp_rows'))[0].c).toBe(20)
140 })
141})