mirror of
https://github.com/maziggy/bambuddy.git
synced 2026-09-30 11:12:35 +02:00
Drop the restore's foreign keys in the database, not in the ORM metadata
Restoring a SQLite backup into PostgreSQL died part-way with insert or update on table "library_files" violates foreign key constraint "library_files_folder_id_fkey" DETAIL: Key (folder_id)=(1) is not present in table "library_folders". The import recreates the schema and is supposed to create every table without foreign keys, so the order rows arrive in cannot matter; the constraints are added back once the data has landed. Phase 1 did that by discarding each ForeignKeyConstraint from table.constraints before create_all -- which only suppresses the inline REFERENCES clause. Table.foreign_key_constraints is derived from the columns' ForeignKey objects and was never touched, and when create_all meets a dependency cycle it cannot sort, it falls back to emitting those tables' keys as separate ALTER TABLE ... ADD FOREIGN KEY statements read from exactly that property. library_files, library_folders and print_archives form such a cycle, so twelve constraints survived across the three of them -- measured against a real PostgreSQL by running the old phase verbatim. The same cycle also costs those tables their place in sorted_tables, so they were imported alphabetically, putting library_files ahead of the library_folders rows its folder_id references. Phase 1 now creates the tables normally and drops every foreign key from pg_constraint afterwards, in the same transaction, scoped to contype 'f' in the public schema. That is indifferent to how create_all chose to emit them, so a future cycle between other tables cannot bring this back. Phase 3 is unchanged. This also removes a second fault: the keys were stripped from the process-wide Base.metadata and only restored after the drop/create transaction, so a failure in between left the running app without them until restart. The metadata is no longer modified at all. Verified end to end against a real PostgreSQL -- a backup whose child rows import before their parents restores cleanly, with all 90 constraints back afterwards. Four regression tests added.
This commit is contained in:
@@ -27,6 +27,7 @@ All notable changes to Bambuddy will be documented in this file.
|
||||
- **Error and warning toasts now stay up twice as long** — Every pop-up notification disappeared after three seconds regardless of what it said. That is about right for "Settings saved", which confirms something you just did and is skimmed rather than read, but errors and warnings are a different kind of message: they carry a reason, often one relayed from the printer or the backend, and they run to a couple of lines. Three seconds was not long enough to finish reading one, and a missed error message is gone for good — there is no notification history to go back to. Errors and warnings now hold for six seconds. Success and informational toasts keep the three-second default, so the common case of clicking something and seeing it confirmed is unchanged, and the close button and the manual dismiss work exactly as before on all of them. The background print-dispatch toast is unaffected: it stays up while it has work in progress and clears itself shortly after the last job settles. Covered by frontend tests.
|
||||
|
||||
### Fixed
|
||||
- **Restoring a SQLite backup into PostgreSQL failed part-way with a foreign-key violation on the file library** — The restore rebuilds the schema and then imports every table, and it is meant to create those tables carrying no foreign keys at all, so that the order rows arrive in cannot matter; the constraints go back on once everything has landed. It did that by taking the keys off the application's own schema description before creating the tables, which only suppresses the `REFERENCES` clause written inside each `CREATE TABLE`. SQLAlchemy keeps a second view of a table's keys, derived from its columns, and that one was untouched -- so whenever it met a group of tables whose dependencies form a loop and could not put them in order, it fell back to adding their keys afterwards with `ALTER TABLE`, reading them from exactly the view that still had them. `library_files`, `library_folders` and `print_archives` all point at one another, so twelve constraints came back across those three tables. The same loop also costs them their place in the import order, so they were imported alphabetically instead -- which puts `library_files` ahead of the `library_folders` rows its `folder_id` refers to, and PostgreSQL refused the very first batch. The restore now drops the foreign keys in the database itself, after the tables exist, instead of trying to stop them being written: it no longer matters how a constraint came to be there, and a future loop between other tables cannot bring the problem back. One more fault went with it -- the keys were stripped from a schema description shared by the whole running process and only restored after the rebuild had finished, so a failure in between left the process without them until it was restarted. Verified end to end against a real PostgreSQL: a backup whose child rows import before their parents now restores cleanly, with all ninety constraints in place afterwards. Separately, the warning about keys that genuinely cannot be put back -- because the backup itself holds rows pointing at something that no longer exists -- now names them by the columns they link, such as `print_archives(library_file_id) -> library_files.id`. These constraints have no name of their own, so the warning used to print `print_archives.None` once for each of that table's five keys, which said nothing about where to look; the offending value PostgreSQL reports is logged alongside it. The restore itself is unaffected, and so is the data -- those columns are simply left unenforced. Covered by backend tests.
|
||||
- **The slice dialog showed a guessed filament list for projects saved by a newer Bambu Studio than the slicer sidecar** — Opening the slice dialog on an unsliced project runs a quick preview slice, purely to ask the slicer which AMS slots the chosen plate actually consumes. Bambu Studio 2.8 writes a machine G-code template containing `{if timelapse_inline_photo}` but does not export a definition for that variable, so the template is unresolvable the moment it leaves Studio: a sidecar running an older build stops with a placeholder parse error before producing any slice data. The preview then returned nothing and the dialog quietly fell back to guessing from the file's painted faces, with no indication the numbers were an estimate. On the H2D project this was found with, the guess dropped a whole slot -- the support material -- from a four-filament plate. Bambuddy now retries the preview once with just that one unparsable template emptied, leaving every other setting in the file untouched, which is what keeps the answer honest: the process settings, support configuration and per-slot filament assignments are all still the project's own, so the filament list and its gram figures match what the file would really print. Only templates that cannot extrude are ever emptied -- a start or filament-change template lays a prime line or purges, so silencing one would change the very grams the preview reports, and Bambuddy would rather return nothing than a confident wrong number. Verified against a real H2D slice: the retry reproduces the full four-slot list, gram for gram. Covered by backend tests.
|
||||
- **Cancelling or deleting a queued item did not stop the preheat already running for it (#2727, contributed by @ticfinack)** — The cancel and delete routes write the item's new status to the database, and nothing else. A dispatch that had already begun preheating was parked in a sleep waiting for the chamber to reach temperature, where it could not see that write — so the heaters kept running out the rest of the wait and soak for a print that was not going to happen, twenty minutes at the default settings and longer if those have been raised, and the printer stayed marked busy the whole time, holding up every other job queued behind it. Those routes now tell the scheduler directly and the waits are taken in slices, so a cancelled preheat is abandoned within seconds and the heaters are switched off on the way out. The same unwinding now covers every other way a dispatch can end without starting a print — a failed upload, an error mid-dispatch, a plate whose file has gone missing — each of which used to leave the bed and chamber heating with nothing to turn them off. What preheat set is recorded and reversed, and a bed the printer reports at some other target is left alone rather than switched off, on the grounds that it belongs to whoever set it. A cancellation that lands after the upload has begun still starts the print, as it always has. Covered by backend tests.
|
||||
- **STEP files were offered for server-side slicing, which cannot work** — The **Slice** action appeared on `.step` / `.stp` files and the backend accepted the job, but neither slicer can load one from its command line: OrcaSlicer 2.4.2 and Bambu Studio 02.07.01.62 both answer `Unknown file format. Input file must have .stl, .obj, .amf(.xml) extension.` So the file was read, converted and uploaded, and the failure came back as "The input model file to the slicer can not be parsed" — which reads as a corrupt model rather than an unsupported format. The Slice button and the pipeline action no longer appear on STEP files, and the endpoint refuses one up front with a message that says to export it as STL or 3MF first. **Open in Slicer** is unchanged and still hands STEP to the desktop application, which opens it perfectly well — that was always the working path for these files.
|
||||
|
||||
@@ -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),
|
||||
)
|
||||
|
||||
@@ -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:<fn>` 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")
|
||||
|
||||
Reference in New Issue
Block a user