From 287f737a52fbc6fcb612b567e5293f02e8077a79 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 7 Oct 2026 23:22:35 -0300 Subject: [PATCH 1/7] fix(metadata): copy and scope cached ISBN and ASIN lookups (#2869) GetBookByISBNWithOutcome and GetCanonicalBookByASIN cached the enriched book for 24 hours under a key with no provider scope and returned the cached pointer on a hit, so a caller editing its book rewrote the entry for every later caller, and a provider configuration change kept serving the old answer. Both now bind the provider snapshot, key on its scope, fetch through the bound providers, and clone on store and on hit. enrichBook now reports whether its enrichment was complete. A record built while an enricher (or the full-record fetch) failed goes in the five minute cache instead of the 24 hour one, matching what #2615 did for GetBook. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/2869-isbn-lookup-cache.md | 2 + internal/metadata/aggregator.go | 61 +++++- internal/metadata/aggregator_enrichment.go | 26 ++- internal/metadata/isbn_cache_test.go | 205 +++++++++++++++++++++ 4 files changed, 276 insertions(+), 18 deletions(-) create mode 100644 changelog.d/2869-isbn-lookup-cache.md create mode 100644 internal/metadata/isbn_cache_test.go 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/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_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/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) + } +} From 4ef22ade9ff32244f4b526a15cefac56921bc9ff Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 7 Oct 2026 23:29:23 -0300 Subject: [PATCH 2/7] fix(metadata): resolve supplement translations for the language filter (#3091) With OpenLibrary primary and Hardcover supplementing, Hardcover's translated works reached the language filter with no language and passed under the default unknown "pass", or were stamped with the author's majority language and passed even under "fail". - GetAuthorWorkLanguageEvidence now asks every enricher that implements the evidence capability, not only the primary. A supplement's failure or missing token costs only its own evidence. - Hardcover evidence follows up on works left indeterminate only because they carry no language of their own: one bounded distinct_on query for any edition with a language. The first query has already proved there is no allowed edition, so a known language marks the work not allowed. Best effort; a failure leaves them indeterminate as before. - OpenLibrary edition sampling skips works it does not own (hc:, audible:, dnb:), which were a guaranteed "not found" request each. - The author majority fallback skips works whose title is in a different script from the majority's titles, and works already proved not allowed. Indeterminate evidence still gets the fallback, as #2754 intended. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/3091-supplement-translations.md | 2 + docs/User-Guide-Wiki.md | 20 ++- internal/api/author_majority_language_test.go | 91 +++++++++++++ internal/api/authors.go | 26 +++- internal/api/authors_test.go | 8 +- internal/api/title_script.go | 75 +++++++++++ internal/metadata/aggregator_author_works.go | 56 ++++++-- internal/metadata/hardcover/client.go | 81 ++++++++++++ internal/metadata/hardcover/client_test.go | 7 +- .../hardcover/language_evidence_known_test.go | 121 ++++++++++++++++++ .../language_evidence_supplement_test.go | 57 +++++++++ internal/metadata/openlibrary/client.go | 22 +++- .../openlibrary/language_sample_scope_test.go | 41 ++++++ 13 files changed, 578 insertions(+), 29 deletions(-) create mode 100644 changelog.d/3091-supplement-translations.md create mode 100644 internal/api/author_majority_language_test.go create mode 100644 internal/api/title_script.go create mode 100644 internal/metadata/hardcover/language_evidence_known_test.go create mode 100644 internal/metadata/language_evidence_supplement_test.go create mode 100644 internal/metadata/openlibrary/language_sample_scope_test.go 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/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index a6bf45b81..fd2aebd4f 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -791,15 +791,23 @@ 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. **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/title_script.go b/internal/api/title_script.go new file mode 100644 index 000000000..9e2f0b972 --- /dev/null +++ b/internal/api/title_script.go @@ -0,0 +1,75 @@ +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. +var titleScripts = []struct { + name string + table *unicode.RangeTable +}{ + {"Latin", unicode.Latin}, + {"Cyrillic", unicode.Cyrillic}, + {"Greek", unicode.Greek}, + {"Arabic", unicode.Arabic}, + {"Hebrew", unicode.Hebrew}, + {"Han", unicode.Han}, + {"Hiragana", unicode.Hiragana}, + {"Katakana", 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/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/hardcover/client.go b/internal/metadata/hardcover/client.go index f774484fe..ae3f98f93 100644 --- a/internal/metadata/hardcover/client.go +++ b/internal/metadata/hardcover/client.go @@ -457,9 +457,90 @@ func (c *Client) GetAuthorWorkLanguageEvidence(ctx context.Context, books []mode } evidence[key] = metadata.AuthorWorkLanguageEvidence{State: metadata.AuthorWorkLanguageAllowed, Language: language} } + c.resolveBlankWorkLanguages(ctx, books, 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 has already proved none of its editions is in an +// allowed language; if any edition records a language at all, that language is +// a non-allowed one, and the work is rejected on 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, 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 + } + evidence[key] = metadata.AuthorWorkLanguageEvidence{State: metadata.AuthorWorkLanguageNotAllowed, 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..2a2d65f24 --- /dev/null +++ b/internal/metadata/hardcover/language_evidence_known_test.go @@ -0,0 +1,121 @@ +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 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/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..007c6574f 100644 --- a/internal/metadata/openlibrary/client.go +++ b/internal/metadata/openlibrary/client.go @@ -868,7 +868,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,6 +884,21 @@ func (c *Client) FillMissingWorkLanguages(ctx context.Context, books []models.Bo return int(filled.Load()) } +// 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 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 @@ -974,10 +989,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) 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) + } + } + } +} From 485a30b0d28b609646da8185c405ca4187e9b2c2 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 7 Oct 2026 23:32:06 -0300 Subject: [PATCH 3/7] fix(metadata): derive OpenLibrary work language and cover from cover_edition (#1779) The cover and language samplers read the first five entries of /works/{id}/editions.json, whose order means nothing, so a work whose first few entries were translations was read as foreign language or got a translated cover. #2197 fixed the truncated GetEditions half; this is the sampler half. sampleWorkEditions now reads the work record and, when it names a cover_edition, that edition: the one OpenLibrary's own work page features. Only what it lacks comes from the editions sample, as before, and a sampled cover now prefers an edition in the sampled language, then one with no language, then any. A work-level cover from the record is preferred to a sampled one. Cost: a work whose featured edition carries both costs two plain document fetches instead of one editions sample; any other work costs the work record plus the sample. The round trip test is updated to pin that. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/1779-openlibrary-cover-edition.md | 2 + docs/User-Guide-Wiki.md | 5 +- internal/metadata/openlibrary/client.go | 114 +++++++++++++++--- .../openlibrary/cover_edition_test.go | 76 ++++++++++++ .../openlibrary/edition_sample_perf_test.go | 9 +- internal/metadata/openlibrary/types.go | 5 + 6 files changed, 191 insertions(+), 20 deletions(-) create mode 100644 changelog.d/1779-openlibrary-cover-edition.md create mode 100644 internal/metadata/openlibrary/cover_edition_test.go diff --git a/changelog.d/1779-openlibrary-cover-edition.md b/changelog.d/1779-openlibrary-cover-edition.md new file mode 100644 index 000000000..c1e8ee6c2 --- /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. This costs one extra small OpenLibrary request per sampled work. diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index fd2aebd4f..518b38df7 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -807,7 +807,10 @@ like everything else on that tab that names server paths. 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. **Reconcile catalogue** treats a + 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/metadata/openlibrary/client.go b/internal/metadata/openlibrary/client.go index 007c6574f..ac534b24c 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 @@ -899,32 +901,113 @@ func ownsWork(book models.Book) bool { return p == "" || strings.EqualFold(p, "openlibrary") } -// 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). +// 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. +// +// So a work with a featured edition that carries both costs two requests +// instead of the one editions sample it used to, and every other work costs +// the work record on top of that sample. 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; @@ -1011,12 +1094,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..de38608f8 --- /dev/null +++ b/internal/metadata/openlibrary/cover_edition_test.go @@ -0,0 +1,76 @@ +package openlibrary + +import ( + "context" + "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) + } +} + +// 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/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 { From 5d3be2fbd7a2e36bb8ea16419e296aaff217c863 Mon Sep 17 00:00:00 2001 From: vavallee Date: Wed, 7 Oct 2026 23:34:38 -0300 Subject: [PATCH 4/7] fix(rebind): drop the previous work's provider editions (#2781) Rebind changed the book's identity and hydrated the new work but left every edition persisted for the old one attached. Search loads all of a book's editions and CriteriaISBN takes the selected or first usable ISBN, so the corrected book could keep searching on the wrong work's ISBN, and the unique foreign_id kept the old edition from its rightful book. After the guarded identity update, rebind now deletes the book's provider editions when the identity actually changed. Kept: Calibre and Audiobookshelf importer editions (calibre:, abs:), the selected edition, and any edition a download or Calibre delivery references. The delete is conditioned on the book already carrying its new identity, and UpsertMetadata refuses old-identity writes once that identity has changed, so an earlier in-flight edition write is removed and a later one cannot land. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/2781-rebind-old-editions.md | 2 + docs/Metadata-Editing-Wiki.md | 7 ++ internal/api/books.go | 11 ++ internal/api/rebind_editions_test.go | 156 ++++++++++++++++++++++++ internal/db/editions.go | 39 ++++++ 5 files changed, 215 insertions(+) create mode 100644 changelog.d/2781-rebind-old-editions.md create mode 100644 internal/api/rebind_editions_test.go 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/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/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/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 { From e47bf12539cbbfcc0bd2c475d54744e79d1570dd Mon Sep 17 00:00:00 2001 From: vavallee Date: Thu, 8 Oct 2026 00:25:37 -0300 Subject: [PATCH 5/7] fix(metadata): read Japanese titles as one script for the majority language (#3091) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit titleScript told Han, Hiragana and Katakana apart, so a katakana heavy Japanese title such as "ノルウェイの森" was judged foreign to an author whose other titles are mostly kanji, lost the majority language and was rejected under unknown_language_behavior = fail. The three now share one CJK bucket. Hangul stays separate, since Korean titles are written almost entirely in it and a Japanese or Chinese translation in a Korean catalogue is plainly foreign. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- internal/api/title_script.go | 13 ++++++-- internal/api/title_script_test.go | 54 +++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 internal/api/title_script_test.go diff --git a/internal/api/title_script.go b/internal/api/title_script.go index 9e2f0b972..153a9d1d0 100644 --- a/internal/api/title_script.go +++ b/internal/api/title_script.go @@ -10,6 +10,13 @@ import ( // 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 @@ -19,9 +26,9 @@ var titleScripts = []struct { {"Greek", unicode.Greek}, {"Arabic", unicode.Arabic}, {"Hebrew", unicode.Hebrew}, - {"Han", unicode.Han}, - {"Hiragana", unicode.Hiragana}, - {"Katakana", unicode.Katakana}, + {"CJK", unicode.Han}, + {"CJK", unicode.Hiragana}, + {"CJK", unicode.Katakana}, {"Hangul", unicode.Hangul}, {"Thai", unicode.Thai}, {"Devanagari", unicode.Devanagari}, 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) + } + } +} From 7c156aca390ea2f3cb80be1210c7e2bd6b453e2c Mon Sep 17 00:00:00 2001 From: vavallee Date: Thu, 8 Oct 2026 00:26:30 -0300 Subject: [PATCH 6/7] fix(hardcover): don't reject a blank work for an allowed known language (#3091) The follow up query for blank-language works rejected the work on any edition language it found, on the reasoning that the allowed-language query had already ruled allowed languages out. But that query matches code2 and code3 only, while the follow up also normalises the language name, so an edition recorded as "English" with no codes was missed by the first and then rejected by the second. The language found is now checked against the profile, and an allowed one keeps the work. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- internal/metadata/hardcover/client.go | 22 +++++++++----- .../hardcover/language_evidence_known_test.go | 30 +++++++++++++++++++ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/internal/metadata/hardcover/client.go b/internal/metadata/hardcover/client.go index ae3f98f93..5daef288e 100644 --- a/internal/metadata/hardcover/client.go +++ b/internal/metadata/hardcover/client.go @@ -457,7 +457,7 @@ func (c *Client) GetAuthorWorkLanguageEvidence(ctx context.Context, books []mode } evidence[key] = metadata.AuthorWorkLanguageEvidence{State: metadata.AuthorWorkLanguageAllowed, Language: language} } - c.resolveBlankWorkLanguages(ctx, books, evidence) + c.resolveBlankWorkLanguages(ctx, books, allowed, evidence) return evidence, nil } @@ -466,15 +466,16 @@ func (c *Client) GetAuthorWorkLanguageEvidence(ctx context.Context, books []mode // 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 has already proved none of its editions is in an -// allowed language; if any edition records a language at all, that language is -// a non-allowed one, and the work is rejected on it. A work with no language -// recorded on any edition stays indeterminate. +// (#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, evidence map[string]metadata.AuthorWorkLanguageEvidence) { +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) @@ -537,7 +538,14 @@ func (c *Client) resolveBlankWorkLanguages(ctx context.Context, books []models.B if language == "" { continue } - evidence[key] = metadata.AuthorWorkLanguageEvidence{State: metadata.AuthorWorkLanguageNotAllowed, Language: language} + 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} } } diff --git a/internal/metadata/hardcover/language_evidence_known_test.go b/internal/metadata/hardcover/language_evidence_known_test.go index 2a2d65f24..217a585ed 100644 --- a/internal/metadata/hardcover/language_evidence_known_test.go +++ b/internal/metadata/hardcover/language_evidence_known_test.go @@ -93,6 +93,36 @@ func TestGetAuthorWorkLanguageEvidence_BlankLanguageResolvedFromKnownEditions(t } } +// 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) { From 5b966511d21f1fb742f43e44d1b69f1815bcf7eb Mon Sep 17 00:00:00 2001 From: vavallee Date: Thu, 8 Oct 2026 00:28:33 -0300 Subject: [PATCH 7/7] fix(metadata): use cover_edition in OpenLibrary GetBook, state sampler cost (#1779) GetBook now takes the featured cover_edition's cover when the work record has no cover of its own: one extra request, only in that case, best effort. The work's own covers[0] still wins when present. The sampler doc comment and changelog understated the cost. Against the single editions sample it replaced, it is up to two extra requests per sampled work: two when the featured edition carries both a language and a cover, three when it lacks either, two when there is no featured edition. A test now pins the three request path. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_016fJcCVbNnKsmj2MAAMwWj9 Signed-off-by: vavallee --- changelog.d/1779-openlibrary-cover-edition.md | 2 +- internal/metadata/openlibrary/client.go | 25 +++++- .../openlibrary/cover_edition_test.go | 78 +++++++++++++++++++ 3 files changed, 101 insertions(+), 4 deletions(-) diff --git a/changelog.d/1779-openlibrary-cover-edition.md b/changelog.d/1779-openlibrary-cover-edition.md index c1e8ee6c2..4028ae803 100644 --- a/changelog.d/1779-openlibrary-cover-edition.md +++ b/changelog.d/1779-openlibrary-cover-edition.md @@ -1,2 +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. This costs one extra small OpenLibrary request per sampled work. +- **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/internal/metadata/openlibrary/client.go b/internal/metadata/openlibrary/client.go index ac534b24c..98df7420b 100644 --- a/internal/metadata/openlibrary/client.go +++ b/internal/metadata/openlibrary/client.go @@ -275,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. @@ -917,9 +929,16 @@ func ownsWork(book models.Book) bool { // one. A work-level cover, which the work record carries for free, is // preferred to a sampled one. // -// So a work with a featured edition that carries both costs two requests -// instead of the one editions sample it used to, and every other work costs -// the work record on top of that sample. +// 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 diff --git a/internal/metadata/openlibrary/cover_edition_test.go b/internal/metadata/openlibrary/cover_edition_test.go index de38608f8..9645ef909 100644 --- a/internal/metadata/openlibrary/cover_edition_test.go +++ b/internal/metadata/openlibrary/cover_edition_test.go @@ -2,6 +2,7 @@ package openlibrary import ( "context" + "net/http" "testing" "github.com/vavallee/bindery/internal/models" @@ -49,6 +50,83 @@ func TestSampleWorkEditions_PrefersCoverEdition(t *testing.T) { } } +// 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) {