ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): refuse a write whose predicate the builder cannot read

#1109
Merged glennmichael123 wants to merge fix/1101-silent-full-table-write into main
2 files +251 -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+69-5
@@ -6960,9 +6960,32 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
69606960 return this
69616961 }
69626962
6963 // A lone SQL fragment: `where(raw('id = 1'))`, `where(sql\`...\`)`.
6964 // The declared signature accepts SqlFragment, so this is an
6965 // advertised call — and it matched no branch, fell through to the
6966 // bare `return this` below, and left the UPDATE with no WHERE at all.
6967 // Every row was rewritten, silently, and the full count was reported
6968 // as success. Handled before the object branch because a bound
6969 // expression is an object and would otherwise be read as the column
6970 // map `{ sql: ... }`. See #1101.
6971 if (isRawExpression(expr)) {
6972 sqlText = `${sqlText} ${getWhereKeyword()} ${expr.raw}`
6973 built = _sql.unsafe(sqlText, params)
6974 return this
6975 }
6976 if (isBoundSqlExpression(expr)) {
6977 const rendered = renderBoundSqlExpression(expr, params.length + 1)
6978 sqlText = `${sqlText} ${getWhereKeyword()} ${rendered.text}`
6979 params.push(...rendered.parameters)
6980 built = _sql.unsafe(sqlText, params)
6981 return this
6982 }
6983
69636984 // Handle object format: where({ column: value })
6964 if (expr && typeof expr === 'object' && !('raw' in expr)) {
6985 if (expr && typeof expr === 'object') {
69656986 const keys = Object.keys(expr)
6987 if (keys.length === 0)
6988 throw new TypeError('[query-builder] updateTable.where({}): an empty object is not a filter. Pass a condition, or drop the where() call if updating every row is intended.')
69666989 const len = keys.length
69676990 const baseIdx = params.length
69686991 const conditions: string[] = Array.from({ length: len })
@@ -6972,8 +6995,19 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
69726995 }
69736996 sqlText = `${sqlText} ${getWhereKeyword()} ${conditions.join(' AND ')}`
69746997 built = _sql.unsafe(sqlText, params)
6998 return this
69756999 }
6976 return this
7000
7001 // Nothing above could turn this into a predicate. Refuse it.
7002 //
7003 // Returning `this` unchanged meant `where(undefined)` and
7004 // `where(null)` produced an UPDATE with no WHERE, so a filter the
7005 // caller believed they had applied rewrote the whole table without
7006 // an error. A write builder is the wrong place to read "I could not
7007 // understand your filter" as "match every row". The select builder
7008 // dropped this same fall-through in #1083; the write builders kept
7009 // it. See #1101.
7010 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.`)
69777011 },
69787012 whereNull(column: string) {
69797013 const keyword = SQL_PATTERNS.WHERE.test(sqlText) ? 'AND' : 'WHERE'
@@ -7153,9 +7187,31 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
71537187 built = null
71547188 return this
71557189 }
7190 // A lone SQL fragment, handled before the object branch for the same
7191 // reason as in updateTable.where: a bound expression is an object and
7192 // would otherwise be read as the column map `{ sql: ... }`. Rendering
7193 // it here also fixes it — routed through applyWhere() below, a
7194 // fragment was interpolated as a bound value, so Postgres rejected
7195 // `where(raw('id = 1'))` with "invalid input syntax for type
7196 // boolean". The signature accepts SqlFragment, so it has to work.
7197 if (isRawExpression(expr)) {
7198 sqlText += ` ${getWhereKeyword()} ${expr.raw}`
7199 built = null
7200 return this
7201 }
7202 if (isBoundSqlExpression(expr)) {
7203 const rendered = renderBoundSqlExpression(expr, delParams.length + 1)
7204 sqlText += ` ${getWhereKeyword()} ${rendered.text}`
7205 delParams.push(...rendered.parameters)
7206 built = null
7207 return this
7208 }
7209
71567210 // Object format: where({ id: 1 })
7157 if (expr && typeof expr === 'object' && !('raw' in expr)) {
7211 if (expr && typeof expr === 'object') {
71587212 const keys = Object.keys(expr)
7213 if (keys.length === 0)
7214 throw new TypeError('[query-builder] deleteFrom.where({}): an empty object is not a filter. Pass a condition, or drop the where() call if deleting every row is intended.')
71597215 const conditions: string[] = []
71607216 for (const key of keys) {
71617217 const paramIndex = delParams.length + 1
@@ -7166,8 +7222,16 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
71667222 built = null
71677223 return this
71687224 }
7169 built = applyWhere(({} as any), ensureDelBuilt(), expr)
7170 return this
7225
7226 // Nothing above could turn this into a predicate.
7227 //
7228 // This previously fell through to applyWhere({}, ..., expr), whose
7229 // first line is `if (!expr) return q` — so `where(undefined)` and
7230 // `where(null)` appended nothing and the DELETE removed every row,
7231 // returning the count as though the filter had been honoured. See
7232 // #1101, and #1083 where the select builder lost the same
7233 // fall-through.
7234 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.`)
71717235 },
71727236 whereNull(column: string) {
71737237 sqlText += ` ${getWhereKeyword()} ${quoteId(String(column))} IS NULL`
packages/bun-query-builder/test/client.write-predicate-required.test.tsadded+182-0
@@ -0,0 +1,182 @@
1/**
2 * A write must never run without the predicate the caller supplied.
3 * stacksjs/bun-query-builder#1101 (and the empty-object half of #1095).
4 *
5 * `updateTable(...).where(x)` and `deleteFrom(...).where(x)` accepted argument
6 * shapes they could not turn into a predicate and then returned `this`
7 * unchanged. The statement kept no WHERE at all, so it ran against every row
8 * and reported the full affected count as success:
9 *
10 * deleteFrom(t).where(undefined) -> deleted all 4 rows
11 * updateTable(t).set(v).where(raw('id = 1')) -> rewrote all 4 rows
12 *
13 * The `raw()` case is the serious one, because it is not a mistake the types
14 * catch: the declared signature is `WhereExpression | K | SqlFragment` and
15 * `raw` is a public export, so that call reads correctly and passes review.
16 *
17 * Two different remedies, deliberately:
18 * - a fragment is SUPPORTED, because the signature promises it,
19 * - anything the builder genuinely cannot read is REFUSED, because a write
20 * builder is the wrong place to treat "I could not understand your filter"
21 * as "match every row".
22 *
23 * `where({})` throws with a message naming the problem instead of the SQL
24 * syntax error it used to produce — that is #1095, whose reported symptom was
25 * always the safe one.
26 *
27 * Every assertion below checks SURVIVING ROWS, not just that a call threw. The
28 * bug was damage, so the table is the thing worth asserting on.
29 */
30
31import { mkdtempSync, rmSync } from 'node:fs'
32import { tmpdir } from 'node:os'
33import { join } from 'node:path'
34import { Database } from 'bun:sqlite'
35import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
36import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
37import { config, setConfig } from '../src/config'
38import { resetConnection } from '../src/db'
39
40const MODELS = {
41 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
42} as any
43
44let dir: string
45let dbPath: string
46let snapshot: { dialect: string, database: Record<string, unknown> }
47
48function qb(): any {
49 return createQueryBuilder({
50 schema: buildDatabaseSchema(MODELS),
51 meta: buildSchemaMeta(MODELS),
52 autoMigration: { enabled: false } as any,
53 })
54}
55
56function rows(): Array<{ id: number, name: string }> {
57 const probe = new Database(dbPath)
58 const out = probe.query('SELECT id, name FROM t ORDER BY id').all() as any[]
59 probe.close()
60 return out
61}
62
63beforeEach(() => {
64 snapshot = { dialect: config.dialect, database: { ...config.database } }
65 dir = mkdtempSync(join(tmpdir(), 'qb-1101-'))
66 dbPath = join(dir, 'writes.sqlite')
67 const seed = new Database(dbPath, { create: true })
68 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
69 seed.run(`INSERT INTO t (id, name) VALUES (1,'a'),(2,'b'),(3,'c'),(4,'d')`)
70 seed.close()
71 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
72 resetConnection()
73})
74
75afterEach(() => {
76 config.dialect = snapshot.dialect as any
77 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
78 Object.assign(config.database, snapshot.database)
79 resetConnection()
80 rmSync(dir, { recursive: true, force: true })
81})
82
83describe('DELETE refuses to run without a predicate (#1101)', () => {
84 for (const [label, value] of [['undefined', undefined], ['null', null]] as const) {
85 it(`where(${label}) throws and leaves every row in place`, () => {
86 // Synchronously, at the point of the mistake — no statement is ever built.
87 expect(() => qb().deleteFrom('t').where(value as any))
88 .toThrow(/expected a condition/)
89
90 // The assertion that matters: all four rows are still here.
91 expect(rows()).toHaveLength(4)
92 })
93 }
94
95 it('where({}) names the problem instead of emitting a broken statement', () => {
96 expect(() => qb().deleteFrom('t').where({}))
97 .toThrow(/an empty object is not a filter/)
98 expect(rows()).toHaveLength(4)
99 })
100})
101
102describe('UPDATE refuses to run without a predicate (#1101)', () => {
103 for (const [label, value] of [['undefined', undefined], ['null', null]] as const) {
104 it(`where(${label}) throws and rewrites nothing`, () => {
105 expect(() => qb().updateTable('t').set({ name: 'X' }).where(value as any))
106 .toThrow(/expected a condition/)
107
108 expect(rows().filter(r => r.name === 'X')).toHaveLength(0)
109 })
110 }
111
112 it('where({}) names the problem instead of emitting a broken statement', () => {
113 expect(() => qb().updateTable('t').set({ name: 'X' }).where({}))
114 .toThrow(/an empty object is not a filter/)
115 expect(rows().filter(r => r.name === 'X')).toHaveLength(0)
116 })
117})
118
119describe('a SQL fragment is honoured, not dropped (#1101)', () => {
120 // The signature accepts SqlFragment, so these have to work — refusing them
121 // would swap a silent catastrophe for a broken advertised API.
122
123 it('DELETE where(raw(...)) removes only the matching row', async () => {
124 await qb().deleteFrom('t').where(raw('id = 1')).execute()
125
126 // Previously this threw on Postgres (the fragment was bound as a value)
127 // and, on the update side, wiped the table.
128 expect(rows().map(r => r.id)).toEqual([2, 3, 4])
129 })
130
131 it('UPDATE where(raw(...)) rewrites only the matching row', async () => {
132 await qb().updateTable('t').set({ name: 'X' }).where(raw('id = 1')).execute()
133
134 const after = rows()
135 expect(after.filter(r => r.name === 'X').map(r => r.id)).toEqual([1])
136 expect(after).toHaveLength(4)
137 })
138
139 it('DELETE honours a parameterised fragment', async () => {
140 await qb().deleteFrom('t').where({ sql: 'id = ?', parameters: [2] } as any).execute()
141 expect(rows().map(r => r.id)).toEqual([1, 3, 4])
142 })
143
144 it('UPDATE honours a parameterised fragment', async () => {
145 await qb().updateTable('t').set({ name: 'X' }).where({ sql: 'id = ?', parameters: [2] } as any).execute()
146 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([2])
147 })
148
149 it('does not mistake a bound fragment for a column map', async () => {
150 // `{ sql, parameters }` is an object, so an ordering slip would render it
151 // as `"sql" = ? AND "parameters" = ?` and match nothing — silently.
152 await qb().deleteFrom('t').where({ sql: 'id = ?', parameters: [3] } as any).execute()
153 expect(rows().map(r => r.id)).toEqual([1, 2, 4])
154 })
155})
156
157describe('ordinary writes are unaffected (#1101)', () => {
158 it('DELETE with an object filter still removes exactly its rows', async () => {
159 await qb().deleteFrom('t').where({ id: 1 }).execute()
160 expect(rows().map(r => r.id)).toEqual([2, 3, 4])
161 })
162
163 it('UPDATE with an object filter still rewrites exactly its rows', async () => {
164 await qb().updateTable('t').set({ name: 'X' }).where({ id: 2 }).execute()
165 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([2])
166 })
167
168 it('the 3-arg form still works on both builders', async () => {
169 await qb().updateTable('t').set({ name: 'X' }).where('id', '=', 3).execute()
170 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([3])
171
172 await qb().deleteFrom('t').where('id', '=', 4).execute()
173 expect(rows().map(r => r.id)).toEqual([1, 2, 3])
174 })
175
176 it('an intentional unfiltered write is still possible without where()', async () => {
177 // Refusing an unreadable filter must not remove the ability to write the
178 // whole table on purpose — the point is that it has to be explicit.
179 await qb().deleteFrom('t').execute()
180 expect(rows()).toHaveLength(0)
181 })
182})