ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(client): an empty insert is a no-op, not a stand-in query

#1107
Merged glennmichael123 wants to merge fix/1097-empty-values into main
2 files +244 -1
packages/bun-query-builder/src/client.tsmodified+76-1
@@ -6657,6 +6657,50 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
66576657 let built: any
66586658 let sqlText = ''
66596659 const params: any[] = []
6660 // Set when `values()` is handed an empty batch. There is no statement to
6661 // run in that case, so every terminal below answers from here instead of
6662 // executing a stand-in query. See #1097.
6663 let noRows = false
6664
6665 /**
6666 * What the dialect returns for an insert that affected no rows.
6667 *
6668 * Postgres gives `[]` for an insert without RETURNING, so an empty batch
6669 * is indistinguishable from a real one that inserted nothing — which is
6670 * the point. The bun:sqlite wrapper reports `{ changes, lastInsertRowid }`
6671 * instead, so it gets zeros.
6672 */
6673 const emptyInsertResult = (): any => (isPostgres ? [] : { changes: 0, lastInsertRowid: 0 })
6674
6675 /**
6676 * The `.returning(...)` / `.returningAll()` surface for an empty batch.
6677 *
6678 * Mirrors the real one's method set so `.returning('id').first()` still
6679 * resolves rather than throwing "first is not a function". No rows were
6680 * inserted, so the row accessors answer empty and the *OrFail pair throws
6681 * the same message they throw when a real RETURNING comes back empty.
6682 */
6683 const emptyReturningBuilder = (): any => {
6684 const noRow = async (): Promise<any> => undefined
6685 const noRowOrFail = async (): Promise<any> => {
6686 throw new Error('Insert with RETURNING returned no rows')
6687 }
6688 return {
6689 where: () => emptyReturningBuilder(),
6690 andWhere: () => emptyReturningBuilder(),
6691 orWhere: () => emptyReturningBuilder(),
6692 orderBy: () => emptyReturningBuilder(),
6693 limit: () => emptyReturningBuilder(),
6694 offset: () => emptyReturningBuilder(),
6695 toSQL: () => makeExecutableQuery(null as any, '') as any,
6696 execute: async () => [],
6697 get: async () => [],
6698 first: noRow,
6699 executeTakeFirst: noRow,
6700 firstOrFail: noRowOrFail,
6701 executeTakeFirstOrThrow: noRowOrFail,
6702 }
6703 }
66606704 const isPostgres = config.dialect === 'postgres'
66616705
66626706 // Quote identifier based on dialect. SQLite supports double-quoted
@@ -6683,9 +6727,24 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
66836727 const rows = Array.isArray(data) ? data : [data]
66846728 const rowCount = rows.length
66856729 if (rowCount === 0) {
6686 built = _sql.unsafe('SELECT 1')
6730 // Inserting no rows is a no-op, not a query. This used to run
6731 // `SELECT 1` as a stand-in, which meant `.execute()` resolved to a
6732 // fabricated row — `[{ '?column?': 1 }]` on Postgres, `[{ '1': 1 }]`
6733 // on SQLite — so a caller checking `result.length` saw 1 where the
6734 // truth was 0, a query hook fired for an insert that never
6735 // happened, and `.returning(...)` appended RETURNING to `SELECT 1`
6736 // and died on a syntax error. See #1097.
6737 //
6738 // `values(rows)` where `rows` came back empty is ordinary calling
6739 // code — the seeder scaffold this package generates is written that
6740 // way — so this answers empty rather than throwing.
6741 noRows = true
6742 built = null
6743 sqlText = ''
6744 params.length = 0
66876745 return this
66886746 }
6747 noRows = false
66896748
66906749 const firstRow = rows[0]
66916750 const keys = Object.keys(firstRow)
@@ -6760,6 +6819,11 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
67606819 return this
67616820 },
67626821 returning(...cols: (keyof any & string)[]) {
6822 // Nothing was inserted, so there is nothing to return. Without this
6823 // the RETURNING clause was appended to the `SELECT 1` stand-in and
6824 // the statement failed to parse.
6825 if (noRows)
6826 return emptyReturningBuilder()
67636827 // Append RETURNING clause to the existing SQL
67646828 const returningSql = `${sqlText} RETURNING ${cols.join(', ')}`
67656829 const q = _sql.unsafe(returningSql, params)
@@ -6798,10 +6862,14 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
67986862 }
67996863 },
68006864 toSQL() {
6865 if (noRows)
6866 return makeExecutableQuery(null as any, '') as any
68016867 if (!built) built = _sql.unsafe(sqlText, params)
68026868 return makeExecutableQuery(built, sqlText) as any
68036869 },
68046870 execute() {
6871 if (noRows)
6872 return Promise.resolve(emptyInsertResult())
68056873 // Ultra-fast path: use _prepareStatement to skip unsafe() and runWithHooks overhead
68066874 const hooks = activeHooks()
68076875 const hasHooks = hooks && (hooks.onQueryStart || hooks.onQueryEnd || hooks.onQueryError || hooks.startSpan || hooks.beforeCreate || hooks.afterCreate || hasSlowQueryHook(hooks))
@@ -6816,11 +6884,15 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
68166884 return runWithHooks(built, 'insert')
68176885 },
68186886 async executeTakeFirst() {
6887 if (noRows)
6888 return emptyInsertResult()
68196889 if (!built) built = _sql.unsafe(sqlText, params)
68206890 const result = await runWithHooks(built, 'insert')
68216891 return result
68226892 },
68236893 async executeTakeFirstOrThrow() {
6894 if (noRows)
6895 throw new Error('Insert failed')
68246896 if (!built) built = _sql.unsafe(sqlText, params)
68256897 const result = await runWithHooks(built, 'insert')
68266898 if (!result)
@@ -6828,6 +6900,9 @@ export function createQueryBuilder<DB extends DatabaseSchema<any>>(state?: Parti
68286900 return result
68296901 },
68306902 returningAll() {
6903 // As in returning(): no rows inserted, nothing to return.
6904 if (noRows)
6905 return emptyReturningBuilder()
68316906 const returningSql = `${sqlText} RETURNING *`
68326907 const q = _sql.unsafe(returningSql, params)
68336908 const runFirst = async () => {
packages/bun-query-builder/test/client.insert-empty-values.test.tsadded+168-0
@@ -0,0 +1,168 @@
1/**
2 * `insertInto(t).values([])` must be a no-op, not a stand-in query.
3 * stacksjs/bun-query-builder#1097.
4 *
5 * It ran `SELECT 1` when handed an empty batch, which is invisible in the
6 * database — no row is ever written — but wrong at every other layer:
7 *
8 * - `.execute()` resolved to a FABRICATED row: `[{ '?column?': 1 }]` on
9 * Postgres, `[{ '1': 1 }]` on SQLite. A caller checking `result.length` saw
10 * 1 where the truth was 0.
11 * - a query hook fired, labelled `insert`, for an insert that never happened.
12 * - `.returning(...)` appended RETURNING to `SELECT 1` and died with
13 * `near "RETURNING": syntax error`.
14 *
15 * It answers empty rather than throwing because `values(rows)` where `rows`
16 * came back empty is ordinary calling code — the seeder scaffold this package
17 * generates (`actions/seed.ts`) is written exactly that way, so a throw would
18 * turn an empty dataset into a failed seed.
19 */
20
21import { mkdtempSync, rmSync } from 'node:fs'
22import { tmpdir } from 'node:os'
23import { join } from 'node:path'
24import { Database } from 'bun:sqlite'
25import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
26import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
27import { config, setConfig } from '../src/config'
28import { resetConnection } from '../src/db'
29
30const MODELS = {
31 t: { columns: { id: { type: 'integer', isPrimaryKey: true }, name: { type: 'string' } } },
32} as any
33
34// A file rather than `:memory:`: the client opens its own connection from the
35// configured path, and a `:memory:` database is per-connection, so the table
36// created here would be invisible to it.
37let dir: string
38let dbPath: string
39let snapshot: { dialect: string, database: Record<string, unknown>, hooks: unknown }
40
41function qb(): any {
42 return createQueryBuilder({
43 schema: buildDatabaseSchema(MODELS),
44 meta: buildSchemaMeta(MODELS),
45 autoMigration: { enabled: false } as any,
46 })
47}
48
49/** Row count read through a fresh connection, not the builder's. */
50function rowCount(): number {
51 const probe = new Database(dbPath)
52 const [{ c }] = probe.query('SELECT count(*) c FROM t').all() as any[]
53 probe.close()
54 return c
55}
56
57beforeEach(() => {
58 snapshot = { dialect: config.dialect, database: { ...config.database }, hooks: (config as any).hooks }
59 dir = mkdtempSync(join(tmpdir(), 'qb-1097-'))
60 dbPath = join(dir, 'empty-values.sqlite')
61 const seed = new Database(dbPath, { create: true })
62 seed.run('CREATE TABLE t (id INTEGER PRIMARY KEY AUTOINCREMENT, name TEXT)')
63 seed.close()
64 setConfig({ dialect: 'sqlite', database: { database: dbPath } } as any)
65 resetConnection()
66})
67
68afterEach(() => {
69 config.dialect = snapshot.dialect as any
70 for (const k of Object.keys(config.database)) delete (config.database as any)[k]
71 Object.assign(config.database, snapshot.database)
72 ;(config as any).hooks = snapshot.hooks
73 resetConnection()
74 rmSync(dir, { recursive: true, force: true })
75})
76
77describe('insertInto().values([]) is a no-op (#1097)', () => {
78 it('reports nothing inserted instead of a fabricated row', async () => {
79 const result: any = await qb().insertInto('t').values([]).execute()
80
81 // The bun:sqlite wrapper reports { changes, lastInsertRowid }; zeros are
82 // the honest answer. What must never come back is a row.
83 expect(Array.isArray(result)).toBe(false)
84 expect(result.changes).toBe(0)
85
86 expect(rowCount()).toBe(0)
87 })
88
89 it('does not run a query, so no hook fires', async () => {
90 const seen: string[] = []
91 setConfig({ hooks: { onQueryStart: () => { seen.push('start') } } } as any)
92
93 await qb().insertInto('t').values([]).execute()
94
95 expect(seen).toEqual([])
96 })
97
98 it('emits no SQL', () => {
99 const sql = qb().insertInto('t').values([]).toSQL()
100 expect(String(sql?.sql ?? '')).toBe('')
101 })
102
103 describe('returning()', () => {
104 it('resolves empty instead of failing to parse', async () => {
105 // Previously: `near "RETURNING": syntax error`, because RETURNING was
106 // appended to the `SELECT 1` stand-in.
107 const rows = await qb().insertInto('t').values([]).returning('id').execute()
108 expect(rows).toEqual([])
109 })
110
111 it('keeps the row accessors present and empty', async () => {
112 const b = qb().insertInto('t').values([]).returning('id')
113
114 // `.returning(...).first()` exists on the real builder, so it has to
115 // exist here too rather than throwing "first is not a function".
116 expect(await b.get()).toEqual([])
117 expect(await b.first()).toBeUndefined()
118 expect(await b.executeTakeFirst()).toBeUndefined()
119 await expect(b.firstOrFail()).rejects.toThrow(/returned no rows/)
120 })
121
122 it('behaves the same through returningAll()', async () => {
123 const rows = await qb().insertInto('t').values([]).returningAll().execute()
124 expect(rows).toEqual([])
125 })
126 })
127
128 it('leaves a non-empty insert alone', async () => {
129 const seen: string[] = []
130 setConfig({ hooks: { onQueryStart: () => { seen.push('start') } } } as any)
131
132 const result: any = await qb().insertInto('t').values([{ name: 'a' }]).execute()
133 expect(result.changes).toBe(1)
134 expect(seen.length).toBe(1)
135
136 const rows = await qb().insertInto('t').values([{ name: 'b' }]).returning('id').execute()
137 expect(rows).toHaveLength(1)
138
139 expect(rowCount()).toBe(2)
140 })
141
142 it('reuses the builder correctly when a later call has rows', async () => {
143 // The empty batch sets a flag; a subsequent values() with rows has to clear
144 // it, or the second insert would silently do nothing.
145 const b = qb().insertInto('t')
146 b.values([])
147 b.values([{ name: 'later' }])
148 const result: any = await b.execute()
149
150 expect(result.changes).toBe(1)
151 expect(rowCount()).toBe(1)
152 })
153})
154
155describe('insertInto().values([]) on Postgres (#1097)', () => {
156 it('resolves to [] — what a real insert affecting no rows returns', async () => {
157 // No connection is opened: the empty batch short-circuits before touching
158 // the driver, which is what makes this assertable without a live server.
159 setConfig({ dialect: 'postgres', database: { url: 'postgres://unused:5432/none' } } as any)
160 resetConnection()
161
162 const result = await qb().insertInto('t').values([]).execute()
163
164 // Postgres returns [] for an insert without RETURNING, so an empty batch is
165 // deliberately indistinguishable from a real one that inserted nothing.
166 expect(result).toEqual([])
167 })
168})