Ver código fonte

fix(db): add the foreign keys older PostgreSQL databases are missing

PostgreSQL databases created before v0.2.4.1 (v1.2.5.2 for the last one)
have no foreign key on print_log_entries.archive_id, print_log_entries.
created_by_id and print_archives.library_file_id: the migrations added the
columns as plain integers. Deleting an archive, user or library file left
its id behind instead of clearing it. A new migration clears ids that point
at deleted rows and adds the constraints, as a fresh install has them.

The #1390 backfill that links log entries to an archive by print name now
runs once. It ran on every start, so a run without an archive, or whose
archive was deleted, got attached to whichever archive shared its name.

The PostgreSQL upgrade check gets data scenarios: a database built along a
fixed chain of releases, seeded with rows, upgraded twice and queried. The
first one seeds orphaned and valid references. The three constraint lines
leave postgres_schema_drift.txt.
maziggy 1 semana atrás
pai
commit
481d43638d

+ 1 - 0
CHANGELOG.md

@@ -161,6 +161,7 @@ All notable changes to Bambuddy will be documented in this file.
 - **The frontend build no longer warns about `path` and `crypto` being externalized for the STEP previewer (#2976)** — `occt-import-js`, the Emscripten build behind STEP previews, requires both modules, but only inside its `ENVIRONMENT_IS_NODE` branches; in the browser it loads its `.wasm` from the URL the preview worker passes and draws randomness from `crypto.getRandomValues`. Vite still externalized both and printed two warnings on every build. `vite.config.ts` now drops exactly those two warnings for that one package through `build.rolldownOptions.onLog`, so an externalization anywhere else, or of any other module, still shows.
 
 ### Fixed
+- **PostgreSQL databases from older versions get the links a new install has** — A database created before v0.2.4.1 (or before v1.2.5.2 for the last one) was missing three links: from a print log entry to its archive and to the user who started it, and from an archive to the library file it was printed from. Deleting an archive, a user or a library file left their ID behind instead of clearing it, so such entries didn't show up under "no user", for example. The links are added on the next start, and IDs of things already deleted are cleared. A print log entry without an archive also stays that way now: every start used to attach it to the newest archive with the same name on the same printer, which could be someone else's. Apart from that, SQLite installs are unchanged.
 - **Every install now gets the dependency versions that were tested** — Bambuddy's Python dependencies only had minimum versions, so each Docker build, install and update took whatever was newest that day: a fresh install could run library versions nobody had tested Bambuddy with (SQLAlchemy 2.1 arrived that way). They are now pinned in `requirements.lock`, which Docker, the installers, the update scripts and the tests all use, and newer versions are taken on deliberately, after testing. Nothing changes for an install today: the pins are the versions a fresh install gets now. A native install moves to the pinned versions with the update after the one that brings this change, since an update still runs the previous version of the update script.
 - **Native installs need Python 3.11 or newer** — Since v0.2.4.8 Bambuddy hasn't started on Python 3.10, while the installer, README and wiki still said 3.10 was enough. The requirement now says 3.11. The installer uses the system's `python3` when it is new enough and otherwise looks for a newer Python next to it: on Ubuntu 22.04, whose `python3` is 3.10, it installs and uses the `python3.11` package. An environment left from an older Python is rebuilt. The update scripts check the install's Python first and, if it is too old, stop before changing anything and link to the steps for moving it to 3.11. Docker and the Windows installer ship Python 3.13 and are not affected. Wiki updated.
 - **The webhook endpoints listed in Settings didn't exist** — **Settings → API Keys → Webhook Endpoints** listed endpoints for the status of all printers, pausing and resuming that Bambuddy doesn't have, and two others under the wrong path. It now lists the real ones: add to the queue, start the next queued job, stop, cancel, printer status and queue status. Wiki updated.

+ 5 - 0
CONTRIBUTING.md

@@ -390,6 +390,11 @@ add. So a new column, constraint or index needs a migration that builds it
 exactly as the model does, including nullability and default; the check fails
 when an upgraded database ends up different.
 
+A migration that only fails with data in the database, such as a constraint
+that existing rows violate, gets a scenario in `SCENARIOS` in the same script:
+a fixed chain of old releases to build the database, rows to add, and a query
+that must find nothing wrong after the upgrade.
+
 `scripts/postgres_schema_drift.txt` lists the drift that already existed when
 the check was introduced, each line an object and exactly how it differs. A
 listed object that starts to differ in another way fails like new drift. The

+ 70 - 7
backend/app/core/database.py

@@ -4681,11 +4681,20 @@ async def _run_migrations(conn):
     # Step 1: link old log entries to their archive via print_name + printer_id.
     # Picks the highest-id matching archive when multiple share the same key
     # (newest archive wins — closest to the log's overwrite-then-leave shape).
+    # One-shot: since #1378 a NULL archive_id is a real answer (no archive, or
+    # its archive was deleted: ON DELETE SET NULL), and running this on every
+    # start linked such runs to whichever archive shares the name.
     from sqlalchemy import text as _text
 
+    link_flag = "_backfill_1390_log_archive_link_done"
     async with conn.begin_nested():
-        await conn.execute(
-            _text("""
+        linked = (
+            await conn.execute(_text('SELECT value FROM settings WHERE "key" = :k'), {"k": link_flag})
+        ).scalar_one_or_none()
+    if linked is None:
+        async with conn.begin_nested():
+            await conn.execute(
+                _text("""
             UPDATE print_log_entries
             SET archive_id = (
                 SELECT a.id
@@ -4700,7 +4709,10 @@ async def _run_migrations(conn):
             )
             WHERE archive_id IS NULL AND print_name IS NOT NULL
             """)
-        )
+            )
+            await conn.execute(
+                _text('INSERT INTO settings ("key", value) VALUES (:k, :v)'), {"k": link_flag, "v": "true"}
+            )
 
     # Step 2: backfill cost / energy_kwh / energy_cost onto the latest linked
     # log entry per archive — the row whose creation time best matches the
@@ -5062,10 +5074,8 @@ async def _run_migrations(conn):
 
     # Migration: per-file print progress inside a project (#1897).
     # - print_archives.library_file_id: which library file a queued run was
-    #   dispatched from; nullable, no FK constraint added to existing tables
-    #   (SQLite can't ADD CONSTRAINT; the application uses SET NULL semantics
-    #   via the ORM on fresh installs and tolerates dangling ids by matching
-    #   hash/filename as fallback anyway).
+    #   dispatched from; nullable. Added without its foreign key (SQLite can't
+    #   ADD CONSTRAINT); on PostgreSQL _migrate_add_missing_set_null_fks adds it.
     # - projects.target_sets: optional copies-per-file target. INTEGER is
     #   spelled identically on SQLite and Postgres — no dialect branch.
     await _safe_execute(conn, "ALTER TABLE print_archives ADD COLUMN library_file_id INTEGER")
@@ -5412,6 +5422,59 @@ async def _run_migrations(conn):
     await _safe_execute(conn, "ALTER TABLE print_archives ADD COLUMN timelapse_plate_id INTEGER")
 
     await _migrate_auth_datetimes_to_timestamp(conn)
+    await _migrate_add_missing_set_null_fks(conn)
+
+
+# Foreign keys that the migrations adding these columns left out, so a
+# PostgreSQL database older than the column has none: (table, column, parent).
+# All ON DELETE SET NULL, as the models declare them.
+_MISSING_SET_NULL_FKS = (
+    ("print_log_entries", "archive_id", "print_archives"),
+    ("print_log_entries", "created_by_id", "users"),
+    ("print_archives", "library_file_id", "library_files"),
+)
+
+
+async def _migrate_add_missing_set_null_fks(conn) -> None:
+    """Add the foreign keys in _MISSING_SET_NULL_FKS where they are missing.
+
+    PostgreSQL only; SQLite can't add a constraint to a table and doesn't
+    enforce foreign keys anyway (PRAGMA foreign_keys is off). A column that
+    already has a foreign key to the parent table, whatever its name, is left
+    alone. Ids left behind by deletes without the constraint are cleared first,
+    which is what ON DELETE SET NULL would have done, and without which the
+    constraint can't be added.
+    """
+    from sqlalchemy import text
+
+    if is_sqlite():
+        return
+    for table, column, parent in _MISSING_SET_NULL_FKS:
+        result = await conn.execute(
+            text(
+                "SELECT 1 FROM pg_constraint c "
+                "JOIN pg_attribute a ON a.attrelid = c.conrelid AND a.attnum = ANY (c.conkey) "
+                "WHERE c.contype = 'f' AND c.conrelid = CAST(:t AS regclass) "
+                "AND c.confrelid = CAST(:p AS regclass) "
+                "AND array_length(c.conkey, 1) = 1 AND a.attname = :c"
+            ),
+            {"t": table, "p": parent, "c": column},
+        )
+        if result.first():
+            continue
+        cleared = await conn.execute(
+            text(
+                f"UPDATE {table} SET {column} = NULL WHERE {column} IS NOT NULL "
+                f"AND NOT EXISTS (SELECT 1 FROM {parent} WHERE {parent}.id = {table}.{column})"
+            )
+        )
+        if cleared.rowcount:
+            logger.info("Cleared %d %s.%s id(s) of deleted %s rows", cleared.rowcount, table, column, parent)
+        await _safe_execute(
+            conn,
+            f"ALTER TABLE {table} ADD CONSTRAINT {table}_{column}_fkey "
+            f"FOREIGN KEY ({column}) REFERENCES {parent}(id) ON DELETE SET NULL",
+        )
 
 
 # Columns that used to be declared DateTime(timezone=True). Every bound datetime

+ 23 - 0
backend/tests/unit/test_print_log_backfill_migration.py

@@ -245,3 +245,26 @@ async def test_backfill_skips_archives_with_any_costed_run(engine_with_legacy_da
     # there too, so the backfill UPDATE would set NULL → NULL anyway, which
     # is the desired no-op.
     assert dict(rows) == {1: 1.11, 2: None, 3: None, 4: None}
+
+
+async def test_linking_runs_once_so_a_later_null_archive_id_stays_null(engine_with_legacy_data):
+    """Since #1378 a NULL archive_id is a real answer: the run had no archive,
+    or its archive was deleted (ON DELETE SET NULL). A later start must not
+    link such a run to whichever archive shares its name."""
+    async with engine_with_legacy_data.begin() as conn:
+        await run_migrations(conn)
+        await conn.execute(
+            text(
+                "INSERT INTO print_log_entries (id, print_name, printer_id, status, created_at) "
+                "VALUES (5, 'cube.3mf', 1, 'completed', CURRENT_TIMESTAMP)"
+            )
+        )
+        await conn.execute(text("UPDATE print_log_entries SET archive_id = NULL WHERE id = 4"))
+    async with engine_with_legacy_data.begin() as conn:
+        await run_migrations(conn)
+
+    async with engine_with_legacy_data.connect() as conn:
+        result = await conn.execute(text("SELECT id, archive_id FROM print_log_entries ORDER BY id"))
+        rows = result.all()
+
+    assert rows == [(1, 1), (2, 1), (3, 1), (4, None), (5, None)]

+ 99 - 2
scripts/check_postgres_upgrade.py

@@ -13,6 +13,9 @@ many of each constraint and index there are. Constraints and indexes are
 compared by definition, so only a name differing is not drift. A restart must
 leave every schema, fresh or upgraded, exactly as the first start left it.
 
+Some upgrades only go wrong with data in the database; SCENARIOS builds those
+along a fixed chain of releases, adds the rows, upgrades and checks the data.
+
 Drift that existed when this check was introduced is listed in
 scripts/postgres_schema_drift.txt, each line the object and exactly how it
 differs. Drift not listed there fails, including a listed object that now
@@ -36,6 +39,7 @@ import sys
 import tarfile
 import tempfile
 from collections import Counter
+from dataclasses import dataclass
 from pathlib import Path
 from urllib.parse import urlsplit, urlunsplit
 
@@ -44,6 +48,66 @@ ALLOWLIST = REPO / "scripts" / "postgres_schema_drift.txt"
 # The first release with PostgreSQL support: the oldest schema an install can have.
 FIRST_PG_RELEASE = "v0.2.3"
 
+
+@dataclass(frozen=True)
+class Scenario:
+    """An upgrade that needs data to go wrong.
+
+    The database is built along ``chain`` (old releases, pinned: a newer one may
+    no longer produce the state this needs), then ``seed`` runs and the current
+    code upgrades it twice. ``check`` must then return no rows; each row it
+    returns is a problem.
+    """
+
+    name: str
+    chain: tuple[str, ...]
+    seed: str
+    check: str
+
+
+SCENARIOS = (
+    Scenario(
+        name="orphaned references",
+        # v1.2.5.7 adds these columns to a v0.2.3 database without their foreign
+        # keys, so deleting an archive, user or library file left ids behind.
+        # Run 2 is such a leftover; its print_name matches archive 1, which an
+        # every-start backfill that fills in a NULL archive_id would link it to.
+        chain=("v0.2.3", "v1.2.5.7"),
+        seed="""
+            INSERT INTO users (id, username, role, is_active, auth_source) VALUES (1, 'kept', 'user', true, 'local');
+            INSERT INTO library_files (id, filename, file_path, file_type, file_size, print_count, is_external)
+            VALUES (1, 'kept.3mf', 'l/kept.3mf', '3mf', 1, 0, false);
+            INSERT INTO print_archives (id, filename, file_path, file_size, status, quantity, is_favorite,
+                                        print_name, library_file_id)
+            VALUES (1, 'kept.3mf', 'a/kept.3mf', 1, 'completed', 1, false, 'Benchy', 1),
+                   (2, 'gone.3mf', 'a/gone.3mf', 1, 'completed', 1, false, 'Other', 999999);
+            INSERT INTO print_log_entries (id, status, print_name, archive_id, created_by_id)
+            VALUES (1, 'completed', 'Benchy', 1, 1),
+                   (2, 'completed', 'Benchy', 999999, 999999);
+        """,
+        check="""
+            SELECT 'print_archives.library_file_id still points at a missing file'
+            FROM print_archives WHERE library_file_id IS NOT NULL
+                AND NOT EXISTS (SELECT 1 FROM library_files WHERE id = library_file_id)
+            UNION ALL
+            SELECT 'print_log_entries.archive_id still points at a missing archive'
+            FROM print_log_entries WHERE archive_id IS NOT NULL
+                AND NOT EXISTS (SELECT 1 FROM print_archives WHERE id = archive_id)
+            UNION ALL
+            SELECT 'print_log_entries.created_by_id still points at a missing user'
+            FROM print_log_entries WHERE created_by_id IS NOT NULL
+                AND NOT EXISTS (SELECT 1 FROM users WHERE id = created_by_id)
+            UNION ALL
+            SELECT 'the run of a deleted archive was linked to archive ' || archive_id
+            FROM print_log_entries WHERE id = 2 AND archive_id IS NOT NULL
+            UNION ALL
+            SELECT 'a valid reference was changed'
+            WHERE NOT EXISTS (SELECT 1 FROM print_log_entries WHERE id = 1 AND archive_id = 1 AND created_by_id = 1)
+                OR NOT EXISTS (SELECT 1 FROM print_archives WHERE id = 1 AND library_file_id = 1)
+        """,
+    ),
+)
+
 _INIT_DB = """
 import asyncio
 from backend.app.core import database
@@ -65,10 +129,12 @@ def newest_stable_release() -> str:
 
 def extract_release(tag: str, into: Path) -> Path:
     """The release's backend/ as a directory to run init_db from (git archive, read-only)."""
+    target = into / tag
+    if target.exists():
+        return target
     archive = into / f"{tag}.tar"
     with archive.open("wb") as out:
         subprocess.run(["git", "archive", tag, "backend"], cwd=REPO, check=True, stdout=out)
-    target = into / tag
     with tarfile.open(archive) as tar:
         tar.extractall(target, filter="data")
     return target
@@ -176,6 +242,27 @@ def start_twice(url: str, label: str) -> tuple[dict[str, str], list[str]]:
     return second, [f"{label}: a restart changed the schema: {line}" for line in sorted(changed)]
 
 
+def run_scenario(scenario: Scenario, url: str, releases_dir: Path) -> list[str]:
+    """Build, seed and upgrade the scenario's database; its problems."""
+    for release in scenario.chain:
+        run_init_db(extract_release(release, releases_dir), url, f"{scenario.name}: {release} install")
+    plain_url = with_database(url, urlsplit(url).path.lstrip("/"))
+    asyncio.run(_admin(plain_url, scenario.seed))
+    _, problems = start_twice(url, f"{scenario.name} (via {', '.join(scenario.chain)})")
+    rows = asyncio.run(_fetch(plain_url, scenario.check))
+    return problems + [f"{scenario.name}: {row[0]}" for row in rows]
+
+
+async def _fetch(url: str, query: str) -> list:
+    import asyncpg
+
+    conn = await asyncpg.connect(url)
+    try:
+        return await conn.fetch(query)
+    finally:
+        await conn.close()
+
+
 async def _admin(url: str, *statements: str) -> None:
     import asyncpg
 
@@ -197,7 +284,11 @@ def main() -> int:
     releases = args.from_release or sorted({FIRST_PG_RELEASE, newest_stable_release()})
     admin_url = with_database(args.url, urlsplit(args.url).path.lstrip("/") or "postgres")
 
-    databases = {"fresh": "bambuddy_check_fresh"} | {r: "bambuddy_check_" + re.sub(r"\W", "_", r) for r in releases}
+    databases = (
+        {"fresh": "bambuddy_check_fresh"}
+        | {r: "bambuddy_check_" + re.sub(r"\W", "_", r) for r in releases}
+        | {s.name: f"bambuddy_check_scenario_{i}" for i, s in enumerate(SCENARIOS)}
+    )
     asyncio.run(_admin(admin_url, *(f"DROP DATABASE IF EXISTS {db}" for db in databases.values())))
     asyncio.run(_admin(admin_url, *(f"CREATE DATABASE {db}" for db in databases.values())))
     try:
@@ -212,6 +303,12 @@ def main() -> int:
                 problems += changed
                 found[release] = drift(fresh, upgraded)
                 print(f"{release}: upgrade ok, {len(found[release])} object(s) differ from a fresh install")
+
+            for scenario in SCENARIOS:
+                url = with_database(args.url, databases[scenario.name], "asyncpg")
+                scenario_problems = run_scenario(scenario, url, Path(tmp))
+                problems += scenario_problems
+                print(f"{scenario.name}: {'ok' if not scenario_problems else 'FAILED'}")
     finally:
         asyncio.run(_admin(admin_url, *(f"DROP DATABASE IF EXISTS {db}" for db in databases.values())))
 

+ 0 - 5
scripts/postgres_schema_drift.txt

@@ -7,11 +7,6 @@
 # an upgraded database in line with the models removes its line.
 # Comment lines start with "#".
 
-# constraints
-constraint print_archives: FOREIGN KEY (library_file_id) REFERENCES library_files(id) ON DELETE SET NULL | fresh install: 1x; upgraded: missing
-constraint print_log_entries: FOREIGN KEY (archive_id) REFERENCES print_archives(id) ON DELETE SET NULL | fresh install: 1x; upgraded: missing
-constraint print_log_entries: FOREIGN KEY (created_by_id) REFERENCES users(id) ON DELETE SET NULL | fresh install: 1x; upgraded: missing
-
 # columns
 column active_print_spoolman.tray_remain_start | fresh install: json (udt json) NULL default None; upgraded: text (udt text) NULL default None
 column api_keys.can_access_cloud | fresh install: boolean (udt bool) NOT NULL default None; upgraded: boolean (udt bool) NULL default false