diff --git a/changelog.d/1779-openlibrary-cover-edition.md b/changelog.d/1779-openlibrary-cover-edition.md new file mode 100644 index 000000000..4028ae803 --- /dev/null +++ b/changelog.d/1779-openlibrary-cover-edition.md @@ -0,0 +1,2 @@ +### Fixed +- **OpenLibrary works take their language and cover from the edition OpenLibrary features** (#1779). When a work had no language or cover of its own, Bindery read a handful of editions in whatever order OpenLibrary returned them, so a work whose first few happened to be translations could be filed as foreign language or shown with a translated cover. It now uses the work's featured edition, the one its openlibrary.org page shows, and only samples other editions for what that edition lacks, preferring a cover in the language it settled on. A single book fetched from OpenLibrary, as on Re-bind, also takes the featured edition's cover when the work has none. This costs up to two extra OpenLibrary requests per work that needs sampling, and one extra request for a single book only when its work has no cover. diff --git a/changelog.d/2781-rebind-old-editions.md b/changelog.d/2781-rebind-old-editions.md new file mode 100644 index 000000000..1720f5a7f --- /dev/null +++ b/changelog.d/2781-rebind-old-editions.md @@ -0,0 +1,2 @@ +### Fixed +- **Re-binding a book drops the old record's editions** (#2781). A book re-bound from the wrong metadata record kept that record's provider editions, so searches could keep using the wrong work's ISBN. A re-bind to a different record now removes them. Editions imported from Calibre or Audiobookshelf, the edition you selected, and editions a download or Calibre delivery points at are kept. Thanks to magrhino for the report. diff --git a/changelog.d/2869-isbn-lookup-cache.md b/changelog.d/2869-isbn-lookup-cache.md new file mode 100644 index 000000000..3af7db36d --- /dev/null +++ b/changelog.d/2869-isbn-lookup-cache.md @@ -0,0 +1,2 @@ +### Fixed +- **ISBN and Audible ASIN lookups no longer hand out a stale or shared record** (#2869). An ISBN or ASIN answer was cached for a day under a key that ignored the metadata provider settings, so changing the primary provider or the Hardcover token kept serving the old answer, and every caller was handed the same cached book. Each lookup now gets its own copy, a settings change fetches a fresh answer, and a record built while an enrichment provider was failing is kept for five minutes instead of a day so it is retried. Thanks to magrhino for the report. diff --git a/changelog.d/3091-supplement-translations.md b/changelog.d/3091-supplement-translations.md new file mode 100644 index 000000000..5de648499 --- /dev/null +++ b/changelog.d/3091-supplement-translations.md @@ -0,0 +1,2 @@ +### Fixed +- **Translations from Hardcover no longer slip past the language filter when OpenLibrary is the primary provider** (#3091). Hardcover's edition languages were only checked when Hardcover was the primary provider, so with OpenLibrary primary its translated works arrived with no language and the default "pass" setting let them in, often monitored and wanted. They are now checked whenever a Hardcover token is set, and a work whose only known editions are in other languages is skipped. A work whose title is in a different script from the author's other titles no longer inherits the author's language, and OpenLibrary no longer spends a request per Hardcover work looking for editions it does not have. Thanks to magrhino for the report. diff --git a/docs/Metadata-Editing-Wiki.md b/docs/Metadata-Editing-Wiki.md index e216bfdb5..5c3e56f41 100644 --- a/docs/Metadata-Editing-Wiki.md +++ b/docs/Metadata-Editing-Wiki.md @@ -29,6 +29,13 @@ Two actions clear locks on their own, deliberately: **Re-bind** and metadata **re-map**. Both mean "this is the wrong record, take the new one", so Bindery applies the new provider record wholesale. +A re-bind to a different record also drops the editions the metadata provider +listed for the old record, so their ISBNs no longer steer searches for the +book. Editions created by the Calibre or Audiobookshelf import for your files +are kept, and so is the edition you selected for the book and any edition a +download or a Calibre delivery recorded. Re-binding to the record the book is +already bound to changes no editions. + ## What you cannot edit: an author's sort name The value that decides which letter an author files under, in the Authors list diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index a6bf45b81..518b38df7 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -791,15 +791,26 @@ like everything else on that tab that names server paths. selector stays disabled until you save one. When a metadata profile restricts languages, Bindery checks Hardcover's - editions for each author work in one batched request. A translated default - edition is not treated as the language of the whole work: any edition in an - allowed language keeps the work, while a work is rejected as non-allowed - only when the lookup completes and finds no allowed edition. This filtering - evidence does not rewrite the displayed language, which remains the + editions for each author work in one batched request. This happens whether + Hardcover is the primary provider or supplements OpenLibrary, so the + Hardcover works merged into an OpenLibrary author are checked too. A + translated default edition is not treated as the language of the whole work: + any edition in an allowed language keeps the work, while a work is rejected + as non-allowed only when the lookup completes and finds no allowed edition + and some edition, default or not, records another language. A work with no + language on any edition stays unknown. This filtering evidence does not + rewrite the displayed language, which remains the provider's preferred/default language or the user's locked value. If the evidence is indeterminate or its lookup fails, normal refreshes fall through to the existing edition-sampled, author-majority, and scalar language before - applying **When book language is unknown**. **Reconcile catalogue** treats a + applying **When book language is unknown**. The author-majority fallback is + not applied to a work whose title is written in a different script from the + author's other titles (a Cyrillic title in an English author's catalogue, for + example), so such a work stays unknown. OpenLibrary edition sampling only + runs for OpenLibrary works, and takes the language and a missing cover from + the edition OpenLibrary features on the work's own page first, falling back + to a small sample of its editions (and a cover in the sampled language) only + for what that edition lacks. **Reconcile catalogue** treats a failed lookup as indeterminate rather than offering the row for removal. - **Google Books** (free API key) and **Audnexus/Audible** (audiobook narrator, duration, by ASIN) enrich further. diff --git a/internal/api/author_majority_language_test.go b/internal/api/author_majority_language_test.go new file mode 100644 index 000000000..9f875931a --- /dev/null +++ b/internal/api/author_majority_language_test.go @@ -0,0 +1,91 @@ +package api + +import ( + "context" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +// The author-majority language fallback stamps the author's main language on +// works nothing else could resolve. It must not stamp it on a title written in +// another script: that is a translation, and stamping "eng" on it let it past +// even unknown_language_behavior = fail (#3091). A work its provider proved is +// in a non-allowed language is rejected on that evidence, and is not stamped +// either. +func TestFetchAuthorBooks_MajorityLanguageSkipsUnresolvableTranslations(t *testing.T) { + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + + ctx := context.Background() + authorRepo := db.NewAuthorRepo(database) + bookRepo := db.NewBookRepo(database) + profileRepo := db.NewMetadataProfileRepo(database) + profile, err := profileRepo.GetByID(ctx, models.DefaultMetadataProfileID) + if err != nil || profile == nil { + t.Fatalf("GetByID(default profile): profile=%+v err=%v", profile, err) + } + profile.AllowedLanguages = "eng" + profile.UnknownLanguageBehavior = models.UnknownLanguageFail + if err := profileRepo.Update(ctx, profile); err != nil { + t.Fatal(err) + } + author := &models.Author{ + ForeignID: "hc:orson-scott-card", Name: "Orson Scott Card", SortName: "Card, Orson Scott", + MetadataProvider: "hardcover", Monitored: false, + } + if err := authorRepo.Create(ctx, author); err != nil { + t.Fatal(err) + } + work := func(id, title, language string) models.Book { + return models.Book{ + ForeignID: id, Title: title, SortTitle: title, Language: language, + MediaType: models.MediaTypeEbook, Status: models.BookStatusWanted, MetadataProvider: "hardcover", + } + } + provider := &languageEvidenceMetaProvider{ + stubMetaProvider: stubMetaProvider{name: "hardcover", works: []models.Book{ + work("hc:enders-game", "Ender's Game", "eng"), + work("hc:speaker-for-the-dead", "Speaker for the Dead", "eng"), + work("hc:xenocide", "Xenocide", "eng"), + work("hc:children-of-the-mind", "Children of the Mind", "eng"), + work("hc:seventh-son", "Seventh Son", "eng"), + // The provider found only Spanish editions. + work("hc:el-septimo-hijo", "El séptimo hijo", ""), + // No evidence either way, but the title is in another script. + work("hc:rozhbi-na-saznanieto", "Рожби на съзнанието", ""), + // No evidence and a Latin title: the fallback still applies. + work("hc:red-prophet", "Red Prophet", ""), + }}, + evidence: map[string]metadata.AuthorWorkLanguageEvidence{ + "hc:el-septimo-hijo": {State: metadata.AuthorWorkLanguageNotAllowed, Language: "spa"}, + }, + } + h := NewAuthorHandler(authorRepo, nil, bookRepo, nil, metadata.NewAggregator(provider), nil, profileRepo, nil) + h.FetchAuthorBooks(author, false, models.MediaTypeEbook) + + books, err := bookRepo.ListByAuthor(ctx, author.ID) + if err != nil { + t.Fatal(err) + } + got := map[string]string{} + for _, b := range books { + got[b.ForeignID] = b.Language + } + for _, translation := range []string{"hc:el-septimo-hijo", "hc:rozhbi-na-saznanieto"} { + if language, added := got[translation]; added { + t.Errorf("%s added with language %q; a translation must not inherit the author's language", translation, language) + } + } + if got["hc:red-prophet"] != "eng" { + t.Errorf("hc:red-prophet language = %q, want eng from the author majority (books: %v)", got["hc:red-prophet"], got) + } + if len(books) != 6 { + t.Errorf("added %d books, want the five English works and Red Prophet: %v", len(books), got) + } +} diff --git a/internal/api/authors.go b/internal/api/authors.go index 9b867d517..6e14297f7 100644 --- a/internal/api/authors.go +++ b/internal/api/authors.go @@ -1977,7 +1977,7 @@ func (h *AuthorHandler) runCatalogueSync(ctx context.Context, author *models.Aut // OpenLibrary metadata gap it has no way to tell apart from an actual // foreign-language book. if len(allowedLangs) > 0 { - applyAuthorMajorityLanguageFallback(books) + applyAuthorMajorityLanguageFallback(books, languageEvidence) } // Everything above can take minutes for a prolific author (works fetch, @@ -3698,7 +3698,17 @@ const authorMajorityLanguageThreshold = 0.9 // least authorMajorityLanguageMinSample resolved works. Mutates books in // place. A no-op when too few works have a resolved language yet, or when // no single language dominates strongly enough to trust. -func applyAuthorMajorityLanguageFallback(books []models.Book) { +// +// A blank work whose title is written in a different script from the +// majority's titles is left blank (#3091): "Рожби на съзнанието" in an English +// author's catalogue is a translation, and stamping "eng" on it let it past +// even unknown_language_behavior = fail. It keeps an unknown language, so the +// profile's unknown-language rule decides it. So is a work its provider has +// already proved is in a language the profile does not allow: the filter +// rejects it on that evidence anyway, and the skipped sample should not show +// it as English. Indeterminate evidence still gets the fallback, as it did +// before: it means the provider found no edition in any language (#2754). +func applyAuthorMajorityLanguageFallback(books []models.Book, evidence map[string]metadata.AuthorWorkLanguageEvidence) { counts := make(map[string]int, 4) resolved := 0 for _, b := range books { @@ -3721,10 +3731,18 @@ func applyAuthorMajorityLanguageFallback(books []models.Book) { if float64(majorityCount)/float64(resolved) < authorMajorityLanguageThreshold { return } + majorityScript := majorityTitleScript(books, majorityLang) for i := range books { - if books[i].Language == "" { - books[i].Language = majorityLang + if books[i].Language != "" { + continue + } + if found, ok := evidence[strings.TrimSpace(books[i].ForeignID)]; ok && found.State == metadata.AuthorWorkLanguageNotAllowed { + continue + } + if script := titleScript(books[i].Title); script != "" && majorityScript != "" && script != majorityScript { + continue } + books[i].Language = majorityLang } } diff --git a/internal/api/authors_test.go b/internal/api/authors_test.go index 1f2b41781..0e208a50d 100644 --- a/internal/api/authors_test.go +++ b/internal/api/authors_test.go @@ -5551,7 +5551,7 @@ func TestApplyAuthorMajorityLanguageFallback(t *testing.T) { {ForeignID: "5", Language: ""}, // unresolved: should be backfilled {ForeignID: "6", Language: ""}, // unresolved: should be backfilled } - applyAuthorMajorityLanguageFallback(books) + applyAuthorMajorityLanguageFallback(books, nil) for _, b := range books { if b.Language != "eng" { t.Errorf("book %s: Language = %q, want eng", b.ForeignID, b.Language) @@ -5567,7 +5567,7 @@ func TestApplyAuthorMajorityLanguageFallback(t *testing.T) { {ForeignID: "4", Language: "ger"}, {ForeignID: "5", Language: ""}, } - applyAuthorMajorityLanguageFallback(books) + applyAuthorMajorityLanguageFallback(books, nil) if books[4].Language != "" { t.Errorf("Language = %q, want unchanged (empty) — no language clears the dominance threshold", books[4].Language) } @@ -5579,7 +5579,7 @@ func TestApplyAuthorMajorityLanguageFallback(t *testing.T) { {ForeignID: "2", Language: "eng"}, {ForeignID: "3", Language: ""}, } - applyAuthorMajorityLanguageFallback(books) + applyAuthorMajorityLanguageFallback(books, nil) if books[2].Language != "" { t.Errorf("Language = %q, want unchanged (empty) — only 2 resolved works is below the minimum sample", books[2].Language) } @@ -5590,7 +5590,7 @@ func TestApplyAuthorMajorityLanguageFallback(t *testing.T) { {ForeignID: "1", Language: ""}, {ForeignID: "2", Language: ""}, } - applyAuthorMajorityLanguageFallback(books) + applyAuthorMajorityLanguageFallback(books, nil) for _, b := range books { if b.Language != "" { t.Errorf("book %s: Language = %q, want unchanged (empty)", b.ForeignID, b.Language) diff --git a/internal/api/books.go b/internal/api/books.go index a8ca4d1e9..f9690095b 100644 --- a/internal/api/books.go +++ b/internal/api/books.go @@ -1302,6 +1302,17 @@ func (h *BookHandler) Rebind(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusConflict, map[string]string{"error": "This book changed while it was being rebound. Try again.", "reason": conflictReasonChanged}) return } + // The previous work's provider editions describe the wrong work: their + // ISBN would keep steering search, and their unique ids keep them from + // the book they belong to (#2781). Only when the identity actually + // changed, and never an imported, selected or referenced edition. + if h.editions != nil && (oldForeignID != book.ForeignID || oldProvider != book.MetadataProvider) { + if n, err := h.editions.DeleteProviderEditionsAfterRebind(r.Context(), book.ID, book.ForeignID, book.MetadataProvider); err != nil { + slog.Warn("rebind: failed to remove the previous work's editions", "bookId", book.ID, "error", err) + } else if n > 0 { + slog.Info("rebind: removed the previous work's provider editions", "bookId", book.ID, "removed", n, "oldForeignId", oldForeignID) + } + } // MediaType is in the preserved-fields list above because it belongs to the // user, so a rebind is exactly the case the pin exists for: hydration must // not widen the format the user keeps (#2768). diff --git a/internal/api/rebind_editions_test.go b/internal/api/rebind_editions_test.go new file mode 100644 index 000000000..3e03b7dab --- /dev/null +++ b/internal/api/rebind_editions_test.go @@ -0,0 +1,156 @@ +package api + +import ( + "context" + "net/http" + "net/http/httptest" + "sort" + "testing" + + "github.com/vavallee/bindery/internal/db" + "github.com/vavallee/bindery/internal/models" +) + +type rebindEditionsFixture struct { + h *BookHandler + books *db.BookRepo + editions *db.EditionRepo + downloads *db.DownloadRepo + book *models.Book + ctx context.Context +} + +func newRebindEditionsFixture(t *testing.T) rebindEditionsFixture { + t.Helper() + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { database.Close() }) + ctx := context.Background() + books := db.NewBookRepo(database) + authors := db.NewAuthorRepo(database) + editions := db.NewEditionRepo(database) + author := &models.Author{ + ForeignID: "OL1A", Name: "Test Author", SortName: "Author, Test", + MetadataProvider: "openlibrary", Monitored: true, + } + if err := authors.Create(ctx, author); err != nil { + t.Fatal(err) + } + book := &models.Book{ + ForeignID: "OL1W", AuthorID: author.ID, Title: "Wrong Book", SortTitle: "Wrong Book", + Status: models.BookStatusWanted, MetadataProvider: "openlibrary", Monitored: true, Genres: []string{}, + } + if err := books.Create(ctx, book); err != nil { + t.Fatal(err) + } + h := NewBookHandler(books, nil, db.NewHistoryRepo(database), nil). + WithAuthors(authors).WithSeries(db.NewSeriesRepo(database)).WithEditionHydration(editions). + WithMetaLookup(&stubLookup{book: &models.Book{ + Title: "Correct Book", SortTitle: "Correct Book", + Author: &models.Author{ForeignID: author.ForeignID, Name: author.Name}, + }}) + return rebindEditionsFixture{h: h, books: books, editions: editions, downloads: db.NewDownloadRepo(database), book: book, ctx: ctx} +} + +// addProviderEdition persists a metadata provider edition the way hydration +// does, against the book's identity at the time. +func (f rebindEditionsFixture) addProviderEdition(t *testing.T, foreignID, isbn string) *models.Edition { + t.Helper() + e := &models.Edition{ForeignID: foreignID, BookID: f.book.ID, Title: "Wrong Book", ISBN13: &isbn, Monitored: true} + current, err := f.books.GetByID(f.ctx, f.book.ID) + if err != nil { + t.Fatal(err) + } + ok, err := f.editions.UpsertMetadata(f.ctx, e, current.ForeignID, current.MetadataProvider) + if err != nil || !ok { + t.Fatalf("UpsertMetadata(%s) = %v, %v", foreignID, ok, err) + } + return e +} + +func (f rebindEditionsFixture) rebind(t *testing.T, foreignID string) { + t.Helper() + rec := httptest.NewRecorder() + f.h.Rebind(rec, rebindRequest(f.book.ID, map[string]any{"provider": "openlibrary", "foreign_id": foreignID})) + if rec.Code != http.StatusOK { + t.Fatalf("rebind: want 200, got %d: %s", rec.Code, rec.Body.String()) + } +} + +func (f rebindEditionsFixture) editionIDs(t *testing.T) []string { + t.Helper() + editions, err := f.editions.ListByBook(f.ctx, f.book.ID) + if err != nil { + t.Fatal(err) + } + out := make([]string, 0, len(editions)) + for _, e := range editions { + out = append(out, e.ForeignID) + } + sort.Strings(out) + return out +} + +// The sequence from the report: a metadata edition of the wrong work is +// persisted, then the book is rebound. The old edition, and with it the old +// work's ISBN that search would use, must not survive (#2781). +func TestRebind_DiscardsPreviousWorksProviderEditions(t *testing.T) { + f := newRebindEditionsFixture(t) + f.addProviderEdition(t, "OL-old-edition", "9780140328721") + + f.rebind(t, "OL2W") + + if got := f.editionIDs(t); len(got) != 0 { + t.Fatalf("editions after rebind = %v, want the previous work's provider edition gone", got) + } +} + +// What rebind keeps: editions the importers created for the user's files, the +// edition the user selected, an edition a download recorded, and every +// edition when the rebind does not change the identity at all. +func TestRebind_KeepsOwnedAndReferencedEditions(t *testing.T) { + f := newRebindEditionsFixture(t) + for _, id := range []string{"calibre:edition:7:EPUB", "abs:edition:lib:item"} { + isbn := "9780000000001" + if err := f.editions.Upsert(f.ctx, &models.Edition{ForeignID: id, BookID: f.book.ID, Title: "Wrong Book", ISBN13: &isbn, Monitored: true}); err != nil { + t.Fatal(err) + } + } + f.addProviderEdition(t, "OL-unreferenced", "9780000000002") + selected := f.addProviderEdition(t, "OL-selected", "9780000000003") + grabbed := f.addProviderEdition(t, "OL-grabbed", "9780000000004") + + book, err := f.books.GetByID(f.ctx, f.book.ID) + if err != nil { + t.Fatal(err) + } + book.SelectedEditionID = &selected.ID + if err := f.books.Update(f.ctx, book); err != nil { + t.Fatal(err) + } + if err := f.downloads.Create(f.ctx, &models.Download{ + GUID: "grab-1", BookID: &book.ID, EditionID: &grabbed.ID, Title: "Wrong Book", Status: models.DownloadStatusCompleted, + }); err != nil { + t.Fatal(err) + } + + // Same identity: nothing is invalidated. + f.rebind(t, "OL1W") + if got := f.editionIDs(t); len(got) != 5 { + t.Fatalf("editions after a same-work rebind = %v, want all five kept", got) + } + + f.rebind(t, "OL2W") + want := []string{"OL-grabbed", "OL-selected", "abs:edition:lib:item", "calibre:edition:7:EPUB"} + got := f.editionIDs(t) + if len(got) != len(want) { + t.Fatalf("editions after rebind = %v, want %v", got, want) + } + for i := range want { + if got[i] != want[i] { + t.Fatalf("editions after rebind = %v, want %v", got, want) + } + } +} diff --git a/internal/api/title_script.go b/internal/api/title_script.go new file mode 100644 index 000000000..153a9d1d0 --- /dev/null +++ b/internal/api/title_script.go @@ -0,0 +1,82 @@ +package api + +import ( + "unicode" + + "github.com/vavallee/bindery/internal/models" +) + +// titleScripts are the writing systems titleScript tells apart. The question +// it answers is only whether a title is plainly not in the script the +// author's majority-language titles are in (#3091), so a coarse set is +// enough. +// +// Han, Hiragana and Katakana are one bucket, "CJK": a Japanese title mixes all +// three ("ノルウェイの森" is mostly katakana, "騎士団長殺し" all kanji), so +// telling them apart would judge an author's own titles foreign to each other. +// Hangul stays separate. Korean titles are written in it almost entirely, so a +// Japanese or Chinese translation in a Korean catalogue is plainly foreign, +// and a Korean translation in a Japanese one likewise. +var titleScripts = []struct { + name string + table *unicode.RangeTable +}{ + {"Latin", unicode.Latin}, + {"Cyrillic", unicode.Cyrillic}, + {"Greek", unicode.Greek}, + {"Arabic", unicode.Arabic}, + {"Hebrew", unicode.Hebrew}, + {"CJK", unicode.Han}, + {"CJK", unicode.Hiragana}, + {"CJK", unicode.Katakana}, + {"Hangul", unicode.Hangul}, + {"Thai", unicode.Thai}, + {"Devanagari", unicode.Devanagari}, + {"Armenian", unicode.Armenian}, + {"Georgian", unicode.Georgian}, +} + +// titleScript returns the writing system most of title's letters are in, or +// "" when it has no letters from any of titleScripts (digits only, say). +func titleScript(title string) string { + counts := make(map[string]int, 2) + for _, r := range title { + if !unicode.IsLetter(r) { + continue + } + for _, s := range titleScripts { + if unicode.Is(s.table, r) { + counts[s.name]++ + break + } + } + } + return mostCommon(counts) +} + +// majorityTitleScript returns the script most of the titles in language are +// written in, or "" when none of them has a recognisable one. +func majorityTitleScript(books []models.Book, language string) string { + counts := make(map[string]int, 2) + for _, b := range books { + if b.Language != language { + continue + } + if script := titleScript(b.Title); script != "" { + counts[script]++ + } + } + return mostCommon(counts) +} + +// mostCommon returns the key with the highest count, the alphabetically first +// on a tie so the answer does not depend on map order. +func mostCommon(counts map[string]int) string { + best, bestN := "", 0 + for key, n := range counts { + if n > bestN || (n == bestN && key < best) { + best, bestN = key, n + } + } + return best +} diff --git a/internal/api/title_script_test.go b/internal/api/title_script_test.go new file mode 100644 index 000000000..62ddf50c6 --- /dev/null +++ b/internal/api/title_script_test.go @@ -0,0 +1,54 @@ +package api + +import ( + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// Japanese mixes kanji, hiragana and katakana in one title, so the script +// check must read all three as one writing system. Otherwise a katakana heavy +// title such as "ノルウェイの森" is judged foreign to an author whose other +// titles are mostly kanji, loses the majority language, and is rejected under +// unknown_language_behavior = fail. +func TestApplyAuthorMajorityLanguageFallback_JapaneseTitlesShareAScript(t *testing.T) { + books := []models.Book{ + {ForeignID: "1", Title: "騎士団長殺し", Language: "jpn"}, + {ForeignID: "2", Title: "風の歌を聴け", Language: "jpn"}, + {ForeignID: "3", Title: "羊をめぐる冒険", Language: "jpn"}, + {ForeignID: "4", Title: "世界の終りとハードボイルド・ワンダーランド", Language: "jpn"}, + {ForeignID: "5", Title: "ノルウェイの森"}, + {ForeignID: "6", Title: "海辺のカフカ"}, + {ForeignID: "7", Title: "ねじまき鳥クロニクル"}, + {ForeignID: "8", Title: "色彩を持たない多崎つくると、彼の巡礼の年"}, + // A translation in another script is still left unknown. + {ForeignID: "9", Title: "Норвежский лес"}, + } + applyAuthorMajorityLanguageFallback(books, nil) + for _, b := range books[4:8] { + if b.Language != "jpn" { + t.Errorf("%q: Language = %q, want jpn from the author majority", b.Title, b.Language) + } + } + if books[8].Language != "" { + t.Errorf("%q: Language = %q, want it left unknown", books[8].Title, books[8].Language) + } +} + +// Hangul stays its own script: a Japanese or Chinese translation in a Korean +// author's catalogue is plainly not Korean. +func TestTitleScript_GroupsCJKButNotHangul(t *testing.T) { + for _, c := range []struct{ title, want string }{ + {"ノルウェイの森", "CJK"}, + {"騎士団長殺し", "CJK"}, + {"ねじまき鳥クロニクル", "CJK"}, + {"三体", "CJK"}, + {"채식주의자", "Hangul"}, + {"Norwegian Wood", "Latin"}, + {"1984", ""}, + } { + if got := titleScript(c.title); got != c.want { + t.Errorf("titleScript(%q) = %q, want %q", c.title, got, c.want) + } + } +} diff --git a/internal/db/editions.go b/internal/db/editions.go index 5b5ec2bfc..c1e3716d0 100644 --- a/internal/db/editions.go +++ b/internal/db/editions.go @@ -280,6 +280,45 @@ func scanEditionFrom(s rowScanner) (models.Edition, error) { return e, nil } +// DeleteProviderEditionsAfterRebind removes the metadata provider editions a +// book still carries from the work it was bound to before a rebind (#2781). +// Those rows describe the wrong work: search used their ISBN for the corrected +// book, and the unique foreign_id kept the old edition from being attached to +// the book it does belong to. +// +// Only provider editions go. An edition the Calibre or Audiobookshelf importer +// created ("calibre:" and "abs:" ids) records a file the user has, so it is +// kept, and so is any edition something still points at: the book's selected +// edition, which the user chose, and an edition a download or a Calibre +// delivery recorded. Provider editions are cached provider data, refetched by +// the next hydration of the new work, not anything the user entered. +// +// The book must already carry its new identity (foreignID, provider), checked +// in the same statement. A metadata write for the old identity is refused by +// UpsertMetadata's parent check once the identity has changed, so one that +// landed earlier is removed here and one that comes later cannot land: the +// two are consistent whichever order they run in. Returns the rows removed. +func (r *EditionRepo) DeleteProviderEditionsAfterRebind(ctx context.Context, bookID int64, foreignID, provider string) (int64, error) { + res, err := r.exec.ExecContext(ctx, ` + DELETE FROM editions + WHERE book_id = ? + AND foreign_id NOT LIKE 'calibre:%' + AND foreign_id NOT LIKE 'abs:%' + AND EXISTS (SELECT 1 FROM books b WHERE b.id = editions.book_id AND b.foreign_id = ? AND b.metadata_provider = ?) + AND NOT EXISTS (SELECT 1 FROM books b WHERE b.id = editions.book_id AND b.selected_edition_id = editions.id) + AND NOT EXISTS (SELECT 1 FROM downloads d WHERE d.edition_id = editions.id) + AND NOT EXISTS (SELECT 1 FROM calibre_deliveries cd WHERE cd.edition_id = editions.id)`, + bookID, foreignID, provider) + if err != nil { + return 0, fmt.Errorf("delete provider editions for rebound book %d: %w", bookID, err) + } + n, err := res.RowsAffected() + if err != nil { + return 0, fmt.Errorf("provider editions removed for book %d: %w", bookID, err) + } + return n, nil +} + func (r *EditionRepo) Delete(ctx context.Context, id int64) error { _, err := r.exec.ExecContext(ctx, `DELETE FROM editions WHERE id = ?`, id) if err != nil { diff --git a/internal/metadata/aggregator.go b/internal/metadata/aggregator.go index ab733eb8c..b16edbfc8 100644 --- a/internal/metadata/aggregator.go +++ b/internal/metadata/aggregator.go @@ -856,12 +856,17 @@ func (a *Aggregator) GetBookByISBN(ctx context.Context, isbn string) (*models.Bo // Such a record is also not cached, so the next lookup asks the primary again // instead of serving the fallback's answer for the life of the cache entry. A // cache hit therefore never carries a primary failure. +// +// The cache key carries the bound provider scope, so an answer built under one +// configuration (a Hardcover token, a primary switch) is not served after it +// changes, and every caller gets its own copy of the cached book (#2869). func (a *Aggregator) GetBookByISBNWithOutcome(ctx context.Context, isbn string) (*models.Book, SearchOutcome, error) { isbn = isbnutil.Normalize(isbn) - key := "isbn:" + isbn + ctx, scope := a.bindCacheProviders(ctx) + key := "isbn:" + scope + ":" + isbn primaryName := a.PrimaryProviderName() - if cached, ok := a.cache.get(key); ok { - return cached.(*models.Book), SearchOutcome{Primary: primaryName}, nil + if cached, ok := a.cachedBook(key); ok { + return cached, SearchOutcome{Primary: primaryName}, nil } var ( @@ -889,7 +894,8 @@ func (a *Aggregator) GetBookByISBNWithOutcome(ctx context.Context, isbn string) if provider == nil { continue } - book, err := provider.GetBookByISBN(ctx, isbn) + bound, _ := resolveCacheProvider(ctx, provider) + book, err := bound.GetBookByISBN(ctx, isbn) if err != nil { if errors.Is(err, ErrProviderNotConfigured) { skippedUnconfigured = true @@ -945,16 +951,24 @@ func (a *Aggregator) GetBookByISBNWithOutcome(ctx context.Context, isbn string) return nil, outcome, nil } -// cacheISBNBook fills in a thin ISBN record and, when store is true, caches it -// under key. +// cacheISBNBook fills in a thin ISBN record and, when store is true, caches a +// copy of it under key. A record filled in while a provider or enricher was +// failing is incomplete, so it goes in the five minute cache rather than the +// 24 hour one and the failing lookup is retried once per window (#2869). func (a *Aggregator) cacheISBNBook(ctx context.Context, key string, book *models.Book, store bool) *models.Book { + complete := true if book != nil && len(book.Description) < 50 { // Try fetching the full record from the provider before falling back to // enrichers. ISBN search results are often lightweight (title + ForeignID // only); GetBook returns the canonical description, cover, etc. if book.ForeignID != "" { if provider := a.providerForForeignID(book.ForeignID); provider != nil { - if full, err := provider.GetBook(ctx, book.ForeignID); err == nil && full != nil { + provider, _ = resolveCacheProvider(ctx, provider) + full, err := provider.GetBook(ctx, book.ForeignID) + if err != nil && !errors.Is(err, ErrProviderNotConfigured) { + complete = false + } + if err == nil && full != nil { if len(full.Description) > len(book.Description) { book.Description = full.Description } @@ -974,16 +988,43 @@ func (a *Aggregator) cacheISBNBook(ctx context.Context, key string, book *models } } } - if len(book.Description) < 50 { - a.enrichBook(ctx, book) + if len(book.Description) < 50 && !a.enrichBook(ctx, book) { + complete = false } } if store { - a.cache.set(key, book) + a.storeBook(ctx, key, book, complete) } return book } +// cachedBook returns a copy of the book cached under key, from the 24 hour +// cache or the five minute one. A cached nil (a lookup that found nothing) is +// a hit too. +func (a *Aggregator) cachedBook(key string) (*models.Book, bool) { + if cached, ok := a.cache.get(key); ok { + return cloneBook(cached.(*models.Book)), true + } + if cached, ok := a.requests.shortCache().get(key); ok { + return cloneBook(cached.(*models.Book)), true + } + return nil, false +} + +// storeBook caches a copy of book under key, so the caller keeps sole use of +// the pointer it returns. An incomplete book is kept for five minutes only. A +// cancelled request stores nothing, since what it built may be partial. +func (a *Aggregator) storeBook(ctx context.Context, key string, book *models.Book, complete bool) { + if ctx.Err() != nil { + return + } + if complete { + a.cache.set(key, cloneBook(book)) + return + } + a.requests.shortCache().set(key, cloneBook(book)) +} + // GetBookFromProvider fetches a single book by foreign ID from the named // provider ("openlibrary" or "hardcover"). It bypasses the TTL cache so the // rebind flow always gets a fresh record. Returns ErrProviderNotConfigured diff --git a/internal/metadata/aggregator_author_works.go b/internal/metadata/aggregator_author_works.go index e9e0a205f..0d58c77dd 100644 --- a/internal/metadata/aggregator_author_works.go +++ b/internal/metadata/aggregator_author_works.go @@ -114,18 +114,56 @@ type authorWorkLanguageEvidenceProvider interface { GetAuthorWorkLanguageEvidence(ctx context.Context, books []models.Book, allowed []string) (map[string]AuthorWorkLanguageEvidence, error) } -// GetAuthorWorkLanguageEvidence asks the primary provider for bounded, -// profile-specific edition evidence. Unsupported providers return nil. Failed -// lookups never return evidence: a secondary lookup error must leave the -// existing scalar, edition-sampled, and majority-language pipeline unchanged. +// GetAuthorWorkLanguageEvidence asks every provider that can supply it for +// bounded, profile-specific edition evidence: the primary, and any enricher +// whose works were merged into the catalogue. With OpenLibrary primary and +// Hardcover supplementing, the hc: works come from Hardcover and only +// Hardcover can say what language their editions are in, so asking the +// primary alone left every translation it supplied unresolved (#3091). Each +// provider answers only for its own works, so the maps do not overlap; on a +// clash the primary's answer is kept. +// +// Unsupported providers contribute nothing, and nil is returned when none +// supports it. A failed primary lookup returns its error and no evidence, as +// before. A failed or unconfigured enricher costs only that enricher's +// evidence: its works fall back to the scalar, edition-sampled and +// majority-language pipeline unchanged. func (a *Aggregator) GetAuthorWorkLanguageEvidence(ctx context.Context, books []models.Book, allowed []string) (map[string]AuthorWorkLanguageEvidence, error) { - resolver, ok := a.primary.(authorWorkLanguageEvidenceProvider) - if !ok || len(allowed) == 0 { + if len(allowed) == 0 { return nil, nil } - evidence, err := resolver.GetAuthorWorkLanguageEvidence(ctx, books, allowed) - if err != nil { - return nil, err + var evidence map[string]AuthorWorkLanguageEvidence + merge := func(found map[string]AuthorWorkLanguageEvidence) { + if evidence == nil && found != nil { + evidence = make(map[string]AuthorWorkLanguageEvidence, len(found)) + } + for key, value := range found { + if _, exists := evidence[key]; !exists { + evidence[key] = value + } + } + } + if resolver, ok := a.primary.(authorWorkLanguageEvidenceProvider); ok { + found, err := resolver.GetAuthorWorkLanguageEvidence(ctx, books, allowed) + if err != nil { + return nil, err + } + merge(found) + } + for _, enricher := range a.enrichers { + resolver, ok := enricher.(authorWorkLanguageEvidenceProvider) + if !ok { + continue + } + found, err := resolver.GetAuthorWorkLanguageEvidence(ctx, books, allowed) + if err != nil { + if !errors.Is(err, ErrProviderNotConfigured) { + slog.Warn("supplementing provider language evidence lookup failed; its works use the existing language fallbacks", + "provider", enricher.Name(), "error", err) + } + continue + } + merge(found) } return evidence, nil } diff --git a/internal/metadata/aggregator_enrichment.go b/internal/metadata/aggregator_enrichment.go index 91ce5ec96..2bec8c706 100644 --- a/internal/metadata/aggregator_enrichment.go +++ b/internal/metadata/aggregator_enrichment.go @@ -58,9 +58,13 @@ func (a *Aggregator) GetCanonicalBookByASIN(ctx context.Context, asin string) (* if a == nil || a.primary == nil || asin == "" { return nil, nil } - key := "asin-canonical:" + asin - if cached, ok := a.cache.get(key); ok { - return cached.(*models.Book), nil + // Scoped and copied like the ISBN lookup (#2869): an answer built under one + // provider configuration is not served after it changes, and a caller that + // edits the book it gets cannot rewrite the cached entry. + ctx, scope := a.bindCacheProviders(ctx) + key := "asin-canonical:" + scope + ":" + asin + if cached, ok := a.cachedBook(key); ok { + return cached, nil } b, err := a.getAudnexBookByASIN(ctx, asin) @@ -84,10 +88,11 @@ func (a *Aggregator) GetCanonicalBookByASIN(ctx context.Context, asin string) (* a.cache.set(key, noBook) return nil, nil } + complete := true if len(canonical.Description) < 50 || canonical.ImageURL == "" { - a.enrichBook(ctx, canonical) + complete = a.enrichBook(ctx, canonical) } - a.cache.set(key, canonical) + a.storeBook(ctx, key, canonical, complete) return canonical, nil } @@ -304,7 +309,11 @@ func enrichmentInputKey(book *models.Book) string { return fmt.Sprintf("%x", sha256.Sum256([]byte(input))) } -func (a *Aggregator) enrichBook(ctx context.Context, book *models.Book) { +// enrichBook reports whether the enrichment is complete: false when an +// enricher failed, or when the result came from a snapshot built during such a +// failure. A caller caching the enriched book uses it to keep an incomplete +// result in the five minute cache rather than the 24 hour one (#2869). +func (a *Aggregator) enrichBook(ctx context.Context, book *models.Book) bool { ctx, scope := a.bindCacheProviders(ctx) cacheKey := enrichBookCacheKey(book) if cacheKey != "" { @@ -320,11 +329,11 @@ func (a *Aggregator) enrichBook(ctx context.Context, book *models.Book) { if cacheKey != "" { if cached, ok := a.cache.get(cacheKey); ok { applyEnrichmentSnapshot(book, cached.(enrichmentSnapshot)) - return + return true } if cached, ok := a.requests.shortCache().get(cacheKey); ok { applyEnrichmentSnapshot(book, cached.(enrichmentSnapshot)) - return + return false } } @@ -422,6 +431,7 @@ func (a *Aggregator) enrichBook(ctx context.Context, book *models.Book) { a.requests.shortCache().set(cacheKey, snap) } } + return cacheable } // applyEnrichmentSnapshot mirrors the per-field merge rules used by diff --git a/internal/metadata/hardcover/client.go b/internal/metadata/hardcover/client.go index f774484fe..5daef288e 100644 --- a/internal/metadata/hardcover/client.go +++ b/internal/metadata/hardcover/client.go @@ -457,9 +457,98 @@ func (c *Client) GetAuthorWorkLanguageEvidence(ctx context.Context, books []mode } evidence[key] = metadata.AuthorWorkLanguageEvidence{State: metadata.AuthorWorkLanguageAllowed, Language: language} } + c.resolveBlankWorkLanguages(ctx, books, allowed, evidence) return evidence, nil } +// resolveBlankWorkLanguages settles the works the allowed-language query left +// indeterminate only because they carry no language of their own. Hardcover +// sets a work's language from its default ebook or audio edition, which is +// usually blank, so a translation such as "O Ponto Azul-Claro" arrived with no +// language, stayed indeterminate, and the unknown-language default let it in +// (#3091). The query above found none of its editions by an allowed language +// code, so an edition that records a language here is checked against the +// profile: a non-allowed language rejects the work, and an allowed one (a +// language Hardcover records by name only, which the code match misses) keeps +// it. A work with no language recorded on any edition stays indeterminate. +// +// One bounded distinct_on query, like the one above. It is best effort: a +// failure leaves the works indeterminate, which is what they were before it +// ran, rather than discarding the evidence already established. +func (c *Client) resolveBlankWorkLanguages(ctx context.Context, books []models.Book, allowed []string, evidence map[string]metadata.AuthorWorkLanguageEvidence) { + var slugs []string + bookIDs := make([]int, 0) + asked := make(map[string]bool) + for _, book := range books { + key := strings.TrimSpace(book.ForeignID) + current, ok := evidence[key] + if !ok || asked[key] || current.State != metadata.AuthorWorkLanguageIndeterminate || + models.NormalizeLanguageCode(book.Language) != "" { + continue + } + asked[key] = true + id := strings.TrimSpace(strings.TrimPrefix(key, idPrefix)) + slugs = append(slugs, id) + if numericID, ok := hardcoverNumericID(id); ok { + bookIDs = append(bookIDs, numericID) + } + } + if len(slugs) == 0 { + return + } + + const gql = `query GetAuthorWorkKnownLanguages($slugs: [String!]!, $bookIds: [Int!]!, $limit: Int!) { + editions( + where: { + _and: [ + {_or: [{book: {slug: {_in: $slugs}}}, {book_id: {_in: $bookIds}}]}, + {language: {id: {_is_null: false}}} + ] + }, + distinct_on: [book_id], + order_by: [{book_id: asc}, {id: asc}], + limit: $limit + ) { + book { id slug } + language { code2 code3 language } + } + }` + var resp struct { + Data struct { + Editions []struct { + Book hcBookRef `json:"book"` + Language *hcLanguage `json:"language"` + } `json:"editions"` + } `json:"data"` + } + if err := c.query(ctx, gql, map[string]any{ + "slugs": slugs, + "bookIds": bookIDs, + "limit": len(slugs), + }, &resp); err != nil { + slog.Debug("hardcover: known edition language lookup failed; blank-language works stay indeterminate", "works", len(slugs), "error", err) + return + } + for _, edition := range resp.Data.Editions { + key := edition.Book.foreignID() + if !asked[key] { + continue + } + language := hardcoverLanguageName(edition.Language) + if language == "" { + continue + } + state := metadata.AuthorWorkLanguageNotAllowed + if models.IsLanguageAllowed(language, allowed, true) { + // The allowed query matches code2 and code3 only, while this + // normalises the language name too, so an edition recorded as + // "English" with no codes lands here. It is an allowed edition. + state = metadata.AuthorWorkLanguageAllowed + } + evidence[key] = metadata.AuthorWorkLanguageEvidence{State: state, Language: language} + } +} + func (c *Client) GetAuthor(ctx context.Context, foreignID string) (*models.Author, error) { if c.authorizationToken(ctx) == "" { return nil, metadata.ErrProviderNotConfigured diff --git a/internal/metadata/hardcover/client_test.go b/internal/metadata/hardcover/client_test.go index 0ba4ebfab..500106ebc 100644 --- a/internal/metadata/hardcover/client_test.go +++ b/internal/metadata/hardcover/client_test.go @@ -790,7 +790,6 @@ func TestGetAuthorWorksByName_NoTokenSkipsRequest(t *testing.T) { func TestGetAuthorWorkLanguageEvidence_BatchesAllowedEditionLookup(t *testing.T) { requests := 0 c := newMockClient(func(r *http.Request) (*http.Response, error) { - requests++ body, err := io.ReadAll(r.Body) if err != nil { t.Fatal(err) @@ -799,6 +798,12 @@ func TestGetAuthorWorkLanguageEvidence_BatchesAllowedEditionLookup(t *testing.T) if err := json.Unmarshal(body, &req); err != nil { t.Fatal(err) } + if strings.Contains(req.Query, "GetAuthorWorkKnownLanguages") { + // The follow up for blank-language works (#3091), covered by + // its own tests. Finding nothing leaves hc:unknown as it was. + return gqlResponse(t, http.StatusOK, map[string]interface{}{"editions": []interface{}{}}), nil + } + requests++ for _, fragment := range []string{ "distinct_on: [book_id]", "order_by: [{book_id: asc}, {id: asc}]", diff --git a/internal/metadata/hardcover/language_evidence_known_test.go b/internal/metadata/hardcover/language_evidence_known_test.go new file mode 100644 index 000000000..217a585ed --- /dev/null +++ b/internal/metadata/hardcover/language_evidence_known_test.go @@ -0,0 +1,151 @@ +package hardcover + +import ( + "context" + "encoding/json" + "io" + "net/http" + "reflect" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/metadata" + "github.com/vavallee/bindery/internal/models" +) + +// knownLanguageEvidenceClient answers the allowed-language lookup with +// allowed, and the follow up for blank-language works with known (or an +// error when knownStatus is not 200). +func knownLanguageEvidenceClient(t *testing.T, allowed, known []map[string]interface{}, knownStatus int, knownRequests *[]map[string]interface{}) *Client { + return newMockClient(func(r *http.Request) (*http.Response, error) { + body, err := io.ReadAll(r.Body) + if err != nil { + t.Fatal(err) + } + var req gqlRequest + if err := json.Unmarshal(body, &req); err != nil { + t.Fatal(err) + } + if strings.Contains(req.Query, "GetAuthorWorkKnownLanguages") { + *knownRequests = append(*knownRequests, req.Variables) + if knownStatus != http.StatusOK { + return gqlResponse(t, knownStatus, `{"error":"upstream unavailable"}`), nil + } + return gqlResponse(t, http.StatusOK, map[string]interface{}{"editions": known}), nil + } + return gqlResponse(t, http.StatusOK, map[string]interface{}{"editions": allowed}), nil + }) +} + +func evidenceEdition(id int, slug, code2, code3, name string) map[string]interface{} { + return map[string]interface{}{ + "book": map[string]interface{}{"id": id, "slug": slug}, + "language": map[string]interface{}{"code2": code2, "code3": code3, "language": name}, + } +} + +// A Hardcover work with no default-edition language and no edition in an +// allowed language, but with editions in another language, is a translation: +// it is rejected rather than left indeterminate (#3091). Before, a blank +// default edition language made every such work indeterminate, so the +// unknown-language "pass" default let translations in by the thousand. +func TestGetAuthorWorkLanguageEvidence_BlankLanguageResolvedFromKnownEditions(t *testing.T) { + var knownRequests []map[string]interface{} + c := knownLanguageEvidenceClient(t, + []map[string]interface{}{evidenceEdition(2, "pale-blue-dot", "en", "eng", "English")}, + []map[string]interface{}{ + evidenceEdition(1, "o-ponto-azul-claro", "pt", "por", "Portuguese"), + // Not a requested work: ignored. + evidenceEdition(9, "not-requested", "fr", "fre", "French"), + }, + http.StatusOK, &knownRequests) + + books := []models.Book{ + {ForeignID: "hc:o-ponto-azul-claro"}, + {ForeignID: "hc:pale-blue-dot"}, + {ForeignID: "hc:no-language-anywhere"}, + {ForeignID: "hc:spanish-default", Language: "spa"}, + } + got, err := c.GetAuthorWorkLanguageEvidence(context.Background(), books, []string{"eng"}) + if err != nil { + t.Fatal(err) + } + want := map[string]metadata.AuthorWorkLanguageEvidence{ + "hc:o-ponto-azul-claro": {State: metadata.AuthorWorkLanguageNotAllowed, Language: "por"}, + "hc:pale-blue-dot": {State: metadata.AuthorWorkLanguageAllowed, Language: "eng"}, + "hc:no-language-anywhere": {State: metadata.AuthorWorkLanguageIndeterminate}, + "hc:spanish-default": {State: metadata.AuthorWorkLanguageNotAllowed, Language: "spa"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("evidence = %#v, want %#v", got, want) + } + // Only the works still undecided with no language of their own are asked + // about: the allowed one and the one with a default language are not. + if len(knownRequests) != 1 { + t.Fatalf("known-language lookups = %d, want 1", len(knownRequests)) + } + slugs, _ := knownRequests[0]["slugs"].([]interface{}) + if len(slugs) != 2 || slugs[0] != "o-ponto-azul-claro" || slugs[1] != "no-language-anywhere" { + t.Fatalf("known-language lookup asked about %v, want the two blank, undecided works", slugs) + } + if limit := knownRequests[0]["limit"]; limit != float64(2) { + t.Fatalf("known-language lookup limit = %v, want 2", limit) + } +} + +// The allowed-language query matches on code2 and code3 only, so an edition +// whose language Hardcover records by name ("English", no codes) is missed by +// it. The follow up normalises the name and must not then reject the work for +// being in the very language the profile allows. +func TestGetAuthorWorkLanguageEvidence_KnownLanguageThatIsAllowedIsNotRejected(t *testing.T) { + var knownRequests []map[string]interface{} + c := knownLanguageEvidenceClient(t, + nil, + []map[string]interface{}{ + evidenceEdition(1, "named-english", "", "", "English"), + evidenceEdition(2, "o-ponto-azul-claro", "pt", "por", "Portuguese"), + }, + http.StatusOK, &knownRequests) + + got, err := c.GetAuthorWorkLanguageEvidence(context.Background(), []models.Book{ + {ForeignID: "hc:named-english"}, + {ForeignID: "hc:o-ponto-azul-claro"}, + }, []string{"eng"}) + if err != nil { + t.Fatal(err) + } + want := map[string]metadata.AuthorWorkLanguageEvidence{ + "hc:named-english": {State: metadata.AuthorWorkLanguageAllowed, Language: "eng"}, + "hc:o-ponto-azul-claro": {State: metadata.AuthorWorkLanguageNotAllowed, Language: "por"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("evidence = %#v, want %#v", got, want) + } +} + +// The follow up is best effort: if it fails, the works it would have resolved +// stay indeterminate and the allowed-language evidence still stands. +func TestGetAuthorWorkLanguageEvidence_KnownLanguageLookupFailureKeepsEvidence(t *testing.T) { + var knownRequests []map[string]interface{} + c := knownLanguageEvidenceClient(t, + []map[string]interface{}{evidenceEdition(2, "pale-blue-dot", "en", "eng", "English")}, + nil, http.StatusInternalServerError, &knownRequests) + + got, err := c.GetAuthorWorkLanguageEvidence(context.Background(), []models.Book{ + {ForeignID: "hc:o-ponto-azul-claro"}, + {ForeignID: "hc:pale-blue-dot"}, + }, []string{"eng"}) + if err != nil { + t.Fatalf("a failed follow up failed the whole lookup: %v", err) + } + want := map[string]metadata.AuthorWorkLanguageEvidence{ + "hc:o-ponto-azul-claro": {State: metadata.AuthorWorkLanguageIndeterminate}, + "hc:pale-blue-dot": {State: metadata.AuthorWorkLanguageAllowed, Language: "eng"}, + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("evidence = %#v, want %#v", got, want) + } + if len(knownRequests) == 0 { + t.Fatal("follow up never attempted") + } +} diff --git a/internal/metadata/isbn_cache_test.go b/internal/metadata/isbn_cache_test.go new file mode 100644 index 000000000..a60c8f8b3 --- /dev/null +++ b/internal/metadata/isbn_cache_test.go @@ -0,0 +1,205 @@ +package metadata + +import ( + "context" + "errors" + "strings" + "testing" + "testing/synctest" + "time" + + "github.com/vavallee/bindery/internal/metadata/audnex" + "github.com/vavallee/bindery/internal/models" +) + +const isbnCacheTestDescription = "A description long enough that the ISBN path skips enrichment entirely." + +// Every caller of an ISBN lookup gets its own copy of the cached book: editing +// the first answer must not rewrite what the next caller is handed (#2869). +func TestGetBookByISBN_CacheHitIsACopy(t *testing.T) { + primary := &mockProvider{name: "openlibrary", getByISBN: &models.Book{ + ForeignID: "OL1W", + Title: "Dune", + Description: isbnCacheTestDescription, + Genres: []string{"Science Fiction"}, + Author: &models.Author{Name: "Frank Herbert"}, + }} + a := newTestAggregator(primary) + + first, err := a.GetBookByISBN(context.Background(), "9780441172719") + if err != nil || first == nil { + t.Fatalf("first lookup: %+v %v", first, err) + } + first.Genres[0] = "mutated" + first.OwnerUserID = 42 + first.Title = "mutated" + first.Author.Name = "mutated" + + second, err := a.GetBookByISBN(context.Background(), "9780441172719") + if err != nil || second == nil { + t.Fatalf("second lookup: %+v %v", second, err) + } + if primary.getByISBNCalls != 1 { + t.Fatalf("provider asked %d times, want 1 (second lookup should be a cache hit)", primary.getByISBNCalls) + } + if second.Genres[0] != "Science Fiction" || second.OwnerUserID != 0 || second.Title != "Dune" || second.Author.Name != "Frank Herbert" { + t.Fatalf("cache entry rewritten through the first caller's book: %+v author=%+v", second, second.Author) + } + // And a hit is not shared between two hit callers either. + second.Genres[0] = "mutated again" + third, _ := a.GetBookByISBN(context.Background(), "9780441172719") + if third.Genres[0] != "Science Fiction" { + t.Fatalf("cache entry rewritten through a cache hit: %v", third.Genres) + } +} + +// isbnScopedProvider stands in for a provider whose configuration (a token, +// a primary switch) changes between two lookups. +type isbnScopedProvider struct { + *mockProvider + scope string +} + +func (p *isbnScopedProvider) ResolveCacheProvider(context.Context) (Provider, string) { + return &mockProvider{name: p.name, getByISBN: &models.Book{ + ForeignID: "OL1W", Title: "answer under " + p.scope, Description: isbnCacheTestDescription, + }}, p.scope +} + +// An ISBN answer built under one provider configuration is not served after +// the configuration changes (#2869). +func TestGetBookByISBN_KeyedByProviderScope(t *testing.T) { + primary := &isbnScopedProvider{mockProvider: &mockProvider{name: "openlibrary"}, scope: "first"} + a := newTestAggregator(primary) + for _, scope := range []string{"first", "second", "first"} { + primary.scope = scope + book, err := a.GetBookByISBN(context.Background(), "9780441172719") + if err != nil || book == nil || book.Title != "answer under "+scope { + t.Fatalf("scope %q: got %+v %v", scope, book, err) + } + } +} + +// An ISBN record built while an enricher was failing is incomplete, so it is +// kept for the five minute window rather than the 24 hour one (#2869). +func TestGetBookByISBN_IncompleteEnrichmentIsShortCached(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + primary := &mockProvider{name: "openlibrary", getByISBN: &models.Book{ + ForeignID: "OL1W", Title: "Dune", Author: &models.Author{Name: "Frank Herbert"}, + }} + enricher := &mockProvider{name: "hardcover", searchBookErr: errors.New("rate limited")} + a := newRequestTestAggregator(primary) + a.enrichers = []Provider{enricher} + + book, err := a.GetBookByISBN(context.Background(), "9780441172719") + if err != nil || book == nil || book.Description != "" { + t.Fatalf("first lookup: %+v %v", book, err) + } + if _, err := a.GetBookByISBN(context.Background(), "9780441172719"); err != nil { + t.Fatal(err) + } + if primary.getByISBNCalls != 1 { + t.Fatalf("incomplete record not cached at all: %d provider calls", primary.getByISBNCalls) + } + + enricher.searchBookErr = nil + enricher.searchBooks = []models.Book{{Title: "Dune", Author: &models.Author{Name: "Frank Herbert"}, Description: isbnCacheTestDescription}} + time.Sleep(5*time.Minute + time.Nanosecond) + book, err = a.GetBookByISBN(context.Background(), "9780441172719") + if err != nil || book == nil || book.Description != isbnCacheTestDescription { + t.Fatalf("record built during an enricher failure was pinned past the short window: %+v %v", book, err) + } + + // The complete record now holds for the long TTL. + time.Sleep(5*time.Minute + time.Nanosecond) + if _, err := a.GetBookByISBN(context.Background(), "9780441172719"); err != nil { + t.Fatal(err) + } + if primary.getByISBNCalls != 2 { + t.Fatalf("complete record not cached for the long TTL: %d provider calls", primary.getByISBNCalls) + } + }) +} + +func canonicalASINTestAggregator(primary Provider) (*Aggregator, *stubAudnexClient) { + audnexClient := &stubAudnexClient{books: map[string]*audnex.Book{ + "B0D3R3MTLM": { + ASIN: "B0D3R3MTLM", + Title: "Iron Flame", + Authors: []audnex.Person{{Name: "Rebecca Yarros"}}, + Language: "English", + }, + }} + return newTestAggregator(primary).WithAudnexClient(audnexClient), audnexClient +} + +func canonicalASINTestPrimary(name string) *mockProvider { + return &mockProvider{ + name: name, + searchBooksByQuery: map[string][]models.Book{ + "Iron Flame Rebecca Yarros": {{ + ForeignID: "OL-IRON", Title: "Iron Flame", EditionCount: 42, + Author: &models.Author{Name: "Rebecca Yarros"}, + }}, + }, + getBookByID: map[string]*models.Book{ + "OL-IRON": { + ForeignID: "OL-IRON", Title: "Iron Flame", Description: strings.Repeat("Canonical. ", 10), + ImageURL: "cover", Genres: []string{"Fantasy"}, MetadataProvider: "openlibrary", + Author: &models.Author{Name: "Rebecca Yarros"}, + }, + }, + } +} + +// The canonical ASIN lookup has the same shape and gets the same treatment: +// a caller editing its answer does not rewrite the cache (#2869). +func TestGetCanonicalBookByASIN_CacheHitIsACopy(t *testing.T) { + a, _ := canonicalASINTestAggregator(canonicalASINTestPrimary("openlibrary")) + first, err := a.GetCanonicalBookByASIN(context.Background(), "B0D3R3MTLM") + if err != nil || first == nil { + t.Fatalf("first lookup: %+v %v", first, err) + } + first.Genres[0] = "mutated" + first.Title = "mutated" + + second, err := a.GetCanonicalBookByASIN(context.Background(), "B0D3R3MTLM") + if err != nil || second == nil { + t.Fatalf("second lookup: %+v %v", second, err) + } + if second.Title != "Iron Flame" || second.Genres[0] != "Fantasy" { + t.Fatalf("cache entry rewritten through the first caller's book: %+v", second) + } +} + +type canonicalScopedProvider struct { + *mockProvider + scope string + byScope map[string]*mockProvider +} + +func (p *canonicalScopedProvider) ResolveCacheProvider(context.Context) (Provider, string) { + return p.byScope[p.scope], p.scope +} + +// A canonical ASIN answer built under one provider configuration is not +// served after it changes (#2869). +func TestGetCanonicalBookByASIN_KeyedByProviderScope(t *testing.T) { + firstPrimary := canonicalASINTestPrimary("openlibrary") + secondPrimary := canonicalASINTestPrimary("openlibrary") + secondPrimary.searchBooksByQuery["Iron Flame Rebecca Yarros"][0].ForeignID = "OL-IRON-2" + primary := &canonicalScopedProvider{ + mockProvider: &mockProvider{name: "openlibrary"}, + scope: "first", + byScope: map[string]*mockProvider{"first": firstPrimary, "second": secondPrimary}, + } + a, _ := canonicalASINTestAggregator(primary) + if book, err := a.GetCanonicalBookByASIN(context.Background(), "B0D3R3MTLM"); err != nil || book == nil || book.ForeignID != "OL-IRON" { + t.Fatalf("first scope: %+v %v", book, err) + } + primary.scope = "second" + book, err := a.GetCanonicalBookByASIN(context.Background(), "B0D3R3MTLM") + if err != nil || book == nil || book.ForeignID != "OL-IRON-2" { + t.Fatalf("second scope served the first configuration's answer: %+v %v", book, err) + } +} diff --git a/internal/metadata/language_evidence_supplement_test.go b/internal/metadata/language_evidence_supplement_test.go new file mode 100644 index 000000000..a3d97de43 --- /dev/null +++ b/internal/metadata/language_evidence_supplement_test.go @@ -0,0 +1,57 @@ +package metadata + +import ( + "context" + "errors" + "reflect" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// With OpenLibrary primary and Hardcover supplementing, the hc: works in the +// catalogue still get Hardcover's language evidence (#3091). Before, only the +// primary was asked, so a supplement's translations were never resolved. +func TestAggregator_GetAuthorWorkLanguageEvidence_AsksSupplementingProviders(t *testing.T) { + want := map[string]AuthorWorkLanguageEvidence{ + "hc:o-ponto-azul-claro": {State: AuthorWorkLanguageNotAllowed, Language: "por"}, + "hc:pale-blue-dot": {State: AuthorWorkLanguageAllowed, Language: "eng"}, + } + hardcover := &mockAuthorWorkLanguageEvidenceProvider{mockProvider: mockProvider{name: "hardcover"}, evidence: want} + agg := newTestAggregator(&mockProvider{name: "openlibrary"}, &mockProvider{name: "googlebooks"}, hardcover) + + books := []models.Book{{ForeignID: "OL1W"}, {ForeignID: "hc:o-ponto-azul-claro"}, {ForeignID: "hc:pale-blue-dot"}} + got, err := agg.GetAuthorWorkLanguageEvidence(context.Background(), books, []string{"eng"}) + if err != nil { + t.Fatal(err) + } + if hardcover.calls != 1 { + t.Fatalf("supplementing Hardcover asked %d times, want 1", hardcover.calls) + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("evidence = %#v, want %#v", got, want) + } +} + +// A supplement's failure, or a supplement with no token, costs only that +// supplement's evidence: the primary's evidence still stands, and no error is +// returned for something the primary did not do. +func TestAggregator_GetAuthorWorkLanguageEvidence_SupplementFailureIsNotFatal(t *testing.T) { + primaryEvidence := map[string]AuthorWorkLanguageEvidence{"p:1": {State: AuthorWorkLanguageAllowed, Language: "eng"}} + primary := &mockAuthorWorkLanguageEvidenceProvider{mockProvider: mockProvider{name: "primary"}, evidence: primaryEvidence} + for _, supplementErr := range []error{errors.New("rate limited"), ErrProviderNotConfigured} { + supplement := &mockAuthorWorkLanguageEvidenceProvider{ + mockProvider: mockProvider{name: "hardcover"}, + evidence: map[string]AuthorWorkLanguageEvidence{"hc:x": {State: AuthorWorkLanguageNotAllowed}}, + err: supplementErr, + } + agg := newTestAggregator(primary, supplement) + got, err := agg.GetAuthorWorkLanguageEvidence(context.Background(), []models.Book{{ForeignID: "p:1"}, {ForeignID: "hc:x"}}, []string{"eng"}) + if err != nil { + t.Fatalf("%v: supplement failure surfaced as an error: %v", supplementErr, err) + } + if !reflect.DeepEqual(got, primaryEvidence) { + t.Fatalf("%v: evidence = %#v, want only the primary's", supplementErr, got) + } + } +} diff --git a/internal/metadata/openlibrary/client.go b/internal/metadata/openlibrary/client.go index dda18d996..98df7420b 100644 --- a/internal/metadata/openlibrary/client.go +++ b/internal/metadata/openlibrary/client.go @@ -76,7 +76,9 @@ const ( // The cost of that is accuracy. OpenLibrary returns editions in no meaningful // order, so a handful of them is a sample rather than the leading editions, and // a work whose first few happen to be translations can be read as -// foreign-language or pick up a foreign cover (#1779, still open on that half). +// foreign-language or pick up a foreign cover. That is why the sample is now +// the fallback: sampleWorkEditions asks the work's featured cover_edition +// first and only samples for what it does not supply (#1779). // // The two derivations used to be separate samplers with separate caches, which // meant a work missing both language and cover cost TWO round trips to the @@ -273,6 +275,18 @@ func (c *Client) GetBook(ctx context.Context, foreignID string) (*models.Book, e if len(resp.Covers) > 0 && resp.Covers[0] > 0 { b.ImageURL = fmt.Sprintf("%s/b/id/%d-L.jpg", coverURL, resp.Covers[0]) + } else if resp.CoverEdition != nil { + // A work record often has no cover of its own while the edition its + // page features does (#1779). One request, only in that case, and + // best effort: a failure leaves the cover to the enrichers as before. + if key := strings.TrimPrefix(strings.TrimSpace(resp.CoverEdition.Key), "/books/"); key != "" { + var featured editionEntry + if err := c.getJSON(ctx, fmt.Sprintf("%s/books/%s.json", baseURL, key), &featured); err != nil { + slog.Debug("openlibrary: cover edition fetch failed", "work", foreignID, "edition", key, "error", err) + } else { + b.ImageURL = firstEditionCover([]editionEntry{featured}) + } + } } // Parse series membership. @@ -868,7 +882,7 @@ func editionFromEntry(e editionEntry) models.Edition { func (c *Client) FillMissingWorkLanguages(ctx context.Context, books []models.Book) int { targets := make([]int, 0, len(books)) for i := range books { - if books[i].Language != "" || books[i].ForeignID == "" { + if books[i].Language != "" || !ownsWork(books[i]) { continue } targets = append(targets, i) @@ -884,32 +898,135 @@ func (c *Client) FillMissingWorkLanguages(ctx context.Context, books []models.Bo return int(filled.Load()) } -// sampleWorkEditions returns the language and cover derived from a work's first -// editionSampleCap editions. Results (including the all-empty result) are cached -// per work ID, so the language and cover derivations cost ONE round trip between -// them rather than one each (#1888). +// ownsWork reports whether book is an OpenLibrary work, the only kind with a +// /works/{id}/editions.json to sample. A work merged in from another provider +// (Hardcover "hc:", Audible "audible:", DNB "dnb:") is skipped: asking +// OpenLibrary for it is a guaranteed "not found", one wasted request per work +// on every pass (#3091). The provider prefix is the test, with the book's +// MetadataProvider as a second one for a supplement that left the prefix off. +func ownsWork(book models.Book) bool { + id := strings.TrimSpace(book.ForeignID) + if id == "" || strings.Contains(id, ":") { + return false + } + p := strings.TrimSpace(book.MetadataProvider) + return p == "" || strings.EqualFold(p, "openlibrary") +} + +// sampleWorkEditions returns the language and cover derived for a work. +// Results (including the all-empty result) are cached per work ID, so the +// language and cover derivations share one derivation between them rather +// than paying for one each (#1888). +// +// The work's cover_edition comes first (#1779). It is the edition +// OpenLibrary's own work page features, so it is what a user comparing +// Bindery with openlibrary.org expects, and unlike the editions endpoint, +// whose order means nothing, it does not change with how many translations +// a work has. The work record and that one edition are both plain document +// fetches. Only what the cover edition does not supply is then taken from an +// editionSampleCap sample of the editions list, as before: the majority +// language, and a cover from an edition in that language where the sample has +// one. A work-level cover, which the work record carries for free, is +// preferred to a sampled one. +// +// Cost, against the single editions sample this used to make: up to two +// extra requests per sampled work. +// - featured edition with both a language and a cover: work record and +// featured edition, two requests (the sample is skipped). +// - featured edition missing either one: work record, featured edition and +// the sample, three requests. +// - no featured edition, or the work record fails: work record and the +// sample, two requests. +// +// Each is made once per work per run and shared by both derivations. func (c *Client) sampleWorkEditions(ctx context.Context, workID string) workEditionSample { if sample, ok := c.cachedWorkSample(workID); ok { return sample } + var sample workEditionSample + var work workResponse + if err := c.getJSON(ctx, fmt.Sprintf("%s/works/%s.json", baseURL, workID), &work); err != nil { + slog.Debug("openlibrary: work record fetch for edition sampling failed", "work", workID, "error", err) + } else { + if work.CoverEdition != nil { + if key := strings.TrimPrefix(strings.TrimSpace(work.CoverEdition.Key), "/books/"); key != "" { + var featured editionEntry + if err := c.getJSON(ctx, fmt.Sprintf("%s/books/%s.json", baseURL, key), &featured); err != nil { + slog.Debug("openlibrary: cover edition fetch failed", "work", workID, "edition", key, "error", err) + } else { + sample.language = majorityEditionLanguage([]editionEntry{featured}) + sample.cover = firstEditionCover([]editionEntry{featured}) + } + } + } + if sample.cover == "" && len(work.Covers) > 0 && work.Covers[0] > 0 { + sample.cover = fmt.Sprintf("%s/b/id/%d-L.jpg", coverURL, work.Covers[0]) + } + } + if sample.language != "" && sample.cover != "" { + c.setCachedWorkSample(workID, sample) + return sample + } + u := fmt.Sprintf("%s/works/%s/editions.json?limit=%d", baseURL, workID, editionSampleCap) var resp editionsResponse if err := c.getJSON(ctx, u, &resp); err != nil { slog.Debug("openlibrary: edition sampling failed", "work", workID, "error", err) - // Cache the miss so we don't retry a flaky/expensive call this run. - c.setCachedWorkSample(workID, workEditionSample{}) - return workEditionSample{} + // Cache what we have, a miss included, so a flaky or expensive call + // is not retried this run. + c.setCachedWorkSample(workID, sample) + return sample } - sample := workEditionSample{ - language: majorityEditionLanguage(resp.Entries), - cover: firstEditionCover(resp.Entries), + if sample.language == "" { + sample.language = majorityEditionLanguage(resp.Entries) + } + if sample.cover == "" { + sample.cover = editionCoverInLanguage(resp.Entries, sample.language) } c.setCachedWorkSample(workID, sample) return sample } +// editionCoverInLanguage returns the cover of the first sampled edition in +// language that has one, so a work read as English does not take the cover +// of a translation that happened to come first (#1779). Failing that it takes +// the first cover of an edition with no language recorded, and only then any +// cover at all, as firstEditionCover always did. +func editionCoverInLanguage(entries []editionEntry, language string) string { + if language != "" { + var inLanguage, unknown []editionEntry + for _, e := range entries { + switch langs := entryLanguages(e); { + case len(langs) == 0: + unknown = append(unknown, e) + case slices.Contains(langs, language): + inLanguage = append(inLanguage, e) + } + } + if cover := firstEditionCover(inLanguage); cover != "" { + return cover + } + if cover := firstEditionCover(unknown); cover != "" { + return cover + } + } + return firstEditionCover(entries) +} + +// entryLanguages returns an edition's language codes ("eng"), without the +// "/languages/" prefix. +func entryLanguages(e editionEntry) []string { + out := make([]string, 0, len(e.Languages)) + for _, l := range e.Languages { + if code := strings.TrimPrefix(l.Key, "/languages/"); code != "" { + out = append(out, code) + } + } + return out +} + // majorityEditionLanguage returns the most common language across the sampled // editions, preferring "eng" on a tie so an author's English work isn't // reclassified by a couple of foreign editions. Empty languages are ignored; @@ -974,10 +1091,7 @@ func (c *Client) setCachedWorkSample(workID string, sample workEditionSample) { func (c *Client) FillMissingWorkCovers(ctx context.Context, books []models.Book) int { targets := make([]int, 0, len(books)) for i := range books { - if books[i].ImageURL != "" || books[i].ForeignID == "" { - continue - } - if p := books[i].MetadataProvider; p != "" && p != "openlibrary" { + if books[i].ImageURL != "" || !ownsWork(books[i]) { continue } targets = append(targets, i) @@ -999,12 +1113,9 @@ func (c *Client) FillMissingWorkCovers(ctx context.Context, books []models.Book) // It used to say the entries arrive "most-held printings first". They do not: // /works/{id}/editions.json takes no sort parameter and its order is neither // publication nor popularity order, so which edition this lands on is not -// meaningful (#1779). It is still a reasonable cover for a work that has none -// of its own, which is the only situation it is consulted in, but a work whose -// arbitrary first editions are a translation can pick up that translation's -// cover. Making the choice deterministic is the open half of #1779 and needs a -// wider sample than editionSampleCap, which is a cost decision rather than a -// one-line change. +// meaningful (#1779). sampleWorkEditions therefore asks the work's +// cover_edition first and, failing that, prefers an edition in the sampled +// language (editionCoverInLanguage); this is the last resort. func firstEditionCover(entries []editionEntry) string { for _, e := range entries { if len(e.Covers) > 0 && e.Covers[0] > 0 { diff --git a/internal/metadata/openlibrary/cover_edition_test.go b/internal/metadata/openlibrary/cover_edition_test.go new file mode 100644 index 000000000..9645ef909 --- /dev/null +++ b/internal/metadata/openlibrary/cover_edition_test.go @@ -0,0 +1,154 @@ +package openlibrary + +import ( + "context" + "net/http" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +func coverLangEntry(key, language string, cover int) editionEntry { + e := editionEntry{Key: "/books/" + key} + if language != "" { + e.Languages = []struct { + Key string `json:"key"` + }{{Key: "/languages/" + language}} + } + if cover > 0 { + e.Covers = []int{cover} + } + return e +} + +// The work's cover_edition, the edition OpenLibrary's own page features, is +// the canonical edition for the derived language and cover (#1779). The +// editions endpoint returns no meaningful order, so its first few entries can +// all be translations: here they are French, and the work used to be read as +// French and given a French cover. +func TestSampleWorkEditions_PrefersCoverEdition(t *testing.T) { + c := newClientWithPaths(t, map[string]interface{}{ + "/works/OL45804W.json": `{"key":"/works/OL45804W","title":"Fantastic Mr Fox","cover_edition":{"key":"/books/OL7M"}}`, + "/books/OL7M.json": jsonStr(coverLangEntry("OL7M", "eng", 77)), + "/works/OL45804W/editions.json": jsonStr(editionsResponse{Entries: []editionEntry{ + coverLangEntry("OL1M", "fre", 11), + coverLangEntry("OL2M", "fre", 12), + coverLangEntry("OL3M", "fre", 13), + }}), + }) + c.workSampleCache = map[string]workEditionSample{} + + books := []models.Book{{ForeignID: "OL45804W", Title: "Fantastic Mr Fox"}} + c.FillMissingWorkLanguages(context.Background(), books) + c.FillMissingWorkCovers(context.Background(), books) + + if books[0].Language != "eng" { + t.Errorf("Language = %q, want eng from the cover edition", books[0].Language) + } + if want := coverURL + "/b/id/77-L.jpg"; books[0].ImageURL != want { + t.Errorf("ImageURL = %q, want the cover edition's %q", books[0].ImageURL, want) + } +} + +// The most expensive path, pinned: the featured edition carries a language but +// no cover, so the sample is still needed for the cover. Work record, cover +// edition and editions sample, three requests, each made once and shared by +// both samplers (#1779). +func TestSampleWorkEditions_ThreeRequestPath(t *testing.T) { + calls := map[string]int{} + count := func(body string) func(*http.Request) string { + return func(r *http.Request) string { + calls[r.URL.Path]++ + return body + } + } + c := newClientWithPaths(t, map[string]interface{}{ + "/works/OL9W.json": count(`{"key":"/works/OL9W","title":"Work","cover_edition":{"key":"/books/OL90M"}}`), + "/books/OL90M.json": count(jsonStr(coverLangEntry("OL90M", "eng", 0))), + "/works/OL9W/editions.json": count(jsonStr(editionsResponse{Entries: []editionEntry{coverLangEntry("OL91M", "fre", 91), coverLangEntry("OL92M", "eng", 92)}})), + }) + c.workSampleCache = map[string]workEditionSample{} + + books := []models.Book{{ForeignID: "OL9W", Title: "Work"}} + c.FillMissingWorkLanguages(context.Background(), books) + c.FillMissingWorkCovers(context.Background(), books) + + if books[0].Language != "eng" { + t.Errorf("Language = %q, want eng from the cover edition", books[0].Language) + } + if want := coverURL + "/b/id/92-L.jpg"; books[0].ImageURL != want { + t.Errorf("ImageURL = %q, want the sampled English edition's %q", books[0].ImageURL, want) + } + total := 0 + for path, n := range calls { + total += n + if n != 1 { + t.Errorf("%s requested %d times, want once", path, n) + } + } + if total != 3 { + t.Errorf("requests = %d (%v), want 3: work record, cover edition, editions sample", total, calls) + } +} + +// GetBook takes the featured edition's cover when the work record carries no +// cover of its own (#1779), costing one request only in that case. +func TestGetBook_UsesCoverEditionWhenWorkHasNoCover(t *testing.T) { + featured := 0 + c := newClientWithPaths(t, map[string]interface{}{ + "/works/OL5W.json": `{"key":"/works/OL5W","title":"Work","cover_edition":{"key":"/books/OL50M"}}`, + "/books/OL50M.json": func(*http.Request) string { + featured++ + return jsonStr(coverLangEntry("OL50M", "eng", 50)) + }, + "/works/OL6W.json": `{"key":"/works/OL6W","title":"Work","covers":[60],"cover_edition":{"key":"/books/OL60M"}}`, + "/books/OL60M.json": func(*http.Request) string { + featured++ + return jsonStr(coverLangEntry("OL60M", "eng", 61)) + }, + }) + + book, err := c.GetBook(context.Background(), "OL5W") + if err != nil { + t.Fatal(err) + } + if want := coverURL + "/b/id/50-L.jpg"; book.ImageURL != want { + t.Errorf("ImageURL = %q, want the cover edition's %q", book.ImageURL, want) + } + book, err = c.GetBook(context.Background(), "OL6W") + if err != nil { + t.Fatal(err) + } + if want := coverURL + "/b/id/60-L.jpg"; book.ImageURL != want { + t.Errorf("ImageURL = %q, want the work's own %q", book.ImageURL, want) + } + if featured != 1 { + t.Errorf("cover edition fetched %d times, want once (only for the work with no cover)", featured) + } +} + +// Without a cover_edition, the sampled cover is taken from an edition in the +// language the sample settled on, not from whichever edition came first. +func TestSampleWorkEditions_CoverFollowsSampledLanguage(t *testing.T) { + c := newClientWithPaths(t, map[string]interface{}{ + "/works/OL1W.json": `{"key":"/works/OL1W","title":"Work"}`, + "/works/OL1W/editions.json": jsonStr(editionsResponse{Entries: []editionEntry{ + coverLangEntry("OL1M", "fre", 11), + coverLangEntry("OL2M", "eng", 0), + coverLangEntry("OL3M", "eng", 33), + coverLangEntry("OL4M", "eng", 0), + }}), + }) + c.workSampleCache = map[string]workEditionSample{} + + books := []models.Book{{ForeignID: "OL1W", Title: "Work"}} + c.FillMissingWorkLanguages(context.Background(), books) + c.FillMissingWorkCovers(context.Background(), books) + + if books[0].Language != "eng" { + t.Fatalf("Language = %q, want eng (majority)", books[0].Language) + } + if want := coverURL + "/b/id/33-L.jpg"; books[0].ImageURL != want { + t.Errorf("ImageURL = %q, want the English edition's %q, not the French first entry's", books[0].ImageURL, want) + } +} diff --git a/internal/metadata/openlibrary/edition_sample_perf_test.go b/internal/metadata/openlibrary/edition_sample_perf_test.go index 5ed5daffd..fb1646716 100644 --- a/internal/metadata/openlibrary/edition_sample_perf_test.go +++ b/internal/metadata/openlibrary/edition_sample_perf_test.go @@ -98,6 +98,11 @@ func itoa(i int) string { // FillMissingWorkCovers both sample /works/{id}/editions.json?limit=5 — the // same URL — so before #1888 a work missing both language and cover cost two // identical round trips. One shared sample now serves both (#1888). +// +// Since #1779 the sample also reads the work record first, for its featured +// cover_edition. The canned body here has none, so each work costs exactly +// the work record plus the editions sample: two distinct requests, shared by +// both samplers, never repeated. func TestEditionSamplingRoundTripsPerWork(t *testing.T) { const works = 65 @@ -113,9 +118,9 @@ func TestEditionSamplingRoundTripsPerWork(t *testing.T) { } urls, _ := rt.snapshot() - if len(urls) != works { + if len(urls) != 2*works { t.Fatalf("edition sampling spent %d OpenLibrary round trips for %d works, want %d "+ - "(one shared sample per work, not one per sampler)", len(urls), works, works) + "(one work record and one shared sample per work, not one per sampler)", len(urls), works, 2*works) } seen := map[string]int{} diff --git a/internal/metadata/openlibrary/language_sample_scope_test.go b/internal/metadata/openlibrary/language_sample_scope_test.go new file mode 100644 index 000000000..0cee67d4b --- /dev/null +++ b/internal/metadata/openlibrary/language_sample_scope_test.go @@ -0,0 +1,41 @@ +package openlibrary + +import ( + "context" + "strings" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +// FillMissingWorkLanguages only samples works OpenLibrary owns (#3091). A +// Hardcover, Audible or DNB supplement has no /works/{id}/editions.json, so +// asking for one is a guaranteed miss: one wasted request per work, per pass. +func TestFillMissingWorkLanguages_SkipsWorksOpenLibraryDoesNotOwn(t *testing.T) { + rt := &countingTransport{body: editionSampleBody} + c := newSamplingClient(rt) + books := []models.Book{ + {ForeignID: "hc:whipping-mek", MetadataProvider: "hardcover"}, + {ForeignID: "hc:the-faces-of-a-martyr"}, + {ForeignID: "audible:B01CZ0WTEM", MetadataProvider: "audible"}, + {ForeignID: "dnb:1305873874"}, + {ForeignID: "OL1W", MetadataProvider: "openlibrary"}, + } + + if filled := c.FillMissingWorkLanguages(context.Background(), books); filled != 1 { + t.Fatalf("filled = %d, want 1 (the OpenLibrary work only)", filled) + } + for _, b := range books[:4] { + if b.Language != "" { + t.Errorf("%s got language %q from an OpenLibrary sample it does not have", b.ForeignID, b.Language) + } + } + urls, _ := rt.snapshot() + for _, u := range urls { + for _, foreign := range []string{"hc:", "audible:", "dnb:", "hc%3A", "audible%3A", "dnb%3A"} { + if strings.Contains(u, foreign) { + t.Errorf("requested a work OpenLibrary does not own: %s", u) + } + } + } +} diff --git a/internal/metadata/openlibrary/types.go b/internal/metadata/openlibrary/types.go index bfb697788..d31a7a3c7 100644 --- a/internal/metadata/openlibrary/types.go +++ b/internal/metadata/openlibrary/types.go @@ -100,6 +100,11 @@ type workResponse struct { Subjects []string `json:"subjects"` Authors []workAuthor `json:"authors"` Series flexStringSlice `json:"series"` + // CoverEdition points at the edition OpenLibrary's own work page + // features, "/books/OL123M" (#1779). + CoverEdition *struct { + Key string `json:"key"` + } `json:"cover_edition"` } type workAuthor struct {