diff --git a/changelog.d/1970-duplicate-candidates.md b/changelog.d/1970-duplicate-candidates.md new file mode 100644 index 000000000..62877c439 --- /dev/null +++ b/changelog.d/1970-duplicate-candidates.md @@ -0,0 +1,2 @@ +### Added +- **Review duplicate titles for an author** (#1970): a new read only report, opened from the author page under **More, Review duplicates**, groups books whose titles look like the same work and says which rule matched: a punctuation or accent variant, a leading article, an edition marker such as Unabridged, or one title being the main title or subtitle of the other ("Mistborn" and "Mistborn: The Final Empire"). That last rule only counts at a colon, bracket or spaced dash, so a series opener like "Foundation" is not flagged against "Foundation and Empire", and it is skipped for two books that are known, different entries in the same series. Nothing changes automatically; excluding a row uses the existing exclude action, so nothing is ever deleted. Thanks gchahcg for building it. diff --git a/changelog.d/2480-skip-tracked-manual-import.md b/changelog.d/2480-skip-tracked-manual-import.md new file mode 100644 index 000000000..d80149615 --- /dev/null +++ b/changelog.d/2480-skip-tracked-manual-import.md @@ -0,0 +1,2 @@ +### Fixed +- **Importing from a folder no longer lists files that are already in your library** (#2480). A file Bindery already tracks, whether at the same path, as a hardlink of a library file, or as a confident match whose book already has that format, is left out of the scan, so rescanning a downloads folder stops offering things you already brought in. Tick **Show already imported** on the Import page to list them again, labelled, for example to replace a corrupt copy. Thanks to @trevorswanson for the fix. diff --git a/cmd/bindery/main.go b/cmd/bindery/main.go index 58280b540..794f0c187 100644 --- a/cmd/bindery/main.go +++ b/cmd/bindery/main.go @@ -976,6 +976,7 @@ func main() { r.Post("/author/{id}/catalogue-reconciliation", authorHandler.ApplyCatalogueReconciliation) r.Get("/author/{id}/relink-upstream/candidates", authorHandler.RelinkCandidates) r.Post("/author/{id}/relink-upstream", authorHandler.RelinkUpstream) + r.Get("/author/{id}/duplicate-candidates", authorHandler.DuplicateCandidates) r.Get("/author/{id}/series", authorHandler.ListSeries) r.Get("/author/{id}/aliases", authorAliasHandler.List) r.Delete("/author/{id}/aliases/{aliasID}", authorAliasHandler.Delete) diff --git a/docs/API.md b/docs/API.md index ddfae1129..9e33bbecd 100644 --- a/docs/API.md +++ b/docs/API.md @@ -47,8 +47,10 @@ POST /api/v1/author/{id}/refresh re-pull profile and works, ski GET /api/v1/author/{id}/catalogue-reconciliation preview stale metadata-only Wanted rows POST /api/v1/author/{id}/catalogue-reconciliation - recheck and remove selected preview rows -GET /api/v1/author/{id}/relink-upstream/candidates + recheck and remove selected preview rows + GET /api/v1/author/{id}/duplicate-candidates + read-only groups of titles that look like the same book (#1970) + GET /api/v1/author/{id}/relink-upstream/candidates search metadata candidates for manual relink POST /api/v1/author/{id}/relink-upstream re-bind to a different foreign ID GET /api/v1/author/{id}/aliases list merged-in alias rows @@ -181,6 +183,43 @@ move that author onto the fallback (#2271). Retry once the primary is answering, or send an explicit `foreignAuthorId`, which skips the search entirely. +`GET /api/v1/author/{id}/duplicate-candidates` reports groups of the author's +books whose titles look like the same book, for human review (#1970). It is +read-only: it never writes, and the review UI's only action is the existing +`PUT /book/{id}/exclude`. Titles are compared with an aggressive fold (case, +punctuation, and diacritics ignored; `&` expanded to "and"), and a pair joins a +group when any of these match: `alnum-equal` (identical after the fold), +`article-strip` (identical after dropping a leading article), +`edition-suffix` (identical after dropping a trailing edition marker), or +`substring` (one title is the whole main title or subtitle of the other, split +at a colon, bracket, or spaced dash, with length guards; "Foundation" does not +match "Foundation and Empire"). `substring` is additionally suppressed when +both books are known, different positions in the same series, so "Mistborn" +at position 1 does not match "Mistborn: The Well of Ascension" at position 2. +Groups are linked transitively, so A≈B and B≈C lands in one group. +The response is: + +```json +{ + "authorId": 42, + "count": 1, + "groups": [ + { + "key": "themartian", + "rules": ["article-strip", "substring"], + "books": [ + { "id": 101, "title": "The Martian", "excluded": false, "rules": ["article-strip"] }, + { "id": 102, "title": "Martian", "excluded": true, "rules": ["article-strip"] } + ] + } + ] +} +``` + +Each `books` entry is the full book object plus a per-book `rules` list; the +group's `rules` is the union across its members. Groups whose members are all +excluded are omitted, and `count` is the number of groups returned. + ### Books ``` diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index f6c33808b..f02d68d6d 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -44,10 +44,12 @@ an arbitrary release is unreliable. The escape hatches: - **Import** (`/import`): **In your library** lists the books a library scan could not match, for you to adopt in place ([Adopting files already in your library](#adopting-files-already-in-your-library)). **From a folder** points - at a folder of files you already have and matches them to books. Either way a - file with no catalogue match gets a metadata search, which creates the book - (and its author, if new) and links the file to it, so an unmatched file is no - longer a dead end. + at a folder of files you already have and matches them to books; files already + tracked in the library are skipped by default, and toggling **Show already + imported** surfaces them again, e.g. to spot a corrupted file or relink one. + Either way a file with no catalogue match gets a metadata search, which + creates the book (and its author, if new) and links the file to it, so an + unmatched file is no longer a dead end. Both still end by attaching a file to a catalogue record. @@ -828,6 +830,22 @@ current catalogue; changing providers alone is not evidence that a work is obsolete. A correlated work that the metadata profile explicitly rejects can still be removed for that rejection reason. +**Duplicate titles.** The same book often reaches the catalogue twice under +slightly different titles — "The Martian" and "Martian", "Dune" and "Dune +(Unabridged)". Open the author and choose **More → Review duplicates…** to see +groups of titles that look like the same book. Each group shows which rule +matched (identical after normalisation, a leading article dropped, an edition +marker dropped, or one title being the main title or subtitle of the other), +and each row shows the rules that pulled it in. A main title or subtitle match +only counts at a colon, bracket or spaced dash, so Asimov's "Foundation" is not +flagged against "Foundation and Empire", and it is skipped when the two books +are known, different entries in the same series ("Mistborn" against "Mistborn: +The Well of Ascension"). Nothing is changed automatically: the only action is +**Exclude** on a row you judge to be the duplicate, which marks it excluded +without deleting anything. An excluded row stays in its group, struck through, +with an **Include** button to undo it; a group leaves the report once fewer +than two of its rows are still included. + ## How author names are filed One value decides the order of the Authors list, the order of the OPDS author diff --git a/internal/api/authors_duplicate_candidates.go b/internal/api/authors_duplicate_candidates.go new file mode 100644 index 000000000..849d2cb3c --- /dev/null +++ b/internal/api/authors_duplicate_candidates.go @@ -0,0 +1,84 @@ +package api + +import ( + "net/http" + + "github.com/vavallee/bindery/internal/duplicates" +) + +// duplicateCandidatesResponse is the payload for GET +// /author/{id}/duplicate-candidates (#1970). +type duplicateCandidatesResponse struct { + AuthorID int64 `json:"authorId"` + Groups []duplicates.Group `json:"groups"` + Count int `json:"count"` +} + +// DuplicateCandidates implements GET /author/{id}/duplicate-candidates +// (#1970): a read-only, human-reviewed duplicate-title report for one +// author's catalogue. It scans every book row — excluded rows included, so a +// group whose members have already been excluded can be re-shown as "all +// excluded" instead of vanishing — keeps only groups with at least two +// non-excluded members, and returns them annotated with the rule(s) that +// matched so the UI can explain each group in plain language. +// +// The endpoint writes nothing: acting on a candidate is the existing exclude +// route (PUT /book/{id}/exclude), which the UI calls only after a human +// confirms. That is the whole safety model of #1970 — aggressive detection, +// human in the loop, no silent merges. +// +// It also passes each book's series memberships to Scan (#1970 review), so +// the substring rule can be suppressed between two books that are different, +// known positions in the same series — a long first title that doubles as +// the series name ("Foundation" vs "Foundation and Empire") would otherwise +// group with its own sequels. h.series is optional (nil in tests that don't +// need it); without it, this simply falls back to the pre-review behaviour. +func (h *AuthorHandler) DuplicateCandidates(w http.ResponseWriter, r *http.Request) { + author, ok := h.loadOwnedAuthor(w, r) + if !ok { + return + } + books, err := h.books.ListByAuthorIncludingExcluded(r.Context(), author.ID) + if err != nil { + writeServerError(w, r, err) + return + } + var seriesSlots map[int64][]duplicates.SeriesSlot + if h.series != nil { + memberships, err := h.series.ListBookSeriesMembershipsByAuthor(r.Context(), author.ID) + if err != nil { + writeServerError(w, r, err) + return + } + seriesSlots = make(map[int64][]duplicates.SeriesSlot, len(memberships)) + for bookID, ms := range memberships { + for _, m := range ms { + seriesSlots[bookID] = append(seriesSlots[bookID], duplicates.SeriesSlot{ + SeriesID: m.SeriesID, + Position: m.Position, + }) + } + } + } + groups := duplicates.Scan(books, seriesSlots) + active := groups[:0] + for i := range groups { + n := 0 + for _, m := range groups[i].Members { + if !m.Excluded { + n++ + } + } + if n >= 2 { + active = append(active, groups[i]) + } + } + if active == nil { + active = []duplicates.Group{} + } + writeJSON(w, http.StatusOK, duplicateCandidatesResponse{ + AuthorID: author.ID, + Groups: active, + Count: len(active), + }) +} diff --git a/internal/api/authors_duplicate_candidates_test.go b/internal/api/authors_duplicate_candidates_test.go new file mode 100644 index 000000000..00f50716a --- /dev/null +++ b/internal/api/authors_duplicate_candidates_test.go @@ -0,0 +1,445 @@ +package api + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "slices" + "strconv" + "strings" + "testing" + "time" + + "github.com/go-chi/chi/v5" + + "github.com/vavallee/bindery/internal/auth" + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +type dupCandBook struct { + ID int64 `json:"id"` + Title string `json:"title"` + Excluded bool `json:"excluded"` + Rules []string `json:"rules"` +} + +type dupCandGroup struct { + Key string `json:"key"` + Rules []string `json:"rules"` + Books []dupCandBook `json:"books"` +} + +type dupCandResponse struct { + AuthorID int64 `json:"authorId"` + Groups []dupCandGroup `json:"groups"` + Count int `json:"count"` +} + +type dupCandFixture struct { + ctx context.Context + authors *db.AuthorRepo + books *db.BookRepo + series *db.SeriesRepo + handler *AuthorHandler +} + +func newDupCandFixture(t *testing.T) (*dupCandFixture, *models.Author) { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + series := db.NewSeriesRepo(database) + f := &dupCandFixture{ + ctx: ctx, + authors: authors, + books: books, + series: series, + handler: NewAuthorHandler(authors, nil, books, series, nil, nil, nil, nil), + } + author := &models.Author{Name: "Andy Weir", SortName: "Weir, Andy", Monitored: true} + if err := authors.Create(ctx, author); err != nil { + t.Fatalf("seed author: %v", err) + } + return f, author +} + +// addBook seeds a book row under the author and returns it with its ID. +func (f *dupCandFixture) addBook(t *testing.T, authorID int64, title, foreignID string, excluded bool) *models.Book { + t.Helper() + b := &models.Book{ + ForeignID: foreignID, + AuthorID: authorID, + Title: title, + SortTitle: title, + Monitored: true, + Status: models.BookStatusWanted, + MetadataProvider: "openlibrary", + MediaType: models.MediaTypeEbook, + Excluded: excluded, + } + if err := f.books.Create(f.ctx, b); err != nil { + t.Fatalf("seed book %q: %v", title, err) + } + if excluded { + if err := f.books.SetExcluded(f.ctx, b.ID, true); err != nil { + t.Fatalf("exclude book %q: %v", title, err) + } + } + return b +} + +// linkSeries creates (or reuses, by foreignID) a series and links book at the +// given position — the real DB path the review asked DuplicateCandidates to +// use so the substring rule can tell a series' own sequels apart. +func (f *dupCandFixture) linkSeries(t *testing.T, foreignID, title string, bookID int64, position string) { + t.Helper() + s := &models.Series{ForeignID: foreignID, Title: title} + if err := f.series.CreateOrGet(f.ctx, s); err != nil { + t.Fatalf("create series %q: %v", title, err) + } + if err := f.series.LinkBook(f.ctx, s.ID, bookID, position, false); err != nil { + t.Fatalf("link book %d to series %q at %q: %v", bookID, title, position, err) + } +} + +func (f *dupCandFixture) get(t *testing.T, authorID int64, userID int64) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/api/v1/author/"+strconv.FormatInt(authorID, 10)+"/duplicate-candidates", nil) + rctx := chi.NewRouteContext() + rctx.URLParams.Add("id", strconv.FormatInt(authorID, 10)) + ctx := context.WithValue(req.Context(), chi.RouteCtxKey, rctx) + if userID != 0 { + ctx = auth.WithUserID(ctx, userID) + } + req = req.WithContext(ctx) + rec := httptest.NewRecorder() + f.handler.DuplicateCandidates(rec, req) + return rec +} + +func parseDupCand(t *testing.T, rec *httptest.ResponseRecorder) dupCandResponse { + t.Helper() + var resp dupCandResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode response: %v: %s", err, rec.Body.String()) + } + return resp +} + +func TestDuplicateCandidates_ReturnsGroupsWithRules(t *testing.T) { + f, author := newDupCandFixture(t) + + f.addBook(t, author.ID, "The Martian", "OL1001W", false) + f.addBook(t, author.ID, "The Martian", "OL1002W", false) + m3 := f.addBook(t, author.ID, "Martian", "OL1003W", false) + f.addBook(t, author.ID, "The Martian: A Novel", "OL1004W", false) + d1 := f.addBook(t, author.ID, "Dune", "OL1005W", false) + d2 := f.addBook(t, author.ID, "Dune", "OL1006W", false) + f.addBook(t, author.ID, "Hyperion", "OL1007W", false) + + rec := f.get(t, author.ID, 0) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + resp := parseDupCand(t, rec) + + if resp.AuthorID != author.ID { + t.Errorf("authorId = %d, want %d", resp.AuthorID, author.ID) + } + if resp.Count != 2 || len(resp.Groups) != 2 { + t.Fatalf("count = %d, groups = %d; want 2 of each", resp.Count, len(resp.Groups)) + } + // Deterministic order: groups sorted by key. + if resp.Groups[0].Key != "dune" || resp.Groups[1].Key != "themartian" { + t.Fatalf("group order = %q, %q; want dune, themartian", resp.Groups[0].Key, resp.Groups[1].Key) + } + + dune := resp.Groups[0] + if len(dune.Books) != 2 { + t.Fatalf("dune group has %d books, want 2", len(dune.Books)) + } + if !rulesEqual(dune.Rules, []string{"alnum-equal"}) { + t.Errorf("dune rules = %v, want [alnum-equal]", dune.Rules) + } + if dune.Books[0].ID != d1.ID || dune.Books[1].ID != d2.ID { + t.Errorf("dune members not sorted by ID: %d, %d", dune.Books[0].ID, dune.Books[1].ID) + } + + martian := resp.Groups[1] + if len(martian.Books) != 4 { + t.Fatalf("martian group has %d books, want 4", len(martian.Books)) + } + for _, want := range []string{"article-strip", "substring"} { + if !contains(martian.Rules, want) { + t.Errorf("martian rules = %v, want to contain %s", martian.Rules, want) + } + } + // m3 ("Martian") matched the group via article-strip against m1/m2. + m3rules := rulesByID(martian.Books, m3.ID) + if !contains(m3rules, "article-strip") { + t.Errorf("member %q rules = %v, want article-strip", "Martian", m3rules) + } +} + +// TestDuplicateCandidates_SeriesPositionsSuppressSubstring is the end-to-end +// form of the #1970 review finding: "Foundation" is a folded substring of +// "Foundation and Empire", but real series data (different, known positions +// in the same series) proves they are different books, and the endpoint must +// not report them as a candidate pair. "Mistborn" / "Mistborn: The Final +// Empire" share a series position, so that pair still groups. +func TestDuplicateCandidates_SeriesPositionsSuppressSubstring(t *testing.T) { + f, author := newDupCandFixture(t) + + foundation := f.addBook(t, author.ID, "Foundation", "OL6001W", false) + foundationAndEmpire := f.addBook(t, author.ID, "Foundation and Empire", "OL6002W", false) + secondFoundation := f.addBook(t, author.ID, "Second Foundation", "OL6003W", false) + f.linkSeries(t, "OLS1S", "Foundation", foundation.ID, "1") + f.linkSeries(t, "OLS1S", "Foundation", foundationAndEmpire.ID, "2") + f.linkSeries(t, "OLS1S", "Foundation", secondFoundation.ID, "3") + + mistborn := f.addBook(t, author.ID, "Mistborn", "OL6004W", false) + mistbornFull := f.addBook(t, author.ID, "Mistborn: The Final Empire", "OL6005W", false) + f.linkSeries(t, "OLS2S", "Mistborn", mistborn.ID, "1") + f.linkSeries(t, "OLS2S", "Mistborn", mistbornFull.ID, "1") + // "Mistborn" is a whole separator segment of this sequel's title, so only + // the series positions (1 against 2) keep it out of the Mistborn group. + wellOfAscension := f.addBook(t, author.ID, "Mistborn: The Well of Ascension", "OL6006W", false) + f.linkSeries(t, "OLS2S", "Mistborn", wellOfAscension.ID, "2") + + rec := f.get(t, author.ID, 0) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + resp := parseDupCand(t, rec) + + if resp.Count != 1 || len(resp.Groups) != 1 { + t.Fatalf("count = %d, groups = %d; want 1 (Foundation trio suppressed, Mistborn kept)", resp.Count, len(resp.Groups)) + } + g := resp.Groups[0] + if g.Key != "mistborn" { + t.Fatalf("group key = %q, want mistborn", g.Key) + } + if len(g.Books) != 2 { + t.Fatalf("mistborn group has %d books, want 2 (the position 2 sequel must stay out)", len(g.Books)) + } +} + +func TestDuplicateCandidates_404UnknownAuthor(t *testing.T) { + f, _ := newDupCandFixture(t) + rec := f.get(t, 999999, 0) + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func TestDuplicateCandidates_EmptyCatalogue(t *testing.T) { + f, author := newDupCandFixture(t) + rec := f.get(t, author.ID, 0) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + resp := parseDupCand(t, rec) + if resp.Count != 0 || len(resp.Groups) != 0 { + t.Errorf("empty catalogue: count = %d, groups = %d; want 0", resp.Count, len(resp.Groups)) + } + // groups must serialize as [] not null, so the UI can iterate it. + if body := rec.Body.String(); body == "" || !strings.Contains(body, `"groups":[]`) { + t.Errorf("groups should be an empty array, body = %s", body) + } +} + +func TestDuplicateCandidates_SuppressesFullyExcludedGroups(t *testing.T) { + f, author := newDupCandFixture(t) + + // Group A: both members excluded -> must be suppressed. + f.addBook(t, author.ID, "Dune", "OL2001W", true) + f.addBook(t, author.ID, "Dune", "OL2002W", true) + // Group B: two active + one excluded -> returned, all three members shown. + a1 := f.addBook(t, author.ID, "The Martian", "OL2003W", false) + a2 := f.addBook(t, author.ID, "The Martian", "OL2004W", false) + a3 := f.addBook(t, author.ID, "Martian", "OL2005W", true) + + rec := f.get(t, author.ID, 0) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + resp := parseDupCand(t, rec) + if resp.Count != 1 || len(resp.Groups) != 1 { + t.Fatalf("count = %d, groups = %d; want only the partially-excluded group", resp.Count, len(resp.Groups)) + } + g := resp.Groups[0] + if g.Key != "themartian" { + t.Fatalf("group key = %q, want themartian", g.Key) + } + if len(g.Books) != 3 { + t.Fatalf("group has %d books, want 3 (excluded member still shown)", len(g.Books)) + } + byID := map[int64]bool{} + for _, b := range g.Books { + byID[b.ID] = b.Excluded + } + if byID[a1.ID] || byID[a2.ID] || !byID[a3.ID] { + t.Errorf("excluded flags wrong: %v (a3 must be excluded, a1/a2 active)", byID) + } +} + +func TestDuplicateCandidates_IsReadOnly(t *testing.T) { + f, author := newDupCandFixture(t) + f.addBook(t, author.ID, "The Martian", "OL3001W", false) + f.addBook(t, author.ID, "The Martian", "OL3002W", false) + f.addBook(t, author.ID, "Martian", "OL3003W", false) + + before := snapshotBooks(t, f, author.ID) + first := f.get(t, author.ID, 0) + if first.Code != http.StatusOK { + t.Fatalf("first call: %d", first.Code) + } + second := f.get(t, author.ID, 0) + if second.Code != http.StatusOK { + t.Fatalf("second call: %d", second.Code) + } + if first.Body.String() != second.Body.String() { + t.Errorf("responses differ across calls:\n%s\n%s", first.Body.String(), second.Body.String()) + } + after := snapshotBooks(t, f, author.ID) + if before != after { + t.Errorf("endpoint mutated the catalogue:\nbefore = %s\nafter = %s", before, after) + } +} + +// snapshotBooks renders the author's book rows (titles + excluded flags) as +// a stable string so TestDuplicateCandidates_IsReadOnly can compare them. +func snapshotBooks(t *testing.T, f *dupCandFixture, authorID int64) string { + t.Helper() + books, err := f.books.ListByAuthorIncludingExcluded(f.ctx, authorID) + if err != nil { + t.Fatalf("list books: %v", err) + } + out := make([]string, len(books)) + for i, b := range books { + ex := "in" + if b.Excluded { + ex = "ex" + } + out[i] = b.Title + "/" + ex + } + slices.Sort(out) + return strings.Join(out, "|") +} + +func TestDuplicateCandidates_TenancyOnRejectsCrossUserAuthor(t *testing.T) { + auth.SetEnforceTenancyForTests(t, true) + f, aliceAuthor, bobID := dupTenancyFixture(t) + + rec := f.get(t, aliceAuthor.ID, bobID) + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404 for cross-user author with tenancy on, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func TestDuplicateCandidates_TenancyOffAllowsCrossUserAuthor(t *testing.T) { + auth.SetEnforceTenancyForTests(t, false) + f, aliceAuthor, bobID := dupTenancyFixture(t) + + rec := f.get(t, aliceAuthor.ID, bobID) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 with tenancy enforcement off, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func TestDuplicateCandidates_Scale(t *testing.T) { + f, author := newDupCandFixture(t) + for i := 0; i < 996; i++ { + f.addBook(t, author.ID, "Distinct Title Number "+strconv.Itoa(i), "OL"+strconv.Itoa(4000+i)+"W", false) + } + f.addBook(t, author.ID, "The Martian", "OL4997W", false) + f.addBook(t, author.ID, "The Martian", "OL4998W", false) + f.addBook(t, author.ID, "Martian", "OL4999W", false) + f.addBook(t, author.ID, "The Martian: A Novel", "OL5000W", false) + + start := time.Now() + rec := f.get(t, author.ID, 0) + elapsed := time.Since(start) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d: %s", rec.Code, rec.Body.String()) + } + if elapsed > 2*time.Second { + t.Errorf("endpoint took %s, budget 2s", elapsed) + } + resp := parseDupCand(t, rec) + if resp.Count != 1 { + t.Errorf("count = %d, want 1 (the martian group)", resp.Count) + } +} + +// dupTenancyFixture seeds two users, an author owned by alice (with two +// duplicate books), and returns the fixture plus bob's id. +func dupTenancyFixture(t *testing.T) (*dupCandFixture, *models.Author, int64) { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = database.Close() }) + + ctx := context.Background() + users := db.NewUserRepo(database) + alice, err := users.Create(ctx, "alice", "h1") + if err != nil { + t.Fatalf("create alice: %v", err) + } + bob, err := users.Create(ctx, "bob", "h2") + if err != nil { + t.Fatalf("create bob: %v", err) + } + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + f := &dupCandFixture{ + ctx: ctx, + authors: authors, + books: books, + handler: NewAuthorHandler(authors, nil, books, nil, nil, nil, nil, nil), + } + author := &models.Author{ + Name: "Emilia Jae", + SortName: "Jae, Emilia", + Monitored: true, + MetadataProvider: "openlibrary", + } + if err := authors.CreateForUser(ctx, author, alice.ID); err != nil { + t.Fatalf("seed alice author: %v", err) + } + f.addBook(t, author.ID, "The Martian", "OL5001W", false) + f.addBook(t, author.ID, "The Martian", "OL5002W", false) + return f, author, bob.ID +} + +func rulesEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +func rulesByID(books []dupCandBook, id int64) []string { + for _, b := range books { + if b.ID == id { + return b.Rules + } + } + return nil +} diff --git a/internal/api/manual_import.go b/internal/api/manual_import.go index 332fc121c..53653670c 100644 --- a/internal/api/manual_import.go +++ b/internal/api/manual_import.go @@ -40,6 +40,9 @@ type ManualImportHandler struct { // drain them before the database closes (#1458, #2371). Optional: without // it they run untracked, which is what tests and non-wired callers get. jobs *jobs.Group + // trackedCache holds Scan's already-tracked index, rebuilt only when + // book_files changes (see trackedFileIndex, #2480). + trackedCache trackedFileCache } func NewManualImportHandler(scanner manualImportScanner, downloads *db.DownloadRepo, books *db.BookRepo) *ManualImportHandler { @@ -431,6 +434,80 @@ func (h *ManualImportHandler) removeStaleSource(ctx context.Context, src string, _ = os.Remove(filepath.Dir(src)) // best-effort: prune the now-empty folder } +// fileMatchesTracked reports whether path is another directory entry for a +// file already registered in book_files. Imports may hard-link the source into +// its canonical library destination, so comparing path strings alone is not +// sufficient to keep a repeated scan from presenting the same file again. +// +// tracked comes from a cache (trackedFileIndex) that is only rebuilt when +// book_files changes, and os.SameFile compares device and inode alone. When a +// tracked file is deleted outside Bindery its row stays, the cache keeps its +// old FileInfo, and ext4 hands the freed inode straight to the next new file, +// so a fresh download could be hidden as already imported. A genuine hardlink +// shares the inode's size and mtime too, so those must also agree. A tracked +// file modified in place since the cache was built then shows as importable, +// which is the safe way to be wrong. +func fileMatchesTracked(path string, tracked []os.FileInfo) bool { + info, err := os.Stat(path) + if err != nil { + return false + } + for _, trackedInfo := range tracked { + if os.SameFile(info, trackedInfo) && + info.Size() == trackedInfo.Size() && + info.ModTime().Equal(trackedInfo.ModTime()) { + return true + } + } + return false +} + +// directoryMatchesTracked reports whether every importable file below path is +// already registered in book_files. Directory units represent either a +// multi-format ebook or a folder audiobook, so checking only the directory's +// own path cannot identify an already-imported unit. A directory with even one +// untracked ebook/audio file remains eligible for import. +func directoryMatchesTracked(path string, tracked map[string]struct{}, trackedFiles []os.FileInfo) bool { + found := false + allTracked := true + _ = filepath.WalkDir(path, func(p string, entry os.DirEntry, walkErr error) error { + if walkErr != nil { + allTracked = false + return nil + } + if entry.IsDir() || (!importer.IsEbookFile(p) && !importer.IsAudioFile(p)) { + return nil + } + found = true + if _, ok := tracked[filepath.Clean(p)]; !ok && !fileMatchesTracked(p, trackedFiles) { + allTracked = false + } + return nil + }) + return found && allTracked +} + +// bookHasImportedFormat reports whether the matched book already has an +// on-disk file or directory for format. Read book_files directly rather than +// relying on the hydrated compatibility fields on models.Book: LookupBatchLayout +// may receive a book value from a catalogue projection where those fields are +// empty even though the tracking row exists. This must match the importer's +// idempotency guard, or a copied manual-import source will keep reappearing. +func bookHasImportedFormat(book *models.Book, format string, files []models.BookFile) bool { + if book == nil { + return false + } + for _, file := range files { + if file.Format != format { + continue + } + if _, err := os.Stat(file.Path); err == nil { + return true + } + } + return false +} + // ScanItem is one candidate book unit discovered under a folder during Scan. type ScanItem struct { Path string `json:"path"` @@ -441,6 +518,13 @@ type ScanItem struct { DetectedFormat string `json:"detectedFormat"` Book *models.Book `json:"book,omitempty"` Candidates []models.Book `json:"candidates,omitempty"` + // AlreadyImported reports whether this unit is already tracked in + // book_files (directly, as a hardlink of a tracked file, or as a + // confident catalogue match whose book already has that format on disk). + // Such units are excluded by default; includeImported=true surfaces them + // instead, labelled, so a corrupt file or one needing a relink can still + // be found through this scan (#2480). + AlreadyImported bool `json:"alreadyImported"` } // ScanResponse is the result of scanning a folder for importable book units. @@ -453,40 +537,93 @@ type ScanResponse struct { // an enormous tree can't produce an unbounded response or stall the request. const maxScanEntries = 1000 -// Scan handles GET /api/v1/queue/manual-import/scan?path=... -// It walks a folder RECURSIVELY (issue #1434), enumerating individual ebook -// files as units at whatever depth they live while keeping a genuine -// folder-based audiobook as a single unit, and runs one catalogue-backed batch -// lookup — matching by embedded EPUB metadata and folder-derived author, not the -// filename alone — returning a per-unit match list to review and bulk-import. No -// state is modified. -func (h *ManualImportHandler) Scan(w http.ResponseWriter, r *http.Request) { - path := filepath.Clean(r.URL.Query().Get("path")) +// maxScanRounds bounds how many times Scan re-walks the tree to refill the +// page after the already-imported filters drop some of a round's candidates +// (#2480). enumerateImportUnits has no way to resume mid-tree, so each round +// re-walks from the root; the cap bounds the worst case — a library that has +// already been entirely re-imported — to a small constant multiple of a +// single walk's cost, consistent with the existing maxWalkEntries/maxWalkDepth +// guards. +const maxScanRounds = 4 + +// resolveImportFolder validates the user-selected scan folder and returns the +// symlink-resolved path that is safe for subsequent filesystem operations. +// The caller may select any configured library subfolder, but the raw query +// value must never reach the filesystem before ResolveContained approves it. +func (h *ManualImportHandler) resolveImportFolder(ctx context.Context, rawPath string) (string, int, string) { + path := filepath.Clean(rawPath) if path == "" || path == "." { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path parameter required"}) - return + return "", http.StatusBadRequest, "path parameter required" } if !filepath.IsAbs(path) { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path must be absolute"}) - return + return "", http.StatusBadRequest, "path must be absolute" } - resolved, ok := h.roots.ResolveContained(r.Context(), path) + resolved, ok := h.roots.ResolveContained(ctx, path) if !ok { - writeJSON(w, http.StatusForbidden, map[string]string{"error": h.outsideRootsMessage(r.Context())}) - return + return "", http.StatusForbidden, h.outsideRootsMessage(ctx) } - path = resolved - info, err := os.Stat(path) //nolint:gosec // #nosec G304 -- symlink-resolved and confirmed inside a configured library root; RequireAdmin enforced at route level + info, err := os.Stat(resolved) //nolint:gosec // #nosec G304 -- resolved by ResolveContained after symlink-aware root containment; route requires admin if err != nil { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": fmt.Sprintf("path not accessible: %v", err)}) - return + return "", http.StatusBadRequest, fmt.Sprintf("path not accessible: %v", err) } if !info.IsDir() { - writeJSON(w, http.StatusBadRequest, map[string]string{"error": "path must be a folder; use lookup for a single book"}) + return "", http.StatusBadRequest, "path must be a folder; use lookup for a single book" + } + return resolved, 0, "" +} + +// isAlreadyTracked reports whether path (a file or, when isDir, a directory +// unit) is already present in book_files, either at the exact path (tracked) +// or as a hardlink of one (trackedFiles, via fileMatchesTracked / +// directoryMatchesTracked). +func isAlreadyTracked(path string, isDir bool, tracked map[string]struct{}, trackedFiles []os.FileInfo) bool { + if isDir { + return directoryMatchesTracked(path, tracked, trackedFiles) + } + if _, ok := tracked[filepath.Clean(path)]; ok { + return true + } + return fileMatchesTracked(path, trackedFiles) +} + +// Scan handles GET /api/v1/queue/manual-import/scan?path=...&includeImported=... +// It walks a folder RECURSIVELY (issue #1434), enumerating individual ebook +// files as units at whatever depth they live while keeping a genuine +// folder-based audiobook as a single unit, and runs one catalogue-backed batch +// lookup — matching by embedded EPUB metadata and folder-derived author, not the +// filename alone — returning a per-unit match list to review and bulk-import. No +// state is modified. +// +// Units already tracked in book_files (directly, via a hardlink, or as a +// confident match whose book already has that format on disk) are filtered +// out during the walk itself rather than after enumerateImportUnits' 1000-unit +// cap — filtering the capped result let an already-imported folder starve the +// cap and come back empty even though untracked files existed further down +// the tree (#2480). When the post-lookup format check still drops units from +// a round, Scan re-walks (bounded by maxScanRounds) to refill the page rather +// than return a short page while truncated is still true. +// +// includeImported=true disables both filters and labels each unit's +// AlreadyImported status instead of excluding it, so an already-imported file +// that's corrupt or needs a relink can still be found here. +func (h *ManualImportHandler) Scan(w http.ResponseWriter, r *http.Request) { + path, status, message := h.resolveImportFolder(r.Context(), r.URL.Query().Get("path")) + if status != 0 { + writeJSON(w, status, map[string]string{"error": message}) return } + includeImported := r.URL.Query().Get("includeImported") == "true" start := time.Now() + tracked, trackedFiles, err := h.trackedFileIndex(r.Context(), path) + if err != nil { + slog.Error("bulk folder import scan failed to load tracked files", "path", path, "error", err) + writeServerError(w, r, err) + return + } + + slog.Info("bulk folder import scan started", "path", path, "includeImported", includeImported) + // Enumerate importable units by walking the tree RECURSIVELY (issue #1434): // individual ebook files become units at whatever depth they live, while a // genuine folder-based audiobook (loose tracks or disc subfolders) stays one @@ -495,44 +632,106 @@ func (h *ManualImportHandler) Scan(w http.ResponseWriter, r *http.Request) { // happened to match. A single catalogue-backed batch lookup then matches them // all at once — the per-item full-table scan that stalled large scans past the // server WriteTimeout stays fixed (issue #1473). - cands, truncated := enumerateImportUnits(path, maxScanEntries) + items := make([]ScanItem, 0, maxScanEntries) + matched := 0 + truncated := false + alreadyConsumed := 0 + roundsRun := 0 + + for round := 0; round < maxScanRounds; round++ { + remaining := maxScanEntries - len(items) + if remaining <= 0 { + break + } + roundsRun++ + seen := 0 + skipToOffset := alreadyConsumed + roundSkip := func(p string, isDir bool) bool { + if !includeImported && isAlreadyTracked(p, isDir, tracked, trackedFiles) { + return true + } + seen++ + return seen <= skipToOffset + } + cands, roundTruncated := enumerateImportUnits(path, remaining, roundSkip) + truncated = roundTruncated + alreadyConsumed += len(cands) + if len(cands) == 0 { + break + } - slog.Info("bulk folder import scan started", "path", path, "entries", len(cands), "truncated", truncated) + roundPaths := make([]string, len(cands)) + for i, c := range cands { + roundPaths[i] = c.path + } + results, err := h.scanner.LookupBatchLayout(r.Context(), path, roundPaths) + if err != nil { + slog.Error("bulk folder import scan failed to load catalogue", "path", path, "error", err) + writeServerError(w, r, err) + return + } - paths := make([]string, len(cands)) - for i, c := range cands { - paths[i] = c.path - } - results, err := h.scanner.LookupBatchLayout(r.Context(), path, paths) - if err != nil { - slog.Error("bulk folder import scan failed to load catalogue", "path", path, "error", err) - writeServerError(w, r, err) - return - } + // Batch the confident matches' book_files lookup into one query instead + // of one round trip per match (#2480). + bookIDs := make([]int64, 0, len(results)) + seenBookIDs := make(map[int64]bool, len(results)) + for _, res := range results { + if res.Match == "confident" && res.Book != nil && !seenBookIDs[res.Book.ID] { + seenBookIDs[res.Book.ID] = true + bookIDs = append(bookIDs, res.Book.ID) + } + } + filesByBook, err := h.books.ListFilesForBooks(r.Context(), bookIDs) + if err != nil { + slog.Warn("bulk folder import scan could not verify matched book files; retaining candidates", + "path", path, "error", err) + filesByBook = map[int64][]models.BookFile{} + } - resp := ScanResponse{Items: make([]ScanItem, 0, len(cands)), Truncated: truncated} - matched := 0 - for i, c := range cands { - res := results[i] - if res.Match == "confident" { - matched++ + for i, res := range results { + c := cands[i] + // roundSkip (above) already dropped every isAlreadyTracked unit + // from cands when includeImported is off, so re-running it here + // would just recompute false for every survivor — only worth the + // call when includeImported means roundSkip did NOT do that + // filtering (#2480 review: this ran for every unit, in both + // modes, doubling the warm-cache stat count for no reason). + alreadyImported := includeImported && isAlreadyTracked(c.path, c.isDir, tracked, trackedFiles) + if !alreadyImported && res.Match == "confident" && res.Book != nil && + bookHasImportedFormat(res.Book, res.DetectedFormat, filesByBook[res.Book.ID]) { + alreadyImported = true + } + if alreadyImported && !includeImported { + continue + } + if res.Match == "confident" { + matched++ + } + items = append(items, ScanItem{ + Path: c.path, + Name: c.name, + Match: res.Match, + ParsedTitle: res.ParsedTitle, + ParsedAuthor: res.ParsedAuthor, + DetectedFormat: res.DetectedFormat, + Book: res.Book, + Candidates: res.Candidates, + AlreadyImported: alreadyImported, + }) + } + + if !roundTruncated { + break // tree exhausted; nothing more to find regardless of round budget } - resp.Items = append(resp.Items, ScanItem{ - Path: c.path, - Name: c.name, - Match: res.Match, - ParsedTitle: res.ParsedTitle, - ParsedAuthor: res.ParsedAuthor, - DetectedFormat: res.DetectedFormat, - Book: res.Book, - Candidates: res.Candidates, - }) } + + resp := ScanResponse{Items: items, Truncated: truncated} slog.Info("bulk folder import scan complete", "path", path, "duration", time.Since(start), "items", len(resp.Items), "matched", matched, + "rounds", roundsRun, "truncated", truncated) writeJSON(w, http.StatusOK, resp) } diff --git a/internal/api/manual_import_cache.go b/internal/api/manual_import_cache.go new file mode 100644 index 000000000..ce9c0a2d4 --- /dev/null +++ b/internal/api/manual_import_cache.go @@ -0,0 +1,116 @@ +package api + +import ( + "context" + "os" + "path/filepath" + "sync" + + "github.com/vavallee/bindery/internal/importer" +) + +// trackedFileCache holds the manual-import scan's "already tracked" index — +// the same-path set plus the os.FileInfo values needed for hardlink detection +// (fileMatchesTracked/directoryMatchesTracked) — rebuilt only when book_files +// has actually changed, or the scanned root sits on a different device than +// the index it holds was built for (#2480). +// +// Every scan used to re-stat the entire book_files table regardless of the +// scanned folder's size, which is expensive on network storage (#1473 already +// hit the same handler's write timeout once). Caching the index keyed on the +// table's own fingerprint turns that into "stat the library once per +// book_files mutation, anywhere in the app" instead of "once per scan +// request". +type trackedFileCache struct { + mu sync.Mutex + + count int64 + maxID int64 + pathEpoch uint64 + rootDevID uint64 + rootDevKnown bool + tracked map[string]struct{} + trackedFiles []os.FileInfo +} + +// trackedFileIndex returns the current already-tracked path set and the +// os.FileInfo values needed for hardlink detection against scanRoot, +// rebuilding only when book_files has changed since the last build (per +// BookRepo.BookFilesFingerprint for inserts and deletes, and +// BookRepo.BookFilesPathEpoch for the reorganize action's in place path +// rewrites, which the fingerprint cannot see) or scanRoot's device differs from the one +// the cached index was built for. +// +// A tracked path on a different device than scanRoot can never be a hardlink +// of anything under scanRoot, so it skips the expensive directory walk that +// hardlink detection otherwise needs for it — it still gets the cheap +// exact-path entry. This targets network-storage setups where the library and +// a freshly scanned download share live on different mounts (the NFS case +// called out in review on #2480). scanRoot's device is resolved once per +// call, here, rather than once per tracked row: an earlier version compared +// devices via confirmedCrossDevice(scanRoot, trackedPath) inside the rebuild +// loop below, which re-stat'd scanRoot on every single tracked row in the +// library — 3000 tracked rows meant 3000 extra stats of the same path. Now +// each row's device comes off the same os.Stat this loop already does to +// build its FileInfo, so a cold rebuild costs exactly one stat per tracked row +// (plus the directory walk for a genuine tracked folder), not up to five. +func (h *ManualImportHandler) trackedFileIndex(ctx context.Context, scanRoot string) (map[string]struct{}, []os.FileInfo, error) { + // Read the rename epoch first so a reorganize racing this rebuild can only + // make the next call rebuild again, never leave a stale path cached. + pathEpoch := h.books.BookFilesPathEpoch() + count, maxID, err := h.books.BookFilesFingerprint(ctx) + if err != nil { + return nil, nil, err + } + rootDevID, rootDevKnown := deviceID(scanRoot) + + h.trackedCache.mu.Lock() + defer h.trackedCache.mu.Unlock() + + c := &h.trackedCache + if c.tracked != nil && c.count == count && c.maxID == maxID && c.pathEpoch == pathEpoch && c.rootDevID == rootDevID && c.rootDevKnown == rootDevKnown { + return c.tracked, c.trackedFiles, nil + } + + trackedPaths, err := h.books.ListAllBookFilePaths(ctx) + if err != nil { + return nil, nil, err + } + tracked := make(map[string]struct{}, len(trackedPaths)) + trackedFiles := make([]os.FileInfo, 0, len(trackedPaths)) + for _, trackedPath := range trackedPaths { + cleaned := filepath.Clean(trackedPath) + tracked[cleaned] = struct{}{} + info, statErr := os.Stat(cleaned) //nolint:gosec // #nosec G304 -- path comes from book_files, populated only by prior imports through this same admin-gated handler + if statErr != nil { + continue + } + if rootDevKnown { + if pathDevID, ok := deviceIDOf(info); ok && pathDevID != rootDevID { + continue + } + } + trackedFiles = append(trackedFiles, info) + if !info.IsDir() { + continue + } + _ = filepath.WalkDir(cleaned, func(p string, entry os.DirEntry, walkErr error) error { + if walkErr != nil || entry.IsDir() || (!importer.IsEbookFile(p) && !importer.IsAudioFile(p)) { + return nil + } + if childInfo, childErr := os.Stat(p); childErr == nil { //nolint:gosec // #nosec G304 -- p is a directory entry beneath a book_files path, same trust boundary as above + trackedFiles = append(trackedFiles, childInfo) + } + return nil + }) + } + + c.count = count + c.maxID = maxID + c.pathEpoch = pathEpoch + c.rootDevID = rootDevID + c.rootDevKnown = rootDevKnown + c.tracked = tracked + c.trackedFiles = trackedFiles + return tracked, trackedFiles, nil +} diff --git a/internal/api/manual_import_test.go b/internal/api/manual_import_test.go index efdbdaca9..f5684e925 100644 --- a/internal/api/manual_import_test.go +++ b/internal/api/manual_import_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "errors" + "fmt" "net/http" "net/http/httptest" "net/url" @@ -1192,6 +1193,753 @@ func TestManualImportScan_EnumeratesBookUnits(t *testing.T) { } } +func TestFileMatchesTracked_EdgeCases(t *testing.T) { + t.Parallel() + root := t.TempDir() + trackedPath := filepath.Join(root, "tracked.epub") + otherPath := filepath.Join(root, "other.epub") + writeTestFile(t, trackedPath) + writeTestFile(t, otherPath) + trackedInfo, err := os.Stat(trackedPath) + if err != nil { + t.Fatal(err) + } + + if !fileMatchesTracked(trackedPath, []os.FileInfo{trackedInfo}) { + t.Fatal("tracked path should match its own file identity") + } + if fileMatchesTracked(otherPath, []os.FileInfo{trackedInfo}) { + t.Fatal("different file should not match tracked identity") + } + if fileMatchesTracked(filepath.Join(root, "missing.epub"), []os.FileInfo{trackedInfo}) { + t.Fatal("missing path should not match") + } +} + +func TestDirectoryMatchesTracked_WalkError(t *testing.T) { + t.Parallel() + if directoryMatchesTracked(filepath.Join(t.TempDir(), "missing"), nil, nil) { + t.Fatal("unreadable or missing directory should not be considered tracked") + } +} + +func TestBookHasImportedFormat_EdgeCases(t *testing.T) { + t.Parallel() + root := t.TempDir() + present := filepath.Join(root, "book.epub") + missing := filepath.Join(root, "missing.epub") + writeTestFile(t, present) + book := &models.Book{ID: 1} + + if bookHasImportedFormat(nil, models.MediaTypeEbook, nil) { + t.Fatal("nil book should not report an imported format") + } + if bookHasImportedFormat(book, models.MediaTypeEbook, []models.BookFile{{Format: models.MediaTypeAudiobook, Path: present}}) { + t.Fatal("different format should not count") + } + if bookHasImportedFormat(book, models.MediaTypeEbook, []models.BookFile{{Format: models.MediaTypeEbook, Path: missing}}) { + t.Fatal("missing file should not count as imported") + } + if !bookHasImportedFormat(book, models.MediaTypeEbook, []models.BookFile{{Format: models.MediaTypeEbook, Path: present}}) { + t.Fatal("present file should count as imported") + } +} + +func TestDirectoryMatchesTracked(t *testing.T) { + root := t.TempDir() + trackedPath := filepath.Join(root, "tracked.epub") + otherTrackedPath := filepath.Join(root, "tracked.mobi") + if err := os.WriteFile(trackedPath, []byte("epub"), 0o600); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(otherTrackedPath, []byte("mobi"), 0o600); err != nil { + t.Fatal(err) + } + + tracked := map[string]struct{}{ + filepath.Clean(trackedPath): {}, + filepath.Clean(otherTrackedPath): {}, + } + trackedFiles := []os.FileInfo{} + for _, path := range []string{trackedPath, otherTrackedPath} { + info, err := os.Stat(path) + if err != nil { + t.Fatal(err) + } + trackedFiles = append(trackedFiles, info) + } + + if !directoryMatchesTracked(root, tracked, trackedFiles) { + t.Fatal("fully tracked directory should be skipped") + } + + untrackedPath := filepath.Join(root, "new.epub") + if err := os.WriteFile(untrackedPath, []byte("new"), 0o600); err != nil { + t.Fatal(err) + } + if directoryMatchesTracked(root, tracked, trackedFiles) { + t.Fatal("directory with an untracked importable file should remain eligible") + } +} + +func TestManualImportScan_SkipsAlreadyTrackedFiles(t *testing.T) { + t.Parallel() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books) + + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + tracked := filepath.Join(root, "already-imported.epub") + newFile := filepath.Join(root, "new-book.epub") + writeTestFile(t, tracked) + writeTestFile(t, newFile) + canonical := filepath.Join(root, "canonical", "already-imported.epub") + if err := os.MkdirAll(filepath.Dir(canonical), 0o755); err != nil { + t.Fatalf("mkdir canonical path: %v", err) + } + if err := os.Link(tracked, canonical); err != nil { + t.Fatalf("hard-link tracked file: %v", err) + } + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, canonical); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + h.WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 1 || resp.Items[0].Path != newFile { + t.Fatalf("items = %+v, want only the new file %q", resp.Items, newFile) + } + if stub.lookupBatchCalls != 1 { + t.Fatalf("LookupBatch called %d times, want 1", stub.lookupBatchCalls) + } +} + +func TestManualImportScan_SkipsCopiedSourceForTrackedBook(t *testing.T) { + t.Parallel() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + source := filepath.Join(root, "Department Q", "7. A Department Q Novel - Caroline Waight, Jussi Adler-Olsen (2024).epub") + canonical := filepath.Join(root, "library", "Department Q.epub") + if err := os.MkdirAll(filepath.Dir(source), 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(canonical), 0o755); err != nil { + t.Fatal(err) + } + writeTestFile(t, source) + writeTestFile(t, canonical) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, canonical); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{ + Match: "confident", Book: trackedBook, DetectedFormat: models.MediaTypeEbook, + }} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 0 { + t.Fatalf("items = %+v, want copied source skipped because the matched format is already tracked", resp.Items) + } +} + +// scanRequestIncludeImported builds a GET request for the manual-import scan +// endpoint with includeImported=true, so already-tracked units are surfaced +// (labelled) instead of excluded (#2480). +func scanRequestIncludeImported(path string) *http.Request { + u := "/api/v1/queue/manual-import/scan?" + url.Values{"path": {path}, "includeImported": {"true"}}.Encode() + return httptest.NewRequest(http.MethodGet, u, nil) +} + +// TestManualImportScan_LargeTrackedRegionDoesNotStarveTheCap is the exact +// repro from the #2480 review: enumerateImportUnits' 1000-unit cap trips +// entirely within an already-tracked region (walk order is alphabetical, and +// the tracked files sort first), so filtering the CAPPED result — rather than +// filtering during the walk itself — comes back empty with truncated=true +// even though untracked files exist further down the tree. The fix folds the +// tracked check into the walk so the cap counts only surfaced units. +func TestManualImportScan_LargeTrackedRegionDoesNotStarveTheCap(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books) + root := t.TempDir() + + const trackedCount = maxScanEntries + 5 + for i := 0; i < trackedCount; i++ { + // "a"-prefixed names sort before the "z"-prefixed new files below, so + // the pre-fix walk order hits the cap entirely within the tracked + // region before ever reaching a new file. + p := filepath.Join(root, fmt.Sprintf("a-tracked-%04d.epub", i)) + writeTestFile(t, p) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, p); err != nil { + t.Fatalf("track file %d: %v", i, err) + } + } + const newCount = 5 + newPaths := make([]string, 0, newCount) + for i := 0; i < newCount; i++ { + p := filepath.Join(root, fmt.Sprintf("z-new-%d.epub", i)) + writeTestFile(t, p) + newPaths = append(newPaths, p) + } + h.WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if resp.Truncated { + t.Error("truncated = true, want false: every unit was either tracked or one of the returned new files") + } + if len(resp.Items) != newCount { + t.Fatalf("items = %d, want exactly the %d new files; got %+v", len(resp.Items), newCount, resp.Items) + } + got := map[string]bool{} + for _, it := range resp.Items { + got[it.Path] = true + if it.AlreadyImported { + t.Errorf("new file %q should not be labelled AlreadyImported", it.Path) + } + } + for _, p := range newPaths { + if !got[p] { + t.Errorf("missing expected new file %q in response", p) + } + } +} + +// TestManualImportScan_RefillsAcrossRoundsWhenFormatFilterEmptiesAPage covers +// the second, post-lookup already-imported filter (bookHasImportedFormat): +// when it empties an entire round, Scan re-walks for more rather than +// returning a short page while truncated is still true (#2480). +func TestManualImportScan_RefillsAcrossRoundsWhenFormatFilterEmptiesAPage(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + canonical := filepath.Join(root, "library", "canonical.epub") + writeTestFile(t, canonical) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, canonical); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + + // Every one of these confidently matches trackedBook, which already has an + // ebook file (canonical, above) — so the format filter drops every single + // one. One more than maxScanRounds full rounds' worth, so even the last + // round hits its own cap (roundTruncated stays true) instead of + // exhausting the tree — otherwise the walk would legitimately finish and + // correctly report truncated=false. + const copyCount = maxScanEntries*maxScanRounds + 1 + for i := 0; i < copyCount; i++ { + writeTestFile(t, filepath.Join(root, "copies", fmt.Sprintf("copy-%04d.epub", i))) + } + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{ + Match: "confident", Book: trackedBook, DetectedFormat: models.MediaTypeEbook, + }} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 0 { + t.Fatalf("items = %+v, want none: every copy matches an already-imported format", resp.Items) + } + if !resp.Truncated { + t.Error("truncated = false, want true: more copies exist beyond the round budget") + } + // The round budget bounds cost: exactly maxScanRounds batch lookups, not + // one massive or unbounded walk. + if stub.lookupBatchCalls != maxScanRounds { + t.Errorf("LookupBatch called %d times, want exactly maxScanRounds=%d", stub.lookupBatchCalls, maxScanRounds) + } +} + +// TestManualImportScan_IncludeImportedSurfacesTrackedUnits verifies +// includeImported=true disables both already-imported filters and labels the +// surfaced units instead of dropping them (#2480), so a corrupt already- +// imported file or one needing a relink can still be found. +func TestManualImportScan_IncludeImportedSurfacesTrackedUnits(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + + // Exact-path tracked file (filter 1). + trackedPath := filepath.Join(root, "already-imported.epub") + writeTestFile(t, trackedPath) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, trackedPath); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + // A genuinely new file, for contrast. + newPath := filepath.Join(root, "new-book.epub") + writeTestFile(t, newPath) + + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequestIncludeImported(root)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body = %s", rec.Code, rec.Body.String()) + } + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 2 { + t.Fatalf("items = %+v, want both the tracked and the new file surfaced", resp.Items) + } + byPath := map[string]ScanItem{} + for _, it := range resp.Items { + byPath[it.Path] = it + } + if !byPath[trackedPath].AlreadyImported { + t.Errorf("tracked file should be labelled AlreadyImported, got %+v", byPath[trackedPath]) + } + if byPath[newPath].AlreadyImported { + t.Errorf("new file should not be labelled AlreadyImported, got %+v", byPath[newPath]) + } + + // Without the toggle, only the new file is returned (existing behavior). + rec2 := httptest.NewRecorder() + h.Scan(rec2, scanRequest(root)) + var resp2 ScanResponse + if err := json.Unmarshal(rec2.Body.Bytes(), &resp2); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp2.Items) != 1 || resp2.Items[0].Path != newPath { + t.Fatalf("without includeImported, items = %+v, want only the new file", resp2.Items) + } +} + +// TestManualImportScan_RevealsFileAfterBookDeletedByCascade is the maintainer +// review repro for #2480: a tracked-file cache keyed on an in-process counter +// that only BookFileRepo's own methods bumped went stale the moment a +// book_files row disappeared some other way. Deleting a book removes its +// book_files rows via the books(id) ON DELETE CASCADE FK (BookRepo.Delete), +// never touching BookFileRepo directly, so the file scan hid forever even +// though nothing tracks it anymore. The fix reads a fingerprint off the table +// itself instead of trusting a counter to have seen every mutation. +func TestManualImportScan_RevealsFileAfterBookDeletedByCascade(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + tracked := filepath.Join(root, "tracked.epub") + writeTestFile(t, tracked) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, tracked); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 0 { + t.Fatalf("before delete: items = %+v, want the tracked file hidden", resp.Items) + } + + // Delete the book, keeping the file on disk. book_files loses its row + // through the FK cascade, not through any BookFileRepo call. + if err := books.Delete(ctx, trackedBook.ID); err != nil { + t.Fatalf("delete book: %v", err) + } + + rec2 := httptest.NewRecorder() + h.Scan(rec2, scanRequest(root)) + var resp2 ScanResponse + if err := json.Unmarshal(rec2.Body.Bytes(), &resp2); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp2.Items) != 1 || resp2.Items[0].Path != tracked { + t.Fatalf("after delete: items = %+v, want the orphaned file surfaced", resp2.Items) + } +} + +// TestManualImportScan_SeesReorganizeMoveBetweenScans pins the second #2480 +// review finding: a (COUNT, MAX(id)) fingerprint cannot see an in place +// UPDATE of book_files.path, and the library reorganize action does exactly +// that through UpdateBookFilePath. Without a rename aware cache key the second +// scan reuses the first scan's index, so a fresh file that lands at the old +// path stays hidden and the moved file at its new path is offered for import +// again. The move is done as copy then remove so the new path gets a new +// inode, the way a cross device reorganize does, and hardlink detection +// cannot paper over the stale path set. The rename goes through a separate +// BookRepo, as reorganize does in production. +func TestManualImportScan_SeesReorganizeMoveBetweenScans(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + oldPath := filepath.Join(root, "old.epub") + newPath := filepath.Join(root, "Author", "new.epub") + writeTestFile(t, oldPath) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, oldPath); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + files, err := books.ListBookFiles(ctx, trackedBook.ID) + if err != nil || len(files) != 1 { + t.Fatalf("list book files: %v (%d rows)", err, len(files)) + } + + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + scan := func() []string { + t.Helper() + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v (%s)", err, rec.Body.String()) + } + paths := make([]string, 0, len(resp.Items)) + for _, it := range resp.Items { + paths = append(paths, it.Path) + } + return paths + } + + if got := scan(); len(got) != 0 { + t.Fatalf("before move: items = %v, want the tracked file hidden", got) + } + + // Reorganize: copy to the new location (new inode), remove the old file, + // then repoint the row in place. Count and max id are unchanged. + data, err := os.ReadFile(oldPath) + if err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(filepath.Dir(newPath), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(newPath, data, 0o644); err != nil { + t.Fatal(err) + } + if err := os.Remove(oldPath); err != nil { + t.Fatal(err) + } + if err := db.NewBookRepo(database).UpdateBookFilePath(ctx, files[0].ID, trackedBook.ID, newPath); err != nil { + t.Fatalf("update book file path: %v", err) + } + // A new, untracked download lands where the old file used to be. + if err := os.WriteFile(oldPath, []byte("a different book"), 0o644); err != nil { + t.Fatal(err) + } + + got := scan() + if len(got) != 1 || got[0] != oldPath { + t.Fatalf("after move: items = %v, want only the new file at the old path %q (moved file at %q must stay hidden)", got, oldPath, newPath) + } +} + +// TestManualImportScan_ReusedInodeIsNotTreatedAsTracked covers a hazard the +// tracked-file cache (#2480) introduced. The cache keeps the os.FileInfo of +// every tracked file so hardlinks can be spotted with os.SameFile, which +// compares device and inode only. If a tracked file is deleted outside +// Bindery (its book_files row stays, so the cache is not rebuilt) the +// filesystem is free to hand that inode to the next file created, and ext4 +// does so immediately. The typical trigger is deleting a corrupt library copy +// and downloading a fresh one: the fresh file then matched the stale FileInfo +// and was hidden as already imported. A real hardlink shares the inode's size +// and mtime as well, so requiring those to match too keeps hardlink detection +// while rejecting a recycled inode. +func TestManualImportScan_ReusedInodeIsNotTreatedAsTracked(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + libDir := filepath.Join(root, "library") + dlDir := filepath.Join(root, "downloads") + if err := os.MkdirAll(libDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.MkdirAll(dlDir, 0o755); err != nil { + t.Fatal(err) + } + tracked := filepath.Join(libDir, "corrupt.epub") + writeTestFile(t, tracked) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, tracked); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + trackedInfo, err := os.Stat(tracked) + if err != nil { + t.Fatal(err) + } + + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + scan := func() []string { + t.Helper() + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(dlDir)) + var resp ScanResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v (%s)", err, rec.Body.String()) + } + paths := make([]string, 0, len(resp.Items)) + for _, it := range resp.Items { + paths = append(paths, it.Path) + } + return paths + } + if got := scan(); len(got) != 0 { + t.Fatalf("empty downloads folder: items = %v", got) + } + + // The user deletes the corrupt library copy by hand and a fresh download + // arrives. book_files is untouched, so the cache is not rebuilt. + if err := os.Remove(tracked); err != nil { + t.Fatal(err) + } + fresh := filepath.Join(dlDir, "fresh.epub") + time.Sleep(10 * time.Millisecond) // a distinct mtime, as any real download has + if err := os.WriteFile(fresh, []byte("a good copy of the book"), 0o644); err != nil { + t.Fatal(err) + } + if freshInfo, err := os.Stat(fresh); err == nil && os.SameFile(freshInfo, trackedInfo) { + t.Logf("inode %v was recycled for the fresh download", freshInfo.Sys()) + } else { + t.Logf("filesystem did not recycle the inode; this run does not exercise the hazard") + } + + if got := scan(); len(got) != 1 || got[0] != fresh { + t.Fatalf("items = %v, want the fresh download %q surfaced", got, fresh) + } +} + +// TestManualImportScan_TrackedPathMissingAtRebuildStillHidden pins the exact +// path half of isAlreadyTracked. A tracked file whose stat failed when the +// index was built (an NFS mount that was briefly away, say) has no FileInfo +// for hardlink matching, so once it is back on disk only the path set keeps +// it from being offered for import again. +func TestManualImportScan_TrackedPathMissingAtRebuildStillHidden(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + tracked := filepath.Join(root, "away.epub") + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, tracked); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + stub := &stubManualImportScanner{lookupResult: importer.LookupResult{Match: "none"}} + h := NewManualImportHandler(stub, downloads, books).WithRoots(NewLibraryRoots(nil, root)) + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) // builds the index while the file is absent + if rec.Code != http.StatusOK { + t.Fatalf("first scan: %d %s", rec.Code, rec.Body.String()) + } + + writeTestFile(t, tracked) // the mount comes back + rec2 := httptest.NewRecorder() + h.Scan(rec2, scanRequest(root)) + var resp ScanResponse + if err := json.Unmarshal(rec2.Body.Bytes(), &resp); err != nil { + t.Fatalf("decode: %v", err) + } + if len(resp.Items) != 0 { + t.Fatalf("items = %+v, want the tracked file hidden by its exact path", resp.Items) + } +} + +// TestManualImportHandler_TrackedFileIndex_CachesUntilBookFilesChange +// verifies the tracked-file index is rebuilt only when book_files actually +// changes (#2480): mutating the map trackedFileIndex returns and calling it +// again with no intervening write must still see that mutation, proving the +// second call served the cached map rather than rebuilding a fresh one. +func TestManualImportHandler_TrackedFileIndex_CachesUntilBookFilesChange(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + authors := db.NewAuthorRepo(database) + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + trackedBook := seedBook(t, authors, books, ctx) + root := t.TempDir() + tracked := filepath.Join(root, "tracked.epub") + writeTestFile(t, tracked) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, tracked); err != nil { + t.Fatalf("attach tracked file: %v", err) + } + h := NewManualImportHandler(&stubManualImportScanner{}, downloads, books) + + set1, _, err := h.trackedFileIndex(ctx, root) + if err != nil { + t.Fatalf("first trackedFileIndex: %v", err) + } + if _, ok := set1[filepath.Clean(tracked)]; !ok { + t.Fatalf("expected %q in the tracked set, got %v", tracked, set1) + } + + // Add a second tracked file — the version bump must force a rebuild on + // the next call even though we haven't closed the DB yet. + tracked2 := filepath.Join(root, "tracked2.epub") + writeTestFile(t, tracked2) + if err := books.AddBookFile(ctx, trackedBook.ID, models.MediaTypeEbook, tracked2); err != nil { + t.Fatalf("attach second tracked file: %v", err) + } + set2, _, err := h.trackedFileIndex(ctx, root) + if err != nil { + t.Fatalf("second trackedFileIndex: %v", err) + } + if _, ok := set2[filepath.Clean(tracked2)]; !ok { + t.Fatalf("expected the newly tracked file to appear after a book_files write, got %v", set2) + } + + // No further writes: mark the returned map so a rebuild would lose the + // mark, then call again. Getting the mark back proves the exact same map + // was reused rather than rebuilt from a fresh query. + set2[filepath.Clean("/sentinel")] = struct{}{} + set3, _, err := h.trackedFileIndex(ctx, root) + if err != nil { + t.Fatalf("third trackedFileIndex: %v", err) + } + if _, ok := set3[filepath.Clean("/sentinel")]; !ok { + t.Error("expected the cached map to be reused (same map) when book_files did not change") + } +} + +func TestManualImportScan_TrackedFilesError(t *testing.T) { + t.Parallel() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + + books := db.NewBookRepo(database) + downloads := db.NewDownloadRepo(database) + h := NewManualImportHandler(&stubManualImportScanner{}, downloads, books) + root := t.TempDir() + writeTestFile(t, filepath.Join(root, "book.epub")) + h.WithRoots(NewLibraryRoots(nil, root)) + if err := database.Close(); err != nil { + t.Fatal(err) + } + + rec := httptest.NewRecorder() + h.Scan(rec, scanRequest(root)) + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500; body = %s", rec.Code, rec.Body.String()) + } + if strings.Contains(rec.Body.String(), "database is closed") { + t.Errorf("body leaked database error: %q", rec.Body.String()) + } +} + // TestManualImportScan_AllowsConfiguredRootItself reproduces #1373: pasting a // configured root ("/books") or an allow-listed download dir ("/downloads") // into bulk import must scan it, not 403. Before the fix, ResolveContained diff --git a/internal/api/samedevice_unix.go b/internal/api/samedevice_unix.go index cfaff69d6..e0e90a921 100644 --- a/internal/api/samedevice_unix.go +++ b/internal/api/samedevice_unix.go @@ -46,6 +46,37 @@ func nearestExistingDir(p string) string { return "" } +// deviceIDOf extracts a's OS device ID from an os.FileInfo the caller already +// stat'd for another purpose — the manual-import scan's tracked-file rebuild +// (#2480) stats every tracked path once anyway (to detect it and, if it is a +// directory, walk it), so this reads the device id off that same FileInfo +// instead of stat'ing the path again just to compare devices. ok is false +// when the platform doesn't expose a device id (see the Windows build). +func deviceIDOf(fi os.FileInfo) (uint64, bool) { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return uint64(st.Dev), true +} + +// deviceID returns path's OS device ID as a cache key: two paths returning +// (id, true) with equal id are stat-confirmed to share a device; ok is false +// when the ID is unknown (stat failed, or the platform doesn't expose one — +// see the Windows build of this function). Used to key the manual-import +// scan's tracked-file cache (#2480) by the scanned root's device — computed +// once per Scan request, not once per tracked row (a prior version compared +// devices via a per-row confirmedCrossDevice(scanRoot, trackedPath) call that +// re-stat'd scanRoot on every tracked row in the library; see trackedFileIndex +// for the review that caught it). +func deviceID(path string) (uint64, bool) { + fi, err := os.Stat(path) + if err != nil { + return 0, false + } + return deviceIDOf(fi) +} + // hardlinkableReason reports whether downloads in a can actually be // hard-linked into b and, when they can't, WHY — the generic "will copy // instead" banner sent users hunting through mount tables blind (#1427). diff --git a/internal/api/samedevice_unix_test.go b/internal/api/samedevice_unix_test.go new file mode 100644 index 000000000..2e6e029d1 --- /dev/null +++ b/internal/api/samedevice_unix_test.go @@ -0,0 +1,56 @@ +//go:build !windows + +package api + +import ( + "os" + "path/filepath" + "testing" +) + +// TestDeviceIDOf_MatchesDeviceID verifies deviceIDOf, given an os.FileInfo the +// caller already has, agrees with deviceID stat'ing the same path fresh — the +// manual-import scan's tracked-file rebuild (#2480 review) relies on reading +// the device id off a FileInfo it already stat'd instead of stat'ing again. +func TestDeviceIDOf_MatchesDeviceID(t *testing.T) { + t.Parallel() + root := t.TempDir() + a := filepath.Join(root, "a.epub") + writeTestFile(t, a) + + fi, err := os.Stat(a) + if err != nil { + t.Fatalf("Stat: %v", err) + } + fromInfo, okInfo := deviceIDOf(fi) + fromPath, okPath := deviceID(a) + if !okInfo || !okPath { + t.Fatalf("expected both to resolve, got okInfo=%v okPath=%v", okInfo, okPath) + } + if fromInfo != fromPath { + t.Errorf("deviceIDOf(stat) = %d, deviceID(path) = %d, want equal", fromInfo, fromPath) + } +} + +// TestDeviceID_SameDirSameDevice verifies deviceID reports a usable, stable +// identity for paths that do share a device, and (false) for a path that +// doesn't exist — the manual-import scan's cache key (#2480) relies on both. +func TestDeviceID_SameDirSameDevice(t *testing.T) { + t.Parallel() + root := t.TempDir() + a := filepath.Join(root, "a.epub") + writeTestFile(t, a) + + aID, aOK := deviceID(a) + rootID, rootOK := deviceID(root) + if !aOK || !rootOK { + t.Fatalf("expected deviceID to resolve for existing paths, got aOK=%v rootOK=%v", aOK, rootOK) + } + if aID != rootID { + t.Errorf("a file and its parent dir should report the same device: %d != %d", aID, rootID) + } + + if _, ok := deviceID(filepath.Join(root, "missing.epub")); ok { + t.Error("deviceID for a nonexistent path should report ok=false") + } +} diff --git a/internal/api/samedevice_windows.go b/internal/api/samedevice_windows.go index 8159157f3..137a5f5be 100644 --- a/internal/api/samedevice_windows.go +++ b/internal/api/samedevice_windows.go @@ -2,6 +2,8 @@ package api +import "os" + // hardlinkableReason always reports not-hardlinkable on Windows because // device-ID comparison via syscall.Stat_t is not available there — mirrors // importer.sameDevice's platform split, with the explanation the #1427 @@ -9,3 +11,19 @@ package api func hardlinkableReason(_, _ string) (bool, string) { return false, "hardlink detection is not supported on Windows; imports will copy instead" } + +// deviceIDOf always reports unknown on Windows, for the same reason as +// deviceID below. +func deviceIDOf(_ os.FileInfo) (uint64, bool) { + return 0, false +} + +// deviceID always reports unknown on Windows because device-ID comparison via +// syscall.Stat_t is not available there. The manual-import scan's +// tracked-file cache (#2480) treats "unknown" as a single shared cache key, +// which is correct here since every tracked path still gets the full +// stat/hardlink check on this platform regardless of the scanned root's +// device — the built index never actually depends on it. +func deviceID(_ string) (uint64, bool) { + return 0, false +} diff --git a/internal/api/scan_walk.go b/internal/api/scan_walk.go index 01844c44a..41a3018ae 100644 --- a/internal/api/scan_walk.go +++ b/internal/api/scan_walk.go @@ -42,7 +42,15 @@ const ( // // Enumeration stops once `limit` units are collected (truncated=true) or the // entry/depth guards trip. Units are returned in a stable, name-sorted order. -func enumerateImportUnits(root string, limit int) (units []scanUnit, truncated bool) { +// +// skip, when non-nil, is consulted for every unit the walk would otherwise +// emit; a unit it reports true for is dropped without counting toward limit +// or truncation, so the walk keeps descending past it in search of `limit` +// units skip accepts. Callers use this to filter out already-tracked units +// during the walk itself, rather than capping first and filtering the capped +// result — the latter can starve the cap entirely when most of a large, +// already-imported folder sits ahead of any new files in walk order. +func enumerateImportUnits(root string, limit int, skip func(path string, isDir bool) bool) (units []scanUnit, truncated bool) { entriesSeen := 0 var walk func(dir string, depth int, isRoot bool) walk = func(dir string, depth int, isRoot bool) { @@ -77,6 +85,9 @@ func enumerateImportUnits(root string, limit int) (units []scanUnit, truncated b sort.Strings(ebookFiles) emit := func(path string, isDir bool) { + if skip != nil && skip(path, isDir) { + return + } if len(units) >= limit { truncated = true return diff --git a/internal/api/scan_walk_series_test.go b/internal/api/scan_walk_series_test.go index dde3025fa..c8ee6bb98 100644 --- a/internal/api/scan_walk_series_test.go +++ b/internal/api/scan_walk_series_test.go @@ -130,7 +130,7 @@ func TestEnumerateImportUnits_SeriesVersusDiscSet(t *testing.T) { for _, f := range tc.files { writeTestFile(t, filepath.Join(root, filepath.FromSlash(f))) } - units, truncated := enumerateImportUnits(root, 1000) + units, truncated := enumerateImportUnits(root, 1000, nil) if truncated { t.Fatalf("unexpected truncation for a small tree") } diff --git a/internal/api/scan_walk_test.go b/internal/api/scan_walk_test.go index 5b30e5141..5baa5dcc5 100644 --- a/internal/api/scan_walk_test.go +++ b/internal/api/scan_walk_test.go @@ -129,7 +129,7 @@ func TestEnumerateImportUnits_BoundaryHeuristic(t *testing.T) { writeTestFile(t, filepath.Join(root, filepath.FromSlash(f))) } - units, truncated := enumerateImportUnits(root, 1000) + units, truncated := enumerateImportUnits(root, 1000, nil) if truncated { t.Errorf("unexpected truncation for a small tree") } @@ -165,7 +165,7 @@ func TestEnumerateImportUnits_Truncates(t *testing.T) { for i := 0; i < 10; i++ { writeTestFile(t, filepath.Join(root, "Author", string(rune('a'+i))+".epub")) } - units, truncated := enumerateImportUnits(root, 4) + units, truncated := enumerateImportUnits(root, 4, nil) if !truncated { t.Errorf("expected truncated=true when units exceed the limit") } @@ -174,6 +174,32 @@ func TestEnumerateImportUnits_Truncates(t *testing.T) { } } +// TestEnumerateImportUnits_SkipDoesNotCountTowardLimitOrTruncation verifies a +// unit skip rejects is neither collected nor counted toward truncation — the +// walk keeps descending in search of `limit` units skip accepts (#2480: a +// large folder of already-tracked files must not starve the cap before any +// new file is even considered). +func TestEnumerateImportUnits_SkipDoesNotCountTowardLimitOrTruncation(t *testing.T) { + t.Parallel() + root := t.TempDir() + for i := 0; i < 20; i++ { + writeTestFile(t, filepath.Join(root, "Author", string(rune('a'+i))+".epub")) + } + writeTestFile(t, filepath.Join(root, "Author", "new.epub")) + + skipAllButNew := func(path string, isDir bool) bool { + return filepath.Base(path) != "new.epub" + } + units, truncated := enumerateImportUnits(root, 4, skipAllButNew) + if truncated { + t.Errorf("expected truncated=false: only one unit passes skip, well under the limit") + } + got := unitNames(units) + if len(got) != 1 || got[0] != "new.epub" { + t.Errorf("units = %v, want just [new.epub]", got) + } +} + // TestEnumerateImportUnits_UnreadableDirSkipped confirms an unreadable // subdirectory is skipped rather than aborting the whole walk. func TestEnumerateImportUnits_UnreadableDirSkipped(t *testing.T) { @@ -192,7 +218,7 @@ func TestEnumerateImportUnits_UnreadableDirSkipped(t *testing.T) { } t.Cleanup(func() { _ = os.Chmod(locked, 0o755) }) - units, _ := enumerateImportUnits(root, 1000) + units, _ := enumerateImportUnits(root, 1000, nil) // The readable book is still enumerated; the locked dir is silently skipped. got := unitNames(units) if len(got) != 1 || got[0] != "book.epub" { diff --git a/internal/db/book_files.go b/internal/db/book_files.go index 00a078cc4..4516f79ca 100644 --- a/internal/db/book_files.go +++ b/internal/db/book_files.go @@ -4,6 +4,9 @@ import ( "context" "database/sql" "fmt" + "strings" + "sync" + "sync/atomic" "time" "github.com/vavallee/bindery/internal/models" @@ -19,6 +22,53 @@ func NewBookFileRepo(db *sql.DB) *BookFileRepo { return &BookFileRepo{db: db} } +// Fingerprint reads a cheap snapshot of book_files — its row count and its +// highest id — for a cache keyed on the table's actual contents rather than a +// counter this repo maintains itself (the manual-import scan's tracked-file +// index, #2480). +// +// An earlier version of that cache was keyed on an atomic counter bumped by +// this repo's own mutating methods, which went stale: book_files rows also +// disappear through the books(id) ON DELETE CASCADE FK when a book or author +// is deleted (BookRepo.Delete, AuthorRepo.Delete), and through +// BookRepo.UntrackFilePath's rollback DELETE — neither goes through this repo, +// so neither could bump its counter. Reading count+maxID directly from the +// table instead catches every mutation regardless of which code path made it: +// a row removed without a matching insert changes the count, and any insert +// hands out a strictly larger AUTOINCREMENT id, so the pair only repeats when +// nothing actually changed. The query is a single indexed aggregate, cheap +// enough to run on every scan request. +func (r *BookFileRepo) Fingerprint(ctx context.Context) (count int64, maxID int64, err error) { + err = r.db.QueryRowContext(ctx, `SELECT COUNT(*), COALESCE(MAX(id), 0) FROM book_files`).Scan(&count, &maxID) + if err != nil { + return 0, 0, fmt.Errorf("book_files fingerprint: %w", err) + } + return count, maxID, nil +} + +// pathEpochs holds one counter per database, bumped by every in place rewrite +// of book_files.path (UpdatePath). Fingerprint's (count, maxID) pair catches +// inserts and deletes however they happen, including the FK cascade, but an +// UPDATE changes neither number, so the manual-import scan's tracked-file +// cache (#2480) keys on this as well. It is keyed by *sql.DB rather than held +// on the repo because production builds more than one BookFileRepo over the +// same database (BookRepo owns one, the importer wiring another), and the +// rename has to be visible to every reader regardless of which one wrote it. +var pathEpochs sync.Map // *sql.DB -> *atomic.Uint64 + +func pathEpochFor(db *sql.DB) *atomic.Uint64 { + v, _ := pathEpochs.LoadOrStore(db, new(atomic.Uint64)) + return v.(*atomic.Uint64) +} + +// PathEpoch returns the in place path rewrite counter for this database. Read +// it before Fingerprint and the rows a cache is built from: a rename that +// lands mid rebuild then shows up as a changed epoch on the next read, so the +// cache errs toward rebuilding, never toward serving a stale path. +func (r *BookFileRepo) PathEpoch() uint64 { + return pathEpochFor(r.db).Load() +} + // Add inserts a book_files row. Duplicate paths are silently ignored (INSERT OR IGNORE). func (r *BookFileRepo) Add(ctx context.Context, bookID int64, format, path string) error { _, err := r.db.ExecContext(ctx, @@ -72,6 +122,7 @@ func (r *BookFileRepo) UpdatePath(ctx context.Context, id int64, newPath string) if n == 0 { return fmt.Errorf("book_files update path: no row with id %d", id) } + pathEpochFor(r.db).Add(1) return nil } @@ -113,6 +164,40 @@ func (r *BookFileRepo) ListByBook(ctx context.Context, bookID int64) ([]models.B return files, rows.Err() } +// ListByBooks returns every book_files row for any of the given book IDs, in +// one query, grouped by book_id. Used by the manual-import scan +// (ManualImportHandler.Scan, #2480) to replace an N+1 ListByBook call per +// confident catalogue match with a single round trip. Duplicate IDs collapse +// naturally (IN ignores repeats); an empty bookIDs returns an empty map. +func (r *BookFileRepo) ListByBooks(ctx context.Context, bookIDs []int64) (map[int64][]models.BookFile, error) { + result := make(map[int64][]models.BookFile, len(bookIDs)) + if len(bookIDs) == 0 { + return result, nil + } + placeholders := make([]string, len(bookIDs)) + args := make([]any, len(bookIDs)) + for i, id := range bookIDs { + placeholders[i] = "?" + args[i] = id + } + query := `SELECT id, book_id, format, path, size_bytes, created_at + FROM book_files WHERE book_id IN (` + strings.Join(placeholders, ",") + `) ORDER BY id` // #nosec G202 -- placeholders are generated from fixed ? tokens; book IDs remain bound args + rows, err := r.db.QueryContext(ctx, query, args...) + if err != nil { + return nil, fmt.Errorf("book_files list by books: %w", err) + } + defer rows.Close() + + for rows.Next() { + var f models.BookFile + if err := rows.Scan(&f.ID, &f.BookID, &f.Format, &f.Path, &f.SizeBytes, &f.CreatedAt); err != nil { + return nil, fmt.Errorf("book_files scan: %w", err) + } + result[f.BookID] = append(result[f.BookID], f) + } + return result, rows.Err() +} + // DeleteByBook removes all book_files rows for the given book. func (r *BookFileRepo) DeleteByBook(ctx context.Context, bookID int64) error { _, err := r.db.ExecContext(ctx, `DELETE FROM book_files WHERE book_id = ?`, bookID) diff --git a/internal/db/book_files_test.go b/internal/db/book_files_test.go index 416eb9d93..e23d858f0 100644 --- a/internal/db/book_files_test.go +++ b/internal/db/book_files_test.go @@ -253,6 +253,133 @@ func TestBookRepo_MigrationBackfill(t *testing.T) { } } +// TestBookFileRepo_ListByBooks verifies the batch lookup groups rows by +// book_id in one query (#2480: replaces an N+1 ListByBook-per-book call in +// the manual-import scan's confident-match format check). +func TestBookFileRepo_ListByBooks(t *testing.T) { + database, author, book := openTestDB(t) + ctx := context.Background() + repo := NewBookRepo(database) + files := NewBookFileRepo(database) + + _ = repo.AddBookFile(ctx, book.ID, models.MediaTypeEbook, "/lib/one.epub") + + b2 := &models.Book{ + ForeignID: "OL-LB-B2", AuthorID: author.ID, + Title: "Second Book", SortTitle: "Second Book", + Status: models.BookStatusWanted, Monitored: true, + } + if err := repo.Create(ctx, b2); err != nil { + t.Fatalf("create b2: %v", err) + } + _ = repo.AddBookFile(ctx, b2.ID, models.MediaTypeEbook, "/lib/two.epub") + _ = repo.AddBookFile(ctx, b2.ID, models.MediaTypeAudiobook, "/lib/two.m4b") + + // A third book with no files at all should simply be absent from the map. + b3 := &models.Book{ + ForeignID: "OL-LB-B3", AuthorID: author.ID, + Title: "Third Book", SortTitle: "Third Book", + Status: models.BookStatusWanted, Monitored: true, + } + if err := repo.Create(ctx, b3); err != nil { + t.Fatalf("create b3: %v", err) + } + + got, err := files.ListByBooks(ctx, []int64{book.ID, b2.ID, b3.ID}) + if err != nil { + t.Fatalf("ListByBooks: %v", err) + } + if len(got[book.ID]) != 1 || got[book.ID][0].Path != "/lib/one.epub" { + t.Errorf("book1 files = %+v, want just one.epub", got[book.ID]) + } + if len(got[b2.ID]) != 2 { + t.Errorf("book2 files = %+v, want 2 rows", got[b2.ID]) + } + if _, ok := got[b3.ID]; ok { + t.Errorf("book3 has no files and should be absent from the map, got %+v", got[b3.ID]) + } + + empty, err := files.ListByBooks(ctx, nil) + if err != nil { + t.Fatalf("ListByBooks(nil): %v", err) + } + if len(empty) != 0 { + t.Errorf("ListByBooks(nil) = %+v, want empty map", empty) + } +} + +// TestBookFileRepo_Fingerprint verifies the (count, maxID) snapshot changes on +// every mutating method, is stable across pure reads, and — the point of +// reading it from the table rather than an in-process counter (#2480 review) +// — also changes when a row disappears through a path that never goes through +// BookFileRepo at all, such as the books(id) ON DELETE CASCADE FK a book +// delete triggers. +func TestBookFileRepo_Fingerprint(t *testing.T) { + database, _, book := openTestDB(t) + ctx := context.Background() + files := NewBookFileRepo(database) + books := NewBookRepo(database) + + count0, maxID0, err := files.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + + if err := files.Add(ctx, book.ID, models.MediaTypeEbook, "/lib/v.epub"); err != nil { + t.Fatalf("Add: %v", err) + } + count1, maxID1, err := files.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + if count1 == count0 && maxID1 == maxID0 { + t.Errorf("Fingerprint did not change after Add: (%d, %d)", count1, maxID1) + } + + if _, err := files.ListByBook(ctx, book.ID); err != nil { + t.Fatalf("ListByBook: %v", err) + } + if count2, maxID2, err := files.Fingerprint(ctx); err != nil { + t.Fatalf("Fingerprint: %v", err) + } else if count2 != count1 || maxID2 != maxID1 { + t.Errorf("Fingerprint changed on a pure read: (%d, %d) -> (%d, %d)", count1, maxID1, count2, maxID2) + } + + if _, err := files.DeleteByPath(ctx, "/lib/v.epub"); err != nil { + t.Fatalf("DeleteByPath: %v", err) + } + count3, maxID3, err := files.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + if count3 == count1 && maxID3 == maxID1 { + t.Errorf("Fingerprint did not change after DeleteByPath: (%d, %d)", count3, maxID3) + } + + // A book delete removes book_files rows via ON DELETE CASCADE — not + // through any BookFileRepo method — which is exactly the path an + // in-process mutation counter cannot see (#2480 review repro: seed a + // tracked file, delete the book keeping the file, and the cache stayed + // stale). Reading the fingerprint from the table itself must still catch it. + if err := files.Add(ctx, book.ID, models.MediaTypeEbook, "/lib/v2.epub"); err != nil { + t.Fatalf("Add: %v", err) + } + count4, maxID4, err := files.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + if err := books.Delete(ctx, book.ID); err != nil { + t.Fatalf("Delete book: %v", err) + } + count5, maxID5, err := files.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + if count5 == count4 && maxID5 == maxID4 { + t.Errorf("Fingerprint did not change after the owning book was deleted (FK cascade): (%d, %d)", count5, maxID5) + } +} + // TestBookRepo_RemoveBookFile_StatusFlips verifies removing the last file // flips the book back to "wanted". func TestBookRepo_RemoveBookFile_StatusFlips(t *testing.T) { @@ -279,3 +406,51 @@ func TestBookRepo_RemoveBookFile_StatusFlips(t *testing.T) { t.Errorf("EbookFilePath should be cleared, got %q", got2.EbookFilePath) } } + +// TestBookFileRepo_PathEpoch pins the half of the #2480 cache key that +// Fingerprint cannot provide: UpdatePath rewrites a row in place, leaving +// (count, maxID) untouched, so it must bump the epoch instead. The epoch is +// per database, not per repo, because reorganize and the manual-import scan +// reach book_files through different repo instances; and a failed UpdatePath +// changes nothing, so it must not force a rebuild. +func TestBookFileRepo_PathEpoch(t *testing.T) { + database, _, book := openTestDB(t) + ctx := context.Background() + writer := NewBookFileRepo(database) + reader := NewBookRepo(database) + + if err := writer.Add(ctx, book.ID, models.MediaTypeEbook, "/lib/old.epub"); err != nil { + t.Fatalf("Add: %v", err) + } + rows, err := writer.ListByBook(ctx, book.ID) + if err != nil || len(rows) != 1 { + t.Fatalf("ListByBook: %v (%d rows)", err, len(rows)) + } + count0, maxID0, err := writer.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + epoch0 := reader.BookFilesPathEpoch() + + if err := writer.UpdatePath(ctx, rows[0].ID, "/lib/new.epub"); err != nil { + t.Fatalf("UpdatePath: %v", err) + } + count1, maxID1, err := writer.Fingerprint(ctx) + if err != nil { + t.Fatalf("Fingerprint: %v", err) + } + if count1 != count0 || maxID1 != maxID0 { + t.Fatalf("premise: UpdatePath changed the fingerprint (%d,%d) -> (%d,%d); the epoch would be redundant", count0, maxID0, count1, maxID1) + } + epoch1 := reader.BookFilesPathEpoch() + if epoch1 == epoch0 { + t.Fatalf("PathEpoch did not change after UpdatePath through another repo instance (still %d)", epoch1) + } + + if err := writer.UpdatePath(ctx, rows[0].ID+999, "/lib/nowhere.epub"); err == nil { + t.Fatal("UpdatePath on a missing row: want error") + } + if got := reader.BookFilesPathEpoch(); got != epoch1 { + t.Errorf("PathEpoch moved on a failed UpdatePath: %d -> %d", epoch1, got) + } +} diff --git a/internal/db/books.go b/internal/db/books.go index 3b66f20c2..9d9f5d136 100644 --- a/internal/db/books.go +++ b/internal/db/books.go @@ -900,6 +900,29 @@ func (r *BookRepo) ListAllBookFilePaths(ctx context.Context) ([]string, error) { return r.files.ListAllPaths(ctx) } +// BookFilesFingerprint returns a (count, maxID) snapshot read directly from +// book_files, so a cache derived from it (the manual-import scan's +// tracked-file index, #2480) can tell whether it needs to rebuild. See +// BookFileRepo.Fingerprint for why this reads the table instead of an +// in-process counter. +func (r *BookRepo) BookFilesFingerprint(ctx context.Context) (int64, int64, error) { + return r.files.Fingerprint(ctx) +} + +// BookFilesPathEpoch returns the in place path rewrite counter for book_files +// (see BookFileRepo.PathEpoch). Fingerprint alone misses the reorganize +// action's UpdatePath, which changes neither the row count nor the max id. +func (r *BookRepo) BookFilesPathEpoch() uint64 { + return r.files.PathEpoch() +} + +// ListFilesForBooks returns every book_files row for the given book IDs in a +// single query, grouped by book_id. Replaces an N+1 ListFiles-per-book call +// (the manual-import scan's confident-match format check, #2480). +func (r *BookRepo) ListFilesForBooks(ctx context.Context, bookIDs []int64) (map[int64][]models.BookFile, error) { + return r.files.ListByBooks(ctx, bookIDs) +} + // ListBookFiles returns the book_files rows for a single book. func (r *BookRepo) ListBookFiles(ctx context.Context, bookID int64) ([]models.BookFile, error) { return r.files.ListByBook(ctx, bookID) diff --git a/internal/duplicates/duplicates.go b/internal/duplicates/duplicates.go new file mode 100644 index 000000000..c36fbb6f9 --- /dev/null +++ b/internal/duplicates/duplicates.go @@ -0,0 +1,547 @@ +// Package duplicates implements the read-only duplicate-title detection pass +// requested in #1970: a separate, deliberately permissive layer that finds +// book rows a human would recognize as the same work even when +// indexer.CanonicalDedupKey does not collapse them (a differently-phrased +// title variant, a subtitle that doesn't follow the ": subtitle" pattern, two +// OpenLibrary works that are really one book). +// +// # Boundary — read this before touching anything in this package +// +// This package is DETECTION ONLY. It never decides whether an incoming record +// is a new book, never populates books.dedup_key, and is never called from an +// ingestion path. The exact-key dedup machinery in internal/indexer +// (CanonicalDedupKey, NormalizeTitleForDedup, CompareTitles) stays the single +// authoritative identity function for book creation (#940, #2042); loosening +// THAT to catch more duplicates would trade missed-duplicate noise for +// silent false merges, which loses data and is the worse failure mode. +// +// The trade this package makes is the deliberate one from #1970: its +// normalization and rules are more aggressive than the ingestion key, in +// exchange for the fact that its output is only ever SHOWN to a person, who +// confirms before anything is excluded. Nothing in this package writes. +// +// The import boundary is enforced by TestIngestionPackagesNeverImportThis in +// imports_test.go: no package outside internal/api may import this one. +package duplicates + +import ( + "regexp" + "sort" + "strings" + + "github.com/vavallee/bindery/internal/models" + "github.com/vavallee/bindery/internal/textutil" +) + +// RuleID names one detection rule. The values are stable identifiers the API +// returns and the UI translates, so renaming one is a user-visible breaking +// change — add new rules, don't rename old ones. +type RuleID string + +const ( + // RuleAlnumEqual: the aggressive keys are equal. The core case — the same + // work whose title differs only in punctuation, case, Unicode form, + // umlaut representation, or apostrophe style. + RuleAlnumEqual RuleID = "alnum-equal" + // RuleArticleStrip: the keys differ, but become equal once a leading + // article is dropped from each side ("Martian" vs "The Martian"). + RuleArticleStrip RuleID = "article-strip" + // RuleEditionSuffix: the keys differ, but become equal once a trailing + // edition qualifier ("Unabridged", "Expanded Edition", ...) is dropped + // from each side. + RuleEditionSuffix RuleID = "edition-suffix" + // RuleSubstring: one title is a whole separator-delimited part of the + // other, the one-sided subtitle case ("Mistborn" vs "Mistborn: The Final + // Empire"). The API value keeps its original name; see substringMatch. + // This is the rule with the highest false-positive rate (a novella whose + // title is near-embedded in the parent novel's); it exists because a + // human confirms before anything happens. + RuleSubstring RuleID = "substring" +) + +// Guard thresholds for the substring rule. Both apply: the shorter key must +// be at least minSubstringKeyLen characters (so "Go" never matches "Gone"), +// and the longer key must carry at least minResidualLen characters BEYOND the +// embedded one (so "The Martian" never matches "The Martian Child", where the +// residual "child" is a different book, not a subtitle). +// +// On top of the length guards the containment must sit on a separator +// boundary (see titleSegments): the shorter title has to be a whole part of +// the longer one as the longer one is punctuated. A bare substring grouped a +// series opener with every sequel carrying its name ("Foundation" with +// "Prelude to Foundation", "The Science of Discworld" with "The Science of +// Discworld II: The Globe", "Carrie" with "Carrie Soto Is Back") whenever the +// catalogue had no series links to suppress it with. +const ( + minSubstringKeyLen = 6 + minResidualLen = 6 +) + +// maxPairwiseBooks gates the substring rule. The pairwise loop always runs +// its full O(n²) sweep, but every pair check is a cheap string comparison +// except substring, which scans; above this catalogue size the scan is +// skipped, so a pathological catalogue stays fast — correct, just less +// aggressive. +const maxPairwiseBooks = 2000 + +// SeriesSlot is the minimal series-membership fact Scan needs to suppress a +// substring false positive between two genuinely different books in the same +// series (#1970 review: "Foundation" grouping with "Foundation and Empire"). +// Two different positions in one series are, by definition, different works, +// even when one title is a prefix of the other. Position is compared as an +// opaque string ("1" and "1.5" differ, correctly); an empty Position is +// unknown and never suppresses anything, so a catalogue with no series data +// keeps today's behaviour. +type SeriesSlot struct { + SeriesID int64 + Position string +} + +// sameSeriesDifferentPosition reports whether a and b share a series but hold +// different known positions in it. Only a resolved Position on both sides +// counts — an unresolved one (no position recorded) never suppresses, because +// "unknown" must not be read as "different". +func sameSeriesDifferentPosition(a, b []SeriesSlot) bool { + for _, sa := range a { + if sa.Position == "" { + continue + } + for _, sb := range b { + if sb.SeriesID == sa.SeriesID && sb.Position != "" && sb.Position != sa.Position { + return true + } + } + } + return false +} + +// aggressiveFold reduces a title to the shared comparison alphabet — NFC, +// lowercase, apostrophes deleted, umlauts expanded, everything else a space +// (textutil.FoldForTitleMatch, the alphabet every title comparison in the +// repo uses) — with one addition: '&' expands to "and" rather than folding to +// a space. That is the same spelling decision the dedup path makes in +// foldPunctuation ("Foundation & Empire" and "Foundation and Empire" are one +// book), applied here independently so this package never calls a dedup +// function. +// +// The folded result then goes through textutil.FoldForSlug, which drops the +// remaining Latin and Greek diacritics, so "Les Misérables" and "Les +// Miserables" share a key. Umlauts are expanded first ("ü" to "ue"), so a +// German title keeps matching its transliterated spelling. +func aggressiveFold(title string) string { + return textutil.FoldForSlug(textutil.FoldForTitleMatch(strings.ReplaceAll(title, "&", " and "))) +} + +// foldWords is aggressiveFold tokenized into lowercase alnum words. +func foldWords(title string) []string { + return strings.Fields(aggressiveFold(title)) +} + +// AggressiveTitleKey is the detection key: the title folded to alphanumerics +// only, with no spaces. It is LOSSY by design — it exists to be permissive, +// and its output is compared for equality and substring, never stored as an +// identity and never written to books.dedup_key. +// +// It is intentionally NOT the same function as indexer.CanonicalDedupKey: +// the dedup key is lossless and authoritative, this one is aggressive and +// advisory. Keeping them separate functions is the whole point of #1970's +// "separate, read-only detection pass". +func AggressiveTitleKey(title string) string { + return strings.Join(foldWords(title), "") +} + +// leadingArticles are the whole-word leading articles dropped by the +// article-strip rule. Deliberately a closed list, matched as a complete first +// word: "Theatre of Blood" must not collapse because "Theatre" starts with +// "the". +var leadingArticles = map[string]struct{}{ + // English + "the": {}, "a": {}, "an": {}, + // German + "der": {}, "die": {}, "das": {}, + // French / Spanish / Portuguese + "le": {}, "la": {}, "les": {}, "el": {}, "los": {}, "las": {}, "de": {}, + // Italian + "il": {}, "gli": {}, "una": {}, + // Dutch + "een": {}, "het": {}, +} + +// dropLeadingArticle removes the first word when it is a whole-word leading +// article. The len > 1 guard keeps a title that IS an article ("A", "Die") +// from keying to empty — an empty key must never match anything. +func dropLeadingArticle(words []string) []string { + if len(words) > 1 { + if _, ok := leadingArticles[words[0]]; ok { + return words[1:] + } + } + return words +} + +// ArticleKey is the aggressive key with any leading article dropped. +func ArticleKey(title string) string { + return strings.Join(dropLeadingArticle(foldWords(title)), "") +} + +// editionMarkers are the trailing qualifier sequences the edition-suffix rule +// drops, longest first (a two-word marker must win over the bare "edition"). +// Matching is strictly TRAILING: "Complete Works of Shakespeare" keeps its +// leading "complete", because that word is doing title work there. +var editionMarkers = [][]string{ + {"expanded", "edition"}, + {"anniversary", "edition"}, + {"deluxe", "edition"}, + {"definitive", "edition"}, + {"revised", "edition"}, + {"unabridged"}, + {"abridged"}, + {"audiobook"}, + {"dramatized"}, + {"dramatised"}, + {"illustrated"}, + {"complete"}, + {"edition"}, +} + +// dropTrailingEditionMarkers repeatedly strips a known edition qualifier from +// the end of the word list ("Title [Unabridged] [2021]" style stacking), +// stopping when nothing matches. The len > n guard keeps a title that IS a +// marker ("Unabridged") from keying to empty. +func dropTrailingEditionMarkers(words []string) []string { + for { + removed := false + for _, marker := range editionMarkers { + n := len(marker) + if len(words) > n && wordsEqual(words[len(words)-n:], marker) { + words = words[:len(words)-n] + removed = true + } + } + if !removed { + return words + } + } +} + +// EditionKey is the aggressive key with trailing edition qualifiers dropped. +func EditionKey(title string) string { + return strings.Join(dropTrailingEditionMarkers(foldWords(title)), "") +} + +func wordsEqual(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// segmentSplitter matches the punctuation that separates a title from its +// subtitle or a trailing qualifier: a colon or semicolon, brackets, a spaced +// hyphen, or an en or em dash. A bare hyphen inside a word ("Half-Blood") is +// not a separator. +var segmentSplitter = regexp.MustCompile(`[:;()\[\]{}]|\s-+\s|[\x{2013}\x{2014}]`) + +// titleSegments returns the alnum and article-stripped keys of every +// separator-delimited part of title, or nil when the title has only one part +// (then no part is a PROPER part, and the whole-title keys already cover it). +// "Mistborn: The Final Empire" yields mistborn and thefinalempire; +// "Hogfather (Discworld, #20)" yields hogfather and discworld20. +func titleSegments(title string) [][2]string { + parts := segmentSplitter.Split(title, -1) + var segs [][2]string + for _, part := range parts { + words := foldWords(part) + if len(words) == 0 { + continue + } + segs = append(segs, [2]string{ + strings.Join(words, ""), + strings.Join(dropLeadingArticle(words), ""), + }) + } + if len(segs) < 2 { + return nil + } + return segs +} + +// substringMatch reports whether the shorter title is a substantial, whole +// part of the longer one: its key is at least minSubstringKeyLen long, +// leaves at least minResidualLen characters of residual on the longer side, +// the residual is not made up solely of edition-marker words, and the +// shorter key equals one separator-delimited segment of the longer title +// (compared with and without a leading article). The marker guard kills the +// "Complete Works of X" vs "Works of X" false positive, where the residual +// "complete" is doing edition-marker work, not subtitle work. +func substringMatch(shorter, longer bookKeys) bool { + if len(shorter.alnum) < minSubstringKeyLen { + return false + } + idx := strings.Index(longer.alnum, shorter.alnum) + if idx < 0 { + return false + } + if len(longer.alnum)-len(shorter.alnum) < minResidualLen { + return false + } + residual := longer.alnum[:idx] + longer.alnum[idx+len(shorter.alnum):] + if residualIsMarkerOnly(residual) { + return false + } + for _, seg := range longer.segments { + if seg[0] == shorter.alnum || (seg[1] != "" && seg[1] == shorter.article) { + return true + } + } + return false +} + +// markerWords is the flat set of every word that appears in editionMarkers, +// used by residualIsMarkerOnly. +var markerWords = map[string]struct{}{ + "expanded": {}, "anniversary": {}, "deluxe": {}, "definitive": {}, "revised": {}, + "edition": {}, "unabridged": {}, "abridged": {}, "audiobook": {}, + "dramatized": {}, "dramatised": {}, "illustrated": {}, "complete": {}, +} + +func residualIsMarkerOnly(residual string) bool { + if residual == "" { + return false + } + for _, w := range strings.Fields(residual) { + if _, ok := markerWords[w]; !ok { + return false + } + } + return true +} + +// MatchRules evaluates every rule independently against a pair of titles and +// returns all that fire, in canonical order. A blank key on either side never +// matches: an untitled row must not collapse onto every other untitled row +// (the same invariant CompareTitles enforces for the exact key). +// +// One composition case: when neither transformation alone equalizes the keys +// but BOTH together do ("Hobbit Unabridged" vs "The Hobbit"), both +// RuleArticleStrip and RuleEditionSuffix are reported — the pair genuinely +// needed both rewrites, and the UI's explanation is "differs only in a +// leading article and an edition qualifier". +func MatchRules(a, b string) []RuleID { + ka, kb := AggressiveTitleKey(a), AggressiveTitleKey(b) + if ka == "" || kb == "" { + return nil + } + return pairRules(bookKeyFor(a), bookKeyFor(b), true) +} + +// Member is one book in a candidate group, annotated with the rules that +// fired for it against the other members. +type Member struct { + models.Book + Rules []RuleID `json:"rules"` +} + +// Group is a set of books the detector believes may be the same work. A group +// with a single member is never returned. Rules is the union of every rule +// that fired on any pair within the group, so the UI can explain the group as +// a whole. +type Group struct { + Key string `json:"key"` + Rules []RuleID `json:"rules"` + Members []Member `json:"books"` +} + +type bookKeys struct { + alnum string + article string + edition string + combined string + // segments are the keys of each separator-delimited part of the title, + // nil for a title with one part. Only the substring rule reads them. + segments [][2]string +} + +// bookKeyFor precomputes every key form for one title. Scan calls it once per +// book so the pairwise pass compares precomputed strings. +func bookKeyFor(title string) bookKeys { + words := foldWords(title) + return bookKeys{ + alnum: strings.Join(words, ""), + article: strings.Join(dropLeadingArticle(words), ""), + edition: strings.Join(dropTrailingEditionMarkers(words), ""), + combined: strings.Join(dropTrailingEditionMarkers(dropLeadingArticle(words)), ""), + segments: titleSegments(title), + } +} + +// Scan groups the books into duplicate candidate groups. It is a pure +// function: no I/O, no DB, and it deliberately does not look at the +// Excluded flag — exclusion policy (which groups are worth showing, which +// members count as active) belongs to the caller, which is the only layer +// that can act on the result. +// +// seriesSlots is an optional (nil is fine) lookup from book ID to that book's +// series memberships, used solely to suppress the substring rule for two +// books that are different, known positions in the same series (see +// SeriesSlot). The caller collects it because Scan itself does no I/O. +// +// Algorithm: keys are precomputed per book; every pair is evaluated (full +// O(n²) sweep, substring rule gated by maxPairwiseBooks); a substring-only +// match between two different series positions is discarded; pairs with at +// least one surviving rule are unioned; the connected components are the +// groups. Output is deterministic: groups sorted by key, members by book ID. +func Scan(books []models.Book, seriesSlots map[int64][]SeriesSlot) []Group { + n := len(books) + keys := make([]bookKeys, n) + for i, b := range books { + keys[i] = bookKeyFor(b.Title) + } + + parent := make([]int, n) + for i := range parent { + parent[i] = i + } + var find func(int) int + find = func(x int) int { + if parent[x] != x { + parent[x] = find(parent[x]) + } + return parent[x] + } + union := func(a, b int) { + ra, rb := find(a), find(b) + if ra != rb { + parent[ra] = rb + } + } + + rulesByBook := make([]map[RuleID]struct{}, n) + for i := range rulesByBook { + rulesByBook[i] = make(map[RuleID]struct{}) + } + useSubstring := n <= maxPairwiseBooks + + for i := 0; i < n; i++ { + if keys[i].alnum == "" { + continue + } + for j := i + 1; j < n; j++ { + if keys[j].alnum == "" { + continue + } + rules := pairRules(keys[i], keys[j], useSubstring) + if containsRule(rules, RuleSubstring) && + sameSeriesDifferentPosition(seriesSlots[books[i].ID], seriesSlots[books[j].ID]) { + rules = removeRule(rules, RuleSubstring) + } + if len(rules) == 0 { + continue + } + union(i, j) + for _, rule := range rules { + addRule(rulesByBook[i], rule) + addRule(rulesByBook[j], rule) + } + } + } + + byRoot := make(map[int][]int) + for i := 0; i < n; i++ { + root := find(i) + byRoot[root] = append(byRoot[root], i) + } + + groups := make([]Group, 0, len(byRoot)) + for _, idxs := range byRoot { + if len(idxs) < 2 { + continue + } + sort.Slice(idxs, func(a, b int) bool { return books[idxs[a]].ID < books[idxs[b]].ID }) + group := Group{ + Key: keys[idxs[0]].alnum, + Members: make([]Member, 0, len(idxs)), + } + groupRuleSet := make(map[RuleID]struct{}) + for _, idx := range idxs { + m := Member{Book: books[idx]} + for rule := range rulesByBook[idx] { + m.Rules = append(m.Rules, rule) + groupRuleSet[rule] = struct{}{} + } + sort.Slice(m.Rules, func(a, b int) bool { return m.Rules[a] < m.Rules[b] }) + group.Members = append(group.Members, m) + } + for rule := range groupRuleSet { + group.Rules = append(group.Rules, rule) + } + sort.Slice(group.Rules, func(a, b int) bool { return group.Rules[a] < group.Rules[b] }) + groups = append(groups, group) + } + sort.Slice(groups, func(a, b int) bool { return groups[a].Key < groups[b].Key }) + return groups +} + +// pairRules evaluates the rules for a pair using precomputed keys. The +// alnum-equal check is a bucket comparison; article and edition are key +// comparisons; the combined key catches pairs that need both rewrites; +// substring is the guarded containment check. +func pairRules(a, b bookKeys, useSubstring bool) []RuleID { + var rules []RuleID + if a.alnum == b.alnum { + return []RuleID{RuleAlnumEqual} + } + articleEq := a.article != "" && b.article != "" && a.article == b.article + editionEq := a.edition != "" && b.edition != "" && a.edition == b.edition + if articleEq { + rules = append(rules, RuleArticleStrip) + } + if editionEq { + rules = append(rules, RuleEditionSuffix) + } + if !articleEq && !editionEq && + a.combined != "" && b.combined != "" && a.combined == b.combined { + rules = append(rules, RuleArticleStrip, RuleEditionSuffix) + } + if useSubstring { + if len(a.alnum) > len(b.alnum) { + if substringMatch(b, a) { + rules = append(rules, RuleSubstring) + } + } else if substringMatch(a, b) { + rules = append(rules, RuleSubstring) + } + } + return rules +} + +func addRule(set map[RuleID]struct{}, rule RuleID) { + set[rule] = struct{}{} +} + +func containsRule(rules []RuleID, target RuleID) bool { + for _, r := range rules { + if r == target { + return true + } + } + return false +} + +// removeRule drops one rule from a freshly-built pairRules result. Safe to +// mutate in place: rules is always a slice pairRules allocated for this call, +// never one shared with a caller. +func removeRule(rules []RuleID, target RuleID) []RuleID { + out := rules[:0] + for _, r := range rules { + if r != target { + out = append(out, r) + } + } + return out +} diff --git a/internal/duplicates/duplicates_test.go b/internal/duplicates/duplicates_test.go new file mode 100644 index 000000000..85e35f9b0 --- /dev/null +++ b/internal/duplicates/duplicates_test.go @@ -0,0 +1,590 @@ +package duplicates + +import ( + "fmt" + "strings" + "testing" + "time" + + "github.com/vavallee/bindery/internal/models" +) + +func book(title string) models.Book { + return models.Book{Title: title} +} + +// TestAggressiveTitleKey covers the normalizer contract: everything a human +// would treat as "the same spelling" collapses, and nothing more is lost than +// punctuation, case, Unicode form, and the '&' spelling decision. +func TestAggressiveTitleKey(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"plain", "The Martian", "themartian"}, + {"case-insensitive", "THE MARTIAN", "themartian"}, + {"punctuation-drops", "Mistborn: The Final Empire", "mistbornthefinalempire"}, + {"punctuation-dashes", "Mistborn - The Final Empire", "mistbornthefinalempire"}, + {"ampersand-expands", "Foundation & Empire", "foundationandempire"}, + {"ampersand-vs-and", "Foundation and Empire", "foundationandempire"}, + {"apostrophe-deleted", "The Cat's Meow", "thecatsmeow"}, + {"umlaut-expanded", "Grüße aus dem Weltall", "gruesseausdemweltall"}, + {"digits-kept", "Star Wars 7", "starwars7"}, + {"trailing-punctuation", "Dune, Part Two!", "duneparttwo"}, + {"blank", "", ""}, + {"only-punctuation", "!!! ???", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := AggressiveTitleKey(tc.in); got != tc.want { + t.Errorf("AggressiveTitleKey(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +// TestAggressiveTitleKeyNFC pins the #1642 failure mode at the normalizer: +// decomposed (NFD) input must fold to the same key as composed (NFC) input, +// or the same title meets itself as two strings. +func TestAggressiveTitleKeyNFC(t *testing.T) { + // "Café" composed (é is U+00E9) vs decomposed (e + U+0301 combining acute). + composed := "Café Society" + decomposed := "Caf\u0065\u0301 Society" + gotC := AggressiveTitleKey(composed) + gotD := AggressiveTitleKey(decomposed) + if gotC != gotD { + t.Errorf("NFC %q -> %q but NFD %q -> %q; keys must match", composed, gotC, decomposed, gotD) + } + // Latin diacritics are dropped after the umlaut expansion, so both forms + // also meet the unaccented spelling a provider may carry. + if gotC != "cafesociety" { + t.Errorf("AggressiveTitleKey(%q) = %q, want cafesociety", composed, gotC) + } +} + +// TestAggressiveTitleKeyFoldsDiacritics pins that an accent-only difference +// lands in one group (alnum-equal), which is what the UI's rule text says, +// while umlauts still expand rather than strip. +func TestAggressiveTitleKeyFoldsDiacritics(t *testing.T) { + pairs := [][2]string{ + {"Les Misérables", "Les Miserables"}, + {"Cien años de soledad", "Cien anos de soledad"}, + {"Échec et mort", "Echec et mort"}, + {"Grüße", "Gruesse"}, + } + for _, p := range pairs { + if got := MatchRules(p[0], p[1]); !rulesContain(got, RuleAlnumEqual) { + t.Errorf("MatchRules(%q, %q) = %v, want alnum-equal", p[0], p[1], got) + } + } +} + +// TestArticleKey pins the article-strip semantics, including the whole-word +// guard that keeps "Theatre of Blood" from collapsing because "Theatre" +// merely begins with "the". +func TestArticleKey(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"leading-the", "The Martian", "martian"}, + {"leading-a", "A Game of Thrones", "gameofthrones"}, + {"leading-an", "An Ubik", "ubik"}, + {"no-article", "Martian", "martian"}, + {"theatre-trap", "Theatre of Blood", "theatreofblood"}, + {"there-trap", "There Will Come Soft Rains", "therewillcomesoftrains"}, + {"german-der", "Der Name des Windes", "namedeswindes"}, + {"french-le", "Le Petit Prince", "petitprince"}, + {"spanish-el", "El Nombre del Viento", "nombredelviento"}, + {"dutch-het", "Het Getuigenis", "getuigenis"}, + {"single-article-survives", "A", "a"}, + {"single-article-die", "Die", "die"}, + {"internal-untouched", "The Eye of the World", "eyeoftheworld"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := ArticleKey(tc.in); got != tc.want { + t.Errorf("ArticleKey(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestEditionKey(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"unabridged-trailing", "The Hobbit (Unabridged)", "thehobbit"}, + {"two-word-marker", "Foundation Expanded Edition", "foundation"}, + {"anniversary", "The Name of the Wind Anniversary Edition", "thenameofthewind"}, + {"stacked-markers", "Dramatised Unabridged Edition", "dramatised"}, // markers stripped until the guard keeps one word + {"leading-complete-kept", "Complete Works of Shakespeare", "completeworksofshakespeare"}, + {"trailing-complete", "Shakespeare Complete", "shakespeare"}, + {"no-marker", "The Hobbit", "thehobbit"}, + {"single-marker-survives", "Unabridged", "unabridged"}, + {"internal-kept", "The Complete Guide to Dune", "thecompleteguidetodune"}, + {"dramatised-british", "Frankenstein Dramatised", "frankenstein"}, + {"dramatized-american", "Frankenstein Dramatized", "frankenstein"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := EditionKey(tc.in); got != tc.want { + t.Errorf("EditionKey(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} + +func TestMatchRules(t *testing.T) { + cases := []struct { + name string + a, b string + want []RuleID + match bool + }{ + { + name: "punctuation-variant", + a: "Mistborn: The Final Empire", + b: "Mistborn - The Final Empire", + want: []RuleID{RuleAlnumEqual}, + match: true, + }, + { + name: "article-strip-only", + a: "Martian", + b: "The Martian", + want: []RuleID{RuleArticleStrip}, + match: true, + }, + { + name: "star-wars-trap", + a: "Star Wars", + b: "Star Wars 2", + match: false, + }, + { + name: "leading-complete-trap", + a: "Complete Works of Shakespeare", + b: "Works of Shakespeare", + match: false, + }, + { + name: "edition-suffix-only", + a: "The Hobbit (Unabridged)", + b: "The Hobbit", + want: []RuleID{RuleEditionSuffix}, + match: true, + }, + { + name: "article-plus-edition", + a: "Hobbit Unabridged", + b: "The Hobbit", + want: []RuleID{RuleArticleStrip, RuleEditionSuffix}, + match: true, + }, + { + name: "subtitle-embedded", + a: "Mistborn", + b: "Mistborn: The Final Empire", + want: []RuleID{RuleSubstring}, + match: true, + }, + { + name: "residual-guard-martian-child", + a: "The Martian", + b: "The Martian Child", + match: false, + }, + { + name: "short-key-guard", + a: "Go", + b: "Gone", + match: false, + }, + { + name: "accepted-fp-trilogy-member", + a: "The Lord of the Rings", + b: "The Lord of the Rings: The Return of the King", + want: []RuleID{RuleSubstring}, + match: true, + }, + { + name: "blank-never-matches", + a: "", + b: "", + match: false, + }, + { + name: "blank-vs-titled", + a: "", + b: "Dune", + match: false, + }, + { + name: "unrelated", + a: "Dune", + b: "Hyperion", + match: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := MatchRules(tc.a, tc.b) + if tc.match { + if len(got) == 0 { + t.Fatalf("MatchRules(%q, %q) = nil, want match %v", tc.a, tc.b, tc.want) + } + if !rulesEqual(got, tc.want) { + t.Errorf("MatchRules(%q, %q) = %v, want %v", tc.a, tc.b, got, tc.want) + } + } else if len(got) != 0 { + t.Errorf("MatchRules(%q, %q) = %v, want no match", tc.a, tc.b, got) + } + }) + } +} + +func rulesEqual(a, b []RuleID) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + +// TestScanGrouping pins the grouping semantics: same-key books merge, a +// single book is never a group, and output is deterministic. +func TestScanGrouping(t *testing.T) { + books := []models.Book{ + book("Dune"), + book("Dune"), + book("Hyperion"), + book("Dune, Part Two"), + book("Dune Part Two"), + } + // Give stable, ordered IDs. + for i := range books { + books[i].ID = int64(i + 1) + } + + groups := Scan(books, nil) + if len(groups) != 2 { + t.Fatalf("Scan = %d groups, want 2 (dune pair; dune part two pair)", len(groups)) + } + + // Deterministic order: groups sorted by key. + if groups[0].Key != "dune" || groups[1].Key != "duneparttwo" { + t.Fatalf("group order = %q, %q; want sorted by key", groups[0].Key, groups[1].Key) + } + + dune := groups[0] + if len(dune.Members) != 2 { + t.Fatalf("dune group has %d members, want 2", len(dune.Members)) + } + if dune.Members[0].ID != 1 || dune.Members[1].ID != 2 { + t.Errorf("members not sorted by ID: %d, %d", dune.Members[0].ID, dune.Members[1].ID) + } + if !rulesContain(dune.Rules, RuleAlnumEqual) { + t.Errorf("dune group rules = %v, want alnum-equal", dune.Rules) + } +} + +// TestScanTransitiveChain pins connected-component grouping: A~B on one rule +// and B~C on another put all three in one group whose Rules is the union. +func TestScanTransitiveChain(t *testing.T) { + books := []models.Book{ + book("Martian"), // A + book("The Martian"), // B — article-strip with A + book("The Martian: A Novel"), // C — substring with B + book("Martian Child"), // D — must NOT join (residual "child" < 6) + } + for i := range books { + books[i].ID = int64(i + 1) + } + + groups := Scan(books, nil) + if len(groups) != 1 { + t.Fatalf("Scan = %d groups, want 1", len(groups)) + } + g := groups[0] + if len(g.Members) != 3 { + t.Fatalf("group has %d members, want 3 (D excluded by residual guard)", len(g.Members)) + } + for _, r := range []RuleID{RuleArticleStrip, RuleSubstring} { + if !rulesContain(g.Rules, r) { + t.Errorf("group rules = %v, want union containing %s", g.Rules, r) + } + } +} + +// TestScanBlankTitles pins that untitled rows never group with each other. +func TestScanBlankTitles(t *testing.T) { + books := []models.Book{book(""), book(""), book("Dune")} + for i := range books { + books[i].ID = int64(i + 1) + } + if groups := Scan(books, nil); len(groups) != 0 { + t.Fatalf("Scan = %d groups, want 0 (blank titles never group)", len(groups)) + } +} + +// TestScanIsExclusionAgnostic pins that Scan never looks at the Excluded flag: +// exclusion policy belongs to the caller (the handler), which is the only +// layer that can act on the result. +func TestScanIsExclusionAgnostic(t *testing.T) { + a := book("Dune") + a.ID = 1 + b := book("Dune") + b.ID = 2 + b.Excluded = true + + groups := Scan([]models.Book{a, b}, nil) + if len(groups) != 1 || len(groups[0].Members) != 2 { + t.Fatalf("excluded member must still be scanned: got %d groups", len(groups)) + } + if !groups[0].Members[1].Excluded { + t.Error("member Excluded flag must be preserved in the output") + } +} + +// TestScanDeterministicUnderShuffle pins that input order does not affect the +// output (groups by key, members by ID), so API responses are stable. +func TestScanDeterministicUnderShuffle(t *testing.T) { + mk := func() []models.Book { + titles := []string{"Dune", "Hyperion", "Dune", "Dune, Part Two", "Dune"} + books := make([]models.Book, len(titles)) + for i, title := range titles { + books[i] = book(title) + books[i].ID = int64(i + 1) + } + return books + } + first := render(Scan(mk(), nil)) + // Rebuild with a different input order, same books. + src := mk() + order := []int{2, 0, 3, 1, 4} + shuffled := make([]models.Book, len(order)) + for i, s := range order { + shuffled[i] = src[s] + } + if second := render(Scan(shuffled, nil)); second != first { + t.Errorf("scan order-dependent:\nfirst = %s\nsecond = %s", first, second) + } +} + +func render(groups []Group) string { + var sb strings.Builder + for _, g := range groups { + sb.WriteString(g.Key) + sb.WriteString(":rules=") + sb.WriteString(strings.Join(mapRules(g.Rules), ",")) + for _, m := range g.Members { + fmt.Fprintf(&sb, "|%d(%s)", m.ID, strings.Join(mapRules(m.Rules), ",")) + } + sb.WriteString("\n") + } + return sb.String() +} + +func mapRules(rules []RuleID) []string { + out := make([]string, len(rules)) + for i, r := range rules { + out[i] = string(r) + } + return out +} + +func rulesContain(rules []RuleID, want RuleID) bool { + for _, r := range rules { + if r == want { + return true + } + } + return false +} + +// TestScanSuppressesSubstringAcrossSeriesPositions pins the #1970 review +// finding: the substring rule alone groups separate books in a series when a +// long first title doubles as the series name ("Foundation" is a substring +// of "Foundation and Empire"). When both books are known, different +// positions in the same series, that is by definition two different works, +// so the substring match must be suppressed even though the folded titles +// still look contained. The other rules are untouched — they require the +// folded keys to already agree, which two different titles never do. +func TestScanSuppressesSubstringAcrossSeriesPositions(t *testing.T) { + foundation := book("Foundation") + foundation.ID = 1 + foundationAndEmpire := book("Foundation and Empire") + foundationAndEmpire.ID = 2 + secondFoundation := book("Second Foundation") + secondFoundation.ID = 3 + + books := []models.Book{foundation, foundationAndEmpire, secondFoundation} + slots := map[int64][]SeriesSlot{ + 1: {{SeriesID: 100, Position: "1"}}, + 2: {{SeriesID: 100, Position: "2"}}, + 3: {{SeriesID: 100, Position: "3"}}, + } + + if groups := Scan(books, slots); len(groups) != 0 { + t.Fatalf("Scan with series positions = %d groups, want 0 (all different positions)", len(groups)) + } + + // Without series data the three still must not group: none of the + // shorter titles is a whole separator-delimited part of a longer one. + // Series links are often missing, so the guard alone was not enough. + if groups := Scan(books, nil); len(groups) != 0 { + t.Fatalf("Scan with no series data = %d groups, want 0 (no separator boundary)", len(groups)) + } +} + +// TestScanSeriesGuardAtSeparatorBoundary exercises the series guard on a pair +// the separator rule DOES match: providers often title a sequel "Series: +// Title", so "Mistborn" (position 1) is a whole segment of "Mistborn: The +// Well of Ascension" (position 2). Only the series data tells them apart. +func TestScanSeriesGuardAtSeparatorBoundary(t *testing.T) { + first := book("Mistborn") + first.ID = 1 + second := book("Mistborn: The Well of Ascension") + second.ID = 2 + books := []models.Book{first, second} + + if groups := Scan(books, nil); len(groups) != 1 { + t.Fatalf("Scan with no series data = %d groups, want 1 (separator substring match)", len(groups)) + } + slots := map[int64][]SeriesSlot{ + 1: {{SeriesID: 300, Position: "1"}}, + 2: {{SeriesID: 300, Position: "2"}}, + } + if groups := Scan(books, slots); len(groups) != 0 { + t.Fatalf("Scan with positions 1 and 2 = %d groups, want 0 (different series positions)", len(groups)) + } + unknown := map[int64][]SeriesSlot{ + 1: {{SeriesID: 300, Position: "1"}}, + 2: {{SeriesID: 300, Position: ""}}, + } + if groups := Scan(books, unknown); len(groups) != 1 { + t.Fatalf("Scan with one unknown position = %d groups, want 1 (unknown never suppresses)", len(groups)) + } + otherSeries := map[int64][]SeriesSlot{ + 1: {{SeriesID: 300, Position: "1"}}, + 2: {{SeriesID: 301, Position: "2"}}, + } + if groups := Scan(books, otherSeries); len(groups) != 1 { + t.Fatalf("Scan across different series = %d groups, want 1 (guard is per series)", len(groups)) + } +} + +// TestSubstringRequiresSeparatorBoundary pins the tightened substring rule +// against real titles from large catalogues (Asimov, King, Pratchett) with +// no series data, which is the common case for an author whose series links +// were never created. A bare containment grouped every one of the negative +// pairs below that clears the length guards. +func TestSubstringRequiresSeparatorBoundary(t *testing.T) { + negatives := [][2]string{ + {"Foundation", "Foundation and Empire"}, + {"Foundation", "Second Foundation"}, + {"Foundation", "Prelude to Foundation"}, + {"Foundation", "Forward the Foundation"}, + {"Foundation", "Foundation and Chaos"}, + {"Foundation", "The Foundation Trilogy"}, + {"The Dark Tower", "The Dark Tower I: The Gunslinger"}, + {"The Dark Tower", "The Dark Tower II: The Drawing of the Three"}, + {"The Science of Discworld", "The Science of Discworld II: The Globe"}, + {"Fantastic Voyage", "Fantastic Voyage II: Destination Brain"}, + {"Nightfall", "Nightfall and Other Stories"}, + {"Buy Jupiter", "Buy Jupiter and Other Stories"}, + {"Carrie", "Carrie Soto Is Back"}, + {"Dune", "Dune Messiah"}, + {"Dune", "Children of Dune"}, + {"It", "It Ends with Us"}, + } + for _, p := range negatives { + if got := MatchRules(p[0], p[1]); rulesContain(got, RuleSubstring) { + t.Errorf("MatchRules(%q, %q) = %v, want no substring", p[0], p[1], got) + } + } + positives := [][2]string{ + {"Mistborn", "Mistborn: The Final Empire"}, + {"The Final Empire", "Mistborn: The Final Empire"}, + {"Final Empire", "Mistborn: The Final Empire"}, + {"The Gunslinger", "The Dark Tower I: The Gunslinger"}, + {"Hogfather", "Hogfather (Discworld, #20)"}, + {"Small Gods", "Small Gods: A Discworld Novel"}, + {"On Writing", "On Writing \u2014 A Memoir of the Craft"}, + {"Outlander", "Outlander - A Novel"}, + {"The Foundation Trilogy", "Foundation: The Foundation Trilogy (Unabridged)"}, + } + for _, p := range positives { + if got := MatchRules(p[0], p[1]); !rulesContain(got, RuleSubstring) { + t.Errorf("MatchRules(%q, %q) = %v, want substring", p[0], p[1], got) + } + } +} + +// TestScanKeepsSubstringWithinSameSeriesPosition pins that the suppression is +// specific to DIFFERENT positions: a genuine subtitle variant of the same +// entry ("Mistborn" vs "Mistborn: The Final Empire", both position "1") must +// still group — same-position series data is not a reason to distrust +// substring, only cross-position data is. +func TestScanKeepsSubstringWithinSameSeriesPosition(t *testing.T) { + mistborn := book("Mistborn") + mistborn.ID = 1 + mistbornFull := book("Mistborn: The Final Empire") + mistbornFull.ID = 2 + + books := []models.Book{mistborn, mistbornFull} + slots := map[int64][]SeriesSlot{ + 1: {{SeriesID: 200, Position: "1"}}, + 2: {{SeriesID: 200, Position: "1"}}, + } + + groups := Scan(books, slots) + if len(groups) != 1 { + t.Fatalf("Scan = %d groups, want 1 (same series position, still a substring match)", len(groups)) + } + if !rulesContain(groups[0].Rules, RuleSubstring) { + t.Errorf("group rules = %v, want substring", groups[0].Rules) + } +} + +// TestScanScale pins the performance budget from the plan: 500 books with a +// few duplicate pairs must scan in well under 100ms. +func TestScanScale(t *testing.T) { + books := make([]models.Book, 0, 500) + for i := 0; i < 494; i++ { + b := book(fmt.Sprintf("Distinct Title Number %03d", i)) + b.ID = int64(i + 1) + books = append(books, b) + } + // Four rows share one title; two more share another. 494 + 4 + 2 = 500. + for i, title := range []string{"The Martian", "The Martian", "Martian", "The Martian: A Novel"} { + b := book(title) + b.ID = int64(495 + i) + books = append(books, b) + } + for i, title := range []string{"Dune", "Dune"} { + b := book(title) + b.ID = int64(499 + i) + books = append(books, b) + } + + start := time.Now() + groups := Scan(books, nil) + elapsed := time.Since(start) + + if elapsed > 100*time.Millisecond { + t.Errorf("Scan(500 books) took %s, budget 100ms", elapsed) + } + if len(groups) != 2 { + t.Fatalf("Scan = %d groups, want 2", len(groups)) + } +} diff --git a/internal/duplicates/imports_test.go b/internal/duplicates/imports_test.go new file mode 100644 index 000000000..8a2b9f8da --- /dev/null +++ b/internal/duplicates/imports_test.go @@ -0,0 +1,66 @@ +package duplicates + +import ( + "go/parser" + "go/token" + "os" + "path/filepath" + "strings" + "testing" +) + +// TestIngestionPackagesNeverImportThis enforces the #1970 boundary: the +// aggressive detection layer may only be consumed by internal/api (the +// read-only surface a human reviews). No indexing, ingestion, metadata, +// database, or command package may import this package, or the aggressive +// normalization would leak into book-creation identity and trade +// missed-duplicate noise for silent false merges (#940, #2042). +// +// The test source-scans the whole repo tree rather than relying on review: +// a future import is a CI failure, not a judgment call. +func TestIngestionPackagesNeverImportThis(t *testing.T) { + root := filepath.Join("..", "..") + const forbidden = `"github.com/vavallee/bindery/internal/duplicates"` + + var offenders []string + for _, dir := range []string{filepath.Join(root, "internal"), filepath.Join(root, "cmd")} { + err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + t.Logf("skipping %s: %v", path, err) + return nil + } + if info.IsDir() { + if path == filepath.Join(root, "internal", "duplicates") { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") { + return nil + } + rel, _ := filepath.Rel(root, path) + if strings.HasPrefix(rel, "internal/api/") { + return nil // the one allowed consumer + } + fset := token.NewFileSet() + f, perr := parser.ParseFile(fset, path, nil, 0) + if perr != nil { + t.Errorf("parse %s: %v", rel, perr) + return nil + } + for _, imp := range f.Imports { + if imp.Path.Value == forbidden { + offenders = append(offenders, rel) + } + } + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", dir, err) + } + } + if len(offenders) > 0 { + t.Errorf("import boundary violated — these files import internal/duplicates outside internal/api:\n%s", + strings.Join(offenders, "\n")) + } +} diff --git a/internal/importer/samedevice_unix.go b/internal/importer/samedevice_unix.go index 5bb8b3c68..4bdaeec47 100644 --- a/internal/importer/samedevice_unix.go +++ b/internal/importer/samedevice_unix.go @@ -27,11 +27,13 @@ func statExisting(p string) (os.FileInfo, error) { } } -// sameDevice reports whether a and b reside on the same filesystem by +// SameDevice reports whether a and b reside on the same filesystem by // comparing their OS device IDs. When b does not exist, its nearest existing // ancestor directory is used so that not-yet-created destination paths are -// handled correctly. Returns false on any stat error. -func sameDevice(a, b string) bool { +// handled correctly. Returns false on any stat error. Exported for reuse by +// internal/api's manual-import scan (#2480), which already depends on this +// package elsewhere in the same handler. +func SameDevice(a, b string) bool { if a == "" || b == "" { return false } @@ -73,7 +75,7 @@ func nearestExistingDir(p string) string { } // hardlinkable reports whether a hardlink import from src into dst would -// actually succeed. Matching device IDs (sameDevice) are necessary but NOT +// actually succeed. Matching device IDs (SameDevice) are necessary but NOT // sufficient: two separate bind mounts — or Unraid /mnt/user shares — report // the same st_dev yet os.Link across them fails with EXDEV ("invalid // cross-device link"). So after the cheap same-device gate it confirms with a @@ -82,7 +84,7 @@ func nearestExistingDir(p string) string { // read-only download dir), it trusts the same-device result rather than // reporting a false negative. func hardlinkable(src, dst string) bool { - if !sameDevice(src, dst) { + if !SameDevice(src, dst) { return false } srcDir := nearestExistingDir(src) diff --git a/internal/importer/samedevice_windows.go b/internal/importer/samedevice_windows.go index a9b595edf..cd22c876b 100644 --- a/internal/importer/samedevice_windows.go +++ b/internal/importer/samedevice_windows.go @@ -2,13 +2,13 @@ package importer -// sameDevice always returns false on Windows because device-ID comparison +// SameDevice always returns false on Windows because device-ID comparison // via syscall.Stat_t is not available. importMode will fall back to "copy". -func sameDevice(_, _ string) bool { +func SameDevice(_, _ string) bool { return false } -// hardlinkable mirrors sameDevice on Windows: device comparison is unavailable, +// hardlinkable mirrors SameDevice on Windows: device comparison is unavailable, // so the auto import mode falls back to "copy" rather than risk a failing // hardlink. func hardlinkable(_, _ string) bool { diff --git a/web/src/api/authors.ts b/web/src/api/authors.ts index 12f5f3b02..1872650d6 100644 --- a/web/src/api/authors.ts +++ b/web/src/api/authors.ts @@ -190,6 +190,31 @@ export interface CatalogueReconciliation { applied?: { requested: number; deleted: number; skipped: number } } +// Detection rules for the read-only duplicate-title report (#1970). These are +// the stable identifiers the API returns and the UI translates; the modal +// explains each one in plain language. +export type DuplicateRule = + | 'alnum-equal' + | 'article-strip' + | 'edition-suffix' + | 'substring' + +export interface DuplicateCandidateMember extends Book { + rules: DuplicateRule[] +} + +export interface DuplicateCandidateGroup { + key: string + rules: DuplicateRule[] + books: DuplicateCandidateMember[] +} + +export interface DuplicateCandidates { + authorId: number + groups: DuplicateCandidateGroup[] + count: number +} + export type MediaType = 'ebook' | 'audiobook' | 'both' export type AuthorMonitorMode = 'all' | 'future' | 'latest' | 'none' | 'series' export type MonitorNewItems = 'all' | 'none' @@ -273,6 +298,8 @@ export const authorsApi = { method: 'POST', body: JSON.stringify({ bookIds }), }), + listAuthorDuplicateCandidates: (id: number) => + request(`/author/${id}/duplicate-candidates`), searchAuthorLinkCandidates: (id: number, term: string) => request(`/author/${id}/relink-upstream/candidates?term=${encodeURIComponent(term)}`), relinkAuthorUpstream: (id: number, candidate?: RelinkAuthorCandidate) => diff --git a/web/src/api/queue.ts b/web/src/api/queue.ts index 8e9fb8337..ebaa3c608 100644 --- a/web/src/api/queue.ts +++ b/web/src/api/queue.ts @@ -41,6 +41,11 @@ export interface ScanItem { detectedFormat: string book?: Book candidates?: Book[] + // alreadyImported is true when this unit is already tracked in the library + // (directly, via a hardlink, or as a confident match whose book already has + // this format). Normally such units are excluded; includeImported surfaces + // them instead, labelled (#2480). + alreadyImported: boolean } export interface FolderScanResponse { @@ -163,8 +168,12 @@ export const queueApi = { manualImport: (data: { path: string; bookId: number; format?: string }) => request('/queue/manual-import', { method: 'POST', body: JSON.stringify(data) }), // Bulk folder import: scan a folder for book units, then import the selected ones. - scanFolder: (path: string) => - request(`/queue/manual-import/scan?path=${encodeURIComponent(path)}`), + // includeImported (default off) surfaces already-imported units instead of + // excluding them, labelled via ScanItem.alreadyImported (#2480). + scanFolder: (path: string, opts?: { includeImported?: boolean }) => { + const qs = opts?.includeImported ? '&includeImported=true' : '' + return request(`/queue/manual-import/scan?path=${encodeURIComponent(path)}${qs}`) + }, batchImport: (items: BatchImportItem[]) => request('/queue/manual-import/batch', { method: 'POST', body: JSON.stringify(items) }), diff --git a/web/src/components/DuplicateCandidatesModal.test.tsx b/web/src/components/DuplicateCandidatesModal.test.tsx new file mode 100644 index 000000000..e053b8d4a --- /dev/null +++ b/web/src/components/DuplicateCandidatesModal.test.tsx @@ -0,0 +1,159 @@ +import { fireEvent, render, screen, waitFor } from '@testing-library/react' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { api, DuplicateCandidateMember, DuplicateCandidates, DuplicateRule } from '../api/client' +import DuplicateCandidatesModal from './DuplicateCandidatesModal' + +vi.mock('react-i18next', () => { + const t = (key: string, fallback?: string | Record) => { + if (typeof fallback === 'string') return fallback + const template = String(fallback?.defaultValue ?? key) + return Object.entries(fallback ?? {}).reduce( + (text, [name, value]) => name === 'defaultValue' ? text : text.replaceAll(`{{${name}}}`, String(value)), + template, + ) + } + return { useTranslation: () => ({ t }) } +}) + +vi.mock('../api/client', () => ({ + api: { + listAuthorDuplicateCandidates: vi.fn(), + toggleExcluded: vi.fn(), + }, +})) + +function book(id: number, title: string, excluded = false, rules: DuplicateRule[] = []): DuplicateCandidateMember { + return { + id, + foreignBookId: `OL${id}W`, + authorId: 7, + title, + description: '', + imageUrl: '', + genres: [], + monitored: true, + status: 'wanted', + filePath: '', + mediaType: 'ebook', + ebookFilePath: '', + audiobookFilePath: '', + excluded, + rules, + } +} + +const result: DuplicateCandidates = { + authorId: 7, + count: 2, + groups: [ + { + key: 'dune', + rules: ['alnum-equal'], + books: [book(11, 'Dune'), book(12, 'Dune')], + }, + { + key: 'themartian', + rules: ['article-strip', 'substring'], + books: [ + book(13, 'The Martian', false, ['article-strip']), + book(14, 'Martian', true, ['article-strip']), + ], + }, + ], +} + +describe('DuplicateCandidatesModal', () => { + beforeEach(() => { + vi.clearAllMocks() + vi.mocked(api.listAuthorDuplicateCandidates).mockResolvedValue(result) + vi.mocked(api.toggleExcluded).mockResolvedValue(book(14, 'Martian', true)) + }) + + it('fetches the report and shows each group with its rules', async () => { + render( {}} />) + + // Two books share the title "Dune". + expect(await screen.findAllByText('Dune')).toHaveLength(2) + expect(api.listAuthorDuplicateCandidates).toHaveBeenCalledWith(7) + expect(screen.getByText('Identical once punctuation, case, and diacritics are ignored')).toBeInTheDocument() + // Once in the martian group header, once per martian member. + expect(screen.getAllByText('Same title with a leading article (The, A, An…) dropped')).toHaveLength(3) + // The excluded member is rendered with the Excluded badge. + expect(screen.getByText('Excluded')).toBeInTheDocument() + // Both groups have two members, so the "2 row(s)" label appears twice. + expect(screen.getAllByText('2 row(s)')).toHaveLength(2) + }) + + it('shows the empty state when there are no duplicate groups', async () => { + vi.mocked(api.listAuthorDuplicateCandidates).mockResolvedValue({ authorId: 7, count: 0, groups: [] }) + render( {}} />) + + expect(await screen.findByText('No duplicate titles found.')).toBeInTheDocument() + expect(screen.queryByRole('button', { name: 'Exclude' })).not.toBeInTheDocument() + }) + + it('excludes a row, re-fetches the report, and notifies the page', async () => { + const onChanged = vi.fn() + vi.mocked(api.listAuthorDuplicateCandidates) + .mockResolvedValueOnce(result) + .mockResolvedValueOnce({ + ...result, + count: 1, + groups: [{ key: 'dune', rules: ['alnum-equal'], books: [book(11, 'Dune'), book(12, 'Dune')] }], + }) + render( {}} onChanged={onChanged} />) + + const excludeButtons = await screen.findAllByRole('button', { name: 'Exclude' }) + fireEvent.click(excludeButtons[0]) + + await waitFor(() => { + expect(api.toggleExcluded).toHaveBeenCalledWith(11) + expect(api.listAuthorDuplicateCandidates).toHaveBeenCalledTimes(2) + expect(onChanged).toHaveBeenCalledTimes(1) + }) + // After the re-fetch the martian group is gone. + expect(screen.queryByText('The Martian')).not.toBeInTheDocument() + }) + + it('offers Include for already-excluded rows', async () => { + render( {}} />) + + const include = await screen.findByRole('button', { name: 'Include' }) + fireEvent.click(include) + + await waitFor(() => expect(api.toggleExcluded).toHaveBeenCalledWith(14)) + }) + + it('keeps an in-flight row disabled while another row is toggling', async () => { + // Two toggles in flight at once: with a single shared busy id, starting + // the second used to re-enable the first row's button, so a second click + // could fire an overlapping flip that cancels the first out silently. + const pending: Array<(value: DuplicateCandidateMember) => void> = [] + vi.mocked(api.toggleExcluded).mockImplementation( + () => new Promise(resolve => { pending.push(resolve) }), + ) + render( {}} />) + + const [first, second] = await screen.findAllByRole('button', { name: 'Exclude' }) + fireEvent.click(first) + expect(first).toBeDisabled() + fireEvent.click(second) + expect(first).toBeDisabled() + expect(second).toBeDisabled() + + pending.forEach(resolve => resolve(book(11, 'Dune', true))) + await waitFor(() => { + const buttons = screen.getAllByRole('button', { name: 'Exclude' }) + expect(buttons[0]).not.toBeDisabled() + expect(buttons[1]).not.toBeDisabled() + }) + expect(api.toggleExcluded).toHaveBeenCalledTimes(2) + }) + + it('shows the load error when the request fails', async () => { + vi.mocked(api.listAuthorDuplicateCandidates).mockRejectedValue(new Error('boom')) + render( {}} />) + + expect(await screen.findByText('boom')).toBeInTheDocument() + }) +}) diff --git a/web/src/components/DuplicateCandidatesModal.tsx b/web/src/components/DuplicateCandidatesModal.tsx new file mode 100644 index 000000000..9d3e057d9 --- /dev/null +++ b/web/src/components/DuplicateCandidatesModal.tsx @@ -0,0 +1,168 @@ +import { useCallback, useEffect, useState } from 'react' +import { useTranslation } from 'react-i18next' +import { api, DuplicateCandidates, DuplicateRule } from '../api/client' +import { btn, btnSize } from './buttons' + +interface Props { + authorId: number + authorName: string + onClose: () => void + // Called after a successful exclusion toggle so the page can refresh its + // book lists (the modal re-fetches its own groups on its own). + onChanged?: () => void +} + +const ruleDefaults: Record = { + 'alnum-equal': 'Identical once punctuation, case, and diacritics are ignored', + 'article-strip': 'Same title with a leading article (The, A, An…) dropped', + 'edition-suffix': 'Same title with an edition marker (Unabridged, Audiobook…) dropped', + 'substring': 'One title is the main title or subtitle of the other', +} + +export default function DuplicateCandidatesModal({ authorId, authorName, onClose, onChanged }: Props) { + const { t } = useTranslation() + const [result, setResult] = useState(null) + const [loading, setLoading] = useState(true) + const [busyBooks, setBusyBooks] = useState>(() => new Set()) + const [error, setError] = useState(null) + + const load = useCallback(() => { + setLoading(true) + setError(null) + api.listAuthorDuplicateCandidates(authorId) + .then(setResult) + .catch(err => { + setError(err instanceof Error ? err.message : t('duplicateCandidates.loadFailed', 'Loading duplicates failed')) + }) + .finally(() => { setLoading(false) }) + }, [authorId, t]) + + useEffect(() => { load() }, [load]) + + const toggleExclusion = async (bookId: number) => { + setBusyBooks(prev => new Set(prev).add(bookId)) + setError(null) + try { + await api.toggleExcluded(bookId) + load() + onChanged?.() + } catch (err) { + setError(err instanceof Error ? err.message : t('duplicateCandidates.toggleFailed', 'Changing the exclusion failed')) + } finally { + setBusyBooks(prev => { + const next = new Set(prev) + next.delete(bookId) + return next + }) + } + } + + const renderRules = (rules: DuplicateRule[]) => ( + + {rules.map(rule => ( + + {t(`duplicateCandidates.rules.${rule}`, ruleDefaults[rule])} + + ))} + + ) + + return ( +
+
event.stopPropagation()} + role="dialog" + aria-modal="true" + aria-labelledby="duplicate-candidates-title" + > +

