ReviewOS

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
9 days ago

Closes stacksjs/bun-query-builder#1154.

The reference was dropped on the way to the column

#1019 made CREATE TABLE inline REFERENCES, which is the only form SQLite accepts — it has no ALTER TABLE … ADD CONSTRAINT, and SQLiteDriver.addForeignKey returns '' so the orchestrator skips that pass. The diff path that adds a column to an existing table never learned the same trick: addColumn rendered the type, nullability and default, and nothing else carried the reference.

So a belongsTo added to a table that already existed produced

ALTER TABLE "monitors" ADD COLUMN "server_id" INTEGER;

and no foreign key at all, on a migration that reported success. The issue reports five FKs lost that way in a StatusHQ schema.

addColumn now renders the same clause renderColumn does, through a shared referencesClause:

ALTER TABLE "monitors" ADD COLUMN "server_id" INTEGER REFERENCES "servers"("id");

ON DELETE / ON UPDATE come with it.

The one shape SQLite refuses

ADD COLUMN … REFERENCES … is rejected when the new column's default is not NULL, once foreign keys are on and the table has rows:

Cannot add a REFERENCES column with non-NULL default value

Inlining unconditionally would have turned a silently-missing FK into a failed migration for that case, so the diff engine now counts an added column with a reference and a non-NULL default as needing a rebuild, alongside the modified-column and FK-change cases it already rebuilds for. The rebuild applies the FK through CREATE TABLE, where any default is fine, and carries the existing rows.

A NULL default (or none) still takes the cheap ADD COLUMN path.

Tests

test/migrations.sqlite-add-column-fk.test.ts — the emitted SQL, and what the database does with it: the inline reference, ON DELETE/ON UPDATE, applying the diff to a live SQLite schema with PRAGMA foreign_keys = ON and an existing row (the FK shows up in PRAGMA foreign_key_list, a bad id is rejected, the row survives), the rebuild path for a non-NULL default, and the two cases that must not change — a NULL default, and a column with no reference. 5 of the 6 fail against main.

The issue's own repro now prints the REFERENCES clause and a populated PRAGMA foreign_key_list.

Locally: full suite 2246 pass / 0 fail / 10 skip, tsc clean, pickier . at its 330-warning baseline.

Not addressed here: #1037, changing or adding an FK on a column that already exists. That still needs the rebuild route the issue describes.

🤖 Generated with Claude Code

3 changed files on the files tab, with 0 review threads.

Sign in to comment on this pull request.