feat(api): return 422 Unprocessable Entity for validation errors (#319)#323
feat(api): return 422 Unprocessable Entity for validation errors (#319)#323nanotaboada wants to merge 2 commits intomasterfrom
Conversation
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
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 56 minutes and 23 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 (1)
WalkthroughA new global exception handler intercepts validation failures ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
🚥 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 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 #323 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 30 32 +2
===========================================
Files 2 3 +1
Lines 88 90 +2
Branches 8 8
===========================================
+ Hits 88 90 +2 ☔ View full report in Codecov by Sentry. 🚀 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 (3)
src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java (1)
207-215: Consider delegating squad-mismatch to bean validation for consistency.The squad-number-mismatch branch at line 214 now returns 422 directly from the controller, which is correct per the issue spec. However, this inlined check is conceptually a validation concern that bypasses the
GlobalExceptionHandlerpath taken by every other 422. As a future improvement, you could expose the mismatch as aMethodArgumentNotValidException(e.g., via a cross-field validator or by throwing a custom exception mapped inGlobalExceptionHandler) so all 422 responses share a single shape once a problem-details body is added. Not blocking — current behavior matches the PR objective.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java` around lines 207 - 215, The controller currently checks squad-number mismatch inline in PlayersController.put and returns a 422 directly; instead delegate this to validation so 422s flow through GlobalExceptionHandler: implement a cross-field constraint on PlayerDTO that validates playerDTO.getSquadNumber() equals the `@PathVariable` squadNumber (or introduce a custom exception type mapped in GlobalExceptionHandler and throw it from put), then remove the inline equality check from PlayersController.put so validation failures produce MethodArgumentNotValidException (or your mapped custom exception) and are handled consistently by GlobalExceptionHandler.src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java (1)
442-513: Add a test that locks in the 400 vs 422 distinction.The CHANGELOG and issue
#319explicitly reserve400 Bad Requestfor malformed requests (unparseable JSON, wrongContent-Type). There is no test asserting that contract, so a future change to the exception handler could silently collapse both paths to the same status and go unnoticed. Consider adding a POST (and optionally PUT) test with a malformed JSON body that assertsHttpStatus.BAD_REQUEST.💚 Example test
`@Test` void givenMalformedJson_whenPost_thenReturnsBadRequest() throws Exception { MockHttpServletRequestBuilder request = MockMvcRequestBuilders .post(PATH) .content("{ not-valid-json ") .contentType(MediaType.APPLICATION_JSON); MockHttpServletResponse response = application .perform(request) .andReturn() .getResponse(); verify(playersServiceMock, never()).create(any(PlayerDTO.class)); then(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java` around lines 442 - 513, Add a test in PlayersControllerTests that submits malformed JSON (e.g. ".content("{ not-valid-json ")") to the POST endpoint (use MockMvcRequestBuilders.post(PATH) with contentType(MediaType.APPLICATION_JSON)), perform the request via application.perform(...).andReturn().getResponse(), verify playersServiceMock.create(...) is never called, and assert response.getStatus() == HttpStatus.BAD_REQUEST.value() (optionally add an equivalent PUT test using playersServiceMock.update(...) and path param). Ensure the new test method has a clear name like givenMalformedJson_whenPost_thenReturnsBadRequest.src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/GlobalExceptionHandler.java (1)
12-15: Consider returning a problem-details body so clients can see which fields failed.The handler currently discards the
MethodArgumentNotValidExceptionand returns an empty 422. Clients get the status code but no indication of which field(s) violated which constraint, which defeats much of the value of switching from 400 to 422. Consider returning a small body (field → message map, or RFC 7807ProblemDetail) and logging the violations via SLF4J for observability.♻️ Example refactor using Spring's
ProblemDetail- `@ExceptionHandler`(MethodArgumentNotValidException.class) - public ResponseEntity<Void> handleValidationException(MethodArgumentNotValidException exception) { - return ResponseEntity.status(HttpStatus.UNPROCESSABLE_ENTITY).build(); - } + `@ExceptionHandler`(MethodArgumentNotValidException.class) + public ResponseEntity<ProblemDetail> handleValidationException(MethodArgumentNotValidException exception) { + Map<String, String> errors = exception.getBindingResult().getFieldErrors().stream() + .collect(Collectors.toMap( + FieldError::getField, + fe -> Optional.ofNullable(fe.getDefaultMessage()).orElse("invalid"), + (a, b) -> a)); + ProblemDetail problem = ProblemDetail.forStatus(HttpStatus.UNPROCESSABLE_ENTITY); + problem.setTitle("Validation failed"); + problem.setProperty("errors", errors); + return ResponseEntity.status(HttpStatus.UNPROCESSABLE_ENTITY).body(problem); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/GlobalExceptionHandler.java` around lines 12 - 15, The handler handleValidationException currently returns an empty 422; change it to build and return a ProblemDetail (or a simple field→message map) containing the validation violations extracted from exception.getBindingResult() (e.g., iterate field errors and collect field names to messages) and return it in the ResponseEntity with status UNPROCESSABLE_ENTITY; also log the extracted violations via the GlobalExceptionHandler's SLF4J logger for observability. Ensure you reference MethodArgumentNotValidException, handleValidationException, and ProblemDetail (or a Map<String,String>) when locating where to implement these changes.
🤖 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/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/GlobalExceptionHandler.java`:
- Around line 12-15: The handler handleValidationException currently returns an
empty 422; change it to build and return a ProblemDetail (or a simple
field→message map) containing the validation violations extracted from
exception.getBindingResult() (e.g., iterate field errors and collect field names
to messages) and return it in the ResponseEntity with status
UNPROCESSABLE_ENTITY; also log the extracted violations via the
GlobalExceptionHandler's SLF4J logger for observability. Ensure you reference
MethodArgumentNotValidException, handleValidationException, and ProblemDetail
(or a Map<String,String>) when locating where to implement these changes.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java`:
- Around line 207-215: The controller currently checks squad-number mismatch
inline in PlayersController.put and returns a 422 directly; instead delegate
this to validation so 422s flow through GlobalExceptionHandler: implement a
cross-field constraint on PlayerDTO that validates playerDTO.getSquadNumber()
equals the `@PathVariable` squadNumber (or introduce a custom exception type
mapped in GlobalExceptionHandler and throw it from put), then remove the inline
equality check from PlayersController.put so validation failures produce
MethodArgumentNotValidException (or your mapped custom exception) and are
handled consistently by GlobalExceptionHandler.
In
`@src/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java`:
- Around line 442-513: Add a test in PlayersControllerTests that submits
malformed JSON (e.g. ".content("{ not-valid-json ")") to the POST endpoint (use
MockMvcRequestBuilders.post(PATH) with contentType(MediaType.APPLICATION_JSON)),
perform the request via application.perform(...).andReturn().getResponse(),
verify playersServiceMock.create(...) is never called, and assert
response.getStatus() == HttpStatus.BAD_REQUEST.value() (optionally add an
equivalent PUT test using playersServiceMock.update(...) and path param). Ensure
the new test method has a clear name like
givenMalformedJson_whenPost_thenReturnsBadRequest.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f3cd6cbc-f705-4769-83fc-8afccc9c4bb5
📒 Files selected for processing (4)
CHANGELOG.mdsrc/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/GlobalExceptionHandler.javasrc/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.javasrc/test/java/ar/com/nanotaboada/java/samples/spring/boot/test/controllers/PlayersControllerTests.java
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
|
@coderabbitai Two findings from the review were deferred as out of scope for #319:
Could you please create a new GitHub issue to track these two improvements? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 66
👉 #324 Here's a summary of what was captured:
The issue has been assigned to you and includes backlinks to PR 🧠 Learnings used |
1 participant
Summary
GlobalExceptionHandler(@RestControllerAdvice) that interceptsMethodArgumentNotValidExceptionand returns422 Unprocessable Entity, while leaving Spring's default handling intact so malformed requests (HttpMessageNotReadableException) continue to return400 Bad RequestHttpStatus.BAD_REQUESTwithHttpStatus.UNPROCESSABLE_ENTITYin the PUT endpoint's squad number mismatch check@ApiResponseannotations on POST and PUT to declare422instead of400for validation failuresPlayersControllerTeststo assert422and reflect the new expected status in their namesTest plan
./mvnw clean install— BUILD SUCCESS, 40 tests pass, all JaCoCo coverage checks metdocker compose build— image built successfully@RestControllerAdviceannotation), 1 rejected (squad mismatch → intentional422per issue spec)Closes #319
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit