feat: GET /books/{id} detail page + PATCH /books/{id}/status (bookshelf-fu9.1) #27
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-fu9.1"
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
GET /books/{id}with content negotiation (HTML + JSON)PATCH /books/{id}/statusto cycle read status (UNREAD/READING/READ)internal/bookspackage: curried service layer, handler, routesGetBook,ListBookFiles,ListBookAuthors,ListBookCategories,ListBookTags,UpdateBookStatustemplates/pages/books_show.htmlwith cover placeholder, file list, metadata table, series inforead_status_controller.jswith optimistic UI updatesinternal/books; integration tests ininternal/db; e2e tests ine2e/apiTest plan
make test— all pass, 100% coverage gatemake coverage— 100.0%make build— compiles without errorsmake integration(requires Docker)make e2e(requires Docker + Chrome)Closes bead bookshelf-fu9.1 on merge.
🤖 Generated with Claude Code
Security Review
Threat checks
ErrNotFoundtohttp.StatusText(404)onlyapp.go:160-165is unchanged, wraps all non-GET/HEADUPDATE book SET read_status = ? WHERE id = ?statement; atomicFindings
Clean / low-risk:
internal/db/sqlc/books.sql.go); every parameter is a?placeholder — no string concatenation.html/templateis used throughout; notemplate.HTML,template.JS, ortemplate.URLcasts exist inbooks_show.htmlor anywhere in the diff. All user-controlled fields (title, authors, description, etc.) are auto-escaped.read_status_controller.js) uses onlytextContentassignment — noinnerHTML, no DOM injection from server data. Content-Type is set toapplication/jsonon the PATCH fetch, matching what the handler expects.data-read-status-book-id-valueholds a numeric ID (rendered as{{.Book.ID}}),data-read-status-status-valueholds a DB-sourced enum — neither is rendered viadata-*into innerHTML.service.go:989checks against a closedmap[string]boolbefore any DB write; invalid values returnErrValidation→ 400.writeErrorfunction inerror_mapper.go:52-64responds withhttp.StatusText(status)only — raw error messages (includingsql.ErrNoRowstext) are never forwarded to the client.MaxBytesmiddleware wraps the full chain inapp.go(line 161); PATCH /status is covered.Notes (not blocking):
handler_test.go:800documents this).json.Decoder.Decodereturns a*json.SyntaxErrorwhich is not wrapped withErrValidation, so ErrorMapper maps it to 500. Consider wrapping decode errors withErrValidationindecodeStatusfor cleaner client experience. Low severity — the body is already bounded by MaxBytes and the client controls the malformed input.middleware.Wrapbut no structured log is emitted per-request at the books level. This is a logging hygiene gap, not a security issue./data/images/{{.Book.ID}}/cover.jpgreferences a route that is not yet registered inapp.go. Theimgtag will 404 silently (no server-side info leak), but note for future: when the/data/route is added, ensure it enforces the samedeleted=0check that the book query does.GET /books/{id}andPATCH /books/{id}/statuswill both need ownership/library-membership checks.Verdict
⚠️ Notes — no security blockers; two low-priority hygiene improvements (malformed-JSON 500→400, missing request logging) worth follow-up beads.
Code Review
Phase 0: DEMO Verification
No DEMO block was found in the bead comments (
bd comments bookshelf-fu9.1) or in the PR body. Per the review protocol this is a hard block. The implementer claimed 100% coverage and full test passage but provided no runnable verification command. Marking NOT APPROVED on this basis, with all findings below.Architecture compliance
routes.goowns route registration (notapp.go) —internal/books/routes.goregisters both routes;app.gocallsbooks.RegisterRoutes(...)internal/books— all tests use stub funcs, no sqlc callsGet(getBook, listFiles, ...)returnsfunc(ctx, id) (Book, error);UpdateStatus(updateStatus)returnsfunc(ctx, id, status) errorinternal/db/books_integration_test.gowith//go:build integrationinternal/booksuse stub funcs,Makefileadds./internal/books/...tomake testCorrectness
GetBookis one query; files/authors/categories/tags are each one query per book ID (5 total, none in a loop)PATCH /statusvalidates enum —validReadStatusesmap inservice.go:925checked before DB write; returnsErrValidation→ 400HasCoverflag wired —service.go:1122HasCover: row.BookCoverHash.Valid; templatebooks_show.html:18branches on{{if .Book.HasCover}}; CSS placeholder div used when NULL, no missing-asset image reference_nextStatusatread_status_controller.js:52), optimistic update + rollback on!resp.okor.catch, self-registers aswindow.ReadStatusController, loaded via<script defer>inbase.htmlFindings
Must-fix (Critical)
internal/books/handler.go:95— Malformed JSON body returns 500, not 400.decodeStatuswraps thejson.Decodeerror withfmt.Errorf("decode body: %w", err)but does NOT wrapmiddleware.ErrValidation, so the error mapper produces 500. The test athandler_test.go:800deliberately asserts500for malformed JSON — this is the wrong expected behaviour. A bad JSON body from a client is a client error (400), not a server error. Fix:return "", fmt.Errorf("decode body: %w", middleware.ErrValidation)(or wrap both).Minor
templates/pages/books_show.html:22—{{slice .Book.Title 0 1}}slices by byte, not rune. A book whose title starts with a multi-byte UTF-8 character (e.g. Chinese, Arabic) will produce an invalid UTF-8 byte in the placeholder. Use a helper or just use the first rune via a template func, or accept it as a known limitation for a future cleanup.templates/pages/books_show.html:18-19— WhenHasCoveris true the template renders<img src="/data/images/{{.Book.ID}}/cover.jpg">. This endpoint does not exist yet (bookshelf-24a is out of scope). The image will 404 in production until that epic is complete. Not a blocker but worth noting.e2e/api/books_test.go— TheseedBookhelper does not seed categories or tags, so the e2e JSON response will always have emptycategories/tags. No e2e assertion covers those fields. Minor gap; unit and integration tests cover it.internal/books/service.go:940—Getmakes 5 sequential DB calls (getBook, listFiles, listAuthors, listCategories, listTags). These could run concurrently witherrgroupfor latency, but the bead spec says nothing about it and the p99 target is 30ms for a single book. Not a blocker.Verdict
🛑 NOT APPROVED — No DEMO block provided (hard block per review protocol) + malformed JSON returns 500 instead of 400 (must-fix bug at
handler.go:95/handler_test.go:800).To re-submit: add a
DEMOblock to bead comments with a runnable command verifying the implementation, and fix the malformed-JSON 400 handling.Review findings addressed (commit
1a7d216)Must-fix:
decodeStatusnow wrapsjson.Decodeerrors withmiddleware.ErrValidationviafmt.Errorf("decode body: %w: %w", middleware.ErrValidation, err). Handler test updated to assert 400 (was 500). No e2e assertion for this path existed.Security hygiene:
*slog.LoggertoUpdateStatusHandler(curried pattern, same aslibraryhandlers). On success emitslogger.Info("book status updated", "book_id", id, "status", status, "trace_id", ...).app.gowiring updated to passlogger.Minor nits:
Book.Initialfield populated viaunicode/utf8.DecodeRuneInString(title)inrowToBook. Template now uses{{.Book.Initial}}instead of{{slice .Book.Title 0 1}}.seedBooknow inserts acategory+tagand maps them. Two newItblocks assertcategoriesandtagsappear in the JSON response.All tests pass, coverage 100%, CI green, mergeable: True.
@ -0,0 +1,43 @@package booksThis file should be called
books.goinstead ofdto.go.@ -0,0 +1,260 @@package books_testhttp handler tests should use https://onsi.github.io/gomega/#codeghttpcode-testing-http-clients
Addressed both review findings in
38ff5f2:internal/books/dto.go→internal/books/books.go— renamed viagit mv(history preserved).handler_test.goswitched to real HTTP roundtrip — replacedhttptest.NewRecorder+mux.ServeHTTPwithhttptest.NewServer+http.DefaultClient. Each Describe spins up its own server inJustBeforeEachand closes it viaDeferCleanup. Requests fire over a real TCP socket; assertions are made onresp.StatusCode,resp.Header, and the body bytes. One Expect per It is preserved.Note on
ghttp: I considered using Gomega'sghttppackage but it's designed for stubbing servers that your code calls into (mocking outbound HTTP), not for testing handlers. The Gomega-native improvement here is the real-roundtrip pattern with stdlibhttp.Client, not theghttpmatchers. If you intended the latter, happy to revisit.Drive-by: removed the
Fnsuffix from closure-scoped func vars (getFn→get,updateStatusFn→updateStatus) per the project convention (noFnsuffix on func values).routes_test.gostill hasgetFnbut was out of scope for this review.CI green, mergeable: true, coverage still 100% on
internal/.38ff5f236c344d43ac42