From 8c754e05dec8b8889ad40529bf16dbde553b0ac7 Mon Sep 17 00:00:00 2001 From: gchahcg <7381837@proton.me> Date: Fri, 16 Oct 2026 22:41:39 -0400 Subject: [PATCH 1/2] fix(series): cover split edition parts listed in another series by the same author (#3324) Hardcover can list a split part only in another numbering of the series, where every part is its own volume at an integer position: "The Eye of the World, Part 1 of 2" at 1 in The Wheel of Time (Finnish Numbering), while the whole sits at 1 in The Wheel of Time. ListCoveredSplitEditionParts only paired rows on one series_id, and the fractional-position signal can't hold there anyway, so the sweep kept searching the part. The query gains a second branch: a part and an eligible whole by the same author, where the whole is in a series and the part's own series holds no book with the whole's title, are matched by the title signal alone (seriesmatch.SplitEditionTitleOf, the title half of SplitEditionPartOf). Positions from two series aren't comparable; inside a shared series the two-signal rule still decides alone. The sweep, the series page, the series unmonitor button and fill's re-queue all read this query. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WA2CUNg5o4wUBaX6yWXNNk Signed-off-by: gchahcg <7381837@proton.me> --- changelog.d/3324-split-parts-across-series.md | 2 + docs/Hardcover-Series-Wiki.md | 2 +- internal/db/split_edition_parts.go | 92 +++++-- internal/db/split_edition_parts_test.go | 243 +++++++++++++++++- internal/seriesmatch/split_edition.go | 10 + internal/seriesmatch/split_edition_test.go | 21 ++ 6 files changed, 342 insertions(+), 28 deletions(-) create mode 100644 changelog.d/3324-split-parts-across-series.md diff --git a/changelog.d/3324-split-parts-across-series.md b/changelog.d/3324-split-parts-across-series.md new file mode 100644 index 000000000..762cd24c4 --- /dev/null +++ b/changelog.d/3324-split-parts-across-series.md @@ -0,0 +1,2 @@ +### Fixed +- **Split edition parts listed in another series** (#3324) — a part Hardcover lists only in another numbering of the series, such as "The Eye of the World, Part 1 of 2" at 1 in "The Wheel of Time (Finnish Numbering)", now counts as covered by the whole you already own: the wanted search skips it, the series page marks it as a split part, and **Unmonitor N split parts** includes it. diff --git a/docs/Hardcover-Series-Wiki.md b/docs/Hardcover-Series-Wiki.md index 6bf60ce03..dda994467 100644 --- a/docs/Hardcover-Series-Wiki.md +++ b/docs/Hardcover-Series-Wiki.md @@ -93,7 +93,7 @@ The fill action may create new authors and books from Hardcover metadata when th When fill re-queues the books already in the series, it also leaves out any that are unmonitored and not imported and that the profile's skip part books or skip missing release date filter rejects, so a fill no longer puts those back on Wanted. A book that is already monitored, for example one you added by hand, is queued as before. A book with no known language is not rejected by the language filter during a fill, whatever the profile does with unknown languages, because Hardcover series entries carry no language; it is resolved later. The other filters need a fresh provider lookup; use **More → Reconcile catalogue…** on the author to apply every filter to books already stored. -Some catalogs list a novel's own split edition beside it, such as "The Way of Kings, Part 1" and "Part 2" at 1.1 and 1.2 under "The Way of Kings" at 1. A book counts as such a split part when both hold: its position is a fraction under the whole's position, and its title is the whole's title followed by "Part N" (a subtitle after the marker is ignored). Position alone is not enough: a real novella at a fractional position, such as "Edgedancer" at 2.5 beside "Words of Radiance" at 2, has no "Part N" marker and stays a normal book, and a catalog that represents a work only as its split parts, with no unsplit row at the integer position at all (Robert Jordan's Wheel of Time "Two Volume Edition" catalog is like this), never triggers the rule either, since there is nothing to cover the parts. On the Hardcover catalog diff a covered part is reported as covered, not missing: it is not counted, not offered an **add** button, and not created by fill, and appears in its own collapsed section on the Series page ("N split editions of books you own") linking to the whole that covers it. The same rule covers rows already stored locally: once the whole is imported, or monitored and not excluded, fill no longer re-queues a stored part and the scheduled wanted search no longer searches it, so a part of a book you already have is never downloaded. Stored parts are marked **Split part** on the series card and are not counted as missing. If a fill from an older version left one monitored, **Unmonitor N split parts** on the series card unmonitors them in one click. Nothing is deleted, and an imported part is never touched. +Some catalogs list a novel's own split edition beside it, such as "The Way of Kings, Part 1" and "Part 2" at 1.1 and 1.2 under "The Way of Kings" at 1. A book counts as such a split part when both hold: its position is a fraction under the whole's position, and its title is the whole's title followed by "Part N" (a subtitle after the marker is ignored). Position alone is not enough: a real novella at a fractional position, such as "Edgedancer" at 2.5 beside "Words of Radiance" at 2, has no "Part N" marker and stays a normal book, and a catalog that represents a work only as its split parts, with no unsplit row at the integer position at all (Robert Jordan's Wheel of Time "Two Volume Edition" catalog is like this), never triggers the rule either, since there is nothing to cover the parts. On the Hardcover catalog diff a covered part is reported as covered, not missing: it is not counted, not offered an **add** button, and not created by fill, and appears in its own collapsed section on the Series page ("N split editions of books you own") linking to the whole that covers it. The same rule covers rows already stored locally: once the whole is imported, or monitored and not excluded, fill no longer re-queues a stored part and the scheduled wanted search no longer searches it, so a part of a book you already have is never downloaded. A stored part listed only in another series by the same author, such as a numbering where every part is its own volume ("The Eye of the World, Part 1 of 2" at 1 in "The Wheel of Time (Finnish Numbering)"), is matched by its title alone, because positions from two different series cannot be compared; a part that shares a series with the whole still needs both signals. Stored parts are marked **Split part** on the series card and are not counted as missing. If a fill from an older version left one monitored, **Unmonitor N split parts** on the series card unmonitors them in one click. Nothing is deleted, and an imported part is never touched. The format dropdown beside **add all** sets the media type of every book the fill creates. Pick **Ebook** and the created books are ebook only, even when Hardcover lists an audiobook edition of the same work, so only one search is queued per book. Pick **Audiobook** for audio only. Pick **Both** if you want Bindery to look for both formats. Books that are already in the series keep whatever media type they were added with, so change those on the book itself. diff --git a/internal/db/split_edition_parts.go b/internal/db/split_edition_parts.go index a4c97941b..223b116cc 100644 --- a/internal/db/split_edition_parts.go +++ b/internal/db/split_edition_parts.go @@ -11,8 +11,9 @@ import ( ) // SplitEditionPart is a local book that is one part of a split edition of -// another book in the same series (#3048): "The Way of Kings, Part 1" at 1.1 -// while "The Way of Kings" itself sits at 1. +// another book (#3048): "The Way of Kings, Part 1" at 1.1 while "The Way of +// Kings" itself sits at 1. SeriesID is the part's series; the whole may sit in +// another series by the same author (#3324). type SplitEditionPart struct { SeriesID int64 BookID int64 @@ -26,39 +27,80 @@ type SplitEditionPart struct { // of a whole book the library already owns or is already looking for. A // seriesID of 0 covers every series. // -// The rule is seriesmatch.SplitEditionPartOf, the same two signals the series -// diff uses for catalogue rows (#2524): a fractional position under the whole's -// integer position, and the whole's title plus a "Part N" marker. Here it is -// applied to rows already stored, which is what an install that pressed Fill -// before #2524 has. +// A part and a whole that share a series are matched by +// seriesmatch.SplitEditionPartOf, the same two signals the series diff uses +// for catalogue rows (#2524): a fractional position under the whole's integer +// position, and the whole's title plus a "Part N" marker. Here it is applied +// to rows already stored, which is what an install that pressed Fill before +// #2524 has. +// +// A part and a whole that share no series, but have the same author, are +// matched by the title signal alone, seriesmatch.SplitEditionTitleOf (#3324). +// Hardcover can list a part only in another numbering of the series, where +// each part is its own volume at an integer position ("The Eye of the World, +// Part 1 of 2" at 1 in The Wheel of Time (Finnish Numbering)). Positions from +// two series are not comparable, so signal 1 cannot be evaluated there. The +// whole must itself be in a series, and the part's own series must not hold +// any book with the whole's title: there only the two signal rule decides, so +// a part numbered as its own volume inside the whole's series is left alone +// even when the library has a second copy of the whole elsewhere. A part +// matched both ways reports the same-series whole. // // Only a part that is not imported is reported: an imported part is already on // disk and there is nothing to stop searching for. The whole has to be a book // the user actually has or wants, so it must be imported or monitored, and not -// excluded — the WHERE clause below and api.eligibleSplitEditionWhole -// (internal/api/series.go) apply the identical rule to the catalogue diff; -// an eligibility change here needs the same change made there. Both rows +// excluded — the whole gate below and api.eligibleSplitEditionWhole +// (internal/api/series.go) apply the identical eligibility rule to the +// catalogue diff; a change to it here needs the same change made there. The +// catalogue diff only pairs within one series and has no counterpart to the +// cross-series match. Both rows // must belong to the same owner, so one user's library never decides what // another user's sweep searches for. func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID int64) ([]SplitEditionPart, error) { + seriesFilter := "" + if seriesID > 0 { + seriesFilter = ` AND sp.series_id = ?` + } + const wholeGate = ` + AND bw.excluded = 0 + AND (bw.status = ? OR bw.monitored = 1) + AND bw.owner_user_id IS bp.owner_user_id` query := ` - SELECT sp.series_id, sp.book_id, sp.position_in_series, bp.title, bp.monitored, - sw.book_id, sw.position_in_series, bw.title + SELECT part_series, part_id, part_pos, part_title, part_monitored, + whole_id, whole_pos, whole_title, same_series + FROM ( + SELECT sp.series_id AS part_series, sp.book_id AS part_id, sp.position_in_series AS part_pos, + bp.title AS part_title, bp.monitored AS part_monitored, + sw.book_id AS whole_id, sw.position_in_series AS whole_pos, bw.title AS whole_title, + 1 AS same_series FROM series_books sp JOIN books bp ON bp.id = sp.book_id JOIN series_books sw ON sw.series_id = sp.series_id AND sw.book_id <> sp.book_id JOIN books bw ON bw.id = sw.book_id WHERE bp.status <> ? - AND instr(sp.position_in_series, '.') > 0 - AND bw.excluded = 0 - AND (bw.status = ? OR bw.monitored = 1) - AND bw.owner_user_id IS bp.owner_user_id` - args := []any{models.BookStatusImported, models.BookStatusImported} + AND instr(sp.position_in_series, '.') > 0` + wholeGate + seriesFilter + ` + UNION ALL + SELECT sp.series_id, sp.book_id, sp.position_in_series, bp.title, bp.monitored, + bw.id, '', bw.title, 0 + FROM series_books sp + JOIN books bp ON bp.id = sp.book_id + JOIN books bw ON bw.author_id = bp.author_id AND bw.id <> bp.id + WHERE bp.status <> ? + AND (bp.title GLOB '*[Pp][Aa][Rr][Tt]*[0-9]*' OR bp.title GLOB '*[Pp][Tt]*[0-9]*')` + wholeGate + ` + AND EXISTS (SELECT 1 FROM series_books ws WHERE ws.book_id = bw.id) + AND NOT EXISTS ( + SELECT 1 FROM series_books x + JOIN series_books y ON y.series_id = x.series_id AND y.book_id <> x.book_id + JOIN books b2 ON b2.id = y.book_id + WHERE x.book_id = bp.id AND lower(b2.title) = lower(bw.title))` + seriesFilter + ` + ) + ORDER BY part_series, part_id, same_series DESC, whole_id` + // Both branches bind the same placeholders in the same order. + branchArgs := []any{models.BookStatusImported, models.BookStatusImported} if seriesID > 0 { - query += ` AND sp.series_id = ?` - args = append(args, seriesID) + branchArgs = append(branchArgs, seriesID) } - query += ` ORDER BY sp.series_id, sp.book_id, sw.book_id` + args := append(append([]any{}, branchArgs...), branchArgs...) rows, err := r.db.QueryContext(ctx, query, args...) if err != nil { @@ -72,15 +114,19 @@ func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID in for rows.Next() { var p SplitEditionPart var partPos, wholePos string - var monitored int + var monitored, sameSeries int if err := rows.Scan(&p.SeriesID, &p.BookID, &partPos, &p.Title, &monitored, - &p.WholeBookID, &wholePos, &p.WholeTitle); err != nil { + &p.WholeBookID, &wholePos, &p.WholeTitle, &sameSeries); err != nil { return nil, fmt.Errorf("scan split edition part: %w", err) } if _, dup := seen[key{p.SeriesID, p.BookID}]; dup { continue } - if !seriesmatch.SplitEditionPartOf(p.Title, partPos, p.WholeTitle, wholePos) { + if sameSeries == 1 { + if !seriesmatch.SplitEditionPartOf(p.Title, partPos, p.WholeTitle, wholePos) { + continue + } + } else if !seriesmatch.SplitEditionTitleOf(p.Title, p.WholeTitle) { continue } p.Monitored = monitored == 1 diff --git a/internal/db/split_edition_parts_test.go b/internal/db/split_edition_parts_test.go index 3ee7e1031..8a737f732 100644 --- a/internal/db/split_edition_parts_test.go +++ b/internal/db/split_edition_parts_test.go @@ -2,6 +2,8 @@ package db import ( "context" + "database/sql" + "fmt" "testing" "github.com/vavallee/bindery/internal/models" @@ -30,10 +32,6 @@ func TestListCoveredSplitEditionParts(t *testing.T) { t.Fatal(err) } - author := &models.Author{ForeignID: "hc:sanderson", Name: "Brandon Sanderson", SortName: "Sanderson, Brandon", MetadataProvider: "hardcover"} - if err := authors.Create(ctx, author); err != nil { - t.Fatal(err) - } type row struct { title, position, status string monitored, excluded bool @@ -41,6 +39,12 @@ func TestListCoveredSplitEditionParts(t *testing.T) { } seed := func(seriesTitle string, rows ...row) (int64, []int64) { t.Helper() + // One author per case: a whole by the same author in another series + // would cover the part across series (#3324) and blur the cases. + author := &models.Author{ForeignID: "hc:" + seriesTitle, Name: seriesTitle, SortName: seriesTitle, MetadataProvider: "hardcover"} + if err := authors.Create(ctx, author); err != nil { + t.Fatal(err) + } s := &models.Series{ForeignID: "hc-series:" + seriesTitle, Title: seriesTitle} if err := series.Create(ctx, s); err != nil { t.Fatal(err) @@ -122,3 +126,234 @@ func TestListCoveredSplitEditionParts(t *testing.T) { t.Errorf("UnmonitorNotImported changed %d rows, want 1 (the imported part is left alone)", n) } } + +// TestListCoveredSplitEditionParts_AcrossSeries is #3324, seeded from the +// live case: Hardcover lists Part 2 of The Eye of the World beside the whole +// in The Wheel of Time (hc-series:1097) at 1.2, but Part 1 only in The Wheel +// of Time (Finnish Numbering) (hc-series:248644), where every part is its own +// volume and sits at the integer position 1. +func TestListCoveredSplitEditionParts_AcrossSeries(t *testing.T) { + f := newSplitPartsFixture(t) + jordan := f.author("hc:robert-jordan") + wot := f.series("hc-series:1097", "The Wheel of Time") + finnish := f.series("hc-series:248644", "The Wheel of Time (Finnish Numbering)") + + whole := f.book(jordan, "The Eye of the World", models.BookStatusImported, true, 0) + f.link(wot, whole, "1") + part2 := f.book(jordan, "The Eye of the World, Part 2 of 2: To The Blight", models.BookStatusWanted, true, 0) + f.link(wot, part2, "1.2") + part1 := f.book(jordan, "The Eye of the World, Part 1 of 2: From The Two Rivers", models.BookStatusWanted, true, 0) + f.link(finnish, part1, "1") + + parts, err := f.books.ListCoveredSplitEditionParts(f.ctx, 0) + if err != nil { + t.Fatal(err) + } + got := make(map[int64]SplitEditionPart, len(parts)) + for _, p := range parts { + got[p.BookID] = p + } + if p, ok := got[part2]; !ok || p.SeriesID != wot || p.WholeBookID != whole { + t.Errorf("part 2 (same series, 1.2) = %+v, %v; want covered by %d in series %d", p, ok, whole, wot) + } + if p, ok := got[part1]; !ok || p.SeriesID != finnish || p.WholeBookID != whole || !p.Monitored { + t.Errorf("part 1 (other series, position 1) = %+v, %v; want covered by %d in series %d", p, ok, whole, finnish) + } + if len(parts) != 2 { + t.Errorf("parts = %+v, want exactly the two Eye of the World parts", parts) + } + + scoped, err := f.books.ListCoveredSplitEditionParts(f.ctx, finnish) + if err != nil { + t.Fatal(err) + } + if len(scoped) != 1 || scoped[0].BookID != part1 { + t.Errorf("scoped to the Finnish series = %+v, want only part 1", scoped) + } +} + +// TestListCoveredSplitEditionParts_AcrossSeriesGuards pins what the +// title-only cross-series rule must not pair. Each case is its own author so +// no case can cover another's part. +func TestListCoveredSplitEditionParts_AcrossSeriesGuards(t *testing.T) { + f := newSplitPartsFixture(t) + bob, err := f.users.Create(f.ctx, "bob", "h2") + if err != nil { + t.Fatal(err) + } + const wholeTitle, partTitle = "The Eye of the World", "The Eye of the World, Part 1 of 2: From The Two Rivers" + + type tc struct { + name string + setup func(name string) int64 // returns the part id + } + cases := []tc{ + {"different author", func(n string) int64 { + a, b := f.author("hc:"+n+"-a"), f.author("hc:"+n+"-b") + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(b, partTitle, models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"different owner", func(n string) int64 { + a := f.author("hc:" + n) + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, bob.ID) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"shares a series row, integer position", func(n string) int64 { + a := f.author("hc:" + n) + s := f.series("s:"+n, n) + f.link(s, f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(s, p, "3") + return p + }}, + {"no part marker", func(n string) int64 { + a := f.author("hc:" + n) + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, "The Eye of the World: The Graphic Novel, Volume One", models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"whole excluded", func(n string) int64 { + a := f.author("hc:" + n) + w := f.book(a, wholeTitle, models.BookStatusImported, true, 0) + f.exclude(w) + f.link(f.series("s:"+n+"-a", n+" a"), w, "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"whole wanted and unmonitored", func(n string) int64 { + a := f.author("hc:" + n) + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusWanted, false, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"shares a series with a second copy of the whole", func(n string) int64 { + a := f.author("hc:" + n) + s := f.series("s:"+n, n) + f.link(s, f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(s, p, "3") + // Another row with the whole's title, in no series shared with the part. + f.link(f.series("s:"+n+"-other", n+" other"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + return p + }}, + {"whole in no series", func(n string) int64 { + a := f.author("hc:" + n) + f.book(a, wholeTitle, models.BookStatusImported, true, 0) + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"part already imported", func(n string) int64 { + a := f.author("hc:" + n) + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusImported, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + } + partIDs := make(map[int64]string, len(cases)) + for i, c := range cases { + partIDs[c.setup(fmt.Sprintf("guard-%d", i))] = c.name + } + // Positive control in the same fixture, so a cross-series branch that + // matched nothing at all could not pass every guard. + ctrl := f.author("hc:control") + f.link(f.series("s:control-a", "control a"), f.book(ctrl, wholeTitle, models.BookStatusImported, true, 0), "1") + control := f.book(ctrl, partTitle, models.BookStatusWanted, true, 0) + f.link(f.series("s:control-b", "control b"), control, "1") + + parts, err := f.books.ListCoveredSplitEditionParts(f.ctx, 0) + if err != nil { + t.Fatal(err) + } + sawControl := false + for _, p := range parts { + if p.BookID == control { + sawControl = true + } + if name, ok := partIDs[p.BookID]; ok { + t.Errorf("%s: part %d reported as covered by %d", name, p.BookID, p.WholeBookID) + } + } + if !sawControl { + t.Errorf("control: cross-series part %d not reported; parts = %+v", control, parts) + } +} + +type splitPartsFixture struct { + t *testing.T + ctx context.Context + db *sql.DB + users *UserRepo + authors *AuthorRepo + books *BookRepo + seriesRepo *SeriesRepo + n int +} + +func newSplitPartsFixture(t *testing.T) *splitPartsFixture { + t.Helper() + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + return &splitPartsFixture{ + t: t, ctx: context.Background(), db: database, + users: NewUserRepo(database), authors: NewAuthorRepo(database), + books: NewBookRepo(database), seriesRepo: NewSeriesRepo(database), + } +} + +func (f *splitPartsFixture) author(foreignID string) int64 { + f.t.Helper() + a := &models.Author{ForeignID: foreignID, Name: foreignID, SortName: foreignID, MetadataProvider: "hardcover"} + if err := f.authors.Create(f.ctx, a); err != nil { + f.t.Fatal(err) + } + return a.ID +} + +func (f *splitPartsFixture) series(foreignID, title string) int64 { + f.t.Helper() + s := &models.Series{ForeignID: foreignID, Title: title} + if err := f.seriesRepo.Create(f.ctx, s); err != nil { + f.t.Fatal(err) + } + return s.ID +} + +func (f *splitPartsFixture) link(seriesID, bookID int64, position string) { + f.t.Helper() + if err := f.seriesRepo.LinkBook(f.ctx, seriesID, bookID, position, true); err != nil { + f.t.Fatal(err) + } +} + +func (f *splitPartsFixture) book(authorID int64, title, status string, monitored bool, owner int64) int64 { + f.t.Helper() + f.n++ + b := &models.Book{ + ForeignID: fmt.Sprintf("hc:book-%d", f.n), AuthorID: authorID, Title: title, SortTitle: title, + Status: status, Monitored: monitored, MediaType: models.MediaTypeEbook, + Genres: []string{}, MetadataProvider: "hardcover", OwnerUserID: owner, + } + if err := f.books.Create(f.ctx, b); err != nil { + f.t.Fatal(err) + } + return b.ID +} + +func (f *splitPartsFixture) exclude(bookID int64) { + f.t.Helper() + if _, err := f.db.ExecContext(f.ctx, "UPDATE books SET excluded = 1 WHERE id = ?", bookID); err != nil { + f.t.Fatal(err) + } +} diff --git a/internal/seriesmatch/split_edition.go b/internal/seriesmatch/split_edition.go index 098e43416..6db905d1a 100644 --- a/internal/seriesmatch/split_edition.go +++ b/internal/seriesmatch/split_edition.go @@ -34,6 +34,16 @@ func SplitEditionPartOf(partTitle, partPosition, wholeTitle, wholePosition strin if !ok || !SamePosition(floor, wholePosition) { return false } + return SplitEditionTitleOf(partTitle, wholeTitle) +} + +// SplitEditionTitleOf is signal 2 of SplitEditionPartOf on its own: partTitle +// is wholeTitle followed by a "Part N" marker. It is only a sound test by +// itself where signal 1 cannot be evaluated, which is a part and a whole that +// share no series (#3324): each series numbers its own volumes, so a position +// in one says nothing about a position in the other. Inside one series, use +// SplitEditionPartOf. +func SplitEditionTitleOf(partTitle, wholeTitle string) bool { loc := splitEditionMarkerRe.FindStringIndex(partTitle) if loc == nil { return false diff --git a/internal/seriesmatch/split_edition_test.go b/internal/seriesmatch/split_edition_test.go index e63a234b6..9115d2901 100644 --- a/internal/seriesmatch/split_edition_test.go +++ b/internal/seriesmatch/split_edition_test.go @@ -30,3 +30,24 @@ func TestSplitEditionPartOf(t *testing.T) { }) } } + +func TestSplitEditionTitleOf(t *testing.T) { + cases := []struct { + name, partTitle, wholeTitle string + want bool + }{ + {"marker and subtitle", "The Eye of the World, Part 1 of 2: From The Two Rivers", "The Eye of the World", true}, + {"pt abbreviation", "The Way of Kings Pt. 2", "The Way of Kings", true}, + {"no marker", "The Eye of the World: The Graphic Novel, Volume One", "The Eye of the World", false}, + {"part of a different book", "The Great Hunt, Part 1 of 2", "The Eye of the World", false}, + {"bare marker", "Part 1", "The Eye of the World", false}, + {"blank whole", "The Eye of the World, Part 1", "", false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := SplitEditionTitleOf(tc.partTitle, tc.wholeTitle); got != tc.want { + t.Fatalf("SplitEditionTitleOf(%q, %q) = %v, want %v", tc.partTitle, tc.wholeTitle, got, tc.want) + } + }) + } +} From 905cbaf93689f9e5145e92737d9c92443e6bf351 Mon Sep 17 00:00:00 2001 From: gchahcg <7381837@proton.me> Date: Sat, 17 Oct 2026 19:12:23 -0400 Subject: [PATCH 2/2] fix(series): keep the cross-series split part query off the owner index (#3324) Review follow-up. The cross-series branch reached the whole through idx_books_owner, which on a single user library is every book against every book: 5.2 s on 3,000 books here, about 30 s on 10k. It now starts from the part, or from the one series' rows under a series filter, and reaches the whole by author; a unary + keeps the owner index out of both branches, which also fixes the same-series branch's plan under a series filter. The shared-series guard moves from a per-pair NOT EXISTS into Go. It only runs for parts that already title-matched, compares with CleanTitle as the match does instead of lower(), and only counts books of the part's own owner, since series rows carry no owner. Each part title is parsed once (seriesmatch.SplitEditionBase). Tests pin the plan (EXPLAIN QUERY PLAN, also under -race), a 3,000-book timing budget (skipped under -race), and the guard, ordering and dedup cases the review found uncovered. The changelog fragment drops its dash. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01WA2CUNg5o4wUBaX6yWXNNk Signed-off-by: gchahcg <7381837@proton.me> --- changelog.d/3324-split-parts-across-series.md | 2 +- internal/db/race_off_test.go | 5 + internal/db/race_on_test.go | 7 + internal/db/split_edition_parts.go | 245 ++++++++++++++---- internal/db/split_edition_parts_test.go | 213 +++++++++++++++ internal/seriesmatch/split_edition.go | 19 +- internal/seriesmatch/split_edition_test.go | 25 ++ 7 files changed, 455 insertions(+), 61 deletions(-) create mode 100644 internal/db/race_off_test.go create mode 100644 internal/db/race_on_test.go diff --git a/changelog.d/3324-split-parts-across-series.md b/changelog.d/3324-split-parts-across-series.md index 762cd24c4..de2b1f120 100644 --- a/changelog.d/3324-split-parts-across-series.md +++ b/changelog.d/3324-split-parts-across-series.md @@ -1,2 +1,2 @@ ### Fixed -- **Split edition parts listed in another series** (#3324) — a part Hardcover lists only in another numbering of the series, such as "The Eye of the World, Part 1 of 2" at 1 in "The Wheel of Time (Finnish Numbering)", now counts as covered by the whole you already own: the wanted search skips it, the series page marks it as a split part, and **Unmonitor N split parts** includes it. +- **Split edition parts listed in another series** (#3324). A part Hardcover lists only in another numbering of the series, such as "The Eye of the World, Part 1 of 2" at 1 in "The Wheel of Time (Finnish Numbering)", now counts as covered by the whole you already own: the wanted search skips it, the series page marks it as a split part, and **Unmonitor N split parts** includes it. diff --git a/internal/db/race_off_test.go b/internal/db/race_off_test.go new file mode 100644 index 000000000..edcd68b53 --- /dev/null +++ b/internal/db/race_off_test.go @@ -0,0 +1,5 @@ +//go:build !race + +package db + +const raceEnabled = false diff --git a/internal/db/race_on_test.go b/internal/db/race_on_test.go new file mode 100644 index 000000000..4c3b49670 --- /dev/null +++ b/internal/db/race_on_test.go @@ -0,0 +1,7 @@ +//go:build race + +package db + +// raceEnabled reports whether the race detector is on. It slows queries +// several fold, so wall clock budgets do not hold under it. +const raceEnabled = true diff --git a/internal/db/split_edition_parts.go b/internal/db/split_edition_parts.go index 223b116cc..503b8fe45 100644 --- a/internal/db/split_edition_parts.go +++ b/internal/db/split_edition_parts.go @@ -35,16 +35,20 @@ type SplitEditionPart struct { // #2524 has. // // A part and a whole that share no series, but have the same author, are -// matched by the title signal alone, seriesmatch.SplitEditionTitleOf (#3324). -// Hardcover can list a part only in another numbering of the series, where -// each part is its own volume at an integer position ("The Eye of the World, -// Part 1 of 2" at 1 in The Wheel of Time (Finnish Numbering)). Positions from -// two series are not comparable, so signal 1 cannot be evaluated there. The -// whole must itself be in a series, and the part's own series must not hold -// any book with the whole's title: there only the two signal rule decides, so -// a part numbered as its own volume inside the whole's series is left alone -// even when the library has a second copy of the whole elsewhere. A part -// matched both ways reports the same-series whole. +// matched by the title signal alone, seriesmatch.SplitEditionTitleOf (#3324), +// evaluated here as its two halves, SplitEditionBase of the part and +// CleanTitle of the whole, so each title is parsed once. Hardcover can list a +// part only in another numbering of the series, where each part is its own +// volume at an integer position ("The Eye of the World, Part 1 of 2" at 1 in +// The Wheel of Time (Finnish Numbering)). Positions from two series are not +// comparable, so signal 1 cannot be evaluated there. The whole must itself be +// in a series, and no series the part is in may hold a book of the part's +// owner with the whole's title (compared by CleanTitle, as the match is; +// series rows carry no owner, so another user's books there are ignored): +// there only the two signal +// rule decides, so a part numbered as its own volume inside the whole's +// series is left alone even when the library has a second copy of the whole +// elsewhere. A part matched both ways reports the same-series whole. // // Only a part that is not imported is reported: an imported part is already on // disk and there is nothing to stop searching for. The whole has to be a book @@ -53,18 +57,130 @@ type SplitEditionPart struct { // (internal/api/series.go) apply the identical eligibility rule to the // catalogue diff; a change to it here needs the same change made there. The // catalogue diff only pairs within one series and has no counterpart to the -// cross-series match. Both rows -// must belong to the same owner, so one user's library never decides what -// another user's sweep searches for. +// cross-series match. Both rows must belong to the same owner, so one user's +// library never decides what another user's sweep searches for. func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID int64) ([]SplitEditionPart, error) { - seriesFilter := "" - if seriesID > 0 { - seriesFilter = ` AND sp.series_id = ?` + matches, guardIDs, err := r.coveredSplitEditionCandidates(ctx, seriesID) + if err != nil { + return nil, err + } + shelvedWithWhole, err := r.partsShelvedWithTitle(ctx, guardIDs) + if err != nil { + return nil, err + } + + type key struct{ series, book int64 } + seen := make(map[key]struct{}) + var parts []SplitEditionPart + for _, m := range matches { + k := key{m.SeriesID, m.BookID} + if _, dup := seen[k]; dup { + continue + } + if _, shelved := shelvedWithWhole[m.BookID]; m.cross && shelved { + continue + } + seen[k] = struct{}{} + parts = append(parts, m.SplitEditionPart) + } + return parts, nil +} + +// splitEditionMatch is a (part, whole) row that passed its rule. A cross +// match still has to clear the shared-series guard. +type splitEditionMatch struct { + SplitEditionPart + cross bool +} + +// coveredSplitEditionCandidates reads the candidate query and keeps the rows +// that pass their rule, part by part with same-series rows first. It returns +// the base title of every cross-matched part, for the guard. The rows are +// closed before it returns, so the guard can query on the same connection. +func (r *BookRepo) coveredSplitEditionCandidates(ctx context.Context, seriesID int64) ([]splitEditionMatch, map[int64]string, error) { + query, args := coveredSplitEditionPartsQuery(seriesID) + rows, err := r.db.QueryContext(ctx, query, args...) + if err != nil { + return nil, nil, fmt.Errorf("list split edition parts: %w", err) + } + defer rows.Close() + + // Cross rows pair one part with many wholes, so each title is parsed once. + type base struct { + title string + ok bool + } + partBases := make(map[int64]base) + wholeTitles := make(map[int64]string) + var matches []splitEditionMatch + guardIDs := make(map[int64]string) // cross-matched part id -> its base title + for rows.Next() { + var m splitEditionMatch + var partPos, wholePos string + var monitored, sameSeries int + if err := rows.Scan(&m.SeriesID, &m.BookID, &partPos, &m.Title, &monitored, + &m.WholeBookID, &wholePos, &m.WholeTitle, &sameSeries); err != nil { + return nil, nil, fmt.Errorf("scan split edition part: %w", err) + } + m.Monitored = monitored == 1 + if sameSeries == 1 { + if !seriesmatch.SplitEditionPartOf(m.Title, partPos, m.WholeTitle, wholePos) { + continue + } + } else { + pb, ok := partBases[m.BookID] + if !ok { + pb.title, pb.ok = seriesmatch.SplitEditionBase(m.Title) + partBases[m.BookID] = pb + } + if !pb.ok { + continue + } + wt, ok := wholeTitles[m.WholeBookID] + if !ok { + wt = seriesmatch.CleanTitle(m.WholeTitle) + wholeTitles[m.WholeBookID] = wt + } + if pb.title != wt { + continue + } + m.cross = true + guardIDs[m.BookID] = pb.title + } + matches = append(matches, m) } + if err := rows.Err(); err != nil { + return nil, nil, fmt.Errorf("list split edition parts: %w", err) + } + return matches, guardIDs, nil +} + +// coveredSplitEditionPartsQuery builds the candidate query behind +// ListCoveredSplitEditionParts: every (part, whole) row that passes the SQL +// side of either rule, same-series rows first for each part. +func coveredSplitEditionPartsQuery(seriesID int64) (string, []any) { + // The unary + keeps idx_books_owner out of both branches. Given the + // choice, SQLite reached the whole through it, which on a single user + // library is every book: every book against every book in the cross + // series branch (about 30 s on 10k books), and every book per series row + // in the same-series branch when there is a series filter. const wholeGate = ` AND bw.excluded = 0 AND (bw.status = ? OR bw.monitored = 1) - AND bw.owner_user_id IS bp.owner_user_id` + AND +bw.owner_user_id IS bp.owner_user_id` + // CROSS JOIN pins the order of the cross-series branch: start from the + // parts (from the one series' rows when there is a series filter), then + // reach the author's other books by idx_books_author_id. + crossFrom := ` + FROM books bp + CROSS JOIN series_books sp ON sp.book_id = bp.id` + seriesFilter := "" + if seriesID > 0 { + crossFrom = ` + FROM series_books sp + CROSS JOIN books bp ON bp.id = sp.book_id` + seriesFilter = ` AND sp.series_id = ?` + } query := ` SELECT part_series, part_id, part_pos, part_title, part_monitored, whole_id, whole_pos, whole_title, same_series @@ -81,18 +197,11 @@ func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID in AND instr(sp.position_in_series, '.') > 0` + wholeGate + seriesFilter + ` UNION ALL SELECT sp.series_id, sp.book_id, sp.position_in_series, bp.title, bp.monitored, - bw.id, '', bw.title, 0 - FROM series_books sp - JOIN books bp ON bp.id = sp.book_id - JOIN books bw ON bw.author_id = bp.author_id AND bw.id <> bp.id + bw.id, '', bw.title, 0` + crossFrom + ` + CROSS JOIN books bw ON bw.author_id = bp.author_id AND bw.id <> bp.id WHERE bp.status <> ? AND (bp.title GLOB '*[Pp][Aa][Rr][Tt]*[0-9]*' OR bp.title GLOB '*[Pp][Tt]*[0-9]*')` + wholeGate + ` - AND EXISTS (SELECT 1 FROM series_books ws WHERE ws.book_id = bw.id) - AND NOT EXISTS ( - SELECT 1 FROM series_books x - JOIN series_books y ON y.series_id = x.series_id AND y.book_id <> x.book_id - JOIN books b2 ON b2.id = y.book_id - WHERE x.book_id = bp.id AND lower(b2.title) = lower(bw.title))` + seriesFilter + ` + AND EXISTS (SELECT 1 FROM series_books ws WHERE ws.book_id = bw.id)` + seriesFilter + ` ) ORDER BY part_series, part_id, same_series DESC, whole_id` // Both branches bind the same placeholders in the same order. @@ -100,40 +209,70 @@ func (r *BookRepo) ListCoveredSplitEditionParts(ctx context.Context, seriesID in if seriesID > 0 { branchArgs = append(branchArgs, seriesID) } - args := append(append([]any{}, branchArgs...), branchArgs...) + return query, append(append([]any{}, branchArgs...), branchArgs...) +} - rows, err := r.db.QueryContext(ctx, query, args...) +// partsShelvedWithTitle returns the parts among bases (part id -> base title) +// that share a series with another book of the same owner whose CleanTitle is +// that base title, which is the cross-series rule's guard. The comparison is +// done here, in Go, so it normalises titles exactly as the match itself does. +func (r *BookRepo) partsShelvedWithTitle(ctx context.Context, bases map[int64]string) (map[int64]struct{}, error) { + out := make(map[int64]struct{}) + if len(bases) == 0 { + return out, nil + } + list := make([]int64, 0, len(bases)) + for id := range bases { + list = append(list, id) + } + const chunk = 500 + for len(list) > 0 { + n := min(chunk, len(list)) + batch := list[:n] + list = list[n:] + if err := r.markShelvedWithTitle(ctx, batch, bases, out); err != nil { + return nil, err + } + } + return out, nil +} + +// markShelvedWithTitle adds to out each part in batch that shares a series +// with another book of the same owner whose CleanTitle is the part's base. +func (r *BookRepo) markShelvedWithTitle(ctx context.Context, batch []int64, bases map[int64]string, out map[int64]struct{}) error { + placeholders := make([]string, len(batch)) + args := make([]any, len(batch)) + for i, id := range batch { + placeholders[i] = "?" + args[i] = id + } + //nolint:gosec // placeholders are a fixed list of "?" markers; every value is bound via args + rows, err := r.db.QueryContext(ctx, ` + SELECT DISTINCT x.book_id, b2.title + FROM series_books x + JOIN books bx ON bx.id = x.book_id + JOIN series_books y ON y.series_id = x.series_id AND y.book_id <> x.book_id + JOIN books b2 ON b2.id = y.book_id + WHERE x.book_id IN (`+strings.Join(placeholders, ",")+`) + AND b2.owner_user_id IS bx.owner_user_id`, args...) if err != nil { - return nil, fmt.Errorf("list split edition parts: %w", err) + return fmt.Errorf("list split edition part neighbours: %w", err) } defer rows.Close() - - type key struct{ series, book int64 } - seen := make(map[key]struct{}) - var parts []SplitEditionPart for rows.Next() { - var p SplitEditionPart - var partPos, wholePos string - var monitored, sameSeries int - if err := rows.Scan(&p.SeriesID, &p.BookID, &partPos, &p.Title, &monitored, - &p.WholeBookID, &wholePos, &p.WholeTitle, &sameSeries); err != nil { - return nil, fmt.Errorf("scan split edition part: %w", err) + var partID int64 + var title string + if err := rows.Scan(&partID, &title); err != nil { + return fmt.Errorf("scan split edition part neighbour: %w", err) } - if _, dup := seen[key{p.SeriesID, p.BookID}]; dup { - continue + if seriesmatch.CleanTitle(title) == bases[partID] { + out[partID] = struct{}{} } - if sameSeries == 1 { - if !seriesmatch.SplitEditionPartOf(p.Title, partPos, p.WholeTitle, wholePos) { - continue - } - } else if !seriesmatch.SplitEditionTitleOf(p.Title, p.WholeTitle) { - continue - } - p.Monitored = monitored == 1 - seen[key{p.SeriesID, p.BookID}] = struct{}{} - parts = append(parts, p) } - return parts, rows.Err() + if err := rows.Err(); err != nil { + return fmt.Errorf("list split edition part neighbours: %w", err) + } + return nil } // UnmonitorNotImported clears the monitored flag on the given books, skipping diff --git a/internal/db/split_edition_parts_test.go b/internal/db/split_edition_parts_test.go index 8a737f732..3decb43bc 100644 --- a/internal/db/split_edition_parts_test.go +++ b/internal/db/split_edition_parts_test.go @@ -4,7 +4,10 @@ import ( "context" "database/sql" "fmt" + "regexp" + "strings" "testing" + "time" "github.com/vavallee/bindery/internal/models" ) @@ -172,6 +175,81 @@ func TestListCoveredSplitEditionParts_AcrossSeries(t *testing.T) { } } +// TestListCoveredSplitEditionParts_ReportsOneWholePerPart pins the ordering +// and dedup when a part has more than one candidate whole. +func TestListCoveredSplitEditionParts_ReportsOneWholePerPart(t *testing.T) { + f := newSplitPartsFixture(t) + + // "The Way of Kings" beside the part in its series, and "Way of Kings" + // (the same CleanTitle) in another series. The same-series whole is the + // one reported: the shared-series guard keeps the other off this part, so + // the two rules never both match one part and the same-series-first + // ordering is only a backstop. + sanderson := f.author("hc:sanderson") + stormlight := f.series("s:stormlight", "The Stormlight Archive") + sameSeriesWhole := f.book(sanderson, "The Way of Kings", models.BookStatusImported, true, 0) + f.link(stormlight, sameSeriesWhole, "1") + f.link(f.series("s:stormlight-alt", "Stormlight (alt)"), f.book(sanderson, "Way of Kings", models.BookStatusImported, true, 0), "1") + kingsPart := f.book(sanderson, "The Way of Kings, Part 1", models.BookStatusWanted, true, 0) + f.link(stormlight, kingsPart, "1.1") + + // Two copies of the whole in another series: the part is reported once. + jordan := f.author("hc:robert-jordan") + wot := f.series("hc-series:1097", "The Wheel of Time") + f.link(wot, f.book(jordan, "The Eye of the World", models.BookStatusImported, true, 0), "1") + f.link(wot, f.book(jordan, "The Eye of the World", models.BookStatusImported, true, 0), "1") + eyePart := f.book(jordan, "The Eye of the World, Part 1 of 2: From The Two Rivers", models.BookStatusWanted, true, 0) + f.link(f.series("hc-series:248644", "The Wheel of Time (Finnish Numbering)"), eyePart, "1") + + parts, err := f.books.ListCoveredSplitEditionParts(f.ctx, 0) + if err != nil { + t.Fatal(err) + } + count := map[int64]int{} + for _, p := range parts { + count[p.BookID]++ + if p.BookID == kingsPart && p.WholeBookID != sameSeriesWhole { + t.Errorf("part matched both ways reported whole %d, want the same-series whole %d", p.WholeBookID, sameSeriesWhole) + } + } + if count[kingsPart] != 1 || count[eyePart] != 1 { + t.Errorf("rows per part = %v, want exactly one each for %d and %d; parts = %+v", count, kingsPart, eyePart, parts) + } +} + +// TestListCoveredSplitEditionParts_GuardIgnoresOtherOwners: series rows carry +// no owner, so a series can list two users' books. Another user's copy of the +// whole beside this user's part must not keep this user's part searched. +func TestListCoveredSplitEditionParts_GuardIgnoresOtherOwners(t *testing.T) { + f := newSplitPartsFixture(t) + alice, err := f.users.Create(f.ctx, "alice", "h1") + if err != nil { + t.Fatal(err) + } + bob, err := f.users.Create(f.ctx, "bob", "h2") + if err != nil { + t.Fatal(err) + } + jordan := f.author("hc:robert-jordan") + wot := f.series("hc-series:1097", "The Wheel of Time") + finnish := f.series("hc-series:248644", "The Wheel of Time (Finnish Numbering)") + // Bob's own copy of the whole, listed in the Finnish series beside Alice's + // part. Created first, so the lower id cannot hide a leak behind ordering. + f.link(finnish, f.book(jordan, "The Eye of the World", models.BookStatusImported, true, bob.ID), "2") + aliceWhole := f.book(jordan, "The Eye of the World", models.BookStatusImported, true, alice.ID) + f.link(wot, aliceWhole, "1") + part := f.book(jordan, "The Eye of the World, Part 1 of 2: From The Two Rivers", models.BookStatusWanted, true, alice.ID) + f.link(finnish, part, "1") + + parts, err := f.books.ListCoveredSplitEditionParts(f.ctx, 0) + if err != nil { + t.Fatal(err) + } + if len(parts) != 1 || parts[0].BookID != part || parts[0].WholeBookID != aliceWhole { + t.Errorf("parts = %+v, want alice's part %d covered by her own whole %d", parts, part, aliceWhole) + } +} + // TestListCoveredSplitEditionParts_AcrossSeriesGuards pins what the // title-only cross-series rule must not pair. Each case is its own author so // no case can cover another's part. @@ -243,6 +321,46 @@ func TestListCoveredSplitEditionParts_AcrossSeriesGuards(t *testing.T) { f.link(f.series("s:"+n+"-other", n+" other"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") return p }}, + {"same author, title is not the whole plus a marker", func(n string) int64 { + // Passes the SQL marker GLOB; only the Go title check rejects it. + a := f.author("hc:" + n) + f.link(f.series("s:"+n+"-a", n+" a"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, "The Eye of the World Companion, Part 2", models.BookStatusWanted, true, 0) + f.link(f.series("s:"+n+"-b", n+" b"), p, "1") + return p + }}, + {"part in two series, one shared with the whole", func(n string) int64 { + // The guard looks at every series the part is in, not only the + // row being reported: the shared series decides, and there the + // part is its own volume. + a := f.author("hc:" + n) + shared := f.series("s:"+n+"-shared", n+" shared") + f.link(shared, f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(shared, p, "3") + f.link(f.series("s:"+n+"-other", n+" other"), p, "1") + return p + }}, + {"shares a series with a differently cased copy of the whole", func(n string) int64 { + a := f.author("hc:" + n) + s := f.series("s:"+n, n) + f.link(s, f.book(a, "the eye of the world", models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(s, p, "3") + f.link(f.series("s:"+n+"-other", n+" other"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + return p + }}, + {"shares a series with a copy of the whole that differs only by normalisation", func(n string) int64 { + // The guard compares titles with CleanTitle, as the match does, + // so "Eye of the World" beside the part still blocks it. + a := f.author("hc:" + n) + s := f.series("s:"+n, n) + f.link(s, f.book(a, "Eye of the World", models.BookStatusImported, true, 0), "1") + p := f.book(a, partTitle, models.BookStatusWanted, true, 0) + f.link(s, p, "3") + f.link(f.series("s:"+n+"-other", n+" other"), f.book(a, wholeTitle, models.BookStatusImported, true, 0), "1") + return p + }}, {"whole in no series", func(n string) int64 { a := f.author("hc:" + n) f.book(a, wholeTitle, models.BookStatusImported, true, 0) @@ -357,3 +475,98 @@ func (f *splitPartsFixture) exclude(bookID int64) { f.t.Fatal(err) } } + +// TestListCoveredSplitEditionParts_LargeLibraryIsFast keeps the cross-series +// branch on a plan that starts from the part: driven from the whole, SQLite +// scanned every book against every same-owner book, about 30 s on 10k books +// (#3324 review). +func TestListCoveredSplitEditionParts_LargeLibraryIsFast(t *testing.T) { + if raceEnabled { + t.Skip("wall clock budget does not hold under -race; TestListCoveredSplitEditionParts_PlanStartsFromThePart pins the plan") + } + f := newSplitPartsFixture(t) + const authors, seriesPerAuthor, booksPerSeries = 50, 3, 20 + for a := 0; a < authors; a++ { + authorID := f.author(fmt.Sprintf("hc:perf-%d", a)) + for s := 0; s < seriesPerAuthor; s++ { + seriesID := f.series(fmt.Sprintf("s:perf-%d-%d", a, s), fmt.Sprintf("perf %d %d", a, s)) + for b := 0; b < booksPerSeries; b++ { + title := fmt.Sprintf("Volume %d %d %d", a, s, b) + if b%4 == 3 { + title = fmt.Sprintf("Volume %d %d %d, Part 1 of 2", a, s, b-1) + } + status := models.BookStatusImported + if b%2 == 1 { + status = models.BookStatusWanted + } + f.link(seriesID, f.book(authorID, title, status, true, 0), fmt.Sprint(b+1)) + } + } + } + + start := time.Now() + parts, err := f.books.ListCoveredSplitEditionParts(f.ctx, 0) + if err != nil { + t.Fatal(err) + } + elapsed := time.Since(start) + // Every part here is its own volume, at an integer position, in its + // whole's own series, so the guard has to leave all of them alone. + if len(parts) != 0 { + t.Errorf("got %d covered parts, want 0 (first: %+v)", len(parts), parts[0]) + } + t.Logf("%d books: %v", authors*seriesPerAuthor*booksPerSeries, elapsed) + if elapsed > 2*time.Second { + t.Fatalf("ListCoveredSplitEditionParts over %d books took %v, want well under 2s", authors*seriesPerAuthor*booksPerSeries, elapsed) + } +} + +// TestListCoveredSplitEditionParts_PlanStartsFromThePart pins the plan the +// timing test measures, deterministically and under -race too: neither branch +// reaches the whole (bw) through idx_books_owner, and the cross-series branch +// reaches it by author. +func TestListCoveredSplitEditionParts_PlanStartsFromThePart(t *testing.T) { + f := newSplitPartsFixture(t) + bwLine := regexp.MustCompile(`\bbw\b`) + authorIdx := regexp.MustCompile(`\bidx_books_author_id\b`) + ownerIdx := regexp.MustCompile(`\bidx_books_owner\b`) + explain := func(seriesID int64) (plan, bw []string) { + t.Helper() + query, args := coveredSplitEditionPartsQuery(seriesID) + rows, err := f.db.QueryContext(f.ctx, "EXPLAIN QUERY PLAN "+query, args...) + if err != nil { + t.Fatal(err) + } + defer rows.Close() + for rows.Next() { + var id, parent, unused int + var detail string + if err := rows.Scan(&id, &parent, &unused, &detail); err != nil { + t.Fatal(err) + } + plan = append(plan, detail) + if bwLine.MatchString(detail) { + bw = append(bw, detail) + } + } + if err := rows.Err(); err != nil { + t.Fatal(err) + } + return plan, bw + } + for _, seriesID := range []int64{0, 1} { + plan, bw := explain(seriesID) + byAuthor := false + for _, line := range bw { + if authorIdx.MatchString(line) { + byAuthor = true + } + if ownerIdx.MatchString(line) { + t.Errorf("seriesID %d: whole looked up through idx_books_owner: %q", seriesID, line) + } + } + if !byAuthor { + t.Errorf("seriesID %d: no step reaches bw by idx_books_author_id; plan:\n%s", seriesID, strings.Join(plan, "\n")) + } + } +} diff --git a/internal/seriesmatch/split_edition.go b/internal/seriesmatch/split_edition.go index 6db905d1a..7e178c9d7 100644 --- a/internal/seriesmatch/split_edition.go +++ b/internal/seriesmatch/split_edition.go @@ -44,16 +44,21 @@ func SplitEditionPartOf(partTitle, partPosition, wholeTitle, wholePosition strin // in one says nothing about a position in the other. Inside one series, use // SplitEditionPartOf. func SplitEditionTitleOf(partTitle, wholeTitle string) bool { + base, ok := SplitEditionBase(partTitle) + return ok && base == CleanTitle(wholeTitle) +} + +// SplitEditionBase returns the CleanTitle form of the title before a "Part N" +// marker, which is what SplitEditionTitleOf compares to CleanTitle of the +// whole, and false when partTitle has no marker or nothing before it. It lets +// a caller pairing one part against many wholes parse the part once. +func SplitEditionBase(partTitle string) (string, bool) { loc := splitEditionMarkerRe.FindStringIndex(partTitle) if loc == nil { - return false - } - prefix := strings.TrimRight(strings.TrimSpace(partTitle[:loc[0]]), " ,:;-") - if prefix == "" { - return false + return "", false } - whole := CleanTitle(wholeTitle) - return whole != "" && CleanTitle(prefix) == whole + base := CleanTitle(strings.TrimRight(strings.TrimSpace(partTitle[:loc[0]]), " ,:;-")) + return base, base != "" } // fractionalFloor returns the integer floor of pos when pos is a fractional diff --git a/internal/seriesmatch/split_edition_test.go b/internal/seriesmatch/split_edition_test.go index 9115d2901..6a5a0ab07 100644 --- a/internal/seriesmatch/split_edition_test.go +++ b/internal/seriesmatch/split_edition_test.go @@ -51,3 +51,28 @@ func TestSplitEditionTitleOf(t *testing.T) { }) } } + +func TestSplitEditionBase(t *testing.T) { + cases := []struct { + title, want string + ok bool + }{ + {"The Eye of the World, Part 1 of 2: From The Two Rivers", "eye of world", true}, + {"The Way of Kings Pt. 2", "way of kings", true}, + {"Words of Radiance - Part 1", "words of radiance", true}, + {"Part 1", "", false}, + {"The, Part 1", "", false}, + {"The Eye of the World", "", false}, + } + for _, tc := range cases { + t.Run(tc.title, func(t *testing.T) { + got, ok := SplitEditionBase(tc.title) + if got != tc.want || ok != tc.ok { + t.Fatalf("SplitEditionBase(%q) = %q, %v; want %q, %v", tc.title, got, ok, tc.want, tc.ok) + } + if ok && got != CleanTitle(got) { + t.Errorf("SplitEditionBase(%q) = %q is not in CleanTitle form", tc.title, got) + } + }) + } +}