fix(sqlite): prevent silent ON DELETE CASCADE data loss during table-rebuild migrations#5784
fix(sqlite): prevent silent ON DELETE CASCADE data loss during table-rebuild migrations#5784Zelys-DFKH wants to merge 1 commit into
Conversation
|
The hoist+session-pragma shape is the right fix. Two design choices worth surfacing for reviewers:
One observation, not blocking: Verified locally: with the fix, both child rows survive the rebuild; reverted, both are wiped silently. The severity here versus #1813 and #4089 is that there's no error and no rollback. |
… CASCADE data loss SQLite ignores PRAGMA foreign_keys inside a transaction. Drizzle-kit table-rebuild migrations emit this pragma at the top, but the migrator wraps everything in BEGIN...COMMIT, making it a no-op. With ON DELETE CASCADE, the DROP TABLE inside the rebuild silently wipes child rows and the migration records as applied with no error. Fix: scan pending migrations for the PRAGMA foreign_keys=OFF signature before BEGIN. If found, hoist it to session level (where SQLite honors it), skip pragma statements inside the transaction loop, and restore the previous FK state in a finally block. Covers both SQLiteSyncDialect and SQLiteAsyncDialect. Fixes drizzle-team#5782 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
25df139 to
606405e
Compare
|
Thanks for verifying this locally and spelling out the design choices. Good catch on the regex coupling. I've added a note above the definitions tying them to drizzle-kit's rebuild format. Pushing the amend now. |
|
The hoist+session-pragma shape is the right fix. Empirically verified against the dist with the dialect patch applied on top: before the fix the child rows go from 2 to 0 silently, after the fix they survive. Two design choices in the patch worth surfacing for reviewers:
One observation, not blocking: One test bug to fix before CI runs: the added test at The severity here versus #1813 and #4089 is that there's no error and no rollback. |
Fixes #5782
Credit to @truffle-dev for the root-cause analysis in the thread.
Every table-rebuild migration from drizzle-kit starts with
PRAGMA foreign_keys=OFF— correct, since the rename dance needs cascades disarmed. But SQLite documents that this pragma is a no-op inside a transaction: "foreign key constraint enforcement may only be enabled or disabled when there is no pending BEGIN or SAVEPOINT." The migrator wraps each migration inBEGIN … COMMIT. So the pragma drizzle-kit generates to protect data is neutralized by the very transaction that protects atomicity.With
ON DELETE CASCADE: theDROP TABLEinside the rebuild fires the cascade, child rows are deleted, the migration commits, and__drizzle_migrationsrecords it as applied. No error. No rollback. Users find out when their tables are empty.This is the silent variant of #1813 and #4089. Those throw
SQLITE_CONSTRAINT, roll back, give the user an error message. This one succeeds.Fix
Before
BEGIN, scan pending migrations forPRAGMA foreign_keys=OFF(the drizzle-kit rebuild signature). If found, read the current FK state, issue the pragma at session level where SQLite actually honors it, skipPRAGMA foreign_keysstatements inside the transaction loop (they'd be no-ops anyway), and restore in afinallyblock. The migrator saves FK state before the hoist and restores it conditionally. Always restoring ON would change the caller's connection state if they never had FK enforcement enabled, since SQLite defaults to OFF.Both
SQLiteSyncDialectandSQLiteAsyncDialectpaths are covered. All SQLite drivers flow through these two methods.db.session.run()anddb.session.transaction()can land on different connections, so the session-level pragma may not carry over. Those drivers don't supportPRAGMA foreign_keysanyway, soneedsFkHoiststays false.Test
Test in
integration-tests/tests/sqlite/better-sqlite.test.tsseeds a parent + child schema withON DELETE CASCADE, runs a table-rebuild migration in the exact drizzle-kit output shape (PRAGMA foreign_keys=OFF;→ create__new_*→ INSERT SELECT → DROP → RENAME →PRAGMA foreign_keys=ON;), and asserts the child rows survive.