ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): an array in object-form where() means IN on writes too

#1127
Merged glennmichael123 wants to merge fix/1114-array-in-write-where into main
2 files +189 -21
packages/bun-query-builder/src/client.tsmodified+33-21
@@ -29,6 +29,35 @@ interface BoundSqlExpression {
2929 parameters?: readonly unknown[]
3030}
3131
32/**
33 * Render one `{ column: value }` entry of an object-form where() as a
34 * predicate, appending its bound parameters to `params`.
35 *
36 * An array value means IN. The select builder has read it that way since
37 * #1013/#1083; the write builders bound the whole array to a single
38 * placeholder, so `where({ id: [3, 2] })` selected two rows on a SELECT and
39 * matched nothing on an UPDATE or a DELETE — silently, reporting success. The
40 * bulk delete of a set of ids is exactly the call people write it for.
41 *
42 * An empty array renders as `1 = 0` rather than disappearing, which is what
43 * the select builder does and the only safe reading on a write: a filter the
44 * caller supplied must never widen to "every row".
45 *
46 * Shared by all three write paths so they cannot drift apart again — the
47 * divergence, not any one branch, is what kept producing these. See #1114.
48 */
49function renderColumnCondition(column: string, value: unknown, params: unknown[]): string {
50 if (Array.isArray(value)) {
51 const placeholders = getPlaceholders(value.length, params.length + 1)
52 const clause = renderInPredicate(column, value, false, placeholders)
53 params.push(...value)
54 return clause
55 }
56 const clause = `${column} = ${getPlaceholder(params.length + 1)}`
57 params.push(value)
58 return clause
59}
60
3261function isBoundSqlExpression(expr: unknown): expr is BoundSqlExpression {
3362 return typeof expr === 'object'
3463 && expr !== null
@@ -7384,13 +7413,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
73847413 const keys = Object.keys(expr)
73857414 if (keys.length === 0)
73867415 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.')
7387 const len = keys.length
7388 const baseIdx = params.length
7389 const conditions: string[] = Array.from({ length: len })
7390 for (let i = 0; i < len; i++) {
7391 conditions[i] = `${quoteId(keys[i])} = ${getPlaceholder(baseIdx + i + 1)}`
7392 params.push((expr as any)[keys[i]])
7393 }
7416 const conditions = keys.map(key => renderColumnCondition(quoteId(key), (expr as any)[key], params))
73947417 appendPredicate(conditions.join(' AND '))
73957418 built = _sql.unsafe(sqlText, params)
73967419 return this
@@ -7688,12 +7711,7 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
76887711 const keys = Object.keys(expr)
76897712 if (keys.length === 0)
76907713 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.')
7691 const conditions: string[] = []
7692 for (const key of keys) {
7693 const paramIndex = delParams.length + 1
7694 conditions.push(`${quoteId(key)} = ${getPlaceholder(paramIndex)}`)
7695 delParams.push((expr as any)[key])
7696 }
7714 const conditions = keys.map(key => renderColumnCondition(quoteId(key), (expr as any)[key], delParams))
76977715 appendPredicate(conditions.join(' AND '))
76987716 built = null
76997717 return this
@@ -8487,14 +8505,8 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
84878505 params.push(...rendered.parameters)
84888506 }
84898507 else if (conditions && typeof conditions === 'object' && Object.keys(conditions).length > 0) {
8490 const condKeys = Object.keys(conditions)
8491 const condLen = condKeys.length
8492 const baseIdx = params.length
8493 const whereClauses: string[] = Array.from({ length: condLen })
8494 for (let i = 0; i < condLen; i++) {
8495 whereClauses[i] = `${condKeys[i]}=${getPlaceholder(baseIdx + i + 1)}`
8496 params.push((conditions as any)[condKeys[i]])
8497 }
8508 const whereClauses = Object.keys(conditions).map(key =>
8509 renderColumnCondition(key, (conditions as any)[key], params))
84988510 sql += ` WHERE ${whereClauses.join(' AND ')}`
84998511 }
85008512 // The empty object is the one that bites in production: `conditions` is
packages/bun-query-builder/test/client.write-array-in.test.tsadded+156-0
@@ -0,0 +1,156 @@
1/**
2 * An array value in object-form where() means IN, on writes as well as reads.
3 * stacksjs/bun-query-builder#1114.
4 *
5 * The select builder has read `where({ id: [3, 2] })` as `id IN (3, 2)` since
6 * #1013/#1083. The write builders bound the whole array to a single
7 * placeholder, so the same input meant two different things depending on which
8 * statement it was attached to:
9 *
10 * selectFrom(t).where({ id: [3, 2] }) -> rows 2 and 3
11 * deleteFrom(t).where({ id: [3, 2] }) -> deletes nothing
12 *
13 * This is the opposite direction to #1101 — the write is too NARROW, so it
14 * destroys nothing. But it fails silently and reports success, and it fails in
15 * exactly the case people write it for: the bulk delete of a set of ids.
16 *
17 * All three write paths now share one renderer with the same semantics. The
18 * mixed case matters most for the shared code: the old per-key index
19 * arithmetic (`baseIdx + i + 1`) assumed one parameter per key, which an
20 * array breaks.
21 */
22
23import { mkdtempSync, rmSync } from 'node:fs'
24import { tmpdir } from 'node:os'
25import { join } from 'node:path'
26import { Database } from 'bun:sqlite'
27import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
28import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
29import { config, setConfig } from '../src/config'
30import { resetConnection } from '../src/db'
31
32const MODELS = {
33 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' }, tenant: { type: 'integer' } } },
34} as any
35
36let dir: string
37let dbPath: string
38let snapshot: { dialect: string, database: Record<string, unknown> }
39
40function qb(): any {
41 return createQueryBuilder({
42 schema: buildDatabaseSchema(MODELS),
43 meta: buildSchemaMeta(MODELS),
44 autoMigration: { enabled: false } as any,
45 })
46}
47
48function ids(): number[] {
49 const probe = new Database(dbPath)
50 const out = (probe.query('SELECT id FROM t ORDER BY id').all() as any[]).map(r => r.id)
51 probe.close()
52 return out
53}
54
55function renamed(): number[] {
56 const probe = new Database(dbPath)
57 const out = (probe.query(`SELECT id FROM t WHERE name = 'X' ORDER BY id`).all() as any[]).map(r => r.id)
58 probe.close()
59 return out
60}
61
62beforeEach(() => {
63 snapshot = { dialect: config.dialect, database: { ...config.database } }
64 dir = mkdtempSync(join(tmpdir(), 'qb-1114-'))
65 dbPath = join(dir, 'arr.sqlite')
66 const seed = new Database(dbPath, { create: true })
67 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT, tenant INTEGER)')
68 seed.run(`INSERT INTO t (id,name,tenant) VALUES (1,'a',1),(2,'b',1),(3,'c',1),(4,'d',999)`)
69 seed.close()
70 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
71 resetConnection()
72})
73
74afterEach(() => {
75 config.dialect = snapshot.dialect as any
76 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
77 Object.assign(config.database, snapshot.database)
78 resetConnection()
79 rmSync(dir, { recursive: true, force: true })
80})
81
82describe('reads and writes agree on what an array means (#1114)', () => {
83 it('SELECT reads it as IN — the behaviour the writes now match', async () => {
84 const rows = await qb().selectFrom('t').select('id').where({ id: [3, 2] }).execute()
85 expect(rows.map((r: any) => r.id).sort()).toEqual([2, 3])
86 })
87
88 it('DELETE removes exactly those rows', async () => {
89 await qb().deleteFrom('t').where({ id: [3, 2] }).execute()
90
91 // Previously: deleted nothing, and reported success.
92 expect(ids()).toEqual([1, 4])
93 })
94
95 it('UPDATE rewrites exactly those rows', async () => {
96 await qb().updateTable('t').set({ name: 'X' }).where({ id: [3, 2] }).execute()
97 expect(renamed()).toEqual([2, 3])
98 })
99
100 it('updateMany does too', async () => {
101 await qb().updateMany('t', { id: [3, 2] } as any, { name: 'X' })
102 expect(renamed()).toEqual([2, 3])
103 })
104})
105
106describe('an array alongside a scalar binds in the right order (#1114)', () => {
107 // The old code computed each parameter index as `baseIdx + i + 1`, one per
108 // key. An array contributes several, so every parameter after it was bound
109 // to the wrong placeholder.
110 it('DELETE with both a list and a scalar', async () => {
111 const sql = String(qb().deleteFrom('t').where({ id: [2, 3], tenant: 1 } as any).toSQL())
112 expect(sql).toBe('DELETE FROM "t" WHERE "id" IN (?, ?) AND "tenant" = ?')
113
114 await qb().deleteFrom('t').where({ id: [2, 3], tenant: 1 } as any).execute()
115 expect(ids()).toEqual([1, 4])
116 })
117
118 it('the scalar still constrains — a row outside the tenant survives', async () => {
119 await qb().deleteFrom('t').where({ id: [3, 4], tenant: 1 } as any).execute()
120
121 // id 4 is tenant 999, so only 3 goes.
122 expect(ids()).toEqual([1, 2, 4])
123 })
124})
125
126describe('an empty array matches nothing, never everything (#1114)', () => {
127 // The only safe reading on a write: a filter the caller supplied must not
128 // widen to "every row" because the collection came back empty. This is what
129 // the select builder already does (FALSE_PREDICATE).
130 it('DELETE with an empty list deletes nothing', async () => {
131 await qb().deleteFrom('t').where({ id: [] } as any).execute()
132 expect(ids()).toEqual([1, 2, 3, 4])
133 })
134
135 it('and does not drop the other conditions either', async () => {
136 await qb().deleteFrom('t').where({ id: [], tenant: 1 } as any).execute()
137 expect(ids()).toEqual([1, 2, 3, 4])
138 })
139
140 it('UPDATE with an empty list rewrites nothing', async () => {
141 await qb().updateTable('t').set({ name: 'X' }).where({ id: [] } as any).execute()
142 expect(renamed()).toEqual([])
143 })
144})
145
146describe('the scalar form is unchanged (#1114)', () => {
147 it('DELETE', async () => {
148 await qb().deleteFrom('t').where({ id: 2 }).execute()
149 expect(ids()).toEqual([1, 3, 4])
150 })
151
152 it('UPDATE with two scalars', async () => {
153 await qb().updateTable('t').set({ name: 'X' }).where({ id: 2, tenant: 1 } as any).execute()
154 expect(renamed()).toEqual([2])
155 })
156})