acbcc61ae4b14bb5d16fafb155f9b17e0b23bfb2
34
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 John O'Keefe
|
49b0b6eeb9 |
test(podcasts): integration suite + RLS auditor + contract widening (task 1.10)
cmd/server/tests/podcast_test.go — six suites, the shared-dev-DB doctrine (test*-named libraries, cascading cleanup, no binary fixtures): - TestPodcastRLSSurfaces: THE AUDITOR (invariant 13) — non-subscriber 404 on feed detail + SSR page (existence leak), episodes absent from generic media reads, admin escape, subscriber visibility, then the hidden-library flip hiding an already-subscribed feed (the feed policy's library-visibility join). Seeding runs as the FIRST ADMIN — the seed identity has neither subscription nor users row, so podcast INSERT..RETURNING (SELECT policy under FORCE) 42501s for it; production pods run as exactly that admin (invariant 12). All direct row probes read through the admin scope for the same reason. - TestPodcastAddFeedFlow: member add + subscribe + metadata-only upsert; duplicate-URL dedupe (no second row); allow_member_feeds gate (member 403 / admin 201 / subscribe-still-allowed). - TestPodcastShrunkenFeedRetention: feed purges an episode from the XML, both rows persist (upsert-without-delete pin, D14). - TestPodcastEditSegregation: generic edit 400 + write-to-file refusal for episodes; control book edit unaffected. - TestPodcastDownloadWorker (ffmpeg-generated tone at runtime): file lands at <download-root>/<feed-slug>/NNNN-…, audio_metadata probed; re-download of a downloaded episode never re-fetches (idempotency pin, enclosure-hit counted); dead enclosure → failed + persisted reason + metadata intact. Fetch/SSRF tests swap the client via SetPodcastHTTPClientForTests (the production client refuses loopback httptest servers by design — the real client gets its own negative test in services). - TestPodcastScannerExclusion: SetFolders on a podcasts library root yields zero scanner folders (invariant 4 pin) — MediaScanner .FolderCount() accessor added. Unit: internal/services/podcast_rss_test.go — namespace-aware parsing fixture (three XML namespaces, fallback chains, hosts/guests), duration spellings, RFC-822 variants, explicit variants, slug/path layout, enclosure-extension selection, stripHTML, and the SSRF matrix (private/loopback/link-local/multicast/mapped-v6 blocked; a loopback httptest server PROVES the production client refuses it). suites green: full integration run 161s / 0 failures; vitest 12 files 163 tests. Contract widening: TestContractPin_MediaListBothBranches' raw-row allowlist gains the 15 podcast episode columns (additive; documented in docs/contract_pins.md — inert defaults on scan rows). Bruno: bruno/ podcasts/ mirrors the §8 endpoints (12 requests) + Bookhoard env vars. tests/ui/COVERAGE.md rows 18-23: podcasts pages/lifecycle/RLS/chain — Playwright sweep pending. |
||
|
John O'Keefe
|
4d697f5fcf |
test(visibility): full suite green under RLS — sync paths, device-auth bridge, seed pool
Phase 5 (plan §7): the entire integration suite passes with the RLS policies live. The suite was the triage enumerator the plan predicted: - device_auth middleware: the THIRD identity bridge — KOReader/Kobo device-authenticated requests read media as the DEVICE OWNER (plan A9 called this path out; the flip was 404s on sync metadata). - conflicts/analytics/progress handlers: detached context.Background() reads threaded to the request context (the requesting user's identity) — 12+ sites; resolve helpers take ctx. - scanner: ScanFolders self-declares its configured admin identity (server maintenance, product-correct for any caller); generic job creation stamps the requesting admin. - harness: setup.DBPool and the sync-test pool declare rls.SeedIdentity at SESSION scope via BeforeAcquire — the pool never serves a second identity, so no A3 bleed; setup.DB falls back to the seed identity (NewSeedDB) for ungoverned-context calls; filter tests use seedExec for raw governed-table statements. - contract pin WIDENED (docs/contract_pins.md 2026-10-29): under RLS, item-scoped gates may answer 404 (row invisible — no existence leak) as well as 403 (Go-level checks on listings keep strict 403). - TestWorker_JobStatusTracking — the pre-existing red from the Phase 0 baseline — passes with the harness seed work; no product change. |
||
|
John O'Keefe
|
bb2cc26e9d |
feat(scanner): scanner entrypoints + generic jobs carry RLS identity; SeedDB harness fallback
The full suite enumerated the remaining triage class: tests drive the scanner directly (ScanFolders(context.Background())) and read seeded media via Queries with bare contexts — every governed read raised. - MediaScanner.ScanFolders self-declares its configured admin (SetAdminID — set by main AND the test helpers): a scan is server maintenance, product-correct regardless of caller. - ScopedDB gains a Fallback identity, exposed as NewSeedDB for the TEST HARNESS ONLY: setup.DB falls back to rls.SeedIdentity when the context carries none, so direct seeding/verification helpers work uniformly instead of per-site context threading. rls.SeedIdentity is the shared constant (no library_visibility rows => full visibility). - generic job creation (jobs.go) stamps the requesting admin into the job Context, same as the scan endpoint. The sync queue processor stays identity-free: sync_queue is not an RLS-governed table. |
||
|
John O'Keefe
|
922b92a8b6 |
feat(db): RLS library-visibility enforcement — invariant GREEN
The dashboard leak is closed at the database engine. The black-box
invariant (restricted user + hidden library => absent from every
media-returning surface) went RED->GREEN with this change set; clause-
protected surfaces never regressed, proving enforcement came from the
policies, not the test.
Phase 3 of plan ~/.opencode/plan/server-rls-library-visibility.md:
- schema.sql: idempotent RLS block — ENABLE + FORCE on media_items,
SELECT policy mirroring COALESCE(is_visible,true) exactly, permissive
per-command write stand-ins (FOR ALL would OR into SELECT).
- internal/rls + internal/database/scoped.go: ScopedDB declares the
acting user per statement (LOCAL GUC). DESIGN CORRECTION during
execution: the decorator never refuses — the ENGINE is the fail-loud
layer (policies raise on governed tables; auth lookups and settings,
ungoverned, simply work). A decorator refusal would have been a new
convention gate — plan B thinking — and broke login (chicken-and-egg:
credentials are checked before any identity exists).
- database/schema.go: the migration tx declares a synthetic identity —
schema.sql's dedup DO block SELECTs media_items, which raises once
policies are live (plan A11). Caught by the two-boot gate: boot 1
created the policies after the block ran; boot 2 raised.
- router.go: RLS bridge at BOTH c.Set("user") sites (JWT + device
auth) — echo-local c.Set never reaches the stdlib ctx the DB layer
receives.
- search: the search service's raw DBTX path receives the ScopedDB
itself, never the bare pool.
- harness: the server under test is wired exactly like main.go; the
seed handle is also ScopedDB-wired but carries seedCtx() — a
synthetic identity with no visibility rows — because sqlc :one
inserts use RETURNING, which evaluates the SELECT policy even on
INSERT. Regex UUID-scraping in the invariant replaced with a
structured JSON decode.
- invariant test: RED -> GREEN across every surface incl. View All;
403 pins and admin-scoping pins hold.
|
||
|
John O'Keefe
|
8c3d919aa0 |
wip(rls): engine-side fail-loud + harness seed identity [pin: work interrupted]
Design correction discovered by the auth chicken-and-egg: the decorator-level refusal was re-created a convention gate (plan B thinking). ScopedDB now NEVER refuses — it declares the GUC when the context carries an identity and declares nothing otherwise; the ENGINE is the fail-loud layer (policies raise on governed tables, ungoverned tables like auth lookups simply work). rls.WithInternal removed as a dead concept. - search service's raw DBTX path now receives the ScopedDB itself (main.go + harness) — a bare pool bypassed the GUC. - database/schema.go: migration tx declares a synthetic identity (schema.sql's dedup DO block SELECTs media_items; with policies live, boot 2 raised 'unrecognized configuration parameter' — A11). - harness: TestServerSetup.DB is a raw-pool SEED handle (write policies permissive, other seeded tables ungoverned); the server under test keeps the scoped wiring. 33 CreateMediaItem sites use seedCtx() — a seed identity with no visibility rows — because RETURNING requires SELECT visibility of the new row. - invariant test: regex UUID-scraping replaced with a structured JSON decode of the sections payload. INTERRUPTION NOTE: an edit to this same effect (seedCtx) was applied, confirmed by the tool, and subsequently vanished from disk while an 'opencode serve' process (PID 84551) is alive on the machine. This commit pins the tree against further loss; implementation is PAUSED pending the user killing that process. |
||
|
John O'Keefe
|
f4b581872f |
wip(rls): ScopedDB + rls package + schema policies + echo bridge [pin compiling state]
Phase 3 of the RLS plan, pinned mid-flight because a parallel writer
was actively editing cmd/server/tests/rls_visibility_test.go (A14):
the file flip-flopped between two implementations and gained a compile
error after the previous commit. This snapshot makes the whole tree
compile; functional verification (two-boot gate, invariant green,
full suite) lands in the next commit(s).
- internal/rls: typed ctx keys — user identity (bridged from echo by
the router) and an INTERNAL marker whose semantics grant nothing:
no GUC is declared, so policy'd tables still raise at the engine;
only RLS-ungoverned tables (system_settings, users counts) work for
user-less startup paths (settings registry Load, CountAdmins).
- internal/database/scoped.go: DBTX decorator — per-statement tx,
commit-on-Close (:many) / commit-on-Scan (:one), error paths always
terminate the tx, refusal before SQL when no identity and not
internal.
- database/schema/schema.sql: idempotent policy block (DROP IF EXISTS
+ CREATE — schema.sql re-runs every boot): ENABLE + FORCE, read
policy exactly mirroring COALESCE(is_visible,true), permissive
per-command write stand-ins (FOR ALL would OR into SELECT).
- router.go: RLS bridge at BOTH c.Set("user") sites (JWT + device
auth) — echo-local storage never reaches the stdlib ctx.
- main.go + test harness: database.New over ScopedDB at both wiring
sites; startup reads use rls.WithInternal.
|
||
|
John O'Keefe
|
8631aa00bf |
refactor: remove dead code across server, templ pages, and client modules
Deadcode-verified removals (RTA reachability from main, cross-checked with -test and textual xrefs): Go server: - whole files: middleware/transaction.go, middleware/error_handler.go, middleware/password_validator.go (live registration machinery kept), handlers/sync.go, sync/offline.go - unrouted endpoints: auth UpdateUsername/UpdateEmail, timezone settings handler, OPDS RegisterOPDS, Kobo LibrarySync handler, ReaderHandler.ShowReader, JSON-data twins on collections/library - duplicate book-matching handlers (book_matching.go Handler twins, matching.go MatchingHandler twins), superseded helpers (router.parseUUID, handlers.ValidateDeviceToken, normalizePath, userExists, generateLockID) Templ/client: - never-routed pages: api_explorer.templ, queue.templ and their client modules api-explorer.ts, api-explorer-docs.ts, queue.ts - fully orphaned wrapper modules events.ts, dom.ts - broken clearSyncQueue (called a nonexistent endpoint) and the hidden, never-populated sync-queue section on the devices page - 166+ unused exports demoted to private (knip-verified); ~85 truly dead declarations removed via tsc noUnusedLocals fixpoint - stale 'interactive API Explorer' claims dropped from docs The docs drift-gate package (internal/docs), templ-bound modules (dashboard, stats, header, index, login, profile*, bookshelf, collections, toast-error), sqlc-generated code, and web/static are intentionally untouched. |
||
|
John O'Keefe
|
3ed852515b |
test(helpers): setSystemSettingRestored — leave shared settings as found
The testuser-experience rule: the suite must leave shared settings exactly as it found them. setSystemSettingRestored snapshots the prior system_settings row, writes through the sanctioned API path (the only one that reloads the settings registry — raw SQL leaves the cache stale), and registers a cleanup restoring what WAS there, never a hardcoded value. setExtractorVersion now rides it, so the ev-pin tests no longer restore scan_extractor_version to a hardcoded 1: a deliberate dev bump of the extractor version survives a suite run instead of being silently cancelled (the live dev server obeys that setting on its next poll and would stop re-extracting parser fixes). The now-redundant explicit t.Cleanup restores are removed from TestExtractorVersionReextract and pinExtractorVersionOne. |
||
|
John O'Keefe
|
19b9c7c9b8 |
fix(tests): wire settings registry into the test auth handler — unstick TestAdminCreateUser
setupTestServer built the settings registry (and wired it into
systemSettingsHandler) but never called authHandler.SetSettings, unlike
main.go:112. The auth handler therefore ran with a nil registry and
defaultTimezone() took its nil-fallback branch, returning "UTC" for every
Register/AdminCreateUser in tests regardless of the system default_timezone
setting — so the
|
||
|
John O'Keefe
|
91b0c45dd4 |
feat(scanner): extractor-version (ev) mechanism — parser fixes reach old rows without a force rescan (faceted plan Phase 10)
Parser fixes (
|
||
|
John O'Keefe
|
4ee24e7152 |
feat(filters): the rule builder — composed engines over the field registry (faceted plan Phase 9)
- Field registry (services/filter_registry.go): ~55 typed fields — every book column, file/lifecycle facts, the three HOLDING STRUCTURES (library, collections with excluded-aware membership, series with FULL primary+alternate_info membership), and the whole user-relative class (status, progress, last read, my rating, review, notes/highlights presence, note/highlight text, time spent, pages read, collections, added by) reading the VIEWER's rows. Operators are typed constants; negation is an operator. - Composed engines (services/filter_query.go): one Go clause builder over raw pgx feeds the search (identical SearchMediaItemsUnifiedRow shape), the field-aware suggestions, and a true-COUNT live count. Mandatory WHERE head everywhere (family lens + integrity facts). Values are always bound parameters — no user string reaches a field/operator position, ever. - Wire: the qf JSON param (validated strictly — unknown fields/operators/malformed values are 400); qf wins over flat params on hand-crafted URLs; flat deep links keep working unchanged. - Endpoints: GET /api/media-items/filter-count (live count); field-values autocomplete upgraded in place — any registry field key, qf binds the OTHER rules (symmetric under-filter suggestions, zero-match values never offered); classic six without a tree keep their static pinned contract. - UI: the drawer becomes the rule builder — the ONLY filter UI (templ markup, Alpine tree state, sanctioned island; grouped field menus, operator menus with red negations, field-aware value menus, live count, chips per rule with red negations, URL rehydration, smart serialization flat-when-simple else qf). - Tests: composed semantics (AND/OR/negation/nesting/enums/bools/ numbers/dates/empty), every user-relative field class with the my-data weld (two readers, different worlds), note-text substring pin (catharsis ≠ Carthage), family lens (hidden libraries nowhere, 403s), suggestion-under-rules exactness, qf validation + precedence, flat-vs-tree equivalence; registry completeness unit test. |
||
|
John O'Keefe
|
188ae076ab |
refactor(utils): one PgTextString for the four pgtype.Text unwrappers
handlers textToString, router textToString, router getText, and utils normText were four copies of the same NULL-to-empty unwrap (99 call sites). One exported utils.PgTextString replaces them all; the private normText (comparison normalizer) stays but is now the odd one out. Harness fix found on the way: setupTestServer built a full wiring literal for route registration and then RETURNED a second, sparser literal that never copied AnnotationService — setup.AnnotationService was always nil on the struct every test receives. Nobody noticed until cross_library_move_test became the first caller (it previously used the now-retired plain CreateMediaHighlight query); it now exercises the real SaveHighlight path through the wired service. |
||
|
John O'Keefe
|
c579179fee |
refactor(annotations): AnnotationService is a required constructor dependency
H1's doctrine applied to the annotation service, which had been left on setters: NewMediaHandler/NewKoboHandler/NewKOReaderHandler/ NewSyncQueueProcessor(WithConfig) now take it as a parameter; the four SetAnnotationService setters are deleted; wiring updated (main.go, router/sync.go, test harness). With the service unskippable, the 18 nil-guards and their plain-query fallbacks die: - media.go: the create fallbacks for notes/highlights/bookmarks (the bookmark one was the H3 census's Bug #1 — it dropped percentage/chapter_reference the sync path carries), the highlight PUT fallback, and the three delete-path plain deletes (tombstone is now the only delete semantics). - kobo.go: both markup-ingest annotation loops (the bookmark fallback wrote the text into media_notes — a category error the fallback never should have had). - koreader.go: all three push-stack guards. - queue.go: the three "annotation service not available" errors in syncNote/syncHighlight/syncBookmark. Behavior note: the deleted highlight create/PUT fallbacks were the only paths passing note_id to the plain query — the sync service has no NoteID field, so the web's highlight→note link was already being dropped on the live path; this commit makes that pre-existing limitation explicit rather than papering it over (surfacing note_id on SaveHighlightRequest is left for a behavior commit that needs it). Pins (now 20 incl. the 3 annotation CRUD suites) + H1 tests + both unit suites green. |
||
|
John O'Keefe
|
8fc74c908a |
fix(conflicts): resolution writes funnel through SaveProgress — audio position survives
Batch H1 step 3b — THE data fix. Conflict resolution was the one live
erasure writer: applyProgressResolution and its verbatim twin
applyResolution both wrote the winner via a raw UpdateUniversalProgress
whose param list omits audio_offset_seconds/context_text/viewports, and
the SQL's DO UPDATE SET ... = EXCLUDED... NULLed exactly those. The web
UI resolves through POST /api/conflicts/:id/resolve and
/api/conflicts/bulk-resolve (main.js resolveConflict/bulkResolve), and
the reader's mark-as-read fires a resolution first when the reader SSR
stamps data-conflict-id/-winner — so every resolution today erased the
audiobook position and reader context.
Now: ProgressService is a required ConflictHandler dependency; BOTH
call sites (single resolve + bulk resolve) share one
applyProgressResolution that funnels through SaveProgress (the verbatim
twin is deleted). Deliberate choices, per the plan:
- SkipConflictDetection on the resolution write: both flows apply
BEFORE the conflict row is marked resolved, so the funnel's LWW
detector would otherwise file a duplicate pending conflict
(HasRecentConflictResolution is false at apply time).
- Source "conflict_resolution" — the row's last-write attribution; the
winner stays durably recorded in resolution_data.winner.
- Broadcast: true — live devices jump to the winning position.
- The winner snapshot maps percentage/epubcfi/context_text/chapter/
character/page/total_pages; a winner without a CFI clears the locator
pair (ClearLocator) instead of re-pairing with the loser's stale
anchor. Everything the snapshot doesn't carry SURVIVES via the LWW
seed: audio_offset_seconds above all. chapter_progress keeps its
stored value (was overwritten with the book percentage), an absent
percentage survives (was written as 0%).
- Accepted: the resolution captures one reading_history session
(attributed to conflict_resolution; the 2h idle ceiling applies).
Verification: TestH1_ConflictResolutionPreservesAudio drives the real
flow — web push with audio 300s, diverging push from a second source
detects a conflict, resolve with the second source as winner, GET shows
percentage 0.3 AND audio_offset_seconds 300 (previously NULLed), and no
duplicate pending conflict remains. Full contract pin suite green.
The host-run TestConflict*/TestProgress* failures are environmental
(container-only /app/uploads path + db hostname; fail identically at
baseline
|
||
|
John O'Keefe
|
e83c00f869 |
refactor(progress): ProgressService required at construction; delete the 9 unreachable erase-fallbacks
Batch H1 step 3a. The ceiling census (exhaustive UpdateUniversalProgress
grep) found 9 nil-guarded `if progressSvc != nil { funnel } else { raw
erase-write }` fallbacks — the survey's "four" under-counted kobo.go,
which has FOUR sites (Markup ReadingSync, Markup last-read-place,
AnalyticsGettests, SyncFromServer), plus one each in media.go, queue.go
and koreader.go. All unreachable: the service is created unconditionally
at main.go:127 and wired into every handler and the queue processor; the
test harness wires them too.
The erasure pattern is now unrepresentable:
- NewMediaHandler / NewKOReaderHandler / NewKoboHandler / both
NewSyncQueueProcessor flavors take the ProgressService as a REQUIRED
constructor arg; the SetProgressService setters are gone.
- Every fallback body (raw UpdateUniversalProgress with explicit-NULL
pgtypes for viewport/scroll/panel/reading_mode/zoom and no
audio_offset_seconds/context_text) is deleted; each write funnels
through SaveProgress with the LWW seed, so fields the writer doesn't
know survive.
- Removed the now-dead koreader pg-type helpers (textPtrToPgText,
intPtrToPgInt4, int64PtrToPgInt8) and their self-tests.
Behavior deltas (stored-row content, no wire shape changes —
KOReader/Kobo response pins stayed green):
- KOReader fallback used to write explicit NULL viewports and drop the
context_text the koplugin sends; the funneled mainline already maps
book.ContextText.
- Queue flushes no longer overwrite chapter_progress with the book
percentage (bogus data; preserved via the seed).
- A web PUT without percentage no longer writes 0% via the deleted
fallback; percentage seeds from the stored row.
Contract suite: KOReader×4 + Kobo×3 pins green.
|
||
|
John O'Keefe
|
6457fbaf72 |
feat(stats): reading statistics — session capture wired + /stats page + widget + per-book card
The design parked in the handoff (multi-tier stats off reading_history; bookorbit-style in data, not looks) is in. The real prerequisite work was data: reading_history existed but NOTHING wrote to it — the whole stats feature was sitting on an empty table. - CAPTURE (no client changes, no repo-boundary crossings): every successful progress write through SaveProgress (KOReader + app + web + Kobo + offline queue — the one chokepoint) now records one reading_history session row: span = the interval since the PREVIOUS write instant, pages = positive page delta when both sides know the page index, marker rows for first contact. Spans over 2h are kept as zero-second markers (overnight suspend→resume, offline flushes must not inflate totals) — the honest 'sync cadence, not a stopwatch' caveat stays. device_id only when a REAL device row matches (the web/app progress path historically stuffs the USER id into DeviceID — probe instead of trusting it; FK violation would silently drop every capture). Source preserved in device_metadata. - API: /api/stats/summary?range=7|30|90|year (totals, zero-filled daily arrays, streaks, most-active day/weekday, top books, library breakdown — one call), /api/stats/this-week (widget: rolling 7d vs previous + streak), /api/stats/books/:id (lifetime + recent sessions). All bucketed in the user's timezone; finished ≡ any push ≥0.995; visible-library lens enforced at source. Legacy /api/analytics/* kept untouched. Range windows anchor end-of-day to the START of today (blasting the clock time into tomorrow resolved 'today' a calendar day late — found by the streak test). - /stats page (sidebar Stats between Progress and Devices, previously /analytics was an orphan): range selector, five stat cards, two Chart.js v4 charts (daily minutes bar, pages/day line — chunked via dynamic import so main.js stays untouched), per-library breakdown, most-read list, empty-state explainer. /analytics now 301s to /stats. The orphaned analytics.templ + analytics.ts removed (their containers and the page JSON never matched; the page was astub). - Dashboard 'This Week' widget (server-rendered, suppressed on service failure) and the book detail 'Your Reading Stats' card (lifetime totals of the viewer's own sessions) complete the tiers: per-user default, per-library breakdown inline, per-book on the page, per-system/admin aggregate parked as before. Tests: statistics_test.go (capture semantics at the chokepoint, tz/day buckets, zero-fill, streak math, finished detection, range validation, auth gate, widget, per-book lifetime). Full suite PASS. |
||
|
John O'Keefe
|
740213f0a4 |
fix(auth): new users inherit the system-wide default timezone
default_timezone existed as a system setting (admin-editable, exposed via SettingsRegistry.DefaultTimezone) but nothing read it at user creation — the CreateUser query didn't even carry the column, so every new user got the DB default UTC regardless. Both creation paths (public register + admin create) now seed the user's timezone from the setting, validated with time.LoadLocation and falling back to UTC when unwired/empty/invalid. Test-infra fixes surfaced by the run: TestUpdateProfileSelfEdit was self-editing the SHARED admin (renaming it past the harness cleanup's preserve-list, leaking a second admin row that broke the last-admin tests) — it now edits a dedicated user with a unique name; the last-admin tests get an ensureSingleAdmin precondition helper; the websocket collection name is run-unique (non-test names persist in the shared DB and UNIQUE(user_id,name) 409s on rerun). |
||
|
John O'Keefe
|
9cd6356892 |
feat(server): ghost-session guard — JWTs for deleted users stop authenticating
The JWT middleware validated signature + expiry and fabricated the request user purely from token claims, so a signature-valid token for a DELETED user kept full API access until token expiry — observed live on hardware (a phone session survived the test suite's user-table recreation and kept serving its true empty sets; open handoff bug 1). An ensureUserExistsMiddleware already existed but was only wired on the frontend SSR group (frontend.go) — every /api group used bare createJWTMiddleware. The existence check now lives INSIDE createJWTMiddleware's SuccessHandler so every protected group (SSR and API alike) gets it: - After claim validation, GetUser by the token's user_id; a missing row clears the token cookie and answers 401 user_not_found for API calls (the app's TokenAuthenticator then re-logins with stored credentials) or redirects to /login?session=invalid for page requests — same shape-aware split the error handler already uses. - Cost: one primary-key lookup per authenticated request (~0.2ms local); instant revocation, no TTL semantics. The redundant SSR-only middleware is deleted (its redirect behavior is preserved inside the guard). The guard exposed two processing-issues tests that were unknowingly RELYING on ghost semantics: they nested a second setupTestServer whose bootstrap deleted the first setup's regular user, then asserted 403 from the dead token. De-nested: a createLibraryDirect helper inserts the library under the live setup's user, no second bootstrap. New TestDeletedUserTokenRejected pins the contract: token works while the user exists -> 401 user_not_found after deletion. Full suite: 219 groups, zero failures. |
||
|
John O'Keefe
|
f24a2d6c22 |
test(server): fixture collisions fixed, stale contract asserts re-anchored, gofmt — suite now fully green
Closes the remaining integration-suite failures (handoff open bug 3),
line by line:
- Fixture family (the upsert equivalence): tags_filter_test
("Science Fiction" found 1 of 3; "Fantasy" 404'd on zero results),
media_item_isbn_test (normalization read back the first case's ISBN
everywhere; edge cases saw a nil ISBN; pagination collapsed 5
creates onto 1 row), search_unified_test (its 3 books were actually
1 row + two no-op creates) — all POSTed a constant file_path into
one library while CreateMediaItem upserts ON CONFLICT
(library_id, file_path). Every fixture now uses unique per-item
paths, the suite's universal convention elsewhere. TestTagsFilter
confirmed same-family: counts came right with no second bug.
- New TestMediaItemCreateDuplicatePath pins the honest-create
contract from
|
||
|
John O'Keefe
|
208fc2c4d6 |
fix(server): kobo markup panic — wire LibraryService into the test harness, guard typed-nil resolver
TestProgressWeb_KoboIntegration 500s'd on every markup with a last-read-place bookmark: the panic-recovered stack was a nil pointer dereference in LibraryService.ResolveMediaPath (s.db deref), reached from KoboHandler.convertKoboCFIToStandard during kepub CFI conversion. Two-layer bug: 1. Test harness (root cause): the router.Config built in test_helpers_test.go omitted LibraryService, so router/sync.go's SetLibraryService(cfg.LibraryService) stored a nil *services.LibraryService in the interface field. Production wiring (main.go:246) was always correct. 2. Latent trap (hardening): `h.libraryService != nil` at the convertKoboCFIToStandard call site can never catch a typed nil — an interface holding a nil *LibraryService is non-nil. ResolveMediaPath now degrades with a "library service not configured" error instead of panicking on a nil receiver; callers already handle the error by skipping CFI conversion. Harness-only fix, zero production behavior change: the endpoint now returns a clean 200 with the position saved wherever kepub conversion is unavailable instead of a panic-recovered 500. Verified: TestProgressWeb_KoboIntegration passes all three subtests (401 unauthenticated, ReadingSync→last-read-place percentage preservation, last-read-place without prior ReadingSync) and on immediate rerun — previously it failed every run. |
||
|
John O'Keefe
|
843f35bbfc |
test(server): runs leave no residue — auto-cleanup for every resource the suite creates
Symptom: repeated full-suite runs poisoned the dev database with leaked rows (duplicate Prefs Scope libraries, colliding device names, saved-filter name conflicts on rerun) because tests created rows but never deleted them, and stale-binary runs left rows the new cleanup could not match. - test_helpers: library creation now registers t.Cleanup DELETE (the leak root cause — createTestLibraryWithFolder never cleaned up). Suite-wide cleanup passes for libraries/collections/devices/ saved-filters run both at setup (pre-pass against legacy residue) and in t.Cleanup (no per-run residue). Preserve the admin's real libraries; delete only convention-named test rows. - setup seeds canonical system collections every run keyed on query_type (idempotent — heals renames/stale tombstones), moved out of the preserved-admin branch so it runs for every suite start. - dashboard_integration: scope-testing library renamed into the test-row convention; ordering assertions on the restored "Want to Read" collection preserved. - filters_test: filter names moved into the convention (rerun-safe — name-uniqueness 409 no longer trips second runs); five .(string) type assertions panic-proofed with require.True guards (a panic here used to kill the whole test process mid-run, which is why a stack of "never ran" failures surfaced at once later). - device_test: device names moved into the convention (ILIKE %test% hit them; duplicate-device-name conflicts gone). |
||
|
John O'Keefe
|
e7ccd7b222 |
test(dashboard): align test seed naming with the canonical Title-case set + fix stale expectations
The test helper createDefaultCollectionsForUser carried its own kebab-case copy of the default system collections — drifted from the canonical Title-case list the registration seed and RestoreSystemCollection use. Every suite run against the shared dev DB re-created a kebab set beside the canonical one (the duplicate sections on the customize screens); the dev DB is cleaned and the helper now uses the canonical list. Two stale expectations corrected while re-running the suite: TestGetSections_EndToEndFlow asserted the kebab ids (now asserts the Title-case names — section ids ARE collection names), and TestGetSections_MissingLibraryID expected a 400 the handler never implemented — absent library_id is the All Libraries view by design (now asserts 200 + aggregated sections). |
||
|
John O'Keefe
|
7b0b2793a6 |
test(infra): self-healing admin password; cross-library move/merge suite
setupTestServer reuses the shared dev admin (testuser@tests.bookhoard.internal) instead of re-inserting it, but never reset its password — once any test mutated the admin's password, every later test in the run failed to log in with 401s until the database was manually wiped. The seeding step now resets the password hash to the known test constant on reuse, so full integration runs are repeatable against an existing database. Adds cross_library_move_test.go, seven integration tests covering the duplicate-content features end to end: - TestCrossLibraryMovePreservesHistory: a book moved between two same-type libraries keeps its row ID — progress and annotations survive, no duplicate, no archived ghost, library_type_name stays truthful. - TestCrossLibraryCopyStaysIndependent: deliberate copies in two libraries stay independent rows with isolated progress. - TestCrossLibraryMoveRejectedForTypeMismatch: a reflowable EPUB is not repointed between manga libraries; a library_type_mismatch processing issue is recorded instead. - TestListHiddenMediaItemsMatchesActiveTwin: archived rows expose their active same-SHA twin (and rows without one report no match). - TestMergeArchivedItemIntoActiveTwin: merging moves progress and annotations onto the active copy and removes the archived row. - TestMergeArchivedItemRejections: non-admin 403, active source, hash mismatch, missing target_id, self-merge, and archived target all fail closed. - TestLibraryTypeNameTriggerOnLibraryChange: the UPDATE trigger refreshes library_type_name when a row changes libraries. Tests create their own temp-dir library folders (host runs don't have /app/uploads), use unique device identifiers (device_identifier is UNIQUE and leftover rows broke reruns), and clean up via an explicit defer that runs while the pool is still open — a t.Cleanup registered for the same purpose silently no-ops because it executes after setup.Close has closed the pool. |
||
|
John O'Keefe
|
4faa217822 |
test: repair integration harness — wire SettingsRegistry, reuse preserved dev admin
Two pre-existing harness breaks found while running the device tests: - RegisterRoutes dereferences cfg.Settings (auth rate limit) but setupTestServer never set it — every integration test nil-panicked at route registration. Wire database.NewSettingsRegistry(queries) the same way main.go does. - The cleanup deliberately PRESERVES testuser@tests.bookhoard.internal (shared dev admin), but setup then blindly re-INSERTed that user, so every run after the first failed on users_email_key. Reuse the user when it exists; default collections are created only for a NEW user (the preserved admin already has its set). Also documented the offline runner used on slow links: build the test binary on the host (CGO_ENABLED=0 go test -c) and execute it inside the bookhoard-tests image against the compose network — no in-container module downloads, no image rebuild. |
||
 john-okeefe
|
75b33fdae6 |
feat(sync): wire annotation sync into all device and web handlers
Complete the annotation sync pipeline across all ingest and serve paths.
Previously, annotations sent inline with KOReader progress pushes were
silently discarded, and no annotations were ever served back to devices.
INGEST (device → server):
KOReader (koreader.go):
- Add processBookAnnotations helper that processes inline highlights,
notes, and bookmarks from every progress push (immediate + checkpoint)
- Highlights get CRE→CFI position conversion before SaveHighlight
- KOReader 'notes' (text + notes) stored as highlights with NoteText
to ensure correct round-trip classification
- Bookmarks routed through SaveBookmark with device sync data
- Called from both updateProgressForBook and handleCheckpointSync
Kobo (kobo.go):
- Markup handler: annotations and bookmarks route through
AnnotationService (SaveHighlight/SaveBookmark)
- Bookmark handler: same routing with device sync data
- SyncFromServer handler: same routing
- All handlers fall back to direct DB calls when annotationSvc == nil
Web reader (media.go):
- CreateMediaHighlight → SaveHighlight (Source="web", ModifiedAt=now)
- CreateMediaNote → SaveNote (Source="web")
- DeleteMediaHighlight → TombstoneHighlightByID
- DeleteMediaNote → TombstoneNoteByID (was hard delete, now tombstone)
- All fall back to old behavior when annotationSvc == nil
SERVE (server → device):
KOReader GetMetadata (koreader.go):
- Query and serve bookmarks from media_bookmarks table (was missing)
- Serve deleted_highlights and deleted_bookmarks arrays containing
device_sync_data + dedup_key for client-side deletion
- Highlights/notes already served with reverse CFI conversion
Kobo Markup handler (kobo.go):
- Track processed books during sync
- Query tombstones per book, extract bookmark_id from device_sync_data
- Return DeletedAnnotations array in KoboSyncStatus response
Conflict resolution (conflicts.go):
- Enable annotation conflict types in ResolveConflict handler
- Add applyAnnotationResolution dispatching to:
applyHighlightResolution / applyBookmarkResolution / applyNoteResolution
- Each looks up by dedup_key and applies winner's fields
- Allow manual override of auto_resolved conflicts
(changed check from != "unresolved" to == "user_resolved")
Infrastructure:
- AnnotationService field + SetAnnotationService in router Config
- Inject AnnotationService into KOReader, Kobo, Media handlers
- Start tombstone purger goroutine in main.go (24h interval)
- Test helpers: construct AnnotationService in test setup
|
||
|
john-okeefe
|
73fc609d7b |
fix(tests): protect dev admin from test cleanup, use isolated test names
Tests were deleting the development admin user, causing ON DELETE SET NULL to cascade and set created_by_admin_id to NULL on all libraries. - test_helpers: skip deletion of testuser@tests.bookhoard.internal - sync_integration_test: use test-sync% prefix for isolated test data |
||
|
john-okeefe
|
cce7ad4907 |
test(series): add unit and integration tests for series feature
Unit tests: - series_service_test.go: test continueSeriesRowToMediaItems conversion with valid fields, null fields, and comprehensive field mapping - series_test.go: test handler initialization and textToString helper Integration tests (series_integration_test.go): - GET /api/series: requires library_id, rejects invalid UUID, returns empty array for empty library, pagination params, limit clamped to 100, response structure validation, special characters in names - GET /api/series/books: requires library_id and name, handles nonexistent series, unauthorized access - Restore Continue Series system collection - Dashboard sections include all 5 collections (including continue-series) Update test helpers: - Add SeriesHandler to setupTestServer router config - Add 5th Continue Series collection to createDefaultCollectionsForUser - Update dashboard integration test for 5 collections |
||
|
john-okeefe
|
94102af4d7 |
test(progress): add comprehensive integration tests for ProgressService
Adds 30 integration tests across 7 test functions covering all progress endpoints with real HTTP requests and database verification: - AuthContexts (8 tests): unauthenticated PUT/GET return 401, regular user and admin both get 200, invalid UUID returns 400, nonexistent item returns 200 with empty data. - MergePreservesFields (2 tests): second PUT with only percentage preserves epubcfi and chapter from first save via GET verification; web save preserves koreader character_offset via DB query. - EnrichmentComputesFields (2 tests): character_offset computed from percentage when total_characters is set on media item; GET returns enriched format_group and total_characters. - ConflictDetection (3 tests): different sources with >1% diff within 5 minutes creates sync_conflicts record; same-source rapid saves create no conflict; <1% diff creates no conflict. - KoboIntegration (3 tests): ReadingSync then last-read-place preserves percentage via DB; standalone last-read-place sets epubcfi/chapter; unauthenticated returns 401. - KOReaderIntegration (2 tests): Bearer token auth with proper request body returns 202 Accepted; unauthenticated returns 401. - DeleteProgress (2 tests): DELETE clears progress; unauthenticated returns 401. - EdgeCases (4 tests): empty body succeeds, 0.0% and 1.0% boundaries, all fields with full DB verification of each column. Updates test_helpers to create ProgressService in setupTestServer and inject into all handlers. Fixes previous tests that used testing.Short() (which caused all tests to be skipped in the container) and assertions against wrong JSON format (pgtype serializes as plain values, not wrapped objects). |
||
|
john-okeefe
|
17f2dc3120 |
fix(tests): initialize ProcessingIssuesHandler in test server setup
The setupTestServer() helper in test_helpers_test.go was not creating a ProcessingIssuesHandler and not passing one to the router config, causing a nil pointer dereference when any processing issues route was hit during tests. Add handler creation and wire it into routerConfig to match how cmd/server/main.go does it. |
||
|
john-okeefe
|
a6700f73e0 |
fix(tests): handle all Close() and Decode() errors across integration tests
Replace all unhandled resp.Body.Close() calls throughout the test suite:
- Deferred calls: replace 'defer VAR.Body.Close()' with a closure that explicitly
discards the error via 'defer func(Body io.ReadCloser) { _ = Body.Close() }(VAR.Body)'
- Immediate calls: replace 'VAR.Body.Close()' with '_ = VAR.Body.Close()'
Replace all unhandled json.NewDecoder(VAR.Body).Decode(&x) calls with error capture
and require.NoError assertion. Files using httptest.ResponseRecorder (collections_preview,
processing_issues) use 'err :=' declaration; suite-style tests (scanner_integration,
dashboard_integration) use s.T() instead of t.
|
||
|
john-okeefe
|
8a5e6963d1 |
fix: ensure test libraries are cleaned up after each test completes
Problem: When running `make test-integration`, the last test to run would leave its "test" libraries in the database. This happened because: 1. setupTestServer() cleaned up old "test" libraries at the START 2. Tests created their own libraries 3. When tests finished, t.Cleanup() called setup.Close() which only closed connections but did NOT delete libraries 4. The LAST test's libraries persisted because no subsequent test cleaned them For example, "Job Status Test Library" from TestWorker_JobStatusTracking would remain in the database after all tests completed, visible when logging into the UI. Root Cause: The cleanup logic only ran at the START of each test (in setupTestServer), not at the END. This worked for intermediate tests (each test cleaned up the previous test's libraries), but the final test had no cleanup. Solution: Added library cleanup to the t.Cleanup() function in setupTestServer(). Now each test deletes its own "test" libraries when it completes, ensuring: - Clean state after `make test-integration` finishes - No residual test data in the database - Safe for tests with subtests (cleanup runs after all subtests finish) Note on Test Structure: Tests like TestOPDSEndpoints and TestCollectionSearchLibraryFilter create libraries once and share them across all subtests. The t.Cleanup() function runs AFTER all subtests complete, so this change is safe and doesn't interfere with subtest resource sharing. Files changed: - cmd/server/tests/test_helpers_test.go: Added library cleanup to t.Cleanup() |
||
|
john-okeefe
|
ddfc832b68 |
feat(api): implement saved filters backend service and handlers
Add complete backend implementation for saved filters CRUD operations with proper service layer architecture and RESTful API endpoints. Service Layer (internal/services/filters.go): - NewFiltersService() constructor following project patterns - GetSavedFilters(): Retrieve all filters for user + resource type - CreateSavedFilter(): Create filter with duplicate name validation - UpdateSavedFilter(): Update filter with ownership verification - DeleteSavedFilter(): Delete filter with user scoping Business Logic: - Filter name uniqueness enforced per user + resource type - User ownership validation on all operations (JWT user_id) - JSONB marshaling/unmarshaling for flexible filter storage - Proper error wrapping with context messages Handler Layer (internal/handlers/filters.go): - NewFiltersHandler() constructor (receives db.Queries) - GetSavedFilters: GET /api/saved-filters?resource_type=X - CreateSavedFilter: POST /api/saved-filters - UpdateSavedFilter: PUT /api/saved-filters/:id - DeleteSavedFilter: DELETE /api/saved-filters/:id Content Negotiation: - Supports both JSON (API clients) and HTML (HTMX) responses - wantsHTML() helper checks Accept header - HX-Redirect header for HTMX form submissions - Proper status codes (200, 201, 204, 400, 401, 404, 409) Router Configuration: - registerFiltersRoutes() function in internal/router/filters.go - JWT middleware protection on all endpoints - RESTful route structure: /api/saved-filters - Registered in main router.go RegisterRoutes() function - Added FiltersHandler to router.Config struct Test Infrastructure: - Added FiltersHandler to test server setup (test_helpers_test.go) - FiltersHandler initialized in setupTestServer() function - Router.Config includes FiltersHandler for integration tests Code Quality: - Follows PROJECT_GUIDELINES.md service layer patterns - Uses database models (not custom domain models) - JSONB returned as []byte (matches collections pattern) - All errors wrapped with context using fmt.Errorf - Handlers create services internally (not dependency injection) Part of: Saved Filters Implementation (Phase 2: Backend) Related: #saved-filters-feature |
||
|
john-okeefe
|
ec4b598728 |
test(server): update CORS configuration in test helpers with explicit settings
Replace default CORS middleware with explicit CORS configuration in test
server setup to align with production security settings. This ensures test
environment matches production behavior and prevents potential CORS-related
test failures.
Changes:
- Replace echomiddleware.CORS() with echomiddleware.CORSWithConfig()
- Configure allowed origins, methods, and headers explicitly
- Set AllowCredentials to false for test environment
- Add ExposeHeaders for Content-Length
This maintains consistency with the CORS configuration applied to the main
server in commit
|
||
|
john-okeefe
|
1e8d3c7107 |
test: rename test files to follow Go conventions
Rename test helper files from .go to _test.go suffix to comply with Go testing conventions. This ensures proper test file recognition by the Go toolchain and improves build organization. - library_test_comprehensive.go → library_test_comprehensive_test.go - test_helpers.go → test_helpers_test.go - test_helpers_db.go → test_helpers_db_test.go |