From ed357ea223c53cd1be90b32f3d0c1cecb99ea8f0 Mon Sep 17 00:00:00 2001 From: Cal Corum Date: Tue, 3 Mar 2026 21:50:20 -0600 Subject: [PATCH] =?UTF-8?q?store:=20PR=20review:=20paper-dynasty-database#?= =?UTF-8?q?53=20=E2=80=94=20batch=20Paperdex=20N+1=20fix=20(APPROVED)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...base53-batch-paperdex-n1-fix-app-c98ad1.md | 38 +++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 graph/workflows/pr-review-paper-dynasty-database53-batch-paperdex-n1-fix-app-c98ad1.md diff --git a/graph/workflows/pr-review-paper-dynasty-database53-batch-paperdex-n1-fix-app-c98ad1.md b/graph/workflows/pr-review-paper-dynasty-database53-batch-paperdex-n1-fix-app-c98ad1.md new file mode 100644 index 00000000000..edf9a76a448 --- /dev/null +++ b/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