feat: implement PATCH /players/{squadNumber} for partial updates#329
Open
the-Sunny-Sharma wants to merge 1 commit intonanotaboada:masterfrom
Open
feat: implement PATCH /players/{squadNumber} for partial updates#329the-Sunny-Sharma wants to merge 1 commit intonanotaboada:masterfrom
the-Sunny-Sharma wants to merge 1 commit intonanotaboada:masterfrom
Conversation
WalkthroughThis PR implements RFC 5789 PATCH support for partial player updates. A new ChangesPATCH Endpoint for Partial Player Updates
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Assessment against linked issues
Possibly related issues
🚥 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java`:
- Around line 250-254: In the patch method, add request validation and a
null-body guard: annotate the PlayerPatchDTO parameter with `@Valid` (e.g., public
ResponseEntity<Void> patch(`@PathVariable` Integer squadNumber, `@Valid`
`@RequestBody` PlayerPatchDTO playerPatchDTO)) and update the initial conditional
to check for null before dereferencing (e.g., if (playerPatchDTO == null ||
playerPatchDTO.getSquadNumber() != null || playerPatchDTO.getId() != null)
return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();). Ensure the
controller imports javax.validation.Valid (or jakarta.validation.Valid) so DTO
constraints on PlayerPatchDTO are enforced.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/models/PlayerPatchDTO.java`:
- Around line 35-45: PlayerPatchDTO lacks Bean Validation annotations—add the
same field-level JSR-380 constraints used in PlayerDTO but using constraints
that permit null (so patch fields remain optional): annotate squadNumber with
`@Min/`@Max, dateOfBirth with `@Past`, string fields (firstName, middleName,
lastName, position, abbrPosition, team, league) with `@Size`(max=...) and any
`@Pattern`(...) used in PlayerDTO for formats, and apply any ID/UUID format
constraint used in PlayerDTO to id; do not add `@NotNull/`@NotBlank (since fields
must remain nullable). Import javax.validation.constraints.* and mirror the
exact size/regex/min-max values from PlayerDTO when updating class
PlayerPatchDTO.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/services/PlayersService.java`:
- Around line 235-243: The method currently dereferences playerPatchDTO without
null-checking (see squadNumber check and the
playersRepository.findBySquadNumber(...).map(existing -> { ... }) block), so add
an early guard for playerPatchDTO == null at the start of the method (similar to
the squadNumber null check) to log a warning and return false; then proceed to
use playerPatchDTO safely in the mapping where existing.setFirstName(...),
existing.setMiddleName(...), etc. are called. Ensure the log uses the same
logger (log) and behavior as the squadNumber guard to keep semantics consistent.
🪄 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: 288c1c36-cc52-413f-a575-628f4f188d18
📒 Files selected for processing (3)
src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.javasrc/main/java/ar/com/nanotaboada/java/samples/spring/boot/models/PlayerPatchDTO.javasrc/main/java/ar/com/nanotaboada/java/samples/spring/boot/services/PlayersService.java
Comment on lines
+250
to
+254
| public ResponseEntity<Void> patch( | ||
| @PathVariable Integer squadNumber, | ||
| @RequestBody PlayerPatchDTO playerPatchDTO) { | ||
| if (playerPatchDTO.getSquadNumber() != null || playerPatchDTO.getId() != null) { | ||
| return ResponseEntity.status(HttpStatus.BAD_REQUEST).build(); |
There was a problem hiding this comment.
Prevent null-body NPE and enable validation on PATCH input.
On Line 253, playerPatchDTO is dereferenced directly. A null JSON body can cause a 500. Also, @Valid is missing, so DTO constraints are skipped.
Suggested fix
public ResponseEntity<Void> patch(
`@PathVariable` Integer squadNumber,
- `@RequestBody` PlayerPatchDTO playerPatchDTO) {
+ `@RequestBody` `@Valid` PlayerPatchDTO playerPatchDTO) {
+ if (playerPatchDTO == null) {
+ return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
+ }
if (playerPatchDTO.getSquadNumber() != null || playerPatchDTO.getId() != null) {
return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();
}As per coding guidelines, "Check that @Valid is used for request validation".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/controllers/PlayersController.java`
around lines 250 - 254, In the patch method, add request validation and a
null-body guard: annotate the PlayerPatchDTO parameter with `@Valid` (e.g., public
ResponseEntity<Void> patch(`@PathVariable` Integer squadNumber, `@Valid`
`@RequestBody` PlayerPatchDTO playerPatchDTO)) and update the initial conditional
to check for null before dereferencing (e.g., if (playerPatchDTO == null ||
playerPatchDTO.getSquadNumber() != null || playerPatchDTO.getId() != null)
return ResponseEntity.status(HttpStatus.BAD_REQUEST).build();). Ensure the
controller imports javax.validation.Valid (or jakarta.validation.Valid) so DTO
constraints on PlayerPatchDTO are enforced.
Comment on lines
+35
to
+45
| private UUID id; | ||
| private Integer squadNumber; | ||
| private String firstName; | ||
| private String middleName; | ||
| private String lastName; | ||
| private LocalDate dateOfBirth; | ||
| private String position; | ||
| private String abbrPosition; | ||
| private String team; | ||
| private String league; | ||
| private Boolean starting11; |
There was a problem hiding this comment.
Add Bean Validation constraints to PlayerPatchDTO fields.
The DTO currently accepts any provided values without JSR-380 validation, which can let invalid patches reach persistence logic. Keep fields nullable, but add the same field-level constraints used by PlayerDTO so validation triggers only when a field is present.
As per coding guidelines, "Use Bean Validation (JSR-380) for input validation in DTOs".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/models/PlayerPatchDTO.java`
around lines 35 - 45, PlayerPatchDTO lacks Bean Validation annotations—add the
same field-level JSR-380 constraints used in PlayerDTO but using constraints
that permit null (so patch fields remain optional): annotate squadNumber with
`@Min/`@Max, dateOfBirth with `@Past`, string fields (firstName, middleName,
lastName, position, abbrPosition, team, league) with `@Size`(max=...) and any
`@Pattern`(...) used in PlayerDTO for formats, and apply any ID/UUID format
constraint used in PlayerDTO to id; do not add `@NotNull/`@NotBlank (since fields
must remain nullable). Import javax.validation.constraints.* and mirror the
exact size/regex/min-max values from PlayerDTO when updating class
PlayerPatchDTO.
Comment on lines
+235
to
+243
| if (squadNumber == null) { | ||
| log.warn("Cannot patch player - squad number is null"); | ||
| return false; | ||
| } | ||
|
|
||
| return playersRepository.findBySquadNumber(squadNumber) | ||
| .map(existing -> { | ||
| if (playerPatchDTO.getFirstName() != null) existing.setFirstName(playerPatchDTO.getFirstName()); | ||
| if (playerPatchDTO.getMiddleName() != null) existing.setMiddleName(playerPatchDTO.getMiddleName()); |
There was a problem hiding this comment.
Guard playerPatchDTO against null before field access.
playerPatchDTO is dereferenced without a null check. If a null payload reaches this method, it will throw at runtime instead of returning a controlled result.
Suggested fix
public boolean patch(Integer squadNumber, PlayerPatchDTO playerPatchDTO) {
log.debug("Patching player with squad number: {}", squadNumber);
- if (squadNumber == null) {
- log.warn("Cannot patch player - squad number is null");
+ if (squadNumber == null || playerPatchDTO == null) {
+ log.warn("Cannot patch player - squad number or payload is null");
return false;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (squadNumber == null) { | |
| log.warn("Cannot patch player - squad number is null"); | |
| return false; | |
| } | |
| return playersRepository.findBySquadNumber(squadNumber) | |
| .map(existing -> { | |
| if (playerPatchDTO.getFirstName() != null) existing.setFirstName(playerPatchDTO.getFirstName()); | |
| if (playerPatchDTO.getMiddleName() != null) existing.setMiddleName(playerPatchDTO.getMiddleName()); | |
| if (squadNumber == null || playerPatchDTO == null) { | |
| log.warn("Cannot patch player - squad number or payload is null"); | |
| return false; | |
| } | |
| return playersRepository.findBySquadNumber(squadNumber) | |
| .map(existing -> { | |
| if (playerPatchDTO.getFirstName() != null) existing.setFirstName(playerPatchDTO.getFirstName()); | |
| if (playerPatchDTO.getMiddleName() != null) existing.setMiddleName(playerPatchDTO.getMiddleName()); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@src/main/java/ar/com/nanotaboada/java/samples/spring/boot/services/PlayersService.java`
around lines 235 - 243, The method currently dereferences playerPatchDTO without
null-checking (see squadNumber check and the
playersRepository.findBySquadNumber(...).map(existing -> { ... }) block), so add
an early guard for playerPatchDTO == null at the start of the method (similar to
the squadNumber null check) to log a warning and return false; then proceed to
use playerPatchDTO safely in the mapping where existing.setFirstName(...),
existing.setMiddleName(...), etc. are called. Ensure the log uses the same
logger (log) and behavior as the squadNumber guard to keep semantics consistent.
10 tasks
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.
Closes #318
What this PR does
Implements
PATCH /players/{squadNumber}for partial player updatesfollowing RFC 7396 (JSON Merge Patch) semantics.
Changes
PlayerPatchDTO— all fields nullable, annotated with@JsonInclude(NON_NULL), excludesidandsquadNumberfrom patchingpatch()inPlayersService— applies only non-null fieldsto the existing entity, follows existing Optional chain pattern
@PatchMapping("/players/{squadNumber}")inPlayersControllerwith full Swagger
@Operationand@ApiResponsesdocumentationAcceptance criteria
PATCH /players/{squadNumber}implemented204 No Contenton success400 Bad Requestif body containssquadNumberorid404 Not Foundwhen player not foundThis change is
Summary by CodeRabbit