From 4bd16538463c0c3c7f54a72809a792f8467ea144 Mon Sep 17 00:00:00 2001 From: VortexUK Date: Sat, 11 Jul 2026 12:48:00 +0100 Subject: [PATCH] fix(db): apply all 10 code-review findings from the store arc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up review of #153-#155 (same 8-angle adversarial pipeline as #152); this fixes everything it found: Correctness hazards: - backend/server/db/__init__.py init_db now reads DB_PATH at call time (was `path: Path = DB_PATH` — the default-arg-captured-at-import pattern the arc eliminated everywhere else; under the documented early-import race, conftest's argless call would have run schema + migrations against the developer's real users.db). - backend/server/api/zones.py drops the import-time PARSES_DB_PATH value copy for the store's dynamic `parses_db.path` — the last consumer that a per-test store.path re-point couldn't reach. - tests/fixtures/parses_db.py: de-duplicated setattr (the second line now re-points DB_PATH as the docstring promised), docstring honest. Structure / dedup: - AsyncStoreBase gains the shared `_db()` asynccontextmanager — the 46 hand-rolled `aiosqlite.connect(self.path)` (+19 row_factory) sites in the six async domain stores collapse onto it; future connection-level policy (busy_timeout, FK pragma) has one home. - ServersStore (sync) moves off the async-named base onto PathBound. - BaseCatalogue._apply_migrations replaces the four drifted per-store migration loops (census logged, parses/recipes/zones swallowed silently) — skips now log at DEBUG uniformly. - CensusStore aligns with the .sql convention (new store.sql; inline schema/DML constants removed), FOREIGN_KEYS corrected to False (the schema declares no FKs), and sql_loader's stale "DDL stays embedded in the .py" docstring bullet updated to reality. - backend/db_catalogue.py log prefix [eq2db] → [db-catalogue]. Docs & tests: - The seven users.db domain docstrings + parses/db.py no longer teach the removed `path: Path = DB_PATH` convention; the inverted "CRITICAL: pass the explicit path" comments in two test files now describe the store model; orphaned test constants removed. - New tests/fixtures/users_db.py point_users_db_at() replaces the ~9 pasted (and already-forked) ALL_STORES re-point loops. - New tests/server/test_db_facade.py: facade completeness guard (a public store method with no alias fails the build with the name spelled out; intentional omissions live in _FACADE_EXEMPT, itself checked for staleness) + alias-integrity guard (every facade alias must be bound to an ALL_STORES instance). Three dead aliases (get_server_by_subdomain_sync, generate_token, hash_token) removed. 1527 tests green (3 new), ruff + pyright clean; /api/server, raid-schedule and character read paths live-verified. Co-Authored-By: Claude Fable 5 --- backend/census/store.py | 97 ++++--------------- backend/census/store.sql | 70 +++++++++++++ backend/db_catalogue.py | 52 ++++++++-- backend/eq2db/recipes.py | 6 +- backend/eq2db/zones.py | 6 +- backend/server/api/zones.py | 10 +- backend/server/db/__init__.py | 9 +- backend/server/db/claims.py | 28 +++--- backend/server/db/favorites.py | 20 ++-- backend/server/db/item_watch.py | 16 ++- backend/server/db/raid_schedule.py | 12 +-- backend/server/db/servers.py | 7 +- backend/server/db/tokens.py | 17 ++-- backend/server/db/users.py | 53 +++++----- backend/server/parses/db.py | 16 ++- backend/sql_loader.py | 6 +- tests/fixtures/parses_db.py | 6 +- tests/fixtures/users_db.py | 34 +++++++ tests/server/test_aa_census_store.py | 4 +- tests/server/test_admin_tamper_reports.py | 9 +- tests/server/test_auth_tokens.py | 5 +- tests/server/test_db.py | 2 - tests/server/test_db_facade.py | 87 +++++++++++++++++ tests/server/test_favorites.py | 8 +- .../test_parses_ingest_client_warnings.py | 9 +- tests/server/test_raid_schedule.py | 11 +-- tests/server/test_rankings.py | 9 +- tests/server/test_server_context.py | 10 +- tests/server/test_server_route.py | 10 +- tests/server/test_users_db_roles.py | 8 +- 30 files changed, 372 insertions(+), 265 deletions(-) create mode 100644 backend/census/store.sql create mode 100644 tests/fixtures/users_db.py create mode 100644 tests/server/test_db_facade.py diff --git a/backend/census/store.py b/backend/census/store.py index f5893a05..9645c3a4 100644 --- a/backend/census/store.py +++ b/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,41 +42,7 @@ 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 @@ -82,7 +50,9 @@ class StoreRecord(TypedDict): 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() diff --git a/backend/census/store.sql b/backend/census/store.sql new file mode 100644 index 00000000..5961949e --- /dev/null +++ b/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 (?, ?, ?, ?); diff --git a/backend/db_catalogue.py b/backend/db_catalogue.py index a0d3739d..4ce6ec8e 100644 --- a/backend/db_catalogue.py +++ b/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 diff --git a/backend/eq2db/recipes.py b/backend/eq2db/recipes.py index b066fc67..46548da1 100644 --- a/backend/eq2db/recipes.py +++ b/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: diff --git a/backend/eq2db/zones.py b/backend/eq2db/zones.py index 56ea72ee..f4e4749a 100644 --- a/backend/eq2db/zones.py +++ b/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. diff --git a/backend/server/api/zones.py b/backend/server/api/zones.py index 3cb6b235..1f67a668 100644 --- a/backend/server/api/zones.py +++ b/backend/server/api/zones.py @@ -27,7 +27,7 @@ from backend.server.core.executor import run_sync from backend.server.core.primary_guild import cached_primary_guild from backend.server.core.session_user import SessionUser -from backend.server.parses.db import DB_PATH as PARSES_DB_PATH +from backend.server.parses.db import store as parses_db from backend.server.server_context import current_world as _current_world from backend.sql_loader import load_sql @@ -245,9 +245,9 @@ async def _resolve_primary_guild(discord_id: str) -> tuple[str | None, str | Non def _most_recent_parsed_guild_sync(discord_id: str) -> str | None: """Most recent non-null guild_name this user has uploaded a parse for.""" - if not PARSES_DB_PATH.exists(): + if not parses_db.path.exists(): return None - with sqlite3.connect(PARSES_DB_PATH) as conn: + with sqlite3.connect(parses_db.path) as conn: conn.execute("PRAGMA query_only = ON") row = conn.execute(_SQL["most_recent_parsed_guild"], (discord_id,)).fetchone() return row[0] if row else None @@ -266,13 +266,13 @@ def _compute_progress_sync(guild_name: str) -> dict[str, list[KilledEncounter]]: and zones data live in separate SQLite files and we'd rather avoid cross-DB ATTACH. """ - if not PARSES_DB_PATH.exists() or not zones_db.path.exists(): + if not parses_db.path.exists() or not zones_db.path.exists(): return {} # Pull every winning row for the guild as (id, title_lower, started_at). # We need the timestamp + id to surface "last kill" — a DISTINCT title pass # wouldn't be enough. - with sqlite3.connect(PARSES_DB_PATH) as pconn: + with sqlite3.connect(parses_db.path) as pconn: pconn.execute("PRAGMA query_only = ON") kills = [ (row[0], row[1].lower(), row[2]) diff --git a/backend/server/db/__init__.py b/backend/server/db/__init__.py index dc14f505..513727e0 100644 --- a/backend/server/db/__init__.py +++ b/backend/server/db/__init__.py @@ -28,7 +28,7 @@ DB_PATH = resolve_db_path("DB_USERS_PATH", "users.db") -def init_db(path: Path = DB_PATH) -> None: +def init_db(path: Path | None = None) -> None: """Create tables if they don't exist + apply migrations. Called once at startup. Idempotent. Order: @@ -40,6 +40,10 @@ def init_db(path: Path = DB_PATH) -> None: (for existing DBs). Column-dependent indexes live in migrations.py AFTER the ADD COLUMN — never in SCHEMA. """ + # Read DB_PATH at call time, not def time — the default-arg capture + # pattern is exactly what the store conversion eliminated everywhere + # else, and conftest re-points DB_PATH after import (BE-096 race). + path = Path(path) if path is not None else DB_PATH path.parent.mkdir(parents=True, exist_ok=True) with sqlite3.connect(path) as conn: conn.executescript(SCHEMA) @@ -81,15 +85,12 @@ def init_db(path: Path = DB_PATH) -> None: update_item_watch_check = item_watch_store.update_item_watch_check # servers registry -get_server_by_subdomain_sync = servers_store.get_server_by_subdomain_sync get_server_by_world_sync = servers_store.get_server_by_world_sync list_servers_sync = servers_store.list_servers_sync set_default_server_sync = servers_store.set_default_server_sync upsert_server_settings_sync = servers_store.upsert_server_settings_sync # api tokens -generate_token = tokens_store.generate_token -hash_token = tokens_store.hash_token list_api_tokens = tokens_store.list_api_tokens lookup_api_token = tokens_store.lookup_api_token mint_api_token = tokens_store.mint_api_token diff --git a/backend/server/db/claims.py b/backend/server/db/claims.py index fc22438c..4d08f1cd 100644 --- a/backend/server/db/claims.py +++ b/backend/server/db/claims.py @@ -1,8 +1,8 @@ """users.db character_claims table helpers. Carved out of the original 1309-line web/db.py. Async (aiosqlite) helpers -for the character claims domain. ``path: Path = DB_PATH`` parameter on every -public function so tests can inject a temp DB. +for the character claims domain. Per-call connections open via the +shared ``AsyncStoreBase._db()``; tests re-point ``store.path``. Claim statuses: pending – submitted, awaiting admin review @@ -16,8 +16,6 @@ from pathlib import Path -import aiosqlite - from backend.db_catalogue import AsyncStoreBase from backend.server.db import DB_PATH from backend.sql_loader import load_sql @@ -45,8 +43,7 @@ async def get_active_claims( Claims are scoped to (discord_id, world) — a user's Varsoon and Wuoshi primaries are completely independent. """ - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["list_active_claims"], (discord_id, world), @@ -70,8 +67,7 @@ async def submit_claim( Already-approved claims for other characters are not affected. Raises ValueError if this character is already claimed on this world. """ - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: # Reject if this character name is already claimed (approved or pending) # by anyone *on this world* (EQ2 names are unique only within a server) async with db.execute( @@ -113,7 +109,7 @@ async def withdraw_claim( character on the same world is automatically promoted to primary. Returns True if something changed. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: # Check if this claim is primary before withdrawing (and capture world from row) async with db.execute( _SQL["select_primary_and_world"], @@ -151,7 +147,7 @@ async def set_primary( Claims on other worlds are not affected. Returns True if the target claim exists and was updated. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: # Verify the claim belongs to this user, is approved, and is on the right world async with db.execute( _SQL["find_approved_claim_for_user_on_world"], @@ -177,8 +173,7 @@ async def get_claim_by_id( claim_id: int, ) -> dict | None: """Return a single claim joined with its submitting user's info.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["find_claim_with_user"], (claim_id,), @@ -198,8 +193,7 @@ async def list_claims( Pending claims are sorted oldest-first (queue order). All other statuses are sorted newest-first. """ - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: where_parts: list[str] = [] params: list = [] if status: @@ -233,7 +227,7 @@ async def review_claim( is the single funnel every approval path (admin + officer) flows through. Returns the updated claim (with user info) or None if not found. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute(_SQL["select_claim_user_and_world"], (claim_id,)) as cur: row = await cur.fetchone() if not row: @@ -273,7 +267,7 @@ async def delete_claim( claim_id: int, ) -> bool: """Hard-delete a claim row. Returns True if a row was deleted.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute(_SQL["delete_claim"], (claim_id,)) deleted = cur.rowcount > 0 await db.commit() @@ -284,7 +278,7 @@ async def delete_claims_for_user( discord_id: str, ) -> int: """Hard-delete all claim rows for a user. Returns the number of rows deleted.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute(_SQL["delete_claims_for_user"], (discord_id,)) count = cur.rowcount await db.commit() diff --git a/backend/server/db/favorites.py b/backend/server/db/favorites.py index 53e837f1..cbe3a3d0 100644 --- a/backend/server/db/favorites.py +++ b/backend/server/db/favorites.py @@ -2,16 +2,15 @@ A favourite is a per-user bookmark of a (character_name, world) pair — NOT ownership; it carries no guild or claim implications. Mirrors the raid_schedule -domain: ``path: Path = DB_PATH`` on every public function so tests can inject a -temp DB. Callers validate + capitalise ``character_name`` before calling in. +domain: per-call connections open via the shared AsyncStoreBase._db(); tests +re-point ``store.path`` (or construct their own store over a tmp DB). +Callers validate + capitalise ``character_name`` before calling in. """ from __future__ import annotations from pathlib import Path -import aiosqlite - from backend.db_catalogue import AsyncStoreBase from backend.server.db import DB_PATH from backend.sql_loader import load_sql @@ -34,7 +33,7 @@ async def add_favorite(self, discord_id: str, character_name: str, world: str, c existed (idempotent) or the cap was reached. The guard lives in the INSERT itself so concurrent requests cannot race a check-then-insert past the cap; callers disambiguate the False case via ``is_favorited``.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["insert_favorite_capped"], (discord_id, character_name, world, discord_id, world, cap), @@ -44,7 +43,7 @@ async def add_favorite(self, discord_id: str, character_name: str, world: str, c async def remove_favorite(self, discord_id: str, character_name: str, world: str) -> bool: """Remove a favourite. Returns False when there was nothing to remove.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute(_SQL["delete_favorite"], (discord_id, character_name, world)) await db.commit() return cur.rowcount > 0 @@ -53,28 +52,27 @@ async def count_favorites_for_character(self, character_name: str, world: str) - """How many users favourited this character. Kept separate from the membership lookup so the API layer can cache the count and genuinely skip this query (and its connection) on a cache hit.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute(_SQL["count_for_character"], (character_name, world)) as cur: row = await cur.fetchone() return row[0] if row else 0 async def is_favorited(self, discord_id: str, character_name: str, world: str) -> bool: """Point lookup on the UNIQUE index — has this user favourited this character?""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute(_SQL["select_is_favorited"], (discord_id, character_name, world)) as cur: return await cur.fetchone() is not None async def count_user_favorites(self, discord_id: str, world: str) -> int: """How many characters this user has favourited on this world.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute(_SQL["count_for_user"], (discord_id, world)) as cur: row = await cur.fetchone() return row[0] if row else 0 async def list_favorites(self, discord_id: str, world: str) -> list[dict]: """The user's favourites on this world, newest first.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["select_for_user"], (discord_id, world)) as cur: return [dict(r) for r in await cur.fetchall()] diff --git a/backend/server/db/item_watch.py b/backend/server/db/item_watch.py index e06d9dc4..5dd3add5 100644 --- a/backend/server/db/item_watch.py +++ b/backend/server/db/item_watch.py @@ -1,16 +1,14 @@ """users.db item_watch table helpers. Carved out of the original 1309-line web/db.py. Async (aiosqlite) helpers -for the item watch domain. ``path: Path = DB_PATH`` parameter on every -public function so tests can inject a temp DB. +for the item watch domain. Per-call connections open via the +shared ``AsyncStoreBase._db()``; tests re-point ``store.path``. """ from __future__ import annotations from pathlib import Path -import aiosqlite - from backend.db_catalogue import AsyncStoreBase from backend.server.db import DB_PATH from backend.sql_loader import load_sql @@ -41,8 +39,7 @@ async def add_item_watch( Raises ValueError on duplicate (same world + guild + character + item_id). Returns the new row dict. """ - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: try: cur = await db.execute( _SQL["add_watch"], @@ -65,8 +62,7 @@ async def list_item_watches( world: str = "Varsoon", ) -> list[dict]: """Return all item watch entries for a guild on a given server, ordered by added_at descending.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["list_for_guild"], (guild_name, world)) as cur: rows = await cur.fetchall() return [dict(r) for r in rows] @@ -78,7 +74,7 @@ async def remove_item_watch( world: str = "Varsoon", ) -> bool: """Delete an item watch entry. Scoped to guild_name and world for safety. Returns True if deleted.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute(_SQL["remove_watch"], (watch_id, guild_name, world)) deleted = cur.rowcount > 0 await db.commit() @@ -95,7 +91,7 @@ async def update_item_watch_check( Updates last_seen_at (and first_seen_at if not yet set) only when seen=True. """ sql = _SQL["update_seen"] if seen else _SQL["update_unseen"] - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: await db.execute(sql, (watch_id,)) await db.commit() diff --git a/backend/server/db/raid_schedule.py b/backend/server/db/raid_schedule.py index 882737e4..17f5b662 100644 --- a/backend/server/db/raid_schedule.py +++ b/backend/server/db/raid_schedule.py @@ -1,8 +1,8 @@ """users.db raid_teams / raid_slots helpers (async aiosqlite). Officer-editable, publicly-viewable guild raid schedules. Mirrors the -item_watch domain: ``path: Path = DB_PATH`` on every public function so tests -can inject a temp DB. A team carries a ``raids`` list of its slots. +item_watch domain: per-call connections via the shared ``AsyncStoreBase._db()``; +tests re-point ``store.path``. A team carries a ``raids`` list of its slots. """ from __future__ import annotations @@ -36,8 +36,7 @@ async def _teams_with_slots(db: aiosqlite.Connection, teams: list[dict]) -> list async def get_schedule(self, world: str, guild_name: str) -> list[dict]: """Return this guild's raid teams (ordered) each with a ``raids`` list.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["select_teams"], (world, guild_name)) as cur: teams = [dict(r) for r in await cur.fetchall()] return await RaidScheduleStore._teams_with_slots(db, teams) @@ -56,7 +55,7 @@ async def replace_schedule( validated by the caller; this just persists. team_index / slot_index are assigned from list order. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: try: await db.execute("BEGIN") await db.execute(_SQL["delete_slots_for_guild"], (world, guild_name)) @@ -95,8 +94,7 @@ async def replace_schedule( async def list_all_teams_with_twitch(self) -> list[dict]: """Every team across all worlds/guilds that has a twitch_login, each with its raids. Used by the Twitch-live poller (Part 2).""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["select_teams_with_twitch"]) as cur: teams = [dict(r) for r in await cur.fetchall()] return await RaidScheduleStore._teams_with_slots(db, teams) diff --git a/backend/server/db/servers.py b/backend/server/db/servers.py index 26f5b498..2d64858e 100644 --- a/backend/server/db/servers.py +++ b/backend/server/db/servers.py @@ -4,8 +4,7 @@ for the servers domain — these are called at startup and from sync admin endpoints, so they use the stdlib driver directly. -``path: Path = DB_PATH`` parameter on every public function so tests can -inject a temp DB. +Tests re-point ``store.path`` or construct ``ServersStore(tmp_db)``. """ from __future__ import annotations @@ -13,14 +12,14 @@ import sqlite3 from pathlib import Path -from backend.db_catalogue import AsyncStoreBase +from backend.db_catalogue import PathBound from backend.server.db import DB_PATH from backend.sql_loader import load_sql _SQL = load_sql(__file__) -class ServersStore(AsyncStoreBase): +class ServersStore(PathBound): """users.db `servers` domain. Schema/migrations are owned by the package orchestrator (backend.server.db.init_db); methods open per-call connections against ``self.path``.""" diff --git a/backend/server/db/tokens.py b/backend/server/db/tokens.py index d0970bf0..b352ccf8 100644 --- a/backend/server/db/tokens.py +++ b/backend/server/db/tokens.py @@ -1,8 +1,8 @@ """users.db api_tokens table helpers. Carved out of the original 1309-line web/db.py. Async (aiosqlite) helpers -for the API token domain. ``path: Path = DB_PATH`` parameter on every public -function so tests can inject a temp DB. +for the API token domain. Per-call connections open via the shared +``AsyncStoreBase._db()``; tests re-point ``store.path``. Raw tokens are 'eq2c_' + 32 url-safe base64 chars (≈192 bits entropy). Only the SHA-256 hash is stored; the raw token is shown to the user once @@ -16,8 +16,6 @@ import time from pathlib import Path -import aiosqlite - from backend.db_catalogue import AsyncStoreBase from backend.server.db import DB_PATH from backend.sql_loader import load_sql @@ -63,8 +61,7 @@ async def mint_api_token( immediately — it cannot be recovered later. """ raw, h, prefix = TokensStore.generate_token() - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: cur = await db.execute( _SQL["mint_token"], (user_id, name, h, prefix), @@ -78,8 +75,7 @@ async def mint_api_token( async def list_api_tokens(self, user_id: str) -> list[dict]: """All tokens for a user, newest first. Hash is omitted — UI doesn't need it.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["list_for_user"], (user_id,), @@ -94,7 +90,7 @@ async def revoke_api_token( ) -> bool: """Mark a token revoked. Scoped to user_id so one user can't revoke another's. Returns True if a row was updated.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["revoke_token"], (token_id, user_id), @@ -109,8 +105,7 @@ async def lookup_api_token(self, raw_token: str) -> dict | None: if not raw_token or not raw_token.startswith(TOKEN_PREFIX): return None h = TokensStore.hash_token(raw_token) - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["lookup_by_hash"], (h,), diff --git a/backend/server/db/users.py b/backend/server/db/users.py index 718cb29b..6cc8ec0e 100644 --- a/backend/server/db/users.py +++ b/backend/server/db/users.py @@ -1,16 +1,14 @@ """users.db users table + role / role_request / role_permission helpers. Carved out of the original 1309-line web/db.py. Async (aiosqlite) helpers -for the users domain. ``path: Path = DB_PATH`` parameter on every public -function so tests can inject a temp DB. +for the users domain. Per-call connections open via the shared +``AsyncStoreBase._db()``; tests re-point ``store.path``. """ from __future__ import annotations from pathlib import Path -import aiosqlite - from backend.db_catalogue import AsyncStoreBase from backend.server.core.sql_helpers import build_where from backend.server.db import DB_PATH @@ -51,8 +49,7 @@ async def upsert_user( """ is_admin = discord_id in admin_ids new_user_status = "approved" if (is_admin or open_signup) else "pending" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: await db.execute( _SQL["upsert_user"], ( @@ -71,8 +68,7 @@ async def upsert_user( async def get_user_access_status(self, discord_id: str) -> str: """Return the access_status for a user, or 'pending' if not found.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["select_access_status"], (discord_id,)) as cur: row = await cur.fetchone() return row["access_status"] if row else "pending" @@ -85,8 +81,7 @@ async def get_display_names_for_discord_ids(self, ids: list[str]) -> dict[str, s callers handle the fallback display.""" if not ids: return {} - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: placeholders = ",".join("?" for _ in ids) async with db.execute( _SQL["select_display_names_by_ids"].format(placeholders=placeholders), @@ -97,8 +92,7 @@ async def get_display_names_for_discord_ids(self, ids: list[str]) -> dict[str, s async def list_pending_users(self) -> list[dict]: """Return all users with access_status = 'pending', newest first.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["list_pending_users"]) as cur: rows = await cur.fetchall() return [dict(r) for r in rows] @@ -108,22 +102,21 @@ async def approve_all_pending(self) -> int: of rows updated. Used at startup when OPEN_SIGNUP is enabled to clear the pre-existing approval backlog. Idempotent — a no-op once nothing is pending. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute(_SQL["approve_all_pending"]) await db.commit() return cur.rowcount async def list_all_users(self) -> list[dict]: """Return all users with access_status and total claim count, newest first.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute(_SQL["list_all_users_with_claim_count"]) as cur: rows = await cur.fetchall() return [dict(r) for r in rows] async def set_user_access(self, discord_id: str, status: str) -> bool: """Set access_status for a user. Returns True if a row was updated.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["update_user_access_status"], (status, discord_id), @@ -148,7 +141,7 @@ async def grant_role( """Grant a role to a user. Idempotent — re-granting an existing (user, role) pair is a no-op (returns False). Returns True when a new row was actually inserted.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["grant_role"], (discord_id, role, granted_by), @@ -158,7 +151,7 @@ async def grant_role( async def revoke_role(self, discord_id: str, role: str) -> bool: """Revoke a role. Returns True if a row was deleted (i.e. the user had it).""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["revoke_role"], (discord_id, role), @@ -168,7 +161,7 @@ async def revoke_role(self, discord_id: str, role: str) -> bool: async def list_roles_for_user(self, discord_id: str) -> list[str]: """All roles assigned to a single user, sorted for stable display.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute( _SQL["list_roles_for_user"], (discord_id,), @@ -179,7 +172,7 @@ async def list_roles_for_user(self, discord_id: str) -> list[str]: async def has_role(self, discord_id: str, role: str) -> bool: """Cheap (indexed) existence check — used on the hot path of the editor auth dep, so kept as a single-row SELECT rather than reusing list_roles.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute( _SQL["check_has_role"], (discord_id, role), @@ -196,7 +189,7 @@ async def create_role_request( user already has a pending request for this role (the partial unique index on (discord_id, role) WHERE status='pending' enforces it). Returns the new request's id.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["create_role_request"], (discord_id, role, user_note), @@ -231,8 +224,7 @@ async def list_role_requests( if status == "pending" else "ORDER BY rr.requested_at DESC, rr.id DESC" ) - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["list_role_requests"].format(where_sql=where_sql, order_sql=order_sql), params, @@ -244,8 +236,7 @@ async def get_role_request(self, request_id: int) -> dict | None: """Single-row fetch, same shape as list_role_requests entries. Used by the admin approve/reject endpoints for the 404 check + the role/discord_id payload that the grant flow needs.""" - async with aiosqlite.connect(self.path) as db: - db.row_factory = aiosqlite.Row + async with self._db(row_factory=True) as db: async with db.execute( _SQL["get_role_request"], (request_id,), @@ -262,7 +253,7 @@ async def review_role_request( ) -> dict | None: """Approve or reject a pending request. Returns the updated row, or None if no pending request with that id exists (already resolved, or unknown).""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["review_role_request"], (status, reviewed_by, admin_note, request_id), @@ -289,7 +280,7 @@ async def review_and_grant_role( directly between submit + approve), the INSERT OR IGNORE is a no-op and the request still transitions to approved. """ - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["review_role_request"], (status, admin_id, note, request_id), @@ -319,7 +310,7 @@ async def withdraw_role_request( """User-initiated cancellation. Scoped to the requester so one user can't withdraw another's request. Only pending requests can be withdrawn — historical rows stay immutable. Returns True if a row transitioned.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: cur = await db.execute( _SQL["withdraw_role_request"], (request_id, discord_id), @@ -337,7 +328,7 @@ async def user_has_capability_via_db( Single JOIN'd EXISTS query — indexed on both join keys. Doesn't consider admin (synthetic) or officer (dynamic) — those live in the auth dep on top of this primitive.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute( _SQL["check_user_has_capability"], (discord_id, capability), @@ -353,7 +344,7 @@ async def role_has_capability( Used by the auth dep to decide whether to bother running the dynamic officer check at all — if officers don't have the capability, no point.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute( _SQL["check_role_has_capability"], (role, capability), @@ -365,7 +356,7 @@ async def list_role_assignments(self) -> dict[str, list[str]]: Used by the admin user-list endpoint to join roles in without N+1 queries against ``list_roles_for_user``.""" - async with aiosqlite.connect(self.path) as db: + async with self._db() as db: async with db.execute(_SQL["list_all_role_assignments"]) as cur: rows = await cur.fetchall() out: dict[str, list[str]] = {} diff --git a/backend/server/parses/db.py b/backend/server/parses/db.py index 4745bcf8..7402f294 100644 --- a/backend/server/parses/db.py +++ b/backend/server/parses/db.py @@ -1,11 +1,11 @@ """ Normalized SQLite store for ingested ACT parses. -Mirrors the layout pattern of `census/recipes_db.py`: - * `_CREATE_*` SQL constants - * `init_db(path)` returns a connection with WAL/foreign-keys enabled - * idempotent `_MIGRATIONS` list for future schema bumps - * thin sync helpers for insert / lookup +All behaviour lives on :class:`ParsesStore` (the catalogue convention — +see backend/db_catalogue.py): ``store.init_db()`` returns a connection +with WAL/foreign-keys enabled (schema + idempotent ``_MIGRATIONS`` applied +via the base template method); the conn-taking insert/lookup helpers are +staticmethods — callers batch several operations per connection. Lives at `data/parses/parses.db` by default. Override with the `DB_PARSES_PATH` env var. @@ -131,11 +131,7 @@ def _create_schema(self, conn: sqlite3.Connection) -> None: conn.execute(_SQL["schema_attack_types"]) conn.execute(_SQL["schema_ingest_log"]) conn.execute(_SQL["schema_tamper_reports"]) - for stmt in _MIGRATIONS: - try: - conn.execute(stmt) - except sqlite3.OperationalError: - pass + self._apply_migrations(conn, _MIGRATIONS) self._migrate_attack_types_unique(conn) self._migrate_encounters_add_world(conn) self._migrate_ingest_log_add_world(conn) diff --git a/backend/sql_loader.py b/backend/sql_loader.py index df4815be..df14d711 100644 --- a/backend/sql_loader.py +++ b/backend/sql_loader.py @@ -30,9 +30,9 @@ Conventions: - One ``.sql`` file per Python module that has DML. Path mirrors the module: ``backend/eq2db/zones.py`` <-> ``backend/eq2db/zones.sql``. - - DDL (CREATE TABLE/INDEX) stays embedded in the ``.py`` next to the - ``init_db()`` migration code that runs it — keeping DDL and the - migrations that depend on it co-located beats hauling it out. + - DDL (CREATE TABLE/INDEX) lives in the ``.sql`` too, as ``schema_*`` + and ``indexes_*`` blocks run by the store's ``_create_schema`` hook + (see backend/db_catalogue.py) — one grep target for all SQL. - Block names are valid Python identifiers ([a-z_][a-z0-9_]*). The loader raises on duplicates so a typo can't silently shadow. """ diff --git a/tests/fixtures/parses_db.py b/tests/fixtures/parses_db.py index dfac3013..8638bc33 100644 --- a/tests/fixtures/parses_db.py +++ b/tests/fixtures/parses_db.py @@ -7,7 +7,7 @@ Two fixtures are exposed: - parses_db_path: yields a tmp_path / "backend.server.parses.db" with the schema - pre-initialised, AND monkeypatches parses_db.DB_PATH to point at it + pre-initialised, AND re-points parses_db.DB_PATH + the shared store at it for the duration of the test. This is what the web tests need. - parses_db_conn: yields an in-memory connection, the schema applied. @@ -30,7 +30,9 @@ def parses_db_path(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: """Tmp-path-backed parses.db with schema initialised + store re-pointed.""" db_file = tmp_path / "backend.server.parses.db" - monkeypatch.setattr(parses_db.store, "path", db_file) + # Re-point BOTH the module constant (metrics.py reads it dynamically) + # and the shared store instance (what the routes read). + monkeypatch.setattr(parses_db, "DB_PATH", db_file) monkeypatch.setattr(parses_db.store, "path", db_file) parses_db.ParsesStore(db_file).init_db().close() return db_file diff --git a/tests/fixtures/users_db.py b/tests/fixtures/users_db.py new file mode 100644 index 00000000..86b00dda --- /dev/null +++ b/tests/fixtures/users_db.py @@ -0,0 +1,34 @@ +"""Shared users.db test plumbing. + +The seven users.db domain stores each carry their own ``path`` (captured +from DB_PATH at import). Re-pointing users.db for a test therefore means +re-pointing the module constant AND every store instance — this helper is +the one place that knows that, so per-file loops can't fork (some copies +had drifted to skip the DB_PATH half). + +Usage in a fixture:: + + from tests.fixtures.users_db import point_users_db_at + + @pytest.fixture(autouse=True) + def _users_tmp(tmp_path, monkeypatch): + db_file = tmp_path / "users.db" + init_db(db_file) + point_users_db_at(monkeypatch, db_file) + return db_file +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from backend.server import db as users_db + + +def point_users_db_at(monkeypatch: pytest.MonkeyPatch, path: Path) -> None: + """Re-point the users.db module constant + every domain store at ``path``.""" + monkeypatch.setattr(users_db, "DB_PATH", path) + for store in users_db.ALL_STORES: + monkeypatch.setattr(store, "path", path) diff --git a/tests/server/test_aa_census_store.py b/tests/server/test_aa_census_store.py index 2ebc01c0..df15e065 100644 --- a/tests/server/test_aa_census_store.py +++ b/tests/server/test_aa_census_store.py @@ -196,8 +196,8 @@ def test_init_db_on_old_schema_adds_character_aas_table(tmp_path): db_path = tmp_path / "backend.census.db" # Create the DB with only the original tables (no character_aas). conn = sqlite3.connect(db_path) - conn.execute(cs._CREATE_CHARACTERS) - conn.execute(cs._CREATE_GUILDS) + conn.execute(cs._SQL["schema_characters"]) + conn.execute(cs._SQL["schema_guilds"]) conn.commit() conn.close() diff --git a/tests/server/test_admin_tamper_reports.py b/tests/server/test_admin_tamper_reports.py index 61a22ef7..eb9be8a6 100644 --- a/tests/server/test_admin_tamper_reports.py +++ b/tests/server/test_admin_tamper_reports.py @@ -37,12 +37,9 @@ def _seed_tamper_reports(now: int = 1700000000) -> list[int]: """Drop a fixed set of reports into the test DB and return their ids, newest-first so tests can refer to them by index.""" _wipe_tamper_reports() - # CRITICAL: pass the explicit path. init_db()'s `path: Path = DB_PATH` - # default arg is bound at function-definition time, so a later - # reassignment of parses_db.DB_PATH (which conftest does for test - # isolation) doesn't reach the no-arg call. The admin route uses - # `init_db(parses_db.DB_PATH)` (explicit), so the only way to read - # the same DB is to also pass explicit here. + # The shared store's `path` is re-pointed by conftest, and init_db() + # reads it at call time — so this argless call and the admin route's + # argless call hit the same (redirected) DB. conn = parses_db.store.init_db() ids: list[int] = [] try: diff --git a/tests/server/test_auth_tokens.py b/tests/server/test_auth_tokens.py index 03dd9125..b31fb58c 100644 --- a/tests/server/test_auth_tokens.py +++ b/tests/server/test_auth_tokens.py @@ -7,6 +7,8 @@ import pytest from httpx import ASGITransport, AsyncClient +from tests.fixtures.users_db import point_users_db_at + def _fake_session_user(request=None) -> dict: return {"id": "123456789", "username": "testuser"} @@ -159,8 +161,7 @@ def tmp_users_db(tmp_path, monkeypatch): # init_db creates schema; point every domain store at the temp path. users_db.init_db(db_path) - for _st in users_db.ALL_STORES: - monkeypatch.setattr(_st, "path", db_path) + point_users_db_at(monkeypatch, db_path) # Seed a user row so the FK on api_tokens.user_id is satisfied. import sqlite3 as _sqlite3 diff --git a/tests/server/test_db.py b/tests/server/test_db.py index 8603eab6..b1d94d3b 100644 --- a/tests/server/test_db.py +++ b/tests/server/test_db.py @@ -7,8 +7,6 @@ from backend.server import db from backend.server.db import get_display_names_for_discord_ids, upsert_user -_PATH = db.DB_PATH # redirected to pytest tmpdir by conftest.py - async def _seed(discord_id: str, discord_name: str) -> None: await upsert_user( diff --git a/tests/server/test_db_facade.py b/tests/server/test_db_facade.py new file mode 100644 index 00000000..4d9ae0d1 --- /dev/null +++ b/tests/server/test_db_facade.py @@ -0,0 +1,87 @@ +"""Guards for the backend.server.db facade (bound-method re-exports). + +The facade re-exports each domain store's bound methods under the original +free-function names. Two failure modes this file pins down: + + 1. Completeness drift — a new public store method that nobody remembers + to alias on the facade fails only at runtime, on the facade path. + ``test_facade_covers_every_public_store_method`` turns that into a + test failure with the missing name spelled out (intentionally + store-only names go in _FACADE_EXEMPT). + + 2. Alias integrity — every facade alias must be the bound method of the + store instance conftest re-points, or patches/repoints silently stop + covering production traffic. +""" + +from __future__ import annotations + +import inspect + +from backend.server import db as users_db + +#: Public store methods deliberately NOT re-exported on the facade — +#: consumers use the store instance (or the class) directly. +_FACADE_EXEMPT = { + # tokens: pure staticmethods used internally / via TokensStore + "generate_token", + "hash_token", + # servers: only the registry loader path uses it, via the store + "get_server_by_subdomain_sync", + # favorites + raid_schedule domains bypass the facade entirely + # (routes import their store instances directly) + "add_favorite", + "remove_favorite", + "count_favorites_for_character", + "is_favorited", + "count_user_favorites", + "list_favorites", + "get_schedule", + "replace_schedule", + "list_all_teams_with_twitch", + # base-class surface + "clear_caches", +} + + +def _public_methods(store) -> set[str]: + return {name for name, member in inspect.getmembers(type(store)) if not name.startswith("_") and callable(member)} + + +def test_facade_covers_every_public_store_method(): + missing = [] + for store in users_db.ALL_STORES: + for name in _public_methods(store) - _FACADE_EXEMPT: + if not hasattr(users_db, name): + missing.append(f"{type(store).__name__}.{name}") + assert not missing, ( + "Public store methods with no facade alias (add the alias in " + f"backend/server/db/__init__.py or add to _FACADE_EXEMPT): {sorted(missing)}" + ) + + +def test_facade_aliases_are_bound_to_the_shared_stores(): + stores = set(users_db.ALL_STORES) + bad = [] + for name in dir(users_db): + if name.startswith("_"): + continue + member = getattr(users_db, name) + bound_self = getattr(member, "__self__", None) + if bound_self is None: + continue # not a bound method (module, constant, plain function) + if isinstance(bound_self, type): + continue # classmethod-style binding — not a store alias + if bound_self not in stores: + bad.append(name) + assert not bad, f"Facade aliases bound to something other than an ALL_STORES instance: {bad}" + + +def test_exempt_names_are_actually_store_methods(): + """Keep _FACADE_EXEMPT honest — a renamed/deleted method must not + linger in the exemption list.""" + all_methods = set() + for store in users_db.ALL_STORES: + all_methods |= _public_methods(store) + stale = _FACADE_EXEMPT - all_methods - {"clear_caches"} + assert not stale, f"_FACADE_EXEMPT entries that match no store method: {sorted(stale)}" diff --git a/tests/server/test_favorites.py b/tests/server/test_favorites.py index 5ef3a79c..dc41e4ba 100644 --- a/tests/server/test_favorites.py +++ b/tests/server/test_favorites.py @@ -20,6 +20,7 @@ from backend.server.cache import favorite_count_cache from backend.server.db import init_db, review_claim, submit_claim from backend.server.db.favorites import store as fav +from tests.fixtures.users_db import point_users_db_at _TEST_SECRET = "pytest-session-secret-not-real-0123456789" @@ -47,11 +48,8 @@ def users_db(tmp_path) -> Path: @pytest.fixture(autouse=True) def _stores_at_tmp(users_db: Path, monkeypatch: pytest.MonkeyPatch): - """Point every users.db domain store at this test's temp DB.""" - from backend.server import db as _db_pkg - - for st in _db_pkg.ALL_STORES: - monkeypatch.setattr(st, "path", users_db) + """Point users.db (constant + every domain store) at this test's temp DB.""" + point_users_db_at(monkeypatch, users_db) async def test_add_remove_round_trip(users_db): diff --git a/tests/server/test_parses_ingest_client_warnings.py b/tests/server/test_parses_ingest_client_warnings.py index e6da6b7b..ec407c79 100644 --- a/tests/server/test_parses_ingest_client_warnings.py +++ b/tests/server/test_parses_ingest_client_warnings.py @@ -34,11 +34,10 @@ def _read_client_warnings(conn, encounter_id: int) -> str | None: def _ingest_and_get(payload: dict, *, tmp_path, monkeypatch) -> tuple[int, Path]: """Drive ``_ingest_payload_sync`` against a fresh tmp DB and return - (encounter_id, db_file). The db_file path MUST be passed explicitly - when reading back — ``parses_db.store.init_db()``'s ``path: Path = DB_PATH`` - default arg captures DB_PATH at function-definition time, so - monkey-patching the module attribute doesn't affect the no-arg call. - All reads in these tests therefore go through ``init_db(db_file)``. + (encounter_id, db_file). The shared store's ``path`` is monkeypatched + to db_file below, so both the ingest write and the read-back go + through the same redirected DB; reads construct ``ParsesStore(db_file)`` + where they want to be explicit. """ from backend.server.api.parses import IngestRequest from backend.server.api.parses.ingest import _ingest_payload_sync diff --git a/tests/server/test_raid_schedule.py b/tests/server/test_raid_schedule.py index d3a46491..873fcf9d 100644 --- a/tests/server/test_raid_schedule.py +++ b/tests/server/test_raid_schedule.py @@ -1,6 +1,7 @@ """Raid-schedule DB layer + API tests. -DB layer is exercised against a temp users.db (explicit ``path=``). The API is +DB layer is exercised against a temp users.db (stores re-pointed by the +autouse fixture). The API is tested with the ``app`` fixture: a signed session cookie for auth + mocked ``_officer_chars`` / db helpers (same pattern as test_item_watch_routes.py). """ @@ -19,6 +20,7 @@ from backend.server.db import init_db from backend.server.db.raid_schedule import store as rs +from tests.fixtures.users_db import point_users_db_at _TEST_SECRET = "pytest-session-secret-not-real-0123456789" @@ -48,11 +50,8 @@ def users_db(tmp_path) -> Path: @pytest.fixture(autouse=True) def _stores_at_tmp(users_db: Path, monkeypatch: pytest.MonkeyPatch): - """Point every users.db domain store at this test's temp DB.""" - from backend.server import db as _db_pkg - - for st in _db_pkg.ALL_STORES: - monkeypatch.setattr(st, "path", users_db) + """Point users.db (constant + every domain store) at this test's temp DB.""" + point_users_db_at(monkeypatch, users_db) async def test_replace_then_get_round_trips(users_db): diff --git a/tests/server/test_rankings.py b/tests/server/test_rankings.py index a65f7997..e70db9c5 100644 --- a/tests/server/test_rankings.py +++ b/tests/server/test_rankings.py @@ -382,6 +382,7 @@ def test_resolve_boss_uses_zones_db_for_raids(self): from backend.server.parses import db as pdb from backend.server.parses.models import Combatant, CombatantSnapshot, Encounter +from tests.fixtures.users_db import point_users_db_at def _ins(conn, encid, title, *, success, players, guild, duration): @@ -582,9 +583,7 @@ async def test_rankings_default_xpac_per_server(app, monkeypatch, tmp_path): # Point the DB at a temp file and seed a Wuoshi server row. p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) ServersStore(p).upsert_server_settings_sync( "Wuoshi", max_level=70, current_xpac="Echoes of Faydwer", launch_dt=None ) @@ -627,9 +626,7 @@ async def test_rankings_leaderboard_is_world_scoped(app, monkeypatch, tmp_path): # Register both servers so x-server: wuoshi resolves correctly. p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) ServersStore(p).upsert_server_settings_sync("Varsoon", max_level=50, current_xpac=None, launch_dt=None) ServersStore(p).upsert_server_settings_sync("Wuoshi", max_level=70, current_xpac=None, launch_dt=None) server_context.load_registry() diff --git a/tests/server/test_server_context.py b/tests/server/test_server_context.py index 558fe15d..ef151473 100644 --- a/tests/server/test_server_context.py +++ b/tests/server/test_server_context.py @@ -8,9 +8,7 @@ def _seed(monkeypatch, tmp_path): p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) sc.load_registry() return p @@ -62,9 +60,7 @@ def test_default_server_returns_is_default_server(monkeypatch, tmp_path): p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) # Set Wuoshi as the default in the DB. db.set_default_server_sync("Wuoshi") @@ -76,6 +72,8 @@ def test_default_server_returns_is_default_server(monkeypatch, tmp_path): import pytest from httpx import ASGITransport, AsyncClient +from tests.fixtures.users_db import point_users_db_at + @pytest.mark.asyncio async def test_middleware_sets_world_from_host(monkeypatch, tmp_path): diff --git a/tests/server/test_server_route.py b/tests/server/test_server_route.py index 2a16d2a9..a4834582 100644 --- a/tests/server/test_server_route.py +++ b/tests/server/test_server_route.py @@ -3,6 +3,8 @@ import pytest from httpx import ASGITransport, AsyncClient +from tests.fixtures.users_db import point_users_db_at + @pytest.mark.asyncio async def test_server_endpoint_reflects_subdomain(app, monkeypatch, tmp_path): @@ -10,9 +12,7 @@ async def test_server_endpoint_reflects_subdomain(app, monkeypatch, tmp_path): p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) server_context.load_registry() async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as c: r = await c.get("/api/server", headers={"host": "wuoshi.eq2lexicon.com"}) @@ -30,9 +30,7 @@ async def test_server_endpoint_unknown_host_defaults(app, monkeypatch, tmp_path) p = tmp_path / "users.db" db.init_db(p) - monkeypatch.setattr(db, "DB_PATH", p) - for _st in db.ALL_STORES: - monkeypatch.setattr(_st, "path", p) + point_users_db_at(monkeypatch, p) server_context.load_registry() async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as c: r = await c.get("/api/server", headers={"host": "localhost"}) diff --git a/tests/server/test_users_db_roles.py b/tests/server/test_users_db_roles.py index 5685ca84..3f00ebe4 100644 --- a/tests/server/test_users_db_roles.py +++ b/tests/server/test_users_db_roles.py @@ -38,6 +38,7 @@ user_has_capability_via_db, withdraw_role_request, ) +from tests.fixtures.users_db import point_users_db_at @pytest.fixture @@ -49,11 +50,8 @@ def db_path(tmp_path: Path) -> Path: @pytest.fixture(autouse=True) def _stores_at_db_path(db_path: Path, monkeypatch: pytest.MonkeyPatch): - """Point every users.db domain store at this test's temp DB.""" - from backend.server import db as users_db - - for st in users_db.ALL_STORES: - monkeypatch.setattr(st, "path", db_path) + """Point users.db (constant + every domain store) at this test's temp DB.""" + point_users_db_at(monkeypatch, db_path) async def _seed_user(db_path: Path, discord_id: str = "user-1", name: str = "TestUser") -> None: