fix(db): apply all 10 code-review findings from the store arc

backend/census/store.py

@@ -7,7 +7,8 @@
 backend/db_catalogue.py): the shared module-level ``store`` instance is the
 runtime entry point (consumers alias it ``census_store``); the get/upsert
 helpers take an open conn (callers batch reads/writes per connection) and are
-staticmethods. Tests construct ``CensusStore(tmp_db)``.
+staticmethods. Tests construct ``CensusStore(tmp_db)``. SQL lives in the sibling
+store.sql (schema_* + DML blocks).
 """
 
 from __future__ import annotations
@@ -21,6 +22,7 @@
 
 from backend.db_catalogue import BaseCatalogue
 from backend.db_helpers import resolve_db_path
+from backend.sql_loader import load_sql
 
 
 class StoreRecord(TypedDict):
@@ -40,49 +42,17 @@ class StoreRecord(TypedDict):
 
 DB_PATH: Path = resolve_db_path("DB_CENSUS_PATH", "census", "census.db")
 
-_CREATE_CHARACTERS = """
-CREATE TABLE IF NOT EXISTS characters (
-    name_lower       TEXT    NOT NULL,
-    world            TEXT    NOT NULL,
-    name             TEXT    NOT NULL,
-    level            INTEGER,
-    guild_name       TEXT,
-    data_json        TEXT    NOT NULL,
-    last_resolved_at INTEGER NOT NULL,
-    updated_at       INTEGER NOT NULL,
-    PRIMARY KEY (name_lower, world)
-);
-"""
-
-_CREATE_GUILDS = """
-CREATE TABLE IF NOT EXISTS guilds (
-    name_lower       TEXT    NOT NULL,
-    world            TEXT    NOT NULL,
-    name             TEXT    NOT NULL,
-    data_json        TEXT    NOT NULL,
-    last_resolved_at INTEGER NOT NULL,
-    updated_at       INTEGER NOT NULL,
-    PRIMARY KEY (name_lower, world)
-);
-"""
-
-_CREATE_CHARACTER_AAS = """
-CREATE TABLE IF NOT EXISTS character_aas (
-    name_lower         TEXT    NOT NULL,
-    world              TEXT    NOT NULL,
-    data_json          TEXT    NOT NULL,
-    last_resolved_at   INTEGER NOT NULL,
-    PRIMARY KEY (name_lower, world)
-);
-"""
+_SQL = load_sql(__file__)
 
 _MIGRATIONS: list[str] = []  # future schema bumps appended here
 
 
 class CensusStore(BaseCatalogue):
     """Read/write access to one census.db file (last-known Census lookups)."""
 
-    FOREIGN_KEYS = True
+    # The three tables declare no foreign keys — the old init set the pragma
+    # as boilerplate, not because anything relied on cascade.
+    FOREIGN_KEYS = False
 
     # census.db predates the shared _meta table and has no build provenance
     # to track — rows carry their own timestamps.
@@ -92,18 +62,10 @@ def __init__(self, path: Path = DB_PATH) -> None:
         super().__init__(path)
 
     def _create_schema(self, conn: sqlite3.Connection) -> None:
-        conn.execute(_CREATE_CHARACTERS)
-        conn.execute(_CREATE_GUILDS)
-        conn.execute(_CREATE_CHARACTER_AAS)
-        for stmt in _MIGRATIONS:
-            try:
-                conn.execute(stmt)
-            except sqlite3.OperationalError as exc:
-                _log.info(
-                    "[census-store] migration skipped (likely already applied): %s — %s",
-                    stmt,
-                    exc,
-                )
+        conn.execute(_SQL["schema_characters"])
+        conn.execute(_SQL["schema_guilds"])
+        conn.execute(_SQL["schema_character_aas"])
+        self._apply_migrations(conn, _MIGRATIONS)
 
     # ── Characters ───────────────────────────────────────────────────────────
 
@@ -143,14 +105,7 @@ def upsert_character(
                 resolved_ts = existing["last_resolved_at"]  # sparse overlay — don't advance freshness
 
         conn.execute(
-            """
-            INSERT INTO characters (name_lower, world, name, level, guild_name, data_json, last_resolved_at, updated_at)
-            VALUES (?, ?, ?, ?, ?, ?, ?, ?)
-            ON CONFLICT(name_lower, world) DO UPDATE SET
-                name=excluded.name, level=excluded.level, guild_name=excluded.guild_name,
-                data_json=excluded.data_json, last_resolved_at=excluded.last_resolved_at,
-                updated_at=excluded.updated_at
-            """,
+            _SQL["upsert_character"],
             (
                 name.lower(),
                 world,
@@ -167,10 +122,7 @@ def upsert_character(
     @staticmethod
     def get_character(conn: sqlite3.Connection, name: str, world: str) -> StoreRecord | None:
         """Return {data, last_resolved_at} or None."""
-        row = conn.execute(
-            "SELECT data_json, last_resolved_at FROM characters WHERE name_lower=? AND world=?",
-            (name.lower(), world),
-        ).fetchone()
+        row = conn.execute(_SQL["select_character"], (name.lower(), world)).fetchone()
         if row is None:
             return None
         return {"data": json.loads(row[0]), "last_resolved_at": row[1]}
@@ -183,23 +135,14 @@ def upsert_guild(conn: sqlite3.Connection, name: str, world: str, data: dict, *,
         the roster list is reliable from Census regardless of member login recency."""
         ts = int(time.time()) if now is None else now
         conn.execute(
-            """
-            INSERT INTO guilds (name_lower, world, name, data_json, last_resolved_at, updated_at)
-            VALUES (?, ?, ?, ?, ?, ?)
-            ON CONFLICT(name_lower, world) DO UPDATE SET
-                name=excluded.name, data_json=excluded.data_json,
-                last_resolved_at=excluded.last_resolved_at, updated_at=excluded.updated_at
-            """,
+            _SQL["upsert_guild"],
             (name.lower(), world, name, json.dumps(data), ts, ts),
         )
         conn.commit()
 
     @staticmethod
     def get_guild(conn: sqlite3.Connection, name: str, world: str) -> StoreRecord | None:
