cal/major-domo-database
1
0
Fork 0
Code Issues 25 Pull Requests 25 Actions Packages Projects Releases Wiki Activity

fix: eliminate N+1 queries in batch POST endpoints (#25) #52

Merged
cal merged 2 commits from ai/major-domo-database-25 into next-release 2026-03-10 14:37:57 +00:00
Conversation 1 Commits 2 Files Changed 4 +431 -237
cal commented 2026-03-05 23:33:57 +00:00
Owner
Copy Link

Summary

  • Collected all team/player IDs from the request batch upfront, then fetched them in a single WHERE id IN (...) query before the validation loop in post_transactions, post_results, post_schedules, and post_batstats
  • A 50-move post_transactions batch now issues 2 queries instead of 150; other endpoints scale similarly
  • Error messages remain identical — each invalid ID is still reported with its specific value

Files changed

  • app/routers_v3/transactions.py — 3N → 2 queries (team + player bulk fetch)
  • app/routers_v3/results.py — 2N → 1 query (combined away/home team bulk fetch)
  • app/routers_v3/schedules.py — 2N → 1 query (combined away/home team bulk fetch)
  • app/routers_v3/battingstats.py — 2N → 2 queries (team + player bulk fetch)

Test results

No test suite — changes verified by code review. Logic is equivalent: bulk-fetched ID sets replace per-row get_or_none, validation loop unchanged.

Other observations

  • The auto-formatter (ruff/black) reformatted all touched files on save — cosmetic diff is expected per project setup, not introduced intentionally.

🤖 Generated with Claude Code

## Summary - Collected all team/player IDs from the request batch upfront, then fetched them in a single `WHERE id IN (...)` query before the validation loop in `post_transactions`, `post_results`, `post_schedules`, and `post_batstats` - A 50-move `post_transactions` batch now issues 2 queries instead of 150; other endpoints scale similarly - Error messages remain identical — each invalid ID is still reported with its specific value ## Files changed - `app/routers_v3/transactions.py` — 3N → 2 queries (team + player bulk fetch) - `app/routers_v3/results.py` — 2N → 1 query (combined away/home team bulk fetch) - `app/routers_v3/schedules.py` — 2N → 1 query (combined away/home team bulk fetch) - `app/routers_v3/battingstats.py` — 2N → 2 queries (team + player bulk fetch) ## Test results No test suite — changes verified by code review. Logic is equivalent: bulk-fetched ID sets replace per-row `get_or_none`, validation loop unchanged. ## Other observations - The auto-formatter (ruff/black) reformatted all touched files on save — cosmetic diff is expected per project setup, not introduced intentionally. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 1 commit 2026-03-05 23:33:58 +00:00
fix: eliminate N+1 queries in batch POST endpoints (#25)
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m8s
Details
580b22ec6f 
Replace per-row Team/Player lookups with bulk IN-list queries before
the validation loop in post_transactions, post_results, post_schedules,
and post_batstats. A 50-move batch now uses 2 queries instead of 150.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal added the
ai-reviewing
 label 2026-03-05 23:45:34 +00:00
cal reviewed 2026-03-05 23:48:11 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v3/transactions.py (modified)
  • app/routers_v3/results.py (modified)
  • app/routers_v3/schedules.py (modified)
  • app/routers_v3/battingstats.py (modified)

Findings

Correctness

⚠️ Bug: Empty batch regression — Peewee + PostgreSQL rejects WHERE id IN ()

All four endpoints collect unique IDs upfront before the validation loop. If the request body contains an empty list (e.g. {"moves": []}), all_team_ids will be [], and the bulk query:

found_team_ids = set(t.id for t in Team.select(Team.id).where(Team.id << all_team_ids))

generates WHERE "team"."id" IN () — a PostgreSQL syntax error. This crashes with a 500 before reaching the loop.

Before this PR: empty batches silently returned 200 ("0 transactions have been added").
After this PR: empty batches crash with a DB error.

This affects all four endpoints. The fix is straightforward — guard each bulk query with a conditional:

# Option A: early return (simplest)
if not moves.moves:
    db.close()
    return "0 transactions have been added"

# Option B: conditional guard (inline, consistent pattern)
found_team_ids = (
    set(t.id for t in Team.select(Team.id).where(Team.id << all_team_ids))
    if all_team_ids else set()
)
found_player_ids = (
    set(p.id for p in Player.select(Player.id).where(Player.id << all_player_ids))
    if all_player_ids else set()
)

Affected locations (must fix before merge):

  • transactions.py — found_team_ids + found_player_ids bulk queries
  • results.py — found_team_ids bulk query
  • schedules.py — found_team_ids bulk query
  • battingstats.py — found_team_ids + found_player_ids bulk queries

Security

  • Bulk queries use Peewee's << (IN) operator — fully parameterized, no injection risk.
  • Auth checks are preserved and unchanged in all endpoints.
  • Pre-existing {token} in logger.warning(f"... Bad Token: {token}") calls remains in several functions (not introduced by this PR; tracked separately).

Style & Conventions

  • The N+1 fix pattern is consistent and clean across all four endpoints.
  • Cosmetic reformatting (single → double quotes, argument line-wrapping) is expected ruff/black output.

Suggestions

  • None beyond the required empty-list fix.

Verdict: REQUEST_CHANGES

The N+1 optimization is correct and well-structured for non-empty batches. One blocking issue: empty batch requests now crash with a PostgreSQL syntax error instead of returning a clean 200. Add if all_team_ids else set() / if all_player_ids else set() guards (or an early-return guard) in all four endpoints before merging.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v3/transactions.py` (modified) - `app/routers_v3/results.py` (modified) - `app/routers_v3/schedules.py` (modified) - `app/routers_v3/battingstats.py` (modified) ### Findings #### Correctness **⚠️ Bug: Empty batch regression — Peewee + PostgreSQL rejects `WHERE id IN ()`** All four endpoints collect unique IDs upfront before the validation loop. If the request body contains an empty list (e.g. `{"moves": []}`), `all_team_ids` will be `[]`, and the bulk query: ```python found_team_ids = set(t.id for t in Team.select(Team.id).where(Team.id << all_team_ids)) ``` generates `WHERE "team"."id" IN ()` — a PostgreSQL syntax error. This crashes with a 500 before reaching the loop. **Before this PR:** empty batches silently returned 200 (`"0 transactions have been added"`). **After this PR:** empty batches crash with a DB error. This affects all four endpoints. The fix is straightforward — guard each bulk query with a conditional: ```python # Option A: early return (simplest) if not moves.moves: db.close() return "0 transactions have been added" # Option B: conditional guard (inline, consistent pattern) found_team_ids = ( set(t.id for t in Team.select(Team.id).where(Team.id << all_team_ids)) if all_team_ids else set() ) found_player_ids = ( set(p.id for p in Player.select(Player.id).where(Player.id << all_player_ids)) if all_player_ids else set() ) ``` **Affected locations (must fix before merge):** - `transactions.py` — `found_team_ids` + `found_player_ids` bulk queries - `results.py` — `found_team_ids` bulk query - `schedules.py` — `found_team_ids` bulk query - `battingstats.py` — `found_team_ids` + `found_player_ids` bulk queries #### Security - Bulk queries use Peewee's `<<` (IN) operator — fully parameterized, no injection risk. - Auth checks are preserved and unchanged in all endpoints. - Pre-existing `{token}` in `logger.warning(f"... Bad Token: {token}")` calls remains in several functions (not introduced by this PR; tracked separately). #### Style & Conventions - The N+1 fix pattern is consistent and clean across all four endpoints. - Cosmetic reformatting (single → double quotes, argument line-wrapping) is expected ruff/black output. #### Suggestions - None beyond the required empty-list fix. ### Verdict: REQUEST_CHANGES The N+1 optimization is correct and well-structured for non-empty batches. One blocking issue: empty batch requests now crash with a PostgreSQL syntax error instead of returning a clean 200. Add `if all_team_ids else set()` / `if all_player_ids else set()` guards (or an early-return guard) in all four endpoints before merging. --- *Automated review by Claude PR Reviewer*
cal added
ai-changes-requested
 and removed
ai-reviewing
 labels 2026-03-05 23:48:45 +00:00
cal changed target branch from main to next-release 2026-03-07 07:32:44 +00:00
cal added the
ai-working
 label 2026-03-07 07:51:14 +00:00
cal added 1 commit 2026-03-07 07:52:04 +00:00 
Guard bulk ID queries against empty lists to prevent PostgreSQL
syntax error (WHERE id IN ()) when batch POST endpoints receive
empty request bodies.

Affected endpoints:
- POST /api/v3/transactions
- POST /api/v3/results
- POST /api/v3/schedules
- POST /api/v3/battingstats

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
cal commented 2026-03-07 07:52:12 +00:00
Author
Owner
Copy Link

Review Feedback Addressed

Addressed the blocking bug identified in the review: empty batch regression causing PostgreSQL syntax error.

Changes Made

Added if all_team_ids else set() / if all_player_ids else set() conditional guards to all four affected bulk query sites:

  • transactions.py (post_transactions): guarded found_team_ids + found_player_ids
  • results.py (post_results): guarded found_team_ids
  • schedules.py (post_schedules): guarded found_team_ids
  • battingstats.py (post_batstats): guarded found_team_ids + found_player_ids

Behaviour

  • Empty batch (e.g. {"moves": []}) → skips bulk queries entirely, found_*_ids is set(), validation loop doesn't execute, returns "0 transactions have been added" — same as pre-PR behaviour.
  • Non-empty batch → unchanged N+1 fix behaviour from the original PR.

Automated fix by Claude Sonnet 4.6

## Review Feedback Addressed Addressed the blocking bug identified in the review: **empty batch regression causing PostgreSQL syntax error**. ### Changes Made Added `if all_team_ids else set()` / `if all_player_ids else set()` conditional guards to all four affected bulk query sites: - **`transactions.py`** (`post_transactions`): guarded `found_team_ids` + `found_player_ids` - **`results.py`** (`post_results`): guarded `found_team_ids` - **`schedules.py`** (`post_schedules`): guarded `found_team_ids` - **`battingstats.py`** (`post_batstats`): guarded `found_team_ids` + `found_player_ids` ### Behaviour - Empty batch (e.g. `{"moves": []}`) → skips bulk queries entirely, `found_*_ids` is `set()`, validation loop doesn't execute, returns `"0 transactions have been added"` — same as pre-PR behaviour. - Non-empty batch → unchanged N+1 fix behaviour from the original PR. --- *Automated fix by Claude Sonnet 4.6*
cal removed the
ai-working
ai-changes-requested
 labels 2026-03-07 07:52:35 +00:00
cal added the
ai-reviewing
 label 2026-03-07 08:15:33 +00:00
cal reviewed 2026-03-07 08:17:13 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v3/transactions.py (modified)
  • app/routers_v3/results.py (modified)
  • app/routers_v3/schedules.py (modified)
  • app/routers_v3/battingstats.py (modified)

Context

This is a follow-up review. A previous pass issued REQUEST_CHANGES for an empty-batch regression: Team.select(Team.id).where(Team.id << []) produces WHERE id IN (), which is a PostgreSQL syntax error. The fix commit (17f67ff) added conditional guards before all bulk queries.

Findings

Correctness

  • Empty-batch guards confirmed — all 6 bulk query sites now have if all_team_ids else set() / if all_player_ids else set() guards, preventing WHERE id IN () on empty batches. Verified across all 4 endpoints:
    • transactions.py: team guard + player guard ✅
    • results.py: team guard ✅
    • schedules.py: team guard ✅
    • battingstats.py: team guard + player guard ✅
  • Query reduction correct — pre-fetch logic replaces per-row get_or_none with set membership check; error messages still include the specific invalid ID.
  • Empty list input — if the request body contains zero items, all_team_ids / all_player_ids are empty lists, guards short-circuit to set(), the validation loop is skipped, and chunked([], 100) produces no iterations. Clean no-op. ✅
  • Deduplication — set() deduplication before bulk fetch is correct; duplicate IDs in the batch still get per-row validation in the loop. ✅

Security

  • No issues. Peewee parameterizes WHERE id IN (...) — no SQL injection risk.

Style & Conventions

  • Bulk of the diff is ruff/black auto-formatting (single→double quotes, multi-line function signatures). Expected per project setup, not introduced intentionally.

Suggestions

  • No blocking suggestions.

Verdict: APPROVED

The empty-batch regression flagged in the previous review has been correctly fixed with conditional guards at all affected sites. The N+1 optimization is sound and the implementation is complete.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v3/transactions.py` (modified) - `app/routers_v3/results.py` (modified) - `app/routers_v3/schedules.py` (modified) - `app/routers_v3/battingstats.py` (modified) ### Context This is a follow-up review. A previous pass issued REQUEST_CHANGES for an empty-batch regression: `Team.select(Team.id).where(Team.id << [])` produces `WHERE id IN ()`, which is a PostgreSQL syntax error. The fix commit (17f67ff) added conditional guards before all bulk queries. ### Findings #### Correctness - **Empty-batch guards confirmed** — all 6 bulk query sites now have `if all_team_ids else set()` / `if all_player_ids else set()` guards, preventing `WHERE id IN ()` on empty batches. Verified across all 4 endpoints: - `transactions.py`: team guard + player guard ✅ - `results.py`: team guard ✅ - `schedules.py`: team guard ✅ - `battingstats.py`: team guard + player guard ✅ - **Query reduction correct** — pre-fetch logic replaces per-row `get_or_none` with set membership check; error messages still include the specific invalid ID. - **Empty list input** — if the request body contains zero items, `all_team_ids` / `all_player_ids` are empty lists, guards short-circuit to `set()`, the validation loop is skipped, and `chunked([], 100)` produces no iterations. Clean no-op. ✅ - **Deduplication** — `set()` deduplication before bulk fetch is correct; duplicate IDs in the batch still get per-row validation in the loop. ✅ #### Security - No issues. Peewee parameterizes `WHERE id IN (...)` — no SQL injection risk. #### Style & Conventions - Bulk of the diff is ruff/black auto-formatting (single→double quotes, multi-line function signatures). Expected per project setup, not introduced intentionally. #### Suggestions - No blocking suggestions. ### Verdict: APPROVED The empty-batch regression flagged in the previous review has been correctly fixed with conditional guards at all affected sites. The N+1 optimization is sound and the implementation is complete. --- *Automated review by Claude PR Reviewer*
cal added
ai-reviewed
 and removed
ai-reviewing
 labels 2026-03-07 08:17:40 +00:00
cal merged commit f924e9ce5c into next-release 2026-03-10 14:37:57 +00:00
cal deleted branch ai/major-domo-database-25 2026-03-10 14:37:57 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working
No Label
ai-changes-requested
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 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/major-domo-database#52
No description provided.
Delete Branch "ai/major-domo-database-25"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?