ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): updateMany runs with the caller's predicate, or not at all

#1119
Merged glennmichael123 wants to merge fix/1112-update-many-predicate into main
2 files +199 -11
packages/bun-query-builder/src/client.tsmodified+42-11
@@ -8079,23 +8079,54 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
80798079
80808080 let sql = `UPDATE ${table} SET ${setClauses.join(',')}`
80818081
8082 // Build WHERE clause
8082 // Build WHERE clause.
8083 //
8084 // Every branch below either appends a predicate or throws. `conditions`
8085 // is a required parameter, so there is no shape of it that means "every
8086 // row" — passing one is the caller saying they have a filter. Anything
8087 // this cannot turn into a predicate used to fall straight through to an
8088 // UPDATE with no WHERE, which rewrote the whole table and reported the
8089 // full count as success. See #1112, and #1101 for the same defect in
8090 // updateTable().
80838091 if (Array.isArray(conditions)) {
8084 sql += ` WHERE ${conditions[0]}${conditions[1]}${getPlaceholder(params.length + 1)}`
8092 // Separated by spaces: the three parts used to be concatenated bare,
8093 // which is fine for `=` and `>=` and produces `namelike?` for every
8094 // word operator the allowed set contains.
8095 const safeOperator = assertSafeWhereOperator(String(conditions[1]), 'updateMany')
8096 sql += ` WHERE ${conditions[0]} ${safeOperator} ${getPlaceholder(params.length + 1)}`
80858097 params.push(conditions[2])
80868098 }
8087 else if (conditions && typeof conditions === 'object' && !('raw' in conditions)) {
8099 // A fragment is inside the declared `WhereExpression` type (through
8100 // `WhereRaw`) and `raw` is a public export, so `updateMany(t, raw('id =
8101 // 1'), data)` typechecks and reads like the intended way to express a
8102 // condition this signature cannot otherwise carry. It matched no branch.
8103 else if (isRawExpression(conditions)) {
8104 sql += ` WHERE ${conditions.raw}`
8105 }
8106 else if (isBoundSqlExpression(conditions)) {
8107 const rendered = renderBoundSqlExpression(conditions, params.length + 1)
8108 sql += ` WHERE ${rendered.text}`
8109 params.push(...rendered.parameters)
8110 }
8111 else if (conditions && typeof conditions === 'object' && Object.keys(conditions).length > 0) {
80888112 const condKeys = Object.keys(conditions)
80898113 const condLen = condKeys.length
8090 if (condLen > 0) {
8091 const baseIdx = params.length
8092 const whereClauses: string[] = Array.from({ length: condLen })
8093 for (let i = 0; i < condLen; i++) {
8094 whereClauses[i] = `${condKeys[i]}=${getPlaceholder(baseIdx + i + 1)}`
8095 params.push((conditions as any)[condKeys[i]])
8096 }
8097 sql += ` WHERE ${whereClauses.join(' AND ')}`
8114 const baseIdx = params.length
8115 const whereClauses: string[] = Array.from({ length: condLen })
8116 for (let i = 0; i < condLen; i++) {
8117 whereClauses[i] = `${condKeys[i]}=${getPlaceholder(baseIdx + i + 1)}`
8118 params.push((conditions as any)[condKeys[i]])
80988119 }
8120 sql += ` WHERE ${whereClauses.join(' AND ')}`
8121 }
8122 // The empty object is the one that bites in production: `conditions` is
8123 // usually built from request input, and `updateMany(t, buildFilter(req.query), data)`
8124 // rewrote every row the moment the filter came back empty.
8125 else if (conditions && typeof conditions === 'object') {
8126 throw new TypeError('[query-builder] updateMany(): an empty object is not a filter. Pass a condition, or use updateTable(table).set(data).execute() if updating every row is intended.')
8127 }
8128 else {
8129 throw new TypeError(`[query-builder] updateMany(): expected a condition, got ${conditions === null ? 'null' : typeof conditions}. Refusing to run an UPDATE with no WHERE — use updateTable(table).set(data).execute() if updating every row is intended.`)
80998130 }
81008131
81018132 return _sql.unsafe(sql, params).execute()
packages/bun-query-builder/test/client.update-many-predicate.test.tsadded+157-0
@@ -0,0 +1,157 @@
1/**
2 * `updateMany` runs with the predicate the caller gave it, or not at all.
3 * stacksjs/bun-query-builder#1112.
4 *
5 * The WHERE clause was appended only for an array, or for an object that both
6 * lacked a `raw` key and had at least one key. Every other shape produced an
7 * UPDATE with no predicate — no error, full affected count reported as
8 * success:
9 *
10 * updateMany('t', raw('id = 1'), { name: 'T' }) -> all four rows rewritten
11 * updateMany('t', {}, { name: 'T' }) -> all four rows rewritten
12 *
13 * The fragment form is the one that reads like correct code: `conditions` is
14 * declared `WhereExpression`, which includes `WhereRaw`, and `raw` is a public
15 * export — so it typechecks and passes review. The empty-object form is the
16 * one that reaches production, because `conditions` is typically built from
17 * request input and an empty filter is what you get from an empty query
18 * string.
19 *
20 * `conditions` is a required parameter, so no value of it can legitimately
21 * mean "every row". Anything unreadable is refused rather than dropped; the
22 * caller who really wants an unfiltered update writes
23 * `updateTable(t).set(data).execute()`, which says so.
24 *
25 * The array form also interpolated its operator into the SQL unvalidated,
26 * while every other builder ran it through `assertSafeWhereOperator` — see the
27 * last block.
28 *
29 * Assertions are on surviving rows. The bug was damage.
30 */
31
32import { mkdtempSync, rmSync } from 'node:fs'
33import { tmpdir } from 'node:os'
34import { join } from 'node:path'
35import { Database } from 'bun:sqlite'
36import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
37import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src'
38import { config, setConfig } from '../src/config'
39import { resetConnection } from '../src/db'
40
41const MODELS = {
42 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
43} as any
44
45let dir: string
46let dbPath: string
47let snapshot: { dialect: string, database: Record<string, unknown> }
48
49function qb(): any {
50 return createQueryBuilder({
51 schema: buildDatabaseSchema(MODELS),
52 meta: buildSchemaMeta(MODELS),
53 autoMigration: { enabled: false } as any,
54 })
55}
56
57function names(): string[] {
58 const probe = new Database(dbPath)
59 const out = (probe.query('SELECT name FROM t ORDER BY id').all() as any[]).map(r => r.name)
60 probe.close()
61 return out
62}
63
64beforeEach(() => {
65 snapshot = { dialect: config.dialect, database: { ...config.database } }
66 dir = mkdtempSync(join(tmpdir(), 'qb-1112-'))
67 dbPath = join(dir, 'many.sqlite')
68 const seed = new Database(dbPath, { create: true })
69 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
70 seed.run(`INSERT INTO t (id, name) VALUES (1,'a'),(2,'b'),(3,'c'),(4,'d')`)
71 seed.close()
72 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
73 resetConnection()
74})
75
76afterEach(() => {
77 config.dialect = snapshot.dialect as any
78 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
79 Object.assign(config.database, snapshot.database)
80 resetConnection()
81 rmSync(dir, { recursive: true, force: true })
82})
83
84describe('a fragment is honoured as a predicate (#1112)', () => {
85 it('raw() updates only the rows it selects', async () => {
86 await qb().updateMany('t', raw('id = 1') as any, { name: 'T' })
87
88 // Previously: ['T','T','T','T'].
89 expect(names()).toEqual(['T', 'b', 'c', 'd'])
90 })
91
92 it('a bound fragment keeps its own parameters ahead of nothing else', async () => {
93 await qb().updateMany('t', { sql: 'id > ?', parameters: [2] } as any, { name: 'T' })
94
95 expect(names()).toEqual(['a', 'b', 'T', 'T'])
96 })
97})
98
99describe('an unreadable condition is refused, not dropped (#1112)', () => {
100 it('{} names the problem and rewrites nothing', async () => {
101 await expect(qb().updateMany('t', {} as any, { name: 'T' }))
102 .rejects.toThrow(/an empty object is not a filter/)
103
104 expect(names()).toEqual(['a', 'b', 'c', 'd'])
105 })
106
107 for (const [label, value] of [['undefined', undefined], ['null', null]] as const) {
108 it(`${label} throws and rewrites nothing`, async () => {
109 await expect(qb().updateMany('t', value as any, { name: 'T' }))
110 .rejects.toThrow(/expected a condition/)
111
112 expect(names()).toEqual(['a', 'b', 'c', 'd'])
113 })
114 }
115
116 it('the message points at the call that does mean every row', async () => {
117 await expect(qb().updateMany('t', {} as any, { name: 'T' }))
118 .rejects.toThrow(/updateTable\(table\)\.set\(data\)\.execute\(\)/)
119 })
120})
121
122describe('the forms that already worked still work (#1112)', () => {
123 it('object conditions', async () => {
124 await qb().updateMany('t', { id: 2 } as any, { name: 'T' })
125 expect(names()).toEqual(['a', 'T', 'c', 'd'])
126 })
127
128 it('array conditions', async () => {
129 await qb().updateMany('t', ['id', '>=', 3] as any, { name: 'T' })
130 expect(names()).toEqual(['a', 'b', 'T', 'T'])
131 })
132
133 it('an empty data object is still a no-op', async () => {
134 expect(await qb().updateMany('t', { id: 2 } as any, {})).toBe(0)
135 expect(names()).toEqual(['a', 'b', 'c', 'd'])
136 })
137})
138
139describe('the array form validates its operator (#1112)', () => {
140 // Every other builder routes the operator through assertSafeWhereOperator;
141 // this one interpolated it. The array form is exactly the shape a filter DSL
142 // builds from request input, so the operator slot is reachable.
143 //
144 // A payload has to keep the placeholder count at one to get past the driver's
145 // arity check — `'>0 OR id>'` does, and rewrote the whole table.
146 it('refuses an operator outside the allowed set, and writes nothing', async () => {
147 await expect(qb().updateMany('t', ['id', '>0 OR id>', 0] as any, { name: 'PWNED' }))
148 .rejects.toThrow(/refusing to use '>0 OR id>' as a SQL operator/)
149
150 expect(names()).toEqual(['a', 'b', 'c', 'd'])
151 })
152
153 it('still accepts the operators it is supposed to', async () => {
154 await qb().updateMany('t', ['name', 'like', 'c'] as any, { name: 'T' })
155 expect(names()).toEqual(['a', 'b', 'T', 'd'])
156 })
157})