feat(series): composite identity (name + volume_number) + display name (bookshelf-uj5h.2) #932
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-uj5h.2"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
(series_name, volume_number)— distinct comic volumes of the same title (e.g. "X-Men (2011)" vs "X-Men (2021)") now appear as separate series instead of collapsing."Name (Year)"whenvolume_numberis set, bareNamewhen NULL (ebook/undated comics)./series/{name}?vol={year}for comic volumes;/series/{name}for name-only series. The?vol=param is parsed and validated (400 on bad input) and threaded togetDetail.idx_comic_metadata_book_volumeoncomic_metadata(book_id, volume_number)— covering scan for the series-list GROUP BY query.buildSeriesLinkandcomicDisplayVolumeNumberextracted from bothhandler.goandstep_edit_handler.go, eliminating the duplicate inline pattern.Test plan
make test— all packages passmake coverage— 100% gate passes, no new exclusionsmake lint— no new violations (31 errcheck issues shown are frombd-bookshelf-bbsd.8worktree, not this PR)?vol=2021→ parseVolParam success, volume passed to getDetail?vol=notanumber→ 400 Bad RequestCloses bead bookshelf-uj5h.2 on merge.
Series identity is now (series_name, volume_number) so distinct comic volumes of the same title (e.g. "X-Men (2011)" vs "X-Men (2021)") appear as separate series instead of being collapsed into one. Changes: - ListSeries groups by (series_name, cm.volume_number) with total-order cursor - ListSeriesCovers/ListSeriesAuthors keyed by SeriesKey{Name, VolumeNumber} - BooksInSeries/SeriesAllAuthors accept *int volumeNumber for scoped filtering - DetailHandler parses ?vol= query param (validation + pass-through to getDetail) - Series.DisplayName returns "Name (Year)" when VolumeNumber is set, bare name otherwise - series_index.html / series_index_fragment.html link to /series/{name}?vol={year} - series_show.html h1 uses DisplayName - buildSeriesLink/comicDisplayVolumeNumber extracted as package-level helpers (DRY) - Migration 0042: covering index on comic_metadata(book_id, volume_number) Closes bead bookshelf-uj5h.2.UI Screenshots — composite series identity
Series list:
X-Men (2011)andX-Men (2021)rendered as distinct rows (de-collapsed):Series detail page for
X-Men (2021)(/series/X-Men?vol=2021):CODE REVIEW — bookshelf-uj5h.2 / PR #932
Reviewed: composite series identity (series_name, volume_number), cursor pagination, library scoping, ?vol= validation, migration 0042, templates, test hygiene.
[MAJOR] templates/pages/series_index.html + series_index_fragment.html + series_show.html — No rendered screenshot posted to PR
The review-standard screenshot gate requires any PR touching
templates/to post a rendered screenshot captured with the go-rod browser harness (post-ui-screenshots-in-pr). PR #932 has no screenshot in the PR body or comments. The UI reviewer cannot verify that DisplayName renders as "X-Men (2021)" on cards, that?vol=links are formed correctly in the rendered anchor hrefs, or that theseries_show.htmldetail page heading is visually correct. Per policy: "A UI PR with no rendered screenshot is not review-ready."Fix: Capture and post screenshots of (a) the series list page showing at least two volumes of the same title with year suffixes, and (b) the series detail heading. The go-rod harness in the e2e suite is the canonical tool for this.
[MINOR] internal/series/store.go:170–178 — ORDER BY on
COALESCE(volume_number, 0)is technically non-total whenvolume_number=0exists alongside NULL for the same series_nameThe GROUP BY key is
(series_name, cm.volume_number), which correctly separates NULL from 0 into distinct groups. But the ORDER BY sorts both asCOALESCE(NULL, 0) = 0andCOALESCE(0, 0) = 0— identical positions for two distinct rows. The cursor predicates inbuildNameCursorHavingandbuildCountCursorHavingshare the same COALESCE collapse, so pagination would be undefined between these two rows. In practicevolume_number=0(year 0 AD) is not a valid comic publication year and cannot appear in real data, so this is not a real flake risk. Per the strict non-deterministic ORDER BY rule, the order is not provably total.Fix (choose one): (a) add a DB-level
CHECK (volume_number > 0)constraint oncomic_metadata.volume_numberso the data model documents the invariant and makes the ORDER BY total by construction; or (b) add a secondary sortcm.volume_number IS NOT NULL ASCafter the COALESCE column to separate NULL from 0 explicitly.[MINOR] templates/pages/series_show.html:69 —
series-titleCSS class added but has no rule in main.css<h1 class="book-detail__title series-title">introduces a class that has no matching rule instatic/css/main.css(confirmed:grep -r "series-title" static/css/returns nothing). This is a no-op today. If it is a semantic hook for future styling, add a comment; if it is leftover from development, remove it to keep the template clean.What I verified:
buildNameCursorHavingASC/DESC andbuildCountCursorHavingASC/DESC predicates correctly implement lexicographic ordering on (series_name, COALESCE(vol, 0)) and (count, series_name, COALESCE(vol, 0)) respectively. The three-column ORDER BY inbuildSeriesOrderClausematches the cursor predicate columns. HAVING uses the same COALESCE as ORDER BY — consistent.buildSeriesLibraryClause(nil → unscoped, empty → AND 1=0) is unchanged and present on every query path: series list, ListSeriesCovers, ListSeriesAuthors (both the list-level and the new{{pairs}}-based variant), BooksInSeries, SeriesAllAuthors. Composite key refactor did not regress any scoping path.parseVolParamatinternal/series/handler.go:330callsstrconv.Atoi; non-integer input wrapsmiddleware.ErrValidation→ 400. Absent param returnsnil, nil. The raw param value is never interpolated into SQL — it is parsed to*intand bound as a prepared-statement placeholder.CREATE INDEX ... ON comic_metadata(book_id, volume_number)), no column addition — Grimmory schema compat preserved. Down migration drops by index name. Index shape covers the LEFT JOIN oncm.book_idplus theGROUP BY cm.volume_numberprojection.style=attributes added;{{with .VolumeNumber}}?vol={{.}}{{end}}correctly omits the param for nil volumes.*_test.gofiles declarepackage series_testorpackage books_test. No unexported symbols referenced..golangci.ymlexclusions: diff of.golangci.ymlis empty.step_edit_handler.gooperation reorder: Fetching comic meta was moved BEFORE building the series link so thatcomicDisplayVolumeNumber(cd)operates on a populated struct rather than nil. Correct.REVIEW VERDICT: 0 blocker, 1 major, 2 minor
CORRECTION to my review (comment 12117): I incorrectly flagged missing screenshots as [MAJOR]. Screenshots were already posted in comment 12116 (at 18:41, before my review). I have now viewed both screenshots:
/series/X-Men?vol=2021): heading renders "X-Men (2021)" correctly. Canonical layout reused, no bespoke classes visible.Corrected verdict: REVIEW VERDICT: 0 blocker, 0 major, 2 minor
The [MAJOR] from the prior comment is retracted. Only the 2 [MINOR] findings stand (COALESCE total-order theoretical edge,
series-titleclass undefined in CSS). PR is APPROVED per review-standard (minor-only findings do not block).Security Review — PR #932 (bookshelf-uj5h.2)
Scope: composite comic series identity (
series_name,volume_number);?vol=URL param; new SQL grouping/cursor predicates; migration 0042.Multi-user / library scoping
Every enumeration and detail path was traced:
buildListSeriesQuerybuildSeriesLibraryClause→AND b.library_id IN (?)/AND 1=0ListSeriesCoverslen(userLibraryIDs)==0;AND b.library_id IN (?)ListSeriesAuthors(new)BooksInSeries(detail)AND b.library_id IN (?)inside subquerySeriesAllAuthorsAND 1=0;AND b.library_id IN (?)SeriesReadStatusWHERE user_id = ?from session; book IDs come from already-scopedBooksInSeriesshowLibraryIDsfromgetUserLibraryIDs(sessionUserID)userIDis always extracted from the session (extractUser(r).ID,d.UserIDFromRequest(r)), never from request params. No cross-user data leak found.SQL injection
All user-controlled values (
?vol=, series name, cursor fields, library IDs) reach the DB only as bound?parameters. Dynamic SQL clauses (ORDER BY, HAVING) use only allowlist-validated constants (SortDir→"ASC"/"DESC",SortKey→"name"/"count"). The{{pairs}}and{{libraryFilter}}template substituions in query strings expand to(?, ?)placeholders and?-parameterized IN-lists respectively — no user content concatenated.The LIKE prefix search (
q+"%") passes the user string as a bound param;%/_metacharacters in the term widen the match but cannot leak out-of-scope data (results remain library-scoped).Findings
[MINOR] internal/series/handler.go:97 —
parseVolParamaccepts arbitrarily large integers for?vol=strconv.Atoion a 64-bit platform accepts values up to INT64_MAX without error; the DB column is INT32. A value like?vol=2147483648parses successfully, finds no results (no match), and returns an empty page rather than a 400. No data leak, but a well-bounded year-range check (e.g., 1900–2100) would give a cleaner error response.[MINOR] templates/pages/series_index.html:14, series_index_fragment.html:2 —
{{urlquery .Name}}used in a URL path segmenturlqueryappliesurl.QueryEscape, which encodes spaces as+. Go's HTTP router does NOT decode+as a space inr.PathValue(...), so a series named "Batman Robin" generates the link/series/Batman+Robin, which the router hands to the handler asBatman+Robin— not matching the DB valueBatman Robin. The server-sidebuildSeriesLinkinstep_edit_handler.go:348correctly usesurl.PathEscape(space →%20). The templates should use a customurlpathhelper or a pre-escaped value from the server to stay consistent with the Go code. Series names without spaces are unaffected.No architecture boundary violations (
seriespackage imports no workflow engine). No secrets or PII in log paths..golangci.ymlunchanged. All test files usepackage series_test/package books_test(black-box). Migration 0042 is an index-only addition — no column schema change.REVIEW VERDICT: 0 blocker, 0 major, 2 minor
- CountDistinctSeries now counts composite (series_name, volume_number) groups to match the series list page grouping; sidebar badge now agrees with the "Showing N" count - Series card hrefs use {{urlpath}} (space→%20) instead of {{urlquery}} (space→+) so multi-word series links work in Go's PathValue() - parseVolParam rejects vol values outside [1000,3000] with 400 - ORDER BY gains (cm.volume_number IS NULL) discriminant making sort total; cursor predicates updated to three/four-level HAVING for name/count sort - Remove dead series-title CSS class from series_show.html h1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>CODE REVIEW: APPROVED
Re-review of the round-2 fix commits. All 5 tagged findings verified resolved. One new minor found.
Phase 0: DEMO
No standalone DEMO block (series page is a full-stack browser feature). CI is green and PR is mergeable — proceeding.
Phase 1: Fix verification
Finding 1 — Nav count now counts composite identity
internal/db/queries/nav_counts.sql+internal/db/sqlc/nav_counts.sql.goNew query does
GROUP BY bm.series_name, cm.volume_numberinside a subselect andCOUNT(*)over it — correctly counting distinct (name, vol) pairs, NULL-volume as its own group. sqlc doc comment is now correctly placed immediately above the function (prior format-mismatch resolved). Nav count carries no library scoping, but this was pre-existing on main before this PR. RESOLVED.Finding 2 — Series links use urlpath
templates/pages/series_index.htmlandseries_index_fragment.htmlBoth templates now use
href="/series/{{urlpath .Name}}{{with .VolumeNumber}}?vol={{.}}{{end}}". Regression test ininternal/series/handler_test.gorenders the real production template with "Batman Robin" and assertsContainSubstring("/series/Batman%20Robin")andNot(ContainSubstring("/series/Batman+Robin")). RESOLVED.Finding 3 — parseVolParam rejects values outside [1000, 3000] with 400
internal/series/handler.go:parseVolParamConstants minVolYear=1000 / maxVolYear=3000 in place. Three test cases: vol=2021 -> 200, vol=notanumber -> 400, vol=2147483648 -> 400 (overflow path). RESOLVED.
Finding 4 — ORDER BY made total via IS_NULL discriminant
internal/series/store.go:buildSeriesOrderClauseName sort:
bm.series_name <dir>, (cm.volume_number IS NULL) <dir>, COALESCE(cm.volume_number, 0) <dir>— 3-column total order.Count sort:
book_count <dir>, bm.series_name <dir>, (cm.volume_number IS NULL) <dir>, COALESCE(cm.volume_number, 0) <dir>— 4-column total order.Cursor HAVING predicates in buildNameCursorHaving and buildCountCursorHaving updated with volumeIsNull() helper. Store tests assert the IS_NULL discriminant present in both query and cursor args. RESOLVED.
Finding 5 — Dead
series-titleCSS class removedtemplates/pages/series_show.html:66h1 now uses
class="book-detail__title"and{{.Detail.DisplayName}}. RESOLVED.Phase 2: Code Quality
[MINOR] internal/series/store.go — stale doc comment in buildListSeriesQuery
, COALESCE(cm.volume_number, 0) " (2 columns) and "Sort=count: ... (stable three-column total order)" but the actual buildSeriesOrderClause produces 3 columns for name sort and 4 for count sort (both include the IS_NULL discriminant). The buildSeriesOrderClause function doc is correct; only the caller summary is stale. No correctness impact.The function doc comment says "Sort=name: ORDER BY bm.series_name
All 5 tagged findings resolved. No blockers, no majors.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Re-Review — PR #932 (bookshelf-uj5h.2)
Scope: focused re-review of the fix round —
CountDistinctSeriesnav_counts SQL rewrite,parseVolParam[1000,3000] guard, and URL path-escaping change.[MAJOR] internal/db/queries/nav_counts.sql:20 — CountDistinctSeries nav count is not library-scoped
The new
CountDistinctSeriesSQL query counts every distinct(series_name, volume_number)pair across all libraries, all users with noAND b.library_id IN (?)guard:The generated Go function signature is
func(ctx context.Context) (int64, error)— it accepts no user ID or library IDs, so the wiring inapp.go(middleware.WrapInt64(q.CountDistinctSeries, navCacheTTL)) cannot scope it per user. Every authenticated user sees the global series count in the nav sidebar regardless of which libraries they have access to. A user with access to zero series will see the total count for the whole installation. The series list page (ListHandler) is correctly fail-closed (userLibraryIDs threaded throughout), making the nav count contradictory and an info leak.Note: the pre-existing query on
mainhad the same scoping gap. The fix round rewrote the SQL (to count composite keys) but did not correct the scoping deficiency — it perpetuates rather than newly introduces it, but the hard rule requires it to be closed.Fix: mirror
CountBooksByShelves— add auserLibraryIDs []int64parameter (or auser_idjoin), applyAND b.library_id IN (?)withAND 1=0on empty-non-nil, and thread the resolved library IDs throughNavCountDeps.CountDistinctSeries→launchNavMetaCounts→ the DB call.parseVolParam [1000,3000] guard — CLEAN.
strconv.Atoihandles fullintrange (no int32 overflow); the [1000,3000] bounds check returnsErrValidation(HTTP 400); the?vol=parameter is the only user-controlled input and flows to a typed*intthat is never string-interpolated into SQL.URL path-escaping — CLEAN.
url.PathEscape(registered as{{urlpath}}) is applied to series names in templates andbuildSeriesLink. The hardcoded/series/prefix rules out open-redirect. Volume number is rendered viastrconv.Itoa(*int)— no injection vector.SQL injection in new queries — CLEAN. All WHERE predicates use bound
?parameters. ORDER BY is constructed from allowlistedSortKey/SortDirconstants validated inparseSeriesSortagainstvalidSortKeys/validSortDirsmaps before any interpolation. HAVING predicates are also fully parameterized.Architecture boundary — CLEAN. No domain package imports the workflow engine.
Secrets/PII — CLEAN. No tokens, credentials, or PII in new log paths.
REVIEW VERDICT: 0 blocker, 1 major, 0 minor
8c9714eb3b35f184cee3Security Re-Review: PR #932 (bookshelf-uj5h.2)
This is a focused re-review confirming the prior [MAJOR] finding (nav-count badge not library-scoped, cross-user info leak) is closed, and verifying the new per-user cache does not introduce a new vulnerability.
Findings
No new security issues found. The fix is correct. Detailed verification below.
Prior [MAJOR] — closed:
CountDistinctSeriesis now library-scoped. The SQL wraps the count in a subquery withbuildSeriesLibraryClauseinjected, which producesAND 1=0for authenticated users with zero library access (fail-closed) andAND b.library_id IN (?, ...)with bound params for non-empty sets. An authenticated user with an empty library set also hits an explicit early-returnreturn 0, nilin the curried function before any DB call. Both paths independently enforce fail-closed.Cache key correctness — verified:
librarySetKeymapsnilto the literal string"nil"and any non-nil slice to a sorted comma-separated decimal integer string viahealthKey. Key collision analysis:nil→"nil"(distinct from all numeric sequences)[]→""(empty join, distinct from"nil"and from any non-empty sequence since""cannot be produced by any non-empty integer list){1,2}→"1,2",{1,3}→"1,3"— different{12}→"12"vs{1,2}→"1,2"— different (comma is non-digit){2,1}→"1,2"same as{1,2}No collisions. Users with different library sets always get distinct cache entries. Users sharing the same library set share a cache entry — the count is identical for them, so sharing is correct.
Cache concurrency — verified: The outer
entriesmap is protected bysync.Mutexthroughout the lookup, creation, and eviction loop.c.Get(ctx)is called outside the lock with only a local pointer copy.NavCache.Getis independently synchronized with its ownsync.Mutexand handles concurrent fill via a single in-flight channel. No data race.Eviction bounds memory — verified: Entries with
lastGet.Before(t.Add(-2×TTL))are deleted on every call under the lock. The eviction loop runs in O(n) where n is the number of distinct library-ID sets seen recently — bounded in practice by the number of active users.userID from session, not request params — verified:
userID = extractUser(r).IDwhereextractUserreads from the JWT/session context set byAuthMiddleware. Library IDs are then fetched from DB keyed on that session userID viausers.GetUserLibraryIDs. Neither userID nor library IDs can be overridden from request params.SQL injection — clean: Library IDs are formatted as
?placeholders with bound args. Sort direction comes from a validatedvalidSortDirsallowlist enum, thenstrings.ToUpper— interpolated into SQL as"ASC"or"DESC"only. Cursor values (CursorName,CursorCount,CursorVolumeNumber) are always bound params. The?vol=handler param is parsed asstrconv.Atoiand range-checked to[1000,3000], used only to filter the series detail query as a bound param.Architecture boundary — clean: No domain package imports the workflow engine.
No secrets/PII in logs — verified.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
Code Review — bookshelf-uj5h.2 (PR #932)
Focused re-review of the nav-count scoping fix:
CountDistinctSerieslibrary-scoping +WrapInt64WithLibrariesper-user cache.nav_cache.go — WrapInt64WithLibraries
Cache key derivation (
librarySetKey) — correct and collision-free.nil→"nil"(3-char alphabetic string, unreachable by any decimal int64 set)[]int64{}→""(empty string, unreachable by any non-empty set)healthKey→ sorted, comma-joined decimal ints; two distinct sorted sets never collide.All three key spaces are disjoint. ✓
Eviction logic (2×TTL idle sweep) — correct.
e.lastGet = tis set BEFORE the sweep runs under the same lock, so the current entry's lastGet is neverBefore(cutoff)during its own eviction sweep — no self-eviction possible. Idle entries (those not accessed in 2×TTL) are deleted on the next call. Map growth is bounded. ✓Concurrency safety — correct. All access to
entrieshappens undersync.Mutex. After unlock,c.Get(ctx)runs onNavCache's own internal mutex (single-flight stampede protection). Even if a concurrent goroutine evicts an entry between the unlock and thec.Get(ctx)call, the locally capturedcpointer is still valid and returns a correct (if uncached) value — no data corruption. ✓No background goroutines —
WrapInt64WithLibrariesis fully inline; the only channel in the file isnavInflight.done(pre-existing single-flight mechanism). No resource leaks. ✓Deep-copy of ids slice in fill closure — correct. A fresh
fillslice is allocated andcopy'd before the innerNewNavCacheclosure captures it, so the caller's slice can't alias or mutate the cached key. ✓nav.go — launchNavMetaCounts scoping
GetUserLibraryIDsis called inside the goroutine using the request context. The goroutine is bounded bywg.Wait()inlaunchNavWave1, which holds before the response is written — context lifetime is guaranteed valid. ✓userID == 0path correctly falls back tonil(unscoped) for unauthenticated users, consistent with the existing design. ✓Nil-guard
counts.GetUserLibraryIDs != nilis correct: any existing wiring that pre-dates this field omits it and gets nil → falls back to unscoped (not a crash). ✓CountDistinctSeries query
The subquery groups by
(series_name, volume_number), matching the series list page's grouping. MySQL treats multiple NULLs as equal in GROUP BY, so non-comic books in a series share oneNULL-volume group — correct.COUNT(*)of the grouped subquery gives the right distinct-pair count. ✓Library scoping reuses
buildSeriesLibraryClause(same function asListSeries): nil → no filter, empty →AND 1=0(fail-closed), non-empty →AND b.library_id IN (...). TheCountDistinctSeriescurried function also has an early-return for the empty case (return 0, nilbefore the DB call), which is correct and slightly more efficient. ✓No new golangci.yml exclusions
.golangci.ymlis unchanged in this diff. ✓Test coverage
nav_cache_test.go—package middleware_test(black-box). ✓nav_test.go—package middleware_test(black-box). ✓store_test.go—package series_test(black-box). ✓ExpectperItthroughout all new tests. ✓nowfunc for deterministic time control. ✓REVIEW VERDICT: 0 blocker, 0 major, 0 minor
7a9c8bb91b3f0b20f111