ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): returningAll() is returning('*'), so it works on MySQL

#1160
Merged glennmichael123 wants to merge fix/returning-all-mysql into main
2 files +189 -66
packages/bun-query-builder/src/client.tsmodified+36-66
@@ -7562,34 +7562,22 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
75627562 return result
75637563 },
75647564 returningAll() {
7565 // As in returning(): no rows inserted, nothing to return.
7566 if (noRows)
7567 return emptyReturningBuilder()
7568 const returningSql = `${sqlText} RETURNING *`
7569 const q = prepareQuery(returningSql, params)
7570 const runFirst = async () => {
7571 const result = await runWithHooks<any[]>(q, 'insert')
7572 return Array.isArray(result) ? result[0] : result
7573 }
7574 return {
7575 toSQL: () => makeExecutableQuery(q, returningSql) as any,
7576 execute: () => runWithHooks<any[]>(q, 'insert'),
7577 get: () => runWithHooks<any[]>(q, 'insert'),
7578 first: runFirst,
7579 executeTakeFirst: runFirst,
7580 async firstOrFail() {
7581 const row = await runFirst()
7582 if (!row)
7583 throw new Error('Insert with RETURNING returned no rows')
7584 return row
7585 },
7586 async executeTakeFirstOrThrow() {
7587 const row = await runFirst()
7588 if (!row)
7589 throw new Error('Insert with RETURNING returned no rows')
7590 return row
7591 },
7592 } as any
7565 /*
7566 * `RETURNING *` is `returning('*')`, so this defers to it.
7567 *
7568 * It used to append the clause itself, a second implementation of
7569 * the same idea, and the two drifted: every fix returning() gained
7570 * stopped here. Most of all the MySQL branch, so `returningAll()`
7571 * emitted `RETURNING *` at a server that has no RETURNING and every
7572 * such write was a syntax error rather than a write - 63 call sites
7573 * in Stacks alone (stacksjs/stacks#2637). It also never reached the
7574 * empty-batch guard, and its handle was missing row accessors the
7575 * return type declares.
7576 *
7577 * mysqlReturningBuilder already reads `*` as "every column", so the
7578 * MySQL read-back needs nothing further.
7579 */
7580 return (this as any).returning('*')
75937581 },
75947582 } as any as TypedInsertQueryBuilder<DB, TTable>
75957583 },
@@ -7909,26 +7897,17 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
79097897 return { numUpdatedRows: result }
79107898 },
79117899 returningAll() {
7912 // Deferred for the same reason as returning(): holding this handle
7913 // while the parent gains a predicate must not execute the statement
7914 // as it stood beforehand. See #1110.
7915 const retAllText = () => `${sqlText} RETURNING *`
7916 const build = () => (params.length > 0 ? prepareQuery(retAllText(), params) : prepareQuery(retAllText()))
7917 return {
7918 toSQL: () => makeExecutableQuery(build(), retAllText()) as any,
7919 execute: () => runWithHooks<any[]>(build(), 'update'),
7920 async executeTakeFirst() {
7921 const result = await runWithHooks<any[]>(build(), 'update')
7922 return Array.isArray(result) ? result[0] : result
7923 },
7924 async executeTakeFirstOrThrow() {
7925 const result = await runWithHooks<any[]>(build(), 'update')
7926 const first = Array.isArray(result) ? result[0] : result
7927 if (!first)
7928 throw new Error('Update with RETURNING failed')
7929 return first
7930 },
7931 } as any
7900 /*
7901 * `returning('*')`, for the reasons given on insertInto's.
7902 *
7903 * The duplicate here missed the MySQL branch, which reads the rows
7904 * by the ids the predicate matched *before* the write, so on MySQL
7905 * this emitted `UPDATE ... RETURNING *` and wrote nothing at all
7906 * (stacksjs/stacks#2637). It also could not take a predicate added
7907 * after it - the #1110 shape - because the handle it returned had no
7908 * where() to delegate.
7909 */
7910 return (this as any).returning('*')
79327911 },
79337912 }
79347913 },
@@ -8183,24 +8162,15 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
81838162 return { numDeletedRows: result }
81848163 },
81858164 returningAll() {
8186 // Deferred, as returning() is — see #1110.
8187 const retAllText = () => `${sqlText} RETURNING *`
8188 const build = () => (delParams.length > 0 ? prepareQuery(retAllText(), delParams) : prepareQuery(retAllText()))
8189 return {
8190 toSQL: () => makeExecutableQuery(build(), retAllText()) as any,
8191 execute: () => runWithHooks<any[]>(build(), 'delete'),
8192 async executeTakeFirst() {
8193 const result = await runWithHooks<any[]>(build(), 'delete')
8194 return Array.isArray(result) ? result[0] : result
8195 },
8196 async executeTakeFirstOrThrow() {
8197 const result = await runWithHooks<any[]>(build(), 'delete')
8198 const first = Array.isArray(result) ? result[0] : result
8199 if (!first)
8200 throw new Error('Delete with RETURNING failed')
8201 return first
8202 },
8203 } as any
8165 /*
8166 * `returning('*')`, for the reasons given on insertInto's.
8167 *
8168 * Besides emitting `RETURNING *` at MySQL (stacksjs/stacks#2637),
8169 * the duplicate here skipped beforeDelete/afterDelete entirely, so
8170 * `.returningAll()` walked straight past an application delete
8171 * guard - the same defect returning() was fixed for.
8172 */
8173 return (this as any).returning('*')
82048174 },
82058175 }
82068176 },
packages/bun-query-builder/test/client.returning-all.test.tsadded+153-0
@@ -0,0 +1,153 @@
1/**
2 * `returningAll()` is `returning('*')`, and must stay that way.
3 * stacksjs/stacks#2637.
4 *
5 * It used to be a second implementation that appended ` RETURNING *` itself,
6 * and every fix `returning()` earned stopped at its door:
7 *
8 * - no MySQL branch, so it emitted `RETURNING *` at a server that has no
9 * RETURNING clause. Every `returningAll()` write on MySQL was a syntax
10 * error rather than a write - and since the statement never ran, nothing
11 * was written either. 63 call sites in Stacks alone, four of them behind
12 * dashboard PATCH routes.
13 * - no delete hooks, so `.returningAll()` on a delete walked straight past
14 * an application `beforeDelete` guard, which is the usual reason to write
15 * one.
16 * - no filter delegation, so a predicate applied after the handle was taken
17 * could not reach the statement (the #1110 shape).
18 * - no row accessors, though the declared return type is SelectQueryBuilder,
19 * which has them - `.returningAll().first()` threw "not a function".
20 *
21 * These run on SQLite, which takes RETURNING natively, so what they pin is
22 * the delegation rather than the MySQL text. The MySQL behaviour was measured
23 * directly against MySQL 8.4.5: before, update/insert/delete each raised
24 * errno 1064 and left the table untouched; after, each returns its row and
25 * the write lands.
26 */
27
28import { mkdtempSync, rmSync } from 'node:fs'
29import { tmpdir } from 'node:os'
30import { join } from 'node:path'
31import { Database } from 'bun:sqlite'
32import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
33import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
34import { config, setConfig } from '../src/config'
35import { resetConnection } from '../src/db'
36
37const MODELS = {
38 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
39} as any
40
41let dir: string
42let dbPath: string
43let snapshot: { dialect: string, database: Record<string, unknown>, hooks: unknown }
44
45function qb(): any {
46 return createQueryBuilder({
47 schema: buildDatabaseSchema(MODELS),
48 meta: buildSchemaMeta(MODELS),
49 autoMigration: { enabled: false } as any,
50 })
51}
52
53function rows(): Array<{ id: number, name: string }> {
54 const probe = new Database(dbPath)
55 const out = probe.query('SELECT id, name FROM t ORDER BY id').all() as any[]
56 probe.close()
57 return out
58}
59
60beforeEach(() => {
61 snapshot = { dialect: config.dialect, database: { ...config.database }, hooks: (config as any).hooks }
62 dir = mkdtempSync(join(tmpdir(), 'qb-2637-'))
63 dbPath = join(dir, 'ret-all.sqlite')
64 const seed = new Database(dbPath, { create: true })
65 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY, name TEXT)')
66 seed.run(`INSERT INTO t (id, name) VALUES (1,'a'),(2,'b'),(3,'c'),(4,'d')`)
67 seed.close()
68 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
69 resetConnection()
70})
71
72afterEach(() => {
73 config.dialect = snapshot.dialect as any
74 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
75 Object.assign(config.database, snapshot.database)
76 ;(config as any).hooks = snapshot.hooks
77 resetConnection()
78 rmSync(dir, { recursive: true, force: true })
79})
80
81describe('returningAll() is returning(\'*\')', () => {
82 // The structural guard. Behaviour tests below say what the handle must do;
83 // this one says there is only one implementation of it, so a reintroduced
84 // bespoke returningAll() fails here rather than silently shedding whatever
85 // returning() gains next.
86 for (const [name, take] of [
87 ['insertInto', (b: any) => b.insertInto('t').values({ name: 'z' })],
88 ['updateTable', (b: any) => b.updateTable('t').set({ name: 'z' })],
89 ['deleteFrom', (b: any) => b.deleteFrom('t')],
90 ] as Array<[string, (b: any) => any]>) {
91 it(`${name} hands back the same surface either way`, () => {
92 const all = Object.keys(take(qb()).returningAll()).sort()
93 const star = Object.keys(take(qb()).returning('*')).sort()
94
95 expect(all).toEqual(star)
96 // Row accessors the declared SelectQueryBuilder type promises. The old
97 // update/delete handles carried four methods and none of these.
98 expect(all).toContain('first')
99 expect(all).toContain('get')
100 expect(all).toContain('firstOrFail')
101 })
102 }
103
104 it('returns every column of the updated row, and writes it', async () => {
105 const row = await qb().updateTable('t').set({ name: 'X' }).where({ id: 2 }).returningAll().executeTakeFirst()
106
107 expect(row).toEqual({ id: 2, name: 'X' })
108 expect(rows().filter(r => r.name === 'X').map(r => r.id)).toEqual([2])
109 })
110
111 it('returns every column of the inserted row', async () => {
112 const row = await qb().insertInto('t').values({ id: 9, name: 'i' }).returningAll().executeTakeFirst()
113
114 expect(row).toEqual({ id: 9, name: 'i' })
115 })
116
117 it('returns every column of the deleted row, and removes it', async () => {
118 const row = await qb().deleteFrom('t').where({ id: 3 }).returningAll().executeTakeFirst()
119
120 expect(row).toEqual({ id: 3, name: 'c' })
121 expect(rows().map(r => r.id)).toEqual([1, 2, 4])
122 })
123
124 it('fires the delete hooks, so a beforeDelete guard still sees it', async () => {
125 const seen: string[] = []
126 ;(config as any).hooks = {
127 beforeDelete: ({ table }: any) => { seen.push(`before:${table}`) },
128 afterDelete: ({ table }: any) => { seen.push(`after:${table}`) },
129 }
130
131 await qb().deleteFrom('t').where({ id: 1 }).returningAll().execute()
132
133 // Both were absent here while `.returning('id')` fired them, which is the
134 // shape that let adding returningAll() bypass a delete guard.
135 expect(seen).toEqual(['before:t', 'after:t'])
136 })
137
138 it('takes a predicate applied after the handle (#1110)', async () => {
139 const builder = qb().deleteFrom('t')
140 const handle = builder.returningAll()
141 builder.where({ id: 4 })
142
143 expect((await handle.execute()).map((r: any) => r.id)).toEqual([4])
144 expect(rows().map(r => r.id)).toEqual([1, 2, 3])
145 })
146
147 for (const method of ['orderBy', 'limit', 'offset', 'orWhere'] as const) {
148 it(`refuses ${method}() rather than ignoring it`, () => {
149 expect(() => (qb().deleteFrom('t').returningAll() as any)[method]('id')).toThrow(/not supported/)
150 expect(rows()).toHaveLength(4)
151 })
152 }
153})