Skip to content

feat: route imdb_search advanced title searches through Kometa IMDb Service - #3379

Open
chazlarson wants to merge 6 commits into
nightlyfrom
imdb-service-search
Open

feat: route imdb_search advanced title searches through Kometa IMDb Service#3379
chazlarson wants to merge 6 commits into
nightlyfrom
imdb-service-search

Conversation

@chazlarson

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • Bug Fix (non-breaking change which fixes an issue)
  • Feature/Tweak (non-breaking change which adds new functionality or enhances existing functionality)
  • Breaking Change (fix or feature that would break any existing functionality for users)
  • Documentation Update
  • Chore (maintenance, dependency bumps, housekeeping - no functional change)
  • Other

Description

Routes imdb_search builder advanced title searches through the Kometa IMDb Service (POST /search/advanced) instead of hitting IMDb GraphQL directly.

What changed:

  • Added _service_post() - POST counterpart to _service_request, with the same error handling (exception wrapping, 404/400+ checks, JSON parse guard)
  • Rewrote the search branch of _pagination(): calls _graphql_json to build constraint variables (preserving all existing translation tables and constraint logic), splits into constraints/sort/limit, and sends a single POST to /search/advanced
  • The service handles cursor pagination and caching server-side (1-day TTL by constraint hash); Kometa receives the full ordered result list in one response, eliminating client-side cursor pagination overhead
  • List and watchlist paths remain unchanged on direct GraphQL

Tested: 46 existing IMDb tests pass. The change is purely a routing swap - all constraint-building logic and translation tables are preserved as-is.

Have you updated the CHANGELOG.md?

  • Yes
  • No

@chazlarson chazlarson left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

All CI checks pass and the PR is mergeable. Core change is clean. Notes below.

✅ Addressed

  • Wasted search_hash fetch (fixed in 1e5b9da): the search path previously called _graphql_json, which triggered _json_operation/search_hash — a dead GitHub-raw HTTP GET on every imdb_search. Now split into _graphql_variables (builds constraints) and a thin _graphql_json wrapper; the search branch calls _graphql_variables directly and no longer fetches the hash. List/watchlist paths unchanged.

🟡 Still open: no test coverage for the new search path

_service_post() and the search-via-service branch of _pagination are untested. Worth a few small tests covering:

  • the constraints/sort/limit split (excluding locale/first/sortBy/sortOrder),
  • limit omitted when data["limit"] == 0,
  • results parsing + the empty-results Failed path,
  • _service_post error handling (≥400, invalid JSON) mirroring the existing _service_request tests.

🟡 PR scope

Branch is stacked on imdb-service-ratings, so the diff into nightly carries all prior service work (ratings/genres/episode-ratings/parental/charts/keywords, gitpython bump, spellcheck tooling). Only ~60 lines are the actual search change. Should land after — or together with — the ratings branch.

✅ Looks good

  • _service_post() correctly mirrors _service_request() error handling (exception wrap, 404/not_found_ok, ≥400, JSON-parse guard).
  • constraints/sort/limit split matches the service contract.
  • Omitting limit when 0 correctly maps Kometa's "no limit" convention onto the service default.
  • List/watchlist correctly left on direct GraphQL; item_count = 100 simplification is sound.
  • No-fallback-for-search is reasonable (no TSV equivalent exists).

Minor nits

  • Search block uses data.get("limit", 0) while the list path uses data["limit"] directly — validate_imdb always sets limit, so the defensive .get is inconsistent (harmless).
  • self._service_post(...) or {} turns a None (404) into "No IMDb IDs Found" rather than a distinct not-found message — fine, but less precise than passing not_found_ok=True.

@chazlarson chazlarson left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review (updated)

Both follow-up items are now resolved. CI green, mergeable.

✅ Addressed

  • Wasted search_hash fetch (1e5b9da): split _graphql_json into _graphql_variables (builds constraints) and a thin _graphql_json wrapper. The search branch calls _graphql_variables directly, so it no longer triggers _json_operation/search_hash — the dead GitHub-raw HTTP GET per imdb_search is gone. List/watchlist paths unchanged.
  • Test coverage for the search path (043fdb6): added 11 tests (57 total, up from 46):
    • constraints/sort/limit split (asserts locale/first/sortBy/sortOrder excluded from constraints; sort split correctly),
    • limit omitted when data['limit'] == 0,
    • results parsing, empty-results and None-response Failed paths,
    • service-failure propagation (no fallback dataset for search),
    • _service_post success / 404 not_found_ok / ≥400 / invalid JSON / connection-error handling, mirroring the _service_request tests.
    • Autouse fixture patches the module logger to a no-op (it's None in tests) so the search path's logger.ghost/exorcise calls don't crash. tests/ is excluded from pyright, so the MagicMock-assignment warnings there are non-issues.

🟡 Scope note (unchanged)

Branch is stacked on imdb-service-ratings, so the diff into nightly carries all prior service work (ratings/genres/episode-ratings/parental/charts/keywords, gitpython bump, spellcheck tooling). Only the search change + its tests are new here. Should land after — or together with — the ratings branch.

✅ Looks good

  • _service_post() mirrors _service_request() error handling and is now test-covered.
  • constraints/sort/limit split matches the service contract.
  • Omitting limit when 0 correctly maps Kometa's "no limit" convention onto the service default.
  • List/watchlist correctly left on direct GraphQL.
  • No-fallback-for-search is reasonable (no TSV equivalent exists) and is now asserted by a test.

Minor nits (non-blocking)

  • Search block uses data.get("limit", 0) while the list path uses data["limit"] directly — harmless inconsistency since validate_imdb always sets limit.
  • self._service_post(...) or {} turns a None (404) into "No IMDb IDs Found" rather than a distinct not-found message — fine, but less precise than passing not_found_ok=True.

@badja-dev badja-dev left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the new search branch in _pagination calls logger.ghost(...) / logger.exorcise() unguarded, while every other new method in this PR (_service_request, _service_post, get_rating, etc.) wraps logger calls in if logger:


Once rebased onto nightly and the above is fix, looks good to me

@chazlarson

Copy link
Copy Markdown
Contributor Author

@badja-dev can you approve if happy?

…ervice

Adds _service_post() helper for POST-based service calls and rewrites the search branch of _pagination() to POST constraints/sort/limit to /search/advanced instead of hitting IMDb GraphQL directly. The service handles pagination and caching server-side; Kometa receives the full ordered result list in one response.

List and watchlist paths remain unchanged on direct GraphQL.
Split _graphql_json into _graphql_variables (builds the constraint dict)
and a thin _graphql_json wrapper that adds the persisted-query envelope.
The search branch of _pagination now calls _graphql_variables directly,
so it no longer triggers _json_operation/search_hash — eliminating a
dead GitHub raw HTTP fetch on every imdb_search. List/watchlist paths
are unchanged.
Adds 11 tests for the new advanced-search-via-service path:
- constraints/sort/limit split (excludes locale/first/sortBy/sortOrder)
- limit omitted when data['limit'] == 0
- results parsing, empty-results and None-response Failed paths
- service failure propagation (no fallback dataset for search)
- _service_post success, 404 not_found_ok, >=400, invalid JSON,
  and connection-error handling (mirrors _service_request tests)

Adds an autouse fixture patching the module logger to a no-op so the
search path's logger.ghost/exorcise calls don't crash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants