fix(eq2db): apply all 10 code-review findings from the catalogue arc#152
Merged
Conversation
Adversarial review of #144-#151 surfaced three shipped breakages plus regressions and drift; this fixes the lot: Broken pipelines (CONFIRMED crashes, no pytest coverage): - scripts/download_spells.py + scripts/backfill_spell_effects.py still called catalogue.init_db(DB_PATH) against the new no-arg signature (TypeError). Both verified working now (backfill run end-to-end against a fresh temp DB). - scripts/dev/_smoke_test_zones_db.py had five stale module-level zones_db.find_by_name calls (AttributeError). Now runs to completion; it surfaces one pre-existing DATA staleness (bosses_total meta=56 vs 62 actual encounters — curator edits postdate the build stamp). Regressions: - SpellCatalogue.find_by_crc cache went lru_cache(maxsize=4096) → unbounded dict caching misses, fed by client ints via /aa/spell. Restored a 4096 bound with FIFO eviction (+ regression test). - BaseCatalogue._fetchall/_fetchone swallowed ALL OperationalError (incl. 'database is locked') as empty results. Now only the unbuilt-schema class ('no such table/column') degrades — logged at WARNING per CLAUDE.md levels — and real faults propagate again (+ tests for both behaviours). Hazards & drift: - zones model-layer path params: 31 `path: Path = DB_PATH` defaults (bound at import, unre-pointable by conftest, never exercised) become required keywords; ZoneCatalogue.__init__ keeps its default. - :memory: half-support removed from BaseCatalogue.init_db — reads guard on path.exists() so a memory catalogue silently returned empties; docstring now states why it's unsupported. - items.py _str_field/_int_field_zero re-implemented backend/census/_coerce helpers — now delegate to them like spells/recipes already do (_int_field keeps its 0→NULL variant). - New BaseCatalogue._find_exact_then_like: the duplicated exact-then-LIKE name search in spells/recipes now shares one implementation AND runs both queries on one connection (undoing the double-connect the #151 helper conversion introduced). - Log prefixes normalised to CLAUDE.md's [lowercase-with-hyphens]: [spells_db]→[spells-db], [recipes_db]→[recipes-db], [db]→[items-db]. 1524 tests green (3 new), ruff + pyright clean, live /aa/spell and recipe search verified on the dev backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This was referenced Jul 11, 2026
VortexUK
added a commit
that referenced
this pull request
Jul 11, 2026
…154) Second PR extending the catalogue architecture beyond eq2db (after CensusStore #153): - backend/server/parses/db.py: init_db collapses into the base template method (_create_schema hosts the six table schemas, the _MIGRATIONS loop, the three named migrations and indexes; FOREIGN_KEYS=True, CREATE_META=False). The 28 conn-taking helpers become staticmethods; invalidate_is_player_cache becomes an instance method over self.path. SwingType + the swing-type constants stay module-level. Shared `store = ParsesStore()` instance. - The hand-rolled :memory: special-case is gone (BaseCatalogue dropped it in #152 for silently-broken reads); tests that used init_db(":memory:") now use tmp_path files. The unused pragma_* SQL blocks are removed (base owns the connection pragmas); the world-migration's own FK toggle blocks stay. - Consumers (admin, parses delete/ingest/list/tamper, rankings, app startup, cleanup) import the instance as parses_db — call sites read the same, and the parses_db.init_db(parses_db.DB_PATH) explicit-default calls collapse to parses_db.init_db(). - conftest + the shared parses_db_path fixture re-point store.path alongside DB_PATH; test monkeypatches move from module DB_PATH to instance path; static calls route through the store. 1524 tests green, ruff + pyright clean. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
VortexUK
added a commit
that referenced
this pull request
Jul 11, 2026
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
An 8-angle adversarial code review of the catalogue rearchitecture (#144–#151) produced 10 verified findings; this PR fixes all of them.
Broken pipelines (CONFIRMED — crashed on invocation, invisible to pytest)
scripts/download_spells.py(×2) +scripts/backfill_spell_effects.pystill calledcatalogue.init_db(DB_PATH)against the new no-arg signature → immediateTypeError; the spells rebuild/backfill pipeline was dead since refactor(eq2db): convert spells module to SpellCatalogue class #144. Fixed and verified: backfill ran end-to-end against a fresh temp DB.scripts/dev/_smoke_test_zones_db.pyhad five stalezones_db.find_by_namecalls (AttributeError) — the documented zones.db refresh verification couldn't run since refactor(eq2db): convert zones module to ZoneCatalogue class #146. Now completes; it flags one pre-existing data staleness (build-timebosses_totalmeta=56 vs 62 actual encounters — curator edits postdate the stamp; the check itself compares a build stamp to a curator-editable table, worth a separate look).Regressions
find_by_crccache unbounded:@lru_cache(maxsize=4096)had become an unbounded per-instance dict that also cached misses (None), fed by arbitrary client ints viaGET /aa/spell/{crc}?tier=N. Restored a 4096 bound with FIFO eviction + regression test asserting eviction._fetchall/_fetchoneover-broad error swallow: allsqlite3.OperationalErrors (includingdatabase is locked) became silent empty results — wrong data served as 200s and potentially cached. Now only the unbuilt-schema class (no such table/no such column) degrades gracefully (logged at WARNING per CLAUDE.md's level guidance); locked-DB/disk/syntax faults propagate again. Tests cover both directions.Hazards & drift
pathdefaults: 31path: Path = DB_PATHdefaults (bound at import time — conftest's re-point can never reach them; every call site passes explicitly) are now required keyword params.ZoneCatalogue.__init__keeps its default.:memory:half-support removed: reads guard onpath.exists(), so a memory catalogue silently returned empties after a green-lookinginit_db. The docstring now states why it's unsupported (per-read connections would each get a fresh empty memory DB)._str_field/_int_field_zeronow delegate tobackend/census/_coerce(semantically identical; spells/recipes already used it) — Census edge-case fixes can't silently miss items ingestion anymore._int_fieldkeeps its items-specific 0→NULL variant.BaseCatalogue._find_exact_then_like: the exact-then-LIKE name search duplicated between spells and recipes is now one shared implementation, and it runs both queries on one connection — undoing the double-connect on the fallback path that the refactor(eq2db): base read helpers + dead code removal #151 conversion introduced.[lowercase-with-hyphens]:[spells_db]→[spells-db],[recipes_db]→[recipes-db],[db]→[items-db].Verification
1524 tests green (3 new: cache eviction bound, unbuilt-schema degrade, fault propagation), ruff + pyright clean. Scripts re-run for real (smoke test end-to-end, backfill against fresh DB, download import). Live
/api/aa/spell/{crc}and recipe search spot-checked on the dev backend.🤖 Generated with Claude Code