ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(sqlite): keep the foreign key when a diff adds a column

#1155
Merged glennmichael123 wants to merge fix/1154-sqlite-add-column-references into main
3 files +209 -15
packages/bun-query-builder/src/drivers/sqlite.tsmodified+43-15
@@ -147,6 +147,21 @@ export class SQLiteDriver implements DialectDriver {
147147 return ''
148148 }
149149
150 /**
151 * `ALTER TABLE … ADD COLUMN`, with the column's FK inline.
152 *
153 * The FK used to be dropped here: `addForeignKey` returns '' on SQLite (no
154 * `ADD CONSTRAINT`), and this rendered only the type and constraints, so a
155 * `belongsTo` added to an EXISTING table produced a plain column and no
156 * foreign key at all — silently, on a migration that reported success. A
157 * fresh CREATE TABLE inlined it correctly the whole time. See #1154.
158 *
159 * SQLite accepts `ADD COLUMN … REFERENCES …` as long as the new column's
160 * default is NULL. A non-NULL default is refused once foreign keys are on and
161 * the table has rows (`Cannot add a REFERENCES column with non-NULL default
162 * value`), so the diff engine sends that combination through `rebuildTable`
163 * instead — see `addedColumnNeedsRebuild`.
164 */
150165 addColumn(tableName: string, column: ColumnPlan): string {
151166 const typeSql = this.getColumnType(column)
152167 const parts: string[] = [this.quoteIdentifier(column.name), typeSql]
@@ -160,6 +175,8 @@ export class SQLiteDriver implements DialectDriver {
160175 parts.push(defaultValue)
161176 }
162177
178 parts.push(...this.referencesClause(column))
179
163180 return `ALTER TABLE ${this.quoteIdentifier(tableName)} ADD COLUMN ${parts.join(' ')};`
164181 }
165182
@@ -335,22 +352,33 @@ export class SQLiteDriver implements DialectDriver {
335352 parts.push(defaultValue)
336353 }
337354
338 // Inline FK — for SQLite this is the ONLY path that works, since
339 // SQLite doesn't support `ALTER TABLE ADD CONSTRAINT`. The
340 // orchestrator (`generateSql` / `generateDiffSql` in migrations.ts)
341 // skips its post-CREATE `addForeignKey` pass when emitting CREATE
342 // TABLE so we don't duplicate the FK on dialects that accept both
343 // forms. Enforcement still requires `PRAGMA foreign_keys = ON` on
344 // the SQLite connection (off by default — set this in the
345 // consumer's connection bootstrap).
346 if (column.references) {
347 parts.push(`REFERENCES ${this.quoteIdentifier(column.references.table)}(${this.quoteIdentifier(column.references.column)})`)
348 if (column.references.onDelete)
349 parts.push(`ON DELETE ${column.references.onDelete.toUpperCase()}`)
350 if (column.references.onUpdate)
351 parts.push(`ON UPDATE ${column.references.onUpdate.toUpperCase()}`)
352 }
355 parts.push(...this.referencesClause(column))
353356
354357 return parts.join(' ')
355358 }
359
360 /**
361 * The inline `REFERENCES` clause, or nothing when the column declares no FK.
362 *
363 * For SQLite this is the ONLY path that works, since SQLite doesn't support
364 * `ALTER TABLE ADD CONSTRAINT`. The orchestrator (`generateSql` /
365 * `generateDiffSql` in migrations.ts) skips its post-CREATE `addForeignKey`
366 * pass when emitting CREATE TABLE so we don't duplicate the FK on dialects
367 * that accept both forms. Enforcement still requires `PRAGMA foreign_keys =
368 * ON` on the SQLite connection (off by default — set this in the consumer's
369 * connection bootstrap).
370 *
371 * Shared with `addColumn`: a column added to an existing table carries its FK
372 * too, and only the CREATE TABLE path used to render it. See #1154.
373 */
374 private referencesClause(column: ColumnPlan): string[] {
375 if (!column.references)
376 return []
377 const parts = [`REFERENCES ${this.quoteIdentifier(column.references.table)}(${this.quoteIdentifier(column.references.column)})`]
378 if (column.references.onDelete)
379 parts.push(`ON DELETE ${column.references.onDelete.toUpperCase()}`)
380 if (column.references.onUpdate)
381 parts.push(`ON UPDATE ${column.references.onUpdate.toUpperCase()}`)
382 return parts
383 }
356384}
packages/bun-query-builder/src/migrations.tsmodified+18-0
@@ -1647,6 +1647,23 @@ function detectColumnRenames(
16471647 }
16481648}
16491649
1650/**
1651 * Whether a column being ADDed on SQLite has to go through a table rebuild.
1652 *
1653 * `ADD COLUMN … REFERENCES …` is the only way to attach a foreign key to a new
1654 * column (SQLite has no `ADD CONSTRAINT`), and SQLite refuses it when the
1655 * column's default is not NULL — `Cannot add a REFERENCES column with non-NULL
1656 * default value`, once foreign keys are on and the table has rows. Rebuilding
1657 * the table applies the same FK through CREATE TABLE, where any default is
1658 * fine. See #1154.
1659 */
1660function addedColumnNeedsRebuild(column: ColumnPlan | undefined): boolean {
1661 if (!column?.references || !column.hasDefault)
1662 return false
1663 const value = column.defaultValue
1664 return value !== undefined && value !== null && String(value).toUpperCase() !== 'NULL'
1665}
1666
16501667/** Whether a column is constrained such that SQLite can't DROP it in place. */
16511668function isColumnConstrained(table: TablePlan, columnName: string): boolean {
16521669 const col = table.columns.find(c => c.name === columnName)
@@ -1987,6 +2004,7 @@ export function generateDiffOperations(previous: MigrationPlan | undefined, next
19872004 = modifiedCols.length > 0
19882005 || fkChangedCols.length > 0
19892006 || removedCols.some(name => isColumnConstrained(prev, name))
2007 || addedCols.some(name => addedColumnNeedsRebuild(currCols[name]))
19902008
19912009 if (needsRebuild) {
19922010 const columnSource: Record<string, string> = {}
packages/bun-query-builder/test/migrations.sqlite-add-column-fk.test.tsadded+148-0
@@ -0,0 +1,148 @@
1/**
2 * A foreign key added to an EXISTING table on SQLite — stacksjs/bun-query-builder#1154.
3 *
4 * `#1019` made `CREATE TABLE` inline `REFERENCES`, which is the only form
5 * SQLite accepts (it has no `ALTER TABLE … ADD CONSTRAINT`). The diff path that
6 * adds a column later did not: `addColumn` rendered the type and constraints
7 * only, and `addForeignKey` returns '' on SQLite so nothing else carried the
8 * reference. A `belongsTo` added to a table that already existed produced
9 *
10 * ALTER TABLE "monitors" ADD COLUMN "server_id" INTEGER;
11 *
12 * and no foreign key at all — on a migration that reported success. A StatusHQ
13 * schema lost five FKs that way.
14 *
15 * SQLite does accept `ADD COLUMN … REFERENCES …`, provided the new column's
16 * default is NULL. With a non-NULL default it refuses once foreign keys are on
17 * and the table has rows (`Cannot add a REFERENCES column with non-NULL default
18 * value`), so that combination goes through a table rebuild instead. Both are
19 * asserted here, and executed, because the SQL text looked plausible either way.
20 */
21
22import type { MigrationPlan, TablePlan } from '../src/migrations'
23import { Database } from 'bun:sqlite'
24import { describe, expect, it } from 'bun:test'
25import { generateDiffSql } from '../src/migrations'
26
27const servers: TablePlan = {
28 table: 'servers',
29 primaryKey: 'id',
30 columns: [
31 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
32 { name: 'name', type: 'string', isPrimaryKey: false, isUnique: false, isNullable: true, hasDefault: false },
33 ],
34 indexes: [],
35}
36
37function monitors(serverId?: Partial<TablePlan['columns'][number]>): TablePlan {
38 return {
39 table: 'monitors',
40 primaryKey: 'id',
41 columns: [
42 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
43 { name: 'name', type: 'string', isPrimaryKey: false, isUnique: false, isNullable: true, hasDefault: false },
44 ...(serverId
45 ? [{
46 name: 'server_id',
47 type: 'bigint',
48 isPrimaryKey: false,
49 isUnique: false,
50 isNullable: true,
51 hasDefault: false,
52 references: { table: 'servers', column: 'id' },
53 ...serverId,
54 } as TablePlan['columns'][number]]
55 : []),
56 ],
57 indexes: [],
58 }
59}
60
61const plan = (table: TablePlan): MigrationPlan => ({ dialect: 'sqlite', tables: [servers, table] } as MigrationPlan)
62
63/** The live schema before the diff: both tables, no `server_id`, one row. */
64function liveDatabase(): Database {
65 const db = new Database(':memory:', { create: true })
66 db.run('PRAGMA foreign_keys = ON')
67 db.run('CREATE TABLE "servers" ("id" INTEGER PRIMARY KEY AUTOINCREMENT, "name" TEXT)')
68 db.run('CREATE TABLE "monitors" ("id" INTEGER PRIMARY KEY AUTOINCREMENT, "name" TEXT)')
69 db.run(`INSERT INTO "servers" ("id", "name") VALUES (1, 'web-1')`)
70 db.run(`INSERT INTO "monitors" ("id", "name") VALUES (1, 'uptime')`)
71 return db
72}
73
74function apply(db: Database, sql: string): void {
75 // The rebuild path emits one chunk of several statements, transaction included.
76 for (const statement of sql.split(';').map(s => s.trim()).filter(Boolean))
77 db.run(`${statement};`)
78}
79
80const foreignKeys = (db: Database): any[] => db.query('PRAGMA foreign_key_list("monitors")').all() as any[]
81
82describe('SQLite ADD COLUMN carries its foreign key (#1154)', () => {
83 it('emits the reference inline', () => {
84 const sql = generateDiffSql(plan(monitors()), plan(monitors({}))).join('\n')
85
86 // Was `ADD COLUMN "server_id" INTEGER;` — the reference was dropped.
87 expect(sql).toContain('ALTER TABLE "monitors" ADD COLUMN "server_id" INTEGER REFERENCES "servers"("id")')
88 // SQLite cannot run either spelling of a constraint-adding ALTER.
89 expect(sql).not.toContain('ADD CONSTRAINT')
90 expect(sql).not.toContain('ADD FOREIGN KEY')
91 })
92
93 it('carries ON DELETE / ON UPDATE actions', () => {
94 const next = monitors({ references: { table: 'servers', column: 'id', onDelete: 'cascade', onUpdate: 'restrict' } })
95 const sql = generateDiffSql(plan(monitors()), plan(next)).join('\n')
96
97 expect(sql).toContain('REFERENCES "servers"("id") ON DELETE CASCADE ON UPDATE RESTRICT')
98 })
99
100 it('the emitted SQL runs and the foreign key is enforced', () => {
101 const db = liveDatabase()
102 apply(db, generateDiffSql(plan(monitors()), plan(monitors({}))).join('\n'))
103
104 const fks = foreignKeys(db)
105 expect(fks.length).toBe(1)
106 expect(fks[0]).toMatchObject({ table: 'servers', from: 'server_id', to: 'id' })
107
108 // The existing row survives, and the constraint actually bites.
109 expect(db.query('SELECT name FROM monitors WHERE id = 1').get()).toEqual({ name: 'uptime' })
110 db.run('UPDATE "monitors" SET "server_id" = 1 WHERE "id" = 1')
111 expect(() => db.run('UPDATE "monitors" SET "server_id" = 999 WHERE "id" = 1')).toThrow(/FOREIGN KEY constraint failed/)
112 db.close()
113 })
114
115 it('a non-NULL default goes through a table rebuild, which SQLite accepts', () => {
116 const next = monitors({ hasDefault: true, defaultValue: 1, isNullable: false })
117 const sql = generateDiffSql(plan(monitors()), plan(next)).join('\n')
118
119 // `ADD COLUMN … REFERENCES` with a non-NULL default is refused outright.
120 expect(sql).not.toContain('ADD COLUMN')
121 expect(sql).toContain('CREATE TABLE "_qb_tmp_monitors"')
122 expect(sql).toContain('REFERENCES "servers"("id")')
123
124 const db = liveDatabase()
125 apply(db, sql)
126 const fks = foreignKeys(db)
127 expect(fks.length).toBe(1)
128 expect(db.query('SELECT name, server_id FROM monitors WHERE id = 1').get()).toEqual({ name: 'uptime', server_id: 1 })
129 db.close()
130 })
131
132 it('a NULL default still uses ADD COLUMN', () => {
133 const next = monitors({ hasDefault: true, defaultValue: null })
134 const sql = generateDiffSql(plan(monitors()), plan(next)).join('\n')
135
136 expect(sql).toContain('ADD COLUMN "server_id" INTEGER')
137 expect(sql).toContain('REFERENCES "servers"("id")')
138 expect(sql).not.toContain('_qb_tmp_monitors')
139 })
140
141 it('a column with no reference is unchanged', () => {
142 const next = monitors({ references: undefined })
143 const sql = generateDiffSql(plan(monitors()), plan(next)).join('\n')
144
145 expect(sql).toContain('ALTER TABLE "monitors" ADD COLUMN "server_id" INTEGER;')
146 expect(sql).not.toContain('REFERENCES')
147 })
148})