feat(books): JSON content-negotiation for GET /books/bulk/step-edit (bookshelf-7hc1) #636
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-7hc1"
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
Adds
WantsJSONcontent-negotiation toGET /books/bulk/step-edit, the only remaining HTML-only page handler that should also serve JSON. ReturnsStepEditResponse{book, step_nav}forAccept: application/jsonrequests; HTML behaviour is completely unchanged.Route Audit Inventory (full scope of bookshelf-7hc1)
Already content-negotiates (HTML+JSON) — no change needed
GET /— homeGET /books,GET /books/{id}— books list/detailGET /authors,GET /authors/{id}— authorsGET /libraries,GET /libraries/{id}, etc. — librariesGET /shelves,GET /shelves/{id}— static shelvesGET /magic-shelves/{id}— magic shelvesGET /series,GET /series/{name}— seriesGET /stats,GET /notebook,GET /admin/audit-log— stats/notebook/auditGET /settings/*— all settings pagesGET /bookdrop— bookdrop proposals listGET /admin/workflows,GET /admin/workflows/{instanceID}— workflow admin (fixed in #614)GET /account,GET /admin/users,GET /admin/users/new,GET /admin/users/{id}/edit— usersJSON-only (correct, no page template) — no change needed
GET /reading-sessions— reading sessions APIGET /books/{id}/annotations— annotations APIGET /books/{id}/bookmarks— bookmarks APIGET /books/{id}/notes— notes APIGET /books/{id}/file/{fileID}/progress— progress APIGET /books/{id}/metadata/providers,/candidates,/llm-fetch/{job_id}— metadata APIsGET /libraries/{id}/scan-status— scan status pollingGET /admin/wf-status/{instanceID}— workflow status pollingBinary/protocol routes — intentionally not JSON
GET /books/{id}/file/{fileID}— file download (Content-Disposition: attachment)GET /books/{id}/file/{fileID}/content,/pages/{n},/audio— reader streamsGET /data/images/{id}/cover.jpg,thumbnail.jpg— cover imagesGET /authors/{id}/photo— author photoGET /auth/oidc/login,GET /auth/oidc/callback— OIDC auth flow redirectsHTML-only, intentional — documented, no code change
GET /login— login form (POST /login already handles JSON for API clients; GET is the browser form only)GET /books/{id}/read— reader iframe shell. This is intentionally HTML-only: the reader JS fetches underlying data (EPUB file, progress, bookmarks) via separate JSON sub-endpoints. There is no meaningful JSON payload for "open reader shell".HTML-only, fixed in this PR
GET /books/bulk/step-edit— ✅ this PR adds WantsJSON → StepEditResponse{book, step_nav}Test plan
StepEditHandler— JSON context: status 200,Content-Type: application/json, book title,step_nav.human_position,step_nav.total_countstep_edit_handler.gocoverage: 100%make coveragepassesmake buildcleanCloses bead bookshelf-7hc1 on merge.
Add WantsJSON branch to StepEditHandler so the multi-book metadata editor endpoint serves application/json when the client sends Accept: application/json, returning StepEditResponse{Book, StepNav}. HTML behaviour is unchanged. Tests: unit tests assert status 200, Content-Type, book title, and step-nav fields for both HTML and JSON branches. step_edit_handler.go maintains 100% coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>CODE REVIEW (RE-REVIEW): APPROVED
Re-review of #567 (bd-bookshelf-c15a.2) against prior findings in comment 6689.
Prior Finding Verification
MAJOR 1: ExternalID OR-dup loss — RESOLVED
The fix rewrites FindDuplicatesByExternalID using two independent EQUI-JOIN subqueries (hardcover and goodreads) merged with UNION ALL (store.go:278-381). Each provider query uses an equi-join with no OR. A book with both hardcover_id and goodreads_id appears exactly once per provider group.
The service.go seen-map key is now a (bookID, groupKey) composite (service.go:92) — so book A can appear in both hc:HC001 and gr:GR001 groups without being dropped. The e2e test seeds exactly the A+B (hc) / A+C (gr) scenario and asserts HaveLen(2). Finding is resolved.
MAJOR 2: Directory/Filename correlated-subquery — RESOLVED
Both FindDuplicatesByDirectory and FindDuplicatesByFilename now use derived-table EQUI-JOINs with no per-row correlated subquery (store.go:453-493 for directory, store.go:551-595 for filename). Migration 0037 adds idx_book_file_dedup_dir (book_id, is_book, file_sub_path) on book_file. All ORDER BY clauses use a two-column total order with unique tiebreaker. Finding is resolved.
MAJOR 3: Positive e2e tests for same_directory, filename, both-ids external_id — RESOLVED
All three are positive tests. Finding is resolved.
MINOR: Browser checkbox MustEval bool read — RESOLVED
e2e/browser/find_duplicates_test.go now uses titleAuthorCB.MustEval("() => this.checked").Bool(). Finding is resolved.
MINOR: Stray double blank line — RESOLVED
The extra blank line after the import block is gone. Finding is resolved.
Fresh-Eyes Pass
Multi-user/library scoping: Both new directory and filename finders filter by userLibraryIDs in the inner grp subquery AND the outer WHERE clause. Empty-slice fail-closed guard present. Correct.
Scale: No N+1. All new queries use derived-table joins. ORDER BY uses total-order two-column keys. LIMIT bound on all queries. Covering index on book_file (migration 0037). Clean.
Grimmory compat: Migrations 0036 and 0037 add indexes only, no new columns. Correct.
CSP: No inline style= in templates. CSS classes used throughout. Clean.
[MINOR] e2e/browser/find_duplicates_test.go:494 — single It block contains multiple Expect assertions
The browser spec chains four independent assertions (balanced chip active, strict chip active, titleAuthor unchecked, badge text). Project convention requires one Expect per It. Split into four It blocks. Does not block merge.
REVIEW VERDICT: 0 blocker, 0 major, 1 minor
Security Review — PR #636 (bookshelf-7hc1)
Reviewed per
.claude/rules/review-standard.md. Scope: content-negotiation JSON branch added toGET /books/bulk/step-edit.What was examined
internal/books/step_edit_handler.go— newWantsJSONbranchinternal/books/dto.go— newStepEditResponsestructinternal/books/books.go—Book/BookFilestruct field tagsinternal/books/service.go—Getfunction body (checkAccess + userID scoping)internal/books/routes.go— route registration + gate middlewareinternal/app/app.go— global middleware chain (AuthMiddleware)internal/users/middleware.go—AuthMiddlewarebehaviour for unauthenticated requestsNo findings
Per-user scoping preserved. The JSON branch reuses the exact same code path as the HTML branch:
userID = d.UserIDFromRequest(r)(sourced from the JWT session, not from the request), thend.Get(ctx, bookID, userID)which callscheckAccess(ctx, userID, id)— the single source-of-truth library-membership gate. A book outside the requesting user's libraries returns 404 before the JSON branch is reached.Auth gate preserved.
GET /books/bulk/step-editis not inisExempt;AuthMiddleware(global, wraps all routes) redirects browsers to/loginand returns 401 to JSON clients when no valid token is present. The route is not wrapped in an admin-only gate (consistent with the HTML path — this is a per-user library-read surface).No over-exposure.
StepEditResponsecontains onlyBookandStepEditNav. TheBookstruct is already fully serialised by the existingGET /books/{id}JSON endpoint (ShowResponse{Book: ...}).BookFile.FileSubPathwas already exposed there — no new surface.StepEditNavcontains only navigation URLs and position counters — no sensitive or internal fields.Pagination / LIMIT preserved.
StepEditHandlerfetches a single book atids[pos]; there is no list query and no unbounded result set. The IDs list is validated againstMaxBulkBookIDsbefore reaching this handler.No sync-protocol routes touched. Kobo/KOReader/OPDS routes are unmodified.
CSRF / method unchanged. This is a GET; no state mutation, no CSRF concern.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
49ee1630944e654ac798