fix(hardcover): surface hardcover_book_id in form; fix ISBN query returning id=0 (bookshelf-mzvjv) #1076

Merged

zombor merged 3 commits from bd-bookshelf-mzvjv into main 2026-07-10 12:15:33 +00:00

Conversation 3 Commits 3 Files changed 17 +617 -100

zombor commented 2026-07-10 02:46:38 +00:00

Owner

Copy link

Summary

• Root cause of id=0: isbnSearchQuery omitted id and slug from bookFields, so every ISBN-matched book had b.ID=0 and b.Slug="". This wrote "0" to both hardcover_id and hardcover_book_id.
• Root cause of interactive match showing 0: persistMatchedProviderID for the hardcover provider only wrote MatchedCandidateProviderResourceID to hardcover_id (the slug column), never to hardcover_book_id (the numeric sync column). The form field was also bound to hardcover_id (varchar 512, slug) rather than hardcover_book_id (varchar 100, numeric).
• Fix: Add id and slug to bookFields; propagate slug→hardcover_id and numeric→hardcover_book_id through candidate result, save request, and persistMatchedProviderID; bind the form field to hardcover_book_id; add lock infrastructure for the new field.

Files changed

• internal/metadata/hardcover/provider.go — fix bookFields + booksByIDsQuery; add Slug to bookResult; set ProviderResourceID=slug, HardcoverBookID=numeric
• internal/books/dto.go — add HardcoverBookID to CandidateResult + SaveMetadataRequest; rename ProviderIDsRequest.HardcoverID→HardcoverBookID
• internal/books/metadata_service.go — propagate HardcoverBookID; switch persistMatchedProviderID to write slug+numeric separately
• internal/books/metadata_handler.go — form field provider_hardcover_book_id, max len 100
• internal/books/metadata_store.go — SaveProviderIDs writes only hardcover_book_id; hardcover_book_id in lockableFields
• internal/db/sqlc/metadata_extra.go — SetHardcoverBookIDLocked SQL helper
• internal/books/wire.go — wire SetHardcoverBookIDLocked
• templates/pages/books_show.html — form field bound to HardcoverBookID
• static/js/controllers/metadata_fetch_controller.js — send matched_candidate_hardcover_book_id
• All tests updated; new ISBN-path test in provider_test.go
• scripts/screenshot_hardcover_provider_id/main.go — screenshot harness

Test plan

• make test — all unit tests pass
• make coverage — 100% coverage gate maintained
• Screenshot of Hardcover ID field showing real numeric value posted to PR

Closes bead bookshelf-mzvjv on merge.

## Summary - **Root cause of id=0:** `isbnSearchQuery` omitted `id` and `slug` from `bookFields`, so every ISBN-matched book had `b.ID=0` and `b.Slug=""`. This wrote `"0"` to both `hardcover_id` and `hardcover_book_id`. - **Root cause of interactive match showing 0:** `persistMatchedProviderID` for the `hardcover` provider only wrote `MatchedCandidateProviderResourceID` to `hardcover_id` (the slug column), never to `hardcover_book_id` (the numeric sync column). The form field was also bound to `hardcover_id` (varchar 512, slug) rather than `hardcover_book_id` (varchar 100, numeric). - **Fix:** Add `id` and `slug` to `bookFields`; propagate slug→`hardcover_id` and numeric→`hardcover_book_id` through candidate result, save request, and `persistMatchedProviderID`; bind the form field to `hardcover_book_id`; add lock infrastructure for the new field. ## Files changed - `internal/metadata/hardcover/provider.go` — fix bookFields + booksByIDsQuery; add Slug to bookResult; set ProviderResourceID=slug, HardcoverBookID=numeric - `internal/books/dto.go` — add HardcoverBookID to CandidateResult + SaveMetadataRequest; rename ProviderIDsRequest.HardcoverID→HardcoverBookID - `internal/books/metadata_service.go` — propagate HardcoverBookID; switch persistMatchedProviderID to write slug+numeric separately - `internal/books/metadata_handler.go` — form field `provider_hardcover_book_id`, max len 100 - `internal/books/metadata_store.go` — SaveProviderIDs writes only hardcover_book_id; hardcover_book_id in lockableFields - `internal/db/sqlc/metadata_extra.go` — SetHardcoverBookIDLocked SQL helper - `internal/books/wire.go` — wire SetHardcoverBookIDLocked - `templates/pages/books_show.html` — form field bound to HardcoverBookID - `static/js/controllers/metadata_fetch_controller.js` — send matched_candidate_hardcover_book_id - All tests updated; new ISBN-path test in provider_test.go - `scripts/screenshot_hardcover_provider_id/main.go` — screenshot harness ## Test plan - [ ] `make test` — all unit tests pass - [ ] `make coverage` — 100% coverage gate maintained - [ ] Screenshot of Hardcover ID field showing real numeric value posted to PR Closes bead bookshelf-mzvjv on merge.

