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

Fix unbounded API queries causing worker timeouts #99

Merged
cal merged 2 commits from bugfix/limit-caps into main 2026-04-01 22:44:40 +00:00
Conversation 0 Commits 2 Files Changed 27 +505 -158
cal commented 2026-04-01 22:23:50 +00:00
Owner
Copy Link

Summary

Resolves #98 — Gunicorn workers timing out from unbounded queries sent through the reverse proxy.

  • Add MAX_LIMIT=500 and DEFAULT_LIMIT=200 constants enforced across all list endpoints
  • Add strip_empty_query_params middleware to prevent empty string params (?game_id=) from bypassing filters
  • Add limit/offset pagination to /transactions (previously returned all rows)
  • Cap existing limit params with le=MAX_LIMIT on 11 endpoints
  • Add limit/offset to 13 endpoints that had no limit at all
  • Remove redundant manual if limit < 1 guards (now handled by ge=1 validation)

Context

12 worker timeouts in 2 days. Source: external clients via nginx-proxy-manager sending limit=99999 and empty filter params. Only /players/search had a max cap — every other endpoint trusted the client.

Test plan

  • 5 new unit tests in test_query_limits.py — all pass
  • Verify limit=1000 returns 422 on any list endpoint
  • Verify ?move_id= treated as absent (not as empty string filter)
  • Verify /transactions response includes count, limit, offset
  • Smoke test bot commands and website after deploy

🤖 Generated with Claude Code

## Summary Resolves #98 — Gunicorn workers timing out from unbounded queries sent through the reverse proxy. - Add `MAX_LIMIT=500` and `DEFAULT_LIMIT=200` constants enforced across all list endpoints - Add `strip_empty_query_params` middleware to prevent empty string params (`?game_id=`) from bypassing filters - Add `limit`/`offset` pagination to `/transactions` (previously returned all rows) - Cap existing `limit` params with `le=MAX_LIMIT` on 11 endpoints - Add `limit`/`offset` to 13 endpoints that had no limit at all - Remove redundant manual `if limit < 1` guards (now handled by `ge=1` validation) ## Context 12 worker timeouts in 2 days. Source: external clients via nginx-proxy-manager sending `limit=99999` and empty filter params. Only `/players/search` had a max cap — every other endpoint trusted the client. ## Test plan - [x] 5 new unit tests in `test_query_limits.py` — all pass - [ ] Verify `limit=1000` returns 422 on any list endpoint - [ ] Verify `?move_id=` treated as absent (not as empty string filter) - [ ] Verify `/transactions` response includes `count`, `limit`, `offset` - [ ] Smoke test bot commands and website after deploy 🤖 Generated with [Claude Code](https://claude.com/claude-code)
cal added 1 commit 2026-04-01 22:23:51 +00:00
Fix unbounded API queries causing Gunicorn worker timeouts
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m32s
Details
16f3f8d8de 
Add MAX_LIMIT=500 cap across all list endpoints, empty string
stripping middleware, and limit/offset to /transactions. Resolves #98.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-04-01 22:28:10 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Files Reviewed

  • app/dependencies.py (modified — constants)
  • app/main.py (modified — middleware)
  • app/routers_v3/awards.py (modified)
  • app/routers_v3/battingstats.py (modified)
  • app/routers_v3/decisions.py (modified)
  • app/routers_v3/divisions.py (modified)
  • app/routers_v3/draftlist.py (modified)
  • app/routers_v3/draftpicks.py (modified)
  • app/routers_v3/fieldingstats.py (modified — heavy reformat + functional changes)
  • app/routers_v3/injuries.py (modified)
  • app/routers_v3/keepers.py (modified)
  • app/routers_v3/managers.py (modified)
  • app/routers_v3/pitchingstats.py (modified)
  • app/routers_v3/players.py (modified)
  • app/routers_v3/results.py (modified)
  • app/routers_v3/sbaplayers.py (modified)
  • app/routers_v3/schedules.py (modified)
  • app/routers_v3/standings.py (modified)
  • app/routers_v3/stratgame.py (modified)
  • app/routers_v3/stratplay/batting.py (modified)
  • app/routers_v3/stratplay/fielding.py (modified)
  • app/routers_v3/stratplay/pitching.py (modified)
  • app/routers_v3/stratplay/plays.py (modified)
  • app/routers_v3/teams.py (modified — import only)
  • app/routers_v3/transactions.py (modified)
  • app/routers_v3/views.py (modified)
  • tests/unit/test_query_limits.py (added)

Findings

Correctness

BUG — fieldingstats.get_fieldingstats: count computed after .limit() is applied

# fieldingstats.py ~line 99
all_stats = all_stats.limit(limit)
if sort:
    ...
