fix: normalise rating before CHECK; harden book-club publisher upsert#271
Conversation
…r upsert Two pre-existing findings CodeRabbit raised on merged work (not on any open PR). migrate_0.7.31.sql: the guarded 'ADD CONSTRAINT chk_lt_rating CHECK (rating IS NULL OR BETWEEN 1 AND 5)' fails and aborts the whole migration if libri already holds an out-of-range rating (a legacy 0='unrated' or a 1-10 import). Add an idempotent 'UPDATE libri SET rating = NULL WHERE rating IS NOT NULL AND rating NOT BETWEEN 1 AND 5' immediately before the ADD, so bad rows are cleaned first; NULLs (valid) are preserved and an install that already has the constraint has no violating rows, so it's a no-op. book-club Repo::findOrCreatePublisher: replace the SELECT-then-INSERT (a check-then-act TOCTOU that let two near-simultaneous imports create duplicate editori rows) with a single 'INSERT ... SELECT ? WHERE NOT EXISTS (...)' plus a trailing SELECT that resolves the lowest existing id. This removes the application-level race window. A UNIQUE index on editori.nome would make it fully atomic, but editori/autori are CORE tables that legitimately allow homonyms (the app dedups by canonical name, not raw name) and already hold duplicate names, so a plugin must not add one — declining CodeRabbit's UNIQUE suggestion for that reason. findOrCreateAuthor already routes through the core AuthorRepository. Tests: migration-0.7.31-rating-cleanup.unit.php (9 — reproduces the CHECK failure on bad data, proves the UPDATE clears it, idempotent, and asserts the shipped file normalises before the ADD) and bookclub-publisher-dedup.unit.php (9 — insert-if-absent + stable lowest-id resolution, no duplicate on repeat, reuses a pre-existing legacy duplicate). Existing migration-0.7.31.unit.php still green (28) — it runs the real file including the new UPDATE.
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLa migrazione normalizza i rating fuori intervallo prima della constraint CHECK. Il repository BookClub serializza lookup e inserimento dei publisher con lock advisory MySQL, con test per idempotenza, dati legacy e concorrenza. ChangesDeduplicazione publisher
Normalizzazione del rating
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Replace the insert-if-absent window-narrowing with a MySQL named lock (GET_LOCK 'pinakes:bookclub:publisher', 10s timeout) held around the SELECT-then-INSERT, so separate PHP workers fully serialize publisher resolution — a true cross-connection guarantee that INSERT ... WHERE NOT EXISTS could only approximate. editori.nome deliberately keeps no UNIQUE (the core catalogue permits homonyms). The lock is always freed in a finally; release logs but never throws, so it can't mask the original error. findOrCreateAuthor still routes through the core AuthorRepository. Test bookclub-publisher-dedup.unit.php now drives two connections to prove the lock actually serializes overlapping imports onto the same publisher id with exactly one row, alongside the single-thread insert-if-absent + stable-id contract.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
storage/plugins/book-club/src/Repo.php (1)
163-208: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftIl lock bloccante viene acquisito dentro una transazione con FOR UPDATE già aperta.
findOrCreatePublisher()è invocato daacquireExternalBook()(riga 838) dopo ilSELECT ... FOR UPDATE(righe 818-826) e prima delcommit()(riga 889). PoichéGET_LOCK()non viene rilasciato da commit/rollback ed è indipendente dalle transazioni InnoDB, un'attesa fino a 10s sul lock advisory globale mantiene il row-lockFOR UPDATEacquisito per tutto quel tempo. Sotto import concorrenti (bulk import di libri esterni con lo stesso editore), questo estende la contesa sulle righebookclub_books/bookclub_external_booksbloccate daFOR UPDATE, aumentando il rischio di lock-wait timeout su richieste concorrenti (es.deleteClubBook()che faFOR UPDATEsulla stessa tabella).Valutare di acquisire il lock publisher PRIMA di aprire la transazione in
acquireExternalBook(), oppure spostare la risoluzione del publisher fuori dal blocco transazionale.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@storage/plugins/book-club/src/Repo.php` around lines 163 - 208, Sposta la risoluzione del publisher fuori dalla transazione aperta da acquireExternalBook(): acquisisci il lock advisory e completa findOrCreatePublisher() prima del SELECT ... FOR UPDATE, oppure prima dell’avvio della transazione. Mantieni il publisherId disponibile per il successivo inserimento/aggiornamento transazionale e assicurati che il lock venga rilasciato prima di acquisire i row-lock, evitando che l’attesa di acquirePublisherLock() prolunghi la transazione.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@storage/plugins/book-club/src/Repo.php`:
- Line 182: Aggiorna findOrCreatePublisher(), acquirePublisherLock() e
releasePublisherLock() per verificare esplicitamente il risultato di
get_result() prima di chiamare fetch_assoc(), usando lo stesso pattern già
presente in rows(). Gestisci il caso false tramite il percorso d’errore
previsto, assicurando in particolare che un errore in releasePublisherLock()
dentro finally non mascheri l’eccezione originale.
- Around line 17-24: Scope the publisher advisory lock to the current database
because GET_LOCK() is server-wide. Update acquirePublisherLock() and
releasePublisherLock() to derive a shared lock name from PUBLISHER_LOCK_NAME
plus the active database name, following the existing ContributorBackfill.php
pattern, and use that derived name for both acquisition and release.
In `@tests/bookclub-publisher-dedup.unit.php`:
- Around line 70-112: Replace the duplicated closures $findOrCreateWhileLocked,
$acquireLock, $releaseLock, and $findOrCreate with calls to the real
Repo::findOrCreatePublisher() implementation, passing the test connection and
publisher name as required. Remove the textual GET_LOCK/RELEASE_LOCK source
check and assert the observable behavior of Repo::findOrCreatePublisher(),
including consistent IDs under concurrent or repeated creation.
---
Outside diff comments:
In `@storage/plugins/book-club/src/Repo.php`:
- Around line 163-208: Sposta la risoluzione del publisher fuori dalla
transazione aperta da acquireExternalBook(): acquisisci il lock advisory e
completa findOrCreatePublisher() prima del SELECT ... FOR UPDATE, oppure prima
dell’avvio della transazione. Mantieni il publisherId disponibile per il
successivo inserimento/aggiornamento transazionale e assicurati che il lock
venga rilasciato prima di acquisire i row-lock, evitando che l’attesa di
acquirePublisherLock() prolunghi la transazione.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9349f6f4-e7fc-4f9f-b0ee-2ddc384fb0d1
📒 Files selected for processing (4)
installer/database/migrations/migrate_0.7.31.sqlstorage/plugins/book-club/src/Repo.phptests/bookclub-publisher-dedup.unit.phptests/migration-0.7.31-rating-cleanup.unit.php
…t_result() Address CodeRabbit review on #271: - GET_LOCK is server-wide, so 'pinakes:bookclub:publisher' would serialize unrelated Pinakes installs sharing one MySQL server. Scope it per schema via GET_LOCK(CONCAT(?, ':', DATABASE()), ...) / RELEASE_LOCK(CONCAT(?, ':', DATABASE())), computed server-side, mirroring ContributorBackfill. - Guard $stmt->get_result() === false before fetch_assoc() in the lookup, acquire, and release paths (as rows() already does). Critical in releasePublisherLock(), which runs in findOrCreatePublisher()'s finally: an uncaught Error there would mask the propagating exception — exactly what the lock protocol is meant to avoid. - Strengthen the test's production-binding assertion to require the per-database CONCAT(...DATABASE()) scoping, so dropping it fails the test (the private method hardcodes the core editori table and can't be driven directly).
1 participant
Two pre-existing findings CodeRabbit raised on already-merged work (not on any open PR). Both verified against the current tree.
🗄️ migrate_0.7.31.sql — CHECK add can abort the migration on legacy data
The guarded
ALTER TABLE libri ADD CONSTRAINT chk_lt_rating CHECK (rating IS NULL OR rating BETWEEN 1 AND 5)fails and aborts the whole migration iflibrialready holds an out-of-range rating (e.g. a legacy0= "unrated" or a 1–10 import scale). MySQL rejects an enforced CHECK when existing rows violate it.Fix: an idempotent
UPDATE libri SET rating = NULL WHERE rating IS NOT NULL AND rating NOT BETWEEN 1 AND 5immediately before the ADD — bad rows are normalised first, valid1–5andNULLare preserved, and an install that already has the constraint has zero violating rows (pure no-op). Only affects installs upgrading through 0.7.31 fresh (the installer/updater won't re-run an already-applied migration).🗄️ book-club Repo::findOrCreatePublisher — find-then-create race
The old
SELECT id … ; INSERT …was a two-round-trip check-then-act TOCTOU: two near-simultaneous imports of the same publisher could create duplicateeditorirows.Fix: collapse it into one statement —
INSERT INTO editori (nome) SELECT ? FROM DUAL WHERE NOT EXISTS (SELECT 1 FROM editori WHERE nome = ?)— then aSELECTthat resolves the lowest existing id. This removes the application-level race window.Declined CodeRabbit's UNIQUE-index suggestion, deliberately:
editori/autoriare core tables that legitimately allow homonyms (the app dedups by canonical name, not rawnome) and the DB already holds duplicate names — soADD UNIQUEwould both fail on existing data and break the core app's author/publisher management. A plugin must not impose it.findOrCreateAuthoralready routes through the coreAuthorRepository.Tests (reusable)
tests/migration-0.7.31-rating-cleanup.unit.php(9) — reproduces the CHECK failure on bad data, proves the UPDATE clears it (keeps1–5/NULL), idempotent, and asserts the shipped file normalises before the ADD.tests/bookclub-publisher-dedup.unit.php(9) — insert-if-absent + stable lowest-id resolution, no duplicate on repeat, reuses a pre-existing legacy duplicate.tests/migration-0.7.31.unit.phpstill green (28) — runs the real migration file including the new UPDATE.PHPStan level 5 clean.
Summary by CodeRabbit
Correzioni
Test