ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix: a multi-row insert names every column any row carries (#1189)

#1190
Merged glennmichael123 wants to merge fix/multi-row-insert-column-union into main
3 files +384 -11
docs/advanced/dialects.mdmodified+27-0
@@ -611,6 +611,33 @@ Two consequences worth knowing:
611611
612612If you want the server to enforce the instant and your dates stay before 2038, declare the column `TIMESTAMP` in a hand-written migration rather than through a model.
613613
614### A multi-row insert with uneven rows loses column defaults on SQLite
615
616A multi-row insert names the union of every row's keys, so a column only some rows set is still written for the rows that set it:
617
618```ts
619await db.insertInto('timeline_entries').values([
620 { kind: 'labeled', subject_text: 'first' },
621 { kind: 'renamed', subject_text: 'second', previous_text: 'kept' },
622]).execute()
623```
624
625A row that lacks one of those columns has to put *something* in its tuple, and that is where the dialects differ:
626
627| dialect | emitted for the gap | effect |
628| --- | --- | --- |
629| Postgres, MySQL | `DEFAULT` | the column's own default applies |
630| SQLite | `NULL` | the column default is **bypassed** |
631
632SQLite has no `DEFAULT` keyword inside a `VALUES` list — it answers `near "DEFAULT": syntax error` — so there is nothing to emit that means "use the default". A column with a non-null default therefore stores `NULL` for the rows that omitted it, and a `NOT NULL` column with a default fails the insert.
633
634This only bites when the rows are uneven *and* the missing column has a default. Two ways out, both explicit:
635
636- set the key on every row, so the batch is uniform and nothing has to be filled in
637- group the rows by shape and insert each group separately, so a column no row sets is simply not named
638
639Uniform batches are unaffected on every dialect, and so is the single-row form.
640
614641## Performance Tips Per Dialect
615642
616643- Postgres: leverage `EXPLAIN (ANALYZE, BUFFERS)` during tuning; create appropriate indexes for JSON paths if needed
packages/bun-query-builder/src/client.tsmodified+120-11
@@ -263,11 +263,13 @@ function quoteInsertIdent(id: string): string {
263263 * stacksjs/bun-query-builder#1052.
264264 */
265265function buildInsertClause(rows: Record<string, any>[], startIndex = 1): { colsSql: string, valuesSql: string, params: any[], nextIndex: number } {
266 const cols = Object.keys(rows[0] ?? {})
266 const { keys: cols, uniform } = insertColumns(rows)
267267 const params: any[] = []
268268 let idx = startIndex
269269 const tuples = rows.map((row) => {
270270 const phs = cols.map((c) => {
271 if (!uniform && !(c in row))
272 return absentCellSql()
271273 params.push(row[c])
272274 return getPlaceholder(idx++)
273275 })
@@ -281,6 +283,73 @@ function buildInsertClause(rows: Record<string, any>[], startIndex = 1): { colsS
281283 }
282284}
283285
286/**
287 * The columns a multi-row INSERT has to name, and whether every row has them.
288 *
289 * The column list used to come from `Object.keys(rows[0])`, and every row was
290 * then read through it as `row[keys[c]]`. A key that appeared only in a later
291 * row had nowhere to go, so its value was dropped from the statement - from
292 * every row that carried it - with no error and the full row count reported.
293 *
294 * That shape is the realistic one, not a corner case: a timeline where a
295 * `renamed` entry carries two titles and a `labeled` entry carries one, an
296 * import where some records set an optional field, an audit log where only some
297 * events carry a reference. The reporter lost five cross-references out of forty
298 * seeded rows and found it only by running `count(reference_number)` afterwards,
299 * because nothing threw and the table was not empty.
300 * stacksjs/bun-query-builder#1189
301 *
302 * So the union of every row's keys, in first-appearance order. `uniform` says
303 * whether the callers' fast paths still apply: when every row has every column,
304 * which is the common case, nothing about the emitted statement changes and no
305 * per-cell presence check runs.
306 */
307function insertColumns(rows: Record<string, any>[]): { keys: string[], uniform: boolean } {
308 const keys = Object.keys(rows[0] ?? {})
309 if (rows.length < 2)
310 return { keys, uniform: true }
311
312 const seen = new Set(keys)
313 let uniform = true
314
315 for (let r = 1; r < rows.length; r++) {
316 const rowKeys = Object.keys(rows[r])
317 // Safe to compare against the union as it stands: the only way it grows is
318 // a key row 0 lacked, and that sets `uniform` false on the spot. So either
319 // this comparison is against the final list, or the flag is already false.
320 if (rowKeys.length !== keys.length)
321 uniform = false
322 for (const key of rowKeys) {
323 if (!seen.has(key)) {
324 seen.add(key)
325 keys.push(key)
326 uniform = false
327 }
328 }
329 }
330
331 return { keys, uniform }
332}
333
334/**
335 * What a row that lacks one of the batch's columns puts in its VALUES tuple.
336 *
337 * `DEFAULT` rather than a bound NULL, so the column's own default still applies
338 * - a timestamp default, a sequence, a non-null default would all be overridden
339 * by writing NULL, and a NOT NULL column with a default would reject the row
340 * outright. It consumes no placeholder, which is why callers advance their
341 * parameter index only on bound cells.
342 *
343 * SQLite has no `DEFAULT` keyword inside a VALUES list (`near "DEFAULT": syntax
344 * error`), so there it binds NULL and the column default is lost. That is a
345 * real difference between dialects and it is documented in `docs/`, but it is
346 * still the value the caller asked for reaching the right column, where before
347 * it reached nothing at all.
348 */
349function absentCellSql(): string {
350 return config.dialect === 'sqlite' || config.dialect === 'browser' ? 'NULL' : 'DEFAULT'
351}
352
284353/** Options shared by the generalized window functions (#1050). */
285354interface WindowOpts {
286355 partitionBy?: string | string[]
@@ -7642,7 +7711,8 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
76427711 noRows = false
76437712
76447713 const firstRow = rows[0]
7645 const keys = Object.keys(firstRow)
7714 // The union across every row, not just the first one's keys (#1189).
7715 const { keys, uniform } = insertColumns(rows)
76467716 const colCount = keys.length
76477717
76487718 // Pre-allocate params array
@@ -7713,11 +7783,22 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
77137783 for (let c = 0; c < colCount; c++) {
77147784 if (c > 0)
77157785 sqlText += ','
7786 // A row missing this column contributes DEFAULT (or NULL on
7787 // SQLite) and binds nothing, so `pidx` only advances on cells
7788 // that actually carry a parameter.
7789 if (!uniform && !(keys[c] in row)) {
7790 sqlText += absentCellSql()
7791 continue
7792 }
77167793 sqlText += getPlaceholder(pidx)
77177794 params[pidx++] = row[keys[c]]
77187795 }
77197796 }
77207797 sqlText += ')'
7798 // Pre-allocated for the uniform case; the absent cells bound no
7799 // parameter, so drop the unused tail rather than passing undefined.
7800 if (pidx !== totalParams)
7801 params.length = pidx
77217802 }
77227803
77237804 // Bound both the number of tables and each retained SQL shape. Column
@@ -9005,11 +9086,11 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
90059086 if (!rows?.length)
90069087 return
90079088
9008 const firstRow = rows[0]
9009 const keys = Object.keys(firstRow)
9089 // The union across every row, not just the first one's keys (#1189).
9090 const { keys, uniform } = insertColumns(rows as Record<string, any>[])
90109091 const colCount = keys.length
90119092 const rowCount = rows.length
9012 const params = Array.from({ length: rowCount * colCount })
9093 const params: any[] = []
90139094
90149095 // Quote table + column identifiers (#1033) — MySQL backticks, else
90159096 // double quotes (Postgres/SQLite).
@@ -9017,17 +9098,20 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
90179098 ? (id: string): string => `\`${String(id).replace(/`/g, '``')}\``
90189099 : (id: string): string => `"${String(id).replace(/"/g, '""')}"`
90199100 let sql = `INSERT INTO ${quoteId(String(table))}(${keys.map(quoteId).join(',')})VALUES`
9020 let pidx = 0
90219101 for (let r = 0; r < rowCount; r++) {
90229102 if (r > 0)
90239103 sql += ','
90249104 sql += '('
9025 const row = rows[r]
9105 const row = rows[r] as Record<string, any>
90269106 for (let c = 0; c < colCount; c++) {
90279107 if (c > 0)
90289108 sql += ','
9029 sql += getPlaceholder(pidx + 1)
9030 params[pidx++] = row[keys[c]]
9109 if (!uniform && !(keys[c] in row)) {
9110 sql += absentCellSql()
9111 continue
9112 }
9113 sql += getPlaceholder(params.length + 1)
9114 params.push(row[keys[c]])
90319115 }
90329116 sql += ')'
90339117 }
@@ -9038,8 +9122,8 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
90389122 if (!rows?.length)
90399123 return
90409124
9041 const firstRow = rows[0]
9042 const keys = Object.keys(firstRow)
9125 // The union across every row, not just the first one's keys (#1189).
9126 const { keys, uniform } = insertColumns(rows as Record<string, any>[])
90439127 const colCount = keys.length
90449128 const rowCount = rows.length
90459129 const totalParams = rowCount * colCount
@@ -9047,6 +9131,31 @@ export function createQueryBuilder<DB extends AnyDatabaseSchema>(state?: Partial
90479131
90489132 // Pre-build a single row placeholder template: (?,?,?,?) or ($1,$2,$3,$4)
90499133 const isPositional = config.dialect === 'postgres'
9134
9135 // Both paths below reuse one shape for every row — the `?` template
9136 // literally, the Postgres one by index arithmetic — which only holds when
9137 // every row carries every column. A heterogeneous batch emits its tuples
9138 // one at a time instead, with DEFAULT (or NULL on SQLite) where a row
9139 // lacks a column.
9140 if (!uniform) {
9141 const tuples = new Array<string>(rowCount)
9142 const bound: any[] = []
9143 for (let r = 0; r < rowCount; r++) {
9144 const row = rows[r] as Record<string, any>
9145 const cells = new Array<string>(colCount)
9146 for (let c = 0; c < colCount; c++) {
9147 if (!(keys[c] in row)) {
9148 cells[c] = absentCellSql()
9149 continue
9150 }
9151 cells[c] = isPositional ? `$${bound.length + 1}` : '?'
9152 bound.push(row[keys[c]])
9153 }
9154 tuples[r] = `(${cells.join(',')})`
9155 }
9156 return prepareQuery(`INSERT INTO ${table}(${keys.join(',')})VALUES${tuples.join(',')}`, bound).execute()
9157 }
9158
90509159 let rowTemplate: string
90519160 if (!isPositional) {
90529161 // SQLite/MySQL: all placeholders are ?, build once and reuse
packages/bun-query-builder/test/client.insert-heterogeneous-rows.test.tsadded+237-0
@@ -0,0 +1,237 @@
1/**
2 * A multi-row INSERT must name every column any row carries —
3 * stacksjs/bun-query-builder#1189.
4 *
5 * The column list came from `Object.keys(rows[0])`, and each row was then read
6 * through it as `row[keys[c]]`. A key that appeared only in a later row had
7 * nowhere to go, so its value was dropped from the statement — from every row
8 * that carried it — with no error and the full row count reported.
9 *
10 * Four builders had it independently: `insertInto().values([...])`, the
11 * `createMany()` and `insertMany()` helpers, and `buildInsertClause()`, which
12 * backs upsert / insertOrIgnore / insertGetId / updateOrInsert.
13 *
14 * The reporter lost five cross-references out of forty seeded rows and found it
15 * only by running `count(reference_number)` afterwards. That is the shape worth
16 * pinning: nothing throws, the count is right, the table is not empty, so a
17 * test asserting "the insert succeeded" cannot see it. Every case below either
18 * reads the value back or asserts the emitted SQL names the column.
19 */
20
21import { afterEach, beforeEach, describe, expect, it } from 'bun:test'
22import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'
23import { tmpdir } from 'node:os'
24import { join, resolve } from 'node:path'
25import process from 'node:process'
26import { buildDatabaseSchema, buildSchemaMeta, createQueryBuilder } from '../src'
27import { config } from '../src/config'
28import { PG_URL, probePostgres } from './pg'
29
30const pgAvailable = await probePostgres()
31const SRC = resolve(import.meta.dir, '../src/index.ts')
32
33function qb() {
34 const models = {
35 entries: {
36 columns: {
37 id: { type: 'integer', isPrimaryKey: true },
38 kind: { type: 'text' },
39 subject_text: { type: 'text' },
40 previous_text: { type: 'text' },
41 },
42 },
43 } as any
44 return createQueryBuilder<any>({
45 schema: buildDatabaseSchema(models),
46 meta: buildSchemaMeta(models),
47 autoMigration: { enabled: false } as any,
48 })
49}
50
51/** The issue's own rows: `previous_text` is set on the second one only. */
52const ROWS = [
53 { kind: 'labeled', subject_text: 'first-row-only' },
54 { kind: 'renamed', subject_text: 'second', previous_text: 'SHOULD PERSIST' },
55]
56
57describe('a multi-row INSERT names every column any row carries (#1189)', () => {
58 let dialect: string
59 beforeEach(() => { dialect = config.dialect })
60 afterEach(() => { config.dialect = dialect as any })
61
62 it('includes a column that only a later row sets', () => {
63 config.dialect = 'sqlite' as any
64 const sql = String((qb() as any).insertInto('entries').values(ROWS).toSQL())
65
66 // Before the fix this read `("kind","subject_text")` and the third value
67 // was absent from the statement entirely.
68 expect(sql).toContain('"previous_text"')
69 expect(sql).toContain('INSERT INTO "entries"("kind","subject_text","previous_text")')
70 })
71
72 it('orders the union by first appearance, so existing columns keep their place', () => {
73 config.dialect = 'sqlite' as any
74 const sql = String((qb() as any).insertInto('entries').values([
75 { kind: 'a' },
76 { subject_text: 'b' },
77 { previous_text: 'c' },
78 ]).toSQL())
79
80 expect(sql).toContain('("kind","subject_text","previous_text")')
81 })
82
83 it('fills the gap with DEFAULT, so a column default still applies', () => {
84 config.dialect = 'postgres' as any
85 const sql = String((qb() as any).insertInto('entries').values(ROWS).toSQL())
86
87 // Row 1 lacks previous_text. DEFAULT rather than a bound NULL: NULL would
88 // override a non-null default and a NOT NULL column with a default would
89 // reject the row. DEFAULT consumes no placeholder, hence $1..$5 for 6 cells.
90 expect(sql).toContain('($1,$2,DEFAULT),($3,$4,$5)')
91 })
92
93 it('binds NULL on SQLite, which has no DEFAULT inside a VALUES list', () => {
94 config.dialect = 'sqlite' as any
95 const sql = String((qb() as any).insertInto('entries').values(ROWS).toSQL())
96
97 // `near "DEFAULT": syntax error` — verified against bun:sqlite. So the
98 // column default is lost here; the value the caller DID supply is not.
99 expect(sql).toContain('(?,?,NULL),(?,?,?)')
100 })
101
102 it('leaves a uniform batch exactly as it was', () => {
103 config.dialect = 'postgres' as any
104 const sql = String((qb() as any).insertInto('entries').values([
105 { kind: 'a', subject_text: 'x' },
106 { kind: 'b', subject_text: 'y' },
107 ]).toSQL())
108
109 // The common case: no presence check, no DEFAULT, placeholders dense.
110 expect(sql).toContain('($1,$2),($3,$4)')
111 expect(sql).not.toContain('DEFAULT')
112 })
113
114 it('still emits nothing for an empty batch (#1097)', () => {
115 config.dialect = 'sqlite' as any
116 expect(String((qb() as any).insertInto('entries').values([]).toSQL())).not.toContain('INSERT')
117 })
118})
119
120/**
121 * The part that matters: the value has to be IN the table afterwards.
122 *
123 * In a subprocess because `config` is process-wide and other files pin the
124 * dialect to sqlite, and because reading the row back is the only assertion
125 * that could have failed while the bug was live — `toSQL()` does not expose
126 * parameters, and every negative case ("this row has no previous_text") passed
127 * throughout.
128 */
129function runProbe(body: string, env: Record<string, string> = {}): { code: number | null, output: string } {
130 const dir = mkdtempSync(join(tmpdir(), 'qb-1189-'))
131 try {
132 const script = join(dir, 'probe.ts')
133 writeFileSync(script, body)
134 const child = Bun.spawnSync([process.execPath, script], {
135 env: { ...process.env, ...env },
136 stdout: 'pipe',
137 stderr: 'pipe',
138 })
139 return { code: child.exitCode, output: `${child.stdout.toString()}${child.stderr.toString()}` }
140 }
141 finally {
142 rmSync(dir, { recursive: true, force: true })
143 }
144}
145
146describe('the value reaches the table (#1189)', () => {
147 it('persists through every multi-row entry point on SQLite', () => {
148 const { code, output } = runProbe(`
149import { Database } from 'bun:sqlite'
150import { setConfig, createQueryBuilder, buildDatabaseSchema, buildSchemaMeta } from ${JSON.stringify(SRC)}
151import { mkdtempSync } from 'node:fs'
152import { tmpdir } from 'node:os'
153import { join } from 'node:path'
154
155const file = join(mkdtempSync(join(tmpdir(), 'qb-1189-db-')), 'r.db')
156new Database(file).run('CREATE TABLE entries (id INTEGER PRIMARY KEY AUTOINCREMENT, kind TEXT, subject_text TEXT, previous_text TEXT)')
157setConfig({ dialect: 'sqlite', database: { database: file } })
158
159const models = { entries: { columns: { id: { type: 'integer', isPrimaryKey: true }, kind: { type: 'text' }, subject_text: { type: 'text' }, previous_text: { type: 'text' } } } }
160const db = createQueryBuilder({ schema: buildDatabaseSchema(models), meta: buildSchemaMeta(models), autoMigration: { enabled: false } })
161const ROWS = ${JSON.stringify(ROWS)}
162const failures = []
163
164async function check(label, run) {
165 new Database(file).run('DELETE FROM entries')
166 await run()
167 const rows = new Database(file).query('SELECT kind, previous_text FROM entries ORDER BY id').all()
168 if (rows.length !== 2)
169 failures.push(label + ': expected 2 rows, got ' + rows.length)
170 // The count was always right. This is the assertion the bug survived.
171 if (rows[1]?.previous_text !== 'SHOULD PERSIST')
172 failures.push(label + ': previous_text = ' + JSON.stringify(rows[1]?.previous_text))
173 if (rows[0]?.previous_text !== null)
174 failures.push(label + ': row 0 should be null, got ' + JSON.stringify(rows[0]?.previous_text))
175}
176
177await check('values()', () => db.insertInto('entries').values(ROWS).execute())
178await check('createMany()', () => db.createMany('entries', ROWS))
179await check('insertMany()', () => db.insertMany('entries', ROWS))
180
181if (failures.length) {
182 console.error(failures.join('\\n'))
183 process.exit(1)
184}
185console.log('OK')
186`)
187
188 expect(output).toContain('OK')
189 expect(code).toBe(0)
190 }, 30_000)
191
192 it.skipIf(!pgAvailable)('takes the column default, not NULL, for the gap on Postgres', () => {
193 // Only observable against a real server: `note` is NOT NULL with a default
194 // and is set on the second row only, so it IS in the union and row 1 has to
195 // fill it. DEFAULT stores the default; a bound NULL would fail the NOT NULL
196 // constraint outright.
197 const { code, output } = runProbe(`
198import { SQL } from 'bun'
199import { setConfig, createQueryBuilder, buildDatabaseSchema, buildSchemaMeta } from ${JSON.stringify(SRC)}
200
201const URL = ${JSON.stringify(PG_URL)}
202const raw = new SQL(URL)
203const T = 't1189_' + Date.now()
204await raw.unsafe('CREATE TABLE ' + T + ' (id serial primary key, kind text, note text NOT NULL DEFAULT \\'from-column-default\\')')
205
206setConfig({ dialect: 'postgres', database: { url: URL } })
207const models = { [T]: { columns: { id: { type: 'integer', isPrimaryKey: true }, kind: { type: 'text' }, note: { type: 'text' } } } }
208const db = createQueryBuilder({ schema: buildDatabaseSchema(models), meta: buildSchemaMeta(models), autoMigration: { enabled: false } })
209const failures = []
210
211try {
212 await db.insertInto(T).values([{ kind: 'without-note' }, { kind: 'with-note', note: 'explicit' }]).execute()
213 const rows = await raw.unsafe('SELECT kind, note FROM ' + T + ' ORDER BY id')
214 if (rows[0]?.note !== 'from-column-default')
215 failures.push('row 0 note = ' + JSON.stringify(rows[0]?.note) + ' (expected the column default)')
216 if (rows[1]?.note !== 'explicit')
217 failures.push('row 1 note = ' + JSON.stringify(rows[1]?.note))
218}
219catch (error) {
220 failures.push('insert threw: ' + error.message)
221}
222finally {
223 await raw.unsafe('DROP TABLE IF EXISTS ' + T)
224 await raw.end()
225}
226
227if (failures.length) {
228 console.error(failures.join('\\n'))
229 process.exit(1)
230}
231console.log('OK')
232`)
233
234 expect(output).toContain('OK')
235 expect(code).toBe(0)
236 }, 30_000)
237})