also looking at this
fix(client): the cursor predicate is a where term, not a second WHERE (#1090)
#1098
2 files
+213
-18
| @@ -186,6 +186,26 @@ function renderSelectColumn(col: unknown): string { | ||
| 186 | 186 | ) |
| 187 | 187 | } |
| 188 | 188 | |
| 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 | */ | |
| 196 | function 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 | ||
| 189 | 209 | // Pre-compiled regex patterns for performance |
| 190 | 210 | const SQL_PATTERNS = { |
| 191 | 211 | SELECT_STAR: /^SELECT\s+\*/i, |
| @@ -3297,9 +3317,14 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti | ||
| 3297 | 3317 | * |
| 3298 | 3318 | * Splicing rather than appending also fixes `.where(a).orderBy(c).orWhere(b)`, |
| 3299 | 3319 | * 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. | |
| 3300 | 3325 | */ |
| 3301 | const currentSql = (): string => { | |
| 3302 | const body = renderWhereTerms(whereTerms) | |
| 3326 | const currentSql = (extra?: WhereTerm): string => { | |
| 3327 | const body = renderWhereTerms(extra ? [...whereTerms, extra] : whereTerms) | |
| 3303 | 3328 | if (!body) |
| 3304 | 3329 | return text |
| 3305 | 3330 | if (whereTail) { |
| @@ -5608,29 +5633,58 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti | ||
| 5608 | 5633 | return { data: hasMore ? data.slice(0, perPage) : data, meta: { perPage, page: p, hasMore } } |
| 5609 | 5634 | }, |
| 5610 | 5635 | 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 = '' | |
| 5612 | 5652 | if (cursor !== undefined && cursor !== null) { |
| 5613 | 5653 | 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(', ')})` | |
| 5619 | 5656 | } |
| 5620 | 5657 | 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)}` | |
| 5624 | 5660 | } |
| 5625 | 5661 | } |
| 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 | ) | |
| 5633 | 5684 | } |
| 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}`) | |
| 5634 | 5688 | const rows = await runWithHooks<any[]>(q, 'select', { signal: abortSignal, timeoutMs }) |
| 5635 | 5689 | // We fetch perPage+1 rows to detect whether more exist; the extra row |
| 5636 | 5690 | // is only a "has more?" probe and is NOT delivered. The next cursor |