From 984965ed9fb19358026372bb1e3a2d8400175cde Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 8 Aug 2026 07:19:54 +0700 Subject: [PATCH] Split Series from Bookmark, keeping the wire format flat (#21) (#29) Co-authored-by: Sulthan Zaki Co-committed-by: Sulthan Zaki --- REDEPLOY.md | 3 +- backend/AGENTS.md | 31 +- backend/api_test.go | 138 +++++++- backend/internal/latest/poller.go | 75 ++--- backend/internal/latest/poller_test.go | 59 +++- .../internal/store/migrations/0002_series.sql | 45 +++ backend/internal/store/store.go | 256 ++++++++++----- backend/internal/store/store_test.go | 307 +++++++++++++++++- 8 files changed, 756 insertions(+), 158 deletions(-) create mode 100644 backend/internal/store/migrations/0002_series.sql diff --git a/REDEPLOY.md b/REDEPLOY.md index 5118915..76a9ecb 100644 --- a/REDEPLOY.md +++ b/REDEPLOY.md @@ -59,7 +59,7 @@ network can reach it — so every command below goes in through the container: ```bash $COMPOSE exec -T postgres psql -U bookmarks -d bookmarks -c '\dt' -# -> bookmarks, schema_migrations +# -> bookmarks, schema_migrations, series ``` Inside the container that connects over the local socket as the `bookmarks` @@ -100,6 +100,7 @@ docker run --rm -v "$BACKUP_DIR":/backup postgres:17-alpine \ pg_restore --list "/backup/bookmarks-$STAMP.dump" | grep 'TABLE DATA' # -> 1234; 0 0 TABLE DATA public bookmarks bookmarks # -> 1235; 0 0 TABLE DATA public schema_migrations bookmarks +# -> 1236; 0 0 TABLE DATA public series series # 2. Sanity-check the live row count you just captured. $COMPOSE exec -T postgres psql -U bookmarks -d bookmarks \ diff --git a/backend/AGENTS.md b/backend/AGENTS.md index d682c1c..358c572 100644 --- a/backend/AGENTS.md +++ b/backend/AGENTS.md @@ -21,7 +21,15 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN container per test binary (`TestMain` -> `pgtest.Main`) and hands each test its own database (`pgtest.URL(t)`). A package whose tests touch the store must have that `TestMain`. -- **Single-user store.** One `bookmarks` table keyed `:` (`asura`|`demonic`|`comix`|`kagane`|`novelfull`|`lightnovelworld`), with a `kind` column (`manga`|`novel`) splitting the two libraries. Sync **last-write-wins**. Schema and endpoint list in plan. +- **Single-user store, two tables.** `series` keyed `(site, series_id)` + (`asura`|`demonic`|`comix`|`kagane`|`novelfull`|`lightnovelworld`) owns the + shared facts — title, cover, canonical URL, `kind` (`manga`|`novel`), + Latest Chapter, `latest_checked_at` — and `bookmarks` holds only what + differs between readers: progress, favourite, lifecycle bucket, + `updated_at`. Sync **last-write-wins**; the wire format stays flat + (ADR-0004). `Store.Upsert` decomposes one flat body across both tables and + enforces the ownership rule: client `title`/`series_url`/`cover` are + written only when the series row is new (ADR-0003). - **Endpoints:** `GET /bookmarks`, `PUT /bookmarks/{key}` (upsert; see `updated_at` rule below), `DELETE /bookmarks/{key}`, `GET /healthz` (no auth). - **Web UI:** same binary serve password-gated browser UI on second hostname — `GET /` (list, or login page when no session), @@ -53,22 +61,25 @@ Guidance for OpenCode (and Claude Code) working under `backend/`. See root `AGEN network access, so `latest_chapter` stay fresh when user not browsing. Second, parallel signal — userscript keep own `maybeCaptureLatestOnSeriesPage`/`backgroundRefreshLatest` logic unchanged. - Two independent clocks: per-bookmark cooldown (`latest_checked_at` column, + Two independent clocks: per-series cooldown (`series.latest_checked_at`, enforced by `Store.DueForLatestCheck`'s WHERE clause) and wake interval. - Row stamped *before* fetch so broken series wait out full - cooldown instead of retrying every tick, and writes go through - `Store.Get` + `Store.Upsert` so new chapter never reorders list. + The poller walks **Series, not Bookmarks** — a series referenced by several + bookmarks is fetched once per cycle, and the due queue orders + `reader_count DESC, latest_checked_at ASC` (ADR-0003). Series row stamped + *before* fetch so broken series wait out full cooldown instead of retrying + every tick; found chapter written straight to the series row via + `Store.SetLatestChapter`, so a bookmark's `updated_at` — and the list + order — is never touched. Fetches use `bogdanfinn/tls-client` with Chrome profile as defence in depth against fingerprint-based blocking; any failure log and skip. kagane and novelfull sit behind Cloudflare JavaScript challenges the TLS client can't clear, so they are browser-only: fetched over CDP via `BROWSER_WS_URL`, and simply not polled when that's unset. See `docs/superpowers/specs/2026-07-26-server-latest-chapter-polling-design.md`. - Poller's `Store.Get` + `Store.Upsert` not wrapped in transaction, so - userscript `PUT` that commits between the two can get overwritten by - poller's stale re-read — reverting that read progress and, since stored - value now differs, moving `updated_at` and reordering list. Known, - accepted limitation for single-user deployment, not bug to fix. + The poller's series write is a single-column UPDATE + (`Store.SetLatestChapter`), not a read-modify-write of the whole bookmark: + it cannot revert read progress or move `updated_at`, so the old + stale-re-read race is gone with the Get+Upsert flow. - **`updated_at` drives list order, so moves only on real reading progress:** server apply its timestamp when row new or `last_chapter_num` changes, else keep stored value — favouriting series or recording newly published chapter must not reorder list. `PUT` therefore returns row **as stored**, clients must adopt that response rather than own payload. See `plans/2026-07-25-bookmark-list-favorites-design.md` §4. - **Lifecycle buckets:** `status` on each bookmark is `reading` | `archived` | `finished`, orthogonal to `favorite`. Archived and finished appear only in diff --git a/backend/api_test.go b/backend/api_test.go index e4282c8..1b280f8 100644 --- a/backend/api_test.go +++ b/backend/api_test.go @@ -50,26 +50,35 @@ func auth(req *http.Request) *http.Request { func floatPtr(f float64) *float64 { return &f } -// seedForCheck inserts a bookmark and forces its latest_checked_at. +// seedForCheck inserts a bookmark (and with it its series) and forces the +// series' latest_checked_at. func seedForCheck(t *testing.T, s *store.Store, key, seriesURL string, checkedAt int64) { t.Helper() + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } if _, err := s.Upsert(store.Bookmark{ Key: key, - Site: "asura", - SeriesID: key, + Site: site, + SeriesID: seriesID, SeriesURL: seriesURL, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed %q: %v", key, err) } - if err := s.MarkLatestChecked(key, checkedAt); err != nil { + if err := s.MarkLatestChecked(site, seriesID, checkedAt); err != nil { t.Fatalf("seed mark %q: %v", key, err) } } func readLatestCheckedAt(t *testing.T, s *store.Store, key string) int64 { t.Helper() - ts, err := s.LatestCheckedAt(key) + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } + ts, err := s.LatestCheckedAt(site, seriesID) if err != nil { t.Fatalf("LatestCheckedAt %q: %v", key, err) } @@ -229,6 +238,125 @@ func TestBookmarkRoundTrip(t *testing.T) { } } +// The wire contract (ADR-0004): GET and PUT speak exactly the flat field set +// they always did, with the series-owned fields as siblings of the bookmark +// fields, not nested. Asserted as a key set, not by inspection. +func TestFlatWireFieldSet(t *testing.T) { + srv := newTestServer(t) + key := "comix:some-title" + in := store.Bookmark{ + Key: key, + Site: "comix", + SeriesID: "some-title", + Title: "Some Title", + SeriesURL: "https://comix.to/title/some-title", + Cover: "https://comix.to/covers/some-title.jpg", + LastChapter: "Chapter 7", + LastChapterNum: 7, + LastChapterURL: "https://comix.to/title/some-title/ch/7", + Favorite: true, + LatestChapter: "Chapter 8", + LatestChapterNum: floatPtr(8), + Status: store.StatusArchived, + Kind: store.KindManga, + } + body, _ := json.Marshal(in) + + wantKeys := map[string]bool{ + "key": true, "site": true, "series_id": true, "title": true, + "series_url": true, "cover": true, "last_chapter": true, + "last_chapter_num": true, "last_chapter_url": true, "favorite": true, + "latest_chapter": true, "latest_chapter_num": true, "updated_at": true, + "status": true, "kind": true, + } + checkFlat := func(t *testing.T, payload []byte) map[string]json.RawMessage { + t.Helper() + var obj map[string]json.RawMessage + if err := json.Unmarshal(payload, &obj); err != nil { + t.Fatalf("decode: %v", err) + } + if len(obj) != len(wantKeys) { + t.Fatalf("field count = %d, want %d (%s)", len(obj), len(wantKeys), payload) + } + for k := range obj { + if !wantKeys[k] { + t.Fatalf("unexpected field %q", k) + } + } + return obj + } + + // PUT + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, auth(httptest.NewRequest(http.MethodPut, "/bookmarks/"+key, bytes.NewReader(body)))) + if rr.Code != http.StatusOK { + t.Fatalf("PUT status = %d, want 200", rr.Code) + } + checkFlat(t, rr.Body.Bytes()) + + // Every field round-trips with its value, and updated_at is server-stamped. + var stored store.Bookmark + if err := json.Unmarshal(rr.Body.Bytes(), &stored); err != nil { + t.Fatalf("decode PUT response: %v", err) + } + latestNum := floatPtr(8) + want := store.Bookmark{ + Key: key, Site: "comix", SeriesID: "some-title", + Title: in.Title, SeriesURL: in.SeriesURL, Cover: in.Cover, + LastChapter: in.LastChapter, LastChapterNum: in.LastChapterNum, + LastChapterURL: in.LastChapterURL, Favorite: true, + LatestChapter: in.LatestChapter, LatestChapterNum: latestNum, + Status: store.StatusArchived, Kind: store.KindManga, + } + if stored.Title != want.Title || stored.SeriesURL != want.SeriesURL || stored.Cover != want.Cover || + stored.LastChapter != want.LastChapter || stored.LastChapterNum != want.LastChapterNum || + stored.LastChapterURL != want.LastChapterURL || stored.Favorite != want.Favorite || + stored.LatestChapter != want.LatestChapter || + stored.LatestChapterNum == nil || *stored.LatestChapterNum != *want.LatestChapterNum || + stored.Status != want.Status || stored.Kind != want.Kind { + t.Fatalf("PUT response = %+v, want %+v", stored, want) + } + if stored.UpdatedAt == 0 { + t.Fatal("updated_at not server-stamped") + } + + // GET reports the same flat shape. + list := getBookmarks(t, srv) + if len(list) != 1 { + t.Fatalf("list = %d items, want 1", len(list)) + } + body2, _ := json.Marshal(list[0]) + checkFlat(t, body2) +} + +// A PUT naming an existing series must ignore client-supplied title, cover and +// URL — the security boundary from ADR-0003, where a hostile site's scraped +// values could otherwise land on a shared row — while progress still lands. +func TestPutExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) { + srv := newTestServer(t) + key := "asura:solo" + + first := putBookmark(t, srv, key, store.Bookmark{ + Title: "Solo Leveling", + SeriesURL: "https://asurascans.com/comics/solo", + Cover: "https://asurascans.com/covers/solo.jpg", + LastChapterNum: 10, + }) + + second := putBookmark(t, srv, key, store.Bookmark{ + Title: "Scraped Rename", + SeriesURL: "https://evil.example/solo", + Cover: "https://evil.example/solo.jpg", + LastChapterNum: 11, + }) + if second.Title != first.Title || second.SeriesURL != first.SeriesURL || second.Cover != first.Cover { + t.Fatalf("stored = %+v, want original title/url/cover kept", second) + } + if second.LastChapterNum != 11 { + t.Fatalf("LastChapterNum = %v, want 11 — progress must still land", second.LastChapterNum) + } +} + // putBookmark PUTs b at key and returns the bookmark the server echoes back, // which is the row as actually stored (not the request payload). func putBookmark(t *testing.T, srv http.Handler, key string, b store.Bookmark) store.Bookmark { diff --git a/backend/internal/latest/poller.go b/backend/internal/latest/poller.go index 57e764f..2498c49 100644 --- a/backend/internal/latest/poller.go +++ b/backend/internal/latest/poller.go @@ -23,9 +23,9 @@ type Fetcher interface { // Two clocks, deliberately independent: // // - Interval is how often this goroutine wakes up and looks. -// - Cooldown is how long one bookmark rests since its own last check. +// - Cooldown is how long one series rests since its own last check. // -// Only the cooldown is per bookmark, and it is enforced by the WHERE clause in +// Only the cooldown is per series, and it is enforced by the WHERE clause in // DueForLatestCheck rather than by any timer. Shortening Interval therefore // cannot shorten anyone's cooldown; it only makes the poller wake up and find // nothing due more often. @@ -78,7 +78,7 @@ func (p *Poller) Run(ctx context.Context) { } } -// runOnce processes one batch of due bookmarks. +// runOnce processes one batch of due series. func (p *Poller) runOnce(ctx context.Context) { cutoff := p.Now().Add(-p.Cooldown).UnixMilli() due, err := p.Store.DueForLatestCheck(cutoff, p.Batch) @@ -88,7 +88,7 @@ func (p *Poller) runOnce(ctx context.Context) { } checked := 0 - for i, b := range due { + for i, sr := range due { if ctx.Err() != nil { break } @@ -107,7 +107,7 @@ func (p *Poller) runOnce(ctx context.Context) { if stopped { break } - p.checkOne(ctx, b) + p.checkOne(ctx, sr) checked++ } // due vs checked is how you tell which constraint is binding: ticks that @@ -119,10 +119,10 @@ func (p *Poller) runOnce(ctx context.Context) { // checkOne re-checks one series. Every failure path here is "log and move on": // the poller is a best-effort enhancement, and no single bad series may stall a // batch or take down the process. -func (p *Poller) checkOne(ctx context.Context, b store.Bookmark) { +func (p *Poller) checkOne(ctx context.Context, sr store.Series) { defer func() { if r := recover(); r != nil { - log.Printf("latest poll %q: recovered from panic: %v", b.Key, r) + log.Printf("latest poll %q: recovered from panic: %v", sr.Key(), r) } }() @@ -130,8 +130,8 @@ func (p *Poller) checkOne(ctx context.Context, b store.Bookmark) { // mid-request still consumes the cooldown. Otherwise a renamed or deleted // series would be retried on every single tick forever. The userscript // stamps in the same order and for the same reason (L471-473). - if err := p.Store.MarkLatestChecked(b.Key, p.Now().UnixMilli()); err != nil { - log.Printf("latest poll %q: mark checked: %v", b.Key, err) + if err := p.Store.MarkLatestChecked(sr.Site, sr.SeriesID, p.Now().UnixMilli()); err != nil { + log.Printf("latest poll %q: mark checked: %v", sr.Key(), err) return } @@ -142,69 +142,52 @@ func (p *Poller) checkOne(ctx context.Context, b store.Bookmark) { // link-local/internal addresses or non-https schemes. The cooldown above // is already consumed, so a row that never passes this check is retried at // cooldown pace rather than hot-looping. - if !fetchableSeriesURL(b.Site, b.SeriesURL) { - log.Printf("latest poll %q: not fetchable: site=%q url=%q", b.Key, b.Site, b.SeriesURL) + if !fetchableSeriesURL(sr.Site, sr.SeriesURL) { + log.Printf("latest poll %q: not fetchable: site=%q url=%q", sr.Key(), sr.Site, sr.SeriesURL) return } - f := p.fetcherFor(b.Site) + f := p.fetcherFor(sr.Site) if f == nil { - log.Printf("latest poll %q: no fetcher for site %q", b.Key, b.Site) + log.Printf("latest poll %q: no fetcher for site %q", sr.Key(), sr.Site) return } - body, status, err := f.Get(ctx, b.SeriesURL) + body, status, err := f.Get(ctx, sr.SeriesURL) if err != nil { - log.Printf("latest poll %q: fetch %s: %v", b.Key, b.SeriesURL, err) + log.Printf("latest poll %q: fetch %s: %v", sr.Key(), sr.SeriesURL, err) return } if status != 200 { - log.Printf("latest poll %q: fetch %s: status %d", b.Key, b.SeriesURL, status) + log.Printf("latest poll %q: fetch %s: status %d", sr.Key(), sr.SeriesURL, status) return } - latest, ok := latestChapterFrom(b.Site, b.SeriesURL, body) + latest, ok := latestChapterFrom(sr.Site, sr.SeriesURL, body) if !ok { // Most likely a challenge page or a layout change. Either way the row is // already stamped, so this waits out a cooldown instead of hot-looping. - log.Printf("latest poll %q: no chapter links in %d bytes", b.Key, len(body)) + log.Printf("latest poll %q: no chapter links in %d bytes", sr.Key(), len(body)) return } - // Re-read: the row may have been updated or deleted while the fetch was in - // flight, and writing b back wholesale would undo that. - // - // ponytail: non-transactional read-modify-write, wrap Get+Upsert in a tx if - // this ever runs for more than one user. A client PUT that commits between - // these two statements is lost to the stale re-read — reverting read - // progress or a status change, and moving updated_at because the stored - // value now differs. Accepted for a single-user deployment: the window is - // milliseconds and the loser is one poll cycle. - cur, found, err := p.Store.Get(b.Key) - if err != nil { - log.Printf("latest poll %q: reread: %v", b.Key, err) - return - } - if !found { - return - } // Equality, not >, mirroring the userscript (L427): a site that retracts a - // chapter should correct the stored number downward. - if cur.LatestChapterNum != nil && *cur.LatestChapterNum == latest.Num { + // chapter should correct the stored number downward. The comparison is + // against the due-query snapshot; a concurrent write in between only costs + // one redundant UPDATE of the same absolute value, never a wrong one. + if sr.LatestChapterNum != nil && *sr.LatestChapterNum == latest.Num { return } - num := latest.Num - cur.LatestChapter = latest.Label - cur.LatestChapterNum = &num - // A candidate only. last_chapter_num is untouched, so the CASE in Upsert - // keeps the stored updated_at and the bookmark list does not reorder. - cur.UpdatedAt = p.Now().UnixMilli() - if _, err := p.Store.Upsert(cur); err != nil { - log.Printf("latest poll %q: upsert: %v", b.Key, err) + // Series-level write: the row is shared, so one update refreshes every + // bookmark joining to it, and the bookmark's updated_at is never touched — + // a newly published chapter is not reading progress and must not reorder + // the list. + if err := p.Store.SetLatestChapter(sr.Site, sr.SeriesID, latest.Label, latest.Num); err != nil { + log.Printf("latest poll %q: set latest chapter: %v", sr.Key(), err) return } - log.Printf("latest poll %q: latest is now %s", b.Key, latest.Label) + log.Printf("latest poll %q: latest is now %s", sr.Key(), latest.Label) } // fetchableSeriesURL reports whether site is a site latestChapterFrom knows how diff --git a/backend/internal/latest/poller_test.go b/backend/internal/latest/poller_test.go index a9fa3d1..8c44c79 100644 --- a/backend/internal/latest/poller_test.go +++ b/backend/internal/latest/poller_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "strings" "sync" "testing" "time" @@ -25,26 +26,35 @@ func newTestStore(t *testing.T) *store.Store { return s } -// seedForCheck inserts a bookmark and forces its latest_checked_at. +// seedForCheck inserts a bookmark (and with it its series) and forces the +// series' latest_checked_at. func seedForCheck(t *testing.T, s *store.Store, key, seriesURL string, checkedAt int64) { t.Helper() + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } if _, err := s.Upsert(store.Bookmark{ Key: key, - Site: "asura", - SeriesID: key, + Site: site, + SeriesID: seriesID, SeriesURL: seriesURL, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed %q: %v", key, err) } - if err := s.MarkLatestChecked(key, checkedAt); err != nil { + if err := s.MarkLatestChecked(site, seriesID, checkedAt); err != nil { t.Fatalf("seed mark %q: %v", key, err) } } func readLatestCheckedAt(t *testing.T, s *store.Store, key string) int64 { t.Helper() - ts, err := s.LatestCheckedAt(key) + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } + ts, err := s.LatestCheckedAt(site, seriesID) if err != nil { t.Fatalf("LatestCheckedAt %q: %v", key, err) } @@ -217,6 +227,41 @@ func TestRunOnceRespectsBatchLimit(t *testing.T) { } } +// The point of the split (ADR-0003): a series referenced by several bookmarks +// is fetched once per due cycle, not once per bookmark. Today the bookmark key +// is :, so the second bookmark only exists once keys stop +// being derived from the series identity (issue #22). +func TestRunOnceFetchesSharedSeriesOnce(t *testing.T) { + s := newTestStore(t) + // The slug must match the fixture's own anchors: asura's parser scopes + // chapter links to the stored slug. + const slug = "chronicles-of-the-demon-faction-f886a8af" + const url = "https://asurascans.com/comics/" + slug + seedForCheck(t, s, "asura:"+slug, url, 0) + if _, err := s.Upsert(store.Bookmark{ + Key: "asura:" + slug + ":2", Site: "asura", SeriesID: slug, UpdatedAt: 2000, + }); err != nil { + t.Fatalf("seed second reader: %v", err) + } + + f := &fakeFetcher{body: asuraSeriesFixture, status: 200} + newTestPoller(t, s, f, time.UnixMilli(5_000_000)).runOnce(context.Background()) + + if got := f.callCount(); got != 1 { + t.Fatalf("fetched shared series %d times, want 1", got) + } + // Both bookmarks join to the same updated series row. + for _, key := range []string{"asura:" + slug, "asura:" + slug + ":2"} { + b, ok, err := s.Get(key) + if err != nil || !ok { + t.Fatalf("Get %s: %v ok=%v", key, err, ok) + } + if b.LatestChapterNum == nil || *b.LatestChapterNum != 181 { + t.Fatalf("%s LatestChapterNum = %v, want 181", key, b.LatestChapterNum) + } + } +} + // One unreachable series must not abandon the rest of the batch. func TestRunOnceOneBadSeriesDoesNotStallBatch(t *testing.T) { s := newTestStore(t) @@ -330,8 +375,8 @@ func TestCheckOneValidatesSeriesURLBeforeFetching(t *testing.T) { now := time.UnixMilli(4_000_000) f := &fakeFetcher{body: asuraSeriesFixture, status: 200} - newTestPoller(t, s, f, now).checkOne(context.Background(), store.Bookmark{ - Key: key, Site: tt.site, SeriesURL: tt.seriesURL, + newTestPoller(t, s, f, now).checkOne(context.Background(), store.Series{ + Site: tt.site, SeriesID: "x", SeriesURL: tt.seriesURL, }) if got := f.callCount(); got != tt.wantCalls { diff --git a/backend/internal/store/migrations/0002_series.sql b/backend/internal/store/migrations/0002_series.sql new file mode 100644 index 0000000..25c6242 --- /dev/null +++ b/backend/internal/store/migrations/0002_series.sql @@ -0,0 +1,45 @@ +-- One row per distinct work, shared by every bookmark that tracks it +-- (ADR-0003). Keyed (site, series_id), the pair a bookmark key decomposes +-- into. title/series_url/cover are written once, at creation, and never +-- again: client-supplied values are ignored once the row exists and the +-- poller is the only party that may change them. kind and the latest-chapter +-- fields are last-write-wins like the bookmark's own fields. +CREATE TABLE series ( + site text NOT NULL, + series_id text NOT NULL, + title text NOT NULL DEFAULT '', + series_url text NOT NULL DEFAULT '', + cover text NOT NULL DEFAULT '', + kind text NOT NULL DEFAULT 'manga', + latest_chapter text NOT NULL DEFAULT '', + latest_chapter_num double precision, + -- When the server last polled this series, unix ms; 0 means never, and sorts + -- first so a new bookmark is picked up on the next tick with no special case. + latest_checked_at bigint NOT NULL DEFAULT 0, + PRIMARY KEY (site, series_id) +); + +-- Backfill from today's rows. The bookmark key's uniqueness makes +-- (site, series_id) unique in practice; DISTINCT is belt and braces. +INSERT INTO series (site, series_id, title, series_url, cover, kind, + latest_chapter, latest_chapter_num, latest_checked_at) +SELECT DISTINCT site, series_id, title, series_url, cover, kind, + latest_chapter, latest_chapter_num, latest_checked_at +FROM bookmarks; + +-- The bookmark keeps only what differs between readers (ADR-0003): progress, +-- favourite, lifecycle bucket. The dropped columns now live on series. +ALTER TABLE bookmarks + DROP COLUMN title, + DROP COLUMN series_url, + DROP COLUMN cover, + DROP COLUMN kind, + DROP COLUMN latest_chapter, + DROP COLUMN latest_chapter_num, + DROP COLUMN latest_checked_at; + +-- A bookmark may not point at a series that does not exist. No cascade: a +-- series outlives its last bookmark, and deleting one is not a store operation. +ALTER TABLE bookmarks + ADD CONSTRAINT bookmarks_series_fk + FOREIGN KEY (site, series_id) REFERENCES series (site, series_id); diff --git a/backend/internal/store/store.go b/backend/internal/store/store.go index 956009a..9e86ea3 100644 --- a/backend/internal/store/store.go +++ b/backend/internal/store/store.go @@ -19,6 +19,11 @@ import ( // // LastChapter* is the user's read progress; LatestChapter* is the newest // chapter the site has published, captured opportunistically by the userscript. +// +// Title, SeriesURL, Cover, Kind and LatestChapter* live on the shared Series +// row (ADR-0003) and are joined in on read; Bookmark carries only what differs +// between readers: progress, favourite, lifecycle bucket, updated_at. The wire +// format stays flat regardless — see ADR-0004. type Bookmark struct { Key string `json:"key"` Site string `json:"site"` @@ -42,6 +47,35 @@ type Bookmark struct { Kind string `json:"kind"` } +// Series is one distinct work, shared by every bookmark that tracks it. It is +// keyed (site, series_id) — the pair a bookmark key decomposes into — and +// exists once no matter how many bookmarks point at it (ADR-0003). +// +// Title, SeriesURL and Cover are written once, at creation: a PUT naming an +// existing Series has them ignored, and only the backend's own Poll may change +// them. Kind and the latest-chapter fields are last-write-wins like the +// bookmark's own fields. Never serialized: the wire format is the flat +// Bookmark (ADR-0004). +type Series struct { + Site string + SeriesID string + Title string + SeriesURL string + Cover string + Kind string + LatestChapter string + LatestChapterNum *float64 // nil until first captured + LatestCheckedAt int64 // unix ms; see MarkLatestChecked + + // readerCount is the number of bookmarks referencing this series, filled + // only by the due-queue query that orders on it. + readerCount int +} + +// Key returns the canonical identity in bookmark-key form (":"), +// used by the poller's logs and by tests asserting on the due queue. +func (s Series) Key() string { return s.Site + ":" + s.SeriesID } + // HasNewChapter reports whether the site has published past the read point. // A nil LatestChapterNum means nothing has been captured yet, which is not the // same as "nothing new". @@ -122,10 +156,18 @@ const ( var migrations embed.FS // bookmarkColumns is the only value ever concatenated into query text. It is a -// compile-time constant; every request value is bound as a parameter. -const bookmarkColumns = `key, site, series_id, title, series_url, cover, - last_chapter, last_chapter_num, last_chapter_url, - favorite, latest_chapter, latest_chapter_num, updated_at, status, kind` +// compile-time constant; every request value is bound as a parameter. The +// series-owned fields are joined in from the series table, in scanBookmark +// order, so the flat Bookmark reads back whole despite the split (ADR-0004). +const bookmarkColumns = `b.key, b.site, b.series_id, s.title, s.series_url, s.cover, + b.last_chapter, b.last_chapter_num, b.last_chapter_url, + b.favorite, s.latest_chapter, s.latest_chapter_num, b.updated_at, b.status, s.kind` + +// seriesColumns is the series row in scanSeries order, used by the poller's +// due query. latest_checked_at lives only on series — see MarkLatestChecked +// for why it stays off every client-visible write. +const seriesColumns = `s.site, s.series_id, s.title, s.series_url, s.cover, + s.kind, s.latest_chapter, s.latest_chapter_num, s.latest_checked_at` // Store is the Postgres-backed bookmark store. type Store struct { @@ -237,14 +279,37 @@ func scanBookmark(scan func(...any) error) (Bookmark, error) { return b, nil } +// scanSeries reads one row in seriesColumns order, plus the due query's +// reader_count column. latest_chapter_num is NULL until the first capture, +// same as on the bookmark read path. +func scanSeries(scan func(...any) error) (Series, error) { + var ( + sr Series + latestChapterNum sql.NullFloat64 + ) + if err := scan( + &sr.Site, &sr.SeriesID, &sr.Title, &sr.SeriesURL, &sr.Cover, + &sr.Kind, &sr.LatestChapter, &latestChapterNum, &sr.LatestCheckedAt, + &sr.readerCount, + ); err != nil { + return Series{}, err + } + if latestChapterNum.Valid { + sr.LatestChapterNum = &latestChapterNum.Float64 + } + return sr, nil +} + // Close releases the underlying database handle. func (s *Store) Close() error { return s.db.Close() } -// List returns every bookmark, newest activity first. +// List returns every bookmark, newest activity first. Series-owned fields are +// joined in, so each Bookmark reads back whole and flat (ADR-0004). func (s *Store) List() ([]Bookmark, error) { rows, err := s.db.Query(`SELECT ` + bookmarkColumns + ` - FROM bookmarks - ORDER BY updated_at DESC`) + FROM bookmarks b + JOIN series s ON s.site = b.site AND s.series_id = b.series_id + ORDER BY b.updated_at DESC`) if err != nil { return nil, fmt.Errorf("query bookmarks: %w", err) } @@ -266,7 +331,9 @@ func (s *Store) List() ([]Bookmark, error) { // the fields they do not touch. func (s *Store) Get(key string) (Bookmark, bool, error) { b, err := scanBookmark(s.db.QueryRow( - `SELECT `+bookmarkColumns+` FROM bookmarks WHERE key = $1`, key).Scan) + `SELECT `+bookmarkColumns+` FROM bookmarks b + JOIN series s ON s.site = b.site AND s.series_id = b.series_id + WHERE b.key = $1`, key).Scan) if errors.Is(err, sql.ErrNoRows) { return Bookmark{}, false, nil } @@ -277,7 +344,15 @@ func (s *Store) Get(key string) (Bookmark, bool, error) { } // Upsert inserts or replaces a bookmark by key (last-write-wins) and returns -// the row as actually stored. +// the row as actually stored — one flat object with the series-owned fields +// joined in, exactly as GET reports it (ADR-0004). +// +// The flat body is decomposed across two tables in one transaction. The series +// row is written first (the bookmarks FK requires it to exist), then the +// bookmark row. On the series side, title/series_url/cover are applied only +// when the row is brand new: once a series exists, client-supplied values are +// ignored, because the row is shared and the values are scraped page content — +// see ADR-0003. Kind and the latest-chapter fields are last-write-wins. // // b.UpdatedAt is only a candidate: it is applied when the row is new or when // last_chapter_num changes, and otherwise the stored value is kept. Clients @@ -296,53 +371,65 @@ func (s *Store) Upsert(b Bookmark) (Bookmark, error) { latestNum = *b.LatestChapterNum } - // IS DISTINCT FROM is Postgres's null-safe comparison, and it is what - // implements the ordering rule. Within DO UPDATE, a bare column is the - // stored row and excluded.* is the incoming one; a brand-new key never - // reaches this clause, so it keeps the fresh timestamp from VALUES. - // - // The status and kind columns resolve on the VALUES side, not in the - // conflict clause: excluded.* is the row *after* these expressions are - // evaluated, so a default applied there would look identical to a real - // 'reading' / 'manga' and would overwrite an archived or novel row on - // every PUT from a client that knows nothing about the column. Resolved - // once here, an empty incoming status or kind means "keep what is - // stored", and only a brand-new row falls through to the literal - // default. The subquery runs inside this transaction, so it sees the - // row this statement is about to conflict with. + // The kind column resolves on the VALUES side, not in the conflict clause: + // excluded.* is the row *after* these expressions are evaluated, so a + // default applied there would look identical to a real 'manga' and would + // overwrite a novel series on every PUT from a client that knows nothing + // about the column. Resolved once here, an empty incoming kind means "keep + // what is stored", and only a brand-new row falls through to the literal + // default. The subquery runs inside this transaction, so it sees the row + // this statement is about to conflict with. Same pattern as the status + // COALESCE on the bookmark insert below. // // The ::text casts are load-bearing: inside COALESCE/NULLIF there is no // target column to infer the parameter type from, and Postgres rejects the // statement rather than guessing. if _, err := tx.Exec(` - INSERT INTO bookmarks (`+bookmarkColumns+`) - VALUES ($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11, $12, $13, - COALESCE(NULLIF($14::text, ''), (SELECT status FROM bookmarks WHERE key = $1), 'reading'), - COALESCE(NULLIF($15::text, ''), (SELECT kind FROM bookmarks WHERE key = $1), 'manga')) + INSERT INTO series (site, series_id, title, series_url, cover, kind, + latest_chapter, latest_chapter_num) + VALUES ($1, $2, $3, $4, $5, + COALESCE(NULLIF($6::text, ''), (SELECT kind FROM series WHERE site = $1 AND series_id = $2), 'manga'), + $7, $8) + ON CONFLICT (site, series_id) DO UPDATE SET + kind=excluded.kind, + latest_chapter=excluded.latest_chapter, + latest_chapter_num=excluded.latest_chapter_num`, + b.Site, b.SeriesID, b.Title, b.SeriesURL, b.Cover, b.Kind, + b.LatestChapter, latestNum); err != nil { + return Bookmark{}, fmt.Errorf("upsert series for %q: %w", b.Key, err) + } + + // IS DISTINCT FROM is Postgres's null-safe comparison, and it is what + // implements the ordering rule. Within DO UPDATE, a bare column is the + // stored row and excluded.* is the incoming one; a brand-new key never + // reaches this clause, so it keeps the fresh timestamp from VALUES. + if _, err := tx.Exec(` + INSERT INTO bookmarks (key, site, series_id, last_chapter, last_chapter_num, + last_chapter_url, favorite, status, updated_at) + VALUES ($1, $2, $3, $4, $5, $6, $7, + COALESCE(NULLIF($8::text, ''), (SELECT status FROM bookmarks WHERE key = $1), 'reading'), + $9) ON CONFLICT (key) DO UPDATE SET - site=excluded.site, series_id=excluded.series_id, title=excluded.title, - series_url=excluded.series_url, cover=excluded.cover, + site=excluded.site, series_id=excluded.series_id, last_chapter=excluded.last_chapter, last_chapter_num=excluded.last_chapter_num, last_chapter_url=excluded.last_chapter_url, favorite=excluded.favorite, - latest_chapter=excluded.latest_chapter, - latest_chapter_num=excluded.latest_chapter_num, status=excluded.status, - kind=excluded.kind, updated_at=CASE WHEN bookmarks.last_chapter_num IS DISTINCT FROM excluded.last_chapter_num THEN excluded.updated_at ELSE bookmarks.updated_at END`, - b.Key, b.Site, b.SeriesID, b.Title, b.SeriesURL, b.Cover, + b.Key, b.Site, b.SeriesID, b.LastChapter, b.LastChapterNum, b.LastChapterURL, - b.Favorite, b.LatestChapter, latestNum, b.UpdatedAt, - b.Status, b.Kind); err != nil { + b.Favorite, b.Status, b.UpdatedAt); err != nil { return Bookmark{}, fmt.Errorf("upsert %q: %w", b.Key, err) } stored, err := scanBookmark(tx.QueryRow( - `SELECT `+bookmarkColumns+` FROM bookmarks WHERE key = $1`, b.Key).Scan) + `SELECT `+bookmarkColumns+` FROM bookmarks b + JOIN series s ON s.site = b.site AND s.series_id = b.series_id + WHERE b.key = $1`, b.Key).Scan) if err != nil { return Bookmark{}, fmt.Errorf("read back %q: %w", b.Key, err) } @@ -360,71 +447,94 @@ func (s *Store) Delete(key string) error { return nil } -// DueForLatestCheck returns bookmarks whose server-side latest-chapter check has -// aged past cutoffMs, least-recently-checked first, at most limit of them. +// DueForLatestCheck returns series whose server-side latest-chapter check has +// aged past cutoffMs, ordered by how many bookmarks reference them (descending) +// then least-recently-checked first, at most limit of them. // -// Oldest-first is what keeps the poller fair when the backlog outgrows its +// The reader_count ordering is the point of the split (ADR-0003): a series +// shared by several readers is fetched once per due cycle, and the popular +// ones stay freshest while the long tail absorbs any shortfall. Within one +// reader count, oldest-first keeps the poll fair when the backlog outgrows its // throughput: the most neglected series is always next, so a large collection // refreshes uniformly slower rather than leaving a tail that never refreshes at // all. The userscript sorts its own queue the same way (L453). // -// Bookmarks with no series_url are skipped — there is nothing to fetch, which -// is the same filter the userscript applies at L452. -// -// Finished series are excluded: nothing more is coming, so fetching them only -// burns requests. Archived ones are deliberately still polled — knowing what a -// shelved series is up to is the whole reason for archiving instead of deleting. -func (s *Store) DueForLatestCheck(cutoffMs int64, limit int) ([]Bookmark, error) { - rows, err := s.db.Query(`SELECT `+bookmarkColumns+` - FROM bookmarks - WHERE series_url <> '' - AND status IS DISTINCT FROM 'finished' - AND latest_checked_at <= $1 - ORDER BY latest_checked_at ASC +// Series with no series_url are skipped — there is nothing to fetch, which is +// the same filter the userscript applies at L452. Series whose only bookmarks +// are finished are skipped too: nothing more is coming, so fetching them only +// burns requests. Archived bookmarks still count — knowing what a shelved +// series is up to is the whole reason for archiving instead of deleting. +// A series with no bookmarks at all never appears: the join excludes it. +func (s *Store) DueForLatestCheck(cutoffMs int64, limit int) ([]Series, error) { + rows, err := s.db.Query(`SELECT `+seriesColumns+`, COUNT(b.key) AS reader_count + FROM series s + JOIN bookmarks b ON b.site = s.site AND b.series_id = s.series_id + WHERE s.series_url <> '' + AND s.latest_checked_at <= $1 + GROUP BY s.site, s.series_id, s.title, s.series_url, s.cover, + s.kind, s.latest_chapter, s.latest_chapter_num, s.latest_checked_at + HAVING COUNT(b.key) FILTER (WHERE b.status <> 'finished') > 0 + ORDER BY reader_count DESC, s.latest_checked_at ASC LIMIT $2`, cutoffMs, limit) if err != nil { - return nil, fmt.Errorf("query due bookmarks: %w", err) + return nil, fmt.Errorf("query due series: %w", err) } defer rows.Close() - out := []Bookmark{} + out := []Series{} for rows.Next() { - b, err := scanBookmark(rows.Scan) + sr, err := scanSeries(rows.Scan) if err != nil { - return nil, fmt.Errorf("scan due bookmark: %w", err) + return nil, fmt.Errorf("scan due series: %w", err) } - out = append(out, b) + out = append(out, sr) } return out, rows.Err() } -// MarkLatestChecked records that the server looked at key at ts, whatever the -// look turned up. Marking a missing key is not an error: the row may have been -// deleted while a fetch was in flight. +// MarkLatestChecked records that the server looked at a series at ts, whatever +// the look turned up. Marking a missing series is not an error: the row may +// have been orphaned while a fetch was in flight. // // This is the one write that does not go through Upsert, and the column is kept -// out of bookmarkColumns on purpose. PUT /bookmarks/{key} decodes a whole -// Bookmark from the client and Upsert writes every column it knows about, so a -// userscript PUT — which has no idea this field exists — would write a zero and -// reset the cooldown, making the poller re-fetch that series every tick for as -// long as the user kept reading it. -func (s *Store) MarkLatestChecked(key string, ts int64) error { +// out of the client-visible read path on purpose. PUT /bookmarks/{key} decodes +// a whole Bookmark from the client and Upsert writes every series column it +// knows about, so a userscript PUT — which has no idea this field exists — +// would write a zero and reset the cooldown, making the poller re-fetch that +// series every tick for as long as the user kept reading it. +func (s *Store) MarkLatestChecked(site, seriesID string, ts int64) error { if _, err := s.db.Exec( - `UPDATE bookmarks SET latest_checked_at = $1 WHERE key = $2`, ts, key); err != nil { - return fmt.Errorf("mark checked %q: %w", key, err) + `UPDATE series SET latest_checked_at = $1 WHERE site = $2 AND series_id = $3`, + ts, site, seriesID); err != nil { + return fmt.Errorf("mark checked %s:%s: %w", site, seriesID, err) } return nil } // LatestCheckedAt reads the column MarkLatestChecked writes. It exists for // tests outside this package (the poller's own tests assert on cooldown -// bookkeeping) — see MarkLatestChecked for why the field itself stays off -// Bookmark. -func (s *Store) LatestCheckedAt(key string) (int64, error) { +// bookkeeping) — see MarkLatestChecked for why the field stays off the +// client-visible row. +func (s *Store) LatestCheckedAt(site, seriesID string) (int64, error) { var ts int64 if err := s.db.QueryRow( - `SELECT latest_checked_at FROM bookmarks WHERE key = $1`, key).Scan(&ts); err != nil { - return 0, fmt.Errorf("latest checked at %q: %w", key, err) + `SELECT latest_checked_at FROM series WHERE site = $1 AND series_id = $2`, + site, seriesID).Scan(&ts); err != nil { + return 0, fmt.Errorf("latest checked at %s:%s: %w", site, seriesID, err) } return ts, nil } + +// SetLatestChapter records the newest chapter the poll found on a series page. +// The poller walks Series rather than Bookmarks, so this is a series-level +// write: the row is shared, and updating it once refreshes every bookmark that +// joins to it. Touching a missing series is not an error. +func (s *Store) SetLatestChapter(site, seriesID, label string, num float64) error { + if _, err := s.db.Exec( + `UPDATE series SET latest_chapter = $3, latest_chapter_num = $4 + WHERE site = $1 AND series_id = $2`, + site, seriesID, label, num); err != nil { + return fmt.Errorf("set latest chapter %s:%s: %w", site, seriesID, err) + } + return nil +} diff --git a/backend/internal/store/store_test.go b/backend/internal/store/store_test.go index c444f02..04f5750 100644 --- a/backend/internal/store/store_test.go +++ b/backend/internal/store/store_test.go @@ -1,7 +1,9 @@ package store import ( + "database/sql" "os" + "strings" "testing" "time" @@ -124,30 +126,41 @@ func TestBookmarkContinueURL(t *testing.T) { } // readLatestCheckedAt reads the column directly. It is deliberately absent from -// Bookmark (see Store.Upsert), so tests cannot assert on it any other way. +// Series (see Store.MarkLatestChecked), so tests cannot assert on it any other +// way. The key splits the same way the API handler derives site/series_id. func readLatestCheckedAt(t *testing.T, s *Store, key string) int64 { t.Helper() + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } var ts int64 if err := s.db.QueryRow( - `SELECT latest_checked_at FROM bookmarks WHERE key = $1`, key).Scan(&ts); err != nil { + `SELECT latest_checked_at FROM series WHERE site = $1 AND series_id = $2`, + site, seriesID).Scan(&ts); err != nil { t.Fatalf("read latest_checked_at %q: %v", key, err) } return ts } -// seedForCheck inserts a bookmark and forces its latest_checked_at. +// seedForCheck inserts a bookmark (and with it its series) and forces the +// series' latest_checked_at. func seedForCheck(t *testing.T, s *Store, key, seriesURL string, checkedAt int64) { t.Helper() + site, seriesID, ok := strings.Cut(key, ":") + if !ok { + t.Fatalf("key %q: no ':' separator", key) + } if _, err := s.Upsert(Bookmark{ Key: key, - Site: "asura", - SeriesID: key, + Site: site, + SeriesID: seriesID, SeriesURL: seriesURL, UpdatedAt: 1000, }); err != nil { t.Fatalf("seed %q: %v", key, err) } - if err := s.MarkLatestChecked(key, checkedAt); err != nil { + if err := s.MarkLatestChecked(site, seriesID, checkedAt); err != nil { t.Fatalf("seed mark %q: %v", key, err) } } @@ -198,8 +211,8 @@ func TestDueForLatestCheckOldestFirstAndLimited(t *testing.T) { if len(due) != 2 { t.Fatalf("got %d rows, want 2 (limit)", len(due)) } - if due[0].Key != "asura:a" || due[1].Key != "asura:b" { - t.Fatalf("got %q,%q; want asura:a,asura:b (oldest first)", due[0].Key, due[1].Key) + if due[0].Key() != "asura:a" || due[1].Key() != "asura:b" { + t.Fatalf("got %q,%q; want asura:a,asura:b (oldest first)", due[0].Key(), due[1].Key()) } } @@ -207,20 +220,21 @@ func TestMarkLatestChecked(t *testing.T) { s := newTestStore(t) seedForCheck(t, s, "asura:x", "https://asurascans.com/comics/x", 0) - if err := s.MarkLatestChecked("asura:x", 4242); err != nil { + if err := s.MarkLatestChecked("asura", "x", 4242); err != nil { t.Fatalf("MarkLatestChecked: %v", err) } if got := readLatestCheckedAt(t, s, "asura:x"); got != 4242 { t.Fatalf("latest_checked_at = %d, want 4242", got) } - // A missing key is not an error: the row may have been deleted mid-fetch. - if err := s.MarkLatestChecked("asura:gone", 1); err != nil { - t.Fatalf("MarkLatestChecked on missing key: %v", err) + // A missing series is not an error: its bookmarks may have been deleted + // mid-fetch. + if err := s.MarkLatestChecked("asura", "gone", 1); err != nil { + t.Fatalf("MarkLatestChecked on missing series: %v", err) } } // Upsert must not touch latest_checked_at. If the column ever migrates into -// bookmarkColumns, this fails and the cooldown is silently dead. +// the client-visible write path, this fails and the cooldown is silently dead. func TestUpsertPreservesLatestCheckedAt(t *testing.T) { s := newTestStore(t) seedForCheck(t, s, "asura:x", "https://asurascans.com/comics/x", 999) @@ -362,7 +376,7 @@ func TestDueForLatestCheckSkipsFinishedKeepsArchived(t *testing.T) { {"asura:finished", StatusFinished}, } { if _, err := store.Upsert(Bookmark{ - Key: tc.key, Site: "asura", SeriesID: tc.key, + Key: tc.key, Site: "asura", SeriesID: strings.TrimPrefix(tc.key, "asura:"), SeriesURL: "https://asurascans.com/comics/" + tc.key, Status: tc.status, UpdatedAt: time.Now().UnixMilli(), }); err != nil { @@ -375,8 +389,8 @@ func TestDueForLatestCheckSkipsFinishedKeepsArchived(t *testing.T) { t.Fatalf("DueForLatestCheck: %v", err) } got := map[string]bool{} - for _, b := range due { - got[b.Key] = true + for _, sr := range due { + got[sr.Key()] = true } if !got["asura:reading"] || !got["asura:archived"] { t.Fatalf("due = %v, want reading and archived present", got) @@ -476,3 +490,264 @@ func TestUpsertEmptyKindKeepsStoredValue(t *testing.T) { t.Fatalf("LastChapterNum = %v, want 11 — progress in the same request must still land", got.LastChapterNum) } } + +// The 0002 backfill must survive a database that already ran 0001 with real +// rows: one series row per distinct (site, series_id) carrying the moved +// columns, and the bookmark keeping the rest. That is the upgrade path for +// every deployed database, so it is exercised rather than trusted. +func TestMigration0002BackfillsExistingBookmarks(t *testing.T) { + url := pgtest.URL(t) + db, err := sql.Open("pgx", url) + if err != nil { + t.Fatalf("open: %v", err) + } + t.Cleanup(func() { db.Close() }) + + // Run only 0001, as a database created before this change would have. + // migrate() normally creates the version table first; do the same here. + if _, err := db.Exec(`CREATE TABLE IF NOT EXISTS schema_migrations ( + version bigint PRIMARY KEY, + applied_at timestamptz NOT NULL DEFAULT now())`); err != nil { + t.Fatalf("create version table: %v", err) + } + body, err := migrations.ReadFile("migrations/0001_bookmarks.sql") + if err != nil { + t.Fatalf("read 0001: %v", err) + } + if err := applyMigration(db, 1, string(body)); err != nil { + t.Fatalf("apply 0001: %v", err) + } + if _, err := db.Exec(` + INSERT INTO bookmarks (key, site, series_id, title, series_url, cover, + last_chapter, last_chapter_num, last_chapter_url, favorite, latest_chapter, + latest_chapter_num, latest_checked_at, status, kind, updated_at) + VALUES ('asura:solo', 'asura', 'solo', 'Solo Leveling', + 'https://asurascans.com/comics/solo', 'https://asurascans.com/covers/solo.jpg', + 'Chapter 10', 10, 'https://asurascans.com/comics/solo/ch/10', true, + 'Chapter 11', 11, 123456, 'reading', 'manga', 1000)`); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + + // Bring it current: 0002 must backfill the series row, not lose data. + if err := migrate(db); err != nil { + t.Fatalf("migrate: %v", err) + } + var ( + title string + checked int64 + fav bool + lastNum float64 + ) + if err := db.QueryRow(`SELECT title, latest_checked_at FROM series + WHERE site = 'asura' AND series_id = 'solo'`).Scan(&title, &checked); err != nil { + t.Fatalf("series row missing after migration: %v", err) + } + if title != "Solo Leveling" || checked != 123456 { + t.Fatalf("series = (%q, %d), want backfilled title and latest_checked_at", title, checked) + } + if err := db.QueryRow(`SELECT favorite, last_chapter_num FROM bookmarks + WHERE key = 'asura:solo'`).Scan(&fav, &lastNum); err != nil { + t.Fatalf("bookmark row missing after migration: %v", err) + } + if !fav || lastNum != 10 { + t.Fatalf("bookmark = (%v, %v), want favorite and progress kept", fav, lastNum) + } + + // The migrated database opens as a normal store. + st, err := Open(url) + if err != nil { + t.Fatalf("Open after migrate: %v", err) + } + defer st.Close() +} + +// readSeries reads the series row directly, for asserting on what Upsert +// actually stored rather than what the joined Bookmark reports. +func readSeries(t *testing.T, s *Store, site, seriesID string) Series { + t.Helper() + sr, err := scanSeries(s.db.QueryRow( + `SELECT `+seriesColumns+`, 0 AS reader_count FROM series s + WHERE s.site = $1 AND s.series_id = $2`, site, seriesID).Scan) + if err != nil { + t.Fatalf("read series %s:%s: %v", site, seriesID, err) + } + return sr +} + +// The first PUT for a series creates its row from the client's title, cover +// and URL — there is no other source for them (ADR-0003). +func TestUpsertCreatesSeriesFromClient(t *testing.T) { + store := newTestStore(t) + if _, err := store.Upsert(Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo", + Cover: "https://asurascans.com/covers/solo.jpg", Kind: KindManga, + UpdatedAt: 1000, + }); err != nil { + t.Fatalf("Upsert: %v", err) + } + + sr := readSeries(t, store, "asura", "solo") + if sr.Title != "Solo Leveling" || sr.SeriesURL != "https://asurascans.com/comics/solo" || + sr.Cover != "https://asurascans.com/covers/solo.jpg" { + t.Fatalf("series = %+v, want client title/url/cover stored", sr) + } +} + +// A PUT naming an existing series must not overwrite its title, cover or URL: +// the row is shared, and those values are scraped page content (ADR-0003). +func TestUpsertExistingSeriesIgnoresClientTitleCoverURL(t *testing.T) { + store := newTestStore(t) + base := Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + Title: "Solo Leveling", SeriesURL: "https://asurascans.com/comics/solo", + Cover: "https://asurascans.com/covers/solo.jpg", LastChapterNum: 10, + UpdatedAt: 1000, + } + if _, err := store.Upsert(base); err != nil { + t.Fatalf("seed: %v", err) + } + + // Same series, hostile/compromised values, real progress advance. + base.Title = "Scraped Rename" + base.SeriesURL = "https://evil.example/solo" + base.Cover = "https://evil.example/solo.jpg" + base.LastChapterNum = 11 + got, err := store.Upsert(base) + if err != nil { + t.Fatalf("Upsert: %v", err) + } + if got.Title != "Solo Leveling" || got.SeriesURL != "https://asurascans.com/comics/solo" || + got.Cover != "https://asurascans.com/covers/solo.jpg" { + t.Fatalf("stored = %+v, want original title/url/cover kept", got) + } + if got.LastChapterNum != 11 { + t.Fatalf("LastChapterNum = %v, want 11 — progress in the same request must still land", got.LastChapterNum) + } +} + +// Kind and latest-chapter are last-write-wins even on an existing series: the +// poller and the userscript both report the latest chapter, and kind is only +// known to whichever client created the row. +func TestUpsertExistingSeriesAcceptsKindAndLatest(t *testing.T) { + store := newTestStore(t) + base := Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", Kind: KindManga, + UpdatedAt: 1000, + } + if _, err := store.Upsert(base); err != nil { + t.Fatalf("seed: %v", err) + } + + num := 12.0 + base.Kind = KindNovel + base.LatestChapter = "Chapter 12" + base.LatestChapterNum = &num + got, err := store.Upsert(base) + if err != nil { + t.Fatalf("Upsert: %v", err) + } + if got.Kind != KindNovel || got.LatestChapter != "Chapter 12" || + got.LatestChapterNum == nil || *got.LatestChapterNum != 12 { + t.Fatalf("stored = %+v, want kind and latest chapter updated", got) + } +} + +// Deleting the last bookmark must leave the series row behind, so a later +// re-bookmark shows title and cover immediately instead of waiting for a poll. +func TestDeleteKeepsSeriesRow(t *testing.T) { + store := newTestStore(t) + if _, err := store.Upsert(Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + Title: "Solo Leveling", Cover: "https://asurascans.com/covers/solo.jpg", + UpdatedAt: 1000, + }); err != nil { + t.Fatalf("seed: %v", err) + } + if err := store.Delete("asura:solo"); err != nil { + t.Fatalf("Delete: %v", err) + } + + sr := readSeries(t, store, "asura", "solo") + if sr.Title != "Solo Leveling" { + t.Fatalf("series = %+v, want it kept after the last bookmark is deleted", sr) + } + + // Re-bookmark with nothing but progress: the stored title/cover come back. + stored, err := store.Upsert(Bookmark{ + Key: "asura:solo", Site: "asura", SeriesID: "solo", + LastChapterNum: 5, UpdatedAt: 2000, + }) + if err != nil { + t.Fatalf("re-upsert: %v", err) + } + if stored.Title != "Solo Leveling" || stored.Cover != "https://asurascans.com/covers/solo.jpg" { + t.Fatalf("re-bookmark = %+v, want title/cover from the surviving series row", stored) + } +} + +// seedSecondReader inserts an extra bookmark on an existing series. Today the +// bookmark key is :, so two bookmarks can share a series only +// once keys stop being derived from the series identity (issue #22); the due +// queue's reader-count ordering must already be right for that world. +func seedSecondReader(t *testing.T, s *Store, key, site, seriesID string, updatedAt int64) { + t.Helper() + if _, err := s.Upsert(Bookmark{ + Key: key, Site: site, SeriesID: seriesID, UpdatedAt: updatedAt, + }); err != nil { + t.Fatalf("seed second reader %q: %v", key, err) + } +} + +// The whole point of the split: a shared series is due once, ordered ahead of +// single-reader series by how many bookmarks reference it. +func TestDueForLatestCheckOrdersByReaderCountThenAge(t *testing.T) { + s := newTestStore(t) + // "pop" has two bookmarks but was checked most recently; "solo" has one and + // was checked long ago. Reader count must win over age. + seedForCheck(t, s, "asura:pop", "https://asurascans.com/comics/pop", 900) + seedSecondReader(t, s, "asura:pop:2", "asura", "pop", 1001) + seedForCheck(t, s, "asura:solo", "https://asurascans.com/comics/solo", 100) + + due, err := s.DueForLatestCheck(1000, 10) + if err != nil { + t.Fatalf("DueForLatestCheck: %v", err) + } + if len(due) != 2 { + t.Fatalf("due = %d rows, want 2", len(due)) + } + if due[0].Key() != "asura:pop" || due[1].Key() != "asura:solo" { + t.Fatalf("due order = %q, %q; want asura:pop (2 readers), asura:solo (1)", + due[0].Key(), due[1].Key()) + } +} + +// A series with no bookmarks must never appear in the due queue, and nothing +// in the store ever deletes it (see TestDeleteKeepsSeriesRow). +func TestDueForLatestCheckExcludesOrphanSeries(t *testing.T) { + s := newTestStore(t) + seedForCheck(t, s, "asura:kept", "https://asurascans.com/comics/kept", 0) + if _, err := s.db.Exec(` + INSERT INTO series (site, series_id, title, series_url, cover, kind, + latest_chapter, latest_chapter_num, latest_checked_at) + VALUES ('asura', 'orphan', 'Orphan', 'https://asurascans.com/comics/orphan', + '', 'manga', '', NULL, 0)`); err != nil { + t.Fatalf("seed orphan series: %v", err) + } + + due, err := s.DueForLatestCheck(1000, 10) + if err != nil { + t.Fatalf("DueForLatestCheck: %v", err) + } + if len(due) != 1 || due[0].Key() != "asura:kept" { + t.Fatalf("due = %+v, want only the bookmarked series", due) + } + var n int + if err := s.db.QueryRow( + `SELECT count(*) FROM series WHERE site = 'asura' AND series_id = 'orphan'`).Scan(&n); err != nil { + t.Fatalf("count orphan series: %v", err) + } + if n != 1 { + t.Fatalf("orphan series count = %d, want 1 (never deleted)", n) + } +}