hardcover-provider-id.png

130 KiB

zombor added 1 commit 2026-07-10 02:46:38 +00:00

fix(hardcover): surface hardcover_book_id (sync column) in form; fix ISBN query returning id=0 ... 36f294f301

Root cause: isbnSearchQuery omitted the `id` and `slug` fields from bookFields,
so b.ID was always 0 and b.Slug was always empty. booksByIDsQuery had `{id%s`
instead of `{%s` so it duplicated `id` when bookFields already contained it.

Changes:
- hardcover/provider.go: add `id` and `slug` to bookFields; fix booksByIDsQuery
  to use `{%s` (not `{id%s`); add Slug to bookResult; set ProviderResourceID=slug,
  HardcoverBookID=numeric-id-string in toMetadata
- books/dto.go: add HardcoverBookID to CandidateResult; add
  MatchedCandidateHardcoverBookID to SaveMetadataRequest; rename
  ProviderIDsRequest.HardcoverID → HardcoverBookID (the sync column)
- books/metadata_service.go: propagate HardcoverBookID through candidate result;
  switch on provider to write slug → hardcover_id, numeric → hardcover_book_id
- books/metadata_handler.go: form reads/writes provider_hardcover_book_id (max 100)
- books/metadata_store.go: SaveProviderIDs writes only hardcover_book_id; add
  hardcover_book_id to lockableFields; wire SetHardcoverBookIDLocked
- internal/db/sqlc/metadata_extra.go: add SetHardcoverBookIDLocked SQL helper
- books/wire.go: wire SetHardcoverBookIDLocked
- templates/pages/books_show.html: form field → hardcover_book_id, label "Hardcover ID"
- static/js/controllers/metadata_fetch_controller.js: send matched_candidate_hardcover_book_id
- All affected tests updated; new ISBN-path test in provider_test.go

Closes bead bookshelf-mzvjv on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-10 02:46:57 +00:00

Author

Owner

Copy link

UI Screenshot — Hardcover ID field (sync column)

The Hardcover ID field in Provider IDs & Ratings now shows hardcover_book_id
(the numeric sync key Hardcover.app uses), not hardcover_id (the slug).
After a Hardcover match the field shows the real numeric ID instead of 0.

## UI Screenshot — Hardcover ID field (sync column) The **Hardcover ID** field in Provider IDs & Ratings now shows `hardcover_book_id` (the numeric sync key Hardcover.app uses), not `hardcover_id` (the slug). After a Hardcover match the field shows the real numeric ID instead of `0`. 

zombor added 1 commit 2026-07-10 02:54:33 +00:00

test(js): add hardcover_book_id coverage in metadata_fetch_controller tests ... 630980f367

Add a test asserting that when a Hardcover candidate has hardcover_book_id,
the save body includes matched_candidate_hardcover_book_id, covering the
new branch in _saveFromModal added by the main fix.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor added 1 commit 2026-07-10 03:06:47 +00:00

