From f68af8dfd5978870b214d9b870c6de55f7ea2041 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 18 Sep 2026 06:31:51 +0000 Subject: [PATCH] Fix M7 clip-columns migration failing against a populated database MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit batch_alter_table's SQLite "recreate the table" strategy drops the original `asset` table, and SQLite enforces foreign keys on DROP TABLE too (an implicit "as if every row were deleted" check). Every connection here runs with PRAGMA foreign_keys=ON, so the drop was refused the moment any other table (assettag, suggestion) held a real row referencing an asset — which any populated database has and an empty one never does, so no existing test caught it. Disable the pragma for just this table rebuild, in both directions since downgrade() recreates asset too. Verified against a seeded SQLite db reproducing the exact production failure (FOREIGN KEY constraint failed on DROP TABLE asset), confirmed the fix resolves it with no data loss and an intact foreign_key_check, and added a regression test that seeds a real cross-reference before migrating so this can't silently regress. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01KzCKjg6yBtAwMFwZmezuw1 --- ...20260918_0900_add_clip_columns_to_asset.py | 13 +++++ backend/tests/test_migrations.py | 47 +++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py b/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py index 8addd50..12b3f75 100644 --- a/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py +++ b/backend/alembic/versions/20260918_0900_add_clip_columns_to_asset.py @@ -28,18 +28,31 @@ def upgrade() -> None: + # SQLite checks foreign keys on DROP TABLE too — an implicit "as if every row were + # deleted" pass — and app.database's connect listener runs every connection here + # with PRAGMA foreign_keys=ON. Batch mode's recreate-the-table strategy for adding + # a constraint needs to DROP `asset`, which real assettag/suggestion rows + # referencing it then block. A fresh test database never catches this: schema + # migrations run before any fixture has inserted a row, so there's nothing yet to + # violate. Migrations run under maintainer control, not the app-runtime mutations + # this pragma is meant to protect — safe to disable for just this rebuild. + op.execute('PRAGMA foreign_keys=OFF') with op.batch_alter_table('asset', schema=None) as batch_op: batch_op.add_column(sa.Column('parent_asset_id', sqlmodel.sql.sqltypes.AutoString(), nullable=True)) batch_op.add_column(sa.Column('in_point', sa.Float(), nullable=True)) batch_op.add_column(sa.Column('out_point', sa.Float(), nullable=True)) batch_op.create_index('ix_asset_parent_asset_id', ['parent_asset_id'], unique=False) batch_op.create_foreign_key('fk_asset_parent_asset_id_asset', 'asset', ['parent_asset_id'], ['id']) + op.execute('PRAGMA foreign_keys=ON') def downgrade() -> None: + # Same reasoning as upgrade() — this recreates `asset` too. + op.execute('PRAGMA foreign_keys=OFF') with op.batch_alter_table('asset', schema=None) as batch_op: batch_op.drop_constraint('fk_asset_parent_asset_id_asset', type_='foreignkey') batch_op.drop_index('ix_asset_parent_asset_id') batch_op.drop_column('out_point') batch_op.drop_column('in_point') batch_op.drop_column('parent_asset_id') + op.execute('PRAGMA foreign_keys=ON') diff --git a/backend/tests/test_migrations.py b/backend/tests/test_migrations.py index dda6b32..bcba722 100644 --- a/backend/tests/test_migrations.py +++ b/backend/tests/test_migrations.py @@ -166,3 +166,50 @@ def test_the_migrated_schema_enforces_case_insensitive_tag_names(alembic_config) "INSERT INTO tag (id, user_id, name, created_at)" " VALUES ('3', 'other', 'nato', '2026-01-01')" ) + + +def test_add_clip_columns_survives_real_foreign_key_references(alembic_config): + """The landmine this guards against was real, not theoretical (M7's first deploy). + + `56ac14e89a0c` -> `7d4b9c1a6f28` batch-recreates `asset` to add its own + self-referencing foreign key. SQLite enforces foreign keys on DROP TABLE too — an + implicit "as if every row were deleted" check — so recreating `asset` while + `PRAGMA foreign_keys=ON` (every connection here runs with it on, see + app.database's connect listener) fails the moment another table holds a row that + actually references one, which is exactly what `assettag` does in any populated + database. A fresh database never catches this: schema migrations always run before + any fixture inserts a row, so there is nothing yet to violate — which is exactly + how this shipped clean and broke on the first real deploy. This seeds a real + cross-reference first, the way production always has one, so the migration is + proven against the case that matters rather than the empty case every other test + in this file uses. + """ + config, db_path = alembic_config + command.upgrade(config, "56ac14e89a0c") + + with sqlite3.connect(db_path) as conn: + conn.execute("PRAGMA foreign_keys=ON") + conn.execute( + "INSERT INTO asset (id, user_id, name, asset_type, source, storage_key," + " size_bytes, field_provenance, upload_date, modified_date, metadata_modified_date)" + " VALUES ('a1', 'u', 'Real asset', 'video', 'local_upload', 'k1'," + " 100, '{}', '2026-01-01', '2026-01-01', '2026-01-01')" + ) + conn.execute( + "INSERT INTO tag (id, user_id, name, created_at)" + " VALUES ('t1', 'u', 'Tag', '2026-01-01')" + ) + conn.execute( + "INSERT INTO assettag (asset_id, tag_id, created_at) VALUES ('a1', 't1', '2026-01-01')" + ) + conn.commit() + + command.upgrade(config, "head") + + with sqlite3.connect(db_path) as conn: + assert conn.execute("SELECT id FROM asset WHERE id = 'a1'").fetchone() is not None + assert conn.execute( + "SELECT * FROM assettag WHERE asset_id = 'a1' AND tag_id = 't1'" + ).fetchone() is not None + conn.execute("PRAGMA foreign_keys=ON") + assert conn.execute("PRAGMA foreign_key_check").fetchall() == []