From d750c1e5d71e46ac605d1c82d5485505f0753e95 Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Wed, 23 Sep 2026 20:26:48 -0500 Subject: [PATCH 1/6] fix(metadata): retain hardcover audiobook runtime before search (#2755) Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- .../2755-hardcover-audiobook-duration.md | 2 + docs/User-Guide-Wiki.md | 4 + internal/bookhydrate/hardcover.go | 92 +++++-- internal/bookhydrate/hardcover_test.go | 255 +++++++++++++++++- internal/db/books.go | 42 +++ internal/db/editions.go | 15 +- internal/db/editions_test.go | 54 ++++ internal/metadata/hardcover/client.go | 3 + internal/metadata/hardcover/client_test.go | 48 ++++ internal/models/edition.go | 2 + 10 files changed, 492 insertions(+), 25 deletions(-) create mode 100644 changelog.d/2755-hardcover-audiobook-duration.md diff --git a/changelog.d/2755-hardcover-audiobook-duration.md b/changelog.d/2755-hardcover-audiobook-duration.md new file mode 100644 index 000000000..a9083c769 --- /dev/null +++ b/changelog.d/2755-hardcover-audiobook-duration.md @@ -0,0 +1,2 @@ +### Fixed +- **Hardcover audiobook duration** (#2755, #2756): use a known audio edition runtime before searching, and let metadata refresh replace blank edition ASINs. diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index 7605dc47d..1d3c291ef 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -566,6 +566,10 @@ Things worth knowing: - **Google Books** (free API key) and **Audnexus/Audible** (audiobook narrator, duration, by ASIN) enrich further. +For a Hardcover audiobook, the chosen audio edition can fill a missing book +duration before indexer search. Audnex may update that duration when an ASIN is +available. + Which of those a given book actually came from is on the book page, under **Metadata source**. It names the provider, shows the identifier the book is bound to with a copy button, and lists any other provider ids the same book is diff --git a/internal/bookhydrate/hardcover.go b/internal/bookhydrate/hardcover.go index 899fb3b37..bf2e2c29e 100644 --- a/internal/bookhydrate/hardcover.go +++ b/internal/bookhydrate/hardcover.go @@ -22,6 +22,7 @@ type EditionUpserter interface { // BookUpdater persists book-level fields promoted during hydration. type BookUpdater interface { Update(context.Context, *models.Book) error + FillMissingAudiobookDuration(context.Context, *models.Book) (bool, int, error) } // AudiobookEnricher fills audiobook metadata once an ASIN is known. @@ -79,6 +80,7 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { if book == nil || book.ID == 0 { return result } + before := *book editionForeignID := strings.TrimSpace(opts.ProviderForeignID) if editionForeignID == "" { editionForeignID = book.ForeignID @@ -118,18 +120,31 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { continue } result.Upserted++ + // UpsertMetadata reloads persisted fields; runtime exists only in the provider response. + edition.DurationSeconds = editions[i].DurationSeconds if isLikelyAudioEdition(edition) { + if edition.ASIN != nil && strings.TrimSpace(*edition.ASIN) == "" { + // UpsertMetadata can retain a whitespace-only ASIN; use the fetched identity for hydration. + edition.ASIN = editions[i].ASIN + } + if editions[i].ASIN != nil && edition.ASIN != nil && strings.TrimSpace(*editions[i].ASIN) != "" && + !strings.EqualFold(strings.TrimSpace(*editions[i].ASIN), strings.TrimSpace(*edition.ASIN)) { + // The stored ASIN belongs to a different narration than the fetched runtime. + edition.DurationSeconds = 0 + } acceptedAudioEditions = append(acceptedAudioEditions, edition) } } // #806: derive audiobook metadata from the chosen Hardcover edition BEFORE - // running Audnex, so Hardcover's deterministic edition data fills the book - // and Audnex is left to cover only the gaps (narrator, refined duration, - // summary, cover-if-missing). preferredAudioEdition picks the same edition - // maybePromoteASIN promotes the ASIN from, keeping the two derivations - // consistent. - if edition, ok := preferredAudioEdition(acceptedAudioEditions); ok { + // running Audnex, so its edition data fills missing book fields. Audnex can + // then refine duration and fill narrator, summary, or a missing cover. + // Prefer the edition matching the book's ASIN or the one promoted below. + if edition, ok := preferredAudioEdition(acceptedAudioEditions, book.ASIN); ok { + if strings.TrimSpace(book.ASIN) != "" && (edition.ASIN == nil || !strings.EqualFold(strings.TrimSpace(*edition.ASIN), strings.TrimSpace(book.ASIN))) { + // A different known ASIN may be a different narration; leave runtime unknown. + edition.DurationSeconds = 0 + } if deriveAudiobookMetadataFromEdition(book, edition, opts.MediaTypePinned) { result.MetadataDerived = true } @@ -155,31 +170,68 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { // book. Derivation alone (e.g. ASIN already set) is enough to warrant a // write so the language/cover/duration we pulled isn't lost. if (result.ASINPromoted || result.MetadataDerived) && opts.Books != nil { - if err := opts.Books.Update(ctx, book); err != nil { + var err error + updated := false + if !result.ASINPromoted && before.DurationSeconds <= 0 && book.DurationSeconds > 0 && + before.ASIN == book.ASIN && before.MediaType == book.MediaType && before.Status == book.Status && + before.Language == book.Language && before.ImageURL == book.ImageURL { + var currentDuration int + updated, currentDuration, err = opts.Books.FillMissingAudiobookDuration(ctx, book) + if !updated { + book.DurationSeconds = currentDuration + result.MetadataDerived = false + if err == nil { + slog.Debug("hardcover duration write skipped after concurrent book update", "bookID", book.ID) + } + } + } else { + err = opts.Books.Update(ctx, book) + updated = err == nil + } + if err != nil { if result.Err == nil { result.Err = err } slog.Warn("hardcover book hydration persist failed", "bookID", book.ID, "asin", book.ASIN, "error", err) } else { - result.BookUpdated = true + result.BookUpdated = updated } } return result } -// preferredAudioEdition returns the audio-looking edition the hydrator should -// derive book metadata from — the same edition maybePromoteASIN promotes the -// ASIN from (highest audioEditionScore, first on ties). Returns ok=false when -// there is no audio edition to derive from. -func preferredAudioEdition(editions []models.Edition) (models.Edition, bool) { +// preferredAudioEdition returns the audio-looking edition whose ASIN matches +// the book, or the edition maybePromoteASIN will use when the book has no ASIN. +// Among matching ASINs, prefer an edition with a known runtime, then the +// highest audioEditionScore (first on ties). +func preferredAudioEdition(editions []models.Edition, bookASIN string) (models.Edition, bool) { best := -1 - bestScore := -1 + targetASIN := strings.TrimSpace(bookASIN) + if targetASIN == "" { + targetASIN = preferredEditionASIN(editions) + } for i := range editions { - score := audioEditionScore(editions[i]) - if best == -1 || score > bestScore { + matchesASIN := targetASIN != "" && editions[i].ASIN != nil && strings.EqualFold(strings.TrimSpace(*editions[i].ASIN), targetASIN) + if best == -1 { + best = i + continue + } + bestMatchesASIN := targetASIN != "" && editions[best].ASIN != nil && strings.EqualFold(strings.TrimSpace(*editions[best].ASIN), targetASIN) + if matchesASIN != bestMatchesASIN { + if matchesASIN { + best = i + } + continue + } + if (matchesASIN || targetASIN == "") && (editions[i].DurationSeconds > 0) != (editions[best].DurationSeconds > 0) { + if editions[i].DurationSeconds > 0 { + best = i + } + continue + } + if audioEditionScore(editions[i]) > audioEditionScore(editions[best]) { best = i - bestScore = score } } if best == -1 { @@ -249,6 +301,10 @@ func deriveAudiobookMetadataFromEdition(book *models.Book, edition models.Editio changed = true } } + if bookAcceptsAudiobookASIN(book) && book.DurationSeconds <= 0 && edition.DurationSeconds > 0 { + book.DurationSeconds = edition.DurationSeconds + changed = true + } return changed } @@ -309,7 +365,7 @@ func isLikelyAudioEdition(edition models.Edition) bool { edition.Format, edition.EditionInfo, }, " ")) - return editionHasAudioMarker(text) + return editionHasAudioMarker(text) || (edition.DurationSeconds > 0 && !edition.IsEbook) } func editionHasAudioMarker(text string) bool { diff --git a/internal/bookhydrate/hardcover_test.go b/internal/bookhydrate/hardcover_test.go index 0ccc37df6..dd7c4e80e 100644 --- a/internal/bookhydrate/hardcover_test.go +++ b/internal/bookhydrate/hardcover_test.go @@ -167,6 +167,200 @@ func TestHydrateHardcoverEditionsDerivesAudiobookMetadataBeforeAudnex(t *testing } } +func TestHydrateHardcoverEditionsDerivesSelectedAudioDuration(t *testing.T) { + for _, tc := range []struct { + name string + mediaType string + pinned bool + bookDuration int + bookASIN string + editionDuration int + unknownFormat bool + asinOnOther bool + wantDuration int + wantASIN string + }{ + {name: "missing duration", mediaType: models.MediaTypeAudiobook, editionDuration: 36000, wantDuration: 36000}, + {name: "runtime without audio format", mediaType: models.MediaTypeAudiobook, editionDuration: 36000, unknownFormat: true, wantDuration: 36000}, + {name: "whitespace ASIN", mediaType: models.MediaTypeAudiobook, bookASIN: " ", editionDuration: 36000, wantDuration: 36000, wantASIN: "B000AUDIO1"}, + {name: "known duration", mediaType: models.MediaTypeAudiobook, bookDuration: 42000, editionDuration: 36000, wantDuration: 42000}, + {name: "missing edition duration", mediaType: models.MediaTypeAudiobook}, + {name: "invalid edition duration", mediaType: models.MediaTypeAudiobook, editionDuration: -1}, + {name: "ASIN chooses matching edition", mediaType: models.MediaTypeAudiobook, editionDuration: 36000, asinOnOther: true, wantDuration: 72000}, + {name: "known ASIN chooses matching edition", mediaType: models.MediaTypeAudiobook, bookASIN: "B000AUDIO1", editionDuration: 36000, asinOnOther: true, wantDuration: 72000}, + {name: "unmatched known ASIN", mediaType: models.MediaTypeAudiobook, bookASIN: "B000UNKNOWN", editionDuration: 36000}, + {name: "pinned ebook", mediaType: models.MediaTypeEbook, pinned: true, editionDuration: 36000}, + } { + t.Run(tc.name, func(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", tc.mediaType) + book.DurationSeconds = tc.bookDuration + book.ASIN = tc.bookASIN + if tc.bookDuration > 0 || tc.bookASIN != "" { + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + } + audioASIN := "B000AUDIO1" + selectedASIN := &audioASIN + var otherASIN *string + if tc.asinOnOther { + selectedASIN, otherASIN = nil, &audioASIN + } + audioFormat := "Audiobook" + if tc.unknownFormat { + audioFormat = "Unknown" + } + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, + Provider: "hardcover", + Editions: editions, + Books: books, + MediaTypePinned: tc.pinned, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + return []models.Edition{ + {ForeignID: "hc:print", Format: "Hardcover"}, + {ForeignID: "hc:audio", Format: audioFormat, ASIN: selectedASIN, DurationSeconds: tc.editionDuration}, + {ForeignID: "hc:other-audio", Format: "Audiobook", ASIN: otherASIN, DurationSeconds: 72000}, + }, nil + }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + if stored.DurationSeconds != tc.wantDuration { + t.Errorf("stored DurationSeconds = %d, want %d; result=%+v", stored.DurationSeconds, tc.wantDuration, result) + } + if tc.wantASIN != "" && stored.ASIN != tc.wantASIN { + t.Errorf("stored ASIN = %q, want %q", stored.ASIN, tc.wantASIN) + } + }) + } +} + +func TestHydrateHardcoverEditionsPrefersRuntimeWhenEditionsHaveNoASIN(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + return []models.Edition{ + {ForeignID: "hc:labeled", Format: "Audiobook"}, + {ForeignID: "hc:runtime", Format: "Unknown", DurationSeconds: 36000}, + }, nil + }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + if stored.DurationSeconds != 36000 || !result.BookUpdated { + t.Fatalf("runtime from unlabeled audio edition was lost: duration=%d result=%+v", stored.DurationSeconds, result) + } +} + +func TestHydrateHardcoverEditionsDurationOnlyPreservesConcurrentChanges(t *testing.T) { + for _, replaceASIN := range []bool{false, true} { + name := "unrelated changes" + if replaceASIN { + name = "changed ASIN" + } + t.Run(name, func(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) + book.ASIN = "B000AUDIO1" + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(ctx context.Context, _ string) ([]models.Edition, error) { + current, err := books.GetByID(ctx, book.ID) + if err != nil { + return nil, err + } + current.Monitored = false + current.Status = models.BookStatusSkipped + current.Narrator = "Concurrent narrator" + if replaceASIN { + current.ASIN = "B000OTHER1" + } + if err := books.Update(ctx, current); err != nil { + return nil, err + } + asin := "B000AUDIO1" + return []models.Edition{{ForeignID: "hc:audio", Format: "Audiobook", ASIN: &asin, DurationSeconds: 36000}}, nil + }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + if result.BookUpdated == replaceASIN { + t.Fatalf("BookUpdated=%v with replaceASIN=%v", result.BookUpdated, replaceASIN) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + wantDuration := 36000 + wantASIN := "B000AUDIO1" + if replaceASIN { + wantDuration = 0 + wantASIN = "B000OTHER1" + } + if book.DurationSeconds != wantDuration { + t.Fatalf("in-memory duration = %d, want %d after persistence", book.DurationSeconds, wantDuration) + } + if stored.Monitored || stored.Status != models.BookStatusSkipped || stored.Narrator != "Concurrent narrator" || + stored.ASIN != wantASIN || stored.DurationSeconds != wantDuration { + t.Fatalf("concurrent book changes were lost or stale duration persisted: %+v", stored) + } + }) + } +} + +func TestHydrateHardcoverEditionsAdoptsConcurrentDuration(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) + book.ASIN = "B000AUDIO1" + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + asin := book.ASIN + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(ctx context.Context, _ string) ([]models.Edition, error) { + concurrentBook, err := books.GetByID(ctx, book.ID) + if err != nil { + return nil, err + } + concurrent := HydrateHardcoverEditions(ctx, Options{ + Book: concurrentBook, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + return []models.Edition{{ForeignID: "hc:audio", Format: "Audiobook", ASIN: &asin, DurationSeconds: 42000}}, nil + }, + }) + if concurrent.Err != nil || !concurrent.BookUpdated { + t.Fatalf("concurrent hydration failed: %+v", concurrent) + } + return []models.Edition{{ForeignID: "hc:audio", Format: "Audiobook", ASIN: &asin, DurationSeconds: 36000}}, nil + }, + }) + if result.Err != nil || result.BookUpdated { + t.Fatalf("stale hydration should lose the guarded write: %+v", result) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + if book.DurationSeconds != 42000 || stored.DurationSeconds != 42000 { + t.Fatalf("in-memory duration=%d stored duration=%d, want concurrent runtime 42000", book.DurationSeconds, stored.DurationSeconds) + } +} + // TestHydrateHardcoverEditionsDoesNotClobberKnownAudiobookFields guards the // "unknown ⇒ don't overwrite known" invariant: Hardcover edition data must not // replace language/cover the book already carries. @@ -355,11 +549,12 @@ func TestHydrateHardcoverEditionsPromotesStoredEditionASIN(t *testing.T) { Books: books, FetchEditions: func(context.Context, string) ([]models.Edition, error) { return []models.Edition{{ - ForeignID: "hc:audio", - Title: "Fetched Audio", - ASIN: &replacementASIN, - Format: "Audiobook", - Monitored: true, + ForeignID: "hc:audio", + Title: "Fetched Audio", + ASIN: &replacementASIN, + Format: "Audiobook", + DurationSeconds: 36000, + Monitored: true, }}, nil }, Enricher: &fakeAudiobookEnricher{}, @@ -380,6 +575,9 @@ func TestHydrateHardcoverEditionsPromotesStoredEditionASIN(t *testing.T) { if stored.ASIN != storedASIN { t.Fatalf("stored book ASIN = %q, want %q", stored.ASIN, storedASIN) } + if stored.DurationSeconds != 0 { + t.Fatalf("stored book duration = %d from mismatched fetched ASIN, want 0", stored.DurationSeconds) + } edition, err := editions.GetByForeignID(ctx, "hc:audio") if err != nil { t.Fatal(err) @@ -389,6 +587,53 @@ func TestHydrateHardcoverEditionsPromotesStoredEditionASIN(t *testing.T) { } } +func TestHydrateHardcoverEditionsUsesRuntimeMatchingRetainedASIN(t *testing.T) { + for _, tc := range []struct { + name string + storedASIN string + fetchedASIN string + siblingASIN string + wantASIN string + wantDuration int + }{ + {name: "valid sibling after ASIN conflict", storedASIN: "B000RETAIN", fetchedASIN: "B000FETCHD", siblingASIN: "B000RETAIN", wantASIN: "B000RETAIN", wantDuration: 72000}, + {name: "stored whitespace ASIN", storedASIN: " ", fetchedASIN: "B000FETCHD", wantASIN: "B000FETCHD", wantDuration: 36000}, + } { + t.Run(tc.name, func(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) + if ok, err := editions.UpsertMetadata(ctx, &models.Edition{ + ForeignID: "hc:audio", BookID: book.ID, Title: "Audio", Format: "Audiobook", ASIN: &tc.storedASIN, + }); err != nil || !ok { + t.Fatalf("seed edition ok=%v err=%v", ok, err) + } + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + fetched := []models.Edition{{ + ForeignID: "hc:audio", Title: "Audio", Format: "Audiobook", ASIN: &tc.fetchedASIN, DurationSeconds: 36000, + }} + if tc.siblingASIN != "" { + fetched = append(fetched, models.Edition{ + ForeignID: "hc:sibling", Title: "Sibling Audio", Format: "Audiobook", ASIN: &tc.siblingASIN, DurationSeconds: 72000, + }) + } + return fetched, nil + }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + if stored.ASIN != tc.wantASIN || stored.DurationSeconds != tc.wantDuration { + t.Fatalf("stored ASIN=%q duration=%d, want ASIN=%q duration=%d; result=%+v", stored.ASIN, stored.DurationSeconds, tc.wantASIN, tc.wantDuration, result) + } + }) + } +} + func TestHydrateHardcoverEditionsDoesNotPromoteNonAudioASIN(t *testing.T) { books, editions, book, ctx := newHydrateBook(t, "hc:hydrated-book", "hardcover", models.MediaTypeAudiobook) kindleASIN := "B111111111" diff --git a/internal/db/books.go b/internal/db/books.go index 6ebe727d0..1c1315dd0 100644 --- a/internal/db/books.go +++ b/internal/db/books.go @@ -757,6 +757,48 @@ func (r *BookRepo) Update(ctx context.Context, b *models.Book) error { return nil } +// FillMissingAudiobookDuration persists a duration derived from edition metadata +// without rewriting a book that may have changed during the provider fetch. +// On a guarded-write miss, it returns the current duration only if the book's +// provider, media type, and ASIN still match the source of the fetched runtime. +func (r *BookRepo) FillMissingAudiobookDuration(ctx context.Context, b *models.Book) (bool, int, error) { + if b == nil || b.ID == 0 || b.DurationSeconds <= 0 { + return false, 0, fmt.Errorf("fill missing audiobook duration: invalid book") + } + if b.MediaType != models.MediaTypeAudiobook && b.MediaType != models.MediaTypeBoth { + return false, 0, nil + } + now := time.Now().UTC() + res, err := r.exec.ExecContext(ctx, ` + UPDATE books SET duration_seconds = ?, updated_at = ? + WHERE id = ? AND foreign_id = ? AND metadata_provider = ? AND media_type = ? + AND asin = ? AND duration_seconds <= 0`, + b.DurationSeconds, timeValueArg(now), b.ID, b.ForeignID, b.MetadataProvider, b.MediaType, b.ASIN) + if err != nil { + return false, 0, fmt.Errorf("fill missing audiobook duration for book %d: %w", b.ID, err) + } + updated, err := res.RowsAffected() + if err != nil { + return false, 0, fmt.Errorf("check audiobook duration update for book %d: %w", b.ID, err) + } + if updated == 0 { + var currentDuration int + err := r.exec.QueryRowContext(ctx, ` + SELECT duration_seconds FROM books + WHERE id = ? AND foreign_id = ? AND metadata_provider = ? AND media_type = ? AND asin = ?`, + b.ID, b.ForeignID, b.MetadataProvider, b.MediaType, b.ASIN).Scan(¤tDuration) + if errors.Is(err, sql.ErrNoRows) { + return false, 0, nil + } + if err != nil { + return false, 0, fmt.Errorf("read current audiobook duration for book %d: %w", b.ID, err) + } + return false, currentDuration, nil + } + b.UpdatedAt = now + return true, b.DurationSeconds, nil +} + // MarkWantedMonitored updates only the fields needed to queue a book for // searching, preserving metadata that may not be present on sparse callers. func (r *BookRepo) MarkWantedMonitored(ctx context.Context, id int64) error { diff --git a/internal/db/editions.go b/internal/db/editions.go index 3a473dfa1..e840988de 100644 --- a/internal/db/editions.go +++ b/internal/db/editions.go @@ -165,6 +165,14 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo if strings.TrimSpace(e.Title) == "" { e.Title = "Unknown Edition" } + if e.ASIN != nil { + asin := strings.TrimSpace(*e.ASIN) + if asin == "" { + e.ASIN = nil + } else { + e.ASIN = &asin + } + } now := time.Now().UTC() isEbook := 0 @@ -172,6 +180,9 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo isEbook = 1 } monitored := 1 + // SQLite TRIM defaults to ASCII space; include Go's Unicode White_Space set + // so legacy blank ASINs use the same rule as incoming strings.TrimSpace. + const asinWhitespace = " \t\n\v\f\r\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" res, err := r.db.ExecContext(ctx, ` INSERT INTO editions (foreign_id, book_id, title, isbn_13, isbn_10, asin, @@ -183,7 +194,7 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo title = COALESCE(NULLIF(editions.title, ''), excluded.title), isbn_13 = COALESCE(NULLIF(editions.isbn_13, ''), excluded.isbn_13), isbn_10 = COALESCE(NULLIF(editions.isbn_10, ''), excluded.isbn_10), - asin = COALESCE(NULLIF(editions.asin, ''), excluded.asin), + asin = CASE WHEN TRIM(COALESCE(editions.asin, ''), ?) = '' THEN excluded.asin ELSE editions.asin END, publisher = COALESCE(NULLIF(editions.publisher, ''), excluded.publisher), publish_date= COALESCE(editions.publish_date, excluded.publish_date), format = COALESCE(NULLIF(editions.format, ''), excluded.format), @@ -196,7 +207,7 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo WHERE editions.book_id = excluded.book_id`, e.ForeignID, e.BookID, e.Title, e.ISBN13, e.ISBN10, e.ASIN, e.Publisher, timeArg(e.PublishDate), e.Format, e.NumPages, e.Language, - e.ImageURL, isEbook, e.EditionInfo, monitored, timeValueArg(now), timeValueArg(now)) + e.ImageURL, isEbook, e.EditionInfo, monitored, timeValueArg(now), timeValueArg(now), asinWhitespace) if err != nil { return false, fmt.Errorf("upsert metadata edition %s: %w", e.ForeignID, err) } diff --git a/internal/db/editions_test.go b/internal/db/editions_test.go index 7bf87c450..ecfa09e33 100644 --- a/internal/db/editions_test.go +++ b/internal/db/editions_test.go @@ -246,6 +246,60 @@ func TestEditionRepo_UpsertMetadataFillsMissingFields(t *testing.T) { } } +func TestEditionRepo_UpsertMetadataReplacesWhitespaceASIN(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + + author := &models.Author{ForeignID: "OL-ASIN-A", Name: "A", SortName: "A", MetadataProvider: "openlibrary", Monitored: true} + if err := NewAuthorRepo(database).Create(ctx, author); err != nil { + t.Fatal(err) + } + book := &models.Book{ForeignID: "hc:asin-book", AuthorID: author.ID, Title: "Book", SortTitle: "Book", Status: "wanted", Genres: []string{}, MetadataProvider: "hardcover", Monitored: true} + if err := NewBookRepo(database).Create(ctx, book); err != nil { + t.Fatal(err) + } + + repo := NewEditionRepo(database) + for _, tc := range []struct{ name, blank string }{ + {name: "ASCII whitespace", blank: " \t "}, + {name: "Unicode whitespace", blank: "\u00a0\u2003"}, + } { + t.Run(tc.name, func(t *testing.T) { + edition := &models.Edition{ForeignID: "hc:asin-" + tc.name, BookID: book.ID, Title: "Audio Edition", ASIN: &tc.blank} + if ok, err := repo.UpsertMetadata(ctx, edition); err != nil || !ok { + t.Fatalf("insert metadata edition: ok=%v err=%v", ok, err) + } + if edition.ASIN != nil { + t.Fatalf("blank ASIN should be stored as missing, got %q", *edition.ASIN) + } + + // Simulate a legacy row written before whitespace ASINs were normalized. + if _, err := database.ExecContext(ctx, "UPDATE editions SET asin = ? WHERE id = ?", tc.blank, edition.ID); err != nil { + t.Fatal(err) + } + valid := "B012345678" + incoming := &models.Edition{ForeignID: edition.ForeignID, BookID: book.ID, Title: edition.Title, ASIN: &valid} + if ok, err := repo.UpsertMetadata(ctx, incoming); err != nil || !ok { + t.Fatalf("update metadata edition: ok=%v err=%v", ok, err) + } + if incoming.ASIN == nil || *incoming.ASIN != valid { + t.Fatalf("upsert returned ASIN %v, want %q", incoming.ASIN, valid) + } + stored, err := repo.GetByForeignID(ctx, edition.ForeignID) + if err != nil { + t.Fatal(err) + } + if stored == nil || stored.ASIN == nil || *stored.ASIN != valid { + t.Fatalf("persisted ASIN = %v, want %q", stored, valid) + } + }) + } +} + func TestEditionRepo_UpsertMetadataSkipsDifferentBook(t *testing.T) { database, err := OpenMemory() if err != nil { diff --git a/internal/metadata/hardcover/client.go b/internal/metadata/hardcover/client.go index f7eaa182d..de77bacd5 100644 --- a/internal/metadata/hardcover/client.go +++ b/internal/metadata/hardcover/client.go @@ -1730,6 +1730,9 @@ func hardcoverEditionToModel(e hcEdition) models.Edition { ed.ISBN10 = nonEmptyStringPtr(e.ISBN10) ed.ISBN13 = nonEmptyStringPtr(e.ISBN13) ed.ASIN = nonEmptyStringPtr(e.ASIN) + if hasPositiveInt(e.AudioSeconds) { + ed.DurationSeconds = *e.AudioSeconds + } return ed } diff --git a/internal/metadata/hardcover/client_test.go b/internal/metadata/hardcover/client_test.go index f9eb2cce4..1dce32acb 100644 --- a/internal/metadata/hardcover/client_test.go +++ b/internal/metadata/hardcover/client_test.go @@ -10,6 +10,8 @@ import ( "strings" "testing" + "github.com/vavallee/bindery/internal/bookhydrate" + "github.com/vavallee/bindery/internal/db" "github.com/vavallee/bindery/internal/metadata" "github.com/vavallee/bindery/internal/models" ) @@ -1178,6 +1180,9 @@ func TestGetEditions_FormatFallbacks(t *testing.T) { if !ebook.IsEbook { t.Error("Kindle edition should be marked as ebook") } + if ebook.DurationSeconds != 0 { + t.Errorf("ebook DurationSeconds = %d, want 0", ebook.DurationSeconds) + } if ebook.PublishDate == nil || ebook.PublishDate.Format("2006-01-02") != "2021-01-01" { t.Errorf("PublishDate = %v, want 2021-01-01", ebook.PublishDate) } @@ -1188,6 +1193,49 @@ func TestGetEditions_FormatFallbacks(t *testing.T) { if audio.IsEbook { t.Error("audiobook should not be marked as ebook") } + if audio.DurationSeconds != 3600 { + t.Errorf("audiobook DurationSeconds = %d, want 3600", audio.DurationSeconds) + } +} + +func TestGetEditions_HydratesBookAudioDuration(t *testing.T) { + client := newMockClient(func(r *http.Request) (*http.Response, error) { + return gqlResponse(t, http.StatusOK, map[string]interface{}{"editions": []map[string]interface{}{ + {"id": 101, "title": "Audio Edition", "physical_format": "Audiobook", "audio_seconds": 36000}, + }}), nil + }) + database, err := db.OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + author := &models.Author{ForeignID: "hc:duration-author", Name: "Author", SortName: "Author", Monitored: true} + if err := db.NewAuthorRepo(database).Create(ctx, author); err != nil { + t.Fatal(err) + } + books := db.NewBookRepo(database) + book := &models.Book{ + ForeignID: "hc:duration-book", AuthorID: author.ID, Title: "Duration Book", SortTitle: "Duration Book", + MetadataProvider: "hardcover", MediaType: models.MediaTypeAudiobook, Status: models.BookStatusWanted, + Monitored: true, Genres: []string{}, + } + if err := books.Create(ctx, book); err != nil { + t.Fatal(err) + } + result := bookhydrate.HydrateHardcoverEditions(ctx, bookhydrate.Options{ + Book: book, Provider: "hardcover", Editions: db.NewEditionRepo(database), Books: books, FetchEditions: client.GetEditions, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + if result.Upserted != 1 || !result.BookUpdated || stored.DurationSeconds != 36000 { + t.Fatalf("Hardcover duration was not persisted before grab: duration=%d result=%+v", stored.DurationSeconds, result) + } } func TestGetEditions_NumericIDUsesBookID(t *testing.T) { diff --git a/internal/models/edition.go b/internal/models/edition.go index fb046e7c8..745ddd2c7 100644 --- a/internal/models/edition.go +++ b/internal/models/edition.go @@ -21,4 +21,6 @@ type Edition struct { Monitored bool `json:"monitored"` CreatedAt time.Time `json:"createdAt"` UpdatedAt time.Time `json:"updatedAt"` + // DurationSeconds carries provider audio runtime to book hydration; it is not stored on editions. + DurationSeconds int `json:"-"` } From 2a9e1751f8b5c3ef07c1e0b581789eb56cc92d5a Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Wed, 23 Sep 2026 21:13:21 -0500 Subject: [PATCH 2/6] test(db): cover guarded audiobook duration updates (#2755) Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- internal/db/books_duration_test.go | 121 +++++++++++++++++++++++++++++ 1 file changed, 121 insertions(+) create mode 100644 internal/db/books_duration_test.go diff --git a/internal/db/books_duration_test.go b/internal/db/books_duration_test.go new file mode 100644 index 000000000..56e714b23 --- /dev/null +++ b/internal/db/books_duration_test.go @@ -0,0 +1,121 @@ +package db + +import ( + "context" + "errors" + "testing" + + "github.com/vavallee/bindery/internal/models" +) + +func TestBookRepoFillMissingAudiobookDuration(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + books := NewBookRepo(database) + author := mkAuthor(t, NewAuthorRepo(database), ctx, "OL-DURATION-A") + book := mkBook(t, books, ctx, author.ID, "hc:duration-book", "Duration Book", models.BookStatusWanted) + book.MetadataProvider = "hardcover" + book.MediaType = models.MediaTypeAudiobook + book.ASIN = "B000AUDIO1" + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + + fetched := *book + fetched.DurationSeconds = 36000 + updated, current, err := books.FillMissingAudiobookDuration(ctx, &fetched) + if err != nil || !updated || current != 36000 { + t.Fatalf("initial duration fill: updated=%v current=%d err=%v", updated, current, err) + } + + stored, err := books.GetByID(ctx, book.ID) + if err != nil || stored == nil { + t.Fatalf("load stored book: %v", err) + } + if stored.DurationSeconds != 36000 { + t.Fatalf("stored duration = %d, want 36000", stored.DurationSeconds) + } + stored.Monitored = false + stored.Status = models.BookStatusSkipped + stored.Narrator = "Concurrent narrator" + if err := books.Update(ctx, stored); err != nil { + t.Fatal(err) + } + + fetched.DurationSeconds = 72000 + updated, current, err = books.FillMissingAudiobookDuration(ctx, &fetched) + if err != nil || updated || current != 36000 { + t.Fatalf("existing duration should win: updated=%v current=%d err=%v", updated, current, err) + } + stored, err = books.GetByID(ctx, book.ID) + if err != nil || stored == nil { + t.Fatalf("reload stored book: %v", err) + } + if stored.DurationSeconds != 36000 || stored.Monitored || stored.Status != models.BookStatusSkipped || stored.Narrator != "Concurrent narrator" { + t.Fatalf("guarded write changed concurrent fields: %+v", stored) + } + + stored.ASIN = "B000OTHER1" + stored.DurationSeconds = 42000 + if err := books.Update(ctx, stored); err != nil { + t.Fatal(err) + } + updated, current, err = books.FillMissingAudiobookDuration(ctx, &fetched) + if err != nil || updated || current != 0 { + t.Fatalf("changed ASIN must not return another narration's duration: updated=%v current=%d err=%v", updated, current, err) + } + stored, err = books.GetByID(ctx, book.ID) + if err != nil || stored == nil { + t.Fatalf("reload changed book: %v", err) + } + if stored.ASIN != "B000OTHER1" || stored.DurationSeconds != 42000 { + t.Fatalf("changed book was overwritten: %+v", stored) + } + + fetched.MediaType = models.MediaTypeEbook + updated, current, err = books.FillMissingAudiobookDuration(ctx, &fetched) + if err != nil || updated || current != 0 { + t.Fatalf("ebook must not receive audio duration: updated=%v current=%d err=%v", updated, current, err) + } + fetched.MediaType = models.MediaTypeAudiobook + fetched.DurationSeconds = 0 + if _, _, err := books.FillMissingAudiobookDuration(ctx, &fetched); err == nil { + t.Fatal("zero candidate duration should be rejected") + } + if _, _, err := books.FillMissingAudiobookDuration(ctx, nil); err == nil { + t.Fatal("nil book should be rejected") + } + fetched.DurationSeconds = 72000 + cancelled, cancel := context.WithCancel(ctx) + cancel() + if _, _, err := books.FillMissingAudiobookDuration(cancelled, &fetched); !errors.Is(err, context.Canceled) { + t.Fatalf("cancelled write error = %v, want context.Canceled", err) + } +} + +func TestBookRepoFillMissingAudiobookDurationForBoth(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + books := NewBookRepo(database) + author := mkAuthor(t, NewAuthorRepo(database), ctx, "OL-DURATION-B") + book := mkBook(t, books, ctx, author.ID, "hc:both-book", "Both Book", models.BookStatusWanted) + book.MetadataProvider = "hardcover" + book.MediaType = models.MediaTypeBoth + book.DurationSeconds = 5400 + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + book.DurationSeconds = 6300 + updated, current, err := books.FillMissingAudiobookDuration(ctx, book) + if err != nil || updated || current != 5400 { + t.Fatalf("known dual-format duration should win: updated=%v current=%d err=%v", updated, current, err) + } +} From f033e8f84c8abb989e88d1eea6555668dc86bee5 Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:13:55 -0500 Subject: [PATCH 3/6] fix(db): normalize blank edition metadata fields (#2755) Treat whitespace-only edition text as missing so provider refreshes can fill legacy rows. Trim incoming text while preserving meaningful stored values, including their original spacing. Cover all edition text fields with empty, ASCII whitespace, Unicode whitespace, and curated-value cases. Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- .../2755-hardcover-audiobook-duration.md | 2 +- internal/db/editions.go | 54 +++++++++----- internal/db/editions_test.go | 73 ++++++++++++++----- 3 files changed, 89 insertions(+), 40 deletions(-) diff --git a/changelog.d/2755-hardcover-audiobook-duration.md b/changelog.d/2755-hardcover-audiobook-duration.md index a9083c769..361d2a347 100644 --- a/changelog.d/2755-hardcover-audiobook-duration.md +++ b/changelog.d/2755-hardcover-audiobook-duration.md @@ -1,2 +1,2 @@ ### Fixed -- **Hardcover audiobook duration** (#2755, #2756): use a known audio edition runtime before searching, and let metadata refresh replace blank edition ASINs. +- **Hardcover audiobook duration** (#2755, #2756): use a known audio edition runtime before searching, and let metadata refresh replace whitespace-only edition text fields. diff --git a/internal/db/editions.go b/internal/db/editions.go index e840988de..640eefb28 100644 --- a/internal/db/editions.go +++ b/internal/db/editions.go @@ -162,15 +162,21 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo if e == nil || strings.TrimSpace(e.ForeignID) == "" || e.BookID == 0 { return false, nil } - if strings.TrimSpace(e.Title) == "" { + for _, value := range []*string{&e.Title, &e.Publisher, &e.Format, &e.Language, &e.ImageURL, &e.EditionInfo} { + *value = strings.TrimSpace(*value) + } + if e.Title == "" { e.Title = "Unknown Edition" } - if e.ASIN != nil { - asin := strings.TrimSpace(*e.ASIN) - if asin == "" { - e.ASIN = nil + for _, value := range []**string{&e.ISBN13, &e.ISBN10, &e.ASIN} { + if *value == nil { + continue + } + trimmed := strings.TrimSpace(**value) + if trimmed == "" { + *value = nil } else { - e.ASIN = &asin + *value = &trimmed } } @@ -181,33 +187,43 @@ func (r *EditionRepo) UpsertMetadata(ctx context.Context, e *models.Edition) (bo } monitored := 1 // SQLite TRIM defaults to ASCII space; include Go's Unicode White_Space set - // so legacy blank ASINs use the same rule as incoming strings.TrimSpace. - const asinWhitespace = " \t\n\v\f\r\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" + // so legacy blank text uses the same rule as incoming strings.TrimSpace. + const metadataWhitespace = " \t\n\v\f\r\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000" + // Column names below are constants, never provider input. Preserve meaningful + // stored values verbatim, including any surrounding whitespace. + fillMissingText := func(column string) string { + return fmt.Sprintf("CASE WHEN TRIM(COALESCE(editions.%[1]s, ''), @whitespace) = '' THEN excluded.%[1]s ELSE editions.%[1]s END", column) + } - res, err := r.db.ExecContext(ctx, ` + //nolint:gosec // G201: expressions contain only fixed column names; all metadata is bound below. + query := fmt.Sprintf(` INSERT INTO editions (foreign_id, book_id, title, isbn_13, isbn_10, asin, publisher, publish_date, format, num_pages, language, image_url, is_ebook, edition_info, monitored, created_at, updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) ON CONFLICT(foreign_id) DO UPDATE SET - title = COALESCE(NULLIF(editions.title, ''), excluded.title), - isbn_13 = COALESCE(NULLIF(editions.isbn_13, ''), excluded.isbn_13), - isbn_10 = COALESCE(NULLIF(editions.isbn_10, ''), excluded.isbn_10), - asin = CASE WHEN TRIM(COALESCE(editions.asin, ''), ?) = '' THEN excluded.asin ELSE editions.asin END, - publisher = COALESCE(NULLIF(editions.publisher, ''), excluded.publisher), + title = %s, + isbn_13 = %s, + isbn_10 = %s, + asin = %s, + publisher = %s, publish_date= COALESCE(editions.publish_date, excluded.publish_date), - format = COALESCE(NULLIF(editions.format, ''), excluded.format), + format = %s, num_pages = COALESCE(editions.num_pages, excluded.num_pages), - language = COALESCE(NULLIF(editions.language, ''), excluded.language), - image_url = COALESCE(NULLIF(editions.image_url, ''), excluded.image_url), + language = %s, + image_url = %s, is_ebook = CASE WHEN editions.is_ebook = 1 THEN 1 ELSE excluded.is_ebook END, - edition_info= COALESCE(NULLIF(editions.edition_info, ''), excluded.edition_info), + edition_info= %s, updated_at = excluded.updated_at WHERE editions.book_id = excluded.book_id`, + fillMissingText("title"), fillMissingText("isbn_13"), fillMissingText("isbn_10"), fillMissingText("asin"), + fillMissingText("publisher"), fillMissingText("format"), fillMissingText("language"), fillMissingText("image_url"), fillMissingText("edition_info")) + res, err := r.db.ExecContext(ctx, query, e.ForeignID, e.BookID, e.Title, e.ISBN13, e.ISBN10, e.ASIN, e.Publisher, timeArg(e.PublishDate), e.Format, e.NumPages, e.Language, - e.ImageURL, isEbook, e.EditionInfo, monitored, timeValueArg(now), timeValueArg(now), asinWhitespace) + e.ImageURL, isEbook, e.EditionInfo, monitored, timeValueArg(now), timeValueArg(now), + sql.Named("whitespace", metadataWhitespace)) if err != nil { return false, fmt.Errorf("upsert metadata edition %s: %w", e.ForeignID, err) } diff --git a/internal/db/editions_test.go b/internal/db/editions_test.go index ecfa09e33..1d85a4dd3 100644 --- a/internal/db/editions_test.go +++ b/internal/db/editions_test.go @@ -2,6 +2,7 @@ package db import ( "context" + "strings" "testing" "github.com/vavallee/bindery/internal/models" @@ -246,7 +247,7 @@ func TestEditionRepo_UpsertMetadataFillsMissingFields(t *testing.T) { } } -func TestEditionRepo_UpsertMetadataReplacesWhitespaceASIN(t *testing.T) { +func TestEditionRepo_UpsertMetadataNormalizesBlankText(t *testing.T) { database, err := OpenMemory() if err != nil { t.Fatal(err) @@ -264,38 +265,70 @@ func TestEditionRepo_UpsertMetadataReplacesWhitespaceASIN(t *testing.T) { } repo := NewEditionRepo(database) - for _, tc := range []struct{ name, blank string }{ - {name: "ASCII whitespace", blank: " \t "}, - {name: "Unicode whitespace", blank: "\u00a0\u2003"}, + textEdition := func(foreignID, value string) *models.Edition { + return &models.Edition{ + ForeignID: foreignID, BookID: book.ID, Title: value, + ISBN13: &value, ISBN10: &value, ASIN: &value, + Publisher: value, Format: value, Language: value, ImageURL: value, EditionInfo: value, + } + } + assertText := func(t *testing.T, got *models.Edition, want string) { + t.Helper() + wantTitle := want + if want == "" { + wantTitle = "Unknown Edition" + } + if got.Title != wantTitle { + t.Errorf("title = %q, want %q", got.Title, wantTitle) + } + for name, value := range map[string]string{"publisher": got.Publisher, "format": got.Format, "language": got.Language, "image_url": got.ImageURL, "edition_info": got.EditionInfo} { + if value != want { + t.Errorf("%s = %q, want %q", name, value, want) + } + } + for name, value := range map[string]*string{"isbn_13": got.ISBN13, "isbn_10": got.ISBN10, "asin": got.ASIN} { + if want == "" { + if value != nil { + t.Errorf("%s = %q, want nil", name, *value) + } + } else if value == nil || *value != want { + t.Errorf("%s = %v, want %q", name, value, want) + } + } + } + for _, tc := range []struct { + name, stored string + want string + }{ + {name: "empty", stored: "", want: "replacement"}, + {name: "ASCII whitespace", stored: " \t\n\v\f\r", want: "replacement"}, + {name: "Unicode whitespace", stored: "\u0085\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000", want: "replacement"}, + {name: "meaningful stored text", stored: " \u2003curated\t ", want: " \u2003curated\t "}, } { t.Run(tc.name, func(t *testing.T) { - edition := &models.Edition{ForeignID: "hc:asin-" + tc.name, BookID: book.ID, Title: "Audio Edition", ASIN: &tc.blank} + edition := textEdition("hc:text-"+tc.name, tc.stored) if ok, err := repo.UpsertMetadata(ctx, edition); err != nil || !ok { - t.Fatalf("insert metadata edition: ok=%v err=%v", ok, err) + t.Fatalf("insert: ok=%v err=%v", ok, err) } - if edition.ASIN != nil { - t.Fatalf("blank ASIN should be stored as missing, got %q", *edition.ASIN) - } - - // Simulate a legacy row written before whitespace ASINs were normalized. - if _, err := database.ExecContext(ctx, "UPDATE editions SET asin = ? WHERE id = ?", tc.blank, edition.ID); err != nil { + assertText(t, edition, strings.TrimSpace(tc.stored)) + // Simulate legacy blank fields and curated text whose spacing must survive. + if _, err := database.ExecContext(ctx, `UPDATE editions SET title=?1, isbn_13=?1, isbn_10=?1, asin=?1, + publisher=?1, format=?1, language=?1, image_url=?1, edition_info=?1 WHERE id=?2`, tc.stored, edition.ID); err != nil { t.Fatal(err) } - valid := "B012345678" - incoming := &models.Edition{ForeignID: edition.ForeignID, BookID: book.ID, Title: edition.Title, ASIN: &valid} + incoming := textEdition(edition.ForeignID, " \u2003replacement\t ") if ok, err := repo.UpsertMetadata(ctx, incoming); err != nil || !ok { - t.Fatalf("update metadata edition: ok=%v err=%v", ok, err) - } - if incoming.ASIN == nil || *incoming.ASIN != valid { - t.Fatalf("upsert returned ASIN %v, want %q", incoming.ASIN, valid) + t.Fatalf("update: ok=%v err=%v", ok, err) } + assertText(t, incoming, tc.want) stored, err := repo.GetByForeignID(ctx, edition.ForeignID) if err != nil { t.Fatal(err) } - if stored == nil || stored.ASIN == nil || *stored.ASIN != valid { - t.Fatalf("persisted ASIN = %v, want %q", stored, valid) + if stored == nil { + t.Fatal("edition not persisted") } + assertText(t, stored, tc.want) }) } } From 7fe1075b79541a9d0ed629e65ca6a6b52098682c Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Thu, 24 Sep 2026 17:14:05 -0500 Subject: [PATCH 4/6] fix(metadata): respect audio evidence and language locks (#2755) Rank explicit audio evidence ahead of runtime and use runtime only to break equally ranked edition ties. Do not infer audio from the runtime of a known print format, including when a stored unknown format remains. Preserve deliberately empty locked languages and cover duration writes combined with language or cover metadata. Document the selection rules. Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- .../2755-hardcover-audiobook-duration.md | 2 +- docs/User-Guide-Wiki.md | 5 +- internal/bookhydrate/hardcover.go | 23 ++- internal/bookhydrate/hardcover_test.go | 147 +++++++++++++++--- 4 files changed, 151 insertions(+), 26 deletions(-) diff --git a/changelog.d/2755-hardcover-audiobook-duration.md b/changelog.d/2755-hardcover-audiobook-duration.md index 361d2a347..34e4878ae 100644 --- a/changelog.d/2755-hardcover-audiobook-duration.md +++ b/changelog.d/2755-hardcover-audiobook-duration.md @@ -1,2 +1,2 @@ ### Fixed -- **Hardcover audiobook duration** (#2755, #2756): use a known audio edition runtime before searching, and let metadata refresh replace whitespace-only edition text fields. +- **Hardcover audiobook duration** (#2755, #2756): use a known audio edition runtime before searching without prioritizing it over stronger audio metadata or promoting print formats; preserve locked languages and let metadata refresh replace whitespace-only edition text fields. diff --git a/docs/User-Guide-Wiki.md b/docs/User-Guide-Wiki.md index 1d3c291ef..e5c94f541 100644 --- a/docs/User-Guide-Wiki.md +++ b/docs/User-Guide-Wiki.md @@ -568,7 +568,10 @@ Things worth knowing: For a Hardcover audiobook, the chosen audio edition can fill a missing book duration before indexer search. Audnex may update that duration when an ASIN is -available. +available. Explicit audio formats take priority over an unknown format with a +runtime; runtime breaks ties between equally ranked editions. A runtime alone +does not turn a known print format into an audiobook. Edition hydration respects +a manually locked language, including a language deliberately cleared to empty. Which of those a given book actually came from is on the book page, under **Metadata source**. It names the provider, shows the identifier the book is diff --git a/internal/bookhydrate/hardcover.go b/internal/bookhydrate/hardcover.go index bf2e2c29e..821eae999 100644 --- a/internal/bookhydrate/hardcover.go +++ b/internal/bookhydrate/hardcover.go @@ -122,6 +122,11 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { result.Upserted++ // UpsertMetadata reloads persisted fields; runtime exists only in the provider response. edition.DurationSeconds = editions[i].DurationSeconds + if !isLikelyAudioEdition(editions[i]) { + // A retained unknown format must not hide a fetched print format and + // turn that print edition into audio solely through its runtime. + edition.DurationSeconds = 0 + } if isLikelyAudioEdition(edition) { if edition.ASIN != nil && strings.TrimSpace(*edition.ASIN) == "" { // UpsertMetadata can retain a whitespace-only ASIN; use the fetched identity for hydration. @@ -203,8 +208,8 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { // preferredAudioEdition returns the audio-looking edition whose ASIN matches // the book, or the edition maybePromoteASIN will use when the book has no ASIN. -// Among matching ASINs, prefer an edition with a known runtime, then the -// highest audioEditionScore (first on ties). +// Among matching ASINs, prefer the highest audioEditionScore, then an edition +// with a known runtime (first on ties). func preferredAudioEdition(editions []models.Edition, bookASIN string) (models.Edition, bool) { best := -1 targetASIN := strings.TrimSpace(bookASIN) @@ -224,15 +229,19 @@ func preferredAudioEdition(editions []models.Edition, bookASIN string) (models.E } continue } + score, bestScore := audioEditionScore(editions[i]), audioEditionScore(editions[best]) + if score != bestScore { + if score > bestScore { + best = i + } + continue + } if (matchesASIN || targetASIN == "") && (editions[i].DurationSeconds > 0) != (editions[best].DurationSeconds > 0) { if editions[i].DurationSeconds > 0 { best = i } continue } - if audioEditionScore(editions[i]) > audioEditionScore(editions[best]) { - best = i - } } if best == -1 { return models.Edition{}, false @@ -365,7 +374,9 @@ func isLikelyAudioEdition(edition models.Edition) bool { edition.Format, edition.EditionInfo, }, " ")) - return editionHasAudioMarker(text) || (edition.DurationSeconds > 0 && !edition.IsEbook) + format := strings.TrimSpace(edition.Format) + unknownFormat := format == "" || strings.EqualFold(format, "unknown") + return editionHasAudioMarker(text) || (unknownFormat && edition.DurationSeconds > 0 && !edition.IsEbook) } func editionHasAudioMarker(text string) bool { diff --git a/internal/bookhydrate/hardcover_test.go b/internal/bookhydrate/hardcover_test.go index dd7c4e80e..8e6a2fcb8 100644 --- a/internal/bookhydrate/hardcover_test.go +++ b/internal/bookhydrate/hardcover_test.go @@ -3,6 +3,7 @@ package bookhydrate import ( "context" "errors" + "fmt" "testing" "github.com/vavallee/bindery/internal/db" @@ -241,26 +242,136 @@ func TestHydrateHardcoverEditionsDerivesSelectedAudioDuration(t *testing.T) { } } -func TestHydrateHardcoverEditionsPrefersRuntimeWhenEditionsHaveNoASIN(t *testing.T) { - books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) - result := HydrateHardcoverEditions(ctx, Options{ - Book: book, Provider: "hardcover", Editions: editions, Books: books, - FetchEditions: func(context.Context, string) ([]models.Edition, error) { - return []models.Edition{ - {ForeignID: "hc:labeled", Format: "Audiobook"}, - {ForeignID: "hc:runtime", Format: "Unknown", DurationSeconds: 36000}, - }, nil - }, - }) - if result.Err != nil { - t.Fatal(result.Err) +func TestHydrateHardcoverEditionsRanksAudioEvidenceBeforeRuntime(t *testing.T) { + for _, matchingASIN := range []bool{false, true} { + for _, runtimeFormat := range []string{"Unknown", "Audiobook"} { + for _, reverse := range []bool{false, true} { + name := fmt.Sprintf("matchingASIN=%v/format=%s/reverse=%v", matchingASIN, runtimeFormat, reverse) + t.Run(name, func(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) + var asin *string + if matchingASIN { + book.ASIN = "B000AUDIO1" + asin = &book.ASIN + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + } + fetched := []models.Edition{ + {ForeignID: "hc:labeled", Format: "Audiobook", ASIN: asin, Language: "eng", ImageURL: "good"}, + {ForeignID: "hc:runtime", Format: runtimeFormat, ASIN: asin, Language: "ger", ImageURL: "other", DurationSeconds: 36000}, + } + if reverse { + fetched[0], fetched[1] = fetched[1], fetched[0] + } + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { return fetched, nil }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + wantLanguage, wantCover, wantDuration := "eng", "good", 0 + if runtimeFormat == "Audiobook" { + wantLanguage, wantCover, wantDuration = "ger", "other", 36000 + } + if stored.Language != wantLanguage || stored.ImageURL != wantCover || stored.DurationSeconds != wantDuration || !result.BookUpdated { + t.Fatalf("stored language=%q cover=%q duration=%d; want %q %q %d; result=%+v", stored.Language, stored.ImageURL, stored.DurationSeconds, wantLanguage, wantCover, wantDuration, result) + } + }) + } + } } - stored, err := books.GetByID(ctx, book.ID) - if err != nil { - t.Fatal(err) +} + +func TestHydrateHardcoverEditionsRuntimeDoesNotOverrideKnownFormat(t *testing.T) { + for _, tc := range []struct{ format, storedFormat string }{ + {format: "Hardcover"}, {format: "Paperback"}, {format: "Mass Market Paperback"}, + {format: "Kindle"}, {format: "Unknown"}, {format: ""}, {format: "Audiobook"}, + {format: "Hardcover", storedFormat: "Unknown"}, {format: "Paperback", storedFormat: "Unknown"}, + } { + t.Run(tc.format+"/stored="+tc.storedFormat, func(t *testing.T) { + format := tc.format + books, editions, book, ctx := newHydrateBook(t, "hc:format-book", "hardcover", "") + asin := "B00FORMAT1" + if tc.storedFormat != "" { + if ok, err := editions.UpsertMetadata(ctx, &models.Edition{ForeignID: "hc:edition", BookID: book.ID, Format: tc.storedFormat}); err != nil || !ok { + t.Fatalf("seed edition: ok=%v err=%v", ok, err) + } + } + isEbook := format == "Kindle" + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + return []models.Edition{{ForeignID: "hc:edition", Format: format, IsEbook: isEbook, ASIN: &asin, Language: "por", DurationSeconds: 36000}}, nil + }, + }) + if result.Err != nil { + t.Fatal(result.Err) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + wantAudio := format == "Unknown" || format == "" || format == "Audiobook" + if wantAudio { + if stored.MediaType != models.MediaTypeAudiobook || stored.ASIN != asin || stored.DurationSeconds != 36000 || !result.ASINPromoted { + t.Fatalf("audio metadata not persisted: book=%+v result=%+v", stored, result) + } + } else if stored.MediaType != models.MediaTypeEbook || stored.ASIN != "" || stored.Language != "" || stored.DurationSeconds != 0 || result.BookUpdated || result.ASINPromoted { + t.Fatalf("non-audio edition promoted: book=%+v result=%+v", stored, result) + } + }) } - if stored.DurationSeconds != 36000 || !result.BookUpdated { - t.Fatalf("runtime from unlabeled audio edition was lost: duration=%d result=%+v", stored.DurationSeconds, result) +} + +func TestHydrateHardcoverEditionsPersistsDurationWithMetadataAndLanguageLock(t *testing.T) { + for _, tc := range []struct { + name, language, cover string + locked bool + }{ + {name: "language", language: "ger"}, + {name: "cover", cover: "https://example.com/cover.jpg"}, + {name: "locked language duration only", language: "ger", locked: true}, + {name: "locked language with cover", language: "ger", cover: "https://example.com/cover.jpg", locked: true}, + } { + t.Run(tc.name, func(t *testing.T) { + books, editions, book, ctx := newHydrateBook(t, "hc:metadata-duration", "hardcover", models.MediaTypeAudiobook) + book.ASIN = "B000AUDIO1" + if tc.locked { + book.LockField(models.BookFieldLanguage) + } + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + asin := book.ASIN + result := HydrateHardcoverEditions(ctx, Options{ + Book: book, Provider: "hardcover", Editions: editions, Books: books, + FetchEditions: func(context.Context, string) ([]models.Edition, error) { + return []models.Edition{{ForeignID: "hc:audio", Format: "Audiobook", ASIN: &asin, Language: tc.language, ImageURL: tc.cover, DurationSeconds: 36000}}, nil + }, + }) + if result.Err != nil || !result.BookUpdated || !result.MetadataDerived || result.ASINPromoted { + t.Fatalf("unexpected hydration result: %+v", result) + } + stored, err := books.GetByID(ctx, book.ID) + if err != nil { + t.Fatal(err) + } + wantLanguage := tc.language + if tc.locked { + wantLanguage = "" + } + for _, got := range []*models.Book{book, stored} { + if got.Language != wantLanguage || got.ImageURL != tc.cover || got.DurationSeconds != 36000 || got.IsFieldLocked(models.BookFieldLanguage) != tc.locked { + t.Fatalf("duration/metadata/lock not preserved: %+v", got) + } + } + }) } } From 33077b1f531cb1d0dc061711585c4a5d50a909e2 Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:00:40 -0500 Subject: [PATCH 5/6] refactor(metadata): drop unreachable whitespace ASIN fallback (#2755) UpsertMetadata now replaces a stored whitespace-only ASIN with the fetched value or NULL, so hydration can no longer read one back. Remove the in-memory fallback and its outdated comment, and seed the regression case through the verbatim Upsert writer so it exercises a real legacy row. Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- internal/bookhydrate/hardcover.go | 4 ---- internal/bookhydrate/hardcover_test.go | 18 +++++++++++++++--- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/internal/bookhydrate/hardcover.go b/internal/bookhydrate/hardcover.go index 821eae999..bd8883c57 100644 --- a/internal/bookhydrate/hardcover.go +++ b/internal/bookhydrate/hardcover.go @@ -128,10 +128,6 @@ func HydrateHardcoverEditions(ctx context.Context, opts Options) Result { edition.DurationSeconds = 0 } if isLikelyAudioEdition(edition) { - if edition.ASIN != nil && strings.TrimSpace(*edition.ASIN) == "" { - // UpsertMetadata can retain a whitespace-only ASIN; use the fetched identity for hydration. - edition.ASIN = editions[i].ASIN - } if editions[i].ASIN != nil && edition.ASIN != nil && strings.TrimSpace(*editions[i].ASIN) != "" && !strings.EqualFold(strings.TrimSpace(*editions[i].ASIN), strings.TrimSpace(*edition.ASIN)) { // The stored ASIN belongs to a different narration than the fetched runtime. diff --git a/internal/bookhydrate/hardcover_test.go b/internal/bookhydrate/hardcover_test.go index 8e6a2fcb8..20f61f5f0 100644 --- a/internal/bookhydrate/hardcover_test.go +++ b/internal/bookhydrate/hardcover_test.go @@ -704,17 +704,29 @@ func TestHydrateHardcoverEditionsUsesRuntimeMatchingRetainedASIN(t *testing.T) { storedASIN string fetchedASIN string siblingASIN string + legacySeed bool wantASIN string wantDuration int }{ {name: "valid sibling after ASIN conflict", storedASIN: "B000RETAIN", fetchedASIN: "B000FETCHD", siblingASIN: "B000RETAIN", wantASIN: "B000RETAIN", wantDuration: 72000}, - {name: "stored whitespace ASIN", storedASIN: " ", fetchedASIN: "B000FETCHD", wantASIN: "B000FETCHD", wantDuration: 36000}, + {name: "legacy whitespace ASIN replaced by upsert", storedASIN: "  ", fetchedASIN: "B000FETCHD", legacySeed: true, wantASIN: "B000FETCHD", wantDuration: 36000}, } { t.Run(tc.name, func(t *testing.T) { books, editions, book, ctx := newHydrateBook(t, "hc:duration-book", "hardcover", models.MediaTypeAudiobook) - if ok, err := editions.UpsertMetadata(ctx, &models.Edition{ + seed := &models.Edition{ ForeignID: "hc:audio", BookID: book.ID, Title: "Audio", Format: "Audiobook", ASIN: &tc.storedASIN, - }); err != nil || !ok { + } + if tc.legacySeed { + // Upsert stores the ASIN verbatim, leaving the whitespace-only row + // that UpsertMetadata normalizes; hydration must not see it. + if err := editions.Upsert(ctx, seed); err != nil { + t.Fatalf("seed legacy edition: %v", err) + } + stored, err := editions.GetByForeignID(ctx, "hc:audio") + if err != nil || stored == nil || stored.ASIN == nil || *stored.ASIN != tc.storedASIN { + t.Fatalf("legacy seed ASIN = %+v err=%v, want %q", stored, err, tc.storedASIN) + } + } else if ok, err := editions.UpsertMetadata(ctx, seed); err != nil || !ok { t.Fatalf("seed edition ok=%v err=%v", ok, err) } result := HydrateHardcoverEditions(ctx, Options{ From b05ebf854f25f0959d1f1cbfc38e35699e02e705 Mon Sep 17 00:00:00 2001 From: magrhino <12738722+magrhino@users.noreply.github.com> Date: Fri, 25 Sep 2026 17:00:40 -0500 Subject: [PATCH 6/6] test(db): pin each identity guard on the audiobook duration fill (#2755) Change only one of foreign_id, metadata_provider, media_type or asin per case. A zero stored duration pins the guarded UPDATE; a known one pins the fallback SELECT, which must not report another identity's duration. Signed-off-by: magrhino <12738722+magrhino@users.noreply.github.com> --- internal/db/books_duration_test.go | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/internal/db/books_duration_test.go b/internal/db/books_duration_test.go index 56e714b23..d89e8e2a5 100644 --- a/internal/db/books_duration_test.go +++ b/internal/db/books_duration_test.go @@ -3,6 +3,7 @@ package db import ( "context" "errors" + "fmt" "testing" "github.com/vavallee/bindery/internal/models" @@ -119,3 +120,56 @@ func TestBookRepoFillMissingAudiobookDurationForBoth(t *testing.T) { t.Fatalf("known dual-format duration should win: updated=%v current=%d err=%v", updated, current, err) } } + +func TestBookRepoFillMissingAudiobookDurationRequiresMatchingIdentity(t *testing.T) { + for _, field := range []struct { + column, value string + }{ + {"foreign_id", "hc:rebound-book"}, + {"metadata_provider", "openlibrary"}, + {"media_type", models.MediaTypeBoth}, + {"asin", "B000OTHER1"}, + } { + // A zero stored duration pins the UPDATE clause; a known one pins the + // fallback SELECT, which must not report another identity's duration. + for _, storedDuration := range []int{0, 42000} { + t.Run(fmt.Sprintf("%s/stored=%d", field.column, storedDuration), func(t *testing.T) { + database, err := OpenMemory() + if err != nil { + t.Fatal(err) + } + defer database.Close() + ctx := context.Background() + books := NewBookRepo(database) + author := mkAuthor(t, NewAuthorRepo(database), ctx, "OL-DURATION-ID") + book := mkBook(t, books, ctx, author.ID, "hc:identity-book", "Identity Book", models.BookStatusWanted) + book.MetadataProvider = "hardcover" + book.MediaType = models.MediaTypeAudiobook + book.ASIN = "B000AUDIO1" + if err := books.Update(ctx, book); err != nil { + t.Fatal(err) + } + fetched := *book + fetched.DurationSeconds = 36000 + + // Change only one identity column, as a concurrent rebind or re-match would. + query := fmt.Sprintf("UPDATE books SET %s = ?, duration_seconds = ? WHERE id = ?", field.column) + if _, err := database.ExecContext(ctx, query, field.value, storedDuration, book.ID); err != nil { + t.Fatal(err) + } + + updated, current, err := books.FillMissingAudiobookDuration(ctx, &fetched) + if err != nil || updated || current != 0 { + t.Fatalf("changed %s: updated=%v current=%d err=%v, want no write and no current duration", field.column, updated, current, err) + } + var duration int + if err := database.QueryRowContext(ctx, "SELECT duration_seconds FROM books WHERE id = ?", book.ID).Scan(&duration); err != nil { + t.Fatal(err) + } + if duration != storedDuration { + t.Fatalf("changed %s: stored duration = %d, want %d", field.column, duration, storedDuration) + } + }) + } + } +}