From 01301805fbcfd2f65aaf690d5c5f98d6dbe0580e Mon Sep 17 00:00:00 2001 From: Sulthan Zaki Date: Sat, 25 Jul 2026 12:06:44 +0700 Subject: [PATCH] feat(backend): favorite + latest-chapter fields, conditional updated_at Adds favorite, latest_chapter and latest_chapter_num to the bookmark record, with an idempotent ALTER TABLE migration so the already-deployed database picks them up. updated_at now moves only when a bookmark is new or last_chapter_num changes. Clients order their list by updated_at, so favoriting a series or recording a newly published chapter must not disturb that order. Upsert consequently returns the row as stored and the handler echoes that rather than the request payload, since the candidate timestamp it sends is often discarded. Scanning also tolerates NULL in the optional columns, which a database created before this code can legitimately contain. Co-Authored-By: Claude Opus 5 --- backend/handlers.go | 11 +- backend/store.go | 208 ++++++++++++++++++++++++++++------- backend/store_test.go | 246 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 422 insertions(+), 43 deletions(-) diff --git a/backend/handlers.go b/backend/handlers.go index 84f7e32..c427bf1 100644 --- a/backend/handlers.go +++ b/backend/handlers.go @@ -60,14 +60,19 @@ func (h *bookmarkHandler) put(w http.ResponseWriter, r *http.Request) { } } } - b.UpdatedAt = time.Now().UnixMilli() // server-assigned, ignore client value + // Candidate timestamp, not a decision: Upsert keeps the stored one unless + // reading progress actually moved. Any client value is ignored. + b.UpdatedAt = time.Now().UnixMilli() - if err := h.store.Upsert(b); err != nil { + stored, err := h.store.Upsert(b) + if err != nil { log.Printf("upsert: %v", err) http.Error(w, "internal error", http.StatusInternalServerError) return } - writeJSON(w, http.StatusOK, b) + // Echo the stored row: clients adopt this as their cached copy, so it must + // carry the authoritative updated_at rather than the candidate above. + writeJSON(w, http.StatusOK, stored) } // delete removes one bookmark. DELETE /bookmarks/{key} diff --git a/backend/store.go b/backend/store.go index 161c4e3..dc8a09a 100644 --- a/backend/store.go +++ b/backend/store.go @@ -9,33 +9,54 @@ import ( ) // Bookmark is one tracked series, keyed ":" across both sites. +// +// LastChapter* is the user's read progress; LatestChapter* is the newest +// chapter the site has published, captured opportunistically by the userscript. type Bookmark struct { - Key string `json:"key"` - Site string `json:"site"` - SeriesID string `json:"series_id"` - Title string `json:"title"` - SeriesURL string `json:"series_url"` - Cover string `json:"cover"` - LastChapter string `json:"last_chapter"` - LastChapterNum float64 `json:"last_chapter_num"` - LastChapterURL string `json:"last_chapter_url"` - UpdatedAt int64 `json:"updated_at"` // unix ms, server-assigned + Key string `json:"key"` + Site string `json:"site"` + SeriesID string `json:"series_id"` + Title string `json:"title"` + SeriesURL string `json:"series_url"` + Cover string `json:"cover"` + LastChapter string `json:"last_chapter"` + LastChapterNum float64 `json:"last_chapter_num"` + LastChapterURL string `json:"last_chapter_url"` + Favorite bool `json:"favorite"` + LatestChapter string `json:"latest_chapter"` + LatestChapterNum *float64 `json:"latest_chapter_num"` // nil until first captured + UpdatedAt int64 `json:"updated_at"` // unix ms; see Upsert } const schema = ` CREATE TABLE IF NOT EXISTS bookmarks ( - key TEXT PRIMARY KEY, - site TEXT NOT NULL, - series_id TEXT NOT NULL, - title TEXT, - series_url TEXT, - cover TEXT, - last_chapter TEXT, - last_chapter_num REAL, - last_chapter_url TEXT, - updated_at INTEGER NOT NULL + key TEXT PRIMARY KEY, + site TEXT NOT NULL, + series_id TEXT NOT NULL, + title TEXT, + series_url TEXT, + cover TEXT, + last_chapter TEXT, + last_chapter_num REAL, + last_chapter_url TEXT, + favorite INTEGER NOT NULL DEFAULT 0, + latest_chapter TEXT NOT NULL DEFAULT '', + latest_chapter_num REAL, + updated_at INTEGER NOT NULL );` +// The columns above that databases created before them will be missing. +// SQLite has no ADD COLUMN IF NOT EXISTS, so each is added only when absent. +var addedColumns = []struct{ name, ddl string }{ + {"favorite", `ALTER TABLE bookmarks ADD COLUMN favorite INTEGER NOT NULL DEFAULT 0`}, + {"latest_chapter", `ALTER TABLE bookmarks ADD COLUMN latest_chapter TEXT NOT NULL DEFAULT ''`}, + {"latest_chapter_num", `ALTER TABLE bookmarks ADD COLUMN latest_chapter_num REAL`}, +} + +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` + // Store is the SQLite-backed bookmark store. type Store struct { db *sql.DB @@ -58,17 +79,91 @@ func OpenStore(path string) (*Store, error) { db.Close() return nil, fmt.Errorf("apply schema: %w", err) } + if err := migrateColumns(db); err != nil { + db.Close() + return nil, fmt.Errorf("migrate schema: %w", err) + } return &Store{db: db}, nil } +// migrateColumns brings a pre-existing bookmarks table up to the current +// schema. Safe to run on every start: columns already present are skipped. +func migrateColumns(db *sql.DB) error { + have, err := existingColumns(db, "bookmarks") + if err != nil { + return err + } + for _, c := range addedColumns { + if _, ok := have[c.name]; ok { + continue + } + if _, err := db.Exec(c.ddl); err != nil { + return fmt.Errorf("add column %q: %w", c.name, err) + } + } + return nil +} + +func existingColumns(db *sql.DB, table string) (map[string]struct{}, error) { + rows, err := db.Query(`SELECT name FROM pragma_table_info(?)`, table) + if err != nil { + return nil, fmt.Errorf("read %s columns: %w", table, err) + } + defer rows.Close() + + out := map[string]struct{}{} + for rows.Next() { + var name string + if err := rows.Scan(&name); err != nil { + return nil, fmt.Errorf("scan column name: %w", err) + } + out[name] = struct{}{} + } + return out, rows.Err() +} + +// scanBookmark reads one row in bookmarkColumns order, translating SQLite's +// integer bool and nullable latest_chapter_num into Go types. +// +// The optional columns are read through Null* types because rows predating +// this code (or written by hand) may hold NULL where the app only ever writes +// zero values. Only latest_chapter_num distinguishes the two: everywhere else +// NULL and the zero value mean the same thing to clients. +func scanBookmark(scan func(...any) error) (Bookmark, error) { + var ( + b Bookmark + title, seriesURL, cover sql.NullString + lastChapter, lastChapterURL, latestChapter sql.NullString + lastChapterNum, latestChapterNum sql.NullFloat64 + favorite sql.NullInt64 + ) + if err := scan( + &b.Key, &b.Site, &b.SeriesID, &title, &seriesURL, &cover, + &lastChapter, &lastChapterNum, &lastChapterURL, + &favorite, &latestChapter, &latestChapterNum, &b.UpdatedAt, + ); err != nil { + return Bookmark{}, err + } + b.Title = title.String + b.SeriesURL = seriesURL.String + b.Cover = cover.String + b.LastChapter = lastChapter.String + b.LastChapterNum = lastChapterNum.Float64 + b.LastChapterURL = lastChapterURL.String + b.Favorite = favorite.Int64 != 0 + b.LatestChapter = latestChapter.String + if latestChapterNum.Valid { + b.LatestChapterNum = &latestChapterNum.Float64 + } + return b, nil +} + // Close releases the underlying database handle. func (s *Store) Close() error { return s.db.Close() } // List returns every bookmark, newest activity first. func (s *Store) List() ([]Bookmark, error) { - rows, err := s.db.Query(` - SELECT key, site, series_id, title, series_url, cover, - last_chapter, last_chapter_num, last_chapter_url, updated_at + rows, err := s.db.Query(`SELECT ` + bookmarkColumns + ` FROM bookmarks ORDER BY updated_at DESC`) if err != nil { @@ -78,11 +173,8 @@ func (s *Store) List() ([]Bookmark, error) { out := []Bookmark{} for rows.Next() { - var b Bookmark - if err := rows.Scan( - &b.Key, &b.Site, &b.SeriesID, &b.Title, &b.SeriesURL, &b.Cover, - &b.LastChapter, &b.LastChapterNum, &b.LastChapterURL, &b.UpdatedAt, - ); err != nil { + b, err := scanBookmark(rows.Scan) + if err != nil { return nil, fmt.Errorf("scan bookmark: %w", err) } out = append(out, b) @@ -90,24 +182,60 @@ func (s *Store) List() ([]Bookmark, error) { return out, rows.Err() } -// Upsert inserts or replaces a bookmark by key (last-write-wins). -func (s *Store) Upsert(b Bookmark) error { - _, err := s.db.Exec(` - INSERT INTO bookmarks - (key, site, series_id, title, series_url, cover, - last_chapter, last_chapter_num, last_chapter_url, updated_at) - VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) +// Upsert inserts or replaces a bookmark by key (last-write-wins) and returns +// the row as actually stored. +// +// 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 +// order their list by updated_at, so favoriting a series or recording a newly +// published chapter must not disturb that order — only real reading progress +// does. Callers must therefore use the returned bookmark, not the argument. +func (s *Store) Upsert(b Bookmark) (Bookmark, error) { + tx, err := s.db.Begin() + if err != nil { + return Bookmark{}, fmt.Errorf("begin %q: %w", b.Key, err) + } + defer tx.Rollback() + + var latestNum any + if b.LatestChapterNum != nil { + latestNum = *b.LatestChapterNum + } + + // IS NOT is SQLite's null-safe comparison. 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 (`+bookmarkColumns+`) + VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) 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, last_chapter=excluded.last_chapter, last_chapter_num=excluded.last_chapter_num, - last_chapter_url=excluded.last_chapter_url, updated_at=excluded.updated_at`, + last_chapter_url=excluded.last_chapter_url, + favorite=excluded.favorite, + latest_chapter=excluded.latest_chapter, + latest_chapter_num=excluded.latest_chapter_num, + updated_at=CASE + WHEN bookmarks.last_chapter_num IS NOT 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.LastChapter, b.LastChapterNum, b.LastChapterURL, b.UpdatedAt) - if err != nil { - return fmt.Errorf("upsert %q: %w", b.Key, err) + b.LastChapter, b.LastChapterNum, b.LastChapterURL, + b.Favorite, b.LatestChapter, latestNum, b.UpdatedAt); err != nil { + return Bookmark{}, fmt.Errorf("upsert %q: %w", b.Key, err) } - return nil + + stored, err := scanBookmark(tx.QueryRow( + `SELECT `+bookmarkColumns+` FROM bookmarks WHERE key = ?`, b.Key).Scan) + if err != nil { + return Bookmark{}, fmt.Errorf("read back %q: %w", b.Key, err) + } + if err := tx.Commit(); err != nil { + return Bookmark{}, fmt.Errorf("commit %q: %w", b.Key, err) + } + return stored, nil } // Delete removes a bookmark by key. Deleting a missing key is not an error. diff --git a/backend/store_test.go b/backend/store_test.go index 3c18906..8c8b081 100644 --- a/backend/store_test.go +++ b/backend/store_test.go @@ -2,11 +2,15 @@ package main import ( "bytes" + "database/sql" "encoding/json" + "fmt" "net/http" "net/http/httptest" "path/filepath" + "strings" "testing" + "time" ) const testToken = "s3cret-token" @@ -187,3 +191,245 @@ func TestBookmarkRoundTrip(t *testing.T) { t.Fatalf("after delete list = %+v, want empty", list) } } + +// 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 Bookmark) Bookmark { + t.Helper() + body, _ := json.Marshal(b) + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, auth(httptest.NewRequest(http.MethodPut, "/bookmarks/"+key, bytes.NewReader(body)))) + if rr.Code != http.StatusOK { + t.Fatalf("PUT %s status = %d, body = %s", key, rr.Code, rr.Body.String()) + } + var out Bookmark + if err := json.Unmarshal(rr.Body.Bytes(), &out); err != nil { + t.Fatalf("decode PUT response: %v", err) + } + return out +} + +func getBookmarks(t *testing.T, srv http.Handler) []Bookmark { + t.Helper() + rr := httptest.NewRecorder() + srv.ServeHTTP(rr, auth(httptest.NewRequest(http.MethodGet, "/bookmarks", nil))) + if rr.Code != http.StatusOK { + t.Fatalf("GET status = %d", rr.Code) + } + var list []Bookmark + if err := json.Unmarshal(rr.Body.Bytes(), &list); err != nil { + t.Fatalf("decode list: %v", err) + } + return list +} + +func floatPtr(f float64) *float64 { return &f } + +// updated_at drives list ordering, so it must move only on a real progress +// advance — never on a favorite toggle or a latest-chapter capture. +func TestUpsertConditionalUpdatedAt(t *testing.T) { + cases := []struct { + name string + mutate func(Bookmark) Bookmark + wantBumped bool + }{ + { + name: "unchanged progress", + mutate: func(b Bookmark) Bookmark { return b }, + wantBumped: false, + }, + { + name: "changed progress", + mutate: func(b Bookmark) Bookmark { + b.LastChapter, b.LastChapterNum = "Chapter 11", 11 + return b + }, + wantBumped: true, + }, + { + name: "favorite only", + mutate: func(b Bookmark) Bookmark { + b.Favorite = true + return b + }, + wantBumped: false, + }, + { + name: "latest chapter only", + mutate: func(b Bookmark) Bookmark { + b.LatestChapter, b.LatestChapterNum = "Chapter 15", floatPtr(15) + return b + }, + wantBumped: false, + }, + { + name: "unrelated metadata only", + mutate: func(b Bookmark) Bookmark { + b.Title, b.Cover = "Renamed", "https://example.test/new.jpg" + return b + }, + wantBumped: false, + }, + } + + for i, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + srv := newTestServer(t) + key := fmt.Sprintf("asura:cond-%d", i) + + first := putBookmark(t, srv, key, Bookmark{ + Title: "Test", + LastChapter: "Chapter 10", + LastChapterNum: 10, + }) + if first.UpdatedAt == 0 { + t.Fatal("new bookmark did not get updated_at set") + } + + // Guarantee a later wall-clock ms so a real bump is observable. + time.Sleep(2 * time.Millisecond) + + second := putBookmark(t, srv, key, tc.mutate(first)) + if tc.wantBumped && second.UpdatedAt <= first.UpdatedAt { + t.Fatalf("updated_at = %d, want > %d", second.UpdatedAt, first.UpdatedAt) + } + if !tc.wantBumped && second.UpdatedAt != first.UpdatedAt { + t.Fatalf("updated_at = %d, want preserved %d", second.UpdatedAt, first.UpdatedAt) + } + + // The PUT response must match what a subsequent GET reports. + list := getBookmarks(t, srv) + if len(list) != 1 { + t.Fatalf("list = %+v, want 1 item", list) + } + if list[0].UpdatedAt != second.UpdatedAt { + t.Fatalf("GET updated_at = %d, PUT echoed %d", list[0].UpdatedAt, second.UpdatedAt) + } + }) + } +} + +func TestFavoriteRoundTrip(t *testing.T) { + srv := newTestServer(t) + key := "demonic:some-series" + + stored := putBookmark(t, srv, key, Bookmark{Title: "Fav", Favorite: true}) + if !stored.Favorite { + t.Fatalf("PUT response favorite = false, want true") + } + + list := getBookmarks(t, srv) + if len(list) != 1 || !list[0].Favorite { + t.Fatalf("favorite did not round-trip: %+v", list) + } + + // Unfavoriting must persist too (guards against a write that only ever ORs in true). + stored = putBookmark(t, srv, key, Bookmark{Title: "Fav", Favorite: false}) + if stored.Favorite { + t.Fatal("PUT response favorite = true after unfavorite") + } + list = getBookmarks(t, srv) + if len(list) != 1 || list[0].Favorite { + t.Fatalf("unfavorite did not round-trip: %+v", list) + } +} + +func TestLatestChapterNullable(t *testing.T) { + srv := newTestServer(t) + key := "asura:latest-test" + + // Never captured: latest_chapter_num must serialize as JSON null. + body, _ := json.Marshal(Bookmark{Title: "No latest yet"}) + 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", rr.Code) + } + if !strings.Contains(rr.Body.String(), `"latest_chapter_num":null`) { + t.Fatalf("want latest_chapter_num null in response, got %s", rr.Body.String()) + } + + list := getBookmarks(t, srv) + if len(list) != 1 || list[0].LatestChapterNum != nil { + t.Fatalf("latest_chapter_num = %v, want nil", list[0].LatestChapterNum) + } + + // Once captured it round-trips as a value. + stored := putBookmark(t, srv, key, Bookmark{ + Title: "No latest yet", + LatestChapter: "Chapter 162", + LatestChapterNum: floatPtr(162), + }) + if stored.LatestChapterNum == nil || *stored.LatestChapterNum != 162 { + t.Fatalf("PUT response latest_chapter_num = %v, want 162", stored.LatestChapterNum) + } + list = getBookmarks(t, srv) + if len(list) != 1 || list[0].LatestChapterNum == nil || *list[0].LatestChapterNum != 162 { + t.Fatalf("latest chapter did not round-trip: %+v", list) + } + if list[0].LatestChapter != "Chapter 162" { + t.Fatalf("latest_chapter = %q, want %q", list[0].LatestChapter, "Chapter 162") + } +} + +// The deployed database predates favorite/latest_chapter*, and CREATE TABLE +// IF NOT EXISTS will not add them — OpenStore must migrate in place. +func TestOpenStoreMigratesLegacySchema(t *testing.T) { + dbPath := filepath.Join(t.TempDir(), "legacy.db") + + legacy, err := sql.Open("sqlite", dbPath) + if err != nil { + t.Fatalf("open legacy db: %v", err) + } + if _, err := legacy.Exec(` + CREATE TABLE bookmarks ( + key TEXT PRIMARY KEY, + site TEXT NOT NULL, + series_id TEXT NOT NULL, + title TEXT, + series_url TEXT, + cover TEXT, + last_chapter TEXT, + last_chapter_num REAL, + last_chapter_url TEXT, + updated_at INTEGER NOT NULL + )`); err != nil { + t.Fatalf("create legacy schema: %v", err) + } + if _, err := legacy.Exec(` + INSERT INTO bookmarks (key, site, series_id, title, last_chapter, last_chapter_num, updated_at) + VALUES ('asura:legacy', 'asura', 'legacy', 'Legacy Series', 'Chapter 7', 7, 123)`); err != nil { + t.Fatalf("seed legacy row: %v", err) + } + if err := legacy.Close(); err != nil { + t.Fatalf("close legacy db: %v", err) + } + + store, err := OpenStore(dbPath) + if err != nil { + t.Fatalf("OpenStore on legacy db: %v", err) + } + t.Cleanup(func() { store.Close() }) + + list, err := store.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(list) != 1 || list[0].Key != "asura:legacy" { + t.Fatalf("legacy row lost: %+v", list) + } + got := list[0] + if got.Title != "Legacy Series" || got.LastChapterNum != 7 || got.UpdatedAt != 123 { + t.Fatalf("legacy data mangled: %+v", got) + } + if got.Favorite || got.LatestChapter != "" || got.LatestChapterNum != nil { + t.Fatalf("new columns should default empty, got %+v", got) + } + + // Reopening an already-migrated database must be a no-op, not an error. + store2, err := OpenStore(dbPath) + if err != nil { + t.Fatalf("OpenStore is not idempotent: %v", err) + } + store2.Close() +}