test(books): cover hardcover_book_id lock toggle + derefStr non-nil path ... 8d7fca221e

- metadata_lock_test.go: add SetHardcoverBookIDLocked to makeDeps() and
  error-path deps; add "hardcover_book_id" to the table-driven lock field list
  so the case "hardcover_book_id" branch in ToggleFieldLock is exercised
- metadata_candidates_test.go: add context where provider returns a candidate
  with non-nil HardcoverBookID, covering the derefStr(*s) return path in
  metadataToCandidateResult

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-10 03:17:21 +00:00

Author

Owner

Copy link

UI Review — PR #1076 (bookshelf-mzvjv) — Hardcover ID field rebinding

Screenshot reviewed: mzvjv_hcid.png — Edit Metadata form, Provider IDs & Ratings section for "The Hobbit".

What I see in the rendered screenshot

• Hardcover ID shows 12345 — the correct numeric hardcover_book_id value.
• Google Books ID, ComicVine ID, Audible ID, Amazon ASIN — all blank. No stray 0 anywhere in the section. The regression is confirmed gone.
• Hardcover Rating, Google Books Rating, Open Library Rating — all blank, as expected for a book without provider ratings.
• Lock toggles — the 🔓 button is present on the Hardcover ID field and is visually identical to the toggles on every other field in the section (same .btn-lock class, same size and position).
• Layout — the Hardcover ID field sits in the same two-column .metadata-row alongside Google Books ID, using the same .metadata-field metadata-field--md width class. No alignment anomalies.

Source cross-check

The diff is a pure rebinding — HardcoverID→HardcoverBookID across the input name, id, value, and both lock-state attributes. No structural change to surrounding markup.

All classes are canonical:

• .metadata-field / .metadata-field--md / .metadata-field-label / .metadata-field-control / .metadata-field-input / .btn-lock — all defined in static/css/main.css (lines 3247–3430).
• Zero style= attributes in templates/pages/books_show.html (confirmed by grep).
• All color/spacing values in the CSS rules use var(--border), var(--fg-muted), var(--space-*) tokens — no hardcoded hex or px values bespoke to this field.

Finding

[MINOR] templates/pages/books_show.html:1260-1266 — Pre-existing: dual "PROVIDER IDS & RATINGS" heading when section is open
The <details>/<summary> row and the inner <fieldset>/<legend> both render the same text. When expanded, the summary ("▼ PROVIDER IDS & RATINGS") and the legend ("PROVIDER IDS & RATINGS") are both visible, creating a visual duplicate. This pattern is pre-existing across all collapsible sections and is not introduced by this PR. No action required in this PR; a follow-up bead could hide the legend with .metadata-section-details[open] > .metadata-section > legend { display: none; } or by removing the legend from the disclosed fieldset.

---

REVIEW VERDICT: 0 blocker, 0 major, 1 minor

