feat: add limit/pagination to cardpositions endpoint (#142) #168

Claude · 2026-03-24T12:32:15Z

Claude commented

2026-03-24 12:32:15 +00:00

Closes #142

Adds limit query parameter to GET /api/v2/cardpositions.

Changes

limit: int = 100 added to get_card_positions() — default 100, consistent with all other paginated endpoints
Capped via max(0, min(limit, 500)) — guards against negative values and upper bound
.limit() applied after all filter branches and sort logic, before count and iteration — both affect count and positions list
count reflects the limited count (consistent with 13 of 14 other paginated endpoints)

File changed: app/routers_v2/cardpositions.py

Test results

No test suite — changes verified by code review. Ruff checks passed on commit.

Closes #142 Adds `limit` query parameter to `GET /api/v2/cardpositions`. ## Changes - `limit: int = 100` added to `get_card_positions()` — default 100, consistent with all other paginated endpoints - Capped via `max(0, min(limit, 500))` — guards against negative values and upper bound - `.limit()` applied after all filter branches and sort logic, before `count` and iteration — both affect count and positions list - `count` reflects the limited count (consistent with 13 of 14 other paginated endpoints) **File changed**: `app/routers_v2/cardpositions.py` ## Test results No test suite — changes verified by code review. Ruff checks passed on commit.

Claude added 1 commit 2026-03-24 12:32:16 +00:00

feat: add limit/pagination to cardpositions endpoint (#142 ) 8af43273d2

Closes #142

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Claude referenced this pull request

2026-03-24 12:32:27 +00:00

feat: add limit/pagination to cardpositions endpoint #142

Claude added the

ai-reviewing

label 2026-03-24 12:49:21 +00:00

Claude reviewed 2026-03-24 12:50:21 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/cardpositions.py (modified)

Findings

Correctness

Limit parameter added correctly: limit: int = 100, clamped via max(0, min(limit, 500)) — guards both negative values and upper bound. ✅
.limit() applied after all 7 filter branches (player_id, position, r, e, arm, pb, overthrow, cardset_id) and all 4 sort branches — correct placement, both count and positions list reflect the limit. ✅
No CSV path in this endpoint — no dual-path concern. ✅

Inconsistency with PR #169 (already merged)

NOTABLE: count in the response reflects the limited count, not total matching records. This was the pre-#169 pattern. PR #169 (feat: capture total_count before limit across all paginated endpoints) has already merged into main (commit bc6c23e) and updated 14 endpoints to compute total_count = queryset.count() before .limit(). The cardpositions endpoint was not included in PR #169, so this PR will be inconsistent with the updated project-wide pattern.
- The PR body says "consistent with 13 of 14 other paginated endpoints" — this was accurate at branch creation time, but main has moved.
- Suggest a follow-up to apply the same total_count pattern here, or include it in a future housekeeping PR.

Security

No issues found. No raw SQL, no exposed secrets, no auth bypass.

Style & Conventions

MINOR: limit: int (not Optional[int]) — same cosmetic pattern used in PRs #157, #159, #162, #165, #166, #167. Both work identically in FastAPI. Non-blocking.

Suggestions

After merge, a follow-up diff (mirroring PR #169) would make this endpoint consistent: move count = all_pos.count() before all_pos = all_pos.limit(limit) and rename the response key to total_count (or keep count as total, depending on the project convention chosen in #169).

Verdict: COMMENT

Implementation is functionally correct. The only notable issue is that count reflects limited rather than total rows, which is now inconsistent with the 14 endpoints updated by the already-merged PR #169. Non-blocking, but a follow-up is recommended to align cardpositions with the updated project-wide pagination pattern.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/cardpositions.py` (modified) ### Findings #### Correctness - Limit parameter added correctly: `limit: int = 100`, clamped via `max(0, min(limit, 500))` — guards both negative values and upper bound. ✅ - `.limit()` applied after all 7 filter branches (`player_id`, `position`, `r`, `e`, `arm`, `pb`, `overthrow`, `cardset_id`) and all 4 sort branches — correct placement, both `count` and `positions` list reflect the limit. ✅ - No CSV path in this endpoint — no dual-path concern. ✅ #### Inconsistency with PR #169 (already merged) - **NOTABLE**: `count` in the response reflects the limited count, not total matching records. This was the pre-#169 pattern. PR #169 (`feat: capture total_count before limit across all paginated endpoints`) has already merged into `main` (commit `bc6c23e`) and updated 14 endpoints to compute `total_count = queryset.count()` **before** `.limit()`. The `cardpositions` endpoint was not included in PR #169, so this PR will be inconsistent with the updated project-wide pattern. - The PR body says "consistent with 13 of 14 other paginated endpoints" — this was accurate at branch creation time, but `main` has moved. - Suggest a follow-up to apply the same `total_count` pattern here, or include it in a future housekeeping PR. #### Security - No issues found. No raw SQL, no exposed secrets, no auth bypass. #### Style & Conventions - **MINOR**: `limit: int` (not `Optional[int]`) — same cosmetic pattern used in PRs #157, #159, #162, #165, #166, #167. Both work identically in FastAPI. Non-blocking. #### Suggestions - After merge, a follow-up diff (mirroring PR #169) would make this endpoint consistent: move `count = all_pos.count()` before `all_pos = all_pos.limit(limit)` and rename the response key to `total_count` (or keep `count` as total, depending on the project convention chosen in #169). ### Verdict: COMMENT Implementation is functionally correct. The only notable issue is that `count` reflects limited rather than total rows, which is now inconsistent with the 14 endpoints updated by the already-merged PR #169. Non-blocking, but a follow-up is recommended to align `cardpositions` with the updated project-wide pagination pattern. --- *Automated review by Claude PR Reviewer*

Claude added

ai-reviewed

and removed

ai-reviewing

labels 2026-03-24 12:50:43 +00:00

cal approved these changes 2026-03-25 14:52:17 +00:00

cal left a comment

Approved as cal — reviewer verdict is COMMENT (non-blocking). The count inconsistency vs PR #169 pattern is noted for a follow-up cleanup PR.

cal added 1 commit 2026-03-25 14:52:38 +00:00

Merge branch 'main' into issue/142-feat-add-limit-pagination-to-cardpositions-endpoin 3d0c99b183

cal merged commit 792c6b96f9 into main

2026-03-25 14:52:56 +00:00