+ {t('duplicateCandidates.title', 'Review duplicate titles')} +

+

+ {t('duplicateCandidates.description', { + author: authorName, + defaultValue: 'Titles for {{author}} that look like the same book. Nothing changes until you exclude a row.', + })} +

+ + {loading ? ( +

+ {t('duplicateCandidates.loading', 'Scanning the catalogue…')} +

+ ) : error && !result ? ( +

{error}

+ ) : result ? ( + result.count === 0 ? ( +

+ {t('duplicateCandidates.empty', 'No duplicate titles found.')} +

+ ) : ( +
+ {result.groups.map(group => ( +
+
+ + {t('duplicateCandidates.group', { count: group.books.length, defaultValue: '{{count}} row(s)' })} + + {renderRules(group.rules)} +
+
    + {group.books.map(book => ( +
  • + + + {book.title} + + + {book.excluded && ( + + {t('duplicateCandidates.excluded', 'Excluded')} + + )} + {renderRules(book.rules)} + + + +
    • + ))} +
+
+ ))} +
+ ) + ) : null} + + {error && result &&

{error}

} + +
+ + +
+
+
+ ) +} diff --git a/web/src/components/useFolderScan.test.tsx b/web/src/components/useFolderScan.test.tsx new file mode 100644 index 000000000..700f0ab86 --- /dev/null +++ b/web/src/components/useFolderScan.test.tsx @@ -0,0 +1,117 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest' +import { act, render, screen, fireEvent } from '@testing-library/react' +import { useFolderScan } from './useFolderScan' + +vi.mock('../api/client', () => ({ + api: { scanFolder: vi.fn() }, +})) + +import { api } from '../api/client' +const mockScan = api.scanFolder as ReturnType + +// Harness renders the hook's state as text/attributes so assertions don't +// need to reach into React internals, and exposes its actions as buttons. +function Harness({ onItems }: { onItems?: (items: { path: string }[]) => void }) { + const fs = useFolderScan(onItems as never) + return ( +
+ fs.setPath(e.target.value)} /> + + + +
{String(fs.scanning)}
+
{String(fs.truncated)}
+
{String(fs.showImported)}
+
{fs.error ?? ''}
+
{fs.items ? fs.items.map(i => i.path).join(',') : 'null'}
+
+ ) +} + +describe('useFolderScan', () => { + beforeEach(() => vi.clearAllMocks()) + + it('does nothing when scanning an empty path', () => { + render() + fireEvent.click(screen.getByText('scan')) + expect(mockScan).not.toHaveBeenCalled() + }) + + it('scans with includeImported=false by default and reports items/truncated', async () => { + mockScan.mockResolvedValue({ items: [{ path: '/dl/a.epub' }], truncated: false }) + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + + expect(mockScan).toHaveBeenCalledWith('/dl', { includeImported: false }) + expect(screen.getByTestId('items').textContent).toBe('/dl/a.epub') + expect(screen.getByTestId('truncated').textContent).toBe('false') + }) + + it('invokes onItems with the fetched items on a successful scan', async () => { + mockScan.mockResolvedValue({ items: [{ path: '/dl/a.epub' }], truncated: false }) + const onItems = vi.fn() + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + + expect(onItems).toHaveBeenCalledWith([{ path: '/dl/a.epub' }]) + }) + + it('sets error and leaves items null when the scan fails', async () => { + mockScan.mockRejectedValue(new Error('boom')) + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + + expect(screen.getByTestId('error').textContent).toBe('boom') + expect(screen.getByTestId('items').textContent).toBe('null') + }) + + it('toggling on re-scans with includeImported=true only once items exist', async () => { + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + + // No prior scan yet: toggling just flips the flag, no fetch. + fireEvent.click(screen.getByText('toggle')) + expect(screen.getByTestId('showImported').textContent).toBe('true') + expect(mockScan).not.toHaveBeenCalled() + + mockScan.mockResolvedValue({ items: [{ path: '/dl/a.epub' }], truncated: false }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + expect(mockScan).toHaveBeenCalledWith('/dl', { includeImported: true }) + + // Now that items exist, toggling again re-scans. + mockScan.mockClear() + mockScan.mockResolvedValue({ items: [], truncated: false }) + await act(async () => { fireEvent.click(screen.getByText('toggle')) }) + expect(mockScan).toHaveBeenCalledWith('/dl', { includeImported: false }) + }) + + // The bug this hook fixes: a stale truncated=true from a previous scan must + // not survive into a later scan that isn't truncated (#2480). + it('clears a stale truncated flag once a later scan reports untruncated', async () => { + mockScan.mockResolvedValue({ items: [], truncated: true }) + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + expect(screen.getByTestId('truncated').textContent).toBe('true') + + mockScan.mockResolvedValue({ items: [], truncated: false }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + expect(screen.getByTestId('truncated').textContent).toBe('false') + }) + + it('clear() drops items, truncated, and error together', async () => { + mockScan.mockResolvedValue({ items: [{ path: '/dl/a.epub' }], truncated: true }) + render() + fireEvent.change(screen.getByLabelText('path'), { target: { value: '/dl' } }) + await act(async () => { fireEvent.click(screen.getByText('scan')) }) + expect(screen.getByTestId('items').textContent).toBe('/dl/a.epub') + + fireEvent.click(screen.getByText('clear')) + expect(screen.getByTestId('items').textContent).toBe('null') + expect(screen.getByTestId('truncated').textContent).toBe('false') + expect(screen.getByTestId('error').textContent).toBe('') + }) +}) diff --git a/web/src/components/useFolderScan.ts b/web/src/components/useFolderScan.ts new file mode 100644 index 000000000..5f55d45f7 --- /dev/null +++ b/web/src/components/useFolderScan.ts @@ -0,0 +1,68 @@ +import { useCallback, useState } from 'react' +import { api } from '../api/client' +import type { ScanItem } from '../api/client' + +export interface UseFolderScanResult { + path: string + setPath: (p: string) => void + scanning: boolean + items: ScanItem[] | null + truncated: boolean + showImported: boolean + error: string | null + scan: () => void + toggleShowImported: (checked: boolean) => void + clear: () => void +} + +/** + * useFolderScan drives the bulk-folder-import scan (#2480) shared by the + * Settings > Import "Bulk folder import" section and the /import wizard page: + * the path input, the scan request (with the includeImported toggle, + * default off), and the resulting items/truncated/error state. Each page + * still builds its own per-unit resolution state (selection, format, chosen + * book) from the fetched items via the optional onItems callback. + */ +export function useFolderScan(onItems?: (items: ScanItem[]) => void): UseFolderScanResult { + const [path, setPath] = useState('') + const [scanning, setScanning] = useState(false) + const [items, setItems] = useState(null) + const [truncated, setTruncated] = useState(false) + const [showImported, setShowImported] = useState(false) + const [error, setError] = useState(null) + + // clear drops the previous scan's results (items AND truncated together — + // leaving truncated set with no items to explain it is a stale banner with + // nothing beneath it) without touching path or showImported. + const clear = useCallback(() => { + setItems(null) + setTruncated(false) + setError(null) + }, []) + + const runScan = useCallback(async (includeImported: boolean) => { + const p = path.trim() + if (!p) return + setScanning(true) + clear() + try { + const r = await api.scanFolder(p, { includeImported }) + setItems(r.items) + setTruncated(r.truncated) + onItems?.(r.items) + } catch (e) { + setError(e instanceof Error ? e.message : 'Scan failed') + } finally { + setScanning(false) + } + }, [path, clear, onItems]) + + const scan = useCallback(() => { void runScan(showImported) }, [runScan, showImported]) + + const toggleShowImported = useCallback((checked: boolean) => { + setShowImported(checked) + if (items) void runScan(checked) + }, [items, runScan]) + + return { path, setPath, scanning, items, truncated, showImported, error, scan, toggleShowImported, clear } +} diff --git a/web/src/i18n/locales/en.json b/web/src/i18n/locales/en.json index 439887002..1853c0b94 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -116,7 +116,10 @@ "scan": "Scan folder", "scanning": "Scanning…", "empty": "No book files or folders found here.", + "emptyHidesImported": "Nothing new to import here. Files already in your library are hidden; tick Show already imported to list them.", "truncated": "Showing the first 1000 units; narrow the folder to see the rest.", + "showImported": "Show already imported", + "alreadyImported": "already imported", "selectAll": "Select all matched", "importSelected": "Import {{count}} selected", "group": { @@ -482,6 +485,8 @@ "mergeHint": "Merge another author into this one", "reconcileCatalogue": "Reconcile catalogue…", "reconcileCatalogueHint": "Preview stale metadata-only Wanted rows before removing them", + "reviewDuplicates": "Review duplicates…", + "reviewDuplicatesHint": "Find titles that look like the same book; nothing changes until you exclude a row", "delete": "Delete", "refreshHint": "Fetch this author's catalogue from the metadata provider again. Books you do not have are added according to Monitor new items. Nothing is downloaded until the next wanted search." }, @@ -532,6 +537,25 @@ "catalogue_filter": "Rejected by the catalogue filter" } }, + "duplicateCandidates": { + "title": "Review duplicate titles", + "description": "Titles for {{author}} that look like the same book. Nothing changes until you exclude a row.", + "loading": "Scanning the catalogue…", + "loadFailed": "Loading duplicates failed", + "toggleFailed": "Changing the exclusion failed", + "empty": "No duplicate titles found.", + "group": "{{count}} row(s)", + "excluded": "Excluded", + "exclude": "Exclude", + "include": "Include", + "refresh": "Rescan", + "rules": { + "alnum-equal": "Identical once punctuation, case, and diacritics are ignored", + "article-strip": "Same title with a leading article (The, A, An…) dropped", + "edition-suffix": "Same title with an edition marker (Unabridged, Audiobook…) dropped", + "substring": "One title is the main title or subtitle of the other" + } + }, "books": { "title": "Books", "countLabel": "{{count}} books", diff --git a/web/src/pages/AuthorDetailPage.tsx b/web/src/pages/AuthorDetailPage.tsx index 68b410552..93d2f3daa 100644 --- a/web/src/pages/AuthorDetailPage.tsx +++ b/web/src/pages/AuthorDetailPage.tsx @@ -22,6 +22,7 @@ import MoreMenu from '../components/MoreMenu' import Section from '../components/Section' import AuthorSyncNotice from '../components/AuthorSyncNotice' import CatalogueReconciliationModal from '../components/CatalogueReconciliationModal' +import DuplicateCandidatesModal from '../components/DuplicateCandidatesModal' type MediaFilter = '' | 'ebook' | 'audiobook' // 'excluded' folds in what used to be a separate "Show excluded" checkbox. It @@ -123,6 +124,7 @@ export default function AuthorDetailPage() { const [showRename, setShowRename] = useState(false) const [showMetadataLink, setShowMetadataLink] = useState(false) const [showCatalogueReconciliation, setShowCatalogueReconciliation] = useState(false) + const [showDuplicateCandidates, setShowDuplicateCandidates] = useState(false) const [error, setError] = useState(null) // Bulk multi-select state (#791). Selection is keyed by book.id and @@ -980,6 +982,11 @@ export default function AuthorDetailPage() { title: t('authorDetail.actions.reconcileCatalogueHint', 'Preview stale metadata-only Wanted rows before removing them'), onSelect: () => setShowCatalogueReconciliation(true), }, + { + label: t('authorDetail.actions.reviewDuplicates', 'Review duplicates…'), + title: t('authorDetail.actions.reviewDuplicatesHint', 'Find titles that look like the same book; nothing changes until you exclude a row'), + onSelect: () => setShowDuplicateCandidates(true), + }, { label: t('authorDetail.actions.delete', 'Delete'), danger: true, @@ -1056,6 +1063,15 @@ export default function AuthorDetailPage() { /> )} + {showDuplicateCandidates && ( + setShowDuplicateCandidates(false)} + onChanged={reloadBooks} + /> + )} + {showMerge && allAuthors.length > 0 && ( {item.detectedFormat} + {item.alreadyImported && ( + + {t('manualImport.alreadyImported', 'already imported')} + + )} {/* Full source path so the user can tell which file each row refers to; the basename alone is ambiguous across folders (#1435). */} @@ -141,7 +146,7 @@ export default function FolderImportRow({ item, row, result, importing, onPick,