Mobile API: real catalogue languages endpoint + tolerant language filter (#282)#285
Mobile API: real catalogue languages endpoint + tolerant language filter (#282)#285fabiodalez-dev wants to merge 7 commits into
Conversation
… language filter Uwe reports (#282) that combining a language with any other filter (category, author, publisher) in the Android app returns no results. Root cause: libri.lingua is unnormalized free text (a dev catalogue holds "italiano", "inglese", "English", "Deutsch", "Français" side by side), but the app's language filter is a hardcoded predefined list. So the app sends e.g. "German" while the row says "Deutsch", the exact-match l.lingua = ? matches nothing, and any filter combined with language collapses to zero. - New GET /api/v1/catalog/languages returns the distinct language values actually present in the (non-deleted) catalogue, each with its book count, ordered by frequency. The app should populate its language filter from this — the real collection values — exactly as it already sources genres/authors/publishers, so a selected language always matches. Read-only, ETag-cached like the other catalog read endpoints; documented in OpenApi; one manifest row added to the idempotency guard. - The search language filter is now case/space-insensitive (LOWER(TRIM(l.lingua)) = LOWER(TRIM(?))) so a value echoed back from the endpoint matches regardless of casing/whitespace. Note: catalog search already supports sort (title/author), so the app's "sort by title/author" request in #282 needs only an app-side UI change; the language-filter fix needs the app to consume the new endpoint. Those app changes ship in the Pinakes-Android repo. Mobile API plugin 1.4.0 -> 1.4.1. Verified: mobile-api-idempotency.spec.js 43/43 (incl. the manifest-covers-every-route guard and 2x /catalog/languages ETag/304), PHPStan clean.
|
Warning Review limit reached
Next review available in: 45 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 Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughLa Mobile API aggiunge ChangesMobile API catalogo
Localizzazioni Book Club
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant CatalogController
participant Database
Client->>CatalogController: GET /api/v1/catalog/languages
CatalogController->>Database: recupera lingue normalizzate e conteggi
Database-->>CatalogController: risultati aggregati
CatalogController-->>Client: risposta 200 con ETag
Client->>CatalogController: richiesta con If-None-Match
CatalogController-->>Client: risposta 304 se l’ETag coincide
Possibly related PRs
Suggested reviewers: 🚥 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 |
…ion strings CodeRabbit flagged %n (a dangerous C sprintf specifier PHP rejects) in the bulk loan-extension count strings. They are only ever expanded via JS String.replace, so no crash occurs today, but %n is a footgun if ever passed to PHP sprintf/vsprintf. Switched to the standard %s (count) + %d (days), which sprintf accepts and the placeholder-parity check recognizes. Keeps all shared locales byte-identical across the open PRs.
Issue #282 asked for two things the tolerant-language work didn't cover: - Sort the catalogue list alphabetically by AUTHOR (title sort already existed; author sort was absent from sortSpec()). Added `author_asc` / `author_desc`, sorting by the same primary-author display subquery that fills the `autore` result column. The sort expression is wrapped in COALESCE(..., '') on both the ORDER BY and the keyset-anchor comparison so authorless books collapse to '' consistently — a raw NULL sort column would make the keyset predicate `NULL <cmp> ?` false and silently drop authorless books from every page after the first. Documented the two new values in the OpenAPI `sort` enum. - The reported filter bug was that a category/author/publisher filter COMBINED with a language returned zero. The tolerant language clause fixes the shared root cause, but there was no test for the combinations. Added section D (genre+language, author+language, publisher+language each return the matching book and exclude a same-facet book in another language) and section E (author sort asc/desc ordering + authorless-book pagination survival). mobile-language-filter-282.unit.php: 12 → 22 assertions, all green. php -l + PHPStan level 5 clean. No new endpoint, so no mobile-api manifest change. Note: the "German" vs stored "Deutsch" mismatch is resolved by the app sourcing values from /catalog/languages (this PR's endpoint) rather than a hardcoded list — that consumer change is on the Android side, not in this backend PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/mobile-api/src/Controllers/CatalogController.php`:
- Around line 525-533: Preserve the normalized language condition in the catalog
filter, and add the corresponding MySQL functional index on libri.lingua using
LOWER(TRIM(lingua)) for MySQL 8.0.13+ targets. Update the schema or migration
associated with CatalogController so the index is created consistently and named
idx_libri_lingua_norm.
- Around line 1105-1120: Aggiorna il docblock di sortSpec() rimuovendo o
correggendo l’affermazione che esclude intenzionalmente gli ordinamenti per
autore. Riduci inoltre le valutazioni ripetute della subquery usata da
$authorExpr nel SELECT, nel confronto keyset e nell’ORDER BY, adottando una
strategia che calcoli il nome dell’autore una sola volta per riga candidata e
riutilizzi quel risultato, preservando COALESCE, la selezione dell’autore
principale e l’ordinamento stabile tramite l.id.
- Around line 384-417: Handle a false result from the database query in the
languages endpoint instead of silently continuing with an empty result. In the
query flow within CatalogController, detect query failure, log the database
error consistently with search(), and return the existing internal_error 500
response; only build the successful 200 response when the query succeeds.
In `@storage/plugins/mobile-api/src/Controllers/OpenApiController.php`:
- Around line 557-579: Update the OpenAPI definition for the /catalog/languages
GET operation in OpenApiController to document the controller’s
conditional-response behavior by adding a 304 Not Modified response alongside
the existing 200, 401, and 500 responses. Match the description and
response-documentation pattern used by the /catalog/books/{id} operation.
🪄 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 Plus
Run ID: bac5dcf0-c502-46a5-9c17-f96cde2a5151
📒 Files selected for processing (11)
locale/de_DE.jsonlocale/en_US.jsonlocale/fr_FR.jsonlocale/it_IT.jsonstorage/plugins/mobile-api/MobileApiPlugin.phpstorage/plugins/mobile-api/plugin.jsonstorage/plugins/mobile-api/src/Controllers/CatalogController.phpstorage/plugins/mobile-api/src/Controllers/OpenApiController.phptests/code-quality.spec.jstests/mobile-api-idempotency.spec.jstests/mobile-language-filter-282.unit.php
| $language = isset($params['language']) ? trim((string) $params['language']) : ''; | ||
| if ($language !== '') { | ||
| $conditions[] = 'l.lingua = ?'; | ||
| // libri.lingua is unnormalized free text (#282): tolerate case / | ||
| // surrounding whitespace so a value taken from /catalog/languages | ||
| // matches regardless of how the client echoes it back. | ||
| $conditions[] = 'LOWER(TRIM(l.lingua)) = LOWER(TRIM(?))'; | ||
| $bind[] = $language; | ||
| $types .= 's'; | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Il filtro lingua normalizzato perde l'uso dell'indice su lingua.
LOWER(TRIM(l.lingua)) = LOWER(TRIM(?)) è corretto per risolvere #282, ma è un'espressione funzionale sulla colonna: MySQL non può usare un indice B-tree standard su lingua per questa condizione, quindi ogni ricerca filtrata per lingua richiede uno scan completo su dataset grandi. Se MySQL 8.0.13+ è il target, un indice funzionale mitiga il problema.
ALTER TABLE libri ADD INDEX idx_libri_lingua_norm ((LOWER(TRIM(lingua))));🤖 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/mobile-api/src/Controllers/CatalogController.php` around
lines 525 - 533, Preserve the normalized language condition in the catalog
filter, and add the corresponding MySQL functional index on libri.lingua using
LOWER(TRIM(lingua)) for MySQL 8.0.13+ targets. Update the schema or migration
associated with CatalogController so the index is created consistently and named
idx_libri_lingua_norm.
…docblock/cost, 304 doc
- CatalogController::languages(): a false result from db->query() silently
produced a 200 with an empty list, masking a DB failure as "no languages".
It now logs the db error and returns internal_error 500, mirroring search().
- sortSpec() docblock still claimed author sorts were "intentionally omitted";
corrected to describe author_asc/desc and note they are subquery-based (not
index-backed), so heavier than column sorts on a large unfiltered catalogue.
- Author sort ORDER BY now reuses the SELECT alias `autore` (COALESCE-wrapped
to keep the keyset's NULL handling) instead of repeating the correlated
subquery, so it is evaluated once per row for ordering rather than twice.
The keyset anchor still carries the full expression (a SELECT alias is not
visible in WHERE).
- OpenApi: documented the 304 Not Modified response on /catalog/languages,
matching the /catalog/books/{id} pattern.
Skipped the suggested functional index idx_libri_lingua_norm on
LOWER(TRIM(lingua)): expression indexes are MySQL 8.0.13+ only and MariaDB —
which Pinakes supports — does not accept that syntax, so it would break MariaDB
installs. The language clause is normally ANDed with an indexed
genre/author/publisher filter anyway.
mobile-language-filter-282 22/22, PHPStan level 5 clean.
Code reviewBranch: Found 4 findings across all lanes:
Deep lane — correctness & security✓ Auto-fixable (2)
Details and fix proposalsF001 — Mobile author_asc/desc sort diverges from the web catalog it claims to mirror: web sorts by author SURNAME (autore_cognome = SUBSTRING_INDEX(preferredSql,' ',-1), authorless LAST via IS NULL), mobile sorts by full displaySql() (first-name/pseudonym order) with authorless FIRST in ASC — different alphabetical order for the same author_asc key.File: Evidence:
Approach: Key the mobile author sort on the principal-author SURNAME (SUBSTRING_INDEX(preferredSql,' ',-1), ruolo='principale') and place authorless rows LAST in both directions via a composite keyset (nullflag ASC always, then COALESCE(surname,'') dir, then l.id dir), exposing autore_cognome as a SELECTed column for the cursor. Lighter alternative: swap only the sort expression to surname keeping the single-column machinery (fixes the surname divergence, leaves authorless-first-in-ASC). Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260724T115705Z5ed6d5): fixed and verified F002 — OpenAPI 'language' query-param description still reads 'Filter by ISO 639-1 language code', contradicting this PR's behavior (free-text lingua values from /catalog/languages, matched via LOWER(TRIM())): a developer following the doc would send 'de'/'en' and match nothing — the exact #282 bug.File: Evidence:
Approach: Rewrite the description to describe a free-text catalogue value sourced from GET /catalog/languages, matched case/whitespace-insensitively; optionally add example 'Italiano'. Docs-only. Files to modify:
Verification:
Edge cases to preserve:
Latest fix attempt (fixrun_20260724T115705Z5ed6d5): fixed and verified Fix runsRun
|
| Finding | Group | Outcome | phase_9_finding |
|---|---|---|---|
| F001 | FG-1 | ✓ fixed and verified | |
| F002 | FG-2 | ✓ fixed and verified |
🤖 Generated with Adam's Claude Code Review Command
Fix groups (committed): - [FG-1] F001 — CatalogController.php, mobile-language-filter-282.unit.php: verified. Align the mobile author sort with the web catalog: key on the principal- author SURNAME (SUBSTRING_INDEX(preferredSql,' ',-1), ruolo='principale') with authorless books LAST in both directions, via a composite keyset (nullflag always-ASC, COALESCE(surname,''), id) and a last_nf-carrying cursor; pre-fix author-sort cursors are ignored (treated as first page). The `autore` display alias is untouched. Test section E strengthened to assert authorless-last in both directions (22/22). - [FG-2] F002 — OpenApiController.php: verified. Rewrote the /catalog/search 'language' param description (free-text catalogue values from GET /catalog/languages, case/whitespace-insensitive) replacing the misleading 'ISO 639-1' claim; added example 'Italiano'. Post-fix review: 2/2 groups verified complete; 0 partial; 0 reverted.
1 participant
Backend part of #282 (HansUwe52's Android-app report). The app-side changes ship in the Pinakes-Android repo (linked separately).
Root cause
Uwe reports that combining a language with any other filter (category / author / publisher) returns no results.
libri.linguais unnormalized free text — a real catalogue holdsitaliano,inglese,English,Deutsch,Françaisside by side — but the app's language filter is a hardcoded predefined list. The app sends e.g.Germanwhile the row saysDeutsch; the exact-matchl.lingua = ?matches nothing, so any filter combined with language collapses to zero. (Category/author/publisher work because the app sources those from the collection.)Changes
GET /api/v1/catalog/languages— distinct language values actually present in the non-deleted catalogue, each with a book count, ordered by frequency. The app populates its language filter from this (real data), exactly as it already does for genres/authors/publishers, so a selected language always matches. Read-only, ETag/304-cached like the sibling catalog reads; documented in OpenApi; one row added to the idempotency manifest.LOWER(TRIM(l.lingua)) = LOWER(TRIM(?)), so a value echoed back from the endpoint matches regardless of casing/whitespace.The sort request
Uwe also asks to sort the home list by title/author.
GET /api/v1/catalog/searchalready supportssort(title/author, keyset-safe) — so that part is purely an app-side UI addition, no backend change needed.Verified
mobile-api-idempotency.spec.js43/43, including the manifest-covers-every-exposed-route guard and2× GET /catalog/languages(idempotent 200 + ETag/304).1.4.0 → 1.4.1.Summary by CodeRabbit
Nuove funzionalità
Correzioni
Documentazione