Merged
Conversation
📝 WalkthroughWalkthroughRole-based authorization was added across case workflows by threading an Changes
Sequence DiagramsequenceDiagram
actor Client
participant CaseController
participant HeaderCurrentUserAdapter
participant CaseService
participant EmployeeRepository
participant CaseRepository
participant Database
Client->>CaseController: PUT /api/cases/{id}/assignments (body)
CaseController->>HeaderCurrentUserAdapter: currentUser()
HeaderCurrentUserAdapter-->>CaseController: Actor
CaseController->>CaseService: assignUsers(Actor, caseId, dto)
CaseService->>CaseService: authorize manager/doctor (require/deny)
alt Doctor acting on non-owned case
CaseService-->>Client: throw NotAuthorizedException
else Allowed
CaseService->>EmployeeRepository: findById(owner/handler/other) (validate roles)
EmployeeRepository->>Database: query
Database-->>EmployeeRepository: employee rows
EmployeeRepository-->>CaseService: employee entities
CaseService->>CaseRepository: findById(caseId) / save(updated case)
CaseRepository->>Database: query/update
Database-->>CaseRepository: entity/result
CaseRepository-->>CaseService: saved CaseEntity
CaseService->>CaseService: map to CaseDTO
CaseService-->>CaseController: CaseDTO
CaseController-->>Client: 200 OK with CaseDTO
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/projektarendehantering/presentation/web/UiController.java (1)
50-53:⚠️ Potential issue | 🟠 MajorRepopulate
patientswhen re-rendering the form.On validation failure we return
cases/newwithout thepatientsattribute added on Line 46, so the retry view loses its patient options and can no longer submit correctly.Suggested fix
- public String createCase(`@Valid` `@ModelAttribute`("createCaseForm") CreateCaseForm form, BindingResult result) { + public String createCase(`@Valid` `@ModelAttribute`("createCaseForm") CreateCaseForm form, + BindingResult result, + Model model) { if (result.hasErrors()) { + model.addAttribute("patients", patientService.getAllPatients()); return "cases/new"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java` around lines 50 - 53, The POST handler createCase currently returns "cases/new" on validation errors without repopulating the "patients" model attribute, causing the retry form to miss patient options; update createCase (signature and body) to accept the Model (or use existing helper) and, when result.hasErrors() is true, add the same "patients" attribute used for the GET/new view (e.g., via patientService.findAll() or the existing populate method) before returning "cases/new" so the form has its patient options on redisplay.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.java (1)
19-20: ClarifyotherIdsemantics to reduce RBAC drift.
otherIdis very generic for an authorization-critical field. Consider renaming (or adding Javadoc) to express the exact actor relationship used in access checks.Also applies to: 50-51
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.java` around lines 19 - 20, The field otherId on CaseEntity is too generic for authorization logic; update CaseEntity to either rename otherId to a clear, specific identifier (e.g., reporterId, ownerId, relatedActorId) that matches its RBAC role or add Javadoc on otherId and handlerId explaining the exact actor relationship used in access checks (who it represents, when it is populated, and how it is used in permission rules); ensure all references/usages (access checks, DTO mapping, persistence queries) are updated to the new name or consult the Javadoc so authorization code remains unambiguous.src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.java (1)
10-12: Add indexes for the new role-filter query columns.These repository methods will trend hot as case volume grows. Ensure DB indexes exist for
owner_id,handler_id, andother_idto avoid full-table scans.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.java` around lines 10 - 12, The repository exposes hot lookup methods (findAllByOwnerId, findAllByHandlerId, findAllByOtherId) but the DB schema lacks indexes on the backing columns (owner_id, handler_id, other_id); add B-tree indexes for these columns in your migration SQL (or JPA/Hibernate schema changes) so queries using CaseRepository.findAllByOwnerId, findAllByHandlerId and findAllByOtherId use the indexes and avoid full table scans—create indexes named clearly (e.g., idx_case_owner_id, idx_case_handler_id, idx_case_other_id) and include them in your migration/versioning system so they are applied reliably.
🤖 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/projektarendehantering/application/service/CaseService.java`:
- Around line 87-90: The patient branch is comparing actor.userId() to otherId,
which misses cases where createCase() stores the patient relation on the Case
entity; update the patient-access logic to use the Case's patient identifier
instead of otherId. Specifically, replace usage of
caseRepository.findAllByOtherId(...) / otherId checks with a repository method
or in-stream filter that compares actor.userId() to the Case's patient id (e.g.,
Case.getPatientId() or Case.getPatient().getId()), and make the same change for
the analogous block referenced at 119-124 so patients see cases tied via the
stored patient relation.
- Around line 113-115: The code in CaseService that blindly copies
dto.getOwnerId()/getHandlerId()/getOtherId() to entity must validate each UUID
before setting it: for each non-null dto ID, query the appropriate
repository/service (e.g., PrincipalRepository/UserRepository or the
domain-specific repo used elsewhere in CaseService) to ensure the record exists
and has the expected role/type, and if not throw a 4xx error (e.g.,
BadRequestException/IllegalArgumentException) with a clear message; only call
entity.setOwnerId(...)/setHandlerId(...)/setOtherId(...) after the
existence-and-role checks pass so you never persist nonexistent or wrong-type
principals.
- Around line 104-115: The assignUsers method currently lets any doctor modify
unowned cases or overwrite owner/handler/other fields; restrict doctor
permissions so only managers can set or change ownerId, and doctors may only
modify handlerId and otherId on cases they already own. Concretely: replace the
current doctor check with a guard that if isDoctor(actor) then require
entity.getOwnerId() != null && entity.getOwnerId().equals(actor.userId());
additionally, when isDoctor(actor) and dto.getOwnerId() != null (or any attempt
to change ownerId), reject with NotAuthorizedException (or ignore ownerId
change) so only isManager(actor) can set entity.setOwnerId(...); allow doctors
to update entity.setHandlerId(...) and entity.setOtherId(...) only when they are
the current owner.
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 27-44: The service methods createEmployee, getEmployee, and
getAllEmployees lack RBAC checks and must be made actor-aware: change the
signatures (or add overloads) to accept the current actor/principal (or fetch it
from a SecurityContext) and enforce permission checks inside EmployeeService
(e.g., verify actor has ROLE_HR or a specific permission before proceeding) when
invoked from EmployeeController; update EmployeeController to pass the current
actor to EmployeeService (mirroring how case endpoints pass the actor) and
ensure repository calls (employeeRepository.save, findById, findAll) only run
after the authorization check succeeds.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java`:
- Around line 50-53: The POST handler createCase currently returns "cases/new"
on validation errors without repopulating the "patients" model attribute,
causing the retry form to miss patient options; update createCase (signature and
body) to accept the Model (or use existing helper) and, when result.hasErrors()
is true, add the same "patients" attribute used for the GET/new view (e.g., via
patientService.findAll() or the existing populate method) before returning
"cases/new" so the form has its patient options on redisplay.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.java`:
- Around line 19-20: The field otherId on CaseEntity is too generic for
authorization logic; update CaseEntity to either rename otherId to a clear,
specific identifier (e.g., reporterId, ownerId, relatedActorId) that matches its
RBAC role or add Javadoc on otherId and handlerId explaining the exact actor
relationship used in access checks (who it represents, when it is populated, and
how it is used in permission rules); ensure all references/usages (access
checks, DTO mapping, persistence queries) are updated to the new name or consult
the Javadoc so authorization code remains unambiguous.
In
`@src/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.java`:
- Around line 10-12: The repository exposes hot lookup methods
(findAllByOwnerId, findAllByHandlerId, findAllByOtherId) but the DB schema lacks
indexes on the backing columns (owner_id, handler_id, other_id); add B-tree
indexes for these columns in your migration SQL (or JPA/Hibernate schema
changes) so queries using CaseRepository.findAllByOwnerId, findAllByHandlerId
and findAllByOtherId use the indexes and avoid full table scans—create indexes
named clearly (e.g., idx_case_owner_id, idx_case_handler_id, idx_case_other_id)
and include them in your migration/versioning system so they are applied
reliably.
🪄 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: 4c74ac74-7bc2-43e8-af2a-574fe423dbfe
📒 Files selected for processing (16)
src/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeMapper.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/common/Role.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/EmployeeRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseAssignmentDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CreateCaseForm.javasrc/main/java/org/example/projektarendehantering/presentation/dto/EmployeeCreateDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/EmployeeDTO.javasrc/main/java/org/example/projektarendehantering/presentation/rest/CaseController.javasrc/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.javasrc/main/java/org/example/projektarendehantering/presentation/rest/PatientController.javasrc/main/java/org/example/projektarendehantering/presentation/web/UiController.java
src/main/java/org/example/projektarendehantering/application/service/CaseService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/projektarendehantering/application/service/CaseService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/projektarendehantering/application/service/CaseService.java
Outdated
Show resolved
Hide resolved
Comment on lines
+27
to
+44
| @Transactional | ||
| public EmployeeDTO createEmployee(EmployeeCreateDTO dto) { | ||
| EmployeeEntity entity = employeeMapper.toEntity(dto); | ||
| entity.setId(UUID.randomUUID()); | ||
| entity.setCreatedAt(Instant.now()); | ||
| return employeeMapper.toDTO(employeeRepository.save(entity)); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public Optional<EmployeeDTO> getEmployee(UUID id) { | ||
| return employeeRepository.findById(id).map(employeeMapper::toDTO); | ||
| } | ||
|
|
||
| @Transactional(readOnly = true) | ||
| public List<EmployeeDTO> getAllEmployees() { | ||
| return employeeRepository.findAll().stream() | ||
| .map(employeeMapper::toDTO) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
Add RBAC to the employee service before exposing it publicly.
These methods are the only new service surface in this PR that is not actor-aware. Combined with src/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.java:23-37, any authenticated caller reaching the controller can create employees or read the full employee directory, which is a privilege escalation.
Suggested direction
+import org.example.projektarendehantering.common.Actor;
+import org.example.projektarendehantering.common.NotAuthorizedException;
+import org.example.projektarendehantering.common.Role;
+
`@Service`
public class EmployeeService {
@@
`@Transactional`
- public EmployeeDTO createEmployee(EmployeeCreateDTO dto) {
+ public EmployeeDTO createEmployee(Actor actor, EmployeeCreateDTO dto) {
+ requireManager(actor);
EmployeeEntity entity = employeeMapper.toEntity(dto);
entity.setId(UUID.randomUUID());
entity.setCreatedAt(Instant.now());
return employeeMapper.toDTO(employeeRepository.save(entity));
}
@@
`@Transactional`(readOnly = true)
- public Optional<EmployeeDTO> getEmployee(UUID id) {
+ public Optional<EmployeeDTO> getEmployee(Actor actor, UUID id) {
+ requireManager(actor);
return employeeRepository.findById(id).map(employeeMapper::toDTO);
}
@@
`@Transactional`(readOnly = true)
- public List<EmployeeDTO> getAllEmployees() {
+ public List<EmployeeDTO> getAllEmployees(Actor actor) {
+ requireManager(actor);
return employeeRepository.findAll().stream()
.map(employeeMapper::toDTO)
.collect(Collectors.toList());
}
+
+ private void requireManager(Actor actor) {
+ if (actor.role() != Role.MANAGER && actor.role() != Role.ADMIN) {
+ throw new NotAuthorizedException("Not allowed to manage employees");
+ }
+ }
}EmployeeController then needs to pass the current actor the same way the case endpoints do.
📝 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
| @Transactional | |
| public EmployeeDTO createEmployee(EmployeeCreateDTO dto) { | |
| EmployeeEntity entity = employeeMapper.toEntity(dto); | |
| entity.setId(UUID.randomUUID()); | |
| entity.setCreatedAt(Instant.now()); | |
| return employeeMapper.toDTO(employeeRepository.save(entity)); | |
| } | |
| @Transactional(readOnly = true) | |
| public Optional<EmployeeDTO> getEmployee(UUID id) { | |
| return employeeRepository.findById(id).map(employeeMapper::toDTO); | |
| } | |
| @Transactional(readOnly = true) | |
| public List<EmployeeDTO> getAllEmployees() { | |
| return employeeRepository.findAll().stream() | |
| .map(employeeMapper::toDTO) | |
| .collect(Collectors.toList()); | |
| import org.example.projektarendehantering.common.Actor; | |
| import org.example.projektarendehantering.common.NotAuthorizedException; | |
| import org.example.projektarendehantering.common.Role; | |
| `@Transactional` | |
| public EmployeeDTO createEmployee(Actor actor, EmployeeCreateDTO dto) { | |
| requireManager(actor); | |
| EmployeeEntity entity = employeeMapper.toEntity(dto); | |
| entity.setId(UUID.randomUUID()); | |
| entity.setCreatedAt(Instant.now()); | |
| return employeeMapper.toDTO(employeeRepository.save(entity)); | |
| } | |
| `@Transactional`(readOnly = true) | |
| public Optional<EmployeeDTO> getEmployee(Actor actor, UUID id) { | |
| requireManager(actor); | |
| return employeeRepository.findById(id).map(employeeMapper::toDTO); | |
| } | |
| `@Transactional`(readOnly = true) | |
| public List<EmployeeDTO> getAllEmployees(Actor actor) { | |
| requireManager(actor); | |
| return employeeRepository.findAll().stream() | |
| .map(employeeMapper::toDTO) | |
| .collect(Collectors.toList()); | |
| } | |
| private void requireManager(Actor actor) { | |
| if (actor.role() != Role.MANAGER && actor.role() != Role.ADMIN) { | |
| throw new NotAuthorizedException("Not allowed to manage employees"); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`
around lines 27 - 44, The service methods createEmployee, getEmployee, and
getAllEmployees lack RBAC checks and must be made actor-aware: change the
signatures (or add overloads) to accept the current actor/principal (or fetch it
from a SecurityContext) and enforce permission checks inside EmployeeService
(e.g., verify actor has ROLE_HR or a specific permission before proceeding) when
invoked from EmployeeController; update EmployeeController to pass the current
actor to EmployeeService (mirroring how case endpoints pass the actor) and
ensure repository calls (employeeRepository.save, findById, findAll) only run
after the authorization check succeeds.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java (1)
45-51: Consider adding pagination for scalability.
findAll()loads all employees into memory. This works for small datasets but can cause performance issues as the employee count grows. Consider adding pagination support.♻️ Suggested approach with pagination
+import org.springframework.data.domain.Page; +import org.springframework.data.domain.Pageable; `@Transactional`(readOnly = true) -public List<EmployeeDTO> getAllEmployees(Actor actor) { +public Page<EmployeeDTO> getAllEmployees(Actor actor, Pageable pageable) { requireCanManageEmployees(actor); - return employeeRepository.findAll().stream() - .map(employeeMapper::toDTO) - .collect(Collectors.toList()); + return employeeRepository.findAll(pageable) + .map(employeeMapper::toDTO); }This requires
EmployeeRepositoryto extendPagingAndSortingRepositoryorJpaRepository(which it likely already does).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java` around lines 45 - 51, The getAllEmployees method currently uses employeeRepository.findAll(), which loads all rows into memory; change it to accept pagination parameters (e.g., PageRequest) and use employeeRepository.findAll(Pageable) to return a Page<EmployeeDTO> (or a paginated DTO list) by mapping Page.getContent() via employeeMapper::toDTO, and update method signature and callers accordingly; ensure EmployeeRepository extends JpaRepository/PagingAndSortingRepository so findAll(Pageable) is available and add sensible defaults (page, size, sort) and validation for those parameters in getAllEmployees.src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
110-116: Avoid usingpeek()for authorization side effects.
peek()is intended for debugging and its use for critical control flow like authorization is discouraged by the Java documentation. While it functions correctly here withcollect(), this pattern is fragile and non-idiomatic.♻️ Suggested refactor using explicit iteration
`@Transactional`(readOnly = true) public List<CaseDTO> getCasesForPatient(Actor actor, UUID patientId) { - return caseRepository.findAllByPatient_Id(patientId).stream() - .peek(entity -> requireCanRead(actor, entity)) - .map(caseMapper::toDTO) - .collect(Collectors.toList()); + List<CaseEntity> entities = caseRepository.findAllByPatient_Id(patientId); + for (CaseEntity entity : entities) { + requireCanRead(actor, entity); + } + return entities.stream() + .map(caseMapper::toDTO) + .collect(Collectors.toList()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 110 - 116, The getCasesForPatient method uses Stream.peek to perform authorization (requireCanRead) which is a misuse of peek; instead, retrieve the List from caseRepository.findAllByPatient_Id(patientId), iterate it explicitly and call requireCanRead(actor, entity) for each entity (throwing on unauthorized), then map each authorized entity with caseMapper.toDTO and collect into a List to return; update the getCasesForPatient implementation to use a simple for-loop or forEach over the fetched list (referencing getCasesForPatient, caseRepository.findAllByPatient_Id, requireCanRead, and caseMapper.toDTO) so authorization is performed deterministically before mapping.
🤖 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/projektarendehantering/application/service/EmployeeService.java`:
- Around line 30-37: createEmployee currently calls employeeMapper.toEntity(dto)
without validating dto, causing a NullPointerException when dto is null; add a
null check at the start of createEmployee (after
requireCanManageEmployees(actor) or before, as preferred) using
Objects.requireNonNull(dto, "dto must not be null") or explicitly throw an
IllegalArgumentException with a clear message, so employeeMapper.toEntity(dto)
cannot return null and subsequent calls like entity.setId(...) and
entity.setCreatedAt(...) are safe; keep the rest of the logic
(employeeMapper.toEntity, entity.setId, entity.setCreatedAt,
employeeRepository.save, employeeMapper.toDTO) unchanged.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 110-116: The getCasesForPatient method uses Stream.peek to perform
authorization (requireCanRead) which is a misuse of peek; instead, retrieve the
List from caseRepository.findAllByPatient_Id(patientId), iterate it explicitly
and call requireCanRead(actor, entity) for each entity (throwing on
unauthorized), then map each authorized entity with caseMapper.toDTO and collect
into a List to return; update the getCasesForPatient implementation to use a
simple for-loop or forEach over the fetched list (referencing
getCasesForPatient, caseRepository.findAllByPatient_Id, requireCanRead, and
caseMapper.toDTO) so authorization is performed deterministically before
mapping.
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`:
- Around line 45-51: The getAllEmployees method currently uses
employeeRepository.findAll(), which loads all rows into memory; change it to
accept pagination parameters (e.g., PageRequest) and use
employeeRepository.findAll(Pageable) to return a Page<EmployeeDTO> (or a
paginated DTO list) by mapping Page.getContent() via employeeMapper::toDTO, and
update method signature and callers accordingly; ensure EmployeeRepository
extends JpaRepository/PagingAndSortingRepository so findAll(Pageable) is
available and add sensible defaults (page, size, sort) and validation for those
parameters in getAllEmployees.
🪄 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: 5eca1368-2ddb-4508-8966-e967f39f35cb
📒 Files selected for processing (3)
src/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/EmployeeService.javasrc/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/projektarendehantering/presentation/rest/EmployeeController.java
Comment on lines
+30
to
+37
| @Transactional | ||
| public EmployeeDTO createEmployee(Actor actor, EmployeeCreateDTO dto) { | ||
| requireCanManageEmployees(actor); | ||
| EmployeeEntity entity = employeeMapper.toEntity(dto); | ||
| entity.setId(UUID.randomUUID()); | ||
| entity.setCreatedAt(Instant.now()); | ||
| return employeeMapper.toDTO(employeeRepository.save(entity)); | ||
| } |
There was a problem hiding this comment.
Add null check for dto to prevent NPE.
If dto is null, employeeMapper.toEntity(dto) returns null (per the mapper implementation), and the subsequent entity.setId() call will throw a NullPointerException. Consider validating the input early.
🛡️ Proposed fix
`@Transactional`
public EmployeeDTO createEmployee(Actor actor, EmployeeCreateDTO dto) {
requireCanManageEmployees(actor);
+ if (dto == null) {
+ throw new IllegalArgumentException("EmployeeCreateDTO cannot be null");
+ }
EmployeeEntity entity = employeeMapper.toEntity(dto);
entity.setId(UUID.randomUUID());
entity.setCreatedAt(Instant.now());
return employeeMapper.toDTO(employeeRepository.save(entity));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/projektarendehantering/application/service/EmployeeService.java`
around lines 30 - 37, createEmployee currently calls
employeeMapper.toEntity(dto) without validating dto, causing a
NullPointerException when dto is null; add a null check at the start of
createEmployee (after requireCanManageEmployees(actor) or before, as preferred)
using Objects.requireNonNull(dto, "dto must not be null") or explicitly throw an
IllegalArgumentException with a clear message, so employeeMapper.toEntity(dto)
cannot return null and subsequent calls like entity.setId(...) and
entity.setCreatedAt(...) are safe; keep the rest of the logic
(employeeMapper.toEntity, entity.setId, entity.setCreatedAt,
employeeRepository.save, employeeMapper.toDTO) unchanged.
This was referenced Mar 31, 2026
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.
Summary by CodeRabbit