ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): place clauses by statement structure, not by text search

#1129
Merged chris wants to merge fix/1121-shared-sql-scanner into main
3 files +392 -72

Review threads live on the whole diff, not on one commit, so none are shown here - a thread's line means something in the branch's final form, and painting it into an intermediate step would put it on code it is not about.

packages/bun-query-builder/src/client.tsmodified+66-72
@@ -11,7 +11,7 @@ import { bunSql, getOrCreateBunSql, resetConnection } from './db'
1111import { resolvePivot } from './pivot'
1212import { singularizerFor } from './inflect'
1313import type { WhereTerm } from './sql-fragments'
14import { FALSE_PREDICATE, renderInPredicate, renderWhereTerms } from './sql-fragments'
14import { FALSE_PREDICATE, renderInPredicate, renderWhereTerms, scanTopLevelKeywords } from './sql-fragments'
1515
1616export { resetConnection }
1717
@@ -2925,6 +2925,49 @@ function sleep(ms: number): Promise<void> {
29252925const reorderCache = new Map<string, string>()
29262926const REORDER_CACHE_MAX = 500
29272927
2928/**
2929 * Clause keywords, as one table per scan site, hoisted to module scope.
2930 *
2931 * A regex literal is constructed each time its expression is evaluated, so
2932 * building these inside the builder cost ~19 allocations per query. They hold
2933 * no state between calls: `scanTopLevelKeywords` sets `lastIndex` before every
2934 * exec, which is the only thing the sticky flag carries.
2935 */
2936const REORDER_KEYWORDS = [
2937 { key: 'GROUP_BY', pattern: /GROUP\s+BY\b/iy },
2938 { key: 'ORDER_BY', pattern: /ORDER\s+BY\b/iy },
2939 { key: 'HAVING', pattern: /HAVING\b/iy },
2940 { key: 'OFFSET', pattern: /OFFSET\b/iy },
2941 { key: 'LIMIT', pattern: /LIMIT\b/iy },
2942 { key: 'WHERE', pattern: /WHERE\b/iy },
2943 ]
2944
2945const TAIL_CLAUSE = [
2946 { key: 'tail', pattern: /GROUP\s+BY\b/iy },
2947 { key: 'tail', pattern: /ORDER\s+BY\b/iy },
2948 { key: 'tail', pattern: /HAVING\b/iy },
2949 { key: 'tail', pattern: /LIMIT\b/iy },
2950 { key: 'tail', pattern: /OFFSET\b/iy },
2951 { key: 'tail', pattern: /WINDOW\b/iy },
2952 { key: 'tail', pattern: /UNION\b/iy },
2953 { key: 'tail', pattern: /INTERSECT\b/iy },
2954 { key: 'tail', pattern: /EXCEPT\b/iy },
2955 { key: 'tail', pattern: /FOR\s+UPDATE\b/iy },
2956 { key: 'tail', pattern: /FOR\s+SHARE\b/iy },
2957 { key: 'tail', pattern: /LOCK\s+IN\s+SHARE\s+MODE\b/iy },
2958]
2959
2960const JOIN_CUT = [
2961 { key: 'cut', pattern: /GROUP\s+BY\b/iy },
2962 { key: 'cut', pattern: /ORDER\s+BY\b/iy },
2963 { key: 'cut', pattern: /WHERE\b/iy },
2964 { key: 'cut', pattern: /HAVING\b/iy },
2965 { key: 'cut', pattern: /LIMIT\b/iy },
2966 { key: 'cut', pattern: /OFFSET\b/iy },
2967 { key: 'cut', pattern: /UNION\b/iy },
2968]
2969
2970
29282971function reorderSelectClauses(sql: string): string {
29292972 const hit = reorderCache.get(sql)
29302973 if (hit !== undefined) return hit
@@ -2940,56 +2983,13 @@ function computeReorderedClauses(sql: string): string {
29402983 // must be checked before single-word prefixes that would
29412984 // otherwise short-circuit them (e.g. "ORDER" alone isn't a clause
29422985 // start; "ORDER BY" is).
2943 const KEYWORDS: Array<{ key: 'WHERE' | 'GROUP_BY' | 'HAVING' | 'ORDER_BY' | 'LIMIT' | 'OFFSET', tokens: RegExp }> = [
2944 { key: 'GROUP_BY', tokens: /^GROUP\s+BY\b/i },
2945 { key: 'ORDER_BY', tokens: /^ORDER\s+BY\b/i },
2946 { key: 'HAVING', tokens: /^HAVING\b/i },
2947 { key: 'OFFSET', tokens: /^OFFSET\b/i },
2948 { key: 'LIMIT', tokens: /^LIMIT\b/i },
2949 { key: 'WHERE', tokens: /^WHERE\b/i },
2950 ]
29512986
2952 const positions: Array<{ key: string, start: number }> = []
2953 let depth = 0
2954 let inString = false
2955 let stringChar = ''
2956
2957 for (let i = 0; i < sql.length; i++) {
2958 const ch = sql[i]
2959 if (inString) {
2960 if (ch === stringChar) {
2961 // SQL escapes a quote by doubling it ('it''s'). Skip the pair.
2962 if (sql[i + 1] === stringChar) { i++; continue }
2963 inString = false
2964 }
2965 continue
2966 }
2967 if (ch === '\'' || ch === '"' || ch === '`') {
2968 inString = true
2969 stringChar = ch
2970 continue
2971 }
2972 if (ch === '(') { depth++; continue }
2973 if (ch === ')') { depth--; continue }
2974 if (depth !== 0) continue
2975 // Keyword must be word-boundary-led — i.e. preceded by whitespace
2976 // (or start of string, which never matches a clause).
2977 if (i === 0 || !/\s/.test(sql[i - 1])) continue
2978 // Cheap pre-filter: every clause keyword starts with G/O/H/L/W. Skip the
2979 // slice + regex battery (the hot part of this scan) for any other char.
2980 const lead = sql.charCodeAt(i) & ~32 // ASCII uppercase
2981 if (lead !== 71 && lead !== 79 && lead !== 72 && lead !== 76 && lead !== 87) continue
2982 const rest = sql.slice(i)
2983 for (const { key, tokens } of KEYWORDS) {
2984 const m = rest.match(tokens)
2985 if (!m) continue
2986 positions.push({ key, start: i })
2987 // Advance past the matched keyword so we don't re-detect it on
2988 // the next iteration of the outer loop.
2989 i += m[0].length - 1
2990 break
2991 }
2992 }
2987 // Shared with the WHERE and JOIN splices, so all three agree on what counts
2988 // as statement structure rather than caller text. This one already skipped
2989 // string literals; it did not skip comments, so `orderByRaw(raw('id desc /*
2990 // where clause */'))` had the comment's `where` promoted to a clause and the
2991 // statement cut at it. See #1121.
2992 const positions = scanTopLevelKeywords(sql, REORDER_KEYWORDS)
29932993
29942994 // Nothing to reorder if zero or one clause: zero clauses means the
29952995 // SQL is just SELECT…FROM, and a single clause is already in
@@ -3493,19 +3493,15 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
34933493 }
34943494
34953495 // Index of the first TOP-LEVEL clause that must follow WHERE, or -1.
3496 // Paren-depth scan so a subquery's own ORDER BY/LIMIT doesn't match.
3497 const TAIL_CLAUSE = /\(|\)|\b(?:GROUP\s+BY|HAVING|ORDER\s+BY|LIMIT|OFFSET|WINDOW|UNION|INTERSECT|EXCEPT|FOR\s+UPDATE|FOR\s+SHARE|LOCK\s+IN\s+SHARE\s+MODE)\b/gi
3496 //
3497 // Shared scanner: paren depth alone was not enough, because a raw fragment
3498 // is caller text. `selectRaw(raw("'a limit 3 b' as t"))` put `limit` inside
3499 // a string literal, the WHERE was spliced into the middle of it, and the
3500 // statement stayed valid with no WHERE at all — returning every row. See
3501 // #1121.
34983502 const firstTailIndex = (s: string): number => {
3499 TAIL_CLAUSE.lastIndex = 0
3500 let depth = 0
3501 let m: RegExpExecArray | null
3502 // eslint-disable-next-line no-cond-assign
3503 while ((m = TAIL_CLAUSE.exec(s))) {
3504 if (m[0] === '(') { depth++ }
3505 else if (m[0] === ')') { depth = Math.max(0, depth - 1) }
3506 else if (depth === 0) { return m.index }
3507 }
3508 return -1
3503 const hits = scanTopLevelKeywords(s, TAIL_CLAUSE, 1)
3504 return hits.length > 0 ? hits[0].start : -1
35093505 }
35103506
35113507 /**
@@ -3690,17 +3686,15 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
36903686 // end of `text`, so `.where(...).join(...)` emitted `... WHERE ... JOIN ...`
36913687 // (invalid on every dialect). Also invalidates `built`. Paren-depth scan so
36923688 // a subquery's inner WHERE doesn't get matched. See #1030.
3689 // Where the JOIN goes: before the first top-level trailing clause.
3690 //
3691 // Same shared scanner, and the same reason. This one also spelled its
3692 // multi-word keywords with a single literal space, so `GROUP\n BY` in a
3693 // fragment was missed entirely — the patterns below take any whitespace.
3694 // See #1121.
36933695 const insertJoin = (joinClause: string) => {
3694 const re = /\(|\)|\b(?:WHERE|GROUP BY|HAVING|ORDER BY|LIMIT|OFFSET|UNION)\b/gi
3695 let depth = 0
3696 let cut = -1
3697 let mm: RegExpExecArray | null
3698 // eslint-disable-next-line no-cond-assign
3699 while ((mm = re.exec(text))) {
3700 if (mm[0] === '(') { depth++ }
3701 else if (mm[0] === ')') { depth = Math.max(0, depth - 1) }
3702 else if (depth === 0) { cut = mm.index; break }
3703 }
3696 const hits = scanTopLevelKeywords(text, JOIN_CUT, 1)
3697 const cut = hits.length > 0 ? hits[0].start : -1
37043698 text = cut >= 0
37053699 ? `${text.slice(0, cut)}${joinClause} ${text.slice(cut)}`
37063700 : `${text} ${joinClause}`
packages/bun-query-builder/src/sql-fragments.tsmodified+148-0
@@ -188,3 +188,151 @@ function closingQuote(sql: string, start: number, quote: string): number {
188188
189189 return sql.length
190190}
191
192/**
193 * A top-level keyword found by {@link scanTopLevelKeywords}.
194 */
195export interface TopLevelKeyword {
196 /** The caller's label for the pattern that matched. */
197 key: string
198 /** Index in the source SQL where the keyword starts. */
199 start: number
200 /** Length of the matched text. */
201 length: number
202}
203
204/**
205 * Find clause keywords in `sql` that are genuinely clause keywords.
206 *
207 * Three separate scanners used to answer this question, each with its own idea
208 * of what SQL is, and a raw fragment is arbitrary caller text:
209 *
210 * firstTailIndex paren-depth only
211 * insertJoin paren-depth only
212 * computeReorderedClauses paren-depth + string literals
213 *
214 * So `selectRaw(raw("'a limit 3 b' as t"))` put the word `limit` where the
215 * first two would read it as a LIMIT clause, and the WHERE was spliced into
216 * the middle of a string literal — leaving a statement that is still valid,
217 * has no WHERE, and returns every row. The JOIN splice had the same hole, and
218 * the third scanner missed comments. See stacksjs/bun-query-builder#1121.
219 *
220 * This skips everything that is not executable statement structure:
221 *
222 * - parenthesised sub-expressions, so a subquery's own ORDER BY is not ours
223 * - `'…'`, `"…"` and `` `…` `` runs, where doubling escapes the delimiter
224 * - `-- …` line comments and `/* … *\/` block comments, which nest
225 * - `$$…$$` / `$tag$…$tag$` dollar-quoted strings, without mistaking a `$1`
226 * placeholder for the start of one
227 *
228 * Backslash escapes inside a literal are deliberately NOT honoured. Whether
229 * `'a\'` ends the string depends on the dialect and, on Postgres, on a server
230 * setting — so this matches the standard (and what the previous literal-aware
231 * scanner did) rather than inventing a rule that would be wrong somewhere.
232 *
233 * A keyword must be preceded by a non-word character, and never matches at
234 * index 0: a statement that opens with a clause keyword has nothing before it
235 * for the clause to attach to.
236 *
237 * @param sql the statement to scan
238 * @param keywords patterns to look for. Each MUST carry the sticky (`y`) flag;
239 * they are matched at a position rather than against a slice, and are tried
240 * in the order given — so longer keywords must come before any single-word
241 * prefix of them (`ORDER BY` before a hypothetical `ORDER`).
242 * @param limit stop after this many hits. Callers that only want the first
243 * clause boundary pass 1 rather than scanning the rest of the statement.
244 */
245export function scanTopLevelKeywords(
246 sql: string,
247 keywords: ReadonlyArray<{ key: string, pattern: RegExp }>,
248 limit = Number.POSITIVE_INFINITY,
249): TopLevelKeyword[] {
250 const hits: TopLevelKeyword[] = []
251 let depth = 0
252
253 // charCode comparisons rather than single-character strings and regexes:
254 // this walks every character of every statement the builder emits, and
255 // `sql[i]` allocates. See the note on the lead-letter check below.
256 const isWordCode = (c: number): boolean =>
257 (c >= 48 && c <= 57) || (c >= 65 && c <= 90) || (c >= 97 && c <= 122) || c === 95
258
259 for (let i = 0; i < sql.length; i++) {
260 const code = sql.charCodeAt(i)
261
262 // -- line comment: everything to the newline is prose.
263 if (code === 45 && sql.charCodeAt(i + 1) === 45) {
264 const newline = sql.indexOf('\n', i + 2)
265 if (newline === -1)
266 return hits
267 i = newline
268 continue
269 }
270
271 // /* block comment */ — Postgres nests these, so a naive indexOf('*\/')
272 // would stop at the inner terminator and resume scanning inside prose.
273 if (code === 47 && sql.charCodeAt(i + 1) === 42) {
274 let nesting = 1
275 let j = i + 2
276 while (j < sql.length && nesting > 0) {
277 if (sql[j] === '/' && sql[j + 1] === '*') { nesting++; j += 2 }
278 else if (sql[j] === '*' && sql[j + 1] === '/') { nesting--; j += 2 }
279 else j++
280 }
281 i = j - 1
282 continue
283 }
284
285 // Quoted run. A doubled delimiter is an escaped one and stays inside.
286 if (code === 39 || code === 34 || code === 96) {
287 let j = i + 1
288 while (j < sql.length) {
289 if (sql.charCodeAt(j) === code) {
290 // A doubled delimiter is an escaped one and stays inside.
291 if (sql.charCodeAt(j + 1) === code) { j += 2; continue }
292 break
293 }
294 j++
295 }
296 i = j
297 continue
298 }
299
300 // Dollar-quoted string. The tag is empty or an identifier, which is what
301 // separates `$$…$$` from the `$1` placeholders Postgres binds with.
302 if (code === 36) {
303 const opening = /^\$(?:[A-Z_][A-Z0-9_]*)?\$/i.exec(sql.slice(i))
304 if (opening) {
305 const closing = sql.indexOf(opening[0], i + opening[0].length)
306 i = closing === -1 ? sql.length : closing + opening[0].length - 1
307 continue
308 }
309 }
310
311 if (code === 40) { depth++; continue }
312 if (code === 41) { depth = Math.max(0, depth - 1); continue }
313 if (depth !== 0) continue
314 if (i === 0) continue
315
316 // Every clause keyword starts with a letter, so this rejects the large
317 // majority of positions before any regex runs. The scan replaced three
318 // separate ones, two of which jumped between matches of a single
319 // alternation regex — without this it walks characters they skipped.
320 const upper = code & ~32
321 if (upper < 65 || upper > 90) continue
322 if (isWordCode(sql.charCodeAt(i - 1))) continue
323
324 for (const { key, pattern } of keywords) {
325 pattern.lastIndex = i
326 const match = pattern.exec(sql)
327 if (!match) continue
328 hits.push({ key, start: i, length: match[0].length })
329 if (hits.length >= limit)
330 return hits
331 // Advance past it, so a multi-word keyword is not re-detected mid-match.
332 i += match[0].length - 1
333 break
334 }
335 }
336
337 return hits
338}
packages/bun-query-builder/test/client.clause-placement.test.tsadded+178-0
@@ -0,0 +1,178 @@
1/**
2 * Clause placement reads statement structure, not text that merely looks like it.
3 * stacksjs/bun-query-builder#1121.
4 *
5 * Three scanners decided where a clause goes, each with its own idea of what
6 * SQL is:
7 *
8 * firstTailIndex paren depth only
9 * insertJoin paren depth only
10 * computeReorderedClauses paren depth + string literals
11 *
12 * A raw fragment is arbitrary caller text, so a keyword inside a string
13 * literal or a comment was read as a clause and the splice landed inside it.
14 * The statement stayed VALID, which is what made this the worst of the family:
15 *
16 * SELECT *, 'a WHERE id = $1 limit 3 b' as t FROM users
17 * SELECT *, 'a INNER JOIN posts ON … where b' as tag FROM users
18 *
19 * The first has no WHERE and returns every row. The second never joins. Both
20 * run without error, so the assertions here are on ROW COUNTS against the
21 * control query — the emitted text alone would not prove the filter applied.
22 *
23 * All three now share one scanner, so they cannot disagree again.
24 */
25
26import { mkdtempSync, rmSync } from 'node:fs'
27import { tmpdir } from 'node:os'
28import { join } from 'node:path'
29import { Database } from 'bun:sqlite'
30import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
31import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
32import { config, setConfig } from '../src/config'
33import { resetConnection } from '../src/db'
34import { scanTopLevelKeywords } from '../src/sql-fragments'
35
36const MODELS = {
37 users: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
38 posts: { columns: { id: { type: 'integer', isPrimaryKey: true }, user_id: { type: 'integer' } } },
39} as any
40
41let dir: string
42let dbPath: string
43let snapshot: { dialect: string, database: Record<string, unknown> }
44
45function qb(): any {
46 return createQueryBuilder({
47 schema: buildDatabaseSchema(MODELS),
48 meta: buildSchemaMeta(MODELS),
49 autoMigration: { enabled: false } as any,
50 })
51}
52
53beforeEach(() => {
54 snapshot = { dialect: config.dialect, database: { ...config.database } }
55 dir = mkdtempSync(join(tmpdir(), 'qb-1121-'))
56 dbPath = join(dir, 'clause.sqlite')
57 const seed = new Database(dbPath, { create: true })
58 seed.run('CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)')
59 seed.run(`INSERT INTO users (id,name) VALUES (1,'a'),(2,'b'),(3,'c')`)
60 seed.run('CREATE TABLE posts (id INTEGER PRIMARY KEY, user_id INTEGER)')
61 seed.run('INSERT INTO posts (id,user_id) VALUES (10,1)')
62 seed.close()
63 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
64 resetConnection()
65})
66
67afterEach(() => {
68 config.dialect = snapshot.dialect as any
69 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
70 Object.assign(config.database, snapshot.database)
71 resetConnection()
72 rmSync(dir, { recursive: true, force: true })
73})
74
75describe('a keyword inside a string literal is not a clause (#1121)', () => {
76 it('the WHERE is not spliced into the literal, and the filter applies', async () => {
77 const q = () => qb().selectFrom('users').selectRaw(raw("'a limit 3 b' as t")).where('id', '=', 1)
78
79 expect(String(q().toSQL())).toContain("'a limit 3 b' as t")
80 expect(String(q().toSQL())).toContain('WHERE id = ?')
81
82 // The assertion that matters: previously 3 rows came back, not 1.
83 expect((await q().execute()).map((r: any) => r.id)).toEqual([1])
84 })
85
86 it('the JOIN is not spliced into the literal, and the join happens', async () => {
87 const q = () => qb().selectFrom('users').selectRaw(raw("'a where b' as tag"))
88 .innerJoin('posts', 'posts.user_id', '=', 'users.id')
89
90 expect(String(q().toSQL())).toContain("'a where b' as tag")
91
92 // Previously 3 unjoined rows; the control joins to exactly one.
93 expect((await q().execute()).length).toBe(1)
94 })
95
96 it('a genuine trailing clause is still found — the WHERE goes before LIMIT', () => {
97 const sql = String(qb().selectFrom('users').where('id', '=', 1).limit(2).toSQL())
98 expect(sql).toMatch(/WHERE id = \?.*LIMIT 2/)
99 })
100
101 it('a subquery\'s own clauses are still ignored', () => {
102 const sql = String(qb().selectFrom('users')
103 .selectRaw(raw('(SELECT id FROM posts ORDER BY id LIMIT 1) as first_post'))
104 .where('id', '=', 1)
105 .toSQL())
106
107 expect(sql).toContain('WHERE id = ?')
108 expect(sql).toContain('(SELECT id FROM posts ORDER BY id LIMIT 1)')
109 })
110})
111
112describe('a keyword inside a comment is not a clause (#1121)', () => {
113 it('a block comment does not get cut in half', () => {
114 const sql = String(qb().selectFrom('users').orderByRaw(raw('id desc /* where clause */')).limit(5).toSQL())
115
116 // Previously: `SELECT * FROM users where clause */ ORDER BY id desc /* LIMIT 5`
117 expect(sql).toBe('SELECT * FROM users ORDER BY id desc /* where clause */ LIMIT 5')
118 })
119
120 it('and the statement still runs', async () => {
121 const rows = await qb().selectFrom('users').orderByRaw(raw('id desc /* where clause */')).limit(5).execute()
122 expect(rows.map((r: any) => r.id)).toEqual([3, 2, 1])
123 })
124})
125
126describe('the shared scanner itself (#1121)', () => {
127 const KW = [{ key: 'where', pattern: /WHERE\b/iy }, { key: 'limit', pattern: /LIMIT\b/iy }]
128 const keys = (sql: string): string[] => scanTopLevelKeywords(sql, KW).map(h => h.key)
129
130 it('finds a real clause', () => {
131 expect(keys('SELECT * FROM t WHERE id = 1 LIMIT 2')).toEqual(['where', 'limit'])
132 })
133
134 it('skips single quotes, double quotes and backticks', () => {
135 expect(keys(`SELECT 'where', "where", \`where\` FROM t`)).toEqual([])
136 })
137
138 it('a doubled quote is an escaped one, not the end of the literal', () => {
139 expect(keys(`SELECT 'it''s where' FROM t`)).toEqual([])
140 })
141
142 it('skips line comments to end of line, but not past it', () => {
143 expect(keys('SELECT 1 -- where\nFROM t WHERE id = 1')).toEqual(['where'])
144 })
145
146 it('skips block comments, including nested ones', () => {
147 expect(keys('SELECT 1 /* where /* limit */ where */ FROM t WHERE id = 1')).toEqual(['where'])
148 })
149
150 it('skips dollar-quoted strings', () => {
151 expect(keys('SELECT $$ where $$ FROM t WHERE id = 1')).toEqual(['where'])
152 expect(keys('SELECT $tag$ where $tag$ FROM t WHERE id = 1')).toEqual(['where'])
153 })
154
155 it('does not mistake a $1 placeholder for a dollar quote', () => {
156 // If $1 opened a quoted run, everything after it would be skipped and the
157 // real WHERE would be invisible.
158 expect(keys('SELECT $1 FROM t WHERE id = $2 LIMIT 1')).toEqual(['where', 'limit'])
159 })
160
161 it('ignores anything inside parentheses', () => {
162 expect(keys('SELECT (SELECT 1 FROM x WHERE y LIMIT 1) FROM t')).toEqual([])
163 })
164
165 it('requires a non-word character before the keyword', () => {
166 expect(keys('SELECT * FROM somewhere')).toEqual([])
167 })
168
169 it('never matches at index 0', () => {
170 expect(keys('WHERE id = 1')).toEqual([])
171 })
172
173 it('an unterminated literal swallows the rest rather than resyncing', () => {
174 // The alternative — guessing where the caller meant it to end — is how a
175 // splice lands inside caller text in the first place.
176 expect(keys(`SELECT 'oops FROM t WHERE id = 1`)).toEqual([])
177 })
178})