-        row = conn.execute(
-            "SELECT data_json, last_resolved_at FROM guilds WHERE name_lower=? AND world=?",
-            (name.lower(), world),
-        ).fetchone()
+        row = conn.execute(_SQL["select_guild"], (name.lower(), world)).fetchone()
         if row is None:
             return None
         return {"data": json.loads(row[0]), "last_resolved_at": row[1]}
@@ -212,10 +155,7 @@ def get_character_aas(conn: sqlite3.Connection, name: str, world: str) -> StoreR
 
         The record carries the model_dump() of the response plus a
         last_resolved_at unix timestamp."""
-        row = conn.execute(
-            "SELECT data_json, last_resolved_at FROM character_aas WHERE name_lower = ? AND world = ?",
-            (name.lower(), world),
-        ).fetchone()
+        row = conn.execute(_SQL["select_character_aas"], (name.lower(), world)).fetchone()
         if row is None:
             return None
         return {
@@ -237,10 +177,7 @@ def upsert_character_aas(
         authoritative."""
         if now is None:
             now = int(time.time())
-        conn.execute(
-            "INSERT OR REPLACE INTO character_aas (name_lower, world, data_json, last_resolved_at) VALUES (?, ?, ?, ?)",
-            (name.lower(), world, json.dumps(data), now),
-        )
+        conn.execute(_SQL["upsert_character_aas"], (name.lower(), world, json.dumps(data), now))
         conn.commit()
 
 

backend/census/store.sql