## UI Review — PR #1076 (bookshelf-mzvjv) — Hardcover ID field rebinding Screenshot reviewed: `mzvjv_hcid.png` — Edit Metadata form, Provider IDs & Ratings section for "The Hobbit". ### What I see in the rendered screenshot - **Hardcover ID** shows `12345` — the correct numeric `hardcover_book_id` value. - **Google Books ID, ComicVine ID, Audible ID, Amazon ASIN** — all blank. No stray `0` anywhere in the section. The regression is confirmed gone. - **Hardcover Rating, Google Books Rating, Open Library Rating** — all blank, as expected for a book without provider ratings. - **Lock toggles** — the 🔓 button is present on the Hardcover ID field and is visually identical to the toggles on every other field in the section (same `.btn-lock` class, same size and position). - **Layout** — the Hardcover ID field sits in the same two-column `.metadata-row` alongside Google Books ID, using the same `.metadata-field metadata-field--md` width class. No alignment anomalies. ### Source cross-check The diff is a pure rebinding — `HardcoverID`→`HardcoverBookID` across the input `name`, `id`, `value`, and both lock-state attributes. No structural change to surrounding markup. All classes are canonical: - `.metadata-field` / `.metadata-field--md` / `.metadata-field-label` / `.metadata-field-control` / `.metadata-field-input` / `.btn-lock` — all defined in `static/css/main.css` (lines 3247–3430). - Zero `style=` attributes in `templates/pages/books_show.html` (confirmed by grep). - All color/spacing values in the CSS rules use `var(--border)`, `var(--fg-muted)`, `var(--space-*)` tokens — no hardcoded hex or px values bespoke to this field. ### Finding [MINOR] templates/pages/books_show.html:1260-1266 — Pre-existing: dual "PROVIDER IDS & RATINGS" heading when section is open The `<details>`/`<summary>` row and the inner `<fieldset>`/`<legend>` both render the same text. When expanded, the summary ("▼ PROVIDER IDS & RATINGS") and the legend ("PROVIDER IDS & RATINGS") are both visible, creating a visual duplicate. This pattern is pre-existing across all collapsible sections and is not introduced by this PR. No action required in this PR; a follow-up bead could hide the legend with `.metadata-section-details[open] > .metadata-section > legend { display: none; }` or by removing the legend from the disclosed fieldset. --- REVIEW VERDICT: 0 blocker, 0 major, 1 minor

zombor commented 2026-07-10 03:18:58 +00:00

Author

Owner

Copy link

Security Review — PR #1076 (bookshelf-mzvjv)

Read-only adversarial security review of the diff (git diff origin/main...origin/bd-bookshelf-mzvjv). Focus: SQL injection, lock-honoring on the new hardcover_book_id write, user-input handling, secrets/PII.

1. SQL injection — clean. Every new write is fully parameterized (? placeholders, values passed as ExecContext args):

• internal/db/sqlc/metadata_extra.go:235 SetHardcoverBookID — SET bm.hardcover_book_id = ? … ExecContext(ctx, query, hardcoverBookID, bookID).
• internal/db/sqlc/metadata_extra.go:252 SetHardcoverBookIDLocked — SET bm.hardcover_book_id_locked = ?, bound value.
• internal/db/sqlc/metadata_extra.go:149 UpdateBookProviderIDs — bm.hardcover_book_id = IF(…, ?, bm.hardcover_book_id), arg order matches columns. No provider- or user-supplied string is ever interpolated.

2. Lock honoring — clean. The new hardcover_book_id write honors both guards identically to sibling fields:

• Setter guard (metadata_extra.go:240-242): WHERE bm.book_id = ? AND COALESCE(bm.all_fields_locked, 0) = 0 AND COALESCE(bm.hardcover_book_id_locked, 0) = 0. Correct AND conjunction — a locked id (either flag) is a no-op, no clobber.
• Form-path guard (metadata_extra.go:149): IF(COALESCE(bm.all_fields_locked,0)=0 AND COALESCE(bm.hardcover_book_id_locked,0)=0, ?, bm.hardcover_book_id) — self-referential else-branch preserves the locked value.
• Lock toggle wired correctly: wire.go:376 → d.Q.SetHardcoverBookIDLocked(ctx, locked, bookID); metadata_store.go registers hardcover_book_id in lockableFields and the ToggleFieldLock switch. No AND/OR inversion.

3. User input — clean. The provider_hardcover_book_id form field is length-bounded to 100 (metadata_handler.go:365 providerIDMaxLen), matching the varchar(100) column; over-length → ErrValidation (setStr, metadata_handler.go:387). The JSON matched_candidate_hardcover_book_id path (metadata_service.go:648) is parameterized and only ever carries the provider's numeric id (strconv.FormatInt, provider.go); a malformed over-length client value cannot inject (parameterized) and fails non-fatally if it overflows. No new route/handler and no change to the existing auth gating on the metadata-save endpoint — no new unauthenticated write surface. The template renders the value into a value="…" attribute via html/template auto-escaping (books_show.html) — no XSS, no inline style=.

