also looking at this
fix(client): updateMany runs with the caller's predicate, or not at all
#1119
2 files
+199
-11
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.
| @@ -8079,23 +8079,54 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial | ||
| 8079 | 8079 | |
| 8080 | 8080 | let sql = `UPDATE ${table} SET ${setClauses.join(',')}` |
| 8081 | 8081 | |
| 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(). | |
| 8083 | 8091 | 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)}` | |
| 8085 | 8097 | params.push(conditions[2]) |
| 8086 | 8098 | } |
| 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) { | |
| 8088 | 8112 | const condKeys = Object.keys(conditions) |
| 8089 | 8113 | 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]]) | |
| 8098 | 8119 | } |
| 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.`) | |
| 8099 | 8130 | } |
| 8100 | 8131 | |
| 8101 | 8132 | return _sql.unsafe(sql, params).execute() |
| @@ -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 | ||
| 32 | import { mkdtempSync, rmSync } from 'node:fs' | |
| 33 | import { tmpdir } from 'node:os' | |
| 34 | import { join } from 'node:path' | |
| 35 | import { Database } from 'bun:sqlite' | |
| 36 | import { afterEach, beforeEach, describe, expect, it } from 'bun:test' | |
| 37 | import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder, raw } from '../src' | |
| 38 | import { config, setConfig } from '../src/config' | |
| 39 | import { resetConnection } from '../src/db' | |
| 40 | ||
| 41 | const MODELS = { | |
| 42 | t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } }, | |
| 43 | } as any | |
| 44 | ||
| 45 | let dir: string | |
| 46 | let dbPath: string | |
| 47 | let snapshot: { dialect: string, database: Record<string, unknown> } | |
| 48 | ||
| 49 | function qb(): any { | |
| 50 | return createQueryBuilder({ | |
| 51 | schema: buildDatabaseSchema(MODELS), | |
| 52 | meta: buildSchemaMeta(MODELS), | |
| 53 | autoMigration: { enabled: false } as any, | |
| 54 | }) | |
| 55 | } | |
| 56 | ||
| 57 | function 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 | ||
| 64 | beforeEach(() => { | |
| 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 | ||
| 76 | afterEach(() => { | |
| 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 | ||
| 84 | describe('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 | ||
| 99 | describe('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 | ||
| 122 | describe('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 | ||
| 139 | describe('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 | }) | |