store: PR review: paper-dynasty-database#53 — batch Paperdex N+1 fix (APPROVED)

graph/workflows/pr-review-paper-dynasty-database53-batch-paperdex-n1-fix-app-c98ad1.md

@@ -0,0 +1,38 @@
+---
+id: c98ad146-a578-430b-af4e-2ecda68361f1
+type: workflow
+title: "PR review: paper-dynasty-database#53 — batch Paperdex N+1 fix (APPROVED)"
+tags: [pr-reviewer, paper-dynasty-database, peewee, n+1, performance, python, fastapi]
+importance: 0.6
+confidence: 0.8
+created: "2026-03-04T03:50:20.621207+00:00"
+updated: "2026-03-04T03:50:20.621207+00:00"
+---
+
+## Review Summary
+
+**PR #53** — fix: batch Paperdex lookups to avoid N+1 queries (#17)
+**Verdict:** APPROVED (posted as COMMENT — Gitea blocks self-approval)
+
+### Files Reviewed
+- `app/routers_v2/players.py`
+- `app/routers_v2/cards.py`
+
+### Key Findings
+
+**Correctness — all good:**
+- Batch logic correct in all 3 locations: `Paperdex.player_id << player_ids` with `dex_by_player` dict grouping
+- Bonus bug fix: original `cards.py` had `Paperdex.player == x` where `x` is a `Card` — Peewee resolves this to `Card.id` (wrong), not `Card.player_id`. Batched version uses `x.player_id` (correct)
+- Count optimization: `len(card_list)` replaces `all_cards.count()` saving a `SELECT COUNT(*)` per request
+- `inc_dex` guard preserved correctly in `get_players`
+
+**Security:** No issues. `player_ids` from ORM objects, not raw user input.
+
+**Suggestions (non-blocking):**
+- PR description says "Players-by-team endpoint" but diff actually patches `get_random_player`
+- SQLite IN-clause limit (999 params) could theoretically be hit on unbounded `get_cards` calls
+- `list(all_cards)` materializes all cards — fine trade-off, but `limit` param should be used
+
+### Pattern Notes
+- Peewee `ForeignKeyField` comparison: `Model.fk == other_model_instance` resolves to `other_model_instance.pk` — can be a subtle bug when types don't match
+- Batch pattern: `dict.setdefault(key, []).append(val)` for grouping FK rows is clean and idiomatic