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

fix: enforce Literal validation on sort parameter in GET /api/v3/players (#66) #68

Open
Claude wants to merge 1 commits from ai/major-domo-database-66 into main
pull from: ai/major-domo-database-66
merge into: cal:main
Conversation 0 Commits 1 Files Changed 1 +2 -2
Claude commented 2026-03-17 23:03:45 +00:00
Collaborator
Copy Link

Closes #66

Summary

  • Changed sort: Optional[str] to sort: Optional[Literal["cost-asc", "cost-desc", "name-asc", "name-desc"]] in the get_players route signature in app/routers_v3/players.py
  • Added Literal to the typing imports

What was broken

The sort parameter in GET /api/v3/players accepted any string value. Invalid values (e.g. sort=INVALID) were silently ignored and fell through to the default ORDER BY id in _apply_player_sort. FastAPI had no way to reject them with a 422.

What was fixed

FastAPI now validates the sort parameter at binding time. Requests with an unrecognised sort value receive a 422 Unprocessable Entity before the handler runs.

Files changed

  • app/routers_v3/players.py — 2-line change (import + parameter type)

Notes

  • The valid values (cost-asc, cost-desc, name-asc, name-desc) were derived from the _apply_player_sort implementation in player_service.py
  • Existing callers in discord-app-v2 already use sort=cost-asc / sort=cost-desc, so no callers are broken by this change
  • No test suite changes needed — no automated tests cover the HTTP layer parameter validation
Closes #66 ## Summary - Changed `sort: Optional[str]` to `sort: Optional[Literal["cost-asc", "cost-desc", "name-asc", "name-desc"]]` in the `get_players` route signature in `app/routers_v3/players.py` - Added `Literal` to the `typing` imports ## What was broken The `sort` parameter in `GET /api/v3/players` accepted any string value. Invalid values (e.g. `sort=INVALID`) were silently ignored and fell through to the default `ORDER BY id` in `_apply_player_sort`. FastAPI had no way to reject them with a 422. ## What was fixed FastAPI now validates the `sort` parameter at binding time. Requests with an unrecognised `sort` value receive a `422 Unprocessable Entity` before the handler runs. ## Files changed - `app/routers_v3/players.py` — 2-line change (import + parameter type) ## Notes - The valid values (`cost-asc`, `cost-desc`, `name-asc`, `name-desc`) were derived from the `_apply_player_sort` implementation in `player_service.py` - Existing callers in `discord-app-v2` already use `sort=cost-asc` / `sort=cost-desc`, so no callers are broken by this change - No test suite changes needed — no automated tests cover the HTTP layer parameter validation
Claude added 1 commit 2026-03-17 23:03:46 +00:00
fix: enforce Literal validation on sort parameter in GET /api/v3/players (#66)
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m19s
Details
3d0699fe8a 
Closes #66

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude added the
ai-reviewing
 label 2026-03-17 23:15:49 +00:00
Claude reviewed 2026-03-17 23:16:39 +00:00
Claude left a comment
Author
Collaborator
Copy Link

Review: APPROVED ✅

Correct, minimal fix. Consistent with PR #36 which applied the same Literal pattern to views.py.

Verification

Literal values match _apply_player_sort exhaustively (player_service.py:294–301, 316–325):

  • cost-asc → ORDER BY wara ASC
  • cost-desc → ORDER BY wara DESC
  • name-asc → ORDER BY name ASC
  • name-desc → ORDER BY name DESC
  • anything else → ORDER BY id (default fallback)

With Optional[Literal[...]], FastAPI rejects anything outside those 4 values with 422 Unprocessable Entity before the handler runs. None (omitted param) remains valid — no regression for existing callers.

Checks

  • ✅ Literal added to typing import in correct position (alphabetical)
  • ✅ Optional[Literal[...]] preserves None as valid — no breaking change
  • ✅ All 4 valid values confirmed against service implementation
  • ✅ discord-app-v2 callers already use cost-asc/cost-desc — no callers broken
  • ✅ 2-line diff, no scope creep

No issues found. Ready to merge.

## Review: APPROVED ✅ Correct, minimal fix. Consistent with PR #36 which applied the same `Literal` pattern to `views.py`. ### Verification **Literal values match `_apply_player_sort` exhaustively** (`player_service.py:294–301`, `316–325`): - `cost-asc` → `ORDER BY wara ASC` - `cost-desc` → `ORDER BY wara DESC` - `name-asc` → `ORDER BY name ASC` - `name-desc` → `ORDER BY name DESC` - anything else → `ORDER BY id` (default fallback) With `Optional[Literal[...]]`, FastAPI rejects anything outside those 4 values with `422 Unprocessable Entity` before the handler runs. `None` (omitted param) remains valid — no regression for existing callers. ### Checks - ✅ `Literal` added to `typing` import in correct position (alphabetical) - ✅ `Optional[Literal[...]]` preserves `None` as valid — no breaking change - ✅ All 4 valid values confirmed against service implementation - ✅ discord-app-v2 callers already use `cost-asc`/`cost-desc` — no callers broken - ✅ 2-line diff, no scope creep No issues found. Ready to merge.
cal removed the
ai-reviewing
 label 2026-03-23 15:32:47 +00:00
Claude added the
ai-reviewing
 label 2026-03-23 15:53:41 +00:00
Claude reviewed 2026-03-23 15:54:37 +00:00
Claude left a comment
Author
Collaborator
Copy Link

AI Code Review

Files Reviewed

  • app/routers_v3/players.py (modified)

Findings

Correctness

  • Literal added to the typing import correctly (alphabetically ordered with existing imports).
  • The 4 Literal values (cost-asc, cost-desc, name-asc, name-desc) exactly match the exhaustive elif branches in _apply_player_sort (player_service.py:294–301). No valid sort value is excluded.
  • Optional[Literal[...]] correctly preserves the None default — callers omitting sort still get the default ORDER BY id behaviour.
  • FastAPI will now reject unrecognised values with 422 Unprocessable Entity at binding time, before the handler runs.

Security

  • No issues. User-supplied string is now constrained to an allowlist at the framework level — no injection risk.

Style & Conventions

  • Follows the same pattern used in PR #36 (views.py sort_by Literal fix). Consistent approach across the codebase.
  • No unnecessary changes to surrounding code.

Suggestions

  • None. The change is minimal and correct.

Verdict: APPROVED

2-line change is correct and complete. Literal values match the implementation, None is preserved, and no callers are broken. Ready to merge.

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v3/players.py` (modified) ### Findings #### Correctness - `Literal` added to the `typing` import correctly (alphabetically ordered with existing imports). - The 4 Literal values (`cost-asc`, `cost-desc`, `name-asc`, `name-desc`) exactly match the exhaustive `elif` branches in `_apply_player_sort` (`player_service.py:294–301`). No valid sort value is excluded. - `Optional[Literal[...]]` correctly preserves the `None` default — callers omitting `sort` still get the default `ORDER BY id` behaviour. - FastAPI will now reject unrecognised values with `422 Unprocessable Entity` at binding time, before the handler runs. #### Security - No issues. User-supplied string is now constrained to an allowlist at the framework level — no injection risk. #### Style & Conventions - Follows the same pattern used in PR #36 (`views.py` `sort_by` Literal fix). Consistent approach across the codebase. - No unnecessary changes to surrounding code. #### Suggestions - None. The change is minimal and correct. ### Verdict: APPROVED 2-line change is correct and complete. Literal values match the implementation, `None` is preserved, and no callers are broken. Ready to merge. --- *Automated review by Claude PR Reviewer*
Claude added the
ai-reviewed
 label 2026-03-23 15:54:41 +00:00
All checks were successful
Build Docker Image / build (pull_request) Successful in 2m19s
Details
This pull request has changes conflicting with the target branch.
  • app/routers_v3/players.py
View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin ai/major-domo-database-66:ai/major-domo-database-66
git checkout ai/major-domo-database-66
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#68
No description provided.
Delete Branch "ai/major-domo-database-66"

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?