also looking at this
fix(client): updateMany runs with the caller's predicate, or not at all
#1119
2 files
+199
-11
| @@ -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 | }) | |