DEMO-1 implement REST API endpoint for updating users #9
Conversation
* main: fail build if IT tests fail
* main: small code cleanup
include code coverage / jacoco information * main: code coverage info on PRs add jacoco with unit and integration test analysis Update README.md Update README.md Update README.md Update README.md
…spring into dummy_feature * 'dummy_feature' of https://github.com/bitcoder/tutorial-spring: ST-2 add bug on purpose (again)
* main: improved code coverage code cleanup avoid dup code bump deps make IndexController render a proper HTML page, LH compliant add environments info fix JDK in Dockerfile Update Twitter link to X in README Correct formatting in README.md Update README.md deploy workflow to render.com fix running using Dockerfile fix building image simple Dockerfile; boots springboot remove unnecessary GH actions temporary PR info add a step to find if we are on a PR run on PR update update workflow to properly run integration tests with jacoco
|
|
1 similar comment
|
|
Test Plan: ST-3 |
There was a problem hiding this comment.
Pull request overview
This pull request implements a REST API PUT endpoint for updating user information, along with improvements to unit tests and CI workflow enhancements. The changes enable clients to modify existing user records via the /api/users/{id} endpoint.
Key Changes:
- Added
updateUserPUT endpoint in UserRestController that allows updating user name, username, and password - Improved test naming and added repository tests for username lookups
- Enhanced GitHub Actions workflow with pull request comment functionality for test plan links
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/main/java/com/sergiofreire/xray/tutorials/springboot/boundary/UserRestController.java | Implements new PUT endpoint for updating user records by ID with error handling for non-existent users |
| src/test/java/com/sergiofreire/xray/tutorials/springboot/UserServiceUnitTest.java | Refactored test method names for better clarity and added duplicate mock setup |
| src/test/java/com/sergiofreire/xray/tutorials/springboot/UserRepositoryTest.java | Added new tests for username lookup functionality (valid and invalid cases) |
| .github/workflows/maven.yml | Added permissions for PR comments and new steps to link test plans in build summaries and PR comments |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| Mockito.when(userRepository.findById(john.getId())).thenReturn(Optional.of(john)); | ||
| Mockito.when(userRepository.findByUsername(john.getUsername())).thenReturn(john); | ||
| Mockito.when(userRepository.findByUsername(john.getUsername())).thenReturn(john); |
There was a problem hiding this comment.
Duplicate mock setup detected. Line 48 already configures the mock for userRepository.findByUsername(john.getUsername()), so this line is redundant and should be removed.
| Mockito.when(userRepository.findByUsername(john.getUsername())).thenReturn(john); |
| @PutMapping("/users/{id}") | ||
| public ResponseEntity<User> updateUser(@PathVariable(value = "id") Long id, @RequestBody User userDetails) | ||
| throws ResourceNotFoundException { | ||
| User user = userService.getUserDetails(id) | ||
| .orElseThrow(() -> new ResourceNotFoundException("User not found for id: " + id)); | ||
| user.setName(userDetails.getName()); | ||
| user.setUsername(userDetails.getUsername()); | ||
| user.setPassword(userDetails.getPassword()); | ||
| final User updatedUser = userService.save(user); | ||
| return ResponseEntity.ok(updatedUser); | ||
| } |
There was a problem hiding this comment.
The new updateUser endpoint lacks test coverage. While there are integration tests for other endpoints (create, get, list, delete) in UserRestControllerIT.java, there is no test for the PUT endpoint to verify that user updates work correctly or handle error cases (e.g., updating a non-existent user).
| } | ||
|
|
||
| @PutMapping("/users/{id}") | ||
| public ResponseEntity<User> updateUser(@PathVariable(value = "id") Long id, @RequestBody User userDetails) |
There was a problem hiding this comment.
Missing input validation. The updateUser method accepts userDetails without validating it (e.g., using @Valid annotation). This could allow invalid data (e.g., empty username, short password) to be saved, bypassing the validation constraints defined in the User entity. Consider adding @Valid before @RequestBody User userDetails to ensure validation is enforced.
| .orElseThrow(() -> new ResourceNotFoundException("User not found for id: " + id)); | ||
| user.setName(userDetails.getName()); | ||
| user.setUsername(userDetails.getUsername()); | ||
| user.setPassword(userDetails.getPassword()); |
There was a problem hiding this comment.
Storing passwords in plain text is a security vulnerability. The update endpoint allows password changes without any hashing or encryption. Passwords should be hashed using a secure algorithm (e.g., bcrypt) before being stored in the database. This applies to line 47 where user.setPassword(userDetails.getPassword()) directly sets the plain text password.
| @PutMapping("/users/{id}") | ||
| public ResponseEntity<User> updateUser(@PathVariable(value = "id") Long id, @RequestBody User userDetails) | ||
| throws ResourceNotFoundException { | ||
| User user = userService.getUserDetails(id) | ||
| .orElseThrow(() -> new ResourceNotFoundException("User not found for id: " + id)); | ||
| user.setName(userDetails.getName()); | ||
| user.setUsername(userDetails.getUsername()); | ||
| user.setPassword(userDetails.getPassword()); | ||
| final User updatedUser = userService.save(user); | ||
| return ResponseEntity.ok(updatedUser); | ||
| } |
There was a problem hiding this comment.
Missing authorization check. The updateUser endpoint allows any user to update any other user's information without verifying that the requester has the necessary permissions or is updating their own account. Consider adding authentication/authorization checks to ensure users can only update their own information or that administrators have proper privileges.
add a new REST API endpoint using PUT to a update a given user