Added create, update and delete methods for UserService. I created a …#14
Merged
Conversation
…separate updateUserAuthorization which requires the requestId id to be validated as a SysAdmin
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUserService gains email lookup plus transactional user-management APIs: Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserService
participant UserMapper
participant UserRepository
participant Database
Client->>UserService: createUser(CreateUserDTO)
UserService->>UserMapper: map DTO -> User
UserService->>UserRepository: save(user)
UserRepository->>Database: INSERT
Database-->>UserRepository: success / DataIntegrityViolation
UserRepository-->>UserService: savedUser / exception
UserService-->>Client: UserDTO / IllegalArgumentException
Client->>UserService: updateUser(UpdateUserDTO, currentAuth)
UserService->>UserRepository: findById(id)
UserRepository->>Database: SELECT
Database-->>UserRepository: user / null
UserRepository-->>UserService: user / not found
UserService->>UserMapper: updateEntityFromDTO(dto, user)
UserService->>UserRepository: save(updatedUser)
UserRepository->>Database: UPDATE
Database-->>UserRepository: success
UserRepository-->>UserService: updatedUser
UserService-->>Client: UserDTO
Client->>UserService: updateUserAuthorization(userId, newAuth, requesterId)
UserService->>UserRepository: findById(requesterId) (validateSysAdmin)
UserRepository-->>UserService: requester
UserService->>UserRepository: findById(userId)
UserRepository-->>UserService: targetUser
UserService->>UserRepository: save(user with newAuth)
UserRepository->>Database: UPDATE
Database-->>UserRepository: success
UserRepository-->>UserService: updatedUser
UserService-->>Client: UserDTO
Client->>UserService: deleteUser(userId, requesterId)
UserService->>UserRepository: findById(requesterId) (validateSysAdmin)
UserService->>UserRepository: findById(userId)
UserRepository-->>UserService: targetUser
UserService->>UserRepository: delete(targetUser)
UserRepository->>Database: DELETE
Database-->>UserRepository: success
UserService-->>Client: void / UnauthorizedException
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
![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
🧹 Nitpick comments (2)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (2)
70-70:UpdateUserDTO.userAuthorizationfield is silently ignored.The
UpdateUserDTOincludes auserAuthorizationfield (per context snippet 4), butUserMapper.updateEntityFromDTOonly updatesfullNameandConsider either removing the field from
UpdateUserDTOor documenting that authorization changes requireupdateUserAuthorization().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` at line 70, UpdateUserDTO.userAuthorization is declared but never applied because UserMapper.updateEntityFromDTO only copies fullName and email; either remove userAuthorization from UpdateUserDTO or explicitly handle it: update UserMapper.updateEntityFromDTO to map dto.getUserAuthorization() onto the User entity (or call a helper to validate/apply roles), and if authorization changes must go through a separate flow keep the field removed and document that updateUserAuthorization() is the supported API; reference UpdateUserDTO.userAuthorization, UserMapper.updateEntityFromDTO, and updateUserAuthorization() when making the change.
94-102: Return value is unused; consider returningvoid.
validateSysAdminreturnsUser, but the caller at line 77 discards the result. Either change the return type tovoidor use the returned requester in the calling method.Also, minor inconsistency: error message has a trailing period ("User not found.") while other
EntityNotFoundExceptionmessages do not (lines 62, 80, 90).♻️ Proposed simplification
- private User validateSysAdmin(Long requesterId) { - User requester = userRepository.findById(requesterId) - .orElseThrow(() -> new EntityNotFoundException("User not found.")); + private void validateSysAdmin(Long requesterId) { + User requester = userRepository.findById(requesterId) + .orElseThrow(() -> new EntityNotFoundException("User not found")); if (requester.getUserAuthorization() != UserAuthorization.SYSADMIN) { throw new UnauthorizedException("User is not authorized to perform this action."); } - return requester; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 94 - 102, The validateSysAdmin method currently returns a User but callers discard it; change its signature to return void and keep its internal checks (use userRepository.findById(...).orElseThrow(new EntityNotFoundException("User not found")) -> remove the trailing period to match other messages) and throw UnauthorizedException when requester.getUserAuthorization() != UserAuthorization.SYSADMIN; update any callers that relied on the returned User to not expect a value or to fetch the User separately if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 46-56: The createUser method currently does a check-then-save
(userRepository.findByEmail(...) then userRepository.save(...)) which is
vulnerable to a race causing a DataIntegrityViolationException; modify
createUser to still attempt the save but catch
org.springframework.dao.DataIntegrityViolationException (thrown by save) and
translate it into the same domain IllegalArgumentException("A user with this
email already exists") so concurrent inserts produce a consistent, user-friendly
exception; keep the pre-check if you want fast-fail but ensure the catch around
userRepository.save(...) in createUser handles DB unique-constraint failures and
returns the mapped DTO as before.
- Line 53: The code currently sets the password in UserService via
user.setPassword(dto.password()) which stores plaintext; add a PasswordEncoder
bean (e.g., BCryptPasswordEncoder) to your Spring configuration, inject that
PasswordEncoder into UserService, and replace the direct assignment with
encoding the DTO password before setting (use
passwordEncoder.encode(dto.password()) in the place of
user.setPassword(dto.password())); ensure the bean is named/available for
injection and remove any placeholder plaintext storage comments in the User
entity.
- Around line 87-92: Add an authorization check to deleteUser by adding a
requesterId parameter and calling validateSysAdmin(requesterId) at the start of
the method (same pattern used in updateUserAuthorization). Specifically, modify
the deleteUser(Long id) signature to accept Long requesterId, invoke
validateSysAdmin(requesterId) before loading the User, and then proceed to find
and delete the user via userRepository as before; keep the
EntityNotFoundException behavior unchanged.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Line 70: UpdateUserDTO.userAuthorization is declared but never applied because
UserMapper.updateEntityFromDTO only copies fullName and email; either remove
userAuthorization from UpdateUserDTO or explicitly handle it: update
UserMapper.updateEntityFromDTO to map dto.getUserAuthorization() onto the User
entity (or call a helper to validate/apply roles), and if authorization changes
must go through a separate flow keep the field removed and document that
updateUserAuthorization() is the supported API; reference
UpdateUserDTO.userAuthorization, UserMapper.updateEntityFromDTO, and
updateUserAuthorization() when making the change.
- Around line 94-102: The validateSysAdmin method currently returns a User but
callers discard it; change its signature to return void and keep its internal
checks (use userRepository.findById(...).orElseThrow(new
EntityNotFoundException("User not found")) -> remove the trailing period to
match other messages) and throw UnauthorizedException when
requester.getUserAuthorization() != UserAuthorization.SYSADMIN; update any
callers that relied on the returned User to not expect a value or to fetch the
User separately if needed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6bfda340-ef40-4fae-bd70-266c0b78197a
📒 Files selected for processing (1)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 87-93: The deleteUser method references an undefined requesterId;
update the method signature for deleteUser to accept a Long requesterId
parameter, e.g., change public void deleteUser(Long id) to public void
deleteUser(Long id, Long requesterId), then propagate that parameter to any
callers; ensure the validateSysAdmin(requesterId) call and subsequent logic in
UserService.deleteUser continue to work with the added parameter and update any
unit tests or call sites that invoke deleteUser accordingly.
- Around line 1-16: The code fails to compile because UserService references
DataIntegrityViolationException but does not import it; add the missing import
for org.springframework.dao.DataIntegrityViolationException at the top of the
UserService class so references inside methods like createUser (or wherever
DataIntegrityViolationException is used) resolve correctly and the class
compiles.
- Around line 58-73: The updateUser method has the same TOCTOU race as
createUser: after checking userRepository.findByEmail(...) you must protect the
subsequent userRepository.save(user) against concurrent unique-email violations;
wrap the save call in a try-catch (as done in createUser) catching the
persistence exception (e.g., DataIntegrityViolationException or
ConstraintViolationException) and translate it into the same
IllegalArgumentException (or other handled domain error) stating "A user with
this email already exists"; keep the existing pre-check using
userRepository.findByEmail(...) but ensure the try-catch around
userMapper.updateEntityFromDTO(dto, user) + userRepository.save(user) in
updateUser to handle any race-condition uniqueness constraint failures.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c4dd1081-6c04-45b7-b311-35c11773ceb9
📒 Files selected for processing (1)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (1)
66-74:⚠️ Potential issue | 🟠 MajorStill missing the duplicate-email catch around
save().The pre-check at Lines 66-70 can still lose a race to another transaction, and Line 73 will then leak a raw
DataIntegrityViolationExceptioninstead of the same user-friendly error used increateUser().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 66 - 74, The pre-check using userRepository.findByEmail(...) still has a race; wrap the call to userRepository.save(user) in updateUser (the block that calls userMapper.updateEntityFromDTO(dto, user) then userRepository.save(user)) with a try/catch that catches DataIntegrityViolationException (or the specific persistence exception your stack throws) and rethrow an IllegalArgumentException with the same friendly message used in createUser (e.g., "A user with this email already exists") so duplicate-email races produce a consistent user-facing error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 61-74: The updateUser flow currently ignores
UpdateUserDTO.userAuthorization(); in UserService.updateUser after loading the
User, validate that dto.userAuthorization() is null/absent and reject requests
that attempt to change authorization (throw e.g. IllegalArgumentException) OR
remove the field from UpdateUserDTO if contract change is acceptable;
alternatively ensure role changes go only through the existing
updateUserAuthorization(...) method by adding an explicit check before calling
userMapper.updateEntityFromDTO(dto, user) that prevents persisting
dto.userAuthorization() (reject or strip it) and document the behavior.
- Around line 49-53: The createUser(CreateUserDTO dto) method currently trusts
dto.userAuthorization() and allows callers to create SYSADMIN accounts; change
it so createUser no longer assigns elevated roles from the DTO: either set a
safe default role (e.g., Role.USER) inside createUser (call
user.setAuthorization(...) instead of using dto.userAuthorization()), or require
an explicit validated admin requester parameter and gate assignment of elevated
roles to that path; keep elevated-role changes restricted to the existing
updateUserAuthorization(...) flow. Ensure the unique symbols to edit are
createUser(CreateUserDTO), CreateUserDTO.userAuthorization(), and
updateUserAuthorization(...) so the role is not accepted directly from the
incoming DTO.
---
Duplicate comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 66-74: The pre-check using userRepository.findByEmail(...) still
has a race; wrap the call to userRepository.save(user) in updateUser (the block
that calls userMapper.updateEntityFromDTO(dto, user) then
userRepository.save(user)) with a try/catch that catches
DataIntegrityViolationException (or the specific persistence exception your
stack throws) and rethrow an IllegalArgumentException with the same friendly
message used in createUser (e.g., "A user with this email already exists") so
duplicate-email races produce a consistent user-facing error.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8e5febc4-8da2-42e6-a163-b225c5f6c935
📒 Files selected for processing (1)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
…out old email check logic in createUser. The current userAuthorization is now a parameter in updateUser. Fix: Changed CI pipeline so pushes in all branches run tests.
…push to prevent doubling test and build.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java (1)
49-55:⚠️ Potential issue | 🔴 CriticalSet the default role before
save(...).Line 54 is too late. With
User.idusingGenerationType.IDENTITY, the insert can happen at Line 53 to fetch the generated key, so the first write still uses whateveruserMapper.toEntity(dto)put inuserAuthorization— ornull, which violates the entity's@NotNull. DefaultUSERon the transient entity before persisting it.Proposed fix
`@Transactional` public UserDTO createUser(CreateUserDTO dto) { User user = userMapper.toEntity(dto); user.setPassword(dto.password()); // mapper doesn't set this currently + user.setUserAuthorization(UserAuthorization.USER); try { User savedUser = userRepository.save(user); - savedUser.setUserAuthorization(UserAuthorization.USER); return userMapper.toDTO(savedUser); } catch (DataIntegrityViolationException e) { throw new IllegalArgumentException("A user with this email already exists", e); } }In Spring Data JPA / Hibernate, when an entity uses `@GeneratedValue(strategy = GenerationType.IDENTITY)`, can `save()` / `persist()` execute the INSERT before transaction commit to obtain the generated id, so field changes made after `save()` happen after the initial INSERT?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java` around lines 49 - 55, The user default role is being set after persisting, risking a null or wrong userAuthorization in the INSERT; in createUser (UserService.createUser) set the default role on the transient User object returned by userMapper.toEntity(dto) before calling userRepository.save(user) — e.g., call user.setUserAuthorization(UserAuthorization.USER) immediately after userMapper.toEntity(dto) and before save so the persisted row always has the USER role and avoids `@NotNull` violations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 62-77: The updateUser method currently overwrites the persisted
role using the caller-provided currentAuth
(savedUser.setUserAuthorization(currentAuth)), which allows callers to escalate
roles; instead, keep the existing role from the loaded User entity (the user
returned by userRepository.findById) when saving (do not use currentAuth or
accept role changes in updateUser), so remove or replace the
savedUser.setUserAuthorization(currentAuth) call to preserve
user.getUserAuthorization(); reserve all role changes for
updateUserAuthorization(...) which enforces validateSysAdmin(...).
- Around line 73-79: The try/catch around userMapper.updateEntityFromDTO(dto,
user) and userRepository.save(user) may miss unique-email violations because JPA
defers SQL until flush/commit; replace the call to userRepository.save(user)
with userRepository.saveAndFlush(user) (or explicitly call flush on the
EntityManager) inside the try so the DataIntegrityViolationException is raised
there and can be caught and rethrown as the IllegalArgumentException; keep the
same catch for DataIntegrityViolationException and wrapping behavior.
---
Duplicate comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/service/UserService.java`:
- Around line 49-55: The user default role is being set after persisting,
risking a null or wrong userAuthorization in the INSERT; in createUser
(UserService.createUser) set the default role on the transient User object
returned by userMapper.toEntity(dto) before calling userRepository.save(user) —
e.g., call user.setUserAuthorization(UserAuthorization.USER) immediately after
userMapper.toEntity(dto) and before save so the persisted row always has the
USER role and avoids `@NotNull` violations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fb7d8f6f-797f-49d4-8672-eb84694ecb26
📒 Files selected for processing (2)
.github/workflows/build-and-test.ymlsrc/main/java/org/example/visacasemanagementsystem/user/service/UserService.java
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/build-and-test.yml
… by updateUserAuthorization method only. Implemented CodeRabbit about duplicate email interception.
Merged
This was referenced Apr 21, 2026
Add comprehensive unit and integration tests for UserViewController, UserMapper, and UserService
#37
Merged
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.
Added create, update and delete methods for UserService.
Created a separate updateUserAuthorization which requires the requestId id to be validated as a SysAdmin
Summary by CodeRabbit
New Features
Bug Fixes
Chores