@@ -0,0 +1,70 @@
+-- SQL for backend/census/store.py (CensusStore). Schema + DML both live
+-- here per the sql_loader convention — one grep target for all SQL.
+
+-- ---------------------------------------------------------------------------
+-- Schema
+-- ---------------------------------------------------------------------------
+
+-- :name schema_characters
+CREATE TABLE IF NOT EXISTS characters (
+    name_lower       TEXT    NOT NULL,
+    world            TEXT    NOT NULL,
+    name             TEXT    NOT NULL,
+    level            INTEGER,
+    guild_name       TEXT,
+    data_json        TEXT    NOT NULL,
+    last_resolved_at INTEGER NOT NULL,
+    updated_at       INTEGER NOT NULL,
+    PRIMARY KEY (name_lower, world)
+);
+
+-- :name schema_guilds
+CREATE TABLE IF NOT EXISTS guilds (
+    name_lower       TEXT    NOT NULL,
+    world            TEXT    NOT NULL,
+    name             TEXT    NOT NULL,
+    data_json        TEXT    NOT NULL,
+    last_resolved_at INTEGER NOT NULL,
+    updated_at       INTEGER NOT NULL,
+    PRIMARY KEY (name_lower, world)
+);
+
+-- :name schema_character_aas
+CREATE TABLE IF NOT EXISTS character_aas (
+    name_lower         TEXT    NOT NULL,
+    world              TEXT    NOT NULL,
+    data_json          TEXT    NOT NULL,
+    last_resolved_at   INTEGER NOT NULL,
+    PRIMARY KEY (name_lower, world)
+);
+
+-- ---------------------------------------------------------------------------
+-- DML
+-- ---------------------------------------------------------------------------
+
+-- :name upsert_character
+INSERT INTO characters (name_lower, world, name, level, guild_name, data_json, last_resolved_at, updated_at)
+VALUES (?, ?, ?, ?, ?, ?, ?, ?)
+ON CONFLICT(name_lower, world) DO UPDATE SET
+    name=excluded.name, level=excluded.level, guild_name=excluded.guild_name,
+    data_json=excluded.data_json, last_resolved_at=excluded.last_resolved_at,
+    updated_at=excluded.updated_at;
+
+-- :name select_character
+SELECT data_json, last_resolved_at FROM characters WHERE name_lower=? AND world=?;
+
+-- :name upsert_guild
+INSERT INTO guilds (name_lower, world, name, data_json, last_resolved_at, updated_at)
+VALUES (?, ?, ?, ?, ?, ?)
+ON CONFLICT(name_lower, world) DO UPDATE SET
+    name=excluded.name, data_json=excluded.data_json,
+    last_resolved_at=excluded.last_resolved_at, updated_at=excluded.updated_at;
+
+-- :name select_guild
+SELECT data_json, last_resolved_at FROM guilds WHERE name_lower=? AND world=?;
+
+-- :name select_character_aas
+SELECT data_json, last_resolved_at FROM character_aas WHERE name_lower = ? AND world = ?;
+
+-- :name upsert_character_aas
+INSERT OR REPLACE INTO character_aas (name_lower, world, data_json, last_resolved_at) VALUES (?, ?, ?, ?);

backend/db_catalogue.py

@@ -43,9 +43,13 @@
 
 import logging
 import sqlite3
-from collections.abc import Mapping, Sequence
+from collections.abc import AsyncIterator, Mapping, Sequence
+from contextlib import asynccontextmanager
 from pathlib import Path
-from typing import ClassVar
+from typing import TYPE_CHECKING, ClassVar
+
+if TYPE_CHECKING:
+    import aiosqlite
 
 from backend.db_helpers import like_escape
 from backend.eq2db import _meta as _meta_db
