ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): choose WHERE vs AND from builder state, not a text search

#1118
Merged glennmichael123 wants to merge fix/1113-where-keyword-state into main
2 files +235 -27

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+55-27
@@ -6966,6 +6966,31 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
69666966
69676967 let sqlText = `UPDATE ${quoteId(String(table))}`
69686968
6969 /**
6970 * Whether this builder has already emitted a predicate — which is the
6971 * thing the keyword actually depends on.
6972 *
6973 * It used to be inferred by testing the whole statement for
6974 * `/\bWHERE\b/`, and plenty of text that is not this builder's predicate
6975 * satisfies that: a subquery inside `set()`, a table named `where`, a
6976 * column named `where`. The first real predicate then came out as `AND`
6977 * and fused onto the SET expression:
6978 *
6979 * SET "flag" = (SELECT ... WHERE x.id > 3) AND "id" = ?
6980 *
6981 * which leaves the UPDATE with no WHERE at all. Postgres rejects that
6982 * only when the types happen not to line up; where the SET target is
6983 * boolean, or on SQLite, it runs against every row and writes the
6984 * predicate into the value. See #1113.
6985 *
6986 * A boolean cannot be fooled by any of that.
6987 */
6988 let hasPredicate = false
6989 const appendPredicate = (predicate: string): void => {
6990 sqlText = `${sqlText} ${hasPredicate ? 'AND' : 'WHERE'} ${predicate}`
6991 hasPredicate = true
6992 }
6993
69696994 return {
69706995 set(values) {
69716996 const keys = Object.keys(values)
@@ -6992,9 +7017,6 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
69927017 return this
69937018 },
69947019 where(expr: any, op?: string, value?: any) {
6995 // Helper to get the correct keyword (WHERE for first condition, AND for subsequent)
6996 const getWhereKeyword = () => SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
6997
69987020 // Handle 3-arg format: where('column', '=', value)
69997021 if (op !== undefined && (typeof expr === 'string' || isRawExpression(expr) || isBoundSqlExpression(expr))) {
70007022 const safeOperator = assertSafeWhereOperator(op, 'updateTable.where')
@@ -7020,14 +7042,14 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
70207042 if (safeOperator.toLowerCase() === 'in' || safeOperator.toLowerCase() === 'not in') {
70217043 const values = Array.isArray(value) ? value : [value]
70227044 const placeholders = getPlaceholders(values.length, params.length + 1)
7023 sqlText = `${sqlText} ${getWhereKeyword()} ${left} ${safeOperator.toUpperCase()} (${placeholders})`
7045 appendPredicate(`${left} ${safeOperator.toUpperCase()} (${placeholders})`)
70247046 params.push(...values)
70257047 built = _sql.unsafe(sqlText, params)
70267048 return this
70277049 }
70287050
70297051 const paramIndex = params.length + 1
7030 sqlText = `${sqlText} ${getWhereKeyword()} ${left} ${safeOperator} ${getPlaceholder(paramIndex)}`
7052 appendPredicate(`${left} ${safeOperator} ${getPlaceholder(paramIndex)}`)
70317053 params.push(value)
70327054 built = _sql.unsafe(sqlText, params)
70337055 return this
@@ -7041,14 +7063,14 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
70417063 if (safeOperator.toLowerCase() === 'in' || safeOperator.toLowerCase() === 'not in') {
70427064 const values = Array.isArray(val) ? val : [val]
70437065 const placeholders = getPlaceholders(values.length, params.length + 1)
7044 sqlText = `${sqlText} ${getWhereKeyword()} ${quoteId(String(col))} ${safeOperator.toUpperCase()} (${placeholders})`
7066 appendPredicate(`${quoteId(String(col))} ${safeOperator.toUpperCase()} (${placeholders})`)
70457067 params.push(...values)
70467068 built = _sql.unsafe(sqlText, params)
70477069 return this
70487070 }
70497071
70507072 const paramIndex = params.length + 1
7051 sqlText = `${sqlText} ${getWhereKeyword()} ${quoteId(String(col))} ${safeOperator} ${getPlaceholder(paramIndex)}`
7073 appendPredicate(`${quoteId(String(col))} ${safeOperator} ${getPlaceholder(paramIndex)}`)
70527074 params.push(val)
70537075 built = _sql.unsafe(sqlText, params)
70547076 return this
@@ -7063,13 +7085,13 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
70637085 // expression is an object and would otherwise be read as the column
70647086 // map `{ sql: ... }`. See #1101.
70657087 if (isRawExpression(expr)) {
7066 sqlText = `${sqlText} ${getWhereKeyword()} ${expr.raw}`
7088 appendPredicate(expr.raw)
70677089 built = _sql.unsafe(sqlText, params)
70687090 return this
70697091 }
70707092 if (isBoundSqlExpression(expr)) {
70717093 const rendered = renderBoundSqlExpression(expr, params.length + 1)
7072 sqlText = `${sqlText} ${getWhereKeyword()} ${rendered.text}`
7094 appendPredicate(rendered.text)
70737095 params.push(...rendered.parameters)
70747096 built = _sql.unsafe(sqlText, params)
70757097 return this
@@ -7087,7 +7109,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
70877109 conditions[i] = `${quoteId(keys[i])} = ${getPlaceholder(baseIdx + i + 1)}`
70887110 params.push((expr as any)[keys[i]])
70897111 }
7090 sqlText = `${sqlText} ${getWhereKeyword()} ${conditions.join(' AND ')}`
7112 appendPredicate(conditions.join(' AND '))
70917113 built = _sql.unsafe(sqlText, params)
70927114 return this
70937115 }
@@ -7104,14 +7126,12 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
71047126 throw new TypeError(`[query-builder] updateTable.where(): expected a condition, got ${expr === null ? 'null' : typeof expr}. Refusing to run an UPDATE with no WHERE — drop the where() call if updating every row is intended.`)
71057127 },
71067128 whereNull(column: string) {
7107 const keyword = SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
7108 sqlText = `${sqlText} ${keyword} ${quoteId(String(column))} IS NULL`
7129 appendPredicate(`${quoteId(String(column))} IS NULL`)
71097130 built = _sql.unsafe(sqlText, params)
71107131 return this
71117132 },
71127133 whereNotNull(column: string) {
7113 const keyword = SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
7114 sqlText = `${sqlText} ${keyword} ${quoteId(String(column))} IS NOT NULL`
7134 appendPredicate(`${quoteId(String(column))} IS NOT NULL`)
71157135 built = _sql.unsafe(sqlText, params)
71167136 return this
71177137 },
@@ -7237,13 +7257,21 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
72377257 const delParams: any[] = []
72387258 let whereCondition: any = null
72397259
7240 // First .where() emits ` WHERE `; subsequent calls emit ` AND `.
7260 // First predicate emits ` WHERE `; subsequent ones emit ` AND `.
72417261 // Without this, chained `.where('a', '=', 1).where('b', '=', 2)`
72427262 // compiled to `... WHERE a = ? WHERE b = ?` and SQLite 500'd
7243 // with `near "WHERE": syntax error`. Mirrors the same helper in
7244 // updateTable() at line ~5454.
7263 // with `near "WHERE": syntax error`.
72457264 // See https://github.com/stacksjs/bun-query-builder/issues/1015
7246 const getWhereKeyword = () => SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
7265 //
7266 // Tracked as builder state rather than read back out of the statement:
7267 // `DELETE FROM "where"` matches /\bWHERE\b/ too, and inferring it from
7268 // the text turned the first predicate into `AND` — `DELETE FROM "where"
7269 // AND "id" = ?`. Mirrors updateTable(). See #1113.
7270 let hasPredicate = false
7271 const appendPredicate = (predicate: string): void => {
7272 sqlText += ` ${hasPredicate ? 'AND' : 'WHERE'} ${predicate}`
7273 hasPredicate = true
7274 }
72477275
72487276 const ensureDelBuilt = () => {
72497277 if (built === null) {
@@ -7271,14 +7299,14 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
72717299 if (safeOperator.toLowerCase() === 'in' || safeOperator.toLowerCase() === 'not in') {
72727300 const values = Array.isArray(value) ? value : [value]
72737301 const placeholders = getPlaceholders(values.length, delParams.length + 1)
7274 sqlText += ` ${getWhereKeyword()} ${quoteId(expr)} ${safeOperator.toUpperCase()} (${placeholders})`
7302 appendPredicate(`${quoteId(expr)} ${safeOperator.toUpperCase()} (${placeholders})`)
72757303 delParams.push(...values)
72767304 built = null
72777305 return this
72787306 }
72797307
72807308 const paramIndex = delParams.length + 1
7281 sqlText += ` ${getWhereKeyword()} ${quoteId(expr)} ${safeOperator} ${getPlaceholder(paramIndex)}`
7309 appendPredicate(`${quoteId(expr)} ${safeOperator} ${getPlaceholder(paramIndex)}`)
72827310 delParams.push(value)
72837311 built = null
72847312 return this
@@ -7291,14 +7319,14 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
72917319 if (safeOperator.toLowerCase() === 'in' || safeOperator.toLowerCase() === 'not in') {
72927320 const values = Array.isArray(val) ? val : [val]
72937321 const placeholders = getPlaceholders(values.length, delParams.length + 1)
7294 sqlText += ` ${getWhereKeyword()} ${quoteId(String(col))} ${safeOperator.toUpperCase()} (${placeholders})`
7322 appendPredicate(`${quoteId(String(col))} ${safeOperator.toUpperCase()} (${placeholders})`)
72957323 delParams.push(...values)
72967324 built = null
72977325 return this
72987326 }
72997327
73007328 const paramIndex = delParams.length + 1
7301 sqlText += ` ${getWhereKeyword()} ${quoteId(String(col))} ${safeOperator} ${getPlaceholder(paramIndex)}`
7329 appendPredicate(`${quoteId(String(col))} ${safeOperator} ${getPlaceholder(paramIndex)}`)
73027330 delParams.push(val)
73037331 built = null
73047332 return this
@@ -7311,13 +7339,13 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
73117339 // `where(raw('id = 1'))` with "invalid input syntax for type
73127340 // boolean". The signature accepts SqlFragment, so it has to work.
73137341 if (isRawExpression(expr)) {
7314 sqlText += ` ${getWhereKeyword()} ${expr.raw}`
7342 appendPredicate(expr.raw)
73157343 built = null
73167344 return this
73177345 }
73187346 if (isBoundSqlExpression(expr)) {
73197347 const rendered = renderBoundSqlExpression(expr, delParams.length + 1)
7320 sqlText += ` ${getWhereKeyword()} ${rendered.text}`
7348 appendPredicate(rendered.text)
73217349 delParams.push(...rendered.parameters)
73227350 built = null
73237351 return this
@@ -7334,7 +7362,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
73347362 conditions.push(`${quoteId(key)} = ${getPlaceholder(paramIndex)}`)
73357363 delParams.push((expr as any)[key])
73367364 }
7337 sqlText += ` ${getWhereKeyword()} ${conditions.join(' AND ')}`
7365 appendPredicate(conditions.join(' AND '))
73387366 built = null
73397367 return this
73407368 }
@@ -7350,12 +7378,12 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
73507378 throw new TypeError(`[query-builder] deleteFrom.where(): expected a condition, got ${expr === null ? 'null' : typeof expr}. Refusing to run a DELETE with no WHERE — drop the where() call if deleting every row is intended.`)
73517379 },
73527380 whereNull(column: string) {
7353 sqlText += ` ${getWhereKeyword()} ${quoteId(String(column))} IS NULL`
7381 appendPredicate(`${quoteId(String(column))} IS NULL`)
73547382 built = null
73557383 return this
73567384 },
73577385 whereNotNull(column: string) {
7358 sqlText += ` ${getWhereKeyword()} ${quoteId(String(column))} IS NOT NULL`
7386 appendPredicate(`${quoteId(String(column))} IS NOT NULL`)
73597387 built = null
73607388 return this
73617389 },
packages/bun-query-builder/test/client.where-keyword-state.test.tsadded+180-0
@@ -0,0 +1,180 @@
1/**
2 * The WHERE/AND keyword follows builder state, not a text search.
3 * stacksjs/bun-query-builder#1113.
4 *
5 * Both write builders chose their keyword by testing the statement built so
6 * far for `/\bWHERE\b/`:
7 *
8 * const getWhereKeyword = () => SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
9 *
10 * Plenty of text that is not this builder's predicate satisfies that. A
11 * subquery inside `set()`, a table named `where`, a column named `where` —
12 * each one made the FIRST predicate come out as `AND`, which fuses it onto
13 * whatever preceded it:
14 *
15 * UPDATE "t" SET "flag" = (SELECT ... WHERE x.id > 3) AND "id" = ?
16 * DELETE FROM "where" AND "id" = ?
17 *
18 * The DELETE is a syntax error. The UPDATE is worse: it parses, the predicate
19 * becomes part of the value being written, and the statement has no WHERE, so
20 * it rewrites every row. Postgres only rejects it when the SET target's type
21 * happens not to be boolean; SQLite takes it as written.
22 *
23 * That last case is why the UPDATE assertions here seed a non-zero column and
24 * check every row. The reported repro writes `0` to rows that already held
25 * `0`, so the damage is invisible unless the fixture makes it visible.
26 */
27
28import { mkdtempSync, rmSync } from 'node:fs'
29import { tmpdir } from 'node:os'
30import { join } from 'node:path'
31import { Database } from 'bun:sqlite'
32import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
33import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
34import { config, setConfig } from '../src/config'
35import { resetConnection } from '../src/db'
36
37// `where` is a legal identifier in every dialect this builder targets, as long
38// as it is quoted — and the builder does quote it.
39const MODELS = {
40 t: {
41 columns: {
42 id: { type: 'integer', isPrimaryKey: true },
43 name: { type: 'string' },
44 flag: { type: 'integer' },
45 },
46 },
47 where: {
48 columns: {
49 id: { type: 'integer', isPrimaryKey: true },
50 name: { type: 'string' },
51 },
52 },
53 hasreserved: {
54 columns: {
55 id: { type: 'integer', isPrimaryKey: true },
56 where: { type: 'string' },
57 },
58 },
59} as any
60
61let dir: string
62let dbPath: string
63let snapshot: { dialect: string, database: Record<string, unknown> }
64
65function qb(): any {
66 return createQueryBuilder({
67 schema: buildDatabaseSchema(MODELS),
68 meta: buildSchemaMeta(MODELS),
69 autoMigration: { enabled: false } as any,
70 })
71}
72
73function all(table: string): any[] {
74 const probe = new Database(dbPath)
75 const out = probe.query(`SELECT * FROM "${table}" ORDER BY id`).all() as any[]
76 probe.close()
77 return out
78}
79
80beforeEach(() => {
81 snapshot = { dialect: config.dialect, database: { ...config.database } }
82 dir = mkdtempSync(join(tmpdir(), 'qb-1113-'))
83 dbPath = join(dir, 'kw.sqlite')
84 const seed = new Database(dbPath, { create: true })
85 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT, flag INTEGER)')
86 // Non-zero flags: an UPDATE that reaches every row has to be visible.
87 seed.run(`INSERT INTO t (id,name,flag) VALUES (1,'a',7),(2,'b',7),(3,'c',7),(4,'d',7)`)
88 seed.run('CREATE TABLE "where" (id INTEGER PRIMARY KEY, name TEXT)')
89 seed.run(`INSERT INTO "where" (id,name) VALUES (1,'a'),(2,'b')`)
90 seed.run('CREATE TABLE hasreserved (id INTEGER PRIMARY KEY, "where" TEXT)')
91 seed.run(`INSERT INTO hasreserved (id,"where") VALUES (1,'a'),(2,'b')`)
92 seed.close()
93 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
94 resetConnection()
95})
96
97afterEach(() => {
98 config.dialect = snapshot.dialect as any
99 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
100 Object.assign(config.database, snapshot.database)
101 resetConnection()
102 rmSync(dir, { recursive: true, force: true })
103})
104
105describe('a WHERE inside a set() fragment does not steal the keyword (#1113)', () => {
106 it('emits WHERE for the predicate, not AND', () => {
107 const sql = String(qb()
108 .updateTable('t')
109 .set({ flag: raw('(SELECT count(*) FROM t x WHERE x.id > 3)') })
110 .where({ id: 1 })
111 .toSQL())
112
113 expect(sql).toContain(') WHERE "id"')
114 expect(sql).not.toContain(') AND "id"')
115 })
116
117 it('updates only the row the predicate selects', async () => {
118 await qb()
119 .updateTable('t')
120 .set({ flag: raw('(SELECT count(*) FROM t x WHERE x.id > 3)') })
121 .where({ id: 1 })
122 .execute()
123
124 // Previously every row was rewritten: the predicate became part of the
125 // value, so rows 2-4 were assigned `(1) AND (id = 1)` = 0 and row 1 got 1.
126 // Seeding 7 is what makes that visible.
127 expect(all('t').map(r => r.flag)).toEqual([1, 7, 7, 7])
128 })
129})
130
131describe('an identifier named `where` does not steal the keyword (#1113)', () => {
132 it('a table named `where` still gets a WHERE clause on DELETE', async () => {
133 const sql = String(qb().deleteFrom('where').where({ id: 1 }).toSQL())
134 expect(sql).toBe('DELETE FROM "where" WHERE "id" = ?')
135
136 // It used to emit `DELETE FROM "where" AND "id" = ?` — a syntax error.
137 await qb().deleteFrom('where').where({ id: 1 }).execute()
138 expect(all('where').map(r => r.id)).toEqual([2])
139 })
140
141 it('a table named `where` still gets a WHERE clause on UPDATE', async () => {
142 await qb().updateTable('where').set({ name: 'X' }).where({ id: 1 }).execute()
143 expect(all('where').map(r => r.name)).toEqual(['X', 'b'])
144 })
145
146 it('a column named `where` in set() does not absorb the predicate', async () => {
147 await qb().updateTable('hasreserved').set({ where: 'X' }).where({ id: 1 }).execute()
148 expect(all('hasreserved').map(r => r.where)).toEqual(['X', 'b'])
149 })
150})
151
152describe('chained predicates still join with AND (#1015 stays fixed)', () => {
153 it('UPDATE: second where() is AND', () => {
154 const sql = String(qb().updateTable('t').set({ name: 'X' }).where('id', '=', 1).where('name', '=', 'a').toSQL())
155 expect(sql).toContain('WHERE "id" = ?')
156 expect(sql).toContain('AND "name" = ?')
157 })
158
159 it('DELETE: second where() is AND', () => {
160 const sql = String(qb().deleteFrom('t').where('id', '=', 1).where('name', '=', 'a').toSQL())
161 expect(sql).toContain('WHERE "id" = ?')
162 expect(sql).toContain('AND "name" = ?')
163 })
164
165 it('DELETE: whereNull then whereNotNull', () => {
166 const sql = String(qb().deleteFrom('t').whereNull('name').whereNotNull('flag').toSQL())
167 expect(sql).toBe('DELETE FROM "t" WHERE "name" IS NULL AND "flag" IS NOT NULL')
168 })
169
170 it('UPDATE: a raw fragment first, then a column predicate', () => {
171 const sql = String(qb().updateTable('t').set({ name: 'X' }).where(raw('id > 2')).where('name', '=', 'c').toSQL())
172 expect(sql).toContain('WHERE id > 2')
173 expect(sql).toContain('AND "name" = ?')
174 })
175
176 it('DELETE: a raw fragment carrying its own WHERE is still the first predicate', () => {
177 const sql = String(qb().deleteFrom('t').where(raw('id IN (SELECT id FROM t x WHERE x.id > 3)')).whereNotNull('name').toSQL())
178 expect(sql).toBe('DELETE FROM "t" WHERE id IN (SELECT id FROM t x WHERE x.id > 3) AND "name" IS NOT NULL')
179 })
180})