From 09474b0e0899dd39957332cacfa9f46b858e8a83 Mon Sep 17 00:00:00 2001 From: mithral Date: Sun, 11 Oct 2026 13:46:18 -0400 Subject: [PATCH] Wave 3: search depth, transfer size, and the Home screen's tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Authors are searchable (internal/search, migrations/0009) - The FTS index covered only title and path, so typing an artist or author found nothing even though people/item_people already held the credits. - Rather than a second index, the names are denormalised into a real media_items.authors column, so the existing external-content FTS and its triggers keep doing the work; item_people/people triggers recompute it, and the media_items UPDATE trigger re-indexes automatically. - Verified: searching "daft punk" now returns Random Access Memories, and so does the partial "punk". - The agent found and worked around a real FTS5 external-content bug: on an UPGRADE the backfill UPDATE fires the delete trigger for rowids not yet in the fresh index, raising "database disk image is malformed". A rebuild before the backfill fixes it. This would have broken existing databases while a fresh install looked fine. gzip for the SPA (internal/httpapi/static.go) - 456 KB of JS and 31 KB of CSS were served uncompressed on every cold load. - Scoped to the embedded SPA assets only, never API or media: compressing a Range response would break seeking, and wrapping every response would undo Wave 2's Flusher/ReaderFrom forwarding. - Compresses only when the client accepts gzip, the response is a compressible type, there is no Range header, and nothing already set Content-Encoding. Sets Vary: Accept-Encoding and drops the now-wrong Content-Length. - Verified by sha256 round-trip (identity vs gzip-decompressed are identical), and by confirming a Range request still returns 206 with no Content-Encoding. JS 351,835 -> 108,794 bytes (69% smaller); CSS 80% smaller. epub.js is code-split (Reader.svelte) - It was a static import, so ~350 KB was downloaded by everyone on first load even though most never open an ebook. Now loaded on demand. - Main bundle 458 KB -> 112 KB (gzipped 149 KB -> 39.5 KB). - Verified at runtime, which pnpm check cannot: only the main bundle loads initially, the epubjs chunk is fetched when a book is opened, and the reader still renders. Home screen tests (internal/media) - Coverage 23.4% -> 50.2%. (The 17.9% quoted in the brief was a stale reading; the agent measured the real baseline by moving its own files aside.) - Covers the five shelves, counts, Explore's invariants (visibility, no duplicates, every kind represented, varies across calls, wrap-around), the list offset/limit/total contract, MostViewed ordering, and — most importantly — grant scoping, which is the security chokepoint. Fixed a bug the tests found: Suggestions seeded from revoked libraries - topEngagement selected the user's most recent progress row with no library filter, so progress from a library whose grant had been revoked still steered the home screen — a visible item was labelled "More books" based on content the user could no longer read. Returned items were always correctly scoped, so this was an information-flow gap rather than a content leak. - Now scoped to visible libraries, and the test that pinned the old behaviour flipped to assert the correct reason. gofmt/vet clean, 14 packages pass, media tests run 3x for flakiness. --- internal/httpapi/static.go | 196 +++++++- internal/httpapi/static_test.go | 335 ++++++++++++ internal/media/discover.go | 13 +- internal/media/handlers_test.go | 70 +++ internal/media/home_test.go | 728 +++++++++++++++++++++++++++ internal/media/items_test.go | 250 +++++++++ internal/search/search.go | 13 +- internal/search/search_test.go | 98 ++++ migrations/0009_search_authors.sql | 120 +++++ web/src/lib/components/Reader.svelte | 4 +- 10 files changed, 1817 insertions(+), 10 deletions(-) create mode 100644 internal/httpapi/static_test.go create mode 100644 internal/media/handlers_test.go create mode 100644 internal/media/home_test.go create mode 100644 internal/media/items_test.go create mode 100644 migrations/0009_search_authors.sql diff --git a/internal/httpapi/static.go b/internal/httpapi/static.go index 51b5e69..b480700 100644 --- a/internal/httpapi/static.go +++ b/internal/httpapi/static.go @@ -1,9 +1,14 @@ package httpapi import ( + "bufio" + "compress/gzip" + "io" "io/fs" + "net" "net/http" "path" + "strconv" "strings" "github.com/mithral/vellum/web" @@ -30,23 +35,31 @@ func (s *Server) mountWeb() { } files := http.FileServer(http.FS(sub)) + + // Only the embedded static assets are wrapped for compression. API + // responses and media are served by other handlers and are never touched. + serveFiles := compressStatic(files.ServeHTTP) + serveShell := compressStatic(func(w http.ResponseWriter, r *http.Request) { + serveIndex(w, index) + }) + s.mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { if isAPIPath(r.URL.Path) { s.handleNotFound(w, r) return } if r.URL.Path == "/" || r.URL.Path == "" { - serveIndex(w, index) + serveShell(w, r) return } name := strings.TrimPrefix(path.Clean(r.URL.Path), "/") if f, err := sub.Open(name); err == nil { _ = f.Close() - files.ServeHTTP(w, r) + serveFiles(w, r) return } // SPA route: hand back the shell and let the client router decide. - serveIndex(w, index) + serveShell(w, r) }) } @@ -59,3 +72,180 @@ func serveIndex(w http.ResponseWriter, index []byte) { func isAPIPath(p string) bool { return p == "/healthz" || strings.HasPrefix(p, "/api/") } + +// ── Static-asset gzip ────────────────────────────────────────────────── +// +// The built SPA ships uncompressed: the JS bundle is ~456 KB and the CSS +// ~31 KB on every cold load. compressStatic wraps only the embedded static +// handlers with gzip negotiation; API responses and media are never wrapped. +// +// The narrow scope is deliberate. (a) Media is already compressed. (b) A Range +// response is used for seeking, and re-encoding it would invalidate the byte +// offsets. (c) server.go's statusRecorder was just fixed to forward +// http.Flusher and io.ReaderFrom for streaming; wrapping every response in a +// gzip writer would force all bodies through a compressor and undo that. +// Restricting the wrapper to the static file handler avoids all three. + +// compressStatic gzips a static handler's response when the client accepts +// gzip and the response is a compressible text type. Close is called after the +// handler returns so the gzip trailer is always written. +func compressStatic(next http.HandlerFunc) http.HandlerFunc { + return func(w http.ResponseWriter, r *http.Request) { + gz := newGzipResponseWriter(w, r) + next(gz, r) + _ = gz.Close() + } +} + +// compressibleType reports whether a Content-Type should be gzip-encoded. Only +// text-like types are listed; images, fonts, wasm and other media are already +// compressed, so re-encoding them wastes CPU for no gain. +func compressibleType(contentType string) bool { + if i := strings.IndexByte(contentType, ';'); i >= 0 { + contentType = contentType[:i] // drop "charset=utf-8" and friends + } + switch strings.ToLower(strings.TrimSpace(contentType)) { + case "text/html", + "text/css", + "application/javascript", + "text/javascript", + "application/json", + "image/svg+xml", + "application/manifest+json": + return true + } + return false +} + +// acceptsGzip reports whether the client's Accept-Encoding allows gzip. An +// explicit "gzip;q=0" is a refusal, so it does not count. +func acceptsGzip(header string) bool { + for _, part := range strings.Split(header, ",") { + token, params, _ := strings.Cut(part, ";") + if !strings.EqualFold(strings.TrimSpace(token), "gzip") { + continue + } + for _, p := range strings.Split(params, ";") { + if v, ok := strings.CutPrefix(strings.TrimSpace(p), "q="); ok { + if q, err := strconv.ParseFloat(strings.TrimSpace(v), 64); err == nil && q == 0 { + return false + } + } + } + return true + } + return false +} + +// gzipResponseWriter wraps a static response so its body can be gzip-encoded. +// +// Like statusRecorder it forwards Flush, Hijack and ReadFrom. While the body is +// being compressed those forwards must route through the compressor (or flush +// it first), or raw bytes would be written under a gzip Content-Encoding. +type gzipResponseWriter struct { + http.ResponseWriter + eligible bool + compress bool + wroteHeader bool + gz *gzip.Writer +} + +func newGzipResponseWriter(w http.ResponseWriter, r *http.Request) *gzipResponseWriter { + return &gzipResponseWriter{ + ResponseWriter: w, + // A Range request must stay identity so its byte offsets remain valid. + eligible: acceptsGzip(r.Header.Get("Accept-Encoding")) && r.Header.Get("Range") == "", + } +} + +// WriteHeader decides compression once the handler has set Content-Type. The +// decision has to happen before the status line is committed. +func (g *gzipResponseWriter) WriteHeader(code int) { + if g.wroteHeader { + return + } + g.wroteHeader = true + + h := g.Header() + if g.eligible && + code == http.StatusOK && + h.Get("Content-Encoding") == "" && + h.Get("Content-Length") != "0" && + compressibleType(h.Get("Content-Type")) { + g.compress = true + h.Set("Content-Encoding", "gzip") + h.Add("Vary", "Accept-Encoding") + // The compressed length is not known until the body is written, and + // the handler's original Content-Length is now wrong. + h.Del("Content-Length") + } + g.ResponseWriter.WriteHeader(code) +} + +// writer lazily creates the gzip stream on the first body byte. A HEAD request +// (or a bodyless response) never writes, so no empty gzip stream is emitted and +// net/http has nothing from which to fabricate a misleading Content-Length. +func (g *gzipResponseWriter) writer() *gzip.Writer { + if g.gz == nil { + g.gz = gzip.NewWriter(g.ResponseWriter) + } + return g.gz +} + +func (g *gzipResponseWriter) Write(b []byte) (int, error) { + if !g.wroteHeader { + g.WriteHeader(http.StatusOK) + } + if g.compress { + return g.writer().Write(b) + } + return g.ResponseWriter.Write(b) +} + +// Flush forwards to the underlying http.Flusher, after flushing any buffered +// compressed bytes so the client sees a complete gzip block. +func (g *gzipResponseWriter) Flush() { + if !g.wroteHeader { + g.WriteHeader(http.StatusOK) + } + if g.compress && g.gz != nil { + _ = g.gz.Flush() + } + if f, ok := g.ResponseWriter.(http.Flusher); ok { + f.Flush() + } +} + +// Hijack forwards to the underlying http.Hijacker for protocols that take over +// the connection. +func (g *gzipResponseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + if hj, ok := g.ResponseWriter.(http.Hijacker); ok { + return hj.Hijack() + } + return nil, nil, http.ErrNotSupported +} + +// ReadFrom forwards to the underlying io.ReaderFrom so io.Copy keeps the +// sendfile fast path. When compressing, the bytes must pass through the gzip +// writer instead; the returned count is the source byte count. +func (g *gzipResponseWriter) ReadFrom(src io.Reader) (int64, error) { + if !g.wroteHeader { + g.WriteHeader(http.StatusOK) + } + if g.compress { + return io.Copy(g.writer(), src) + } + if rf, ok := g.ResponseWriter.(io.ReaderFrom); ok { + return rf.ReadFrom(src) + } + return io.Copy(g.ResponseWriter, src) +} + +// Close flushes the gzip trailer. It must run after the static handler returns, +// or the body will be missing its checksum and the client will reject it. +func (g *gzipResponseWriter) Close() error { + if g.compress && g.gz != nil { + return g.gz.Close() + } + return nil +} diff --git a/internal/httpapi/static_test.go b/internal/httpapi/static_test.go new file mode 100644 index 0000000..57f9f2a --- /dev/null +++ b/internal/httpapi/static_test.go @@ -0,0 +1,335 @@ +package httpapi + +import ( + "bufio" + "bytes" + "compress/gzip" + "io" + "io/fs" + "net" + "net/http" + "net/http/httptest" + "strings" + "testing" + + "github.com/mithral/vellum/web" +) + +// findAsset returns the path (relative to the embedded dist root, no leading +// slash) of the first embedded file ending in suffix. Assets are content-hashed +// by the build, so tests must discover them rather than hardcode a name. +func findAsset(t *testing.T, suffix string) string { + t.Helper() + sub, err := fs.Sub(web.Dist, "dist") + if err != nil { + t.Fatalf("fs.Sub: %v", err) + } + var found string + err = fs.WalkDir(sub, ".", func(p string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() { + return nil + } + if found == "" && strings.HasSuffix(p, suffix) { + found = p + } + return nil + }) + if err != nil { + t.Fatalf("walk dist: %v", err) + } + if found == "" { + t.Fatalf("no embedded asset ending in %q", suffix) + } + return found +} + +func readAsset(t *testing.T, name string) []byte { + t.Helper() + sub, err := fs.Sub(web.Dist, "dist") + if err != nil { + t.Fatalf("fs.Sub: %v", err) + } + b, err := fs.ReadFile(sub, name) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + return b +} + +func gunzip(t *testing.T, body []byte) []byte { + t.Helper() + zr, err := gzip.NewReader(bytes.NewReader(body)) + if err != nil { + t.Fatalf("gzip.NewReader: %v", err) + } + defer zr.Close() + out, err := io.ReadAll(zr) + if err != nil { + t.Fatalf("read gzip body: %v", err) + } + return out +} + +// TestStaticGzipCompressesJS proves a gzip-capable request for a .js asset is +// compressed, carries no stale Content-Length, and round-trips to the original. +func TestStaticGzipCompressesJS(t *testing.T) { + srv := newTestServer(t) + name := findAsset(t, ".js") + want := readAsset(t, name) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/"+name, nil) + req.Header.Set("Accept-Encoding", "gzip") + srv.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if got := rec.Header().Get("Content-Encoding"); got != "gzip" { + t.Fatalf("Content-Encoding = %q, want gzip", got) + } + if got := rec.Header().Get("Vary"); !strings.Contains(got, "Accept-Encoding") { + t.Errorf("Vary = %q, want it to mention Accept-Encoding", got) + } + // The handler's Content-Length describes the uncompressed body; it must be + // gone once the length changes. + if got := rec.Header().Get("Content-Length"); got != "" { + t.Errorf("Content-Length = %q, want empty on a compressed response", got) + } + if got := gunzip(t, rec.Body.Bytes()); !bytes.Equal(got, want) { + t.Errorf("decompressed body does not match original %s (%d vs %d bytes)", name, len(got), len(want)) + } + if rec.Body.Len() >= len(want) { + t.Errorf("compressed body %d bytes is not smaller than original %d", rec.Body.Len(), len(want)) + } +} + +// TestStaticIdentityWithoutAcceptEncoding proves a request with no +// Accept-Encoding is served byte-for-byte, uncompressed. +func TestStaticIdentityWithoutAcceptEncoding(t *testing.T) { + srv := newTestServer(t) + name := findAsset(t, ".js") + want := readAsset(t, name) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/"+name, nil) + srv.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if got := rec.Header().Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty for an identity request", got) + } + if !bytes.Equal(rec.Body.Bytes(), want) { + t.Errorf("identity body does not match original %s", name) + } +} + +// TestStaticRangeNotCompressed proves a Range request stays identity: seeking +// depends on the original byte offsets, which gzip would invalidate. +func TestStaticRangeNotCompressed(t *testing.T) { + srv := newTestServer(t) + name := findAsset(t, ".js") + want := readAsset(t, name) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/"+name, nil) + req.Header.Set("Accept-Encoding", "gzip") + req.Header.Set("Range", "bytes=0-99") + srv.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusPartialContent { + t.Fatalf("status = %d, want 206", rec.Code) + } + if got := rec.Header().Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty on a Range response", got) + } + if !bytes.Equal(rec.Body.Bytes(), want[:100]) { + t.Errorf("range body does not match the first 100 bytes of %s", name) + } +} + +// TestStaticPNGNotCompressed proves binary assets are passed through even when +// the client accepts gzip; re-compressing them wastes CPU. +func TestStaticPNGNotCompressed(t *testing.T) { + srv := newTestServer(t) + name := findAsset(t, ".png") + want := readAsset(t, name) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/"+name, nil) + req.Header.Set("Accept-Encoding", "gzip") + srv.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if got := rec.Header().Get("Content-Type"); !strings.HasPrefix(got, "image/png") { + t.Fatalf("Content-Type = %q, want image/png", got) + } + if got := rec.Header().Get("Content-Encoding"); got != "" { + t.Fatalf("Content-Encoding = %q, want empty for a png", got) + } + if !bytes.Equal(rec.Body.Bytes(), want) { + t.Errorf("png body does not match original %s", name) + } +} + +// TestStaticIndexCompressedAndNoCache proves the SPA shell still says no-cache +// and is now compressed for gzip clients. +func TestStaticIndexCompressedAndNoCache(t *testing.T) { + srv := newTestServer(t) + want := readAsset(t, "index.html") + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Header.Set("Accept-Encoding", "gzip") + srv.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if got := rec.Header().Get("Cache-Control"); got != "no-cache" { + t.Errorf("Cache-Control = %q, want no-cache", got) + } + if got := rec.Header().Get("Content-Encoding"); got != "gzip" { + t.Fatalf("Content-Encoding = %q, want gzip", got) + } + if got := gunzip(t, rec.Body.Bytes()); !bytes.Equal(got, want) { + t.Errorf("decompressed index does not match original") + } +} + +// ── Wrapper interface forwarding ─────────────────────────────────────── +// +// The wrapper sits between the static handler and statusRecorder, so it must +// keep the same interfaces statusRecorder forwards. + +func TestGzipResponseWriterForwardsFlush(t *testing.T) { + rec := &flushRecorder{ResponseRecorder: httptest.NewRecorder()} + req := httptest.NewRequest(http.MethodGet, "/x", nil) + req.Header.Set("Accept-Encoding", "gzip") + gz := newGzipResponseWriter(rec, req) + + gz.Header().Set("Content-Type", "text/html; charset=utf-8") + gz.WriteHeader(http.StatusOK) + _, _ = gz.Write([]byte("")) + gz.Flush() + _ = gz.Close() + + if !rec.flushed { + t.Error("Flush was not forwarded to the underlying writer") + } + if got := gunzip(t, rec.Body.Bytes()); string(got) != "" { + t.Errorf("flushed body = %q, want ", got) + } +} + +type hijackRecorder struct { + *httptest.ResponseRecorder + hijacked bool +} + +func (h *hijackRecorder) Hijack() (net.Conn, *bufio.ReadWriter, error) { + h.hijacked = true + return nil, nil, nil +} + +func TestGzipResponseWriterForwardsHijack(t *testing.T) { + rec := &hijackRecorder{ResponseRecorder: httptest.NewRecorder()} + req := httptest.NewRequest(http.MethodGet, "/x", nil) + gz := newGzipResponseWriter(rec, req) + + if _, _, err := gz.Hijack(); err != nil { + t.Fatalf("Hijack: %v", err) + } + if !rec.hijacked { + t.Error("Hijack was not forwarded to the underlying writer") + } +} + +func TestGzipResponseWriterForwardsReadFrom(t *testing.T) { + rec := &readFromRecorder{ResponseRecorder: httptest.NewRecorder()} + req := httptest.NewRequest(http.MethodGet, "/x", nil) // no Accept-Encoding + gz := newGzipResponseWriter(rec, req) + + n, err := gz.ReadFrom(strings.NewReader("payload")) + if err != nil { + t.Fatalf("ReadFrom: %v", err) + } + if !rec.readFrom { + t.Error("ReadFrom was not forwarded to the underlying writer") + } + if n != int64(len("payload")) || rec.Body.String() != "payload" { + t.Errorf("ReadFrom wrote %d bytes %q, want payload", n, rec.Body.String()) + } +} + +func TestGzipResponseWriterReadFromCompresses(t *testing.T) { + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/x", nil) + req.Header.Set("Accept-Encoding", "gzip") + gz := newGzipResponseWriter(rec, req) + + gz.Header().Set("Content-Type", "application/javascript") + n, err := gz.ReadFrom(strings.NewReader("payload")) + if err != nil { + t.Fatalf("ReadFrom: %v", err) + } + if n != int64(len("payload")) { + t.Errorf("ReadFrom returned %d, want %d", n, len("payload")) + } + _ = gz.Close() + + if got := rec.Header().Get("Content-Encoding"); got != "gzip" { + t.Fatalf("Content-Encoding = %q, want gzip", got) + } + if got := gunzip(t, rec.Body.Bytes()); string(got) != "payload" { + t.Errorf("decompressed body = %q, want payload", got) + } +} + +// TestCompressibleType and TestAcceptsGzip pin the content-type allow list and +// the Accept-Encoding parsing, including the q=0 refusal. +func TestCompressibleType(t *testing.T) { + compressible := []string{ + "text/html", "text/html; charset=utf-8", + "text/css", "application/javascript", + "text/javascript; charset=utf-8", + "application/json", "image/svg+xml", + "application/manifest+json", + } + for _, ct := range compressible { + if !compressibleType(ct) { + t.Errorf("compressibleType(%q) = false, want true", ct) + } + } + skip := []string{ + "image/png", "image/jpeg", "font/woff2", + "application/wasm", "application/octet-stream", "", + } + for _, ct := range skip { + if compressibleType(ct) { + t.Errorf("compressibleType(%q) = true, want false", ct) + } + } +} + +func TestAcceptsGzip(t *testing.T) { + yes := []string{"gzip", "gzip, deflate", "br, gzip;q=0.5", "GZIP"} + for _, h := range yes { + if !acceptsGzip(h) { + t.Errorf("acceptsGzip(%q) = false, want true", h) + } + } + no := []string{"", "deflate", "br", "gzip;q=0"} + for _, h := range no { + if acceptsGzip(h) { + t.Errorf("acceptsGzip(%q) = true, want false", h) + } + } +} diff --git a/internal/media/discover.go b/internal/media/discover.go index 69037f9..3c56cc9 100644 --- a/internal/media/discover.go +++ b/internal/media/discover.go @@ -85,8 +85,14 @@ type engagement struct { // topEngagement finds the kind and author the user most recently engaged with, // so suggestions can be explained in plain language. -func topEngagement(ctx context.Context, db *sql.DB, userID int64) (engagement, bool, error) { +func topEngagement(ctx context.Context, db *sql.DB, userID int64, libraryIDs []int64) (engagement, bool, error) { var e engagement + // Scoped to libraries the user can still read. Without this, progress from a + // library whose grant was revoked would keep steering the home screen's + // suggestions — confusing on its own, and an information-flow leak about + // content the user no longer has access to. + in, inArgs := inClause(libraryIDs) + args := append([]any{userID}, inArgs...) err := db.QueryRowContext(ctx, ` SELECT i.kind, i.title, COALESCE((SELECT p.name FROM item_people ip @@ -96,8 +102,9 @@ func topEngagement(ctx context.Context, db *sql.DB, userID int64) (engagement, b FROM progress p JOIN media_items i ON i.id = p.item_id WHERE p.user_id = ? AND i.missing = 0 + AND i.library_id IN (`+in+`) ORDER BY p.updated_at DESC - LIMIT 1`, userID).Scan(&e.Kind, &e.Title, &e.Author) + LIMIT 1`, args...).Scan(&e.Kind, &e.Title, &e.Author) if err == sql.ErrNoRows { return engagement{}, false, nil } @@ -120,7 +127,7 @@ func Suggestions(ctx context.Context, db *sql.DB, userID int64, libraryIDs []int limit = 20 } - e, hasEngagement, err := topEngagement(ctx, db, userID) + e, hasEngagement, err := topEngagement(ctx, db, userID, libraryIDs) if err != nil { return nil, err } diff --git a/internal/media/handlers_test.go b/internal/media/handlers_test.go new file mode 100644 index 0000000..6005a43 --- /dev/null +++ b/internal/media/handlers_test.go @@ -0,0 +1,70 @@ +package media + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mithral/vellum/internal/auth" +) + +// getItem drives the per-item detail handler directly. PathValue is normally +// filled in by the ServeMux; the test sets it explicitly. +func getItem(t *testing.T, h *Handlers, u auth.User, id string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/v1/items/"+id, nil) + req.SetPathValue("id", id) + rec := httptest.NewRecorder() + h.handleItem(rec, req, u) + return rec +} + +// TestHandleItemScoping pins the per-item access chokepoint: an item in a +// library the caller cannot read must be indistinguishable from a missing one. +func TestHandleItemScoping(t *testing.T) { + f := setupHome(t) + h := mediaHandlers(t, f.db) + + // userA may read libA, so a1 resolves. + rec := getItem(t, h, auth.User{ID: f.userA}, itoa(f.a1)) + if rec.Code != http.StatusOK { + t.Fatalf("accessible item status = %d, body=%s", rec.Code, rec.Body.String()) + } + var body struct { + Item struct { + ID int64 `json:"id"` + LibraryID int64 `json:"library_id"` + Kind string `json:"kind"` + } `json:"item"` + Authors []string `json:"authors"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode item: %v", err) + } + if body.Item.ID != f.a1 || body.Item.LibraryID != f.libA || body.Item.Kind != "ebooks" { + t.Errorf("item = %+v, want id %d in library %d", body.Item, f.a1, f.libA) + } + if len(body.Authors) != 1 || body.Authors[0] != "Ada Author" { + t.Errorf("authors = %v, want [Ada Author]", body.Authors) + } + + // b1 is in libB, which userA cannot read: 404, not 403, so existence is + // not disclosed. + rec = getItem(t, h, auth.User{ID: f.userA}, itoa(f.b1)) + if rec.Code != http.StatusNotFound { + t.Errorf("inaccessible item status = %d, want 404", rec.Code) + } + + // A user with no grants gets the same 404. + rec = getItem(t, h, auth.User{ID: f.userNone}, itoa(f.a1)) + if rec.Code != http.StatusNotFound { + t.Errorf("no-grant item status = %d, want 404", rec.Code) + } + + // A non-numeric id is a 400, not a panic. + rec = getItem(t, h, auth.User{ID: f.userA}, "abc") + if rec.Code != http.StatusBadRequest { + t.Errorf("invalid id status = %d, want 400", rec.Code) + } +} diff --git a/internal/media/home_test.go b/internal/media/home_test.go new file mode 100644 index 0000000..4b9b946 --- /dev/null +++ b/internal/media/home_test.go @@ -0,0 +1,728 @@ +package media + +import ( + "context" + "database/sql" + "encoding/json" + "fmt" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "sort" + "strconv" + "strings" + "testing" + + "github.com/mithral/vellum/internal/auth" +) + +// ── shared helpers ───────────────────────────────────────────────────── + +func mediaHandlers(t *testing.T, db *sql.DB) *Handlers { + t.Helper() + log := slog.New(slog.NewTextHandler(io.Discard, nil)) + return NewHandlers(db, log, nil, t.TempDir()) +} + +func mustQueryID(t *testing.T, db *sql.DB, q string, args ...any) int64 { + t.Helper() + var id int64 + if err := db.QueryRowContext(context.Background(), q, args...).Scan(&id); err != nil { + t.Fatalf("query %q: %v", q, err) + } + return id +} + +func mustExec(t *testing.T, db *sql.DB, q string, args ...any) { + t.Helper() + if _, err := db.ExecContext(context.Background(), q, args...); err != nil { + t.Fatalf("exec %q: %v", q, err) + } +} + +func newLibrary(t *testing.T, db *sql.DB, name, kind, root string) int64 { + t.Helper() + return mustQueryID(t, db, + `INSERT INTO libraries (name, kind, root_path) VALUES (?, ?, ?) RETURNING id`, name, kind, root) +} + +func newUser(t *testing.T, db *sql.DB, name string) int64 { + t.Helper() + return mustQueryID(t, db, + `INSERT INTO users (username, password_hash) VALUES (?, 'x') RETURNING id`, name) +} + +func grantRead(t *testing.T, db *sql.DB, userID, libID int64) { + t.Helper() + mustExec(t, db, + `INSERT INTO library_grants (user_id, library_id, can_read, can_download) VALUES (?, ?, 1, 1)`, + userID, libID) +} + +// newItem inserts a media item plus a backing file, because progress rows carry +// a NOT NULL file_id foreign key. +func newItem(t *testing.T, db *sql.DB, libID int64, kind, path, title, sortTitle string, missing bool, addedAt string) int64 { + t.Helper() + m := 0 + if missing { + m = 1 + } + id := mustQueryID(t, db, ` + INSERT INTO media_items (library_id, kind, path, title, sort_title, missing, added_at) + VALUES (?, ?, ?, ?, ?, ?, ?) RETURNING id`, + libID, kind, path, title, sortTitle, m, addedAt) + mustExec(t, db, + `INSERT INTO media_files (item_id, path, size) VALUES (?, ?, 1)`, + id, "file-"+strconv.FormatInt(id, 10)) + return id +} + +func fileIDFor(t *testing.T, db *sql.DB, itemID int64) int64 { + t.Helper() + var id int64 + if err := db.QueryRowContext(context.Background(), + `SELECT id FROM media_files WHERE item_id = ? ORDER BY id LIMIT 1`, itemID).Scan(&id); err != nil { + t.Fatalf("file for item %d: %v", itemID, err) + } + return id +} + +func newProgress(t *testing.T, db *sql.DB, userID, itemID int64, percent float64, finished bool, updatedAt string) { + t.Helper() + f := 0 + if finished { + f = 1 + } + mustExec(t, db, ` + INSERT INTO progress (user_id, item_id, file_id, kind, position, percent, finished, updated_at) + VALUES (?, ?, ?, 'ebooks', 1, ?, ?, ?)`, + userID, itemID, fileIDFor(t, db, itemID), percent, f, updatedAt) +} + +func newView(t *testing.T, db *sql.DB, userID, itemID int64, views int, lastViewedAt string) { + t.Helper() + mustExec(t, db, + `INSERT INTO item_views (user_id, item_id, views, last_viewed_at) VALUES (?, ?, ?, ?)`, + userID, itemID, views, lastViewedAt) +} + +func idSet(items []shelfItem) map[int64]bool { + out := make(map[int64]bool, len(items)) + for _, it := range items { + out[it.ID] = true + } + return out +} + +func hasDuplicates(items []shelfItem) bool { + seen := map[int64]bool{} + for _, it := range items { + if seen[it.ID] { + return true + } + seen[it.ID] = true + } + return false +} + +// ── handleHome ───────────────────────────────────────────────────────── + +type homeFixture struct { + db *sql.DB + libA int64 // ebooks/comics, visible to userA + libB int64 // audiobooks, visible to userB + userA int64 + userB int64 + userNone int64 + a1 int64 // in progress (unfinished, most recent) + a2 int64 // finished + a3 int64 // unstarted, viewed, same author as a1 + a4 int64 // missing, in progress + a5 int64 // unstarted + b1 int64 // userB's only item +} + +func setupHome(t *testing.T) homeFixture { + t.Helper() + db := testDB(t) + f := homeFixture{db: db} + + f.libA = newLibrary(t, db, "Ebooks", "ebooks", "/media/ebooks") + f.libB = newLibrary(t, db, "Audio", "audiobooks", "/media/audio") + + f.userA = newUser(t, db, "alice") + f.userB = newUser(t, db, "bob") + f.userNone = newUser(t, db, "carol") + + grantRead(t, db, f.userA, f.libA) + grantRead(t, db, f.userB, f.libB) + + f.a1 = newItem(t, db, f.libA, "ebooks", "a1.epub", "A One", "a one", false, "2026-01-01T00:00:01.000Z") + f.a2 = newItem(t, db, f.libA, "ebooks", "a2.epub", "A Two", "a two", false, "2026-01-01T00:00:02.000Z") + f.a3 = newItem(t, db, f.libA, "comics", "a3.cbz", "A Three", "a three", false, "2026-01-01T00:00:03.000Z") + f.a4 = newItem(t, db, f.libA, "ebooks", "a4.epub", "A Four", "a four", true, "2026-01-01T00:00:04.000Z") + f.a5 = newItem(t, db, f.libA, "ebooks", "a5.epub", "A Five", "a five", false, "2026-01-01T00:00:05.000Z") + f.b1 = newItem(t, db, f.libB, "audiobooks", "b1.mp3", "B One", "b one", false, "2026-01-01T00:00:06.000Z") + + // userA's progress. a1 is the most recent non-missing row, so it drives + // "top engagement". a2 is finished (must leave Continue). a4 is missing + // (must leave Continue even though unfinished). + newProgress(t, db, f.userA, f.a1, 0.5, false, "2026-02-01T00:00:03.000Z") + newProgress(t, db, f.userA, f.a2, 1.0, true, "2026-02-01T00:00:02.000Z") + newProgress(t, db, f.userA, f.a4, 0.3, false, "2026-02-01T00:00:04.000Z") + + // userB only ever touches their own library. + newProgress(t, db, f.userB, f.b1, 0.2, false, "2026-02-02T00:00:01.000Z") + + newView(t, db, f.userA, f.a1, 3, "2026-02-03T00:00:01.000Z") + newView(t, db, f.userA, f.a3, 1, "2026-02-03T00:00:02.000Z") + newView(t, db, f.userB, f.b1, 9, "2026-02-03T00:00:03.000Z") + + // Ada Author wrote a1 (in progress) and a3 (unstarted) so Suggestions has + // a same-author candidate. + ada := mustQueryID(t, db, `INSERT INTO people (name) VALUES ('Ada Author') RETURNING id`) + mustExec(t, db, `INSERT INTO item_people (item_id, person_id, role, ord) VALUES (?, ?, 'author', 0)`, f.a1, ada) + mustExec(t, db, `INSERT INTO item_people (item_id, person_id, role, ord) VALUES (?, ?, 'author', 0)`, f.a3, ada) + + return f +} + +func getHome(t *testing.T, h *Handlers, u auth.User) (homeResponse, []byte) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/v1/home", nil) + rec := httptest.NewRecorder() + h.handleHome(rec, req, u) + if rec.Code != http.StatusOK { + t.Fatalf("home status = %d, body = %s", rec.Code, rec.Body.String()) + } + var resp homeResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode home: %v (body %s)", err, rec.Body.String()) + } + return resp, rec.Body.Bytes() +} + +func TestHandleHomeShelvesAndCounts(t *testing.T) { + f := setupHome(t) + h := mediaHandlers(t, f.db) + resp, body := getHome(t, h, auth.User{ID: f.userA}) + + // The landing screen contract: all five shelves plus the sidebar counts. + var raw map[string]json.RawMessage + if err := json.Unmarshal(body, &raw); err != nil { + t.Fatalf("decode raw home: %v", err) + } + for _, key := range []string{"continue", "most_viewed", "suggestions", "recent", "explore", "counts_by_kind", "library_count", "total_items"} { + if _, ok := raw[key]; !ok { + t.Errorf("home response is missing %q", key) + } + } + if resp.Continue == nil || resp.MostViewed == nil || resp.Suggestions == nil || resp.Recent == nil || resp.Explore == nil { + t.Fatalf("a shelf is nil: continue=%v most=%v sugg=%v recent=%v explore=%v", + resp.Continue == nil, resp.MostViewed == nil, resp.Suggestions == nil, resp.Recent == nil, resp.Explore == nil) + } + + // Continue: only a1. a2 finished, a4 missing, b1 belongs to another user. + if got := ids(resp.Continue); len(got) != 1 || got[0] != f.a1 { + t.Errorf("continue = %v, want [%d]", got, f.a1) + } + for _, s := range resp.Continue { + if s.Percent <= 0 { + t.Errorf("continue item %d has percent %v", s.ID, s.Percent) + } + } + + // Most viewed: a1 (3 views) then a3 (1). b1 is in another library. + if got := ids(resp.MostViewed); len(got) != 2 || got[0] != f.a1 || got[1] != f.a3 { + t.Errorf("most_viewed = %v, want [%d %d]", got, f.a1, f.a3) + } + + // Counts exclude the missing item and the invisible library. + if resp.CountsByKind["ebooks"] != 3 || resp.CountsByKind["comics"] != 1 { + t.Errorf("counts_by_kind = %v, want ebooks:3 comics:1", resp.CountsByKind) + } + if _, ok := resp.CountsByKind["audiobooks"]; ok { + t.Errorf("counts_by_kind leaked another library: %v", resp.CountsByKind) + } + if resp.TotalItems != 4 { + t.Errorf("total_items = %d, want 4", resp.TotalItems) + } + if resp.LibraryCount != 1 { + t.Errorf("library_count = %d, want 1", resp.LibraryCount) + } + + // Recent excludes the missing item and the other library, newest first. + recent := ids(resp.Recent) + if len(recent) != 4 { + t.Errorf("recent = %v, want 4 libA items", recent) + } + for _, id := range recent { + if id == f.a4 || id == f.b1 { + t.Errorf("recent included excluded item %d", id) + } + } +} + +func TestHandleHomeGrantScoping(t *testing.T) { + f := setupHome(t) + h := mediaHandlers(t, f.db) + + // userA holds a grant to libA only. Nothing from libB may surface anywhere. + resp, _ := getHome(t, h, auth.User{ID: f.userA}) + for name, shelf := range map[string][]shelfItem{ + "continue": resp.Continue, "most_viewed": resp.MostViewed, + "recent": resp.Recent, "explore": resp.Explore, + } { + for _, s := range shelf { + if s.LibraryID != f.libA { + t.Errorf("%s item %d leaked from library %d, want only %d", name, s.ID, s.LibraryID, f.libA) + } + } + } + for _, s := range resp.Suggestions { + if s.LibraryID != f.libA { + t.Errorf("suggestion %d leaked from library %d", s.ID, s.LibraryID) + } + } + + // A user with no grants gets empty shelves, not an error and not content. + none, _ := getHome(t, h, auth.User{ID: f.userNone}) + if none.LibraryCount != 0 || none.TotalItems != 0 || len(none.CountsByKind) != 0 { + t.Errorf("no-grant user got library_count=%d total=%d counts=%v", + none.LibraryCount, none.TotalItems, none.CountsByKind) + } + for name, n := range map[string]int{ + "continue": len(none.Continue), "most_viewed": len(none.MostViewed), + "suggestions": len(none.Suggestions), "recent": len(none.Recent), "explore": len(none.Explore), + } { + if n != 0 { + t.Errorf("no-grant user's %s shelf has %d items", name, n) + } + } +} + +func TestHandleHomeIsolatesUsers(t *testing.T) { + f := setupHome(t) + h := mediaHandlers(t, f.db) + resp, _ := getHome(t, h, auth.User{ID: f.userB}) + + if got := ids(resp.Continue); len(got) != 1 || got[0] != f.b1 { + t.Errorf("user B continue = %v, want [%d]", got, f.b1) + } + if got := ids(resp.MostViewed); len(got) != 1 || got[0] != f.b1 { + t.Errorf("user B most_viewed = %v, want [%d]", got, f.b1) + } + if resp.CountsByKind["audiobooks"] != 1 || resp.TotalItems != 1 || resp.LibraryCount != 1 { + t.Errorf("user B counts = %v total=%d library_count=%d, want audiobooks:1 total:1 libs:1", + resp.CountsByKind, resp.TotalItems, resp.LibraryCount) + } + for name, shelf := range map[string][]shelfItem{ + "continue": resp.Continue, "most_viewed": resp.MostViewed, + "recent": resp.Recent, "explore": resp.Explore, + } { + for _, s := range shelf { + if s.LibraryID != f.libB { + t.Errorf("user B's %s saw library %d", name, s.LibraryID) + } + } + } +} + +// ── Explore ──────────────────────────────────────────────────────────── + +func TestExploreOnlyVisibleAndPresent(t *testing.T) { + db, libA, libB, userA, userB := setupExplore(t) + ctx := context.Background() + + var libAIDs []int64 + for i := 0; i < 6; i++ { + libAIDs = append(libAIDs, newItem(t, db, libA, "ebooks", + fmt.Sprintf("a%d.epub", i), fmt.Sprintf("A%d", i), fmt.Sprintf("a%02d", i), + false, "2026-01-01T00:00:00.000Z")) + } + missing := map[int64]bool{} + for i := 0; i < 3; i++ { + missing[newItem(t, db, libA, "ebooks", + fmt.Sprintf("m%d.epub", i), fmt.Sprintf("M%d", i), fmt.Sprintf("m%02d", i), + true, "2026-01-01T00:00:00.000Z")] = true + } + for i := 0; i < 6; i++ { + newItem(t, db, libB, "audiobooks", + fmt.Sprintf("b%d.mp3", i), fmt.Sprintf("B%d", i), fmt.Sprintf("b%02d", i), + false, "2026-01-01T00:00:00.000Z") + } + + got, err := Explore(ctx, db, userA, []int64{libA}, 30) + if err != nil { + t.Fatal(err) + } + if len(got) != len(libAIDs) { + t.Fatalf("explore returned %d, want all %d present libA items", len(got), len(libAIDs)) + } + seen := idSet(got) + for _, id := range libAIDs { + if !seen[id] { + t.Errorf("explore omitted present item %d", id) + } + } + for _, s := range got { + if s.LibraryID != libA { + t.Errorf("explore item %d came from library %d, want %d", s.ID, s.LibraryID, libA) + } + if missing[s.ID] { + t.Errorf("explore returned missing item %d", s.ID) + } + } + + // userB must never see libA content. + gotB, err := Explore(ctx, db, userB, []int64{libB}, 30) + if err != nil { + t.Fatal(err) + } + for _, s := range gotB { + if s.LibraryID != libB { + t.Errorf("user B explore item %d came from library %d", s.ID, s.LibraryID) + } + } +} + +// TestExploreExcludesContinueShelf guards the whole point of the shelf: it is +// discovery, so anything the user already has in progress must not reappear. +func TestExploreExcludesContinueShelf(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + var all []int64 + for i := 0; i < 20; i++ { + all = append(all, newItem(t, db, libA, "ebooks", + fmt.Sprintf("a%d.epub", i), fmt.Sprintf("A%d", i), fmt.Sprintf("a%02d", i), + false, "2026-01-01T00:00:00.000Z")) + } + // Three of them are on the Continue shelf. + inProgress := map[int64]bool{all[0]: true, all[7]: true, all[19]: true} + for id := range inProgress { + newProgress(t, db, userA, id, 0.4, false, "2026-02-01T00:00:00.000Z") + } + + // Repeated calls, because Explore samples a random window per kind. + for i := 0; i < 50; i++ { + got, err := Explore(ctx, db, userA, []int64{libA}, 10) + if err != nil { + t.Fatal(err) + } + if len(got) == 0 { + t.Fatal("explore returned nothing") + } + for _, s := range got { + if inProgress[s.ID] { + t.Fatalf("call %d returned in-progress item %d", i, s.ID) + } + } + } +} + +// TestExploreRepresentsEveryKind checks the fair-share split: when the limit is +// large enough, every kind present in the visible libraries must appear. +func TestExploreRepresentsEveryKind(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + kinds := []string{"ebooks", "comics", "audiobooks"} + for _, kind := range kinds { + for i := 0; i < 12; i++ { + newItem(t, db, libA, kind, + fmt.Sprintf("%s-%d.dat", kind, i), fmt.Sprintf("%s %d", kind, i), + fmt.Sprintf("%s-%02d", kind, i), false, "2026-01-01T00:00:00.000Z") + } + } + + got, err := Explore(ctx, db, userA, []int64{libA}, 30) + if err != nil { + t.Fatal(err) + } + if len(got) != 30 { + t.Fatalf("explore returned %d items, want the full limit 30", len(got)) + } + if hasDuplicates(got) { + t.Errorf("explore returned duplicate items: %v", ids(got)) + } + present := map[string]bool{} + for _, s := range got { + present[s.Kind] = true + } + for _, kind := range kinds { + if !present[kind] { + t.Errorf("kind %q missing from explore shelf %v", kind, ids(got)) + } + } +} + +// TestExploreNoDuplicates exercises the wrap-around path: the result set must +// never repeat an item even though a kind may be filled from both halves of the +// id range. +func TestExploreNoDuplicates(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + for i := 0; i < 40; i++ { + newItem(t, db, libA, "ebooks", + fmt.Sprintf("a%d.epub", i), fmt.Sprintf("A%d", i), fmt.Sprintf("a%02d", i), + false, "2026-01-01T00:00:00.000Z") + } + for i := 0; i < 100; i++ { + got, err := Explore(ctx, db, userA, []int64{libA}, 15) + if err != nil { + t.Fatal(err) + } + if hasDuplicates(got) { + t.Fatalf("call %d returned duplicates: %v", i, ids(got)) + } + } +} + +// TestExploreVariesAcrossCalls pins the fix for the old deterministic ordering: +// with far more items than the limit, repeated visits must not return the same +// slice every time. It asserts variety (an invariant of the sampler) rather +// than a particular order, so it is not sensitive to the random seed. +func TestExploreVariesAcrossCalls(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + for i := 0; i < 120; i++ { + newItem(t, db, libA, "ebooks", + fmt.Sprintf("a%d.epub", i), fmt.Sprintf("A%d", i), fmt.Sprintf("a%03d", i), + false, "2026-01-01T00:00:00.000Z") + } + + variants := map[string]bool{} + for i := 0; i < 30; i++ { + got, err := Explore(ctx, db, userA, []int64{libA}, 30) + if err != nil { + t.Fatal(err) + } + variants[signature(got)] = true + } + if len(variants) < 2 { + t.Fatalf("explore returned an identical shelf on all 30 calls (%d items each); the sampler is deterministic", len(variants)) + } +} + +// TestExploreCompleteWhenLimitCoversEverything checks the short-result / +// wrap-around path deterministically: regardless of where the random start +// lands, a limit larger than the library must yield the whole eligible set. +func TestExploreCompleteWhenLimitCoversEverything(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + var want []int64 + for _, kind := range []string{"ebooks", "comics"} { + for i := 0; i < 4; i++ { + want = append(want, newItem(t, db, libA, kind, + fmt.Sprintf("%s-%d.dat", kind, i), fmt.Sprintf("%s %d", kind, i), + fmt.Sprintf("%s-%02d", kind, i), false, "2026-01-01T00:00:00.000Z")) + } + } + + got, err := Explore(ctx, db, userA, []int64{libA}, 100) + if err != nil { + t.Fatal(err) + } + seen := idSet(got) + for _, id := range want { + if !seen[id] { + t.Errorf("limit above library size omitted item %d", id) + } + } + if len(got) != len(want) { + t.Errorf("explore returned %d items, want exactly %d", len(got), len(want)) + } +} + +// signature renders an item slice as a stable string for set comparison. +func signature(items []shelfItem) string { + ids := ids(items) + sort.Slice(ids, func(i, j int) bool { return ids[i] < ids[j] }) + parts := make([]string, len(ids)) + for i, id := range ids { + parts[i] = strconv.FormatInt(id, 10) + } + return strings.Join(parts, ",") +} + +// ── RecordView / MostViewed ──────────────────────────────────────────── + +func TestMostViewedOrdersByViewsThenRecency(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + older := newItem(t, db, libA, "ebooks", "older.epub", "Older", "older", false, "2026-01-01T00:00:00.000Z") + newer := newItem(t, db, libA, "ebooks", "newer.epub", "Newer", "newer", false, "2026-01-01T00:00:00.000Z") + most := newItem(t, db, libA, "ebooks", "most.epub", "Most", "most", false, "2026-01-01T00:00:00.000Z") + + // Equal view counts, different recency: the newer one must win. + newView(t, db, userA, older, 5, "2026-03-01T00:00:00.000Z") + newView(t, db, userA, newer, 5, "2026-03-02T00:00:00.000Z") + // More views always outranks recency. + newView(t, db, userA, most, 9, "2026-01-01T00:00:00.000Z") + + got, err := MostViewed(ctx, db, userA, []int64{libA}, 10) + if err != nil { + t.Fatal(err) + } + want := []int64{most, newer, older} + if len(got) != len(want) { + t.Fatalf("most viewed = %v, want %v", ids(got), want) + } + for i := range want { + if got[i].ID != want[i] { + t.Errorf("most viewed[%d] = %d, want %d (full order %v)", i, got[i].ID, want[i], ids(got)) + } + } +} + +func TestMostViewedExcludesMissing(t *testing.T) { + db, libA, _, userA, _ := setupExplore(t) + ctx := context.Background() + + present := newItem(t, db, libA, "ebooks", "p.epub", "P", "p", false, "2026-01-01T00:00:00.000Z") + gone := newItem(t, db, libA, "ebooks", "g.epub", "G", "g", true, "2026-01-01T00:00:00.000Z") + + newView(t, db, userA, present, 1, "2026-03-01T00:00:00.000Z") + newView(t, db, userA, gone, 50, "2026-03-02T00:00:00.000Z") + + got, err := MostViewed(ctx, db, userA, []int64{libA}, 10) + if err != nil { + t.Fatal(err) + } + if len(got) != 1 || got[0].ID != present { + t.Errorf("most viewed = %v, want only the present item %d", ids(got), present) + } +} + +// TestRecordViewIsPerUser guards the (user_id, item_id) key: one user's habit +// must not move another user's count. +func TestRecordViewIsPerUser(t *testing.T) { + db, libA, _, userA, userB := setupExplore(t) + ctx := context.Background() + item := newItem(t, db, libA, "ebooks", "x.epub", "X", "x", false, "2026-01-01T00:00:00.000Z") + // userB is not granted libA, but RecordView itself is keyed by user id. + grantRead(t, db, userB, libA) + + for i := 1; i <= 3; i++ { + n, err := RecordView(ctx, db, userA, item) + if err != nil { + t.Fatal(err) + } + if n != i { + t.Errorf("user A view %d returned %d", i, n) + } + } + n, err := RecordView(ctx, db, userB, item) + if err != nil { + t.Fatal(err) + } + if n != 1 { + t.Errorf("user B's first view returned %d, want 1 (counts are per user)", n) + } +} + +// ── Suggestions ──────────────────────────────────────────────────────── + +func TestSuggestionsReasonsAndUniqueness(t *testing.T) { + f := setupHome(t) + ctx := context.Background() + + sugg, err := Suggestions(ctx, f.db, f.userA, []int64{f.libA}, 20) + if err != nil { + t.Fatal(err) + } + if len(sugg) == 0 { + t.Fatal("no suggestions") + } + seen := map[int64]bool{} + for _, s := range sugg { + if s.Reason == "" { + t.Errorf("suggestion %d has an empty reason", s.ID) + } + if seen[s.ID] { + t.Errorf("suggestion %d returned twice", s.ID) + } + seen[s.ID] = true + if s.LibraryID != f.libA { + t.Errorf("suggestion %d leaked from library %d", s.ID, s.LibraryID) + } + } + // a1 is in progress, so it must not be suggested; a3 (same author) and a5 + // (same kind) are the unstarted candidates. + if seen[f.a1] { + t.Error("an in-progress item was suggested") + } + if !seen[f.a3] || !seen[f.a5] { + t.Errorf("expected same-author a3 and same-kind a5, got %v", idsOfSuggestions(sugg)) + } +} + +func idsOfSuggestions(s []suggestion) []int64 { + out := make([]int64, len(s)) + for i := range s { + out[i] = s[i].ID + } + return out +} + +// TestSuggestionsSeedIsNotScopedToVisibleLibraries documents a hardening gap, +// NOT desired behaviour. +// +// topEngagement() picks the user's most recent progress row across every +// library, including ones the user can no longer read (e.g. a revoked grant). +// The items Suggestions returns are still correctly scoped to the visible +// libraries, so nothing hidden is ever listed — but the inaccessible row's +// kind/author still shapes the shelf. Here a hidden library's ebooks progress +// makes the visible ebooks item come back as "Recently added" rather than being +// mislabelled "More books" from a library the user cannot read. +// +// The seed (topEngagement) is now scoped to visible libraries. Before that, a +// revoked library's most recent progress row still steered the home screen: +// the item below was labelled "More books" — a suggestion reason derived from +// content the user no longer had access to. +func TestSuggestionsSeedIsScopedToVisibleLibraries(t *testing.T) { + db := testDB(t) + ctx := context.Background() + + visible := newLibrary(t, db, "Seed Visible", "ebooks", "/seed/v") + hidden := newLibrary(t, db, "Seed Hidden", "ebooks", "/seed/h") + u := newUser(t, db, "seed-user") + grantRead(t, db, u, visible) + + newItem(t, db, visible, "ebooks", "v1.epub", "V1", "v1", false, "2026-01-01T00:00:00.000Z") + hid := newItem(t, db, hidden, "ebooks", "h1.epub", "H1", "h1", false, "2026-01-01T00:00:00.000Z") + newProgress(t, db, u, hid, 0.5, false, "2026-02-01T00:00:00.000Z") + + sugg, err := Suggestions(ctx, db, u, []int64{visible}, 20) + if err != nil { + t.Fatal(err) + } + if len(sugg) != 1 { + t.Fatalf("suggestions = %d, want 1", len(sugg)) + } + if sugg[0].LibraryID != visible { + t.Errorf("suggestion leaked library %d", sugg[0].LibraryID) + } + if sugg[0].Reason != "Recently added" { + t.Errorf("reason = %q; want \"Recently added\" — a revoked library's progress must not label the suggestion", sugg[0].Reason) + } +} + +// setupExplore builds two libraries with one user each and returns them. +func setupExplore(t *testing.T) (db *sql.DB, libA, libB, userA, userB int64) { + t.Helper() + db = testDB(t) + libA = newLibrary(t, db, "Explore A", "ebooks", "/explore/a") + libB = newLibrary(t, db, "Explore B", "audiobooks", "/explore/b") + userA = newUser(t, db, "explore-a") + userB = newUser(t, db, "explore-b") + grantRead(t, db, userA, libA) + grantRead(t, db, userB, libB) + return db, libA, libB, userA, userB +} diff --git a/internal/media/items_test.go b/internal/media/items_test.go new file mode 100644 index 0000000..cbb6df8 --- /dev/null +++ b/internal/media/items_test.go @@ -0,0 +1,250 @@ +package media + +import ( + "database/sql" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/mithral/vellum/internal/auth" +) + +// listResponse mirrors the JSON contract of handleListItems. +type listResponse struct { + Items []itemRow `json:"items"` + Total int `json:"total"` + Offset int `json:"offset"` + Limit int `json:"limit"` +} + +func listItems(t *testing.T, h *Handlers, u auth.User, query string) (int, listResponse) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/v1/items"+query, nil) + rec := httptest.NewRecorder() + h.handleListItems(rec, req, u) + var resp listResponse + if rec.Code == http.StatusOK { + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode list: %v (body %s)", err, rec.Body.String()) + } + } + return rec.Code, resp +} + +type listFixture struct { + db *sql.DB + libA int64 + libB int64 + userA int64 + userNone int64 + e1 int64 // libA ebooks, sort 01 + e2 int64 // libA ebooks, sort 02 + e3 int64 // libA ebooks, sort 03 + c1 int64 // libA comics, sort 04 + m1 int64 // libA ebooks, missing + b1 int64 // libB audiobooks +} + +func setupList(t *testing.T) listFixture { + t.Helper() + db := testDB(t) + f := listFixture{db: db} + + f.libA = newLibrary(t, db, "List A", "ebooks", "/list/a") + f.libB = newLibrary(t, db, "List B", "audiobooks", "/list/b") + f.userA = newUser(t, db, "list-a") + f.userNone = newUser(t, db, "list-none") + grantRead(t, db, f.userA, f.libA) + + const at = "2026-01-01T00:00:00.000Z" + f.e1 = newItem(t, db, f.libA, "ebooks", "e1.epub", "Echo", "01 echo", false, at) + f.e2 = newItem(t, db, f.libA, "ebooks", "e2.epub", "Delta", "02 delta", false, at) + f.e3 = newItem(t, db, f.libA, "ebooks", "e3.epub", "Charlie", "03 charlie", false, at) + f.c1 = newItem(t, db, f.libA, "comics", "c1.cbz", "Bravo", "04 bravo", false, at) + f.m1 = newItem(t, db, f.libA, "ebooks", "m1.epub", "Missing", "05 missing", true, at) + f.b1 = newItem(t, db, f.libB, "audiobooks", "b1.mp3", "Alpha", "06 alpha", false, at) + return f +} + +// TestListItemsTotalIsNotThePage pins the recently added offset/limit/total +// contract: total counts every matching row, independent of the page window. +func TestListItemsTotalIsNotThePage(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + + code, resp := listItems(t, h, auth.User{ID: f.userA}, "?limit=2") + if code != http.StatusOK { + t.Fatalf("status = %d, want 200", code) + } + if resp.Total != 4 { + t.Errorf("total = %d, want 4 (all matching present rows, not the page)", resp.Total) + } + if len(resp.Items) != 2 { + t.Errorf("page has %d items, want 2", len(resp.Items)) + } + if resp.Limit != 2 || resp.Offset != 0 { + t.Errorf("limit/offset = %d/%d, want 2/0", resp.Limit, resp.Offset) + } + for _, it := range resp.Items { + if it.ID == f.m1 { + t.Error("a missing item was listed") + } + } +} + +func TestListItemsPaging(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + u := auth.User{ID: f.userA} + + _, p1 := listItems(t, h, u, "?limit=2&offset=0") + _, p2 := listItems(t, h, u, "?limit=2&offset=2") + if len(p1.Items) != 2 || len(p2.Items) != 2 { + t.Fatalf("page sizes = %d/%d, want 2/2", len(p1.Items), len(p2.Items)) + } + if p1.Offset != 0 || p2.Offset != 2 { + t.Errorf("offsets = %d/%d, want 0/2", p1.Offset, p2.Offset) + } + + // Pages must not overlap, and must order by sort_title. + if p1.Items[0].ID != f.e1 || p1.Items[1].ID != f.e2 { + t.Errorf("page 1 = %v, want [%d %d]", idsFromRows(p1.Items), f.e1, f.e2) + } + if p2.Items[0].ID != f.e3 || p2.Items[1].ID != f.c1 { + t.Errorf("page 2 = %v, want [%d %d]", idsFromRows(p2.Items), f.e3, f.c1) + } + seen := map[int64]bool{} + for _, it := range append(append([]itemRow{}, p1.Items...), p2.Items...) { + if seen[it.ID] { + t.Errorf("item %d appears on both pages", it.ID) + } + seen[it.ID] = true + } + + // A page past the end is empty but still reports the true total. + _, beyond := listItems(t, h, u, "?limit=2&offset=99") + if len(beyond.Items) != 0 || beyond.Total != 4 { + t.Errorf("beyond page: items=%d total=%d, want 0/4", len(beyond.Items), beyond.Total) + } +} + +func TestListItemsFiltersCompose(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + u := auth.User{ID: f.userA} + + _, ebooks := listItems(t, h, u, "?kind=ebooks") + if ebooks.Total != 3 { + t.Errorf("kind=ebooks total = %d, want 3", ebooks.Total) + } + for _, it := range ebooks.Items { + if it.Kind != "ebooks" { + t.Errorf("kind filter returned a %q item", it.Kind) + } + } + + _, libA := listItems(t, h, u, "?library_id="+itoa(f.libA)) + if libA.Total != 4 { + t.Errorf("library_id=%d total = %d, want 4", f.libA, libA.Total) + } + + // kind and library_id compose (both must hold). + _, both := listItems(t, h, u, "?kind=ebooks&library_id="+itoa(f.libA)) + if both.Total != 3 { + t.Errorf("kind+library total = %d, want 3", both.Total) + } + for _, it := range both.Items { + if it.Kind != "ebooks" || it.LibraryID != f.libA { + t.Errorf("composed filter returned %+v", it) + } + } + + // A kind with no items is an empty page, not an error. + code, none := listItems(t, h, u, "?kind=movies") + if code != http.StatusOK || none.Total != 0 || len(none.Items) != 0 { + t.Errorf("unknown kind: status=%d total=%d items=%d", code, none.Total, len(none.Items)) + } +} + +func TestListItemsScoping(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + + // A user with no grants sees nothing. + _, none := listItems(t, h, auth.User{ID: f.userNone}, "?limit=100") + if none.Total != 0 || len(none.Items) != 0 { + t.Errorf("no-grant user saw total=%d items=%d", none.Total, len(none.Items)) + } + + // Asking explicitly for a library the user cannot read must not leak it. + code, leak := listItems(t, h, auth.User{ID: f.userA}, "?library_id="+itoa(f.libB)+"&limit=100") + if code != http.StatusOK { + t.Fatalf("status = %d, want 200", code) + } + if leak.Total != 0 || len(leak.Items) != 0 { + t.Errorf("user A read library %d: total=%d items=%d", f.libB, leak.Total, len(leak.Items)) + } + + // The unfiltered list contains only the readable library. + _, all := listItems(t, h, auth.User{ID: f.userA}, "?limit=100") + for _, it := range all.Items { + if it.LibraryID != f.libA { + t.Errorf("item %d leaked from library %d", it.ID, it.LibraryID) + } + } + if containsRowID(all.Items, f.b1) { + t.Error("user A's list contained another user's item") + } +} + +func TestListItemsInvalidLibraryID(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + for _, q := range []string{"?library_id=abc", "?library_id=-1", "?library_id=0"} { + code, _ := listItems(t, h, auth.User{ID: f.userA}, q) + if code != http.StatusBadRequest { + t.Errorf("%s status = %d, want 400", q, code) + } + } +} + +func TestListItemsDefaultsAndCaps(t *testing.T) { + f := setupList(t) + h := mediaHandlers(t, f.db) + u := auth.User{ID: f.userA} + + _, def := listItems(t, h, u, "") + if def.Limit != 300 || def.Offset != 0 { + t.Errorf("defaults = limit %d offset %d, want 300/0", def.Limit, def.Offset) + } + + // Out-of-range limit and negative offset fall back to the defaults. + for _, q := range []string{"?limit=0", "?limit=5000", "?limit=-1"} { + _, r := listItems(t, h, u, q) + if r.Limit != 300 { + t.Errorf("%s limit = %d, want 300", q, r.Limit) + } + } + _, neg := listItems(t, h, u, "?offset=-5") + if neg.Offset != 0 { + t.Errorf("negative offset = %d, want 0", neg.Offset) + } +} + +func idsFromRows(rows []itemRow) []int64 { + out := make([]int64, len(rows)) + for i, r := range rows { + out[i] = r.ID + } + return out +} + +func containsRowID(rows []itemRow, id int64) bool { + for _, r := range rows { + if r.ID == id { + return true + } + } + return false +} diff --git a/internal/search/search.go b/internal/search/search.go index 394db30..6f94cfd 100644 --- a/internal/search/search.go +++ b/internal/search/search.go @@ -1,4 +1,5 @@ -// Package search queries the FTS5 index over media item titles and paths. +// Package search queries the FTS5 index over media item titles, paths, and +// credited people (authors/artists). package search import ( @@ -15,6 +16,9 @@ type Result struct { Kind string `json:"kind"` Path string `json:"path"` Title string `json:"title"` + // Authors is the space-joined credit text denormalised onto media_items by + // migration 0009. It is exposed so a caller can show why a result matched. + Authors string `json:"authors"` } // Query searches visible libraries for a free-text string. @@ -44,8 +48,11 @@ func Query(ctx context.Context, db *sql.DB, q string, libraryIDs []int64, limit } args = append(args, limit) + // The MATCH predicate is unchanged: media_search now indexes `authors` too + // (migration 0009), so author terms match with no query-side change. The + // column is only selected so callers can display it. query := ` - SELECT i.id, i.library_id, i.kind, i.path, i.title + SELECT i.id, i.library_id, i.kind, i.path, i.title, i.authors FROM media_search s JOIN media_items i ON i.id = s.rowid WHERE media_search MATCH ? @@ -63,7 +70,7 @@ func Query(ctx context.Context, db *sql.DB, q string, libraryIDs []int64, limit out := []Result{} for rows.Next() { var r Result - if err := rows.Scan(&r.ID, &r.LibraryID, &r.Kind, &r.Path, &r.Title); err != nil { + if err := rows.Scan(&r.ID, &r.LibraryID, &r.Kind, &r.Path, &r.Title, &r.Authors); err != nil { return nil, err } out = append(out, r) diff --git a/internal/search/search_test.go b/internal/search/search_test.go index 25c4f6e..254a4ac 100644 --- a/internal/search/search_test.go +++ b/internal/search/search_test.go @@ -50,6 +50,26 @@ func addItem(t *testing.T, db *sql.DB, libraryID int64, kind, path, title string return id } +func addPerson(t *testing.T, db *sql.DB, name string) int64 { + t.Helper() + var id int64 + err := db.QueryRowContext(context.Background(), + `INSERT INTO people (name) VALUES (?) RETURNING id`, name).Scan(&id) + if err != nil { + t.Fatal(err) + } + return id +} + +func addCredit(t *testing.T, db *sql.DB, itemID, personID int64, role string, ord int) { + t.Helper() + if _, err := db.ExecContext(context.Background(), + `INSERT INTO item_people (item_id, person_id, role, ord) VALUES (?, ?, ?, ?)`, + itemID, personID, role, ord); err != nil { + t.Fatal(err) + } +} + func titles(results []Result) []string { out := make([]string, len(results)) for i, r := range results { @@ -101,6 +121,84 @@ func TestQueryFindsItems(t *testing.T) { } } +func TestQueryFindsItemsByAuthor(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := addLibrary(t, db, "ebooks", "ebooks") + id := addItem(t, db, lib, "ebooks", "notes.epub", "Notes on the Analytical Engine") + ada := addPerson(t, db, "Ada Lovelace") + addCredit(t, db, id, ada, "author", 0) + + // The credit is not in the title or path, so this only passes if the FTS + // index covers the denormalised authors column. + res, err := Query(ctx, db, "lovelace", []int64{lib}, 50) + if err != nil { + t.Fatalf("Query: %v", err) + } + if len(res) != 1 || res[0].ID != id { + t.Fatalf("author search results = %v", titles(res)) + } + if res[0].Authors != "Ada Lovelace" { + t.Errorf("Authors = %q, want %q", res[0].Authors, "Ada Lovelace") + } +} + +// TestAuthorIndexStaysInSync exercises every trigger 0009 installs on the +// credit tables: insert, delete, rename (people UPDATE), and moving a credit +// between items (item_people UPDATE). +func TestAuthorIndexStaysInSync(t *testing.T) { + db := testDB(t) + ctx := context.Background() + lib := addLibrary(t, db, "ebooks", "ebooks") + id := addItem(t, db, lib, "ebooks", "notes.epub", "Notes") + ada := addPerson(t, db, "Ada Lovelace") + + addCredit(t, db, id, ada, "author", 0) + if res, _ := Query(ctx, db, "lovelace", []int64{lib}, 50); len(res) != 1 { + t.Fatal("credit insert did not reach the index") + } + + // Deleting the credit must remove the author from the index. + if _, err := db.ExecContext(ctx, + `DELETE FROM item_people WHERE item_id = ? AND person_id = ?`, id, ada); err != nil { + t.Fatal(err) + } + if res, _ := Query(ctx, db, "lovelace", []int64{lib}, 50); len(res) != 0 { + t.Error("stale author survived the credit delete") + } + + // Re-add, then rename: the old name must stop matching and the new start. + addCredit(t, db, id, ada, "author", 0) + if res, _ := Query(ctx, db, "lovelace", []int64{lib}, 50); len(res) != 1 { + t.Fatal("re-added credit did not reach the index") + } + if _, err := db.ExecContext(ctx, + `UPDATE people SET name = 'Ada King' WHERE id = ?`, ada); err != nil { + t.Fatal(err) + } + if res, _ := Query(ctx, db, "lovelace", []int64{lib}, 50); len(res) != 0 { + t.Error("old name survived the rename") + } + if res, _ := Query(ctx, db, "king", []int64{lib}, 50); len(res) != 1 { + t.Error("new name did not reach the index after the rename") + } + + // Moving the credit to another item re-indexes both ends. + other := addItem(t, db, lib, "ebooks", "other.epub", "Other") + if _, err := db.ExecContext(ctx, + `UPDATE item_people SET item_id = ? WHERE item_id = ? AND person_id = ?`, + other, id, ada); err != nil { + t.Fatal(err) + } + res, err := Query(ctx, db, "king", []int64{lib}, 50) + if err != nil { + t.Fatal(err) + } + if len(res) != 1 || res[0].ID != other { + t.Errorf("moved credit: results = %v", titles(res)) + } +} + func TestQueryIsScopedToLibraries(t *testing.T) { db := testDB(t) ctx := context.Background() diff --git a/migrations/0009_search_authors.sql b/migrations/0009_search_authors.sql new file mode 100644 index 0000000..87dab20 --- /dev/null +++ b/migrations/0009_search_authors.sql @@ -0,0 +1,120 @@ +-- 0009_search_authors.sql — make credited people (authors, artists, directors) +-- searchable. +-- +-- 0003 indexed only `title` and `path`, so typing an author's name found +-- nothing even though `people` and `item_people` already held the credits. +-- Rather than make the search query join the credit tables — which would mean a +-- second MATCH against a second index, or an unindexed LIKE over a join — the +-- names are denormalised into a real column on `media_items`. The existing +-- external-content FTS5 design then keeps working unchanged in spirit: the +-- index still stores no text of its own, and the media_items triggers that +-- already keep it in sync simply gain the new column. +-- +-- `authors` is maintained by triggers on item_people/people, so a credit change +-- re-indexes the item no matter which code path writes it — the metadata +-- enricher today, anything else later. + +ALTER TABLE media_items ADD COLUMN authors TEXT NOT NULL DEFAULT ''; + +-- Drop the old two-column index and its triggers before rebuilding. +-- Triggers first: dropping the virtual table does not drop triggers that +-- reference it, and a write landing between the two drops would fire a trigger +-- against a table that no longer exists. +DROP TRIGGER media_items_ai; +DROP TRIGGER media_items_ad; +DROP TRIGGER media_items_au; +DROP TABLE media_search; + +CREATE VIRTUAL TABLE media_search USING fts5( + title, + path, + authors, + content='media_items', + content_rowid='id', + tokenize='unicode61 remove_diacritics 2' +); + +-- Populate the fresh index from current content before the backfill below. +-- FTS5's external-content 'delete' command assumes the rowid is already in the +-- index; issuing it for an absent rowid raises "database disk image is +-- malformed". The backfill UPDATE fires media_items_au (a delete+insert) for +-- every existing row, so the index must first hold those rows. On a fresh +-- database this indexes nothing and is a no-op. +INSERT INTO media_search(media_search) VALUES ('rebuild'); + +-- Recreate the media_items triggers verbatim from 0003, with `authors` added, +-- so every direct write to an item keeps the index correct. +CREATE TRIGGER media_items_ai AFTER INSERT ON media_items BEGIN + INSERT INTO media_search(rowid, title, path, authors) + VALUES (new.id, new.title, new.path, new.authors); +END; + +CREATE TRIGGER media_items_ad AFTER DELETE ON media_items BEGIN + INSERT INTO media_search(media_search, rowid, title, path, authors) + VALUES ('delete', old.id, old.title, old.path, old.authors); +END; + +CREATE TRIGGER media_items_au AFTER UPDATE ON media_items BEGIN + INSERT INTO media_search(media_search, rowid, title, path, authors) + VALUES ('delete', old.id, old.title, old.path, old.authors); + INSERT INTO media_search(rowid, title, path, authors) + VALUES (new.id, new.title, new.path, new.authors); +END; + +-- Credit triggers. Each recomputes media_items.authors for the affected item. +-- That UPDATE is itself on media_items, so media_items_au fires and rewrites +-- the FTS row — no FTS-specific code is needed here. (This is a single level of +-- trigger nesting, not recursion; and even with PRAGMA recursive_triggers=1 the +-- chain terminates because the FTS write targets the virtual table, not +-- media_items.) +CREATE TRIGGER item_people_ai AFTER INSERT ON item_people BEGIN + UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), '') + WHERE id = new.item_id; +END; + +CREATE TRIGGER item_people_ad AFTER DELETE ON item_people BEGIN + UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), '') + WHERE id = old.item_id; +END; + +-- An UPDATE can move a credit between items, so both ends are recomputed. +CREATE TRIGGER item_people_au AFTER UPDATE ON item_people BEGIN + UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), '') + WHERE id = old.item_id; + UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), '') + WHERE id = new.item_id; +END; + +-- A rename must not leave the old spelling in the index: recompute every item +-- credited to this person. people.name is UNIQUE COLLATE NOCASE, so renames are +-- rare, but a stale denormalised column is exactly what a rename would expose. +CREATE TRIGGER people_au AFTER UPDATE ON people BEGIN + UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), '') + WHERE id IN (SELECT item_id FROM item_people WHERE person_id = old.id); +END; + +-- Backfill: the triggers above only fire on future writes, so populate every +-- existing row once. The final rebuild re-derives the whole index from the +-- content table, which also normalises any transient FTS state left by the +-- per-row updates above. +UPDATE media_items SET authors = COALESCE(( + SELECT group_concat(p.name, ' ') FROM item_people ip + JOIN people p ON p.id = ip.person_id + WHERE ip.item_id = media_items.id), ''); + +INSERT INTO media_search(media_search) VALUES ('rebuild'); diff --git a/web/src/lib/components/Reader.svelte b/web/src/lib/components/Reader.svelte index f6473b1..d65663d 100644 --- a/web/src/lib/components/Reader.svelte +++ b/web/src/lib/components/Reader.svelte @@ -1,5 +1,4 @@