cal/paper-dynasty-database
1
0
Fork 0
Code Issues 10 Pull Requests 5 Actions Packages Projects Releases Wiki Activity

fix: ensure count is never null in GET /refractor/cards (#182) #184

Open
Claude wants to merge 2 commits from issue/182-bug-get-refractor-cards-returns-count-null-with-ou into main
pull from: issue/182-bug-get-refractor-cards-returns-count-null-with-ou
merge into: cal:main
Conversation 0 Commits 2 Files Changed 2 +2 -2
Claude commented 2026-04-06 06:07:58 +00:00
Collaborator
Copy Link

Closes #182

Summary

GET /api/v2/refractor/cards could return {"count": null, "items": []} when a filter combination yielded 0 matching rows. Peewee 3.17.9's .count() on a complex multi-join query (three-table JOIN with LEFT OUTER) can return None when the cursor wrapper produces an empty result for a 0-row count query.

Fix

app/routers_v2/refractor.py — one line:

# before
total = query.count()

# after
total = query.count() or 0

Guards the count result with or 0 so the response contract (count is always an integer) is always upheld.

Additional context

The reported reproduction step (tier=99) is already rejected upstream by FastAPI's ge=0, le=4 constraint on the tier parameter, returning a 422 before the query runs. The or 0 guard covers the remaining cases where a valid filter combination (e.g. tier=4 with no T4 cards) produces 0 results.

Files changed

  • app/routers_v2/refractor.py — total = query.count() or 0

Other observations

The GET /api/v2/teams/{team_id}/refractors endpoint in teams.py (line 1579) has the same total = query.count() pattern and no ge=0, le=4 constraint on tier — it would be susceptible to the same null-count bug for any 0-row result, including tier=99. Out of scope for this issue but worth a follow-up.

Closes #182 ## Summary `GET /api/v2/refractor/cards` could return `{"count": null, "items": []}` when a filter combination yielded 0 matching rows. Peewee 3.17.9's `.count()` on a complex multi-join query (three-table JOIN with LEFT OUTER) can return `None` when the cursor wrapper produces an empty result for a 0-row count query. ## Fix `app/routers_v2/refractor.py` — one line: ```python # before total = query.count() # after total = query.count() or 0 ``` Guards the count result with `or 0` so the response contract (`count` is always an integer) is always upheld. ## Additional context The reported reproduction step (`tier=99`) is already rejected upstream by FastAPI's `ge=0, le=4` constraint on the `tier` parameter, returning a 422 before the query runs. The `or 0` guard covers the remaining cases where a valid filter combination (e.g. `tier=4` with no T4 cards) produces 0 results. ## Files changed - `app/routers_v2/refractor.py` — `total = query.count() or 0` ## Other observations The `GET /api/v2/teams/{team_id}/refractors` endpoint in `teams.py` (line 1579) has the same `total = query.count()` pattern and no `ge=0, le=4` constraint on `tier` — it would be susceptible to the same null-count bug for any 0-row result, including `tier=99`. Out of scope for this issue but worth a follow-up.
Claude added 2 commits 2026-04-06 06:08:00 +00:00 
Dev environment uses sba_postgres container, paperdynasty_dev database,
sba_admin user — not pd_postgres/pd_master as previously documented.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Peewee's .count() on a complex multi-join query can return None when the
result wrapper yields an empty cursor for a 0-row result set.  Guard with
`or 0` so the response contract (count is always an integer) is upheld.

Note: tier values outside 0–4 already return 422 via FastAPI's ge/le
constraints on the tier parameter, so the reproduced scenario
(tier=99 → count: null) is also covered by validation.

Closes #182

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-04-06 06:15:43 +00:00
Claude reviewed 2026-04-06 06:17:26 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v2/refractor.py (modified)
  • CLAUDE.md (modified)

Findings

Correctness

  • Fix is correct. query.count() or 0 maps both None and 0 to 0 while leaving positive integers unchanged. The response contract (count is always an integer) is upheld in all cases.
  • Claim is plausible. Peewee's .count() on a ModelSelect with multi-model .select() wraps the original query as a subquery (SELECT COUNT(1) FROM (...) AS t1). If the cursor wrapper returns no rows rather than a (0,) row, the result is None before Python coerces it. The guard is the right defensive fix.
  • CLAUDE.md hunk note. The CLAUDE.md change in this PR is identical to commit cc65dc5 already on main (merged 2026-04-06). The PR branch predates that merge. Depending on git's merge strategy this will either auto-resolve cleanly (same change on both sides) or produce a trivially resolvable conflict — worth a quick check before merging.

Security

  • No issues. Input is validated by FastAPI query constraints (ge=0, le=4 on tier, ge=1, le=100 on limit). No raw SQL or user-controlled expressions.

Style & Conventions

  • One-line change, idiomatic Python null-coalesce idiom, consistent with the project's existing pattern.

Suggestions

  • The teams.py GET /{team_id}/refractors endpoint (line 1579) has the same total = query.count() pattern and no ge=0, le=4 guard on tier, making it susceptible to the same null-count contract violation for any 0-row result (including tier=99). The PR author already flagged this as a follow-up — tracking issue recommended.

Verdict: APPROVED (posted as COMMENT — self-review restriction)

Clean, targeted, correct fix. The one-line change directly addresses the null-count contract violation with no side effects. The CLAUDE.md merge note is cosmetic and non-blocking.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/refractor.py` (modified) - `CLAUDE.md` (modified) ### Findings #### Correctness - **Fix is correct.** `query.count() or 0` maps both `None` and `0` to `0` while leaving positive integers unchanged. The response contract (`count` is always an integer) is upheld in all cases. - **Claim is plausible.** Peewee's `.count()` on a `ModelSelect` with multi-model `.select()` wraps the original query as a subquery (`SELECT COUNT(1) FROM (...) AS t1`). If the cursor wrapper returns no rows rather than a `(0,)` row, the result is `None` before Python coerces it. The guard is the right defensive fix. - **CLAUDE.md hunk note.** The CLAUDE.md change in this PR is identical to commit `cc65dc5` already on `main` (merged 2026-04-06). The PR branch predates that merge. Depending on git's merge strategy this will either auto-resolve cleanly (same change on both sides) or produce a trivially resolvable conflict — worth a quick check before merging. #### Security - No issues. Input is validated by FastAPI query constraints (`ge=0, le=4` on `tier`, `ge=1, le=100` on `limit`). No raw SQL or user-controlled expressions. #### Style & Conventions - One-line change, idiomatic Python null-coalesce idiom, consistent with the project's existing pattern. #### Suggestions - The `teams.py` `GET /{team_id}/refractors` endpoint (line 1579) has the same `total = query.count()` pattern and no `ge=0, le=4` guard on `tier`, making it susceptible to the same null-count contract violation for any 0-row result (including `tier=99`). The PR author already flagged this as a follow-up — tracking issue recommended. ### Verdict: APPROVED (posted as COMMENT — self-review restriction) Clean, targeted, correct fix. The one-line change directly addresses the null-count contract violation with no side effects. The CLAUDE.md merge note is cosmetic and non-blocking. --- *Automated review by Claude PR Reviewer*
Claude added the
ai-reviewed
 label 2026-04-06 06:17:59 +00:00
This pull request doesn't have enough approvals yet. 0 of 1 approvals granted.
You are not authorized to merge this pull request.
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin issue/182-bug-get-refractor-cards-returns-count-null-with-ou:issue/182-bug-get-refractor-cards-returns-count-null-with-ou
git checkout issue/182-bug-get-refractor-cards-returns-count-null-with-ou
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
ai-changes-requested

   
ai-failed

   
ai-merged

PR merged by AI ops agent

  
ai-pr-opened

   
ai-reviewed

   
ai-reviewing

   
ai-reviewing

   
ai-working

   
bug

   
enhancement

   
evolution

Card Evolution system

  
performance

   
phase-0

Render Pipeline Optimization

  
phase-1a

Schema & Data Foundation

  
phase-1b

API & Formula Engine

  
phase-1c

Bot Integration

  
phase-1d

Deployment

  
security

   
tech-debt

   
todo
No Label
ai-changes-requested
ai-failed
ai-merged
ai-pr-opened
ai-reviewed
ai-reviewing
ai-reviewing
ai-working
bug
enhancement
evolution
performance
phase-0
phase-1a
phase-1b
phase-1c
phase-1d
security
tech-debt
todo
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/paper-dynasty-database#184
No description provided.
Delete Branch "issue/182-bug-get-refractor-cards-returns-count-null-with-ou"

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?