Feature/ticket controller - #11
Merged
Merged
Conversation
📝 WalkthroughWalkthroughAdds ticket management: a Spring MVC Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Controller as TicketController
participant Service as TicketService
participant Repo as TicketRepository
participant Mapper as TicketMapper
User->>Controller: GET /create
Controller-->>User: render create view (TicketCreateDTO)
User->>Controller: POST /create (TicketCreateDTO)
Controller->>Service: createNewTicket(dto)
Service->>Repo: save(Ticket)
Repo-->>Service: Ticket(saved)
Service->>Mapper: entityToViewDTO(Ticket)
Mapper-->>Service: TicketViewDTO
Service-->>Controller: TicketViewDTO
Controller-->>User: redirect /home
rect rgba(0,128,0,0.5)
User->>Controller: GET /view/id/{id} or /view/token/{token}
end
Controller->>Service: getTicketById/getTicketByToken
Service->>Repo: find...
Repo-->>Service: Ticket
Service->>Mapper: entityToViewDTO(Ticket)
Mapper-->>Controller: TicketViewDTO
Controller-->>User: render view with ticket
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
…ture/ticket-controller
![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 (5)
src/main/java/org/example/alfs/controllers/TicketController.java (2)
6-6: Unused import.The
Ticketentity import is not used in this controller.🧹 Remove unused import
-import org.example.alfs.entities.Ticket;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/TicketController.java` at line 6, Remove the unused import org.example.alfs.entities.Ticket from the top of the TicketController class: open the TicketController file, delete the import line for Ticket, and ensure the class compiles (no other references to Ticket remain); this cleans up the unused import in the TicketController.
19-19: Mark the field asprivate finalfor immutability.The
ticketServicefield should beprivate finalto ensure it's immutable after construction, which is a best practice for dependency injection.♻️ Add access modifier and final
- TicketService ticketService; + private final TicketService ticketService;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/TicketController.java` at line 19, Make the ticketService field in TicketController immutable by changing its declaration to private final and ensure any constructor assigns it (e.g., the TicketController constructor sets this.ticketService = ticketService); update any field usages accordingly and remove/adjust any setters if present so the dependency remains readonly after construction (references: class TicketController, field ticketService, type TicketService).src/main/java/org/example/alfs/services/TicketService.java (2)
12-12: Unused import.The
Optionalimport is not used directly sinceorElseThrow()returns the unwrapped value.🧹 Remove unused import
import java.util.List; -import java.util.Optional;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/TicketService.java` at line 12, Remove the unused import statement "import java.util.Optional;" from TicketService (it’s not referenced because orElseThrow() returns the unwrapped value); open the TicketService class and delete that import line (or run your IDE's Organize/Optimize Imports) to keep imports clean and avoid unused-import warnings.
26-37: Consider adding@Transactionalfor write operations.While Spring Data's
save()method is transactional internally, annotating service methods with@Transactionalis a best practice for explicit transaction boundary control and consistency across the service layer.♻️ Add `@Transactional` annotation
+import org.springframework.transaction.annotation.Transactional; //createNewTicket + `@Transactional` public TicketViewDTO createNewTicket(TicketCreateDTO ticketCreateDTO) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 26 - 37, Add an explicit transaction boundary to this write operation: annotate the createNewTicket method in TicketService with Spring's `@Transactional` (import javax.transaction.Transactional or org.springframework.transaction.annotation.Transactional per project conventions) so the call to ticketRepository.save(ticket) is executed within a managed transaction; ensure the service class has the appropriate import and, if using class-level annotations, consider placing `@Transactional` on the service class instead of the method to cover other mutating methods while leaving read-only methods unannotated or marked `@Transactional`(readOnly=true) as needed.src/main/java/org/example/alfs/mapper/TicketMapper.java (1)
10-24: Consider adding null-safety for the input parameter.If
ticketis null, this method will throw aNullPointerException. While the service layer should prevent this, defensive coding would improve robustness.🛡️ Optional: Add null check
public TicketViewDTO entityToViewDTO(Ticket ticket){ + if (ticket == null) { + return null; + } TicketViewDTO ticketViewDTO = new TicketViewDTO();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/mapper/TicketMapper.java` around lines 10 - 24, The entityToViewDTO method in TicketMapper lacks null-safety for its input; add an explicit null check at the top of TicketMapper.entityToViewDTO (e.g., if (ticket == null) return null;) so the method returns null (or alternatively throw an IllegalArgumentException) instead of causing a NullPointerException, and keep the rest of the mapping logic unchanged; update any callers/tests accordingly if they expect a different behavior.
🤖 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/alfs/controllers/TicketController.java`:
- Around line 36-46: The POST handler createNewTicket in TicketController uses
`@ModelAttribute` without a name so the form model becomes "ticketCreateDTO", but
the GET handler populates the form as "ticket"; to fix, explicitly set the model
attribute name on the POST handler to "ticket" (i.e., change the `@ModelAttribute`
usage in createNewTicket to use the same name used by the GET handler) so
BindingResult errors will redisplay the form correctly with the expected model
attribute.
- Around line 71-81: The two stub methods assignTicket and updateStatus
currently redirect to "/tickets/{id}" which does not exist and causes 404s;
update their redirect targets to the actual view endpoint used by this
controller ("/view/id/{id}") so both methods return "redirect:/view/id/" + id;
keep the TODO about implementing DTO/service but ensure the immediate fix is to
change the redirect strings in assignTicket and updateStatus to point to
"/view/id/{id}".
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 40-54: Replace the generic RuntimeException thrown in
getTicketByToken and getTicketById with a domain/HTTP-aware exception: either
throw a ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found") when
ticketRepository.findByReporterToken(...) or ticketRepository.findById(...)
returns empty, or create and throw a custom TicketNotFoundException (e.g.,
extend RuntimeException) and map it to 404 (via `@ResponseStatus` or a controller
advice). Update the two methods (getTicketByToken and getTicketById) to use the
chosen exception and keep the rest (ticketMapper.entityToViewDTO) unchanged.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/controllers/TicketController.java`:
- Line 6: Remove the unused import org.example.alfs.entities.Ticket from the top
of the TicketController class: open the TicketController file, delete the import
line for Ticket, and ensure the class compiles (no other references to Ticket
remain); this cleans up the unused import in the TicketController.
- Line 19: Make the ticketService field in TicketController immutable by
changing its declaration to private final and ensure any constructor assigns it
(e.g., the TicketController constructor sets this.ticketService =
ticketService); update any field usages accordingly and remove/adjust any
setters if present so the dependency remains readonly after construction
(references: class TicketController, field ticketService, type TicketService).
In `@src/main/java/org/example/alfs/mapper/TicketMapper.java`:
- Around line 10-24: The entityToViewDTO method in TicketMapper lacks
null-safety for its input; add an explicit null check at the top of
TicketMapper.entityToViewDTO (e.g., if (ticket == null) return null;) so the
method returns null (or alternatively throw an IllegalArgumentException) instead
of causing a NullPointerException, and keep the rest of the mapping logic
unchanged; update any callers/tests accordingly if they expect a different
behavior.
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Line 12: Remove the unused import statement "import java.util.Optional;" from
TicketService (it’s not referenced because orElseThrow() returns the unwrapped
value); open the TicketService class and delete that import line (or run your
IDE's Organize/Optimize Imports) to keep imports clean and avoid unused-import
warnings.
- Around line 26-37: Add an explicit transaction boundary to this write
operation: annotate the createNewTicket method in TicketService with Spring's
`@Transactional` (import javax.transaction.Transactional or
org.springframework.transaction.annotation.Transactional per project
conventions) so the call to ticketRepository.save(ticket) is executed within a
managed transaction; ensure the service class has the appropriate import and, if
using class-level annotations, consider placing `@Transactional` on the service
class instead of the method to cover other mutating methods while leaving
read-only methods unannotated or marked `@Transactional`(readOnly=true) as 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: 55ec2aab-96bb-40a0-987e-ce6cf8100b5b
📒 Files selected for processing (3)
src/main/java/org/example/alfs/controllers/TicketController.javasrc/main/java/org/example/alfs/mapper/TicketMapper.javasrc/main/java/org/example/alfs/services/TicketService.java
Comment on lines
+71
to
+81
| @PostMapping("/{id}/assign") | ||
| public String assignTicket(@PathVariable Long id) { | ||
| return "redirect:/tickets/" + id; | ||
| } | ||
|
|
||
| //update status | ||
| // TODO implement DTO + service | ||
| @PostMapping("/{id}/status") | ||
| public String updateStatus(@PathVariable Long id) { | ||
| return "redirect:/tickets/" + id; | ||
| } |
There was a problem hiding this comment.
Stub methods redirect to non-existent routes.
Both assignTicket and updateStatus redirect to /tickets/{id}, but this controller has no mapping for that path. The existing view endpoint is at /view/id/{id}. This will cause 404 errors when these stubs are invoked.
🐛 Fix redirect URLs to match existing routes
`@PostMapping`("/{id}/assign")
public String assignTicket(`@PathVariable` Long id) {
- return "redirect:/tickets/" + id;
+ return "redirect:/view/id/" + id;
}
//update status
// TODO implement DTO + service
`@PostMapping`("/{id}/status")
public String updateStatus(`@PathVariable` Long id) {
- return "redirect:/tickets/" + id;
+ return "redirect:/view/id/" + id;
}📝 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
| @PostMapping("/{id}/assign") | |
| public String assignTicket(@PathVariable Long id) { | |
| return "redirect:/tickets/" + id; | |
| } | |
| //update status | |
| // TODO implement DTO + service | |
| @PostMapping("/{id}/status") | |
| public String updateStatus(@PathVariable Long id) { | |
| return "redirect:/tickets/" + id; | |
| } | |
| `@PostMapping`("/{id}/assign") | |
| public String assignTicket(`@PathVariable` Long id) { | |
| return "redirect:/view/id/" + id; | |
| } | |
| //update status | |
| // TODO implement DTO + service | |
| `@PostMapping`("/{id}/status") | |
| public String updateStatus(`@PathVariable` Long id) { | |
| return "redirect:/view/id/" + id; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/alfs/controllers/TicketController.java` around
lines 71 - 81, The two stub methods assignTicket and updateStatus currently
redirect to "/tickets/{id}" which does not exist and causes 404s; update their
redirect targets to the actual view endpoint used by this controller
("/view/id/{id}") so both methods return "redirect:/view/id/" + id; keep the
TODO about implementing DTO/service but ensure the immediate fix is to change
the redirect strings in assignTicket and updateStatus to point to
"/view/id/{id}".
Contributor
There was a problem hiding this comment.
This will be fixed later since we don't really know yet where to redirect
There was a problem hiding this comment.
@addee1, understood! Since the final routes are still being decided (and the existing //TODO: Decide final routes and redirects comment already captures that intent), keeping the placeholder redirects for now makes sense. Just make sure to update them once the routing is finalized to avoid any 404 errors in testing.
🧠 Learnings used
Learnt from: FionaSprinkles
Repo: ithsjava25/project-backend-alfs PR: 11
File: src/main/java/org/example/alfs/services/TicketService.java:50-54
Timestamp: 2026-04-09T13:26:47.188Z
Learning: In `ithsjava25/project-backend-alfs`, the IDOR vulnerability in `TicketService.getTicketById()` (and the `GET /view/id/{id}` endpoint in `TicketController`) is a known, intentionally deferred issue. Authorization/ownership checks will be added in a future PR when authentication and authorization are implemented. Do not re-flag this as a new issue.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/alfs/controllers/TicketController.java (1)
71-81:⚠️ Potential issue | 🟠 MajorAssign/status endpoints are still no-op and redirect to the wrong path.
On Line 73 and Line 80,
redirect:/tickets/{id}does not match this controller’s view route (/view/id/{id}), and both handlers currently report success without performing any update.🐛 Minimal immediate fix for redirect mismatch
`@PostMapping`("/{id}/assign") public String assignTicket(`@PathVariable` Long id) { - return "redirect:/tickets/" + id; + return "redirect:/view/id/" + id; } ... `@PostMapping`("/{id}/status") public String updateStatus(`@PathVariable` Long id) { - return "redirect:/tickets/" + id; + return "redirect:/view/id/" + id; }If you want, I can draft the DTO/service contract for
assignandstatusso these endpoints perform real state changes instead of no-op redirects.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/controllers/TicketController.java` around lines 71 - 81, The assignTicket and updateStatus handlers are no-ops and both redirect to the wrong view path; change their redirects to use the controller's view route ("redirect:/view/id/" + id) and implement real updates by accepting appropriate DTOs and delegating to a service: add parameters (e.g., AssignTicketDto or StatusUpdateDto) to assignTicket and updateStatus, validate them, call a TicketService.assignTicket(id, dto) and TicketService.updateStatus(id, dto) (create these methods if missing), handle service errors and then return "redirect:/view/id/" + id on success; keep method names assignTicket and updateStatus so callers remain consistent.
🤖 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/alfs/controllers/TicketController.java`:
- Around line 51-67: The controller currently lets
ticketService.getTicketByToken(token) and ticketService.getTicketById(id)
propagate RuntimeException("Ticket not found") which yields 500; update
viewTicketByToken and viewTicketById to catch that specific not-found
RuntimeException (or detect by message/type) and convert it to an HTTP 404
response (e.g., by throwing new ResponseStatusException(HttpStatus.NOT_FOUND,
"Ticket not found") or setting response status and returning an error view).
Ensure you reference the service calls ticketService.getTicketByToken and
ticketService.getTicketById and preserve existing behavior for other exceptions.
---
Duplicate comments:
In `@src/main/java/org/example/alfs/controllers/TicketController.java`:
- Around line 71-81: The assignTicket and updateStatus handlers are no-ops and
both redirect to the wrong view path; change their redirects to use the
controller's view route ("redirect:/view/id/" + id) and implement real updates
by accepting appropriate DTOs and delegating to a service: add parameters (e.g.,
AssignTicketDto or StatusUpdateDto) to assignTicket and updateStatus, validate
them, call a TicketService.assignTicket(id, dto) and
TicketService.updateStatus(id, dto) (create these methods if missing), handle
service errors and then return "redirect:/view/id/" + id on success; keep method
names assignTicket and updateStatus so callers remain consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c95df1f2-15bd-4487-ac77-16895c8dd5fc
📒 Files selected for processing (1)
src/main/java/org/example/alfs/controllers/TicketController.java
Comment on lines
+51
to
+67
| public String viewTicketByToken(@PathVariable String token, Model model) { | ||
|
|
||
| TicketViewDTO ticket = ticketService.getTicketByToken(token); | ||
| model.addAttribute("ticket", ticket); | ||
|
|
||
| return "view"; | ||
| } | ||
|
|
||
| //view ticket by id | ||
| @GetMapping("/view/id/{id}") | ||
| public String viewTicketById(@PathVariable Long id, Model model) { | ||
|
|
||
| TicketViewDTO ticket = ticketService.getTicketById(id); | ||
| model.addAttribute("ticket", ticket); | ||
|
|
||
| return "view"; | ||
| } |
There was a problem hiding this comment.
Map service “not found” failures to HTTP 404 instead of server errors.
On Line 53 and Line 63, service calls can throw RuntimeException("Ticket not found"); without handling, this typically surfaces as 500 instead of a not-found response.
🐛 Proposed controller-level fix
+import org.springframework.http.HttpStatus;
+import org.springframework.web.server.ResponseStatusException;
...
`@GetMapping`("/view/token/{token}")
public String viewTicketByToken(`@PathVariable` String token, Model model) {
-
- TicketViewDTO ticket = ticketService.getTicketByToken(token);
+ TicketViewDTO ticket;
+ try {
+ ticket = ticketService.getTicketByToken(token);
+ } catch (RuntimeException ex) {
+ throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found", ex);
+ }
model.addAttribute("ticket", ticket);
return "view";
}
...
`@GetMapping`("/view/id/{id}")
public String viewTicketById(`@PathVariable` Long id, Model model) {
-
- TicketViewDTO ticket = ticketService.getTicketById(id);
+ TicketViewDTO ticket;
+ try {
+ ticket = ticketService.getTicketById(id);
+ } catch (RuntimeException ex) {
+ throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found", ex);
+ }
model.addAttribute("ticket", ticket);
return "view";
}📝 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
| public String viewTicketByToken(@PathVariable String token, Model model) { | |
| TicketViewDTO ticket = ticketService.getTicketByToken(token); | |
| model.addAttribute("ticket", ticket); | |
| return "view"; | |
| } | |
| //view ticket by id | |
| @GetMapping("/view/id/{id}") | |
| public String viewTicketById(@PathVariable Long id, Model model) { | |
| TicketViewDTO ticket = ticketService.getTicketById(id); | |
| model.addAttribute("ticket", ticket); | |
| return "view"; | |
| } | |
| public String viewTicketByToken(`@PathVariable` String token, Model model) { | |
| TicketViewDTO ticket; | |
| try { | |
| ticket = ticketService.getTicketByToken(token); | |
| } catch (RuntimeException ex) { | |
| throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found", ex); | |
| } | |
| model.addAttribute("ticket", ticket); | |
| return "view"; | |
| } | |
| //view ticket by id | |
| `@GetMapping`("/view/id/{id}") | |
| public String viewTicketById(`@PathVariable` Long id, Model model) { | |
| TicketViewDTO ticket; | |
| try { | |
| ticket = ticketService.getTicketById(id); | |
| } catch (RuntimeException ex) { | |
| throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found", ex); | |
| } | |
| model.addAttribute("ticket", ticket); | |
| return "view"; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/alfs/controllers/TicketController.java` around
lines 51 - 67, The controller currently lets
ticketService.getTicketByToken(token) and ticketService.getTicketById(id)
propagate RuntimeException("Ticket not found") which yields 500; update
viewTicketByToken and viewTicketById to catch that specific not-found
RuntimeException (or detect by message/type) and convert it to an HTTP 404
response (e.g., by throwing new ResponseStatusException(HttpStatus.NOT_FOUND,
"Ticket not found") or setting response status and returning an error view).
Ensure you reference the service calls ticketService.getTicketByToken and
ticketService.getTicketById and preserve existing behavior for other exceptions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/alfs/services/TicketService.java (2)
42-54: Extract shared “ticket not found” handling to reduce duplication.Line 44–Line 45 and Line 52–Line 53 repeat the same exception flow. Consider a small helper for consistency and easier future changes.
♻️ Suggested refactor
public class TicketService { @@ + private Ticket requireTicket(Optional<Ticket> optionalTicket) { + return optionalTicket.orElseThrow( + () -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found") + ); + } + public TicketViewDTO getTicketByToken(String token) { - - Ticket ticket = ticketRepository.findByReporterToken(token). - orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found")); + Ticket ticket = requireTicket(ticketRepository.findByReporterToken(token)); return ticketMapper.entityToViewDTO(ticket); } @@ public TicketViewDTO getTicketById(Long id) { - - Ticket ticket = ticketRepository.findById(id). - orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found")); + Ticket ticket = requireTicket(ticketRepository.findById(id)); return ticketMapper.entityToViewDTO(ticket); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 42 - 54, Extract the duplicated "ticket not found" handling in TicketService by adding a private helper method (e.g., findTicketOrThrow) that accepts a Supplier/Optional or an identifier and returns a Ticket or throws new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found"); update getTicketByToken to call findTicketOrThrow using ticketRepository.findByReporterToken(token) and getTicketById to call it using ticketRepository.findById(id), then map the returned Ticket with ticketMapper.entityToViewDTO(ticket); this centralizes the exception creation and removes the duplicated orElseThrow expressions.
59-92: Consider pagination for collection queries to prevent unbounded reads.Line 59–Line 92 currently loads full result sets for reporter/investigator/status filters. This can degrade latency and memory as data grows. Recommend pageable service/repository variants for UI lists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 59 - 92, These methods (getTicketsByReporterId, getTicketsByInvestigatorId, getTicketsByStatus, getTicketsByStatusAndInvestigator) currently stream full result sets from ticketRepository (findByReporterId, findByInvestigatorId, findByStatus, findByStatusAndInvestigatorId) and map via ticketMapper::entityToViewDTO, causing unbounded reads; change each service signature to accept a Pageable (or page/size params), update the repository to return Page<Ticket> (or Slice<Ticket>) variants, call repository.find...(…, pageable) and map the Page to Page<TicketViewDTO> using page.map(ticketMapper::entityToViewDTO), and ensure a sensible default page size/fallback is applied when Pageable is not provided.
🤖 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/alfs/services/TicketService.java`:
- Around line 50-54: getTicketById in TicketService currently returns any ticket
by numeric id without authorization checks, exposing an IDOR risk; modify
getTicketById (and the TicketController GET /view/id/{id} flow) to enforce
ownership/authorization by retrieving the authenticated principal (e.g., from
SecurityContext), validating that the current user is the ticket owner or
assignee (or has an appropriate role) before mapping and returning via
ticketMapper.entityToViewDTO, and throw a
ResponseStatusException(HttpStatus.FORBIDDEN) when the check fails; use
ticketRepository to fetch the ticket first and compare its owner/assignee fields
to the authenticated user's identifier.
---
Nitpick comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 42-54: Extract the duplicated "ticket not found" handling in
TicketService by adding a private helper method (e.g., findTicketOrThrow) that
accepts a Supplier/Optional or an identifier and returns a Ticket or throws new
ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found"); update
getTicketByToken to call findTicketOrThrow using
ticketRepository.findByReporterToken(token) and getTicketById to call it using
ticketRepository.findById(id), then map the returned Ticket with
ticketMapper.entityToViewDTO(ticket); this centralizes the exception creation
and removes the duplicated orElseThrow expressions.
- Around line 59-92: These methods (getTicketsByReporterId,
getTicketsByInvestigatorId, getTicketsByStatus,
getTicketsByStatusAndInvestigator) currently stream full result sets from
ticketRepository (findByReporterId, findByInvestigatorId, findByStatus,
findByStatusAndInvestigatorId) and map via ticketMapper::entityToViewDTO,
causing unbounded reads; change each service signature to accept a Pageable (or
page/size params), update the repository to return Page<Ticket> (or
Slice<Ticket>) variants, call repository.find...(…, pageable) and map the Page
to Page<TicketViewDTO> using page.map(ticketMapper::entityToViewDTO), and ensure
a sensible default page size/fallback is applied when Pageable is not provided.
🪄 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: 0adf696e-8ac5-42c1-a0da-aaaee090fc01
📒 Files selected for processing (1)
src/main/java/org/example/alfs/services/TicketService.java
Comment on lines
+50
to
+54
| public TicketViewDTO getTicketById(Long id) { | ||
|
|
||
| Ticket ticket = ticketRepository.findById(id). | ||
| orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Ticket not found")); | ||
| return ticketMapper.entityToViewDTO(ticket); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate controller(s) and inspect mappings + security annotations around getTicketById usage
fd -i "TicketController.java" src
rg -n -C3 'getTicketById|@GetMapping|@RequestMapping|/view|/ticket|@PreAuthorize|@Secured|@RolesAllowed' src/main/java/org/example/alfs/controllers
# 2) Inspect Spring Security configuration for route-level protections
fd -i "SecurityConfig.java" src
rg -n -C3 'SecurityFilterChain|authorizeHttpRequests|requestMatchers|permitAll|authenticated|hasRole|hasAnyRole' src/main/java
# Expected:
# - Endpoints calling getTicketById should require authenticated/authorized access
# (or enforce ownership checks before returning data).Repository: ithsjava25/project-backend-alfs
Length of output: 5261
🏁 Script executed:
cat -n src/main/java/org/example/alfs/controllers/TicketController.javaRepository: ithsjava25/project-backend-alfs
Length of output: 2960
🏁 Script executed:
# Search for Spring Security configuration
find src -name "*Security*" -o -name "*Config*.java" | head -20Repository: ithsjava25/project-backend-alfs
Length of output: 132
🏁 Script executed:
# Check if there are any authentication/authorization annotations used elsewhere in the project
rg -l '@PreAuthorize|@Secured|@RolesAllowed' src/main/javaRepository: ithsjava25/project-backend-alfs
Length of output: 57
🏁 Script executed:
find src -name "application*.properties" -o -name "application*.yml" -o -name "application*.yaml"Repository: ithsjava25/project-backend-alfs
Length of output: 116
🏁 Script executed:
# Look for main application class
find src -name "*Application.java"Repository: ithsjava25/project-backend-alfs
Length of output: 126
🏁 Script executed:
# Search for custom filters, interceptors, or authentication setup
rg -l 'FilterChain|Filter|Interceptor|SecurityFilterChain|HttpSecurity|Authentication' src/main/javaRepository: ithsjava25/project-backend-alfs
Length of output: 57
🏁 Script executed:
# Check pom.xml or build.gradle for Spring Security dependency
find . -maxdepth 2 \( -name "pom.xml" -o -name "build.gradle" \)Repository: ithsjava25/project-backend-alfs
Length of output: 84
🏁 Script executed:
cat -n src/main/resources/application.propertiesRepository: ithsjava25/project-backend-alfs
Length of output: 146
🏁 Script executed:
cat -n src/main/java/org/example/alfs/AlfsApplication.javaRepository: ithsjava25/project-backend-alfs
Length of output: 482
🏁 Script executed:
# Check for Spring Security in pom.xml
grep -A2 -B2 'spring-security' ./pom.xmlRepository: ithsjava25/project-backend-alfs
Length of output: 57
🏁 Script executed:
# Look for any SecurityConfig bean or configuration class
rg -n '@Configuration|@EnableWebSecurity|@Bean.*Security' src/main/javaRepository: ithsjava25/project-backend-alfs
Length of output: 149
🏁 Script executed:
cat -n src/main/java/org/example/alfs/config/PasswordConfig.javaRepository: ithsjava25/project-backend-alfs
Length of output: 687
🏁 Script executed:
# Check TicketService.getTicketById() for any access control logic
cat -n src/main/java/org/example/alfs/services/TicketService.java | grep -A10 "getTicketById"Repository: ithsjava25/project-backend-alfs
Length of output: 581
🏁 Script executed:
# Check if pom.xml has spring-security dependency at all
cat -n ./pom.xml | head -50Repository: ithsjava25/project-backend-alfs
Length of output: 2144
Confirm IDOR exposure: endpoint allows unrestricted ticket access by numeric ID.
The GET /view/id/{id} endpoint (TicketController line 60–66) retrieves any ticket by numeric ID without ownership validation. The service method getTicketById() returns ticket data directly with no access checks. Although Spring Security is on the classpath, no SecurityFilterChain configuration or method-level security annotations exist to restrict or validate access. A user could enumerate ticket IDs to view other users' tickets. Add ownership checks or restrict ticket access to authenticated users with proper authorization logic (e.g., verify the requester owns or is assigned to the ticket).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 50 -
54, getTicketById in TicketService currently returns any ticket by numeric id
without authorization checks, exposing an IDOR risk; modify getTicketById (and
the TicketController GET /view/id/{id} flow) to enforce ownership/authorization
by retrieving the authenticated principal (e.g., from SecurityContext),
validating that the current user is the ticket owner or assignee (or has an
appropriate role) before mapping and returning via ticketMapper.entityToViewDTO,
and throw a ResponseStatusException(HttpStatus.FORBIDDEN) when the check fails;
use ticketRepository to fetch the ticket first and compare its owner/assignee
fields to the authenticated user's identifier.
Contributor
There was a problem hiding this comment.
Will be handled when authentication/authorization is implemented.
There was a problem hiding this comment.
@FionaSprinkles, understood — that makes sense. Once authentication is in place, the ownership/authorization check can be wired into getTicketById (and related endpoints) naturally.
Would you like me to open a GitHub issue to track this so it doesn't get lost? It would serve as a reminder to add the IDOR protection once the auth layer is ready.
✏️ Learnings added
Learnt from: FionaSprinkles
Repo: ithsjava25/project-backend-alfs PR: 11
File: src/main/java/org/example/alfs/services/TicketService.java:50-54
Timestamp: 2026-04-09T13:26:47.188Z
Learning: In `ithsjava25/project-backend-alfs`, the IDOR vulnerability in `TicketService.getTicketById()` (and the `GET /view/id/{id}` endpoint in `TicketController`) is a known, intentionally deferred issue. Authorization/ownership checks will be added in a future PR when authentication and authorization are implemented. Do not re-flag this as a new issue.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: addee1
Repo: ithsjava25/project-backend-alfs PR: 9
File: src/main/java/org/example/alfs/config/SecurityConfig.java:27-29
Timestamp: 2026-04-09T09:49:11.596Z
Learning: In the project `ithsjava25/project-backend-alfs`, the Spring Security configuration (`src/main/java/org/example/alfs/config/SecurityConfig.java`) intentionally disables CSRF and frame options, and permits the H2 console without authentication. This is by design for development-only use — the project is a stateless JWT-based REST API not targeting production deployment. Do not flag these settings as security issues.
This was referenced Apr 10, 2026
Merged
This was referenced Apr 23, 2026
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
3 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.
Summary by CodeRabbit