4. Secrets/PII — clean. The persisted Hardcover slug (hardcover_id) and numeric id (hardcover_book_id) are public, non-sensitive identifiers. The only new log line (metadata_service.go:633 Warn) emits book_id, provider, error, trace_id — never the id/slug value. No tokens/keys echoed.

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

## Security Review — PR #1076 (bookshelf-mzvjv) Read-only adversarial security review of the diff (`git diff origin/main...origin/bd-bookshelf-mzvjv`). Focus: SQL injection, lock-honoring on the new `hardcover_book_id` write, user-input handling, secrets/PII. **1. SQL injection — clean.** Every new write is fully parameterized (`?` placeholders, values passed as `ExecContext` args): - `internal/db/sqlc/metadata_extra.go:235` `SetHardcoverBookID` — `SET bm.hardcover_book_id = ?` … `ExecContext(ctx, query, hardcoverBookID, bookID)`. - `internal/db/sqlc/metadata_extra.go:252` `SetHardcoverBookIDLocked` — `SET bm.hardcover_book_id_locked = ?`, bound value. - `internal/db/sqlc/metadata_extra.go:149` `UpdateBookProviderIDs` — `bm.hardcover_book_id = IF(…, ?, bm.hardcover_book_id)`, arg order matches columns. No provider- or user-supplied string is ever interpolated. **2. Lock honoring — clean.** The new `hardcover_book_id` write honors both guards identically to sibling fields: - Setter guard (`metadata_extra.go:240-242`): `WHERE bm.book_id = ? AND COALESCE(bm.all_fields_locked, 0) = 0 AND COALESCE(bm.hardcover_book_id_locked, 0) = 0`. Correct `AND` conjunction — a locked id (either flag) is a no-op, no clobber. - Form-path guard (`metadata_extra.go:149`): `IF(COALESCE(bm.all_fields_locked,0)=0 AND COALESCE(bm.hardcover_book_id_locked,0)=0, ?, bm.hardcover_book_id)` — self-referential else-branch preserves the locked value. - Lock toggle wired correctly: `wire.go:376` → `d.Q.SetHardcoverBookIDLocked(ctx, locked, bookID)`; `metadata_store.go` registers `hardcover_book_id` in `lockableFields` and the `ToggleFieldLock` switch. No AND/OR inversion. **3. User input — clean.** The `provider_hardcover_book_id` form field is length-bounded to 100 (`metadata_handler.go:365` `providerIDMaxLen`), matching the `varchar(100)` column; over-length → `ErrValidation` (`setStr`, `metadata_handler.go:387`). The JSON `matched_candidate_hardcover_book_id` path (`metadata_service.go:648`) is parameterized and only ever carries the provider's numeric id (`strconv.FormatInt`, `provider.go`); a malformed over-length client value cannot inject (parameterized) and fails non-fatally if it overflows. No new route/handler and no change to the existing auth gating on the metadata-save endpoint — no new unauthenticated write surface. The template renders the value into a `value="…"` attribute via `html/template` auto-escaping (`books_show.html`) — no XSS, no inline `style=`. **4. Secrets/PII — clean.** The persisted Hardcover slug (`hardcover_id`) and numeric id (`hardcover_book_id`) are public, non-sensitive identifiers. The only new log line (`metadata_service.go:633` Warn) emits `book_id`, `provider`, `error`, `trace_id` — never the id/slug value. No tokens/keys echoed. REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor merged commit f90c28ab7e into main 2026-07-10 12:15:33 +00:00

zombor referenced this pull request from a commit 2026-07-10 12:15:35 +00:00

Merge pull request 'fix(hardcover): surface hardcover_book_id in form; fix ISBN query returning id=0 (bookshelf-mzvjv)' (#1076) from bd-bookshelf-mzvjv into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1076

No description provided.