From 4cd1091d16a09fcb214caf06086c11c3a341ed92 Mon Sep 17 00:00:00 2001 From: CaliBrain Date: Sun, 16 Aug 2026 20:55:08 -0400 Subject: [PATCH] fix(hardcover): send the field count Hardcover's Book search requires (#1224) Advanced title search, advanced title+author search, and the title typeahead returned zero results every time, and the sort fallback added in #1183 blamed the sort value for it. Hardcover turns the `fields` search parameter into Typesense's `query_by` but keeps `num_typos` and `query_by_weights` as fixed-length presets per query_type. For query_type=Book the preset expects exactly five fields, so a shorter list is not searched loosely - the whole search is rejected with a null results body. Confirmed against the live API: 1, 2, 3, 4 and 6 fields are all rejected, only 5 works, and weights must match one-for-one when sent. Every Book-type list we sent was the wrong length - the title typeahead and advanced title search sent 2, title+author sent 3. - Send BOOK_SEARCH_FIELDS (the full five) for every narrowed Book search and express the intent through weights instead. Weights only bias ranking - a field weighted 0 still matches - so a title search now ranks titles first rather than restricting to them. That is the closest behaviour Hardcover still allows, and there is no client-side filter to restore the old precision. - Pin the field and weight counts in tests, since the failure mode is a silent zero results rather than an error. The sort fallback from #1183 also misread these rejections: - Select the `error` field on every search and log Hardcover's own explanation. The reason is only ever in that sibling field, so a rejection surfaced as "returned no result body" with nothing to act on. Reading it is what made the field-count rule findable. - Drop `sort` entirely on the retry instead of sending an empty string. An empty sort is a value like any other and can be rejected too. - Arm the 900s sticky window only after the sortless retry succeeds. It was armed before the retry and never rolled back, so one rejected typeahead disabled sorting process-wide for 15 minutes whatever the actual cause. Verified against the live Hardcover API: advanced title search 0 -> 84 results, title+author 0 -> 139, title typeahead 0 -> 84 with the exact title top. 2566 unit tests pass; ruff, basedpyright and vulture clean. Refs #1183. The sort_by regression #1183 was written for is gone from Hardcover's side - every sort value it rejected, including the one in the report, is accepted again today. Two plain-search rejections in that report (fields=None) remain unexplained: they could not be reproduced under any per_page, page depth, sort value or query shape, and are most likely transient upstream. They now self-report the reason if they recur. --- shelfmark/metadata_providers/hardcover.py | 85 +++++++++++++------ .../metadata/test_hardcover_field_options.py | 12 ++- .../metadata/test_hardcover_search_fields.py | 82 ++++++++++++++++++ .../metadata/test_hardcover_sort_fallback.py | 85 +++++++++++++++++-- 4 files changed, 231 insertions(+), 33 deletions(-) create mode 100644 tests/metadata/test_hardcover_search_fields.py diff --git a/shelfmark/metadata_providers/hardcover.py b/shelfmark/metadata_providers/hardcover.py index 2bc9537f..71404ed6 100644 --- a/shelfmark/metadata_providers/hardcover.py +++ b/shelfmark/metadata_providers/hardcover.py @@ -318,6 +318,7 @@ query SearchFieldOptions( fields: $fields, weights: $weights ) { + error results } } @@ -536,13 +537,19 @@ SORT_MAPPING: dict[SortOrder, str] = { SortOrder.OLDEST: "release_year:asc", } -# Mapping from abstract search type to Hardcover fields parameter -SEARCH_TYPE_FIELDS: dict[SearchType, str] = { - SearchType.GENERAL: "title,isbns,series_names,author_names,alternative_titles", - SearchType.TITLE: "title,alternative_titles", - SearchType.AUTHOR: "author_names", - # ISBN is handled separately via search_by_isbn() -} +# `fields` becomes Typesense's `query_by`, but Hardcover keeps `num_typos` and +# `query_by_weights` as fixed-length presets per query_type. Passing a different +# number of fields than the preset expects makes Typesense reject the whole search, +# complaining that the number of num_typos values does not match the number of +# query_by fields. So a Book search may only ever narrow to *these five* names -- +# a shorter list is rejected outright rather than searched, and any weights sent +# alongside must match one-for-one. +# Weights only bias ranking: a field weighted 0 still matches, so `fields` can no +# longer restrict which fields a Book query looks at. +BOOK_SEARCH_FIELDS = "title,alternative_titles,author_names,series_names,isbns" +BOOK_SEARCH_FIELD_COUNT = 5 +BOOK_TITLE_WEIGHTS = "5,1,0,0,0" +BOOK_TITLE_AUTHOR_WEIGHTS = "5,1,3,0,0" SERIES_SEARCH_FIELDS = "name,books,author_name" SERIES_SEARCH_WEIGHTS = "2,1,1" @@ -550,22 +557,28 @@ SERIES_SEARCH_SORT = "_text_match:desc,readers_count:desc" AUTHOR_SUGGESTION_FIELDS = "name,name_personal,alternate_names" AUTHOR_SUGGESTION_WEIGHTS = "4,3,2" AUTHOR_SUGGESTION_SORT = "_text_match:desc,books_count:desc" -TITLE_SUGGESTION_FIELDS = "title,alternative_titles" -TITLE_SUGGESTION_WEIGHTS = "5,2" +TITLE_SUGGESTION_FIELDS = BOOK_SEARCH_FIELDS +TITLE_SUGGESTION_WEIGHTS = "5,2,0,0,0" TITLE_SUGGESTION_SORT = "_text_match:desc,users_count:desc" # Hardcover forwards `sort` to Typesense's `sort_by` and rejects the whole search # if it does not like the value -- an unknown field, a bare field name with no # direction, more than three keys. A rejected search comes back as HTTP 200 with # no GraphQL errors and a null `results` body, which is otherwise indistinguishable -# from "nothing matched". An empty sort is always accepted, so fall back to it and -# keep the fallback sticky for a while rather than paying for a doomed request on -# every search. -SORT_FALLBACK = "" +# from "nothing matched"; the reason only shows up in the sibling `error` field, +# so every search asks for it. Dropping `sort` from the request is the one shape +# Hardcover always accepts -- an empty string is a value like any other and has +# been rejected too -- so retry that way and keep the fallback sticky for a while +# rather than paying for a doomed request on every search. SORT_FALLBACK_TTL = 900.0 _sort_fallback_until = 0.0 +def _without_sort(variables: dict[str, Any]) -> dict[str, Any]: + """Drop `sort` entirely so Hardcover applies its own default ordering.""" + return {key: value for key, value in variables.items() if key != "sort"} + + def _search_payload_rejected(result: dict[str, Any] | None) -> bool: """Report whether Hardcover answered a search with a null results body. @@ -580,6 +593,17 @@ def _search_payload_rejected(result: dict[str, Any] | None) -> bool: return root["results"] is None +def _search_rejection_reason(result: dict[str, Any] | None) -> str: + """Return Hardcover's explanation for a rejected search, if it sent one.""" + if not isinstance(result, dict): + return "" + root = result.get("search", result) + if not isinstance(root, dict): + return "" + error = root.get("error") + return error.strip() if isinstance(error, str) else "" + + def _combine_headline_description(headline: str | None, description: str | None) -> str | None: """Combine headline (tagline) and description into a single description.""" if headline and description: @@ -1012,13 +1036,15 @@ class HardcoverProvider(MetadataProvider): """Build search query, fields, and weights based on provided values. Returns (query, fields, weights) tuple. Fields/weights are None for general search. + A narrowed search still sends all of BOOK_SEARCH_FIELDS -- Hardcover rejects a + shorter list outright -- and leans on the weights to rank the wanted field first. """ if author and not title and not series: return author, None, None if title and not author and not series: - return title, "title,alternative_titles", "5,1" + return title, BOOK_SEARCH_FIELDS, BOOK_TITLE_WEIGHTS if author and title and not series: - return f"{title} {author}", "title,alternative_titles,author_names", "5,1,3" + return f"{title} {author}", BOOK_SEARCH_FIELDS, BOOK_TITLE_AUTHOR_WEIGHTS return default_query, None, None def _detect_list_url(self, query: str) -> tuple[str | None, str] | None: @@ -2384,6 +2410,7 @@ class HardcoverProvider(MetadataProvider): graphql_query = """ query SearchBooks($query: String!, $limit: Int!, $page: Int!, $sort: String, $fields: String, $weights: String) { search(query: $query, query_type: "Book", per_page: $limit, page: $page, sort: $sort, fields: $fields, weights: $weights) { + error results } } @@ -2392,6 +2419,7 @@ class HardcoverProvider(MetadataProvider): graphql_query = """ query SearchBooks($query: String!, $limit: Int!, $page: Int!, $sort: String) { search(query: $query, query_type: "Book", per_page: $limit, page: $page, sort: $sort) { + error results } } @@ -2690,33 +2718,42 @@ class HardcoverProvider(MetadataProvider): sort = variables.get("sort") if sort and time.monotonic() < _sort_fallback_until: - variables = {**variables, "sort": SORT_FALLBACK} + variables = _without_sort(variables) sort = None result = self._execute_query(query, variables) if not _search_payload_rejected(result): return result + reason = _search_rejection_reason(result) if not sort: logger.error( - "Hardcover rejected this search (query_type=%s, fields=%s) and returned " - "no result body", + "Hardcover rejected this search (query_type=%s, fields=%s): %s", variables.get("queryType", "Book"), variables.get("fields"), + reason or "no error message", + ) + return None + + retry = self._execute_query(query, _without_sort(variables)) + if _search_payload_rejected(retry): + # The sort was not the culprit, so leave sorting alone for other searches. + logger.error( + "Hardcover rejected this search (query_type=%s, fields=%s) with and without " + "a sort order: %s", + variables.get("queryType", "Book"), + variables.get("fields"), + _search_rejection_reason(retry) or reason or "no error message", ) return None logger.warning( - "Hardcover rejected sort '%s'; retrying searches without a sort order for %ss", + "Hardcover rejected sort '%s' (%s); dropping the sort order from searches for %ss", sort, + reason or "no error message", int(SORT_FALLBACK_TTL), ) _sort_fallback_until = time.monotonic() + SORT_FALLBACK_TTL - - retry = self._execute_query(query, {**variables, "sort": SORT_FALLBACK}) - if _search_payload_rejected(retry): - logger.error("Hardcover rejected this search even without a sort order") - return None return retry def _parse_search_result(self, item: dict) -> BookMetadata | None: diff --git a/tests/metadata/test_hardcover_field_options.py b/tests/metadata/test_hardcover_field_options.py index 1f938219..2cb15f0e 100644 --- a/tests/metadata/test_hardcover_field_options.py +++ b/tests/metadata/test_hardcover_field_options.py @@ -1,4 +1,8 @@ -from shelfmark.metadata_providers.hardcover import HardcoverProvider +from shelfmark.metadata_providers.hardcover import ( + TITLE_SUGGESTION_FIELDS, + TITLE_SUGGESTION_WEIGHTS, + HardcoverProvider, +) class TestHardcoverFieldOptions: @@ -130,8 +134,10 @@ class TestHardcoverFieldOptions: "limit": 7, "page": 1, "sort": "_text_match:desc,users_count:desc", - "fields": "title,alternative_titles", - "weights": "5,2", + # Hardcover rejects a Book search that narrows to fewer fields than its + # preset expects, so the typeahead sends the full list and leans on weights. + "fields": TITLE_SUGGESTION_FIELDS, + "weights": TITLE_SUGGESTION_WEIGHTS, } def test_get_search_field_options_skips_short_text_queries(self): diff --git a/tests/metadata/test_hardcover_search_fields.py b/tests/metadata/test_hardcover_search_fields.py new file mode 100644 index 00000000..ade312e8 --- /dev/null +++ b/tests/metadata/test_hardcover_search_fields.py @@ -0,0 +1,82 @@ +"""Guards on the shape of Hardcover's `fields`/`weights` search parameters. + +Hardcover turns `fields` into Typesense's `query_by` but keeps `num_typos` and +`query_by_weights` as fixed-length presets per query_type. A field list of the +wrong length is not searched loosely -- the whole search is rejected with a null +results body, which used to surface as "0 results". These tests pin the counts +so a narrower field list cannot silently ship again. +""" + +import pytest + +from shelfmark.metadata_providers.hardcover import ( + AUTHOR_SUGGESTION_FIELDS, + AUTHOR_SUGGESTION_WEIGHTS, + BOOK_SEARCH_FIELD_COUNT, + BOOK_SEARCH_FIELDS, + BOOK_TITLE_AUTHOR_WEIGHTS, + BOOK_TITLE_WEIGHTS, + SERIES_SEARCH_FIELDS, + SERIES_SEARCH_WEIGHTS, + TITLE_SUGGESTION_FIELDS, + TITLE_SUGGESTION_WEIGHTS, + HardcoverProvider, +) + + +def _count(value: str) -> int: + return len([part for part in value.split(",") if part.strip()]) + + +class TestBookSearchFieldCounts: + def test_book_field_list_matches_hardcovers_preset_length(self): + assert _count(BOOK_SEARCH_FIELDS) == BOOK_SEARCH_FIELD_COUNT + + @pytest.mark.parametrize( + ("label", "weights"), + [ + ("title", BOOK_TITLE_WEIGHTS), + ("title+author", BOOK_TITLE_AUTHOR_WEIGHTS), + ("title typeahead", TITLE_SUGGESTION_WEIGHTS), + ], + ) + def test_book_weights_line_up_with_the_field_list(self, label, weights): + assert _count(weights) == BOOK_SEARCH_FIELD_COUNT, label + + def test_title_typeahead_uses_the_full_book_field_list(self): + assert TITLE_SUGGESTION_FIELDS == BOOK_SEARCH_FIELDS + + +class TestNonBookSearchFieldCounts: + @pytest.mark.parametrize( + ("fields", "weights"), + [ + (AUTHOR_SUGGESTION_FIELDS, AUTHOR_SUGGESTION_WEIGHTS), + (SERIES_SEARCH_FIELDS, SERIES_SEARCH_WEIGHTS), + ], + ) + def test_weights_line_up_with_their_field_list(self, fields, weights): + assert _count(fields) == _count(weights) + + +class TestBuildSearchParams: + @pytest.mark.parametrize( + ("author", "title", "series"), + [ + ("", "Dune", ""), + ("Herbert", "Dune", ""), + ("Herbert", "", ""), + ("", "", ""), + ], + ) + def test_every_branch_sends_a_usable_field_weight_pair(self, author, title, series): + provider = HardcoverProvider(api_key="test-token") + + _query, fields, weights = provider._build_search_params("dune", author, title, series) + + if fields is None: + # No override: Hardcover applies its own preset, so weights must be absent too. + assert weights is None + return + assert _count(fields) == BOOK_SEARCH_FIELD_COUNT + assert _count(weights) == BOOK_SEARCH_FIELD_COUNT diff --git a/tests/metadata/test_hardcover_sort_fallback.py b/tests/metadata/test_hardcover_sort_fallback.py index 65c99b13..954d4996 100644 --- a/tests/metadata/test_hardcover_sort_fallback.py +++ b/tests/metadata/test_hardcover_sort_fallback.py @@ -1,3 +1,4 @@ +import logging from typing import Any import pytest @@ -6,9 +7,10 @@ from shelfmark.metadata_providers import MetadataSearchOptions from shelfmark.metadata_providers.hardcover import HardcoverProvider # Hardcover answers a rejected search with HTTP 200, no GraphQL errors, and a -# null results body. A search that genuinely matched nothing still returns a -# results object with found: 0. -REJECTED = {"search": {"results": None}} +# null results body; the reason shows up in the sibling error field. A search +# that genuinely matched nothing still returns a results object with found: 0. +REJECTED = {"search": {"error": "Parameter `sort_by` is malformed.", "results": None}} +REJECTED_SILENTLY = {"search": {"results": None}} EMPTY = {"search": {"results": {"hits": [], "found": 0}}} ONE_HIT = {"search": {"results": {"hits": [{"document": {"id": 7, "title": "Dune"}}], "found": 1}}} @@ -19,6 +21,29 @@ def _reset_sort_fallback(monkeypatch): monkeypatch.setattr("shelfmark.metadata_providers.hardcover._sort_fallback_until", 0.0) +@pytest.fixture +def hardcover_logs(): + """Collect Hardcover log messages. + + The provider's logger is built outside the standard hierarchy, so its + records never reach the root handler that caplog installs. + """ + from shelfmark.metadata_providers import hardcover + + messages: list[str] = [] + + class _Capture(logging.Handler): + def emit(self, record: logging.LogRecord) -> None: + messages.append(record.getMessage()) + + handler = _Capture() + hardcover.logger.addHandler(handler) + try: + yield messages + finally: + hardcover.logger.removeHandler(handler) + + def _reject_sorted(calls: list[dict[str, Any]], *, success=ONE_HIT): """Build an _execute_query stand-in that rejects any request carrying a sort.""" @@ -38,7 +63,9 @@ class TestHardcoverSortFallback: result = provider._execute_search_query("query", {"query": "dune", "sort": "relevance"}) assert result == ONE_HIT - assert [call["sort"] for call in calls] == ["relevance", ""] + # The retry drops sort entirely -- an empty sort is a value Hardcover can reject too. + assert [call.get("sort") for call in calls] == ["relevance", None] + assert "sort" not in calls[1] def test_treats_an_empty_result_set_as_success(self, monkeypatch): provider = HardcoverProvider(api_key="test-token") @@ -64,6 +91,24 @@ class TestHardcoverSortFallback: assert result is None assert len(calls) == 2 + def test_keeps_sorting_when_the_sort_was_not_the_culprit(self, monkeypatch): + """A rejection that survives dropping the sort must not disable sorting globally.""" + provider = HardcoverProvider(api_key="test-token") + calls: list[dict[str, Any]] = [] + monkeypatch.setattr( + provider, "_execute_query", lambda query, variables: calls.append(variables) or REJECTED + ) + + provider._execute_search_query("query", {"query": "dune", "sort": "rating:desc"}) + provider._execute_search_query("query", {"query": "hyperion", "sort": "rating:desc"}) + + assert [call.get("sort") for call in calls] == [ + "rating:desc", + None, + "rating:desc", + None, + ] + def test_reports_failure_for_an_unsorted_rejection(self, monkeypatch): provider = HardcoverProvider(api_key="test-token") calls: list[dict[str, Any]] = [] @@ -76,6 +121,24 @@ class TestHardcoverSortFallback: assert result is None assert len(calls) == 1 + def test_logs_the_reason_hardcover_gave(self, monkeypatch, hardcover_logs): + provider = HardcoverProvider(api_key="test-token") + monkeypatch.setattr(provider, "_execute_query", lambda query, variables: REJECTED) + + provider._execute_search_query("query", {"query": "dune", "sort": ""}) + + assert any("Parameter `sort_by` is malformed." in message for message in hardcover_logs) + + def test_falls_back_to_a_placeholder_when_hardcover_says_nothing( + self, monkeypatch, hardcover_logs + ): + provider = HardcoverProvider(api_key="test-token") + monkeypatch.setattr(provider, "_execute_query", lambda query, variables: REJECTED_SILENTLY) + + provider._execute_search_query("query", {"query": "dune", "sort": ""}) + + assert any("no error message" in message for message in hardcover_logs) + def test_skips_the_doomed_request_on_later_searches(self, monkeypatch): provider = HardcoverProvider(api_key="test-token") calls: list[dict[str, Any]] = [] @@ -84,7 +147,7 @@ class TestHardcoverSortFallback: provider._execute_search_query("query", {"query": "dune", "sort": "relevance"}) provider._execute_search_query("query", {"query": "hyperion", "sort": "relevance"}) - assert [call["sort"] for call in calls] == ["relevance", "", ""] + assert [call.get("sort") for call in calls] == ["relevance", None, None] def test_search_returns_results_despite_a_rejected_sort(self, monkeypatch): provider = HardcoverProvider(api_key="test-token") @@ -97,7 +160,7 @@ class TestHardcoverSortFallback: assert result.total_found == 1 assert [book.title for book in result.books] == ["Dune"] - assert [call["sort"] for call in calls] == ["_text_match:desc,users_count:desc", ""] + assert [call.get("sort") for call in calls] == ["_text_match:desc,users_count:desc", None] class TestSearchPayloadRejection: @@ -105,9 +168,19 @@ class TestSearchPayloadRejection: from shelfmark.metadata_providers.hardcover import _search_payload_rejected assert _search_payload_rejected(REJECTED) is True + assert _search_payload_rejected(REJECTED_SILENTLY) is True assert _search_payload_rejected(EMPTY) is False assert _search_payload_rejected(ONE_HIT) is False assert _search_payload_rejected(None) is False assert _search_payload_rejected({}) is False # Non-search payloads (list lookups, book fetches) must pass through. assert _search_payload_rejected({"series": [{"id": 1}]}) is False + + def test_reads_the_error_hardcover_attached(self): + from shelfmark.metadata_providers.hardcover import _search_rejection_reason + + assert _search_rejection_reason(REJECTED) == "Parameter `sort_by` is malformed." + assert _search_rejection_reason(REJECTED_SILENTLY) == "" + assert _search_rejection_reason(EMPTY) == "" + assert _search_rejection_reason(None) == "" + assert _search_rejection_reason({"search": {"error": None, "results": None}}) == ""