@@ -177,7 +181,7 @@ def _fetchall(self, sql: str, params: Sequence | Mapping = ()) -> list[sqlite3.R
         except sqlite3.OperationalError as exc:
             if not _is_unbuilt_schema(exc):
                 raise
-            _log.warning("[eq2db] read on unbuilt db %r: %s", self, exc)
+            _log.warning("[db-catalogue] read on unbuilt db %r: %s", self, exc)
             return []
         finally:
             conn.close()
@@ -194,7 +198,7 @@ def _fetchone(self, sql: str, params: Sequence | Mapping = ()) -> sqlite3.Row |
         except sqlite3.OperationalError as exc:
             if not _is_unbuilt_schema(exc):
                 raise
-            _log.warning("[eq2db] read on unbuilt db %r: %s", self, exc)
+            _log.warning("[db-catalogue] read on unbuilt db %r: %s", self, exc)
             return None
         finally:
             conn.close()
@@ -216,7 +220,7 @@ def _find_exact_then_like(self, exact_sql: str, like_sql: str, name: str) -> lis
         except sqlite3.OperationalError as exc:
             if not _is_unbuilt_schema(exc):
                 raise
-            _log.warning("[eq2db] read on unbuilt db %r: %s", self, exc)
+            _log.warning("[db-catalogue] read on unbuilt db %r: %s", self, exc)
             return []
         finally:
             conn.close()
@@ -234,7 +238,7 @@ def init_db(self) -> sqlite3.Connection:
         its own connection against ``self.path``, so a memory DB would be a
         fresh empty database per read — tests use a tmp_path file instead.
         """
-        _log.debug("[eq2db] init_db %r", self)
+        _log.debug("[db-catalogue] init_db %r", self)
         self.path.parent.mkdir(parents=True, exist_ok=True)
         conn = sqlite3.connect(self.path)
         conn.execute("PRAGMA journal_mode = WAL;")
@@ -254,6 +258,18 @@ def _create_schema(self, conn: sqlite3.Connection) -> None:
         Runs inside init_db before the commit — don't commit here."""
         raise NotImplementedError
 
+    def _apply_migrations(self, conn: sqlite3.Connection, stmts: Sequence[str]) -> None:
+        """Run idempotent ALTER-style migrations, skipping already-applied
+        ones. The skip is logged at DEBUG (init_db runs per request on some
+        routes) so a genuinely broken statement — which raises the same
+        OperationalError as a duplicate column — at least leaves a trace,
+        unlike the silent per-store `pass` loops this replaces."""
+        for stmt in stmts:
+            try:
+                conn.execute(stmt)
+            except sqlite3.OperationalError as exc:
+                _log.debug("[db-catalogue] migration skipped on %r (already applied?): %s", self, exc)
+
     def _post_init(self, conn: sqlite3.Connection) -> None:
         """Optional post-commit startup work (data backfills). Default: none."""
 
@@ -264,6 +280,24 @@ class AsyncStoreBase(PathBound):
     Unlike :class:`BaseCatalogue`, these stores do NOT own their schema:
     the whole users.db family shares one file whose tables/migrations are
     orchestrated by ``backend.server.db.init_db()`` at startup. Each domain
-    method opens its own aiosqlite connection against ``self.path`` —
-    exactly the per-call transaction shape the old free functions had,
-    minus the ``path: Path = DB_PATH`` threading."""
+    method opens its own connection via :meth:`_db` — exactly the per-call
+    transaction shape the old free functions had, minus the
+    ``path: Path = DB_PATH`` threading.
+
+    (ServersStore, the one synchronous domain store, inherits
+    :class:`PathBound` directly — it must never grow aiosqlite methods.)
+    """
+
+    @asynccontextmanager
+    async def _db(self, *, row_factory: bool = False) -> AsyncIterator[aiosqlite.Connection]:
+        """The one place a users.db domain connection is opened — future
+        connection-level policy (busy_timeout, a foreign_keys pragma once
+        the data is audited for violations) lands here, not at N call
+        sites. ``row_factory=True`` sets aiosqlite.Row for dict-shaped
+        reads."""
+        import aiosqlite  # deferred: sync-only consumers never pay the import
+
+        async with aiosqlite.connect(self.path) as db:
+            if row_factory:
+                db.row_factory = aiosqlite.Row
+            yield db

backend/eq2db/recipes.py

@@ -155,11 +155,7 @@ def _create_schema(self, conn: sqlite3.Connection) -> None:
         conn.execute(_SQL["schema_recipes"])
         conn.execute(_SQL["schema_recipe_classes"])
         # Migrate existing DBs that predate the spell-tier columns
-        for stmt in _MIGRATIONS:
-            try:
-                conn.execute(stmt)
-            except sqlite3.OperationalError:
-                pass  # column already exists
+        self._apply_migrations(conn, _MIGRATIONS)
         conn.executescript(_SQL["indexes_recipes"])
 
     def _post_init(self, conn: sqlite3.Connection) -> None:

backend/eq2db/zones.py

@@ -1233,11 +1233,7 @@ def _create_schema(self, conn: sqlite3.Connection) -> None:
         # Migration: zone categories + position for drag-reorder.
         # Idempotent — already-applied schemas raise OperationalError on the
         # duplicate-column attempt, which we swallow.
-        for stmt in (_SQL["migrate_add_featured_position"], _SQL["migrate_add_featured_category"]):
-            try:
-                conn.execute(stmt)
-            except sqlite3.OperationalError:
-                pass  # column already exists
+        self._apply_migrations(conn, (_SQL["migrate_add_featured_position"], _SQL["migrate_add_featured_category"]))
         # Migration: drop the pre-v2 zone_bosses table if it lingers from
         # an older DB build. No need to preserve data — bosses are always
         # rebuilt from the curated source file.