perf: batch image_url prefetch in list_card_states to eliminate N+1 (#199) #206

Claude · 2026-04-08T02:04:52Z

Claude commented

2026-04-08 02:04:52 +00:00

Closes #199

Problem

_build_card_state_response issued a separate CardModel.get() for every card state with variant > 0. On the /cards list endpoint with limit=100, this meant up to 100 extra DB round-trips — one per card — scaling linearly with page size.

Fix

Pre-fetch all image_url values before the response-building loop using at most 2 bulk queries (one against BattingCard, one against PitchingCard):

Scan the page of states and partition variant player IDs by card type (batter vs sp/rp).
Issue SELECT … WHERE player_id IN (…) against each card table (skipped entirely if the set is empty).
Build a (player_id, variant) → image_url dict.
Pass each resolved value directly into _build_card_state_response via a new image_url parameter.

The helper now skips its own DB lookup whenever the caller passes image_url (using a sentinel _UNSET to distinguish "not provided" from None). The single-card get_card_state path is unchanged and still resolves image_url inline — acceptable for a single-item response.

Files changed

app/routers_v2/refractor.py

Query reduction

Before	After
1 + N queries (N = variant cards in page)	1 + 0–2 queries (regardless of page size)

Closes #199 ## Problem `_build_card_state_response` issued a separate `CardModel.get()` for every card state with `variant > 0`. On the `/cards` list endpoint with `limit=100`, this meant up to 100 extra DB round-trips — one per card — scaling linearly with page size. ## Fix Pre-fetch all `image_url` values before the response-building loop using at most **2 bulk queries** (one against `BattingCard`, one against `PitchingCard`): 1. Scan the page of states and partition variant player IDs by card type (`batter` vs `sp`/`rp`). 2. Issue `SELECT … WHERE player_id IN (…)` against each card table (skipped entirely if the set is empty). 3. Build a `(player_id, variant) → image_url` dict. 4. Pass each resolved value directly into `_build_card_state_response` via a new `image_url` parameter. The helper now skips its own DB lookup whenever the caller passes `image_url` (using a sentinel `_UNSET` to distinguish "not provided" from `None`). The single-card `get_card_state` path is unchanged and still resolves `image_url` inline — acceptable for a single-item response. ## Files changed - `app/routers_v2/refractor.py` ## Query reduction | Before | After | |--------|-------| | 1 + N queries (N = variant cards in page) | 1 + 0–2 queries (regardless of page size) |

Claude added 1 commit 2026-04-08 02:04:54 +00:00

perf: batch image_url prefetch in list_card_states to eliminate N+1 (#199 ) b7196c1c56

Replace per-row CardModel.get() in _build_card_state_response with a
bulk prefetch in list_card_states: collect variant player IDs, issue at
most 2 queries (BattingCard + PitchingCard), build a (player_id, variant)
-> image_url map, and pass the resolved value directly to the helper.

The single-card get_card_state path is unchanged and still resolves
image_url inline (one extra query is acceptable for a single-item response).

Closes #199

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-04-08 02:05:03 +00:00

perf: N+1 query in _build_card_state_response for image_url #199

Claude added the

ai-reviewing

label 2026-04-08 02:15:53 +00:00

Claude reviewed 2026-04-08 02:17:50 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/refractor.py (modified)

Findings

Correctness

No issues found. The implementation is correct across all code paths:

_UNSET sentinel correctly distinguishes image_url=None (explicit pre-fetch result) from image_url=_UNSET (no pre-fetch, do inline lookup). The sentinel is defined at module level and used via is identity check — clean pattern.
Pre-fetch loop correctly filters state.variant and state.variant > 0 before partitioning into batter_pids/pitcher_pids. States with variant=None or variant=0 fall through to image_url_map.get(...) which returns None, matching original behavior.
Map key (player_id, variant) correctly disambiguates multiple variants per player. The bulk query fetches all cards for the partitioned player IDs (including variant=0), but the keyed lookup is precise.
Non-variant states receive image_url=None explicitly, which short-circuits the inline DB lookup (None is not _UNSET). Result is image_url: null — same as original.
state.track guard in the pre-fetch loop (if state.track else None) is sufficient — list_card_states JOINs RefractorTrack unconditionally, so state.track is always populated.
Empty set gates (if batter_pids: / if pitcher_pids:) correctly prevent empty IN () clauses that would produce a DB error.
Single-card path (get_card_state) is unchanged — still calls _build_card_state_response(state) with no image_url argument, triggering the inline lookup. Correct.

Security

No issues found. No new inputs, no injection vectors introduced.

Style & Conventions

No issues found. No lazy imports added. Type annotations on local variables (set[int], dict[tuple[int, int], str | None]) are not evaluated at runtime and are consistent with the style elsewhere.

Suggestions

The bulk query fetches all BattingCard/PitchingCard rows for the partitioned player IDs, including non-variant rows (variant=0). Adding & (CardModel.variant > 0) to each bulk query would reduce the result set slightly and make intent clearer. Non-blocking — map key includes variant so no correctness impact.
No tests added, consistent with the codebase pattern of follow-up test PRs.

Verdict: APPROVED

Clean, well-scoped N+1 fix. The _UNSET sentinel approach correctly preserves backward compatibility for the single-card endpoint while eliminating per-row queries on the list path. No correctness, security, or convention issues found. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/refractor.py` (modified) ### Findings #### Correctness No issues found. The implementation is correct across all code paths: - **`_UNSET` sentinel** correctly distinguishes `image_url=None` (explicit pre-fetch result) from `image_url=_UNSET` (no pre-fetch, do inline lookup). The sentinel is defined at module level and used via `is` identity check — clean pattern. - **Pre-fetch loop** correctly filters `state.variant and state.variant > 0` before partitioning into `batter_pids`/`pitcher_pids`. States with `variant=None` or `variant=0` fall through to `image_url_map.get(...)` which returns `None`, matching original behavior. - **Map key `(player_id, variant)`** correctly disambiguates multiple variants per player. The bulk query fetches all cards for the partitioned player IDs (including variant=0), but the keyed lookup is precise. - **Non-variant states** receive `image_url=None` explicitly, which short-circuits the inline DB lookup (`None is not _UNSET`). Result is `image_url: null` — same as original. - **`state.track` guard** in the pre-fetch loop (`if state.track else None`) is sufficient — `list_card_states` JOINs `RefractorTrack` unconditionally, so `state.track` is always populated. - **Empty set gates** (`if batter_pids:` / `if pitcher_pids:`) correctly prevent empty `IN ()` clauses that would produce a DB error. - **Single-card path** (`get_card_state`) is unchanged — still calls `_build_card_state_response(state)` with no `image_url` argument, triggering the inline lookup. Correct. #### Security No issues found. No new inputs, no injection vectors introduced. #### Style & Conventions No issues found. No lazy imports added. Type annotations on local variables (`set[int]`, `dict[tuple[int, int], str | None]`) are not evaluated at runtime and are consistent with the style elsewhere. #### Suggestions - The bulk query fetches all `BattingCard`/`PitchingCard` rows for the partitioned player IDs, including non-variant rows (variant=0). Adding `& (CardModel.variant > 0)` to each bulk query would reduce the result set slightly and make intent clearer. Non-blocking — map key includes variant so no correctness impact. - No tests added, consistent with the codebase pattern of follow-up test PRs. ### Verdict: APPROVED Clean, well-scoped N+1 fix. The `_UNSET` sentinel approach correctly preserves backward compatibility for the single-card endpoint while eliminating per-row queries on the list path. No correctness, security, or convention issues found. Ready to merge. --- *Automated review by Claude PR Reviewer*

Claude added the

ai-reviewed

label 2026-04-08 02:18:11 +00:00

cal approved these changes 2026-04-08 02:25:44 +00:00

cal left a comment

Approved via pd-pr

cal merged commit 3cc4c65717 into main

2026-04-08 02:25:53 +00:00

cal deleted branch issue/199-perf-n-1-query-in-build-card-state-response-for-im

2026-04-08 02:25:53 +00:00

cal referenced this issue from a commit

2026-04-08 02:25:55 +00:00

Merge pull request 'perf: batch image_url prefetch in list_card_states to eliminate N+1 (#199)' (#206) from issue/199-perf-n-1-query-in-build-card-state-response-for-im into main

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#206