Open
Conversation
…d MedicalRecordController incompatible type
…serserivce to remind to secure the password when implement spring-security
|
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:
📝 WalkthroughWalkthroughAdds a new REST Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UserController
participant UserService
participant UserRepository
participant ClinicRepository
Client->>UserController: POST /api/users (UserRequest)
UserController->>UserService: createUser(request)
UserService->>UserRepository: existsByEmail(email)
alt email not exists
UserService->>ClinicRepository: findById(clinicId) (if role == VET)
ClinicRepository-->>UserService: Clinic
UserService->>UserRepository: save(User entity)
UserRepository-->>UserService: persisted User
UserService-->>UserController: UserResponse
UserController-->>Client: 201 Created (UserResponse)
else email exists
UserService-->>UserController: throw BusinessRuleException
UserController-->>Client: 400/409 Error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 2
🧹 Nitpick comments (5)
src/main/java/org/example/vet1177/services/UserService.java (1)
109-120: Clinic assignment for non-VET users silently clears any provided clinicId.When a non-VET user is created/updated with a
clinicId, it's silently set tonull(Line 118). This could be confusing for API consumers. Consider throwing an error ifclinicIdis provided for non-VET roles, or document this behavior.💡 Alternative: Reject clinicId for non-VET roles
private void applyClinicRules(User user, UserRequest request) { if (request.getRole() == Role.VET) { if (request.getClinicId() == null) { throw new IllegalArgumentException("Veterinär måste vara kopplad till en klinik"); } Clinic clinic = clinicService.getById(request.getClinicId()); user.setClinic(clinic); } else { + if (request.getClinicId() != null) { + throw new IllegalArgumentException("Endast veterinärer kan kopplas till en klinik"); + } user.setClinic(null); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/UserService.java` around lines 109 - 120, The applyClinicRules method currently clears any clinicId for non-VET roles; change it to reject unexpected clinicId input by validating in applyClinicRules(User user, UserRequest request): if request.getRole() != Role.VET and request.getClinicId() != null then throw an IllegalArgumentException (or a domain-specific exception) with a clear message indicating clinicId is only allowed for VET role; keep existing behavior for Role.VET (validate clinicId non-null, fetch via clinicService.getById and set on user) and remove the silent user.setClinic(null) branch.src/main/java/org/example/vet1177/dto/request/user/UserRequest.java (1)
19-20: Add password strength validation.The password field only validates non-blank but has no minimum length or complexity requirements. Consider adding
@Size(min = 8)or a custom pattern validator for stronger security.🔒 Proposed fix to add password validation
+import jakarta.validation.constraints.Size; + `@NotBlank` +@Size(min = 8, message = "Lösenord måste vara minst 8 tecken") private String password;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/dto/request/user/UserRequest.java` around lines 19 - 20, The password field in UserRequest currently only has `@NotBlank`; add a minimum-length and/or complexity constraint (e.g., annotate the private String password field in class UserRequest with `@Size`(min = 8) and optionally `@Pattern`(...) for complexity) so passwords meet strength requirements, and update imports for javax.validation.constraints.Size and/or Pattern and provide a clear validation message string for the annotations.src/main/java/org/example/vet1177/controller/UserController.java (2)
45-46: Duplicate comment.Lines 45 and 46 contain the same comment. Remove one.
🧹 Remove duplicate
- //PUT /users/{id} - uppdatera användare // PUT /users/{id} - uppdatera användare `@PutMapping`("/{id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controller/UserController.java` around lines 45 - 46, Remove the duplicate comment in UserController.java that repeats the PUT /users/{id} description; inside the UserController class (near the PUT handler for updating users) keep a single explanatory comment "// PUT /users/{id} - uppdatera användare" and delete the redundant line so only one instance remains.
24-29: Consider adding pagination forgetAllUsers().Returning all users in a single response could cause performance issues as the user base grows. Consider adding pagination support.
♻️ Example with Spring Data pagination
`@GetMapping` public ResponseEntity<Page<UserResponse>> getAllUsers( `@RequestParam`(defaultValue = "0") int page, `@RequestParam`(defaultValue = "20") int size) { Page<UserResponse> users = userService.getAllUsers(PageRequest.of(page, size)); return ResponseEntity.ok(users); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controller/UserController.java` around lines 24 - 29, The getAllUsers method in UserController currently returns the full user list; change it to accept pagination parameters (e.g., page and size request params) and call a paginated service method (e.g., userService.getAllUsers(Pageable) or userService.getAllUsers(PageRequest.of(page,size))) returning a Page<UserResponse> (or Slice) instead of List; update the controller method signature and return type, add `@RequestParam` defaults for page/size, and ensure the UserService and its data-repository method (e.g., UserService.getAllUsers and underlying repository) are updated to accept a Pageable and return a Page<UserResponse>.src/main/java/org/example/vet1177/dto/response/user/UserResponse.java (1)
8-86: Consider using a Java record to reduce boilerplate.The DTO contains only data fields with getters/setters. A record would be more concise and immutable by default.
♻️ Proposed refactor using Java record
package org.example.vet1177.dto.response.user; import org.example.vet1177.entities.Role; import java.time.Instant; import java.util.UUID; public record UserResponse( UUID id, String name, String email, Role role, UUID clinicId, Instant createdAt, Instant updatedAt ) {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/dto/response/user/UserResponse.java` around lines 8 - 86, The UserResponse class is pure data with boilerplate getters/setters; replace it with a compact Java record to make it immutable and concise: declare a record named UserResponse(UUID id, String name, String email, Role role, UUID clinicId, Instant createdAt, Instant updatedAt) and remove the existing class body, constructors and accessor/mutator methods; ensure the file has imports for org.example.vet1177.entities.Role, java.time.Instant and java.util.UUID and that the project source/target compatibility supports Java records (Java 16+), then update any usages that rely on setters to use the record's canonical constructor or with-expression style replacements.
🤖 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/vet1177/controller/UserController.java`:
- Around line 14-16: The class-level mapping on UserController is inconsistent:
replace the existing `@RequestMapping`("/users") with the project's API prefix so
the controller uses `@RequestMapping`("/api/users") (i.e., update the
UserController class annotation). Locate the `@RequestMapping` on the
UserController class and change the path to include "/api" so routes match other
controllers like /api/medical-records and /api/activity-logs.
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 67-75: The updateUser method currently ignores the required
password field on UserRequest (while createUser handles passwords), leading to
ambiguous behavior; update this by one of three fixes: (1) introduce a separate
UserUpdateRequest DTO without a password and change updateUser to accept it
(update references to UserRequest in updateUser, validateEmailUniqueForUpdate,
applyClinicRules, and mapToResponse accordingly), or (2) implement password
update inside updateUser by hashing the incoming request.getPassword() and
calling user.setPasswordHash(hashed) before saving (reuse the same hashing
utility used by createUser), or (3) explicitly document in updateUser with a
clear comment and/or throw a ValidationException that password updates are
unsupported so callers aren’t misled. Ensure whichever option you pick keeps
validateEmailUniqueForUpdate(id), applyClinicRules(user, request), and
userRepository.save(user) behavior consistent.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/controller/UserController.java`:
- Around line 45-46: Remove the duplicate comment in UserController.java that
repeats the PUT /users/{id} description; inside the UserController class (near
the PUT handler for updating users) keep a single explanatory comment "// PUT
/users/{id} - uppdatera användare" and delete the redundant line so only one
instance remains.
- Around line 24-29: The getAllUsers method in UserController currently returns
the full user list; change it to accept pagination parameters (e.g., page and
size request params) and call a paginated service method (e.g.,
userService.getAllUsers(Pageable) or
userService.getAllUsers(PageRequest.of(page,size))) returning a
Page<UserResponse> (or Slice) instead of List; update the controller method
signature and return type, add `@RequestParam` defaults for page/size, and ensure
the UserService and its data-repository method (e.g., UserService.getAllUsers
and underlying repository) are updated to accept a Pageable and return a
Page<UserResponse>.
In `@src/main/java/org/example/vet1177/dto/request/user/UserRequest.java`:
- Around line 19-20: The password field in UserRequest currently only has
`@NotBlank`; add a minimum-length and/or complexity constraint (e.g., annotate the
private String password field in class UserRequest with `@Size`(min = 8) and
optionally `@Pattern`(...) for complexity) so passwords meet strength
requirements, and update imports for javax.validation.constraints.Size and/or
Pattern and provide a clear validation message string for the annotations.
In `@src/main/java/org/example/vet1177/dto/response/user/UserResponse.java`:
- Around line 8-86: The UserResponse class is pure data with boilerplate
getters/setters; replace it with a compact Java record to make it immutable and
concise: declare a record named UserResponse(UUID id, String name, String email,
Role role, UUID clinicId, Instant createdAt, Instant updatedAt) and remove the
existing class body, constructors and accessor/mutator methods; ensure the file
has imports for org.example.vet1177.entities.Role, java.time.Instant and
java.util.UUID and that the project source/target compatibility supports Java
records (Java 16+), then update any usages that rely on setters to use the
record's canonical constructor or with-expression style replacements.
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 109-120: The applyClinicRules method currently clears any clinicId
for non-VET roles; change it to reject unexpected clinicId input by validating
in applyClinicRules(User user, UserRequest request): if request.getRole() !=
Role.VET and request.getClinicId() != null then throw an
IllegalArgumentException (or a domain-specific exception) with a clear message
indicating clinicId is only allowed for VET role; keep existing behavior for
Role.VET (validate clinicId non-null, fetch via clinicService.getById and set on
user) and remove the silent user.setClinic(null) branch.
🪄 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: 06358564-556f-488d-b681-bfd4a7f9c938
📒 Files selected for processing (7)
src/main/java/org/example/vet1177/controller/ActivityLogController.javasrc/main/java/org/example/vet1177/controller/MedicalRecordController.javasrc/main/java/org/example/vet1177/controller/UserController.javasrc/main/java/org/example/vet1177/dto/request/user/UserRequest.javasrc/main/java/org/example/vet1177/dto/response/user/UserResponse.javasrc/main/java/org/example/vet1177/repository/UserRepository.javasrc/main/java/org/example/vet1177/services/UserService.java
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/org/example/vet1177/services/UserService.java (1)
30-40: Password storage acknowledged as TODO — ensure field naming aligns with intent.The TODO correctly notes that BCrypt hashing should be added. However, storing a plaintext password in a field named
passwordHash(per project conventions) is misleading. Consider either:
- Renaming the field temporarily to
passworduntil hashing is implemented, or- Adding a more prominent warning in the code that the current implementation stores plaintext
Based on learnings: the team prefers naming the password field
passwordHashto clearly signal that only hashed/encoded passwords should be stored, not plaintext.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/UserService.java` around lines 30 - 40, The createUser method is currently storing plaintext credentials into the User entity which is misleading given the project convention of using a passwordHash field; update createUser to encode the incoming request.getPassword() before constructing/persisting the User (use Spring's BCryptPasswordEncoder or equivalent) so the value saved into the User's passwordHash field is the hashed value; keep existing calls to validateEmailUnique, applyClinicRules, userRepository.save, and mapToResponse unchanged but ensure the User constructor receives the encoded password (or the setter for passwordHash is used) rather than the raw password.src/main/java/org/example/vet1177/controller/UserController.java (1)
46-47: Remove duplicate comment.Lines 46-47 contain the same comment twice.
🧹 Proposed fix
- //PUT /users/{id} - uppdatera användare - // PUT /users/{id} - uppdatera användare + // PUT /users/{id} - uppdatera användare `@PutMapping`("/{id}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controller/UserController.java` around lines 46 - 47, Remove the duplicated comment in UserController.java by keeping a single, clear comment for the PUT /users/{id} endpoint; locate the duplicate comments near the PUT handler in class UserController (around the method that handles updating users, e.g., the method annotated with `@PutMapping` or named updateUser) and delete the redundant line so only one comment remains.
🤖 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/vet1177/controller/UserController.java`:
- Around line 40-53: UserService's createUser/updateUser currently throw
IllegalArgumentException for email-uniqueness and clinic rules which bubbles to
the generic exception handler and returns 500; add a dedicated handler in
GlobalExceptionHandler (an `@ExceptionHandler`(IllegalArgumentException.class)
method) that maps these IllegalArgumentException cases to an appropriate 4xx
response (e.g., return ResponseEntity.status(HttpStatus.CONFLICT) for
duplicate-email messages or HttpStatus.BAD_REQUEST for clinic rule violations)
and include the exception message in the response body, or alternatively modify
UserService to throw an existing BusinessRuleException or a new
DuplicateEmailException instead of IllegalArgumentException so the exception is
already handled correctly.
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 109-119: Replace the generic IllegalArgumentException in
validateEmailUnique and validateEmailUniqueForUpdate with a business-rule
exception that the GlobalExceptionHandler maps to 400 (e.g., throw new
BusinessRuleException("Email används redan") or create/throw a
DuplicateEmailException that extends the existing BusinessRuleException); update
the two methods (validateEmailUnique, validateEmailUniqueForUpdate) to throw
that specific exception so email-uniqueness violations are handled as Bad
Request rather than causing HTTP 500.
- Around line 121-134: In applyClinicRules, replace the thrown
IllegalArgumentException when clinicId is null with a BusinessRuleException so
the global handler maps it correctly; specifically change the throw in
applyClinicRules from throw new IllegalArgumentException("Veterinär måste vara
kopplad till en klinik") to throw new BusinessRuleException("Veterinär måste
vara kopplad till en klinik") (keeping the same message), leaving the
clinicRepository.findById(...).orElseThrow(...) and user.setClinic(...) logic
unchanged.
- Around line 71-87: The updateUser method currently calls
applyClinicRules(user, request.getClinicId()) unconditionally, causing null
clinicId to overwrite or error on partial updates; modify updateUser to only
invoke clinic-assignment logic when request.getClinicId() != null, and implement
a new helper (e.g., applyClinicRulesForUpdate or refactor applyClinicRules) that
looks up the clinic via clinicRepository.findById(clinicId) (throw
ResourceNotFoundException if missing) and then: if user.getRole() == Role.VET
set user.setClinic(clinic), otherwise throw an IllegalArgumentException
indicating only vets can be assigned to clinics—ensuring non-VET updates do not
null out an existing clinic when clinicId is absent.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/controller/UserController.java`:
- Around line 46-47: Remove the duplicated comment in UserController.java by
keeping a single, clear comment for the PUT /users/{id} endpoint; locate the
duplicate comments near the PUT handler in class UserController (around the
method that handles updating users, e.g., the method annotated with `@PutMapping`
or named updateUser) and delete the redundant line so only one comment remains.
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 30-40: The createUser method is currently storing plaintext
credentials into the User entity which is misleading given the project
convention of using a passwordHash field; update createUser to encode the
incoming request.getPassword() before constructing/persisting the User (use
Spring's BCryptPasswordEncoder or equivalent) so the value saved into the User's
passwordHash field is the hashed value; keep existing calls to
validateEmailUnique, applyClinicRules, userRepository.save, and mapToResponse
unchanged but ensure the User constructor receives the encoded password (or the
setter for passwordHash is used) rather than the raw password.
🪄 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: 64e36d1d-8aea-4b50-9210-e95c6f38079d
📒 Files selected for processing (4)
src/main/java/org/example/vet1177/controller/UserController.javasrc/main/java/org/example/vet1177/dto/request/user/UserRequest.javasrc/main/java/org/example/vet1177/dto/request/user/UserUpdateRequest.javasrc/main/java/org/example/vet1177/services/UserService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/vet1177/dto/request/user/UserRequest.java
Comment on lines
+40
to
+53
| @PostMapping | ||
| public ResponseEntity<UserResponse> createUser(@Valid @RequestBody UserRequest request) { | ||
| UserResponse user = userService.createUser(request); | ||
| return ResponseEntity.status(HttpStatus.CREATED).body(user); | ||
| } | ||
|
|
||
| //PUT /users/{id} - uppdatera användare | ||
| // PUT /users/{id} - uppdatera användare | ||
| @PutMapping("/{id}") | ||
| public ResponseEntity<UserResponse> updateUser(@PathVariable UUID id, | ||
| @Valid @RequestBody UserUpdateRequest request) { | ||
| UserResponse user = userService.updateUser(id, request); | ||
| return ResponseEntity.ok(user); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify GlobalExceptionHandler doesn't handle IllegalArgumentException
rg -n "IllegalArgumentException" --type=java src/main/java/org/example/vet1177/exception/Repository: ithsjava25/project-backend-org-random-coders
Length of output: 70
🏁 Script executed:
fd -t f "GlobalExceptionHandler" src/main/java/Repository: ithsjava25/project-backend-org-random-coders
Length of output: 159
🏁 Script executed:
cat -n src/main/java/org/example/vet1177/exception/GlobalExceptionHandler.javaRepository: ithsjava25/project-backend-org-random-coders
Length of output: 3164
🏁 Script executed:
rg -n "IllegalArgumentException" --type=java src/main/java/ -A 3 -B 1Repository: ithsjava25/project-backend-org-random-coders
Length of output: 6319
🏁 Script executed:
rg -n "throw new IllegalArgumentException" --type=java src/main/java/Repository: ithsjava25/project-backend-org-random-coders
Length of output: 1996
IllegalArgumentException from UserService will return HTTP 500 instead of appropriate 4xx responses.
The GlobalExceptionHandler does not have an @ExceptionHandler(IllegalArgumentException.class) method. When UserService throws IllegalArgumentException for email uniqueness violations (lines 111, 117 in UserService) or clinic requirement violations (line 124), these exceptions fall through to the generic Exception handler, returning HTTP 500 with "Something went wrong" instead of a meaningful response like 400 Bad Request or 409 Conflict.
Consider either:
- Adding an
@ExceptionHandler(IllegalArgumentException.class)inGlobalExceptionHandler, or - Changing the service to throw
BusinessRuleException(already handled as 400 Bad Request) or a customDuplicateEmailException
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/controller/UserController.java` around
lines 40 - 53, UserService's createUser/updateUser currently throw
IllegalArgumentException for email-uniqueness and clinic rules which bubbles to
the generic exception handler and returns 500; add a dedicated handler in
GlobalExceptionHandler (an `@ExceptionHandler`(IllegalArgumentException.class)
method) that maps these IllegalArgumentException cases to an appropriate 4xx
response (e.g., return ResponseEntity.status(HttpStatus.CONFLICT) for
duplicate-email messages or HttpStatus.BAD_REQUEST for clinic rule violations)
and include the exception message in the response body, or alternatively modify
UserService to throw an existing BusinessRuleException or a new
DuplicateEmailException instead of IllegalArgumentException so the exception is
already handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/vet1177/services/UserService.java`:
- Around line 112-122: Add a DB-level UNIQUE constraint/index on the users.email
column (via migration) and remove reliance on existsByEmail* as the only
protection; in the service where users are created/updated (the method that
calls validateEmailUnique or validateEmailUniqueForUpdate and then
userRepository.save), catch the persistence exception thrown for duplicate keys
(e.g., DataIntegrityViolationException or the DB-specific duplicate-key
exception) and rethrow a BusinessRuleException("Email används redan") so
concurrent/in-flight requests produce the same business error; keep
validateEmailUnique/validateEmailUniqueForUpdate as fast pre-checks but ensure
the DB constraint and exception handling are the authoritative enforcement.
- Around line 93-96: deleteUser currently calls userRepository.delete(user)
unconditionally which will fail if Pet or MedicalRecord rows reference the user;
before deleting in deleteUser, query the Pet and MedicalRecord repositories
(e.g., petRepository.existsByOwnerId(id) or
medicalRecordRepository.countByUserId(id)) and if either indicates existing
references throw a domain conflict exception (e.g.,
ResourceConflictException("User", id, "has dependent pets/medical records"))
instead of deleting; if these repository methods don't exist, add efficient
exists/count methods to PetRepository and MedicalRecordRepository and keep the
deleteUser logic transactional to ensure consistency.
- Around line 31-38: The createUser flow is storing plaintext passwords; before
building/persisting the User, encode the password with BCryptPasswordEncoder and
pass the encoded value into the entity (use passwordHash naming), e.g. call new
BCryptPasswordEncoder().encode(request.getPassword()) in createUser and supply
that encoded string to the User constructor/field (and update the User entity to
use passwordHash instead of plaintext password), keep validateEmailUnique and
UserRequest/Response unchanged and ensure no plaintext password is logged or
returned.
- Around line 124-135: The applyClinicRules(User user, UUID clinicId) method
currently discards a supplied clinicId for non-VET users; change it to mirror
applyClinicRulesForUpdate(...) by rejecting invalid input: when user.getRole()
!= Role.VET and clinicId != null, throw a BusinessRuleException indicating
non-VET users may not be assigned a clinic; otherwise keep the existing behavior
of setting user.setClinic(null) for non-VETs and the current VET logic (validate
clinicId and set the clinic) intact.
🪄 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: 3b44e793-ec41-4289-8069-ea9f2c9cb8fa
📒 Files selected for processing (2)
src/main/java/org/example/vet1177/controller/UserController.javasrc/main/java/org/example/vet1177/services/UserService.java
Comment on lines
+112
to
+122
| private void validateEmailUnique(String email) { | ||
| if (userRepository.existsByEmail(email)) { | ||
| throw new BusinessRuleException("Email används redan"); | ||
| } | ||
| } | ||
|
|
||
| private void validateEmailUniqueForUpdate(String email, UUID userId) { | ||
| if (userRepository.existsByEmailAndIdNot(email, userId)) { | ||
| throw new BusinessRuleException("Email används redan"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Back email uniqueness with a database constraint.
existsByEmail* plus a later save is only a pre-check. Two concurrent requests can pass these methods before either write commits, so the unique-email rule is still race-prone here. Please back it with a DB-level unique constraint/index on email and translate duplicate-key failures to the same business error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/services/UserService.java` around lines 112
- 122, Add a DB-level UNIQUE constraint/index on the users.email column (via
migration) and remove reliance on existsByEmail* as the only protection; in the
service where users are created/updated (the method that calls
validateEmailUnique or validateEmailUniqueForUpdate and then
userRepository.save), catch the persistence exception thrown for duplicate keys
(e.g., DataIntegrityViolationException or the DB-specific duplicate-key
exception) and rethrow a BusinessRuleException("Email används redan") so
concurrent/in-flight requests produce the same business error; keep
validateEmailUnique/validateEmailUniqueForUpdate as fast pre-checks but ensure
the DB constraint and exception handling are the authoritative enforcement.
TatjanaTrajkovic
requested review from
TatjanaTrajkovic,
annikaholmqvist94 and
johanbriger
…e key violations. reject clinicId for non-VET users on create
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/services/UserService.java (1)
16-18: Inject the configuredPasswordEncoderbean instead of constructing one inline.
SecurityConfig.javaprovides aPasswordEncoderbean that should be reused here. Constructingnew BCryptPasswordEncoder()directly in line 28 bypasses the shared configuration and risks encoder settings drifting.♻️ Proposed fix
-import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder; import org.springframework.security.crypto.password.PasswordEncoder; @@ - private final PasswordEncoder passwordEncoder = new BCryptPasswordEncoder(); + private final PasswordEncoder passwordEncoder; @@ public UserService(UserRepository userRepository, ClinicRepository clinicRepository, PetRepository petRepository, - MedicalRecordRepository medicalRecordRepository) { + MedicalRecordRepository medicalRecordRepository, + PasswordEncoder passwordEncoder) { this.userRepository = userRepository; this.clinicRepository = clinicRepository; this.petRepository = petRepository; this.medicalRecordRepository = medicalRecordRepository; + this.passwordEncoder = passwordEncoder; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/UserService.java` around lines 16 - 18, The UserService currently constructs a new BCryptPasswordEncoder inline; instead inject the shared PasswordEncoder bean and use it for encoding to respect SecurityConfig. Add a PasswordEncoder field to UserService (constructor-inject or `@Autowired`) and remove any direct "new BCryptPasswordEncoder()" usage (e.g., in the user creation/registration method), replacing it with the injected passwordEncoder.encode(...) call so the service reuses the configured bean.
🤖 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/vet1177/services/UserService.java`:
- Around line 113-137: The deleteUser method currently does pre-checks but is
race-prone; keep the existing exists* guards, and wrap the
userRepository.delete(user) call in a try/catch that catches persistence
integrity exceptions (e.g.,
org.springframework.dao.DataIntegrityViolationException or
javax.persistence.PersistenceException) and rethrow a BusinessRuleException with
a domain-friendly message like the existing "Användaren har kopplade djur och
kan inte raderas" (or a more general message about linked records) so low-level
DB constraint failures are translated to the same domain error; refer to the
deleteUser method and the userRepository.delete(user) call when adding this
catch-and-translate logic.
- Around line 117-122: The code performs the same pet dependency check twice
using petRepository.existsByOwner_Id(id) and throws BusinessRuleException on
both; remove the duplicate so only a single existsByOwner_Id(id) check remains
before throwing BusinessRuleException in the UserService delete/remove flow
(keep one check and one exception throw referencing the same message).
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/UserService.java`:
- Around line 16-18: The UserService currently constructs a new
BCryptPasswordEncoder inline; instead inject the shared PasswordEncoder bean and
use it for encoding to respect SecurityConfig. Add a PasswordEncoder field to
UserService (constructor-inject or `@Autowired`) and remove any direct "new
BCryptPasswordEncoder()" usage (e.g., in the user creation/registration method),
replacing it with the injected passwordEncoder.encode(...) call so the service
reuses the configured bean.
🪄 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: fa60aec3-ed32-49be-a7f6-a88fdfab7d60
📒 Files selected for processing (4)
pom.xmlsrc/main/java/org/example/vet1177/repository/MedicalRecordRepository.javasrc/main/java/org/example/vet1177/repository/PetRepository.javasrc/main/java/org/example/vet1177/services/UserService.java
✅ Files skipped from review due to trivial changes (1)
- pom.xml
TatjanaTrajkovic
approved these changes
Apr 6, 2026
Contributor
TatjanaTrajkovic
left a comment
TatjanaTrajkovic
left a comment
There was a problem hiding this comment.
Bra kämpat Linda! Ser bra ut för min del!
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
2 participants
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 issue #109 #92 #131
UserController, UserService & UserDTO
Vad som lagts till
UserRequestmed validering (name, email, password, role, clinicId)UserResponsemed id, name, email, role, clinicId, createdAt, updatedAtUserControllermed endpoints: GET /users, GET /users/{id}, POST /users, PUT /users/{id}, DELETE /users/{id}UserServiceuppdaterad med getAllUsers, getById, createUser, updateUser, deleteUsergetUserEntityByIdtillagd för intern användning av andra servicesmapToResponsehelper för att undvika kodupprepningÄndringar i andra filer
ActivityLogControllerochMedicalRecordControllerbehövde uppdateras eftersomgetByIdnu returnerarUserResponseistället förUser. Lade tillgetUserEntityByIdiUserServiceför att lösa detta. Detta är inte den mest hållbara lösningen — en bättre approach hade varit att flytta ansvaret till respektive service, men valdes bort för att undvika merge-konflikter.TODOs / Issues
Summary by CodeRabbit
New Features
Bug Fixes
Chores