Split Series from Bookmark, keeping the wire format flat (#21) (#29)

Co-authored-by: Sulthan Zaki <sultankiki05@gmail.com> Co-committed-by: Sulthan Zaki <sultankiki05@gmail.com>
2026-08-08 07:19:54 +07:00
parent 08749df050
commit 984965ed9f
8 changed files with 756 additions and 158 deletions
@@ -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 <site>:<series_id>, 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)
+	}
+}