return_stats = {
    "count": all_stats.count(),   # ← BUG: count on already-limited queryset
    "stats": [...],
}

Peewee's .count() wraps the queryset in a subquery and preserves the LIMIT clause, so all_stats.limit(200).count() returns at most 200 — not the total number of matching records. Every other endpoint in this PR correctly captures the total before pagination:

total_count = all_stats.count()          # correct pattern, used everywhere else
all_stats = all_stats.offset(offset).limit(limit)
return {"count": total_count, ...}

fieldingstats.get_fieldingstats was buried under the heavy reformatting and the fix was missed. The repair is two lines: capture total_count = all_stats.count() before the .limit() call, then reference total_count in the response dict. (This endpoint doesn't need offset, consistent with battingstats.get_batstats, decisions, pitchingstats.get_pitstats.)

Non-blocking — fieldingstats.get_totalstats: total_count is immediately discarded

total_count = all_stats.count()          # computed correctly...
all_stats = all_stats.offset(offset).limit(limit)
return_stats = {"count": total_count, "stats": []}
for x in all_stats:
    if x.sum_xch + x.sum_sbc <= 0:
        continue
    return_stats["stats"].append(...)
return_stats["count"] = len(return_stats["stats"])  # ...then overwritten with page size

The final line overwrites total_count with the length of the current page (after the post-filter). This is pre-existing behaviour; the PR doesn't regress it. But the total_count variable is now dead/misleading. Either drop the last overwrite (count reflects total) or don't capture total_count at all. Low priority.

transactions.py pagination is correct. total_count is captured before .offset().limit(). The new limit/offset keys in the response are additive — existing clients that only read count and transactions are not affected.

Breaking Changes

Adding limit/offset to endpoints that previously returned all rows is a breaking change for callers relying on complete result sets. The bot likely calls /transactions?season=12, /battingstats, /pitchingstats, etc. without an explicit limit and previously received every row. After this change those requests return at most 200 rows by default (max 500).

The "Smoke test bot commands and website after deploy" checkbox being unchecked is correct — this is the most operationally risky part of the change. Pay particular attention to transaction history display, stat leaderboards, and any bot command that pages through a full season.

is_trade removal from /transactions is safe — it previously returned 501, so removal is strictly better.

Security

  • No new injection vectors.
  • strip_empty_query_params middleware uses parse_qsl correctly (not naive string splitting). The hasattr guard before del request._query_params is correct — if the cached attribute doesn't exist yet nothing breaks.
  • request._query_params is a private Starlette attribute. Given this project had a Starlette version incident (PR #63 / requirements pin), this is worth monitoring. The hasattr guard means a future rename silently reverts to non-stripped behaviour rather than crashing — acceptable risk.

Style & Conventions

fieldingstats.py is the only file where ruff/black reformatting (single→double quotes, import block restructuring) is mixed with functional limit changes. This makes the functional diff very hard to audit. For future safety-focused PRs, separating cosmetic and functional changes into separate commits would make review easier.

sbaplayers.py: The CSV code path (early return before the new total_count/pagination lines) means CSV exports remain unbounded. Likely intentional for batch use, but worth confirming.

players.py: offset remains Optional[int] = Query(default=None, ...) rather than int = Query(default=0, ...) like all other paginated endpoints added in this PR. Minor inconsistency.

Test Coverage

  • 5 new unit tests cover: limit > MAX_LIMIT → 422, limit=0 → 422, limit=-1 → 422, transactions response has limit/offset keys, empty string param stripped. Good coverage of the happy path.
  • The module-level stub-before-import pattern is correct for avoiding psycopg2/redis import failures in CI.
  • After fixing the fieldingstats count bug, add a regression test for it.

Verdict: REQUEST_CHANGES

One functional bug must be fixed before merge: fieldingstats.get_fieldingstats calls .count() after .limit() is applied, so the count field in the response is capped at the limit (200 by default) rather than reflecting the total number of matching records. This was obscured by the large reformatting diff in that file. Every other endpoint was correctly updated; this one needs a two-line fix.

Everything else — the middleware implementation, the limit enforcement pattern across 20+ endpoints, the transactions pagination with limit/offset in the response, the stratplay paginate() changes — is correct and well-executed. Good work catching this before it caused more timeouts.

Automated review by Claude PR Reviewer — verdict: REQUEST_CHANGES (Gitea blocks self-review)

## AI Code Review ### Files Reviewed - `app/dependencies.py` (modified — constants) - `app/main.py` (modified — middleware) - `app/routers_v3/awards.py` (modified) - `app/routers_v3/battingstats.py` (modified) - `app/routers_v3/decisions.py` (modified) - `app/routers_v3/divisions.py` (modified) - `app/routers_v3/draftlist.py` (modified) - `app/routers_v3/draftpicks.py` (modified) - `app/routers_v3/fieldingstats.py` (modified — heavy reformat + functional changes) - `app/routers_v3/injuries.py` (modified) - `app/routers_v3/keepers.py` (modified) - `app/routers_v3/managers.py` (modified) - `app/routers_v3/pitchingstats.py` (modified) - `app/routers_v3/players.py` (modified) - `app/routers_v3/results.py` (modified) - `app/routers_v3/sbaplayers.py` (modified) - `app/routers_v3/schedules.py` (modified) - `app/routers_v3/standings.py` (modified) - `app/routers_v3/stratgame.py` (modified) - `app/routers_v3/stratplay/batting.py` (modified) - `app/routers_v3/stratplay/fielding.py` (modified) - `app/routers_v3/stratplay/pitching.py` (modified) - `app/routers_v3/stratplay/plays.py` (modified) - `app/routers_v3/teams.py` (modified — import only) - `app/routers_v3/transactions.py` (modified) - `app/routers_v3/views.py` (modified) - `tests/unit/test_query_limits.py` (added) --- ### Findings #### Correctness **BUG — `fieldingstats.get_fieldingstats`: count computed after `.limit()` is applied** ```python # fieldingstats.py ~line 99 all_stats = all_stats.limit(limit) if sort: ... return_stats = { "count": all_stats.count(), # ← BUG: count on already-limited queryset "stats": [...], } ``` Peewee's `.count()` wraps the queryset in a subquery and preserves the `LIMIT` clause, so `all_stats.limit(200).count()` returns at most 200 — not the total number of matching records. Every other endpoint in this PR correctly captures the total **before** pagination: ```python total_count = all_stats.count() # correct pattern, used everywhere else all_stats = all_stats.offset(offset).limit(limit) return {"count": total_count, ...} ``` `fieldingstats.get_fieldingstats` was buried under the heavy reformatting and the fix was missed. The repair is two lines: capture `total_count = all_stats.count()` before the `.limit()` call, then reference `total_count` in the response dict. (This endpoint doesn't need `offset`, consistent with `battingstats.get_batstats`, `decisions`, `pitchingstats.get_pitstats`.) **Non-blocking — `fieldingstats.get_totalstats`: `total_count` is immediately discarded** ```python total_count = all_stats.count() # computed correctly... all_stats = all_stats.offset(offset).limit(limit) return_stats = {"count": total_count, "stats": []} for x in all_stats: if x.sum_xch + x.sum_sbc <= 0: continue return_stats["stats"].append(...) return_stats["count"] = len(return_stats["stats"]) # ...then overwritten with page size ``` The final line overwrites `total_count` with the length of the current page (after the post-filter). This is pre-existing behaviour; the PR doesn't regress it. But the `total_count` variable is now dead/misleading. Either drop the last overwrite (count reflects total) or don't capture `total_count` at all. Low priority. **`transactions.py` pagination is correct.** `total_count` is captured before `.offset().limit()`. The new `limit`/`offset` keys in the response are additive — existing clients that only read `count` and `transactions` are not affected. #### Breaking Changes Adding `limit`/`offset` to endpoints that previously returned all rows is a **breaking change for callers relying on complete result sets**. The bot likely calls `/transactions?season=12`, `/battingstats`, `/pitchingstats`, etc. without an explicit `limit` and previously received every row. After this change those requests return at most 200 rows by default (max 500). The "Smoke test bot commands and website after deploy" checkbox being unchecked is correct — this is the most operationally risky part of the change. Pay particular attention to transaction history display, stat leaderboards, and any bot command that pages through a full season. `is_trade` removal from `/transactions` is safe — it previously returned 501, so removal is strictly better. #### Security - No new injection vectors. - `strip_empty_query_params` middleware uses `parse_qsl` correctly (not naive string splitting). The `hasattr` guard before `del request._query_params` is correct — if the cached attribute doesn't exist yet nothing breaks. - `request._query_params` is a private Starlette attribute. Given this project had a Starlette version incident (PR #63 / requirements pin), this is worth monitoring. The `hasattr` guard means a future rename silently reverts to non-stripped behaviour rather than crashing — acceptable risk. #### Style & Conventions `fieldingstats.py` is the only file where ruff/black reformatting (single→double quotes, import block restructuring) is mixed with functional limit changes. This makes the functional diff very hard to audit. For future safety-focused PRs, separating cosmetic and functional changes into separate commits would make review easier. `sbaplayers.py`: The CSV code path (early return before the new `total_count`/pagination lines) means CSV exports remain unbounded. Likely intentional for batch use, but worth confirming. `players.py`: `offset` remains `Optional[int] = Query(default=None, ...)` rather than `int = Query(default=0, ...)` like all other paginated endpoints added in this PR. Minor inconsistency. #### Test Coverage - 5 new unit tests cover: `limit > MAX_LIMIT → 422`, `limit=0 → 422`, `limit=-1 → 422`, transactions response has `limit`/`offset` keys, empty string param stripped. Good coverage of the happy path. - The module-level stub-before-import pattern is correct for avoiding psycopg2/redis import failures in CI. - After fixing the `fieldingstats` count bug, add a regression test for it. --- ### Verdict: REQUEST_CHANGES One functional bug must be fixed before merge: **`fieldingstats.get_fieldingstats` calls `.count()` after `.limit()` is applied**, so the `count` field in the response is capped at the limit (200 by default) rather than reflecting the total number of matching records. This was obscured by the large reformatting diff in that file. Every other endpoint was correctly updated; this one needs a two-line fix. Everything else — the middleware implementation, the limit enforcement pattern across 20+ endpoints, the transactions pagination with `limit`/`offset` in the response, the stratplay `paginate()` changes — is correct and well-executed. Good work catching this before it caused more timeouts. --- *Automated review by Claude PR Reviewer — verdict: REQUEST_CHANGES (Gitea blocks self-review)*
cal added 1 commit 2026-04-01 22:40:09 +00:00
Fix fieldingstats count computed after limit applied
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m9s
Details
67e87a893a 
Capture total_count before .limit() so the response count reflects
all matching rows, not just the capped page size. Resolves #100.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
cal reviewed 2026-04-01 22:42:08 +00:00
cal left a comment
Author
Owner
Copy Link

AI Code Review

Follow-up Review — commit 67e87a8

Reviewing the fix for the bug identified in the previous review: get_fieldingstats was calling .count() after .limit(), causing the count field in the response to reflect only the number of rows in the current page rather than the total matching records.

Fix Verification

app/routers_v3/fieldingstats.py — the fix is correct:

# Before (buggy)
if limit:
    all_stats = all_stats.limit(limit)
# ...
'count': all_stats.count(),  # count() called after limit() — returns ≤ limit, not total

# After (fixed)
total_count = all_stats.count()   # captured before limit is applied
all_stats = all_stats.limit(limit)
# ...
"count": total_count,              # true total matching records

all_stats.count() at that point in the code is an unexecuted Peewee ModelSelect — calling .count() issues a SELECT COUNT(*) against the full filtered result set. .limit() is then applied separately for the data fetch. This is the correct pattern and exactly mirrors what was done for awards.py and other endpoints in this same PR.

Findings

Correctness

  • Fix is correct. total_count is now the true count of all matching records before pagination, consistent with how all other endpoints in this PR handle it.

Security

  • No issues.

Style & Conventions

  • No issues.

Verdict: APPROVED

The bug is resolved. The fix is minimal, correct, and consistent with the pattern used across the rest of the PR.

Automated review by Claude PR Reviewer

## AI Code Review ### Follow-up Review — commit 67e87a8 Reviewing the fix for the bug identified in the previous review: `get_fieldingstats` was calling `.count()` after `.limit()`, causing the `count` field in the response to reflect only the number of rows in the current page rather than the total matching records. ### Fix Verification **`app/routers_v3/fieldingstats.py`** — the fix is correct: ```python # Before (buggy) if limit: all_stats = all_stats.limit(limit) # ... 'count': all_stats.count(), # count() called after limit() — returns ≤ limit, not total # After (fixed) total_count = all_stats.count() # captured before limit is applied all_stats = all_stats.limit(limit) # ... "count": total_count, # true total matching records ``` `all_stats.count()` at that point in the code is an unexecuted Peewee `ModelSelect` — calling `.count()` issues a `SELECT COUNT(*)` against the full filtered result set. `.limit()` is then applied separately for the data fetch. This is the correct pattern and exactly mirrors what was done for `awards.py` and other endpoints in this same PR. ### Findings #### Correctness - Fix is correct. `total_count` is now the true count of all matching records before pagination, consistent with how all other endpoints in this PR handle it. #### Security - No issues. #### Style & Conventions - No issues. ### Verdict: APPROVED The bug is resolved. The fix is minimal, correct, and consistent with the pattern used across the rest of the PR. --- *Automated review by Claude PR Reviewer*
cal merged commit 81baa54681 into main 2026-04-01 22:44:40 +00:00
cal deleted branch bugfix/limit-caps 2026-04-01 22:44:41 +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#99
No description provided.
Delete Branch "bugfix/limit-caps"

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?