feat(api): REST polish — cache fix, Location header, 409 detail (#530)#548
Merged
nanotaboada merged 3 commits intomasterfrom Apr 1, 2026
Merged
feat(api): REST polish — cache fix, Location header, 409 detail (#530)#548nanotaboada merged 3 commits intomasterfrom
nanotaboada merged 3 commits intomasterfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 28 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughGET /players/ now treats cached empty lists as hits; POST /players/ sets a Location header on 201 and returns a human-readable detail on 409. Tests and changelog updated; pre-commit and pre-release docs added. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant API as API (routes/player_route.py)
participant Cache as Cache
participant DB as Database
Client->>API: GET /players/
API->>Cache: fetch players key
alt cache miss (None)
Cache-->>API: None
API->>DB: query players
DB-->>API: []
API-->>Cache: store empty list
API-->>Client: 200 OK []
else cache hit (list or empty)
Cache-->>API: [] or [players]
API-->>Client: 200 OK (cached content)
end
Client->>API: POST /players/ (body)
API->>DB: check existing by squad_number
alt exists
DB-->>API: found
API-->>Client: 409 Conflict (detail: "A Player with this squad number already exists.")
else not found
DB-->>API: none
API->>DB: create player
DB-->>API: created player (with squad_number)
API-->>API: set response.headers["Location"] = /players/squadnumber/{squad_number}
API-->>Client: 201 Created (Location header)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Out-of-scope changes
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #548 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 113 114 +1
=========================================
+ Hits 113 114 +1
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_main.py (1)
292-304: Make this Location-header test self-contained with a 201 assertion.Right now header correctness is asserted, but not the success status in this same test.
♻️ Suggested patch
response = client.post(PATH, json=player.__dict__) # Assert + assert response.status_code == 201 assert "Location" in response.headers assert ( response.headers["Location"] == f"/players/squadnumber/{player.squad_number}" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_main.py` around lines 292 - 304, The test test_request_post_player_body_nonexistent_response_header_location currently only checks the Location header; update it to be self-contained by asserting the response status code is 201 before validating headers. In the test body (using client.post(PATH, json=player.__dict__) and the nonexistent_player() fixture), add assert response.status_code == 201 immediately after the post and then proceed to assert the Location header equals f"/players/squadnumber/{player.squad_number}". Ensure the test still uses PATH, client, and player from nonexistent_player().routes/player_route.py (1)
49-53: Add explicit return type hints on modified handlers.Both updated route functions still omit return annotations. Adding them keeps this file aligned with the repo typing standard.
♻️ Suggested patch
async def post_async( player_model: Annotated[PlayerRequestModel, Body(...)], async_session: Annotated[AsyncSession, Depends(generate_async_session)], response: Response, -): +) -> PlayerResponseModel: @@ async def get_all_async( response: Response, async_session: Annotated[AsyncSession, Depends(generate_async_session)], -): +) -> List[PlayerResponseModel]:As per coding guidelines:
Type hints are required everywhere — functions, variables, and return types.Also applies to: 97-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@routes/player_route.py` around lines 49 - 53, Add explicit return type annotations to the updated route handlers: annotate post_async with a concrete return type (for example -> Response or the specific response model like -> PlayerResponseModel if the handler returns a model) and do the same for the other modified handler referenced around lines 97-100; ensure you import Response or the proper response model/type and update the function signatures (post_async and the other route function name) to include the chosen return type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@routes/player_route.py`:
- Around line 49-53: Add explicit return type annotations to the updated route
handlers: annotate post_async with a concrete return type (for example ->
Response or the specific response model like -> PlayerResponseModel if the
handler returns a model) and do the same for the other modified handler
referenced around lines 97-100; ensure you import Response or the proper
response model/type and update the function signatures (post_async and the other
route function name) to include the chosen return type.
In `@tests/test_main.py`:
- Around line 292-304: The test
test_request_post_player_body_nonexistent_response_header_location currently
only checks the Location header; update it to be self-contained by asserting the
response status code is 201 before validating headers. In the test body (using
client.post(PATH, json=player.__dict__) and the nonexistent_player() fixture),
add assert response.status_code == 201 immediately after the post and then
proceed to assert the Location header equals
f"/players/squadnumber/{player.squad_number}". Ensure the test still uses PATH,
client, and player from nonexistent_player().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93b147a0-4e5c-4f77-b26c-e4dd8de7ad33
📒 Files selected for processing (3)
CHANGELOG.mdroutes/player_route.pytests/test_main.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
.claude/commands/prerelease.md (1)
26-27: Add an explicit/precommitgate before push/PR.Please add a step to run
/precommitafter preparing the changelog commit (and before pushing/opening the PR) so release branches consistently pass the project’s expected checks.Proposed doc tweak
-6. **After confirmation**: commit, push the branch, open a PR into `master`. +6. **After confirmation**: commit, run `/precommit`, then push the branch and open a PR into `master`.Based on learnings: "Run
/precommitto execute the full pre-commit checklist for this project".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/prerelease.md around lines 26 - 27, Update the prerelease instructions to add an explicit step to run the project precommit hook: after the changelog/confirmation commit and before pushing or opening the PR (the step currently worded as "6. **After confirmation**: commit, push the branch, open a PR into `master`.") insert a new line instructing the user to run the `/precommit` command to execute the full pre-commit checklist, and only then push and open the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In @.claude/commands/prerelease.md:
- Around line 26-27: Update the prerelease instructions to add an explicit step
to run the project precommit hook: after the changelog/confirmation commit and
before pushing or opening the PR (the step currently worded as "6. **After
confirmation**: commit, push the branch, open a PR into `master`.") insert a new
line instructing the user to run the `/precommit` command to execute the full
pre-commit checklist, and only then push and open the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4764e611-d235-4b8c-aca3-f438d9fbf52b
📒 Files selected for processing (4)
.claude/commands/precommit.md.claude/commands/prerelease.mdroutes/player_route.pytests/test_main.py
✅ Files skipped from review due to trivial changes (2)
- .claude/commands/precommit.md
- tests/test_main.py
🚧 Files skipped from review as they are similar to previous changes (1)
- routes/player_route.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/commands/prerelease.md (1)
15-17: Keep confirmation gating explicit beforegit pull.To stay consistent with “propose before acting,” reword this step to require confirmation before pulling from remote.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/prerelease.md around lines 15 - 17, Update the "Verify working tree" step to require explicit confirmation before running any git pull: change the line that currently reads "Confirm we are on `master` with a clean working tree. Pull latest if behind remote." to a two-part instruction that first instructs the user to verify they are on `master` with a clean working tree and then to explicitly confirm (e.g., "Confirm to proceed") before executing any `git pull` from remote; ensure the reworded step clearly separates verification from the action and prompts for confirmation prior to pulling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/prerelease.md:
- Line 1: Add a top-level H1 as the first line of the prerelease markdown to
satisfy markdownlint MD041: insert a line like "# Pre-release Checklist" (or an
appropriate title) at the very top of .claude/commands/prerelease.md so the file
begins with an H1 heading before the existing body text.
---
Nitpick comments:
In @.claude/commands/prerelease.md:
- Around line 15-17: Update the "Verify working tree" step to require explicit
confirmation before running any git pull: change the line that currently reads
"Confirm we are on `master` with a clean working tree. Pull latest if behind
remote." to a two-part instruction that first instructs the user to verify they
are on `master` with a clean working tree and then to explicitly confirm (e.g.,
"Confirm to proceed") before executing any `git pull` from remote; ensure the
reworded step clearly separates verification from the action and prompts for
confirmation prior to pulling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 423d7112-892b-4c61-afb5-6372d1bb7d8c
📒 Files selected for processing (1)
.claude/commands/prerelease.md
- Fix GET /players/ cache check: `if not players` → `if players is None`
so empty collections are cached correctly
- Add Location header to POST /players/ 201 response pointing to
/players/squadnumber/{squad_number} per RFC 7231 §7.1.2
- Add detail message to POST /players/ 409 response
- Add return type annotations to modified route handlers
- Add [Unreleased] subsections to CHANGELOG.md
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
nanotaboada
force-pushed
the
feat/rest-polish-530
branch
from
cac3a42 to
ce0b687
Compare
- MD001: demote Phase headings from H3 to H2 (no heading level skip) - MD031: add blank line before fenced code block - MD040: add language specifier to unlabelled code fence - Step 1: require explicit confirmation before git pull Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
nanotaboada
force-pushed
the
feat/rest-polish-530
branch
from
28bb309 to
9d1fd90
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
GET /players/cache check:if not players→if players is Nonesoempty collections are cached correctly instead of re-fetching the DB on every
request
Locationheader toPOST /players/201 response pointing to/players/squadnumber/{squad_number}per RFC 7231 §7.1.2detailmessage toPOST /players/409 response[Unreleased]subsections (Added / Changed / Fixed / Removed) toCHANGELOG.mdCloses #530
Test plan
test_request_post_player_body_existing_response_body_detail— asserts409 detail message
test_request_post_player_body_nonexistent_response_header_location—asserts Location header on 201
flake8— no issuesblack --check— no changes neededpytest— 22/22 passed🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
Changed
Fixed
Tests
Documentation