diff --git a/apps/board/migrations.py b/apps/board/migrations.py index 6a951c1..3fc9622 100644 --- a/apps/board/migrations.py +++ b/apps/board/migrations.py @@ -45,11 +45,7 @@ def _attachments_accept_messages(db: sqlite3.Connection) -> None: on can cascade, and switched back on after, with a check that nothing was broken in between. """ - if "message_id" in _columns(db, "attachments"): - return # a new database: schema.sql already created it this way - - db.execute("PRAGMA foreign_keys = OFF") - try: + if "message_id" not in _columns(db, "attachments"): db.execute(""" CREATE TABLE attachments_new ( id INTEGER PRIMARY KEY, @@ -83,8 +79,11 @@ def _attachments_accept_messages(db: sqlite3.Connection) -> None: broken = db.execute("PRAGMA foreign_key_check").fetchall() if broken: raise RuntimeError(f"migration left dangling references: {broken}") - finally: - db.execute("PRAGMA foreign_keys = ON") + + # Outside the branch above: a database created fresh by schema.sql has the + # column but not this index, because schema.sql cannot carry it — see the + # note there. Both paths end up with the same table and the same indexes. + db.execute("CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id)") # (number, description, function). The number is the value written to @@ -101,6 +100,11 @@ def apply(db: sqlite3.Connection) -> list[str]: for number, description, step in sorted(STEPS): if number <= version: continue + # Outside the transaction, deliberately: "PRAGMA foreign_keys is a + # no-op within a transaction". Setting it inside BEGIN looks like it + # worked and changes nothing, which is how a table rebuild ends up + # running with enforcement still on. + db.execute("PRAGMA foreign_keys = OFF") db.execute("BEGIN IMMEDIATE") try: step(db) @@ -111,5 +115,7 @@ def apply(db: sqlite3.Connection) -> list[str]: except Exception: db.execute("ROLLBACK") raise + finally: + db.execute("PRAGMA foreign_keys = ON") done.append(f"{number}: {description}") return done diff --git a/apps/board/schema.sql b/apps/board/schema.sql index e1964ff..41d53cb 100644 --- a/apps/board/schema.sql +++ b/apps/board/schema.sql @@ -137,7 +137,12 @@ CREATE TABLE IF NOT EXISTS attachments ( CREATE INDEX IF NOT EXISTS attachments_thread ON attachments(thread_id); CREATE INDEX IF NOT EXISTS attachments_comment ON attachments(comment_id); -CREATE INDEX IF NOT EXISTS attachments_message ON attachments(message_id); +-- The index on message_id is NOT here, and that is not an oversight. +-- CREATE INDEX IF NOT EXISTS guards the index NAME, not the column: run it +-- against a database whose attachments table predates message_id and it fails +-- with "no such column", taking the whole start-up with it. Any index on a +-- column a migration introduces belongs in that migration, after the column +-- exists. See migrations.py. -- Gitea access tokens for the editor. -- diff --git a/apps/board/tests/test_migrations.py b/apps/board/tests/test_migrations.py index 5017ed9..7fbc848 100644 --- a/apps/board/tests/test_migrations.py +++ b/apps/board/tests/test_migrations.py @@ -9,6 +9,7 @@ never needed for. from __future__ import annotations import sqlite3 +from pathlib import Path import pytest @@ -42,6 +43,10 @@ def old_db(tmp_path): """A database as it existed before this migration, with real rows in it.""" db = sqlite3.connect(tmp_path / "old.db", isolation_level=None) db.row_factory = sqlite3.Row + # As connect() does in db.py. Without this the fixture runs with SQLite's + # default (off) and a migration that mishandles foreign keys passes here + # and fails on the server. + db.execute("PRAGMA foreign_keys = ON") db.executescript(OLD_SCHEMA) db.execute("INSERT INTO members (id, gitea_login) VALUES (1, 'salvador')") db.execute("INSERT INTO threads (id) VALUES (7)") @@ -126,3 +131,58 @@ def test_a_fresh_database_skips_it(app, db): done and return quietly rather than rebuilding a table it just created.""" assert "message_id" in {row[1] for row in db.execute("PRAGMA table_info(attachments)")} assert migrations.apply(db) == [] + + +# --- the whole start-up, not just the step ------------------------------- + +def test_the_app_starts_against_a_database_from_before_all_this(tmp_path): + """The test that was missing, and the reason the site went down. + + Every other test here calls `migrations.apply` directly. The failure was + one layer above it: `init_db` runs `schema.sql` *first*, and schema.sql + carried `CREATE INDEX ... ON attachments(message_id)`. IF NOT EXISTS guards + the index name, not the column — so against a real database the script died + with "no such column: message_id" before any migration could fix anything, + the app never finished starting, and Caddy answered 502. + + This builds a database with the schema as it shipped, puts a real row in + it, and starts the application the way gunicorn does. + """ + from apps.board.app import create_app + from apps.board.db import connect + + shipped = (Path(__file__).resolve().parents[3] / "apps/board/schema.sql") + old_sql = shipped.read_text(encoding="utf-8") + # Reduce it to the shape that predates this migration: no message_id + # anywhere, and the CHECK that goes with it. + old_sql = old_sql.replace(" message_id INTEGER REFERENCES messages(id),\n", "") + old_sql = old_sql.replace( + " CHECK ((thread_id IS NOT NULL) + (comment_id IS NOT NULL)\n" + " + (message_id IS NOT NULL) = 1)", + " CHECK ((thread_id IS NULL) <> (comment_id IS NULL))") + + path = str(tmp_path / "board.db") + db = connect(path) + db.executescript(old_sql) + db.execute("INSERT INTO members (gitea_login, display_name, role) " + "VALUES ('salvador', 'Salvador', 'user')") + db.execute("INSERT INTO threads (author_id, title, body_md) VALUES (1, 'Hola', 'T')") + db.execute("""INSERT INTO attachments + (thread_id, stored_name, original_name, content_type, bytes, uploaded_by) + VALUES (1, 'foto-abc123abc123.jpg', 'foto.jpg', 'image/jpeg', 2048, 1)""") + db.close() + + create_app({"SECRET_KEY": "x", "DB_PATH": path, "OWNER_LOGIN": "salvador", + "UPLOAD_DIR": str(tmp_path / "uploads"), "TESTING": True}) + + db = connect(path) + assert db.execute("PRAGMA user_version").fetchone()[0] == 1 + # The row the server actually has, still there and still whole. + kept = db.execute("SELECT stored_name, thread_id, bytes FROM attachments").fetchone() + assert (kept["stored_name"], kept["thread_id"], kept["bytes"]) == ( + "foto-abc123abc123.jpg", 1, 2048) + + # And starting again changes nothing, because a container restarts. + create_app({"SECRET_KEY": "x", "DB_PATH": path, "OWNER_LOGIN": "salvador", + "UPLOAD_DIR": str(tmp_path / "uploads"), "TESTING": True}) + db.close()