ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): pick the select builder's keywords from state, not from text

#1126
Merged glennmichael123 wants to merge fix/1120-1122-select-keyword-state into main
2 files +198 -5

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+48-5
@@ -3413,6 +3413,28 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
34133413 // UNION/INTERSECT/EXCEPT belong to the RIGHT-hand SELECT and must render at
34143414 // the end of `text`, or their params would bind in the wrong order.
34153415 let whereTail = false
3416 /**
3417 * Whether the operand a tail predicate would attach to already carries one.
3418 *
3419 * Only consulted once `whereTail` is set. It used to be inferred by testing
3420 * the WHOLE statement for `/\bWHERE\b/`, but by then `text` holds the LEFT
3421 * select — WHERE and all — so a predicate on the left was read as one on
3422 * the right and the right side's first predicate came out as `AND`:
3423 *
3424 * SELECT * FROM a WHERE y = $1 UNION SELECT * FROM b AND x = $2
3425 *
3426 * which does not parse. Set from the right-hand operand alone. See #1120.
3427 */
3428 let tailHasPredicate = false
3429 /**
3430 * Whether this builder has emitted its HAVING.
3431 *
3432 * Same defect as #1113, different keyword: scanning `text` for
3433 * `/\bHAVING\b/` also matches a raw select fragment that merely contains
3434 * the word, so the first real HAVING was emitted as `AND` and fused onto
3435 * the GROUP BY list. See #1122.
3436 */
3437 let hasHaving = false
34163438
34173439 // Lazy building: don't prepare statement until execution
34183440 // built is initialized lazily to avoid expensive template tag calls on every query
@@ -3479,7 +3501,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
34793501 if (!body)
34803502 return text
34813503 if (whereTail) {
3482 const kw = SQL_PATTERNS.WHERE.test(text) ? 'AND' : 'WHERE'
3504 const kw = tailHasPredicate ? 'AND' : 'WHERE'
34833505 return `${text} ${kw} ${body}`
34843506 }
34853507 const cut = firstTailIndex(text)
@@ -3600,7 +3622,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
36003622 // the other side's bound params and renumbering its `$n` placeholders past
36013623 // ours on Postgres — previously the set-op appended text only, dropping the
36023624 // right side's params and colliding `$1`. See stacksjs/bun-query-builder#1029.
3603 const appendSetOp = (op: string, other: { toSQL: () => any, __rawState?: () => { sql: string, params: unknown[] } }) => {
3625 const appendSetOp = (op: string, other: { toSQL: () => any, __rawState?: () => { sql: string, params: unknown[] }, __hasPredicate?: () => boolean }) => {
36043626 // Params are one flat array in push order, so the WHERE text has to
36053627 // precede the set operator's params in the emitted string. Materialize it
36063628 // into `text` now; terms added after this point belong to the RIGHT-hand
@@ -3618,10 +3640,17 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
36183640 : st.sql
36193641 text += ` ${op} ${otherSql}`
36203642 whereParams.push(...st.params)
3643 // A predicate added after this point attaches to the RIGHT operand, so
3644 // the keyword depends on that operand alone — not on the statement,
3645 // which by now also holds the left side's WHERE. Ask the builder; only
3646 // scan when it is a foreign one that cannot answer. See #1120.
3647 tailHasPredicate = other.__hasPredicate?.() ?? SQL_PATTERNS.WHERE.test(otherSql)
36213648 }
36223649 else {
36233650 // Foreign builder without __rawState — fall back to text-only (no param merge).
3624 text += ` ${op} ${String(other.toSQL())}`
3651 const otherSql = String(other.toSQL())
3652 text += ` ${op} ${otherSql}`
3653 tailHasPredicate = other.__hasPredicate?.() ?? SQL_PATTERNS.WHERE.test(otherSql)
36253654 }
36263655 built = null
36273656 }
@@ -5614,12 +5643,15 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
56145643 having(expr: any) {
56155644 // Chained having() calls join with AND, not a second HAVING keyword
56165645 // (`HAVING a HAVING b` is invalid). See stacksjs/bun-query-builder#1034.
5617 const kw = /\bHAVING\b/i.test(text) ? 'AND' : 'HAVING'
5646 // Read from builder state, not from `text` — a raw select fragment that
5647 // merely contains the word made the first HAVING emit as AND. See #1122.
5648 const kw = hasHaving ? 'AND' : 'HAVING'
56185649 // Handle array format: ['COUNT(id)', '>', 3]
56195650 if (Array.isArray(expr)) {
56205651 const paramIdx = whereParams.length + 1
56215652 text = `${text} ${kw} ${expr[0]} ${expr[1]} ${getPlaceholder(paramIdx)}`
56225653 whereParams.push(expr[2])
5654 hasHaving = true
56235655 built = null
56245656 }
56255657 // Handle object format
@@ -5635,20 +5667,23 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
56355667 whereParams.push(expr[key])
56365668 }
56375669 text = `${text} ${kw} ${conditions.join(' AND ')}`
5670 hasHaving = true
56385671 built = null
56395672 }
56405673 }
56415674 // Handle raw expressions
56425675 else if (expr && typeof (expr as any).raw !== 'undefined') {
56435676 text += ` ${kw} ${(expr as any).raw}`
5677 hasHaving = true
56445678 built = null
56455679 }
56465680 return this as any
56475681 },
56485682 havingRaw(fragment: any) {
56495683 const frag = renderRawFragment(fragment, 'havingRaw(fragment)')
5650 const kw = /\bHAVING\b/i.test(text) ? 'AND' : 'HAVING'
5684 const kw = hasHaving ? 'AND' : 'HAVING'
56515685 text += ` ${kw} ${frag}`
5686 hasHaving = true
56525687 built = null
56535688 return this as any
56545689 },
@@ -6330,6 +6365,14 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
63306365 __rawState() {
63316366 return { sql: reorderSelectClauses(currentSql()), params: [...whereParams] }
63326367 },
6368 // Internal: does this builder's SQL already carry a predicate that a
6369 // further where() would have to join with AND? Used by appendSetOp on
6370 // the other side, so it can answer that about the right-hand operand
6371 // instead of scanning a statement that also contains the left one.
6372 // See stacksjs/bun-query-builder#1120.
6373 __hasPredicate() {
6374 return whereTerms.length > 0 || tailHasPredicate
6375 },
63336376 raw() {
63346377 return (ensureBuilt() as any).raw()
63356378 },
packages/bun-query-builder/test/client.select-keyword-state.test.tsadded+150-0
@@ -0,0 +1,150 @@
1/**
2 * The select builder's clause keywords follow builder state, not a text scan.
3 * stacksjs/bun-query-builder#1120 and #1122.
4 *
5 * Two sites picked a keyword by searching the statement built so far — the
6 * same defect #1113 fixed in the write builders, in a builder that kept it:
7 *
8 * const kw = SQL_PATTERNS.WHERE.test(text) ? 'AND' : 'WHERE' // :3482
9 * const kw = /\bHAVING\b/i.test(text) ? 'AND' : 'HAVING' // :5617, :5650
10 *
11 * #1120 — the WHERE branch is reached only after a set operator, and by then
12 * `text` holds the LEFT select including its WHERE. So a predicate on the left
13 * was read as one on the right:
14 *
15 * SELECT * FROM a WHERE y = $1 UNION SELECT * FROM b AND x = $2
16 *
17 * #1122 — `text` also holds the select list, so a raw fragment that merely
18 * contains the word `having` made the first real HAVING emit as AND, fusing it
19 * onto the GROUP BY list.
20 *
21 * Both emit invalid SQL rather than running against the wrong rows, so unlike
22 * #1113 these assert on the emitted statement — that is where the defect lives.
23 * The union cases also execute, because a keyword decision that produces valid
24 * SQL can still produce the wrong ROWS, and only running it settles that.
25 */
26
27import { mkdtempSync, rmSync } from 'node:fs'
28import { tmpdir } from 'node:os'
29import { join } from 'node:path'
30import { Database } from 'bun:sqlite'
31import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
32import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
33import { config, setConfig } from '../src/config'
34import { resetConnection } from '../src/db'
35
36const MODELS = {
37 a: { columns: { id: { type: 'integer', isPrimaryKey: true }, x: { type: 'integer' }, y: { type: 'integer' } } },
38 b: { columns: { id: { type: 'integer', isPrimaryKey: true }, x: { type: 'integer' }, y: { type: 'integer' } } },
39 users: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
40} as any
41
42let dir: string
43let dbPath: string
44let snapshot: { dialect: string, database: Record<string, unknown> }
45
46function qb(): any {
47 return createQueryBuilder({
48 schema: buildDatabaseSchema(MODELS),
49 meta: buildSchemaMeta(MODELS),
50 autoMigration: { enabled: false } as any,
51 })
52}
53
54beforeEach(() => {
55 snapshot = { dialect: config.dialect, database: { ...config.database } }
56 dir = mkdtempSync(join(tmpdir(), 'qb-1120-'))
57 dbPath = join(dir, 'sel.sqlite')
58 const seed = new Database(dbPath, { create: true })
59 seed.run('CREATE TABLE a (id INTEGER PRIMARY KEY, x INTEGER, y INTEGER)')
60 seed.run('CREATE TABLE b (id INTEGER PRIMARY KEY, x INTEGER, y INTEGER)')
61 seed.run('INSERT INTO a (id,x,y) VALUES (1,1,2),(2,5,9)')
62 seed.run('INSERT INTO b (id,x,y) VALUES (3,1,4),(4,7,8)')
63 seed.run('CREATE TABLE users (id INTEGER PRIMARY KEY, name TEXT)')
64 seed.run(`INSERT INTO users (id,name) VALUES (1,'a'),(2,'b'),(3,'b')`)
65 seed.close()
66 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
67 resetConnection()
68})
69
70afterEach(() => {
71 config.dialect = snapshot.dialect as any
72 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
73 Object.assign(config.database, snapshot.database)
74 resetConnection()
75 rmSync(dir, { recursive: true, force: true })
76})
77
78describe('a predicate after a set operator (#1120)', () => {
79 it('a WHERE on the left does not make the right one an AND', () => {
80 const sql = String(qb().selectFrom('a').where('y', '=', 2).union(qb().selectFrom('b')).where('x', '=', 1).toSQL())
81
82 // Previously: `... UNION SELECT * FROM b AND x = ?`, which does not parse.
83 expect(sql).toContain('UNION SELECT * FROM b WHERE x = ?')
84 })
85
86 it('and the statement actually runs', async () => {
87 const rows = await qb().selectFrom('a').where('y', '=', 2).union(qb().selectFrom('b')).where('x', '=', 1).execute()
88
89 expect(rows.map((r: any) => r.id).sort()).toEqual([1, 3])
90 })
91
92 it('with no predicate on the left, the right one is still WHERE', () => {
93 const sql = String(qb().selectFrom('a').union(qb().selectFrom('b')).where('x', '=', 1).toSQL())
94 expect(sql).toContain('UNION SELECT * FROM b WHERE x = ?')
95 })
96
97 it('when the RIGHT side has its own predicate, AND is correct and is what is emitted', () => {
98 const sql = String(qb().selectFrom('a').union(qb().selectFrom('b').where('y', '=', 4)).where('x', '=', 1).toSQL())
99
100 expect(sql).toContain('WHERE y = ? AND x = ?')
101 })
102
103 it('intersect behaves the same way', () => {
104 const sql = String(qb().selectFrom('a').where('y', '=', 2).intersect(qb().selectFrom('b')).where('x', '=', 1).toSQL())
105 expect(sql).toContain('INTERSECT SELECT * FROM b WHERE x = ?')
106 })
107
108 it('chained predicates on the tail still join with AND', () => {
109 const sql = String(qb().selectFrom('a').where('y', '=', 2).union(qb().selectFrom('b')).where('x', '=', 1).where('y', '=', 4).toSQL())
110 expect(sql).toContain('UNION SELECT * FROM b WHERE x = ? AND y = ?')
111 })
112})
113
114describe('HAVING is chosen from state, not from the select list (#1122)', () => {
115 it('havingRaw() after a raw fragment containing the word', () => {
116 const sql = String(qb().selectFrom('users').selectRaw(raw("'having a party' as t")).groupBy('id').havingRaw(raw('COUNT(id) > 3')).toSQL())
117
118 // Previously: `... GROUP BY id AND COUNT(id) > 3`.
119 expect(sql).toContain('GROUP BY id HAVING COUNT(id) > 3')
120 })
121
122 it('having() in its array form, same fragment', () => {
123 const sql = String(qb().selectFrom('users').selectRaw(raw("'having a party' as t")).groupBy('id').having(['COUNT(id)', '>', 3]).toSQL())
124
125 expect(sql).toContain('HAVING COUNT(id) > ?')
126 expect(sql).not.toContain('GROUP BY id AND')
127 })
128
129 it('chained having() calls still join with AND (#1034 stays fixed)', () => {
130 const sql = String(qb().selectFrom('users').groupBy('id').having(['COUNT(id)', '>', 1]).having(['COUNT(id)', '<', 9]).toSQL())
131
132 expect(sql).toContain('HAVING COUNT(id) > ?')
133 expect(sql).toContain('AND COUNT(id) < ?')
134 })
135
136 it('having() then havingRaw() joins with AND', () => {
137 const sql = String(qb().selectFrom('users').groupBy('id').having(['COUNT(id)', '>', 1]).havingRaw(raw('COUNT(id) < 9')).toSQL())
138
139 expect(sql).toContain('HAVING COUNT(id) > ?')
140 expect(sql).toContain('AND COUNT(id) < 9')
141 })
142
143 it('a having that appends nothing does not claim the keyword', () => {
144 // The empty-object branch emits no text, so the next call is still the
145 // first HAVING rather than a stray AND.
146 const sql = String(qb().selectFrom('users').groupBy('id').having({}).havingRaw(raw('COUNT(id) > 3')).toSQL())
147
148 expect(sql).toContain('HAVING COUNT(id) > 3')
149 })
150})