From c2831bbee09553fa620c0ed8ee15e2e74e3a4ddb Mon Sep 17 00:00:00 2001 From: gchahcg <7381837@proton.me> Date: Mon, 28 Sep 2026 17:21:04 -0400 Subject: [PATCH 1/2] feat(author): surface duplicate-title candidates for human review (#1970) (#2678) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(author): surface duplicate-title candidates for human review (#1970) The same book often reaches a catalogue twice under slightly different titles ("The Martian" / "Martian", "Dune" / "Dune (Unabridged)"), and there was no way to see the collisions. This adds a read-only report that groups an author's titles that look like the same book, and a review modal whose only action is the existing exclude route. Detection lives in internal/duplicates, a package nothing else may import (a source-scan test pins the boundary): an aggressive title fold (case, punctuation, diacritics; & expanded to "and") feeds four rules — alnum-equal, article-strip, edition-suffix, and a guarded substring — and matches are linked transitively into groups. Each group and member reports which rule(s) matched, so the UI can explain every group in plain language. The aggressive fold is separate from the dedup key on purpose: it may be aggressive, the dedup key may not. GET /author/{id}/duplicate-candidates writes nothing; groups whose members are all already excluded are omitted. The author page's More menu gains "Review duplicates…", and the modal lists each group with its rules and an Exclude button per row, which calls PUT /book/{id}/exclude and re-fetches. Excluded members are shown struck through, and a group disappears once every member is excluded. API.md documents the endpoint and response; the user guide explains the review flow. Signed-off-by: gch ahcg * fix(web): address review findings on duplicate-candidates (#2678) The modal tracked one busy book id at a time, so starting a second row's toggle re-enabled the first row's button; a second click could fire an overlapping flip that cancels the first out silently. Track in-flight rows per book instead. The Scan godoc claimed maxPairwiseBooks caps the O(n^2) pairwise pass, but only the substring rule is gated above 2000 books. State what the loop actually does. Signed-off-by: gch ahcg * fix(duplicates): suppress substring across different series positions (#1970) The substring rule alone grouped separate books in a series whenever a long series-opener title doubled as a prefix of a sequel's title ("Foundation" vs "Foundation and Empire", "Second Foundation") — found in review by running real titles through duplicates.Scan. DuplicateCandidates now also loads the author's series memberships and passes them to Scan, which drops a substring-only match between two books that are known, different positions in the same series — by definition different works. Same-position pairs (a genuine subtitle variant, e.g. Mistborn / Mistborn: The Final Empire) are unaffected. Missing series data (h.series nil, or no membership recorded) falls back to today's behaviour, per the review's accepted fallback. Signed-off-by: gch ahcg * fix(duplicates): only match a subtitle at a separator boundary (#1970) The series position guard only helps when series links exist, and they are often missing. Without them the substring rule still grouped a series opener with every sequel carrying its name. Real catalogues with no series data: Asimov "Foundation" pulled in nine other titles as one group, King's "The Dark Tower" merged true and false pairs into a single transitive group, and "The Science of Discworld" grouped with its two sequels, as did "Carrie" with "Carrie Soto Is Back". The shorter title must now equal a whole part of the longer one as it is punctuated (split at a colon, semicolon, bracket, spaced hyphen, or en or em dash), compared with and without a leading article. Mistborn, The Gunslinger, Hogfather (Discworld, #20) and the other real subtitle cases still group; the series guard still covers "Mistborn" against "Mistborn: The Well of Ascension". The aggressive key also drops Latin diacritics after the umlaut expansion, so "Les Misérables" meets "Les Miserables", which is what the alnum-equal rule text in the UI already claimed. The handler series test gains a position 2 sequel that the separator rule matches, so removing the guard now fails it; with the old Foundation fixture it passed either way. Signed-off-by: vavallee Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 * docs(changelog): rewrite the duplicate review fragment (#1970) Drop the dashes, describe the separator rule, and credit gchahcg. Signed-off-by: vavallee Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 --------- Signed-off-by: gch ahcg Signed-off-by: vavallee Co-authored-by: gch ahcg Co-authored-by: vavallee Co-authored-by: Claude Opus 5.5 --- changelog.d/1970-duplicate-candidates.md | 2 + cmd/bindery/main.go | 1 + docs/API.md | 43 +- docs/User-Guide-Wiki.md | 16 + internal/api/authors_duplicate_candidates.go | 84 +++ .../api/authors_duplicate_candidates_test.go | 445 +++++++++++++ internal/duplicates/duplicates.go | 547 ++++++++++++++++ internal/duplicates/duplicates_test.go | 590 ++++++++++++++++++ internal/duplicates/imports_test.go | 66 ++ web/src/api/authors.ts | 27 + .../DuplicateCandidatesModal.test.tsx | 159 +++++ .../components/DuplicateCandidatesModal.tsx | 168 +++++ web/src/i18n/locales/en.json | 21 + web/src/pages/AuthorDetailPage.tsx | 16 + 14 files changed, 2183 insertions(+), 2 deletions(-) create mode 100644 changelog.d/1970-duplicate-candidates.md create mode 100644 internal/api/authors_duplicate_candidates.go create mode 100644 internal/api/authors_duplicate_candidates_test.go create mode 100644 internal/duplicates/duplicates.go create mode 100644 internal/duplicates/duplicates_test.go create mode 100644 internal/duplicates/imports_test.go create mode 100644 web/src/components/DuplicateCandidatesModal.test.tsx create mode 100644 web/src/components/DuplicateCandidatesModal.tsx 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/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..6705ef141 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -828,6 +828,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/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/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/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/i18n/locales/en.json b/web/src/i18n/locales/en.json index 439887002..cb48365ee 100644 --- a/web/src/i18n/locales/en.json +++ b/web/src/i18n/locales/en.json @@ -482,6 +482,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 +534,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 && ( Date: Mon, 28 Sep 2026 17:28:39 -0400 Subject: [PATCH 2/2] fix(import): skip files already tracked in library (#2480) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(import): skip files already tracked in library Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * test(import): cover tracked-file scan failures Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(import): detect tracked hardlinked files Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(api): isolate manual import path validation Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(import): skip fully tracked directory units Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(import): suppress already imported scan matches Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(import): skip copied sources for tracked books Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * test(import): cover manual scan tracking helpers Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * perf(db): batch book_files lookups and expose a change counter The manual-import scan looked up book_files once per confident catalogue match (N+1) to check whether a match's format was already imported. ListByBooks/ListFilesForBooks replace that with one query. BookFileRepo also gets an atomic version counter bumped on every mutation, so a derived cache (the scan's tracked-file index) can tell cheaply whether it needs to rebuild instead of re-querying on every request. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Fn38ejN3ZtW9HNAjcS4p8m Signed-off-by: Claude * fix(import): filter already-tracked scan units before the cap, not after enumerateImportUnits capped at 1000 raw units before the already-tracked filter ran, so a folder where the first 1000 units were all already imported came back empty with truncated=true even though untracked files existed further down the tree. The tracked/hardlink check now runs inside the walk via a skip predicate, so the cap counts only surfaced units; when the separate post-lookup "book already has this format" check still empties part of a page, Scan re-walks (bounded by maxScanRounds) to refill it instead of returning a short, misleadingly non-empty-looking truncated page. Also adds includeImported (default off) to disable both filters and label each unit's AlreadyImported status instead of dropping it, and skips the tracked-file hardlink stat/walk for tracked paths confirmed to be on a different device than the scan root (the common NFS/ multi-mount case) — both feeding the new tracked-file index cache that rebuilds only when book_files actually changes. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Fn38ejN3ZtW9HNAjcS4p8m Signed-off-by: Claude * feat(web): add a show-already-imported toggle to manual import Filtering already-imported files out of the bulk scan made them unreachable, with no way to spot a corrupt file or one needing a relink. Both scan UIs (Settings > Import > Bulk folder import, and the /import page) get a "Show already imported" checkbox (default off, mirroring the wanted-list includeExcluded pattern) that re-scans with includeImported and labels surfaced units instead of pre-selecting them. The truncation banner also now renders whenever the scan reports truncated, not only when the item list happens to be non-empty. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Fn38ejN3ZtW9HNAjcS4p8m Signed-off-by: Claude * refactor(api): reuse importer.SameDevice instead of a third copy confirmedCrossDevice duplicated the same device-comparison logic already sitting in hardlinkableReason's own copy of importer.sameDevice -- justified there as avoiding an api->importer dependency, but that dependency already exists throughout this package (manual_import.go, scan_walk.go, books.go, ...). Export sameDevice as SameDevice and call it directly instead of adding a fourth copy of the same comparison. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Fn38ejN3ZtW9HNAjcS4p8m Signed-off-by: Claude * refactor(web): extract useFolderScan, fix stale truncated banner FolderScanSection and ManualImportPage had near-duplicated scan state and logic (path/scanning/items/truncated/showImported plus the run/ toggle functions), which had already started drifting between the two copies. useFolderScan consolidates it, following the existing usePolling/useView hook convention. Consolidating surfaced a real bug shared by both copies: neither reset `truncated` when a new scan started, so toggling "show already imported" (or, on the settings page, editing the path) after a truncated scan could leave that banner showing over a later, untruncated result. Fixed once in the hook instead of twice. Also tightens two related correctness gaps the "show already imported" toggle introduced in ManualImportPage: "Select all matched" no longer sweeps up an already-imported unit the toggle deliberately left unselected, and the per-row Import button is now disabled to match what clicking it actually does, instead of silently no-op'ing on an unselected row. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01Fn38ejN3ZtW9HNAjcS4p8m Signed-off-by: Claude * fix(web): align manual import's show-already-imported checkbox top-right Move it into the header row next to the "Manual Import" title, matching the Wanted page's "Show excluded" placement, instead of sitting on its own line below the scan input. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_016BZJo7qdcVP6K8vRiF42C8 Signed-off-by: Claude * fix(import): rebuild the tracked-file cache from book_files itself, cut redundant stats Two issues from review on #2480: The cache was keyed on BookFileRepo's own atomic mutation counter, which only advances when BookFileRepo's own methods run. book_files rows also disappear through the books(id) ON DELETE CASCADE FK when a book or author is deleted (BookRepo.Delete), and through BookRepo.UntrackFilePath's calibre-rollback DELETE — neither touches BookFileRepo, so neither bumped the counter. Reproduced: seed a tracked epub, scan (hidden), delete the book keeping the file, scan again — still hidden. BookFileRepo.Fingerprint replaces the counter with a (count, maxID) snapshot read straight off the table, which catches every mutation regardless of which code path made it. The cold rebuild also cost far more than it should: confirmedCrossDevice restatted the scan root AND re-stat'd every tracked path (once directly, once inside importer.SameDevice) before the loop's own os.Stat ran a third time on the same path — up to 5 stats per tracked row, versus main's 8 stats for a whole scan. trackedFileIndex now resolves the root's device once per call and reads each tracked path's device off the FileInfo the loop already has, down to 1 stat per row. Separately, isAlreadyTracked ran once in roundSkip and again in the results loop for the same unit; roundSkip already filters every already-tracked unit out of cands when includeImported is off, so the second call only ever matters when includeImported is on, and can be skipped otherwise. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01L3vXCo9PwmffPWUik6Gwvh Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * i18n(web): add manualImport already-imported and show-imported keys Both strings existed only as inline t() defaults in FolderImportView and FolderImportRow, flagged in review on #2480. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01L3vXCo9PwmffPWUik6Gwvh Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * chore(changelog): add fragment for the skip-tracked manual import feature Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> * fix(import): rebuild the tracked-file cache after a reorganize move The (COUNT, MAX(id)) fingerprint catches inserts and deletes, including the FK cascade, but UpdatePath rewrites book_files.path in place and changes neither number. After Reorganize Library moved a tracked file, the next scan served the old index: the moved file was offered for import again and a new file landing at the old path stayed hidden. UpdatePath now bumps a per database epoch, and the cache keys on it alongside the fingerprint. It is per *sql.DB rather than per repo because reorganize and the scan reach book_files through different repo instances. The epoch is read before the fingerprint so a rename racing a rebuild only causes another rebuild. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee * fix(import): do not hide a new file that recycled a tracked inode The tracked-file cache keeps each tracked file's os.FileInfo and matches hardlinks with os.SameFile, which compares device and inode only. When a tracked file is deleted outside Bindery its book_files row stays, the cache is not rebuilt, and ext4 gives the freed inode to the next file created. Deleting a corrupt library copy and downloading a fresh one is enough: the fresh download matched the stale FileInfo and was hidden as already imported until something else touched book_files. A real hardlink shares the inode's size and mtime as well, so require those to match too. A tracked file edited in place since the cache was built now shows as importable, which is the safe direction. Also adds a test that a tracked path missing from disk when the index was built stays hidden by its exact path once it comes back, which is the only thing that covered the path set lookup: hardlink matching masked its removal in every existing test. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee * fix(web): say already imported files are hidden when a folder scan is empty With the already imported filter on, an empty scan usually means every file in the folder is already in the library, but the page still said "No book files or folders found here." Point at the Show already imported toggle instead, and keep the plain empty message for when the toggle is on and the folder really has nothing. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee * chore(changelog): file the skip tracked fragment under Fixed Moves it from Added to Fixed, drops the dashes, and credits @trevorswanson in the usual form. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --------- Signed-off-by: Trevor Swanson <83826109+trevorswanson@users.noreply.github.com> Signed-off-by: Claude Signed-off-by: vavallee Co-authored-by: Claude Co-authored-by: vavallee --- .../2480-skip-tracked-manual-import.md | 2 + docs/User-Guide-Wiki.md | 10 +- internal/api/manual_import.go | 297 +++++-- internal/api/manual_import_cache.go | 116 +++ internal/api/manual_import_test.go | 748 ++++++++++++++++++ internal/api/samedevice_unix.go | 31 + internal/api/samedevice_unix_test.go | 56 ++ internal/api/samedevice_windows.go | 18 + internal/api/scan_walk.go | 13 +- internal/api/scan_walk_series_test.go | 2 +- internal/api/scan_walk_test.go | 32 +- internal/db/book_files.go | 85 ++ internal/db/book_files_test.go | 175 ++++ internal/db/books.go | 23 + internal/importer/samedevice_unix.go | 12 +- internal/importer/samedevice_windows.go | 6 +- web/src/api/queue.ts | 13 +- web/src/components/useFolderScan.test.tsx | 117 +++ web/src/components/useFolderScan.ts | 68 ++ web/src/i18n/locales/en.json | 3 + web/src/pages/import/FolderImportRow.tsx | 7 +- .../pages/import/FolderImportView.test.tsx | 93 ++- web/src/pages/import/FolderImportView.tsx | 83 +- 23 files changed, 1902 insertions(+), 108 deletions(-) create mode 100644 changelog.d/2480-skip-tracked-manual-import.md create mode 100644 internal/api/manual_import_cache.go create mode 100644 internal/api/samedevice_unix_test.go create mode 100644 web/src/components/useFolderScan.test.tsx create mode 100644 web/src/components/useFolderScan.ts 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/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index 6705ef141..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. 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/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/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/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 cb48365ee..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": { diff --git a/web/src/pages/import/FolderImportRow.tsx b/web/src/pages/import/FolderImportRow.tsx index 89b446c16..a13fd3859 100644 --- a/web/src/pages/import/FolderImportRow.tsx +++ b/web/src/pages/import/FolderImportRow.tsx @@ -54,6 +54,11 @@ export default function FolderImportRow({ item, row, result, importing, onPick, {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,