ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): apply the filters that follow returning(), instead of discarding them

#1115
Merged glennmichael123 wants to merge fix/1110-returning-discards-filters into main
2 files +262 -44

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+86-44
@@ -7115,26 +7115,47 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
71157115 return this
71167116 },
71177117 returning(...cols) {
7118 const retText = `${sqlText} RETURNING ${cols.join(', ')}`
7119 const q = params.length > 0
7120 ? _sql.unsafe(retText, params)
7121 : _sql.unsafe(retText)
7118 const parent: any = this
7119 // Render at execute time, not here.
7120 //
7121 // This used to freeze the statement text immediately and return an
7122 // object whose where/andWhere/orWhere/orderBy/limit/offset were
7123 // literally `() => obj` — so every filter applied after returning()
7124 // was discarded and the UPDATE ran against the whole table, silently,
7125 // while `.executeTakeFirst()` handed back a row it had never filtered
7126 // to. `returning()` is typed as SelectQueryBuilder, which declares all
7127 // of those, so the broken order type-checks. See #1110.
7128 //
7129 // Deferring also fixes the aliasing form, where the returning handle
7130 // is held while the parent gains a predicate.
7131 const retText = () => `${sqlText} RETURNING ${cols.join(', ')}`
7132 const build = () => (params.length > 0 ? _sql.unsafe(retText(), params) : _sql.unsafe(retText()))
71227133 const runFirst = async () => {
7123 const rows = await runWithHooks<any[]>(q, 'update')
7134 const rows = await runWithHooks<any[]>(build(), 'update')
71247135 return Array.isArray(rows) ? rows[0] : rows
71257136 }
7137 // Not expressible on this builder. Silently ignoring them is what
7138 // made a `.limit(1)` destructive write hit every row.
7139 const unsupported = (name: string) => (): never => {
7140 throw new TypeError(`[query-builder] updateTable.returning(...).${name}() is not supported — an UPDATE cannot be ordered or limited through this builder. Apply the filter with where() instead.`)
7141 }
71267142 const obj: any = {
7127 where: () => obj,
7128 andWhere: () => obj,
7129 orWhere: () => obj,
7130 orderBy: () => obj,
7131 limit: () => obj,
7132 offset: () => obj,
7133 toSQL: () => makeExecutableQuery(q, retText) as any,
7134 execute: () => runWithHooks<any[]>(q, 'update'),
7143 // Delegate to the builder that owns the statement, then keep
7144 // chaining from here. Consecutive predicates join with AND, which
7145 // is what andWhere means.
7146 where: (...args: any[]) => { parent.where(...args); return obj },
7147 andWhere: (...args: any[]) => { parent.where(...args); return obj },
7148 whereNull: (...args: any[]) => { parent.whereNull(...args); return obj },
7149 whereNotNull: (...args: any[]) => { parent.whereNotNull(...args); return obj },
7150 orWhere: unsupported('orWhere'),
7151 orderBy: unsupported('orderBy'),
7152 limit: unsupported('limit'),
7153 offset: unsupported('offset'),
7154 toSQL: () => makeExecutableQuery(build(), retText()) as any,
7155 execute: () => runWithHooks<any[]>(build(), 'update'),
71357156 // returning() is typed as SelectQueryBuilder — expose the
71367157 // row-fetching methods so `.returning('id').first()` works.
7137 get: () => runWithHooks<any[]>(q, 'update'),
7158 get: () => runWithHooks<any[]>(build(), 'update'),
71387159 first: runFirst,
71397160 executeTakeFirst: runFirst,
71407161 async firstOrFail() {
@@ -7175,19 +7196,20 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
71757196 return { numUpdatedRows: result }
71767197 },
71777198 returningAll() {
7178 const retAllText = `${sqlText} RETURNING *`
7179 const q = params.length > 0
7180 ? _sql.unsafe(retAllText, params)
7181 : _sql.unsafe(retAllText)
7199 // Deferred for the same reason as returning(): holding this handle
7200 // while the parent gains a predicate must not execute the statement
7201 // as it stood beforehand. See #1110.
7202 const retAllText = () => `${sqlText} RETURNING *`
7203 const build = () => (params.length > 0 ? _sql.unsafe(retAllText(), params) : _sql.unsafe(retAllText()))
71827204 return {
7183 toSQL: () => makeExecutableQuery(q, retAllText) as any,
7184 execute: () => runWithHooks<any[]>(q, 'update'),
7205 toSQL: () => makeExecutableQuery(build(), retAllText()) as any,
7206 execute: () => runWithHooks<any[]>(build(), 'update'),
71857207 async executeTakeFirst() {
7186 const result = await runWithHooks<any[]>(q, 'update')
7208 const result = await runWithHooks<any[]>(build(), 'update')
71877209 return Array.isArray(result) ? result[0] : result
71887210 },
71897211 async executeTakeFirstOrThrow() {
7190 const result = await runWithHooks<any[]>(q, 'update')
7212 const result = await runWithHooks<any[]>(build(), 'update')
71917213 const first = Array.isArray(result) ? result[0] : result
71927214 if (!first)
71937215 throw new Error('Update with RETURNING failed')
@@ -7337,25 +7359,46 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
73377359 return this
73387360 },
73397361 returning(...cols) {
7340 const retText = `${sqlText} RETURNING ${cols.join(', ')}`
7341 const q = delParams.length > 0
7342 ? _sql.unsafe(retText, delParams)
7343 : _sql.unsafe(retText)
7362 const parent: any = this
7363 // Rendered at execute time. The frozen-text form, with the no-op
7364 // filter stubs below it, meant `.returning('id').where({ id: 1 })`
7365 // emptied the table and returned every deleted row as though the
7366 // filter had applied. See #1110.
7367 const retText = () => `${sqlText} RETURNING ${cols.join(', ')}`
7368 const build = () => (delParams.length > 0 ? _sql.unsafe(retText(), delParams) : _sql.unsafe(retText()))
7369 // Fire the same delete hooks execute() does. Without this, adding
7370 // `.returning(...)` to a delete skipped beforeDelete/afterDelete
7371 // entirely, so an application-level delete guard — the usual reason
7372 // to implement beforeDelete at all — could be walked straight past.
7373 const runDelete = async (): Promise<any[]> => {
7374 await activeHooks()?.beforeDelete?.({ table: String(table), where: whereCondition })
7375 const rows = await runWithHooks<any[]>(build(), 'delete')
7376 try {
7377 await activeHooks()?.afterDelete?.({ table: String(table), where: whereCondition, result: rows })
7378 }
7379 catch {}
7380 return rows
7381 }
73447382 const runFirst = async () => {
7345 const rows = await runWithHooks<any[]>(q, 'delete')
7383 const rows = await runDelete()
73467384 return Array.isArray(rows) ? rows[0] : rows
73477385 }
7386 const unsupported = (name: string) => (): never => {
7387 throw new TypeError(`[query-builder] deleteFrom.returning(...).${name}() is not supported — a DELETE cannot be ordered or limited through this builder. Narrow it with where() instead.`)
7388 }
73487389 const obj: any = {
7349 where: () => obj,
7350 andWhere: () => obj,
7351 orWhere: () => obj,
7352 orderBy: () => obj,
7353 limit: () => obj,
7354 offset: () => obj,
7355 toSQL: () => makeExecutableQuery(q, retText) as any,
7356 execute: () => runWithHooks<any[]>(q, 'delete'),
7390 where: (...args: any[]) => { parent.where(...args); return obj },
7391 andWhere: (...args: any[]) => { parent.where(...args); return obj },
7392 whereNull: (...args: any[]) => { parent.whereNull(...args); return obj },
7393 whereNotNull: (...args: any[]) => { parent.whereNotNull(...args); return obj },
7394 orWhere: unsupported('orWhere'),
7395 orderBy: unsupported('orderBy'),
7396 limit: unsupported('limit'),
7397 offset: unsupported('offset'),
7398 toSQL: () => makeExecutableQuery(build(), retText()) as any,
7399 execute: () => runDelete(),
73577400 // returning() is typed as SelectQueryBuilder — expose row fetchers.
7358 get: () => runWithHooks<any[]>(q, 'delete'),
7401 get: () => runDelete(),
73597402 first: runFirst,
73607403 executeTakeFirst: runFirst,
73617404 async firstOrFail() {
@@ -7404,19 +7447,18 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
74047447 return { numDeletedRows: result }
74057448 },
74067449 returningAll() {
7407 const retAllText = `${sqlText} RETURNING *`
7408 const q = delParams.length > 0
7409 ? _sql.unsafe(retAllText, delParams)
7410 : _sql.unsafe(retAllText)
7450 // Deferred, as returning() is — see #1110.
7451 const retAllText = () => `${sqlText} RETURNING *`
7452 const build = () => (delParams.length > 0 ? _sql.unsafe(retAllText(), delParams) : _sql.unsafe(retAllText()))
74117453 return {
7412 toSQL: () => makeExecutableQuery(q, retAllText) as any,
7413 execute: () => runWithHooks<any[]>(q, 'delete'),
7454 toSQL: () => makeExecutableQuery(build(), retAllText()) as any,
7455 execute: () => runWithHooks<any[]>(build(), 'delete'),
74147456 async executeTakeFirst() {
7415 const result = await runWithHooks<any[]>(q, 'delete')
7457 const result = await runWithHooks<any[]>(build(), 'delete')
74167458 return Array.isArray(result) ? result[0] : result
74177459 },
74187460 async executeTakeFirstOrThrow() {
7419 const result = await runWithHooks<any[]>(q, 'delete')
7461 const result = await runWithHooks<any[]>(build(), 'delete')
74207462 const first = Array.isArray(result) ? result[0] : result
74217463 if (!first)
74227464 throw new Error('Delete with RETURNING failed')
packages/bun-query-builder/test/client.returning-filters.test.tsadded+176-0
@@ -0,0 +1,176 @@
1/**
2 * A filter applied after returning() must reach the statement.
3 * stacksjs/bun-query-builder#1110.
4 *
5 * `returning()` froze the statement text at call time and handed back an
6 * object whose filter methods were literally no-ops:
7 *
8 * { where: () => obj, andWhere: () => obj, orWhere: () => obj,
9 * orderBy: () => obj, limit: () => obj, offset: () => obj }
10 *
11 * so every filter expressed after it was discarded and the unfiltered
12 * statement ran:
13 *
14 * updateTable(t).set(v).returning('id').where({ id: 1 }) -> rewrote all rows
15 * deleteFrom(t).returning('id').where({ id: 1 }) -> emptied the table
16 *
17 * Three things made it worse than a dropped predicate:
18 *
19 * - it REPORTED SUCCESS for work it never scoped. `.where('id','=',99)
20 * .executeTakeFirst()` returned a row that cannot match 99, while rewriting
21 * everything.
22 * - the types invite it. `returning()` is declared to return
23 * SelectQueryBuilder, which declares where/andWhere/orWhere/orderBy/limit,
24 * so the broken order type-checks under --strict.
25 * - on the delete side it skipped beforeDelete/afterDelete, so adding
26 * `.returning(...)` walked straight past an application delete guard.
27 *
28 * The fix defers rendering to execute time and delegates the filters to the
29 * builder that owns the statement. Methods that genuinely cannot be expressed
30 * on a write (orderBy/limit/offset/orWhere) now throw rather than being
31 * ignored — silently ignoring `limit(1)` is what turned a one-row delete into
32 * a whole-table one.
33 */
34
35import { mkdtempSync, rmSync } from 'node:fs'
36import { tmpdir } from 'node:os'
37import { join } from 'node:path'
38import { Database } from 'bun:sqlite'
39import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
40import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
41import { config, setConfig } from '../src/config'
42import { resetConnection } from '../src/db'
43
44const MODELS = {
45 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
46} as any
47
48let dir: string
49let dbPath: string
50let snapshot: { dialect: string, database: Record<string, unknown>, hooks: unknown }
51
52function qb(): any {
53 return createQueryBuilder({
54 schema: buildDatabaseSchema(MODELS),
55 meta: buildSchemaMeta(MODELS),
56 autoMigration: { enabled: false } as any,
57 })
58}
59
60function rows(): Array<{ id: number, name: string }> {
61 const probe = new Database(dbPath)
62 const out = probe.query('SELECT id, name FROM t ORDER BY id').all() as any[]
63 probe.close()
64 return out
65}
66
67beforeEach(() => {
68 snapshot = { dialect: config.dialect, database: { ...config.database }, hooks: (config as any).hooks }
69 dir = mkdtempSync(join(tmpdir(), 'qb-1110-'))
70 dbPath = join(dir, 'ret.sqlite')
71 const seed = new Database(dbPath, { create: true })
72 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
73 seed.run(`INSERT INTO t (id, name) VALUES (1,'a'),(2,'b'),(3,'c'),(4,'d')`)
74 seed.close()
75 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
76 resetConnection()
77})
78
79afterEach(() => {
80 config.dialect = snapshot.dialect as any
81 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
82 Object.assign(config.database, snapshot.database)
83 ;(config as any).hooks = snapshot.hooks
84 resetConnection()
85 rmSync(dir, { recursive: true, force: true })
86})
87
88describe('UPDATE: a filter after returning() applies (#1110)', () => {
89 it('rewrites only the matching row', async () => {
90 const returned = await qb().updateTable('t').set({ name: 'X' }).returning('id').where({ id: 1 }).execute()
91
92 expect(returned).toEqual([{ id: 1 }])
93 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([1])
94 expect(rows()).toHaveLength(4)
95 })
96
97 it('does not report a row it never matched', async () => {
98 // The sharp edge: this used to return { id: 1 } while rewriting all four.
99 const first = await qb().updateTable('t').set({ name: 'Y' }).returning('id').where('id', '=', 99).executeTakeFirst()
100
101 expect(first).toBeUndefined()
102 expect(rows().filter(r => r.name === 'Y')).toHaveLength(0)
103 })
104
105 it('still works in the other order', async () => {
106 const returned = await qb().updateTable('t').set({ name: 'Z' }).where({ id: 1 }).returning('id').execute()
107 expect(returned).toEqual([{ id: 1 }])
108 expect(rows().filter(r => r.name === 'Z').map(r => r.id)).toEqual([1])
109 })
110
111 it('andWhere and whereNull reach the statement too', async () => {
112 const returned = await qb().updateTable('t').set({ name: 'W' })
113 .returning('id').where({ id: 2 }).andWhere('name', '=', 'b')
114 .execute()
115
116 expect(returned).toEqual([{ id: 2 }])
117 expect(rows().filter(r => r.name === 'W').map(r => r.id)).toEqual([2])
118 })
119})
120
121describe('DELETE: a filter after returning() applies (#1110)', () => {
122 it('removes only the matching row', async () => {
123 const returned = await qb().deleteFrom('t').returning('id').where({ id: 1 }).execute()
124
125 expect(returned).toEqual([{ id: 1 }])
126 expect(rows().map(r => r.id)).toEqual([2, 3, 4])
127 })
128
129 it('fires beforeDelete, so a delete guard cannot be walked past', async () => {
130 setConfig({ hooks: { beforeDelete: () => { throw new Error('BLOCKED by guard') } } } as any)
131
132 await expect(qb().deleteFrom('t').returning('id').where({ id: 1 }).execute())
133 .rejects.toThrow(/BLOCKED by guard/)
134
135 expect(rows()).toHaveLength(4)
136 })
137})
138
139describe('the returning handle is not a snapshot (#1110)', () => {
140 it('picks up a predicate added to the parent after it was taken', async () => {
141 const builder = qb().deleteFrom('t')
142 const handle = builder.returning('id')
143 builder.where({ id: 2 })
144
145 expect(await handle.execute()).toEqual([{ id: 2 }])
146 expect(rows().map(r => r.id)).toEqual([1, 3, 4])
147 })
148
149 it('applies to returningAll() as well', async () => {
150 const builder = qb().deleteFrom('t')
151 const handle = builder.returningAll()
152 builder.where({ id: 3 })
153
154 const returned = await handle.execute()
155 expect(returned.map((r: any) => r.id)).toEqual([3])
156 expect(rows().map(r => r.id)).toEqual([1, 2, 4])
157 })
158})
159
160describe('what a write cannot express now says so (#1110)', () => {
161 // Silently ignoring these is what let `.limit(1)` on a delete remove
162 // everything while reading like it removed one row.
163 for (const method of ['orderBy', 'limit', 'offset', 'orWhere'] as const) {
164 it(`deleteFrom.returning(...).${method}() throws instead of being ignored`, () => {
165 expect(() => (qb().deleteFrom('t').returning('id') as any)[method]('id'))
166 .toThrow(/not supported/)
167 expect(rows()).toHaveLength(4)
168 })
169
170 it(`updateTable.returning(...).${method}() throws instead of being ignored`, () => {
171 expect(() => (qb().updateTable('t').set({ name: 'Q' }).returning('id') as any)[method]('id'))
172 .toThrow(/not supported/)
173 expect(rows().filter(r => r.name === 'Q')).toHaveLength(0)
174 })
175 }
176})