perf(cache): hot-dataset caching — book-detail DTO, reviews, bounded catalog (availability stays live) (#387) - #389
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…g pages — availability stays live (#387) Step 4 of the caching overhaul. Cache the remaining hot public datasets while NEVER caching real-time availability (a stale copie_disponibili is a double-loan bug). - Book-detail (book_detail_{locale}_{id}, TTL 300): the static DTO — book row minus copie_disponibili/copie_totali/stato, plus authors/publishers/series/related — is cached; availability is stripped before storage and re-read LIVE on every request (fetchLiveAvailability, PK IN(...) with deleted_at IS NULL) for the book and its related cards. A soft-deleted book 404s from the live read. - Reviews cached separately (book_reviews_{locale}_{id}), invalidated by ContentCache::reviewsChanged() from RecensioniRepository approve/reject/delete. - Bounded catalog listing pages cached only for the finite low-cardinality filter space (facet-cache bounding, page <= 10, canonical sort); rows availability- stripped, merged live per request, soft-deleted rows dropped. - Register book_detail_/book_reviews_ in QueryCache::NAMESPACE_PREFIXES; booksChanged() also bumps book_detail_ and already fires from every write-path and from DataIntegrity availability recompute, so catalog+detail invalidate on every loan-driven availability change. Autocomplete/search-preview deliberately NOT cached: SearchController returns live availability and the preview key would embed free text (unbounded key space = disk-fill vector the codebase forbids). Left live. Upgrade-safe: no migration/new config/new extension; cold cache = identical behavior. New tests/hot-dataset-cache-387.unit.php (26 checks) proves via real renders that a warm cache serves fresh availability while cached metadata stays. Full suite 139/139, PHPStan clean, soft-delete clean.
6988507 to
f20914f
Compare
|
Review manuale completa (CodeRabbit salta automaticamente le PR con base non-default). Correzioni in
Test: hot-dataset 30/30 su DB isolato; cache regressions 55/55; APCu 10/10; PHPStan pulito. Branch ribasato sulla PR #388 aggiornata. |
🔍 adamsreview --full — PR #389 (hot-dataset caching)Ran diff-local + structural/opus lenses over the stacked diff, then validated against the code. The availability-never-cached invariant holds — the real value here is a set of invalidation gaps: write-paths that mutate now-cached columns but don't bump the cache. These are regressions the caching introduces (before this PR every page rebuilt from the DB, so a missing invalidation was invisible). None are blockers (all bounded by the short TTL), but the cover/series ones are worth fixing. 🟠 F1 — cover updates don't invalidate the cache (medium)
🟠 F2 — series mutations don't invalidate book-detail (medium)The 🟡 F3 / F4 — lower-severity staleness (low)
✅ Verified clean (positive confirmations)
Verdict: mergeable. F1/F2 are real staleness regressions worth a follow-up (add the missing |
… profile-name changes (adamsreview #389, #387) Step-4 caching introduced staleness on write-paths that mutate now-cached columns but didn't bump the cache (before caching, every page rebuilt from the DB so the gap was invisible). Add the missing invalidations: - LibriController::fetchCover() (single) → ContentCache::booksChanged(); syncCovers() (bulk loop) → deferBooksChanged() (collapses to one bump at shutdown). copertina_url is in the cached book_detail_ DTO and catalog rows. - SeriesRepository mutations (assignPrimarySeries, updatePrimaryOrder, removeBookFromSeries, deleteSeries, renameSeries, mergeSeries) → deferBooksChanged(). collana/numero_serie and the sibling-volumes list are in the cached DTO, so a series change was leaving stale data on every sibling's page. Repository-level so every caller (CollaneController endpoints) is covered. - ProfileController profile update → ContentCache::reviewsChanged(); the cached reviews block stores CONCAT(nome,' ',cognome), so a name change showed the old reviewer name until TTL. Not fixed (deliberate): the z39-server SBN ids and frbr-lrm opera_id plugin writes land in the l.* DTO but are read by NO view (0 occurrences in book-detail.php), so invalidating them is a functional no-op today — left latent rather than touching two plugins for zero user-visible benefit. hot-dataset test 30/30, full suite 139/139, PHPStan clean.
The book-detail page cache refactor (#389) moved the DTO build into buildBookDetailStatic(), where the $collana series name is a local variable used only for the sibling query and never returned in the DTO. bookDetail() therefore left $collana undefined in the render scope, so the view rendered the "Nella stessa collana" section (driven by $seriesBooks, which IS in the DTO) with an empty series name for every book that belongs to a collana. Re-derive $collana in the render scope from the cached book row; the field survives stripLiveAvailability(), which removes only copie_*/stato. Fixes the deep-regression failure issue-75 test 6 (frontend same-series section) and the equivalent user-facing rendering regression.
1 participant
Step 4 of #387, stacked on #388 (uses its generation-key invalidation). Availability is NEVER cached — the design separates the static DTO (cached) from live copie_disponibili/copie_totali/stato (read fresh every request).
Cached, and how each invalidates
Not cached (deliberate)
Autocomplete/search-preview: SearchController carries live availability, and the preview key embeds free text -> unbounded key space = disk-fill vector the codebase forbids. Left live.
Safety / tests
No migration/new config/new extension; cold cache = identical behavior. tests/hot-dataset-cache-387.unit.php (26 checks) proves via real renders that a warm cache serves fresh availability while keeping cached metadata. Full suite 139/139, PHPStan level 5 clean, soft-delete guard clean.
Part of #387.