fix(db): apply all 10 code-review findings from the store arc#157
Merged
Conversation
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 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The 8-angle review of #153–#155 surfaced 10 findings (no broken pipelines this round — the finders smoke-imported all consumers and diffed the facade exports programmatically); this fixes all of them.
Correctness
init_dbdefault-arg capture: the facade orchestrator keptpath: Path = DB_PATH— the exact import-time-binding pattern the arc eliminated everywhere else. Under the early-import race conftest documents, its argless call would run schema+migrations against the developer's realdata/users.db. Now readsDB_PATHat call time.PARSES_DB_PATH: the last import-time value copy of a store path becomes dynamicparses_db.pathreads, so per-teststore.pathre-points reach it.setattrline becomes theDB_PATHre-point its docstring always promised.Structure
AsyncStoreBase._db(): one asynccontextmanager replaces the 46 hand-rolledaiosqlite.connect(self.path)(+19row_factory) sites across the six async stores — the single future home for connection-level policy (busy_timeout / FK pragma once the data is audited).ServersStore(sync) moves ontoPathBoundso the async base's contract stays honest.BaseCatalogue._apply_migrations: replaces four drifted per-store migration loops (census logged at INFO, the other three swallowed silently); skips log at DEBUG uniformly so a genuinely broken migration leaves a trace..sqlconvention (newbackend/census/store.sql; inline schema/DML removed),FOREIGN_KEYScorrected toFalse(no FKs in the schema), andsql_loader's stale "DDL stays embedded in the .py" bullet updated to what the codebase actually does.[eq2db]→[db-catalogue]on the promoted base.Docs & tests
path=convention; the two inverted "CRITICAL" comments (which asserted the opposite of the code beneath them) now describe the store model; orphaned_PATHconstants removed.tests/fixtures/users_db.pypoint_users_db_at()replaces the ~9 pasted-and-already-forkedALL_STORESloops (the forked copies that skipped theDB_PATHhalf were the silent-wrong-DB risk).tests/server/test_db_facade.py(3 tests): completeness guard — a public store method with no facade alias fails CI with the name spelled out (intentional omissions in_FACADE_EXEMPT, itself checked for staleness); alias-integrity guard — every facade alias must be a bound method of anALL_STORESinstance. The three dead aliases the review found are deleted.Verification
1527 tests green (3 new), ruff + pyright clean. Live-verified
/api/server(ServersStore), raid-schedule (RaidScheduleStore via_db()), and the character read path on the dev backend.🤖 Generated with Claude Code