diff --git a/backend/app/api/routes/settings.py b/backend/app/api/routes/settings.py index b34f0bd30..be5e4468a 100644 --- a/backend/app/api/routes/settings.py +++ b/backend/app/api/routes/settings.py @@ -815,15 +815,8 @@ async def _import_sqlite_to_postgres(sqlite_path: Path, postgres_url: str): sorted_tables = [t.name for t in metadata.sorted_tables if t.name in tables_to_import] # Phase 1: Drop all tables and recreate WITHOUT foreign keys. - # This avoids all FK ordering/orphan issues during import. - saved_fks = {} - for table in metadata.sorted_tables: - fks = list(table.foreign_key_constraints) - if fks: - saved_fks[table.name] = fks - for fk in fks: - table.constraints.discard(fk) - + # This avoids all FK ordering/orphan issues during import; the + # constraints go back on at the end, once every row has landed. async with pg_engine.begin() as conn: # Cap how long DROP TABLE will wait for AccessExclusiveLock so # any residual concurrent writer (per-printer MQTT clients @@ -856,11 +849,38 @@ async def _import_sqlite_to_postgres(sqlite_path: Path, postgres_url: str): ) await conn.run_sync(metadata.create_all) - # Restore FK definitions in metadata (needed for re-adding later) - for table_name, fks in saved_fks.items(): - table_obj = metadata.tables[table_name] - for fk in fks: - table_obj.constraints.add(fk) + # Now strip the foreign keys, at the database level. + # + # This used to be done by discarding each ForeignKeyConstraint + # from `table.constraints` before `create_all`. That only + # suppresses the inline REFERENCES clause inside CREATE TABLE: + # `Table.foreign_key_constraints` is derived from the *columns'* + # ForeignKey objects, which the discard never touched. When + # `create_all` meets a dependency cycle it can't sort -- and + # library_files / library_folders / print_archives are exactly + # such a cycle -- it falls back to emitting those tables' keys + # as separate ALTER TABLE ... ADD FOREIGN KEY statements read + # straight from that property. Twelve constraints survived, + # including library_files.folder_id, and because the same cycle + # also drops the ordering edge from `sorted_tables` the child + # table was imported before its parent and the restore died on + # a ForeignKeyViolationError. + # + # Dropping them from pg_constraint instead is indifferent to how + # create_all chose to emit them, so a future model cycle cannot + # reintroduce this. It also keeps the app's global Base.metadata + # untouched: the old code only put the constraints back *after* + # the transaction, so a failure in here left the running process + # with an FK-less metadata until restart. + await conn.execute( + text( + "DO $$ DECLARE r RECORD; BEGIN " + "FOR r IN (SELECT conrelid::regclass AS tbl, conname FROM pg_constraint " + "WHERE contype = 'f' AND connamespace = 'public'::regnamespace) LOOP " + "EXECUTE 'ALTER TABLE ' || r.tbl || ' DROP CONSTRAINT ' || quote_ident(r.conname); " + "END LOOP; END $$;" + ) + ) # Phase 2: Import data (no FKs to worry about) async with pg_engine.begin() as conn: @@ -958,7 +978,7 @@ async def _import_sqlite_to_postgres(sqlite_path: Path, postgres_url: str): src.close() logger.info("Cross-database import complete: %d tables imported", len(tables_to_import)) - # Recreate FK constraints from ORM metadata (not from saved definitions). + # Recreate FK constraints from ORM metadata, which Phase 1 left intact. # Use individual transactions so orphaned SQLite data doesn't block valid FKs. from sqlalchemy.schema import AddConstraint @@ -968,11 +988,28 @@ async def _import_sqlite_to_postgres(sqlite_path: Path, postgres_url: str): try: async with pg_engine.begin() as fk_conn: await fk_conn.execute(AddConstraint(fk)) - except Exception: - failed_fks.append(f"{table.name}.{fk.name}") + except Exception as e: + # Name the constraint by what it links, not by `fk.name`: + # these are unnamed in the ORM, so that field is None and + # the warning used to read "print_archives.None" for every + # one of the five keys on that table -- unusable for + # working out which rows to go and look at. + cols = ", ".join(c.name for c in fk.columns) + target = fk.elements[0].target_fullname if fk.elements else "unknown" + failed_fks.append(f"{table.name}({cols}) -> {target}") + # Postgres puts the offending key in a DETAIL line; it + # names the exact orphan value, which is the one thing + # that turns this into an actionable report. + detail = next( + (ln.strip() for ln in str(e).splitlines() if ln.startswith("DETAIL:")), + str(e).splitlines()[0] if str(e) else e.__class__.__name__, + ) + logger.info("FK %s(%s) -> %s not restored: %s", table.name, cols, target, detail) if failed_fks: logger.warning( - "Could not restore %d FK constraints (orphaned data in SQLite): %s", + "Could not restore %d FK constraints (orphaned data in the backup): %s. " + "The data is restored and usable; those columns are simply no longer " + "enforced. See the INFO lines above for the offending key in each case.", len(failed_fks), ", ".join(failed_fks), ) diff --git a/backend/tests/unit/test_postgres_restore_drop_cascade.py b/backend/tests/unit/test_postgres_restore_drop_cascade.py index 516c91c15..7d7172b3d 100644 --- a/backend/tests/unit/test_postgres_restore_drop_cascade.py +++ b/backend/tests/unit/test_postgres_restore_drop_cascade.py @@ -15,10 +15,14 @@ so orphan tables can no longer block the restore. These tests guard against a regression to `metadata.drop_all` (which would re-introduce the bug for any user with orphan tables). + +The second half of the file covers the follow-on fix: the recreated +tables must carry no foreign keys at all while rows are being imported. """ from __future__ import annotations +import logging import sqlite3 import tempfile from pathlib import Path @@ -35,6 +39,9 @@ def _make_sqlite_source() -> Path: conn = sqlite3.connect(str(path)) # `users` is in the ORM metadata so `tables_to_import` is non-empty. conn.execute("CREATE TABLE users (id INTEGER PRIMARY KEY, username TEXT)") + # At least one row, so the import actually emits an INSERT -- the + # loop skips empty tables outright. + conn.execute("INSERT INTO users (id, username) VALUES (1, 'alice')") conn.commit() conn.close() return path @@ -161,3 +168,181 @@ async def test_restore_cascade_drop_targets_only_public_schema(): assert "schemaname = '*'" not in cascade finally: sqlite_path.unlink(missing_ok=True) + + +def _mock_pg_engine( + executed_sql: list[str], + create_all_error: Exception | None = None, + fk_error: Exception | None = None, +): + """Build a fake async engine that records every statement, plus a + `run_sync:` marker, into `executed_sql` in execution order. + + `fk_error` makes every ADD CONSTRAINT fail, standing in for a backup + carrying orphaned rows.""" + from sqlalchemy.schema import AddConstraint + + mock_conn = MagicMock() + + def _execute(stmt, *a, **k): + if fk_error is not None and isinstance(stmt, AddConstraint): + raise fk_error + executed_sql.append(getattr(stmt, "text", str(stmt))) + + mock_conn.execute = AsyncMock(side_effect=_execute) + + async def _run_sync(fn, *args, **kw): + executed_sql.append("run_sync:" + getattr(fn, "__name__", repr(fn))) + if create_all_error is not None: + raise create_all_error + return None + + mock_conn.run_sync = AsyncMock(side_effect=_run_sync) + + begin_cm = MagicMock() + begin_cm.__aenter__ = AsyncMock(return_value=mock_conn) + begin_cm.__aexit__ = AsyncMock(return_value=False) + + mock_engine = MagicMock() + mock_engine.begin = MagicMock(return_value=begin_cm) + mock_engine.dispose = AsyncMock() + return mock_engine + + +def _fk_names(table) -> set[str]: + return {id(fk) for fk in table.constraints if hasattr(fk, "elements")} + + +@pytest.mark.asyncio +async def test_restore_drops_every_foreign_key_before_importing_rows(): + """The recreated schema must carry no FK constraints while rows land. + + Regression (#restore FK violation): the fix used to discard each + ForeignKeyConstraint from `table.constraints` before `create_all`. + That only suppresses the inline REFERENCES clause -- when `create_all` + hits a dependency cycle it cannot sort (library_files / + library_folders / print_archives are exactly such a cycle) it emits + those tables' keys as separate ALTER TABLE ... ADD FOREIGN KEY + statements read from `Table.foreign_key_constraints`, which the + discard never touched. The child table then imported before its + parent and Postgres raised ForeignKeyViolationError on + `library_files_folder_id_fkey`.""" + from backend.app.api.routes import settings as settings_module + + sqlite_path = _make_sqlite_source() + try: + executed_sql: list[str] = [] + with patch( + "backend.app.core.database._create_engine", + new=MagicMock(return_value=_mock_pg_engine(executed_sql)), + ): + await settings_module._import_sqlite_to_postgres(sqlite_path, "postgresql+asyncpg://test/test") + + fk_drops = [i for i, s in enumerate(executed_sql) if "pg_constraint" in s and "DROP CONSTRAINT" in s] + assert fk_drops, ( + "Expected an unconditional DROP CONSTRAINT sweep over pg_constraint " + "so no foreign key survives create_all's cycle-breaking ALTER " + "TABLE statements. Captured SQL: " + "; ".join(s[:100] for s in executed_sql) + ) + drop_sql = executed_sql[fk_drops[0]] + # Foreign keys only ('f'), scoped to public -- not PK/unique/check, + # and not another application's schema on a shared Postgres. + assert "contype = 'f'" in drop_sql, drop_sql + assert "'public'::regnamespace" in drop_sql, drop_sql + + # It has to land after the tables exist and before the first row. + create_idx = executed_sql.index("run_sync:create_all") + insert_idx = next((i for i, s in enumerate(executed_sql) if s.startswith("INSERT INTO")), -1) + assert insert_idx > 0, f"no row import happened, so the ordering is untested: {executed_sql}" + assert create_idx < fk_drops[0] < insert_idx, ( + f"FK drop must sit between create_all and the first INSERT: {executed_sql}" + ) + finally: + sqlite_path.unlink(missing_ok=True) + + +@pytest.mark.asyncio +@pytest.mark.parametrize("create_all_fails", [False, True]) +async def test_restore_never_mutates_the_process_wide_orm_metadata(create_all_fails): + """`Base.metadata` is global to the running app. The old code removed + every FK from it and only put them back *after* the drop/create + transaction, so a failure in there left the live process unable to + emit or re-add foreign keys until restart.""" + from backend.app.api.routes import settings as settings_module + from backend.app.core.database import Base + + table = Base.metadata.tables["library_files"] + before = _fk_names(table) + assert before, "library_files should carry FK constraints to begin with" + + sqlite_path = _make_sqlite_source() + try: + executed_sql: list[str] = [] + boom = RuntimeError("create_all exploded") if create_all_fails else None + engine = _mock_pg_engine(executed_sql, create_all_error=boom) + with patch("backend.app.core.database._create_engine", new=MagicMock(return_value=engine)): + if create_all_fails: + with pytest.raises(RuntimeError, match="create_all exploded"): + await settings_module._import_sqlite_to_postgres(sqlite_path, "postgresql+asyncpg://test/test") + else: + await settings_module._import_sqlite_to_postgres(sqlite_path, "postgresql+asyncpg://test/test") + + assert _fk_names(table) == before, "The restore must not add or remove constraints on the shared ORM metadata" + finally: + sqlite_path.unlink(missing_ok=True) + + +@pytest.mark.asyncio +async def test_unrestorable_fk_is_reported_by_its_columns(caplog): + """A key that can't go back on must be named by what it links. + + These constraints are unnamed in the ORM, so `fk.name` is None and the + warning used to read "print_archives.None" once per failure -- five of + that table's keys share it, so the report said nothing about which + columns to inspect.""" + from backend.app.api.routes import settings as settings_module + + orphan = RuntimeError( + 'violates foreign key constraint "library_files_folder_id_fkey"\n' + 'DETAIL: Key (folder_id)=(9) is not present in table "library_folders".' + ) + sqlite_path = _make_sqlite_source() + try: + executed_sql: list[str] = [] + engine = _mock_pg_engine(executed_sql, fk_error=orphan) + with ( + patch("backend.app.core.database._create_engine", new=MagicMock(return_value=engine)), + caplog.at_level(logging.INFO, logger="backend.app.api.routes.settings"), + ): + await settings_module._import_sqlite_to_postgres(sqlite_path, "postgresql+asyncpg://test/test") + + warning = next((r.getMessage() for r in caplog.records if r.levelno == logging.WARNING), None) + assert warning is not None, "a failed FK restore must be reported" + assert ".None" not in warning, f"constraints must not be named by fk.name: {warning}" + assert "library_files(folder_id) -> library_folders.id" in warning, warning + # And the offending value is recorded so the rows can be found. + assert any("Key (folder_id)=(9)" in r.getMessage() for r in caplog.records), ( + "the Postgres DETAIL line names the orphan; it must survive into the log" + ) + finally: + sqlite_path.unlink(missing_ok=True) + + +def test_library_tables_form_an_fk_cycle(): + """Documents why the restore cannot import in dependency order. + + library_files -> library_folders -> print_archives -> library_files. + SQLAlchemy's `sorted_tables` gives up on these three and falls back to + alphabetical, which puts the child (library_files) before its parent. + Dropping the constraints outright is the only ordering-independent + answer; if this cycle is ever broken, the restore still works, but the + comment in `_import_sqlite_to_postgres` should be revisited.""" + from backend.app.core.database import Base + + def refs(name: str) -> set[str]: + table = Base.metadata.tables[name] + return {fk.column.table.name for fk in table.foreign_keys} + + assert "library_folders" in refs("library_files") + assert "print_archives" in refs("library_folders") + assert "library_files" in refs("print_archives")