fix(sqlite): preserve cascade dependents during table rebuilds - #6209
fix(sqlite): preserve cascade dependents during table rebuilds#6209ChrisMGeo wants to merge 1 commit into
Conversation
6dfcf2b to
350fc3d
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dfcf2bf67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| `INSERT OR REPLACE INTO ${ | ||
| quoteSQLiteIdentifier(dep.tableName) | ||
| } (${dependentColumns}) SELECT ${dependentColumns} FROM ${quoteSQLiteIdentifier(dep.backupTableName)};`, |
There was a problem hiding this comment.
When PRAGMA foreign_keys=OFF takes effect, as it does during the standard SQLite push flow that executes these statements individually, dropping the parent does not remove any dependent rows. This unconditional restore then re-inserts every backed-up row; a dependent table without a primary key or unique constraint gets duplicate rows, while constrained tables undergo unnecessary REPLACE operations and their associated side effects. Restore only rows that were actually cascaded, or keep enforcement enabled while using the backup strategy.
Useful? React with 👍 / 👎.
| : SQLiteSquasher.unsquashFK(fk); | ||
|
|
||
| if ( | ||
| data.tableTo !== current |
There was a problem hiding this comment.
When a table is renamed and also requires rebuilding in the same migration, rootTable uses the new name but fromSchema and its foreign keys still use the old name. The rename statement is emitted before the rebuild, yet this comparison finds no dependents, so in D1 or a transactional migrator where foreign-key enforcement remains enabled, dropping the renamed parent still cascades and permanently deletes its child rows.
Useful? React with 👍 / 👎.
| const newTable = toSchema.tables[table.name]; | ||
| if (!newTable) continue; | ||
| const oldTable = table; | ||
| const columns = Object.keys(oldTable.columns).filter((column) => newTable.columns[column]); |
There was a problem hiding this comment.
When a dependent column is renamed in the same diff as a parent rebuild, this name-based intersection omits that column even though the rename statement runs before the rebuild. After the parent drop cascades the row, restoration therefore writes NULL or a default instead of the original value, or fails entirely when the renamed column is NOT NULL without a default. The backup column mapping needs to account for resolved column renames rather than matching raw names only.
Useful? React with 👍 / 👎.
| const newTable = toSchema.tables[table.name]; | ||
| if (!newTable) continue; | ||
| const oldTable = table; | ||
| const columns = Object.keys(oldTable.columns).filter((column) => newTable.columns[column]); |
There was a problem hiding this comment.
When a cascade-dependent table has a stored or virtual generated column, this collects that column along with ordinary common columns, and the generated restore later names it explicitly in an INSERT OR REPLACE. SQLite rejects writes to generated columns with cannot INSERT into generated column, so rebuilding the parent fails instead of preserving its dependents. Generated columns should be omitted from the backup/restore column list and allowed to recompute.
Useful? React with 👍 / 👎.
| visited.add(table.name); | ||
| result.push({ | ||
| tableName: table.name, | ||
| backupTableName: getCascadeBackupTableName(rootTable, table.name), | ||
| columns, |
There was a problem hiding this comment.
For a fan-in cascade graph, marking a table visited and appending it on the first discovered path does not guarantee that all tables it references are restored first. For example, if the root cascades to a and b, c cascades from a, and x cascades from both b and c, the stack traversal can produce a, b, x, c; with enforcement enabled, restoring x fails immediately because c is still absent. The dependents need a foreign-key-safe topological restore order rather than first-discovery order.
Useful? React with 👍 / 👎.
| const newTable = toSchema.tables[table.name]; | ||
| if (!newTable) continue; |
There was a problem hiding this comment.
When a cascade-dependent table is renamed in the same migration as its parent is rebuilt, fromSchema supplies the old dependent name but toSchema is keyed by the new name, so this lookup skips the dependent entirely. Because table renames are emitted before rebuilds and SQLite retains the foreign key through the rename, dropping the parent then cascades rows from the newly named table without any backup. The collector must resolve the dependent rename and use its post-rename name in the generated backup statements.
Useful? React with 👍 / 👎.
350fc3d to
1edd995
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 participant
Fixes #5782. Successor to #5074.
Drizzle Kit emits SQLite table-rebuild migrations with
PRAGMA foreign_keys=OFF. That pragma is ineffective inside the ORM migrator transaction, and Cloudflare D1 does not allow disabling foreign-key enforcement. WithON DELETE CASCADE, dropping a rebuilt parent silently deletes dependent rows.This change preserves cascade dependents during generated SQLite and libSQL rebuilds by:
SELECT *, so a dependent table can also gain columns in the same schema diffThe regression coverage exercises generated migrations and push migrations with populated direct, multi-level, and fan-out cascade relationships, newly added dependents, and changed dependent columns.
Validation:
pnpm --filter drizzle-kit exec tsc -p tsconfig.build.json --noEmitpnpm lintRelated: #5074, #5784.