feat(routes): add payload validation, return 422 on failure (#97, #94)#98
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughField-level validation was added to PlayerRequest with the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Route as Route (create/update player)
participant Validator as Validator (validator::Validate)
participant Service as Service/DB
Client->>Route: POST/PUT /players with JSON payload
Route->>Validator: payload.validate()
alt validation succeeds
Route->>Service: call create/update with validated payload
Service-->>Route: success response
Route-->>Client: 200/201
else validation fails
Validator-->>Route: Validation errors
Route-->>Client: 422 Unprocessable Entity
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 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 #98 +/- ##
==========================================
+ Coverage 84.33% 84.88% +0.54%
==========================================
Files 6 6
Lines 166 172 +6
==========================================
+ Hits 140 146 +6
Misses 26 26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Owner
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/routes/players.rs (1)
109-112: Optional: centralize duplicated payload validation block.
create_playerandupdate_playernow share identical validation logic. A small helper can keep behavior aligned and reduce drift risk.♻️ Suggested DRY refactor
+fn validate_payload(payload: &PlayerRequest) -> Result<(), Status> { + if payload.validate().is_err() { + return Err(Status::UnprocessableEntity); + } + Ok(()) +} + fn create_player( player_request: Json<PlayerRequest>, players: &State<PlayerCollection>, ) -> Result<(Status, Json<PlayerResponse>), Status> { let payload = player_request.into_inner(); - if payload.validate().is_err() { - return Err(Status::UnprocessableEntity); - } + validate_payload(&payload)?; let mut connection = players.get().map_err(|_| Status::InternalServerError)?; match player_service::create(&mut connection, payload) { @@ fn update_player( @@ ) -> Result<Status, Status> { let payload = player_request.into_inner(); - if payload.validate().is_err() { - return Err(Status::UnprocessableEntity); - } + validate_payload(&payload)?; let mut connection = players.get().map_err(|_| Status::InternalServerError)?;Also applies to: 147-150
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/routes/players.rs` around lines 109 - 112, Both create_player and update_player contain identical payload validation (payload.validate().is_err() -> Err(Status::UnprocessableEntity)); extract this into a small helper (e.g., fn validate_player_payload(req: &PlayerRequest) -> Result<(), Status> or fn validate_payload<T: Validate>(payload: &T) -> Result<(), Status>) and call it from create_player and update_player instead of duplicating the block; ensure the helper calls payload.validate() and maps failures to Status::UnprocessableEntity so behavior remains identical.tests/player_routes_tests.rs (1)
358-369: Use shared JSON fixtures and mutate fields instead of ad-hoc payload literals.These six tests duplicate full request bodies inline. Please start from
player_request_for_creation_json()/player_request_for_update_json()and override only the invalid field under test.As per coding guidelines: "Use `player_request_for_creation_json()` and `player_request_for_update_json()` fixtures — no ad-hoc JSON payloads".♻️ Suggested fixture-based pattern
+fn invalid_creation_payload(mutator: impl FnOnce(&mut serde_json::Value)) -> serde_json::Value { + let mut body = player_request_for_creation_json(); + mutator(&mut body); + body +} + +fn invalid_update_payload(mutator: impl FnOnce(&mut serde_json::Value)) -> serde_json::Value { + let mut body = player_request_for_update_json(); + mutator(&mut body); + body +} @@ - let body = serde_json::json!({ - "firstName": "", - ... - }); + let body = invalid_creation_payload(|b| b["firstName"] = serde_json::json!("")); @@ - let body = serde_json::json!({ - ... - "squadNumber": 0, - ... - }); + let body = invalid_creation_payload(|b| b["squadNumber"] = serde_json::json!(0)); @@ - let body = serde_json::json!({ - ... - "lastName": "", - ... - }); + let body = invalid_update_payload(|b| b["lastName"] = serde_json::json!(""));Also applies to: 385-396, 413-424, 442-453, 470-481, 498-509
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/player_routes_tests.rs` around lines 358 - 369, The tests inline duplicate full player JSON payloads; replace each ad-hoc body with a clone of the shared fixture (use player_request_for_creation_json() for create tests and player_request_for_update_json() for update tests), then mutate only the specific field under test (e.g., set "firstName" to empty) before serializing/sending; locate the current local `body` assignment in the tests and change it to start from the fixture, modify the target key, and reuse the rest of the payload unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/routes/players.rs`:
- Around line 109-112: Both create_player and update_player contain identical
payload validation (payload.validate().is_err() ->
Err(Status::UnprocessableEntity)); extract this into a small helper (e.g., fn
validate_player_payload(req: &PlayerRequest) -> Result<(), Status> or fn
validate_payload<T: Validate>(payload: &T) -> Result<(), Status>) and call it
from create_player and update_player instead of duplicating the block; ensure
the helper calls payload.validate() and maps failures to
Status::UnprocessableEntity so behavior remains identical.
In `@tests/player_routes_tests.rs`:
- Around line 358-369: The tests inline duplicate full player JSON payloads;
replace each ad-hoc body with a clone of the shared fixture (use
player_request_for_creation_json() for create tests and
player_request_for_update_json() for update tests), then mutate only the
specific field under test (e.g., set "firstName" to empty) before
serializing/sending; locate the current local `body` assignment in the tests and
change it to start from the fixture, modify the target key, and reuse the rest
of the payload unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 11cbb70b-b09d-4a8b-a8b2-7ac4a86bfad7
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
CHANGELOG.mdCargo.tomlsrc/models/player.rssrc/routes/players.rstests/player_routes_tests.rs
…ixtures Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
PlayerRequestusing thevalidatorcrate (#[derive(Validate)])first_name,last_name,date_of_birth,position,abbr_position,team,league) must be non-empty;middle_nameis intentionally left unvalidated (optional)squad_numbermust be in range 1–99.validate()before the service layer, returning422 Unprocessable Entityon failure;400 Bad Requestremains reserved for malformed JSONTest plan
test_request_post_players_body_empty_first_name_response_status_unprocessable_entitytest_request_post_players_body_squad_number_zero_response_status_unprocessable_entitytest_request_post_players_body_squad_number_above_maximum_response_status_unprocessable_entitytest_request_put_player_squadnumber_body_empty_last_name_response_status_unprocessable_entitytest_request_put_player_squadnumber_body_squad_number_zero_response_status_unprocessable_entitytest_request_put_player_squadnumber_body_squad_number_above_maximum_response_status_unprocessable_entitycargo clippyclean,docker compose buildsuccess, CodeRabbit: no findingsCloses #97
Closes #94
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests