ReviewOS
Reload

also looking at this

stacks/bun-query-builder

fix(schema): emit inline FOREIGN KEY on SQLite CREATE TABLE (#1019)

#1020
Merged glennmichael123 wants to merge fix/inline-foreign-keys into main
6 files +239 -10
packages/bun-query-builder/src/drivers/mysql.tsmodified+11-0
@@ -205,6 +205,17 @@ export class MySQLDriver implements DialectDriver {
205205 parts.push(defaultValue)
206206 }
207207
208 // MySQL FKs are added via a separate `ALTER TABLE … ADD
209 // CONSTRAINT FOREIGN KEY` pass (see migrations.ts), NOT inline.
210 // MySQL strictly requires the referenced table to exist before
211 // CREATE TABLE with an inline FK, which makes inline FKs in the
212 // initial-migration pass fragile when `plan.tables` is
213 // alphabetically ordered (e.g. `comments` before `users`). The
214 // deferred-ALTER form sidesteps the dependency order entirely.
215 // SQLite is the only driver where the inline path is forced —
216 // it doesn't support ALTER TABLE ADD CONSTRAINT and is lenient
217 // about forward-reference targets.
218
208219 return parts.join(' ')
209220 }
210221}
packages/bun-query-builder/src/drivers/postgres.tsmodified+10-0
@@ -186,6 +186,16 @@ export class PostgresDriver implements DialectDriver {
186186 parts.push(defaultValue)
187187 }
188188
189 // PostgreSQL FKs are added via a separate `ALTER TABLE … ADD
190 // CONSTRAINT FOREIGN KEY` pass (see migrations.ts), NOT inline.
191 // Postgres strictly requires the referenced table to exist
192 // before CREATE TABLE with an inline FK, so the deferred-ALTER
193 // form sidesteps dependency-ordering questions when
194 // `plan.tables` is iterated in (e.g.) alphabetical order.
195 // SQLite is the only driver where the inline path is forced —
196 // it doesn't support ALTER TABLE ADD CONSTRAINT and is lenient
197 // about forward-reference targets.
198
189199 return parts.join(' ')
190200 }
191201}
packages/bun-query-builder/src/drivers/sqlite.tsmodified+26-8
@@ -107,14 +107,16 @@ export class SQLiteDriver implements DialectDriver {
107107 return `CREATE ${kind}INDEX IF NOT EXISTS ${this.quoteIdentifier(idxName)} ON ${this.quoteIdentifier(tableName)} (${columns})${where};`
108108 }
109109
110 addForeignKey(tableName: string, columnName: string, refTable: string, refColumn: string, onDelete?: string, onUpdate?: string): string {
111 const fkName = `${tableName}_${columnName}_fk`
112 let sql = `ALTER TABLE ${this.quoteIdentifier(tableName)} ADD CONSTRAINT ${this.quoteIdentifier(fkName)} FOREIGN KEY (${this.quoteIdentifier(columnName)}) REFERENCES ${this.quoteIdentifier(refTable)}(${this.quoteIdentifier(refColumn)})`
113 if (onDelete)
114 sql += ` ON DELETE ${onDelete.toUpperCase()}`
115 if (onUpdate)
116 sql += ` ON UPDATE ${onUpdate.toUpperCase()}`
117 return `${sql};`
110 addForeignKey(_tableName: string, _columnName: string, _refTable: string, _refColumn: string, _onDelete?: string, _onUpdate?: string): string {
111 // SQLite doesn't support `ALTER TABLE … ADD CONSTRAINT FOREIGN
112 // KEY` — it can only declare FKs inline on `CREATE TABLE`, which
113 // `renderColumn` already does. Return an empty string so the
114 // orchestrator (`generateSql` / `generateDiffSql`) skips emitting
115 // an unrunnable ALTER migration file for SQLite.
116 //
117 // Consumers that previously stripped these files from disk after
118 // generation (e.g. stacksjs/stacks#1916) can drop that workaround.
119 return ''
118120 }
119121
120122 addColumn(tableName: string, column: ColumnPlan): string {
@@ -200,6 +202,22 @@ export class SQLiteDriver implements DialectDriver {
200202 parts.push(defaultValue)
201203 }
202204
205 // Inline FK — for SQLite this is the ONLY path that works, since
206 // SQLite doesn't support `ALTER TABLE ADD CONSTRAINT`. The
207 // orchestrator (`generateSql` / `generateDiffSql` in migrations.ts)
208 // skips its post-CREATE `addForeignKey` pass when emitting CREATE
209 // TABLE so we don't duplicate the FK on dialects that accept both
210 // forms. Enforcement still requires `PRAGMA foreign_keys = ON` on
211 // the SQLite connection (off by default — set this in the
212 // consumer's connection bootstrap).
213 if (column.references) {
214 parts.push(`REFERENCES ${this.quoteIdentifier(column.references.table)}(${this.quoteIdentifier(column.references.column)})`)
215 if (column.references.onDelete)
216 parts.push(`ON DELETE ${column.references.onDelete.toUpperCase()}`)
217 if (column.references.onUpdate)
218 parts.push(`ON UPDATE ${column.references.onUpdate.toUpperCase()}`)
219 }
220
203221 return parts.join(' ')
204222 }
205223}
packages/bun-query-builder/src/migrations.tsmodified+22-2
@@ -757,11 +757,27 @@ export function generateSql(plan: MigrationPlan): string[] {
757757 createMigrationFile(combinedStatement, `create-${t.table}-table`)
758758 }
759759
760 // First, create all foreign key constraints (ALTER statements - will execute last)
760 // Foreign key constraints (ALTER statements — execute last).
761 //
762 // MySQL/PostgreSQL strictly require the referenced table to exist
763 // before CREATE TABLE if the FK is declared inline, which makes the
764 // alphabetically-ordered `plan.tables` iteration order fragile.
765 // Deferring the FK to a separate ALTER pass means tables can be
766 // created in any order and FKs land after.
767 //
768 // SQLite skips this pass entirely — it doesn't support `ALTER TABLE
769 // ADD CONSTRAINT FOREIGN KEY` at all. Its FKs ride inline on the
770 // CREATE TABLE statement via `renderColumn`, which works because
771 // SQLite is lenient about forward-reference targets (the constraint
772 // is checked at INSERT time, not CREATE). SQLite's `addForeignKey`
773 // returns an empty string so the `if (!alterTableStatement)
774 // continue` line below skips the no-op file. See
775 // stacksjs/bun-query-builder#1019.
761776 for (const t of plan.tables) {
762777 for (const c of t.columns) {
763778 if (c.references) {
764779 const alterTableStatement = driver.addForeignKey(t.table, c.name, c.references.table, c.references.column, c.references.onDelete, c.references.onUpdate)
780 if (!alterTableStatement) continue
765781 statements.push(alterTableStatement)
766782 createMigrationFile(alterTableStatement, `alter-${t.table}-${c.name}`)
767783 }
@@ -959,13 +975,17 @@ export function generateDiffSql(previous: MigrationPlan | undefined, next: Migra
959975 }
960976 }
961977
962 // 3) Add foreign key constraints for new tables (ALTER statements - execute last)
978 // 3) Add foreign key constraints for new tables (ALTER statements -
979 // execute last). Mirrors the initial-migration FK pass above:
980 // MySQL/Postgres use deferred ALTER, SQLite skips (FKs are already
981 // inline from CREATE TABLE).
963982 for (const tableName of Object.keys(nextTables)) {
964983 if (!prevTables[tableName]) {
965984 const t = nextTables[tableName]
966985 for (const c of t.columns) {
967986 if (c.references) {
968987 const alterTableStatement = driver.addForeignKey(t.table, c.name, c.references.table, c.references.column, c.references.onDelete, c.references.onUpdate)
988 if (!alterTableStatement) continue
969989 chunks.push(alterTableStatement)
970990 createMigrationFile(alterTableStatement, `alter-${t.table}-${c.name}`)
971991 }
packages/bun-query-builder/test/foreign-keys-inline.test.tsadded+145-0
@@ -0,0 +1,145 @@
1import type { TablePlan } from '../src/migrations'
2import { describe, expect, it } from 'bun:test'
3import { generateSql } from '../src/migrations'
4
5// stacksjs/bun-query-builder#1019 — Foreign keys must reach the
6// generated DDL via *some* path on every supported dialect. Prior
7// to this fix:
8//
9// - All three drivers' `renderColumn` silently ignored
10// `column.references`, so inline FKs never emitted at all.
11// - SQLite's `addForeignKey` produced `ALTER TABLE … ADD
12// CONSTRAINT FOREIGN KEY …`, which SQLite cannot execute.
13// The generated file landed on disk and either failed at
14// migrate time or had to be stripped by the consumer.
15//
16// After this fix:
17//
18// - SQLite emits FKs inline on `CREATE TABLE` via `renderColumn`
19// (the only path SQLite supports). Its `addForeignKey` returns
20// an empty string so the orchestrator skips the unrunnable
21// ALTER pass entirely.
22// - MySQL / PostgreSQL keep the deferred-ALTER strategy — inline
23// FKs would fail when `plan.tables` is iterated in an order
24// that references a forward-defined table (e.g. alphabetical:
25// `comments` before `users`).
26
27function makePlan(dialect: 'sqlite' | 'mysql' | 'postgres'): any {
28 return {
29 dialect,
30 tables: [
31 {
32 table: 'users',
33 primaryKey: 'id',
34 columns: [
35 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
36 ] satisfies TablePlan['columns'],
37 indexes: [],
38 },
39 {
40 table: 'posts',
41 primaryKey: 'id',
42 columns: [
43 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
44 {
45 name: 'user_id',
46 type: 'bigint',
47 isPrimaryKey: false,
48 isUnique: false,
49 isNullable: false,
50 hasDefault: false,
51 references: { table: 'users', column: 'id', onDelete: 'cascade' },
52 },
53 ] satisfies TablePlan['columns'],
54 indexes: [],
55 },
56 ],
57 }
58}
59
60describe('CREATE TABLE foreign-key emission (stacksjs/bun-query-builder#1019)', () => {
61 it('sqlite emits inline REFERENCES — the only path that works', () => {
62 const sql = generateSql(makePlan('sqlite')).join('\n')
63
64 expect(sql).toMatch(/CREATE TABLE IF NOT EXISTS "posts"[\s\S]*"user_id"\s+INTEGER\b[\s\S]*?REFERENCES\s+"users"\("id"\)\s+ON DELETE CASCADE/)
65 // No separate ALTER pass — SQLite can't execute ADD CONSTRAINT.
66 expect(sql).not.toContain('ALTER TABLE')
67 expect(sql).not.toContain('ADD CONSTRAINT')
68 })
69
70 it('mysql defers FKs to ALTER TABLE ADD CONSTRAINT after all CREATE TABLEs', () => {
71 const sql = generateSql(makePlan('mysql')).join('\n')
72
73 // The CREATE TABLE block for posts must NOT carry inline
74 // REFERENCES — emitting inline would fail when iteration order
75 // lands `posts` before `users`. Match only the create-table
76 // statement body (between `CREATE TABLE … (` and the matching
77 // closing `);`) and assert REFERENCES is absent inside it.
78 const createPosts = sql.match(/CREATE TABLE[^;]*posts[^;]*;/)
79 expect(createPosts).toBeTruthy()
80 expect(createPosts?.[0]).not.toContain('REFERENCES')
81 // The deferred ALTER pass still emits the FK.
82 expect(sql).toContain('ALTER TABLE')
83 expect(sql).toContain('FOREIGN KEY')
84 expect(sql).toContain('REFERENCES `users`(`id`)')
85 expect(sql).toContain('ON DELETE CASCADE')
86 })
87
88 it('postgres defers FKs to ALTER TABLE ADD CONSTRAINT after all CREATE TABLEs', () => {
89 const sql = generateSql(makePlan('postgres')).join('\n')
90
91 const createPosts = sql.match(/CREATE TABLE[^;]*posts[^;]*;/)
92 expect(createPosts).toBeTruthy()
93 expect(createPosts?.[0]).not.toContain('REFERENCES')
94 expect(sql).toContain('ALTER TABLE')
95 expect(sql).toContain('FOREIGN KEY')
96 expect(sql).toContain('REFERENCES "users"("id")')
97 expect(sql).toContain('ON DELETE CASCADE')
98 })
99
100 it('sqlite inline FK honours onUpdate when supplied', () => {
101 const plan: any = {
102 dialect: 'sqlite',
103 tables: [{
104 table: 'orders',
105 primaryKey: 'id',
106 columns: [
107 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
108 {
109 name: 'customer_id',
110 type: 'bigint',
111 isPrimaryKey: false,
112 isUnique: false,
113 isNullable: false,
114 hasDefault: false,
115 references: { table: 'customers', column: 'id', onDelete: 'restrict', onUpdate: 'cascade' },
116 },
117 ],
118 indexes: [],
119 }],
120 }
121 const sql = generateSql(plan).join('\n')
122
123 expect(sql).toContain('REFERENCES "customers"("id")')
124 expect(sql).toContain('ON DELETE RESTRICT')
125 expect(sql).toContain('ON UPDATE CASCADE')
126 })
127
128 it('columns without references do not get a stray REFERENCES clause', () => {
129 const basePlan: any = {
130 tables: [{
131 table: 'logs',
132 primaryKey: 'id',
133 columns: [
134 { name: 'id', type: 'bigint', isPrimaryKey: true, isUnique: false, isNullable: false, hasDefault: false },
135 { name: 'message', type: 'text', isPrimaryKey: false, isUnique: false, isNullable: true, hasDefault: false },
136 ],
137 indexes: [],
138 }],
139 }
140 for (const dialect of ['sqlite', 'mysql', 'postgres'] as const) {
141 const sql = generateSql({ ...basePlan, dialect }).join('\n')
142 expect(sql).not.toContain('REFERENCES')
143 }
144 })
145})
packages/bun-query-builder/test/migrations.diff.test.tsmodified+25-0
@@ -68,6 +68,31 @@ describe('migrations - diffing and hashing', () => {
6868 expect(sql.join('\n')).toContain('ALTER TABLE "projects" ADD CONSTRAINT')
6969 })
7070
71 it('sqlite diff emits inline REFERENCES for new tables (FK rides on CREATE)', () => {
72 const models2 = defineModels({
73 ...baseModels,
74 Project: {
75 name: 'Project',
76 table: 'projects',
77 primaryKey: 'id',
78 attributes: {
79 id: { validation: { rule: {} } },
80 user_id: { validation: { rule: {} } },
81 name: { validation: { rule: {} } },
82 },
83 },
84 } as const)
85 const p1 = buildMigrationPlan(baseModels as any, { dialect: 'sqlite' })
86 const p2 = buildMigrationPlan(models2 as any, { dialect: 'sqlite' })
87 const joined = generateDiffSql(p1, p2).join('\n')
88
89 expect(joined).toContain('CREATE TABLE')
90 expect(joined).toMatch(/"user_id"[^,)]*REFERENCES\s+"users"\("id"\)/)
91 // SQLite can't run ALTER TABLE ADD CONSTRAINT, so no separate
92 // ALTER must be emitted for new tables.
93 expect(joined).not.toContain('ADD CONSTRAINT')
94 })
95
7196 it('dialect specific types map as expected', () => {
7297 const planPg = buildMigrationPlan(baseModels as any, { dialect: 'postgres' })
7398 const sqlPg = generateSql(planPg)