fix: N+1 OMDb calls in title search with enrich opt-out - #135
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses OMDb quota/performance issues in IMDBScraper title searches by adding an opt-out for per-hit detail enrichment, eliminating the previous N+1 request pattern when callers don’t need full movie details.
Changes:
- Added
enrich: bool = TruetoIMDBScraper.scrape/scrape_asyncand threaded it through the title-search implementations. - Skipped per-hit OMDb detail lookups when
enrich=False, returning normalized lightweight search rows instead. - Added/expanded tests covering sync + async behavior, including the one-call-per-page expectation for
enrich=False.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/pyscrappy/scrapers/imdb.py |
Adds enrich parameter, gates per-hit detail lookups, and documents the quota/call impact. |
tests/test_scrapers/test_imdb.py |
Adds sync + async tests to ensure enrich=False avoids extra HTTP calls. |
Suppressed comments (1)
tests/test_scrapers/test_imdb.py:177
- The async
enrich=Falsetest checks the call count, but it doesn’t validate that the returned item is the lightweight search result (and not a partially-enriched shape). Asserting a couple of expected/absent fields would make the test cover the behavioral contract as well as the N+1 avoidance.
assert scraper._async_http.get_html.call_count == 1
assert len(result.data) == 1
assert result.errors == []
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- ruff: sort imports in test_imdb.py, format imdb.py + test_imdb.py (the failing lint check). - Align the scrape_async docstring with sync: document the max_pages + max_pages*10 call multiplier (Copilot). - Strengthen both enrich=False tests to assert the returned row is the lightweight search row: search fields present (title/imdb_id/type), detail-only fields absent (genre/director) — _normalise drops missing keys, so 'not in' is the correct assertion (Copilot).
|
Thanks @ParthP22, solid fix, enrich=False is threaded through both sync and async and hits every acceptance criterion on #132. I pushed a commit on top: fixed the failing lint (import sort + ruff format), aligned the async docstring with the sync one to document the max_pages + max_pages*10 multiplier, and strengthened both enrich=False tests to assert the row is the lightweight one. Worth noting for the shape check: _normalise drops missing keys entirely rather than setting them to None, so the right assertion is "genre" not in movie (not genre is None). Green now, 479 passed. Nice first contribution. |
|
Thank you for the fixes and the explanation. I understand now where I missed some things, so I will keep them in mind. |
Summary
Closes #132.
Adds an
enrich: bool = Trueparameter toIMDBScraper.scrape/scrape_async, letting callers opt out of the per-hit OMDb detail lookup that title searches previously always performed. Default behavior is unchanged.Problem
Every title search hit was enriched with a second OMDb call (
_get({"i": imdb_id})), with no way to skip it. Call volume was roughlymax_pages + max_pages * 10(OMDb returns ~10 hits/page), which can burn through the free-tier daily quota (1,000 calls/day) quickly and without warning. The docstring mentioned "10 results per page" but never the hidden per-hit cost.Changes
scrape/scrape_async: addedenrich: bool = Trueparam, threaded through to_search_by_title/_search_by_title_async._search_by_title/_search_by_title_async: the per-hit detail call is now skipped whenenrich=False, returning the lightweight search row instead (title, year, imdbID, type, poster).scrapeandscrape_asyncnow document the call multiplier (max_pages + max_pages * 10) and theenrich=Falseopt-out.Tests
Added to
tests/test_scrapers/test_imdb.py:TestIMDBSearchByTitle::test_search_without_enrich_skips_details(sync) — assertsenrich=Falseresults in exactly one HTTP call (get_html.call_count == 1), and that the search still returns data with no errors.TestIMDBSearchByTitleAsync(new class, mirrors the sync class) — covers the async path end to end:test_search_async_enriches_with_details— default (enrich=True) behavior unchanged.test_search_async_no_results— not-found case still works async.test_search_async_without_enrich_skips_details— same one-call assertion as the sync test, via a new_scraper_with_asyncfixture.The
enrich=Falsetests queue only one mock response on purpose — if the per-hit lookup ever fired when it shouldn't, the mock would run out of queued responses and error, in addition to the explicitcall_count == 1assertion failing._scraper_with_asyncmocks_async_httpwithAsyncMockon.get_html(mirroring the existing_scraper_withpattern for_http), since CONTRIBUTING.md's testing principles reference_httpspecifically but the async path needed its own equivalent.pytest tests/ -v— all passingruff check src/— passingAcceptance criteria (from #132)
scrape(query=..., details=False)issues one request per page (no per-hit lookups)enrich=Falseper the issue's suggested naming.