mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-10-05 16:21:11 +01:00
fix(googlebooks): page by the capped size, not the raw limit (#1370)
The Google Books search builds `maxResults` as `min(limit, 40)` because the API caps a page at 40 volumes, but advances `startIndex` by the full `limit`. The pages then stop tiling. With `limit=50`, page 1 covers items 0 to 39 and page 2 starts at 50, so items 40 to 49 are never returned and every later page drops another 10. This computes the page size once and uses it for both `maxResults` and `startIndex`. The shipped frontend asks for 40 and is unaffected. `/api/metadata/search` clamps `limit` to 100, so an API caller passing 41 to 100 was hitting it. One thing I left alone. The provider uses the base `search_paginated` heuristic, `has_more = len(books) >= options.limit`, which still reports `has_more: false` for a limit above 40 since Google can never return that many. That was already the behaviour before this change, and fixing it means either touching the shared heuristic or adding a provider override, so I kept this patch to the stride. Happy to follow up if you want it. ## Verification - `tests/metadata/test_googlebooks_parse.py`: pages 1 and 2 at `limit=50` must tile exactly, plus a guard that `limit=25` still strides by 25. The first fails on current main and passes here. - Full suite (3165), ruff, ruff format, basedpyright, vulture green.
This commit is contained in:
@@ -158,10 +158,11 @@ class GoogleBooksProvider(MetadataProvider):
|
||||
query = "+".join(query_parts)
|
||||
|
||||
# Build request params
|
||||
page_size = min(options.limit, 40) # Google max is 40
|
||||
params: dict[str, Any] = {
|
||||
"q": query,
|
||||
"maxResults": min(options.limit, 40), # Google max is 40
|
||||
"startIndex": (options.page - 1) * options.limit,
|
||||
"maxResults": page_size,
|
||||
"startIndex": (options.page - 1) * page_size,
|
||||
"printType": "books", # Exclude magazines
|
||||
}
|
||||
|
||||
|
||||
@@ -39,6 +39,15 @@ class _FlakyGoogleBooksSession:
|
||||
)
|
||||
|
||||
|
||||
class _RecordingGoogleBooksSession:
|
||||
def __init__(self):
|
||||
self.params = []
|
||||
|
||||
def get(self, *args, **kwargs):
|
||||
self.params.append(dict(kwargs["params"]))
|
||||
return _GoogleBooksResponse({"items": []})
|
||||
|
||||
|
||||
def test_googlebooks_search_does_not_cache_request_failures():
|
||||
get_metadata_cache().clear()
|
||||
provider = GoogleBooksProvider(api_key="test-key")
|
||||
@@ -54,6 +63,33 @@ def test_googlebooks_search_does_not_cache_request_failures():
|
||||
assert [book.title for book in result] == ["Recovered Book"]
|
||||
|
||||
|
||||
def test_googlebooks_pages_tile_when_limit_exceeds_api_maximum():
|
||||
get_metadata_cache().clear()
|
||||
provider = GoogleBooksProvider(api_key="test-key")
|
||||
session = _RecordingGoogleBooksSession()
|
||||
provider.session = session
|
||||
|
||||
for page in (1, 2):
|
||||
provider.search(MetadataSearchOptions(query="Dune", limit=50, page=page))
|
||||
|
||||
first, second = session.params
|
||||
assert first["maxResults"] == 40
|
||||
assert first["startIndex"] == 0
|
||||
assert second["startIndex"] == first["startIndex"] + first["maxResults"]
|
||||
|
||||
|
||||
def test_googlebooks_page_stride_follows_limit_below_api_maximum():
|
||||
get_metadata_cache().clear()
|
||||
provider = GoogleBooksProvider(api_key="test-key")
|
||||
session = _RecordingGoogleBooksSession()
|
||||
provider.session = session
|
||||
|
||||
provider.search(MetadataSearchOptions(query="Dune", limit=25, page=3))
|
||||
|
||||
assert session.params[0]["maxResults"] == 25
|
||||
assert session.params[0]["startIndex"] == 50
|
||||
|
||||
|
||||
class TestGoogleBooksParseVolume:
|
||||
def test_parse_volume_returns_metadata_for_valid_payload(self):
|
||||
provider = GoogleBooksProvider(api_key="test-key")
|
||||
|
||||
Reference in New Issue
Block a user