Implement file handling for incidents with S3/MinIO support#76
Merged
Conversation
- Added file upload and download functionality for incidents
in DocumentController.
- Updated S3Service and MinIO integration for storing and retrieving files.
- Linked file uploads to incident creation and ensured correct redirects
to `/incidents/{id}` after upload and comment creation.
- Updated PageController submitIncident redirect to match `/incidents/{id}`.
- Updated CommentController redirect after creating comment to `/incidents/{id}`.
- Fixed frontend form to pass incidentId correctly for file uploads and comments.
- Ensured API endpoints for incidents remain consistent with `/api/incidents`.
- Refactored URLs to consistently use plural `/incidents` in UI routes.
📝 WalkthroughWalkthroughAdds document upload/download support: MinIO client config and S3Service, new Document JPA entity/repository and Flyway migration, DocumentService and DocumentController, integration into Incident flows and UI templates, and two OkHttp Maven dependencies. Changes
Sequence DiagramsequenceDiagram
actor User
participant PageCtrl as PageController
participant IncidentSvc as IncidentService
participant DocSvc as DocumentService
participant S3Svc as S3Service
participant MinIO as MinIO
participant DocRepo as DocumentRepository
participant DB as Database
participant DocCtrl as DocumentController
User->>PageCtrl: POST /create-incident (form + files)
PageCtrl->>IncidentSvc: createIncident(request, files, user)
IncidentSvc->>DocSvc: uploadFile(file, incident) for each file
DocSvc->>S3Svc: uploadFile(fileKey, file)
S3Svc->>MinIO: putObject(bucket, fileKey, stream)
MinIO-->>S3Svc: success
DocSvc->>DocRepo: save(Document)
DocRepo->>DB: INSERT document
DB-->>DocRepo: id
DocSvc-->>IncidentSvc: document saved
IncidentSvc-->>PageCtrl: incident created
PageCtrl-->>User: redirect /incidents/{id}
User->>DocCtrl: GET /documents/download/{incidentId}
DocCtrl->>IncidentSvc: getById(incidentId, user)
IncidentSvc->>DocSvc: getDocumentsByIncident(incident)
DocSvc->>S3Svc: downloadFile(fileKey)
S3Svc->>MinIO: getObject(bucket, fileKey)
MinIO-->>S3Svc: InputStream
S3Svc-->>DocSvc: InputStream
DocSvc-->>DocCtrl: InputStreamResource
DocCtrl-->>User: ResponseEntity (attachment)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
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 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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/templates/createincident.html (1)
111-114:⚠️ Potential issue | 🟡 MinorCategory validation error message is misplaced.
The error message for
incidentCategoryvalidation is positioned after the file input, visually disconnecting it from the category dropdown. Move it directly below the<select>element.🐛 Proposed fix
<select th:field="*{incidentCategory}"> <option value="">-- Select Category --</option> <option value="OTHER">Other</option> <option value="LAUNDRY_ROOM">Laundry room</option> <option value="NOISE_DISTURBANCE">Noise Disturbance</option> <option value="DAMAGE">Damage</option> </select> + <div th:if="${`#fields.hasErrors`('incidentCategory')}" + th:errors="*{incidentCategory}" + style="color: red;"> + </div> <!--FILES--> <label>Upload file:</label> <input type="file" name="files" multiple/> - <div th:if="${`#fields.hasErrors`('incidentCategory')}" - th:errors="*{incidentCategory}" - style="color: red;"> - </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/createincident.html` around lines 111 - 114, The validation error div for incidentCategory is placed after the file input and should be moved so it appears directly under the category dropdown; locate the <select> bound to incidentCategory (the element using th:field or name incidentCategory) and cut the existing <div th:if="${`#fields.hasErrors`('incidentCategory')}" th:errors="*{incidentCategory}" ...> block and paste it immediately after that <select> element so the error message is visually associated with the category control.
🧹 Nitpick comments (4)
src/main/java/org/example/team6backend/document/service/DocumentService.java (2)
51-54: Fix misleading parameter name.The parameter is named
incidentIdbut its type isIncident. This is confusing—rename it toincidentto match its actual type.Suggested fix
/** Fetch all files connected to one incident */ - public List<Document> getDocumentsByIncident(Incident incidentId) { - return documentRepository.findByIncident(incidentId); + public List<Document> getDocumentsByIncident(Incident incident) { + return documentRepository.findByIncident(incident); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/DocumentService.java` around lines 51 - 54, The method getDocumentsByIncident has a misleading parameter name: incidentId is typed as Incident; rename the parameter to incident to reflect its actual type and update its Javadoc and internal usage (documentRepository.findByIncident(incident)) accordingly; also update all call sites and any tests referencing getDocumentsByIncident(Incident) to use the new parameter name to keep signatures consistent.
45-49: Consider handling partial failure in deleteFile.If
s3Service.deleteFile()succeeds butdocumentRepository.delete()fails, the database record will point to a non-existent S3 object. Consider wrapping in a try-catch to handle this edge case, or documenting the expected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/DocumentService.java` around lines 45 - 49, The current deleteFile(Document) method can leave the DB pointing at a missing S3 object if s3Service.deleteFile(document.getFileKey()) succeeds but documentRepository.delete(document) fails; change the flow to avoid partial failure by either performing the database delete inside a transaction first and then calling s3Service.deleteFile(), or by wrapping the two calls in a try-catch and compensating on failure (e.g., if repository.delete(...) fails after S3 deletion, attempt to restore the S3 object or log and surface a retryable error). Update the deleteFile(Document) implementation to use a transactional repository delete (or explicit rollback/compensation) and handle exceptions from both s3Service.deleteFile(...) and documentRepository.delete(...) so the system does not end up with a dangling reference.src/main/java/org/example/team6backend/page/PageController.java (1)
108-113: Consider handling file upload failures gracefully.If
documentService.uploadFile()throws an exception after the incident is saved, the user will see an error but the incident will already exist without the attached file. Consider catching the exception and either redirecting with an error message or rolling back the entire operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/page/PageController.java` around lines 108 - 113, The controller currently calls incidentService.createIncident(incident) then documentService.uploadFile(files, saved) without handling upload failures; wrap the upload call in a try-catch around documentService.uploadFile(files, saved) (in PageController) and on exception perform a compensating action such as calling incidentService.deleteIncident(saved.getId()) (or mark the saved incident as failed) and then redirect with an error indicator (use RedirectAttributes or append an error query param) while logging the exception; alternatively, annotate the controller method or a service-level method with `@Transactional` and move both createIncident and uploadFile into that transactional boundary if upload is transactional-capable so the create will roll back automatically.src/main/java/org/example/team6backend/document/entity/Document.java (1)
13-24: Consider adding column constraints.The entity lacks constraints that could prevent data integrity issues:
fileKeyshould likely beunique = trueandnullable = falsesince it's the S3 object identifierfileNameandcontentTypecould benefit fromlengthconstraints to prevent truncation- The
incidentrelationship might needoptional = falseif documents must always belong to an incidentExample constraints
- `@Column`(name = "file_key") + `@Column`(name = "file_key", nullable = false, unique = true) private String fileKey;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/entity/Document.java` around lines 13 - 24, Update the Document entity fields to enforce DB constraints: mark fileKey column as unique and nullable = false (Column name "file_key" on field fileKey), set length limits on fileName and contentType via their `@Column` annotations (e.g., specify length attributes on fileName and contentType), and make the incident relation required by setting `@ManyToOne`(optional = false) and `@JoinColumn`(nullable = false) on the incident field; also consider marking fileSize nullable = false if a size is always expected. Ensure you only change the annotations on fields fileKey, fileName, contentType, fileSize, and incident in class Document.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pom.xml`:
- Around line 107-111: The pom declares an explicit OkHttp dependency
(artifactId "okhttp" version 4.11.0) which downgrades the OkHttp used by MinIO
SDK 8.6.0; either remove this explicit dependency so MinIO's transitive OkHttp
5.1.0 is used, or update the okhttp dependency version to 5.1.0 to match
MinIO—adjust the dependency entry for artifactId "okhttp" accordingly or delete
that dependency block so the project uses the transitive version.
In `@src/main/java/org/example/team6backend/config/MinioConfig.java`:
- Around line 20-23: The minioClient() method in MinioConfig has a Spotless
formatting/indentation violation; run mvn spotless:apply and reformat the method
to match project style (correct indentation and brace placement) so the method
signature and body align with the rest of the codebase; ensure
MinioConfig.minioClient() uses the project's expected whitespace and indentation
rules and re-run CI to confirm the Spotless check passes.
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Line 54: The code in DocumentController uses
documentService.getDocumentsByIncident(incident).get(0) which will throw
IndexOutOfBoundsException when the returned list is empty; update the handler in
DocumentController to first assign the result to a List<Document> (import
java.util.List if missing), check list.isEmpty() (or use
list.stream().findFirst()) and handle the empty case (return appropriate
response or throw a controlled exception) before accessing element 0, and
replace the direct .get(0) with a safe access to avoid the unchecked list
access.
- Around line 48-50: The `@GetMapping` path variable name and the method parameter
in downloadFile are mismatched: the route uses "{id}" but the parameter is
incidentId, causing Spring binding to fail; fix by making them consistent—either
rename the parameter to Long id, change the mapping to "/download/{incidentId}",
or annotate the parameter with `@PathVariable`("id") (e.g., `@PathVariable`("id")
Long incidentId); ensure the chosen name matches the intended semantics
(incident ID vs document ID) across the controller.
In `@src/main/java/org/example/team6backend/document/entity/Document.java`:
- Around line 1-73: The Document class has Spotless formatting violations around
the field annotations and method bodies; run mvn spotless:apply to auto-fix
formatting, then verify consistent indentation for the `@Column` annotations on
fileName, contentType, fileKey, fileSize and the bodies of getters/setters
(e.g., getFileName, setFileName, getFileSize, setFileSize) in the Document class
so the annotations and method blocks follow the project's formatting rules
before committing.
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Line 23: The fileKey in DocumentService is built with spaces (" _ ") which can
create URL-encoding problems; update the construction of fileKey (the UUID +
file.getOriginalFilename() expression) to use a separator without spaces (e.g.,
"_" or "-" or no separator) and ensure any concatenation in the method that
references fileKey (including upload or S3 putObject calls) uses the new
separator consistently so S3 object keys contain no spaces.
In `@src/main/java/org/example/team6backend/document/service/S3Service.java`:
- Around line 1-63: Reformat S3Service to satisfy Spotless: run mvn
spotless:apply and commit the changes, ensuring the `@PostConstruct` init() method
and surrounding blocks follow project formatting (proper indentation, spaces
around parentheses and braces, and consistent alignment for the try/catch and
if/else blocks). Specifically check the init() method, the try/catch that throws
RuntimeException, and the MinioClient calls (bucketExists, makeBucket) so method
chaining and builder calls are wrapped/indented per Spotless rules; re-run
Spotless and the build to confirm no formatting violations remain.
In `@src/main/java/org/example/team6backend/incident/entity/Incident.java`:
- Around line 44-45: Add missing accessors and enable orphan removal for the
documents collection: update the Incident entity's documents field declaration
to include orphanRemoval = true on the `@OneToMany` annotation (i.e.,
`@OneToMany`(mappedBy = "incident", cascade = CascadeType.ALL, orphanRemoval =
true)) and add public List<Document> getDocuments() and public void
setDocuments(List<Document> documents) methods so the documents can be
accessed/modified by DTOs/serializers and removed children are deleted
automatically.
In `@src/main/java/org/example/team6backend/page/PageController.java`:
- Around line 91-92: The file-upload parameter in PageController's endpoint is
declared as a single MultipartFile (parameter name files) while the form allows
multiple files; change the parameter type to List<MultipartFile> (or
MultipartFile[]) for the files argument in the controller method signature, then
update the upload logic in the same method (where files is processed) to iterate
over the collection (handle null/empty, validate each file, and call the
existing storage/upload routines for each MultipartFile) so all selected files
are processed instead of only the first.
In `@src/main/resources/db/migration/V8__create_document_table.sql`:
- Around line 1-14: The DB migration adds ON DELETE CASCADE for document rows
but leaves S3/MinIO files orphaned and duplicates cascading behavior already
present on the JPA side; fix by adding a cleanup hook and clarifying cascade
strategy: implement a `@PreRemove` method on the Document entity that calls your
object-storage client to delete the file_key from S3/MinIO (or register an
EntityListener) so files are removed when Document is deleted, and choose one
cascade approach—either remove ON DELETE CASCADE from the
V8__create_document_table.sql migration or remove the JPA cascade (e.g.,
Incident.documents cascade = CascadeType.ALL) and prefer JPA cascade with
orphanRemoval = true for finer control.
In `@src/main/resources/templates/createincident.html`:
- Around line 108-110: The file input in the template allows multiple files but
the handler PageController.submitIncident only accepts a single MultipartFile;
update the backend to accept MultipartFile[] (change the `@RequestParam` in
PageController.submitIncident to MultipartFile[] files), then iterate over that
array and call the existing upload logic in DocumentService for each file (or
adapt DocumentService to accept multiple files) so all uploaded files are
processed; alternatively remove the multiple attribute from the input if you
prefer single-file handling.
In `@src/main/resources/templates/dashboard.html`:
- Line 487: The dashboard's incident item onclick uses the wrong route and will
404; update the onclick in the incident-item element (the DIV with class
"incident-item" and onclick currently set to "/incident/${i.id}") to use the
same route as PageController ("/incidents/${i.id}"), or alternatively change the
controller mapping to accept "/incident/{id}"—preferably change the onclick to
window.location.href='/incidents/${i.id}' so it matches PageController's
/incidents/{id}.
In `@src/main/resources/templates/incidents.html`:
- Around line 172-174: The viewIncident function in the template navigates to
the singular route `/incident/${id}` which mismatches PageController's mapping
`/incidents/{id}`; update the navigation target in the viewIncident function to
use the plural path `/incidents/${id}` so it matches the PageController mapping
and avoids 404s.
---
Outside diff comments:
In `@src/main/resources/templates/createincident.html`:
- Around line 111-114: The validation error div for incidentCategory is placed
after the file input and should be moved so it appears directly under the
category dropdown; locate the <select> bound to incidentCategory (the element
using th:field or name incidentCategory) and cut the existing <div
th:if="${`#fields.hasErrors`('incidentCategory')}" th:errors="*{incidentCategory}"
...> block and paste it immediately after that <select> element so the error
message is visually associated with the category control.
---
Nitpick comments:
In `@src/main/java/org/example/team6backend/document/entity/Document.java`:
- Around line 13-24: Update the Document entity fields to enforce DB
constraints: mark fileKey column as unique and nullable = false (Column name
"file_key" on field fileKey), set length limits on fileName and contentType via
their `@Column` annotations (e.g., specify length attributes on fileName and
contentType), and make the incident relation required by setting
`@ManyToOne`(optional = false) and `@JoinColumn`(nullable = false) on the incident
field; also consider marking fileSize nullable = false if a size is always
expected. Ensure you only change the annotations on fields fileKey, fileName,
contentType, fileSize, and incident in class Document.
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 51-54: The method getDocumentsByIncident has a misleading
parameter name: incidentId is typed as Incident; rename the parameter to
incident to reflect its actual type and update its Javadoc and internal usage
(documentRepository.findByIncident(incident)) accordingly; also update all call
sites and any tests referencing getDocumentsByIncident(Incident) to use the new
parameter name to keep signatures consistent.
- Around line 45-49: The current deleteFile(Document) method can leave the DB
pointing at a missing S3 object if s3Service.deleteFile(document.getFileKey())
succeeds but documentRepository.delete(document) fails; change the flow to avoid
partial failure by either performing the database delete inside a transaction
first and then calling s3Service.deleteFile(), or by wrapping the two calls in a
try-catch and compensating on failure (e.g., if repository.delete(...) fails
after S3 deletion, attempt to restore the S3 object or log and surface a
retryable error). Update the deleteFile(Document) implementation to use a
transactional repository delete (or explicit rollback/compensation) and handle
exceptions from both s3Service.deleteFile(...) and
documentRepository.delete(...) so the system does not end up with a dangling
reference.
In `@src/main/java/org/example/team6backend/page/PageController.java`:
- Around line 108-113: The controller currently calls
incidentService.createIncident(incident) then documentService.uploadFile(files,
saved) without handling upload failures; wrap the upload call in a try-catch
around documentService.uploadFile(files, saved) (in PageController) and on
exception perform a compensating action such as calling
incidentService.deleteIncident(saved.getId()) (or mark the saved incident as
failed) and then redirect with an error indicator (use RedirectAttributes or
append an error query param) while logging the exception; alternatively,
annotate the controller method or a service-level method with `@Transactional` and
move both createIncident and uploadFile into that transactional boundary if
upload is transactional-capable so the create will roll back automatically.
🪄 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: 6643402f-925b-4084-a365-1e8113f683ad
📒 Files selected for processing (13)
pom.xmlsrc/main/java/org/example/team6backend/config/MinioConfig.javasrc/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/entity/Document.javasrc/main/java/org/example/team6backend/document/repository/DocumentRepository.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/document/service/S3Service.javasrc/main/java/org/example/team6backend/incident/entity/Incident.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/db/migration/V8__create_document_table.sqlsrc/main/resources/templates/createincident.htmlsrc/main/resources/templates/dashboard.htmlsrc/main/resources/templates/incidents.html
- Fixed multiple file upload handling by changing controller to accept List<MultipartFile>
- Added validation to skip empty files during upload
- Fixed path variable mismatch in DocumentController download endpoint
- Prevented IndexOutOfBoundsException when no documents exist for an incident
- Updated redirects to consistently use /incidents/{id}
- Added proper deletion flow to ensure files are removed from MinIO/S3 via DocumentService
- Improved robustness of file handling and error handling across controllers
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/org/example/team6backend/page/PageController.java (1)
112-118: Consider handling upload failures gracefully.If
documentService.uploadFile()throws during the loop, the incident is already persisted but the user sees an error page with no indication of partial success. They lose the uploaded files but the incident exists.This may be acceptable given users can upload files later via the document upload endpoint. If you want more resilience, wrap individual uploads in try-catch and collect failures to display.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/page/PageController.java` around lines 112 - 118, The loop in PageController that calls documentService.uploadFile(file, saved) should handle per-file failures so a thrown exception on one file doesn't abort all uploads or surface as a generic error page after the incident is persisted; wrap each call to documentService.uploadFile(...) in a try-catch, record failed file names or exceptions (e.g., into a List<String> failedFiles or Map<MultipartFile, Exception>), continue processing remaining files, and then surface the partial success/failure information to the user (or add it to the model) so callers know which uploads failed and can retry; reference the files variable and saved incident when implementing this change.src/main/java/org/example/team6backend/document/controller/DocumentController.java (1)
48-66: Past issues addressed. Consider endpoint design for multiple documents.The path variable mismatch and empty-list handling from previous reviews are fixed. However, the endpoint only downloads the first document when an incident may have multiple files. Users cannot select which file to download.
Option 1: Download by document ID instead of incident ID
- `@GetMapping`("/download/{incidentId}") - public ResponseEntity<InputStreamResource> downloadFile(`@PathVariable` Long incidentId, + `@GetMapping`("/download/{documentId}") + public ResponseEntity<InputStreamResource> downloadFile(`@PathVariable` Long documentId, `@AuthenticationPrincipal` CustomUserDetails userDetails) { AppUser user = userDetails.getUser(); - Incident incident = incidentService.getById(incidentId, user); - - List<Document> documents = documentService.getDocumentsByIncident(incident); - if (documents.isEmpty()) { - return ResponseEntity.notFound().build(); - } - Document document = documents.get(0); + Document document = documentService.getDocumentById(documentId); // Add this method + // Validate user has access to the document's incident + incidentService.getById(document.getIncident().getId(), user);Additionally, sanitize the filename in the
Content-Dispositionheader to prevent header injection if filenames contain quotes or newlines:.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + document.getFileName().replaceAll("[\"\\r\\n]", "_") + "\"")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/controller/DocumentController.java` around lines 48 - 66, The download endpoint in DocumentController.downloadFile currently picks the first item from documentService.getDocumentsByIncident(incident) which prevents selecting among multiple documents and risks header injection; change the API to accept a document identifier (e.g., documentId path variable or query param) and call documentService.getById(documentId, user) (or lookup the chosen Document) instead of always using documents.get(0), and sanitize the filename used in the Content-Disposition header by replacing quotes and newline characters (e.g., replaceAll for [\"\r\n] to underscores) before inserting it into the header.
🤖 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/team6backend/incident/service/IncidentService.java`:
- Around line 92-98: The deleteIncident method currently calls
documentService.getDocumentsByIncident and loops documentService.deleteFile for
S3 cleanup then calls incidentRepository.delete without transaction or failure
handling; make it transactional by annotating deleteIncident with `@Transactional`
(on IncidentService) and change the flow to first attempt S3 deletions for each
Document via documentService.deleteFile while collecting any failures (e.g.,
keep a list of failed Document ids/exceptions), and only call
incidentRepository.delete(incident) if all S3 deletions succeed (or
alternatively throw a custom exception to abort and allow transaction rollback);
ensure you log/propagate S3 errors clearly so callers can decide whether to
retry or abort, and preserve JPA cascade (Incident entity with cascade =
CascadeType.ALL, orphanRemoval = true) instead of manually deleting DB
documents.
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 48-66: The download endpoint in DocumentController.downloadFile
currently picks the first item from
documentService.getDocumentsByIncident(incident) which prevents selecting among
multiple documents and risks header injection; change the API to accept a
document identifier (e.g., documentId path variable or query param) and call
documentService.getById(documentId, user) (or lookup the chosen Document)
instead of always using documents.get(0), and sanitize the filename used in the
Content-Disposition header by replacing quotes and newline characters (e.g.,
replaceAll for [\"\r\n] to underscores) before inserting it into the header.
In `@src/main/java/org/example/team6backend/page/PageController.java`:
- Around line 112-118: The loop in PageController that calls
documentService.uploadFile(file, saved) should handle per-file failures so a
thrown exception on one file doesn't abort all uploads or surface as a generic
error page after the incident is persisted; wrap each call to
documentService.uploadFile(...) in a try-catch, record failed file names or
exceptions (e.g., into a List<String> failedFiles or Map<MultipartFile,
Exception>), continue processing remaining files, and then surface the partial
success/failure information to the user (or add it to the model) so callers know
which uploads failed and can retry; reference the files variable and saved
incident when implementing this change.
🪄 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: dc1889f5-e478-4aaa-abb2-130b8810e438
📒 Files selected for processing (7)
pom.xmlsrc/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/incident/entity/Incident.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/templates/dashboard.html
✅ Files skipped from review due to trivial changes (1)
- pom.xml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/resources/templates/dashboard.html
- src/main/java/org/example/team6backend/incident/entity/Incident.java
- src/main/java/org/example/team6backend/document/service/DocumentService.java
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/team6backend/page/PageController.java`:
- Around line 111-119: The controller currently calls
incidentService.createIncident(incident) before looping
documentService.uploadFile(file, saved), so a single upload failure can leave a
persisted incident with partial/no documents; fix by moving orchestration into a
service method (e.g., IncidentService.createIncidentWithDocuments(Incident
incident, List<MultipartFile> files)) that either (1) is annotated
`@Transactional` for DB changes and uploads document metadata inside the same
transaction and throws on failure so the incident is rolled back, or (2) if
storage is external/non-transactional, explicitly catch exceptions from
documentService.uploadFile, delete any already-uploaded files via
documentService.deleteDocument(...) and remove the saved incident via
incidentService.deleteIncident(...) before rethrowing or returning a
partial-success result; update the controller to call this single service method
(replace createIncident + upload loop with createIncidentWithDocuments) and
ensure errors return a clear partial/failed response.
- Around line 94-102: The POST validation-error branch returns "createincident"
but never puts the CSRF token into the model, causing the template to break; in
the controller method that accepts `@Valid` `@ModelAttribute` IncidentRequest (the
method using BindingResult and Model), retrieve the CSRF token (e.g. from
HttpServletRequest.getAttribute("_csrf") or via a CsrfToken method argument) and
call model.addAttribute("_csrf", csrfToken) before returning "createincident" so
the template has the same _csrf model attribute the GET handler provides.
🪄 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: 9d01de4f-ea6f-4549-aa95-316f17b96941
📒 Files selected for processing (3)
src/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/templates/dashboard.html
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/templates/dashboard.html
- src/main/java/org/example/team6backend/incident/service/IncidentService.java
There was a problem hiding this comment.
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/team6backend/incident/service/IncidentService.java (1)
35-51:⚠️ Potential issue | 🔴 CriticalCritical syntax error: Malformed constructor structure.
The constructor starting at line 37 is missing its closing brace
}. This causes the field declarations at lines 42-43 to be parsed as statements inside the constructor body, resulting in the "illegal start of expression" error reported by the pipeline.Additionally, there appear to be two conflicting constructors:
- Lines 37-41: Initializes
incidentRepository,activityLogService,documentService— but NOTuserRepositoryornotificationService.- Lines 45-51: Initializes
incidentRepository,activityLogService,userRepository,notificationService— but NOTdocumentService.This looks like an incomplete merge. You need a single constructor that initializes all five dependencies.
🐛 Proposed fix: Merge into a single constructor
private final IncidentRepository incidentRepository; private final ActivityLogService activityLogService; private final DocumentService documentService; + private final AppUserRepository userRepository; + private final NotificationService notificationService; - public IncidentService(IncidentRepository incidentRepository, ActivityLogService activityLogService, - DocumentService documentService) { - this.incidentRepository = incidentRepository; - this.activityLogService = activityLogService; - this.documentService = documentService; - private final AppUserRepository userRepository; - private final NotificationService notificationService; - - public IncidentService(IncidentRepository incidentRepository, ActivityLogService activityLogService, - AppUserRepository userRepository, NotificationService notificationService) { + public IncidentService(IncidentRepository incidentRepository, ActivityLogService activityLogService, + DocumentService documentService, AppUserRepository userRepository, NotificationService notificationService) { this.incidentRepository = incidentRepository; this.activityLogService = activityLogService; + this.documentService = documentService; this.userRepository = userRepository; this.notificationService = notificationService; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 35 - 51, The IncidentService class has a malformed/duplicated constructor: the first constructor that sets incidentRepository, activityLogService and documentService is missing a closing brace and a second constructor initializes userRepository and notificationService separately; fix this by merging into a single IncidentService constructor that closes properly and accepts/assigns all five dependencies (incidentRepository, activityLogService, documentService, userRepository, notificationService), remove the duplicate/incorrect constructor and ensure all corresponding fields (documentService, userRepository, notificationService, incidentRepository, activityLogService) are assigned.
♻️ Duplicate comments (1)
src/main/java/org/example/team6backend/incident/service/IncidentService.java (1)
107-113:⚠️ Potential issue | 🔴 CriticalMissing closing brace and
@Transactionalannotation.The
deleteIncidentmethod is missing its closing brace}on line 113, which will cause a compilation error.Additionally, as noted in a previous review, this method performs multiple operations (S3 deletions + DB deletion) without transaction management or error handling. If
documentService.deleteFile()fails mid-loop, partial deletions occur with no rollback. SinceDocumentService.deleteFile()performs both S3 deletion and DB record removal, failures can leave inconsistent state.🐛 Proposed fix: Add closing brace, `@Transactional`, and error handling
+ `@Transactional` public void deleteIncident(Incident incident) { List<Document> documents = documentService.getDocumentsByIncident(incident); for (Document document : documents) { - documentService.deleteFile(document); + try { + documentService.deleteFile(document); + } catch (RuntimeException e) { + log.error("Failed to delete file {} from S3, continuing with deletion", document.getFileKey(), e); + } } incidentRepository.delete(incident); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 107 - 113, Add the missing closing brace for deleteIncident and make the method transactional and failure-safe: close the method body, annotate deleteIncident with `@Transactional`, then ensure you perform S3 deletions via documentService.deleteFile(document) and if any call fails propagate a runtime exception (or collect and throw a RuntimeException) so the transaction rolls back; keep the incidentRepository.delete(incident) inside the same transactional method (deleteIncident) so DB removals rollback on error — reference deleteIncident, documentService.deleteFile, and incidentRepository.delete when making the changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 35-51: The IncidentService class has a malformed/duplicated
constructor: the first constructor that sets incidentRepository,
activityLogService and documentService is missing a closing brace and a second
constructor initializes userRepository and notificationService separately; fix
this by merging into a single IncidentService constructor that closes properly
and accepts/assigns all five dependencies (incidentRepository,
activityLogService, documentService, userRepository, notificationService),
remove the duplicate/incorrect constructor and ensure all corresponding fields
(documentService, userRepository, notificationService, incidentRepository,
activityLogService) are assigned.
---
Duplicate comments:
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 107-113: Add the missing closing brace for deleteIncident and make
the method transactional and failure-safe: close the method body, annotate
deleteIncident with `@Transactional`, then ensure you perform S3 deletions via
documentService.deleteFile(document) and if any call fails propagate a runtime
exception (or collect and throw a RuntimeException) so the transaction rolls
back; keep the incidentRepository.delete(incident) inside the same transactional
method (deleteIncident) so DB removals rollback on error — reference
deleteIncident, documentService.deleteFile, and incidentRepository.delete when
making the changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29b81a95-b555-4cb9-9e2e-974cf5e21646
📒 Files selected for processing (1)
src/main/java/org/example/team6backend/incident/service/IncidentService.java
- Move incident creation logic from controller to service layer - Remove SecurityContext usage from IncidentService for cleaner separation of concerns - Centralize incident + document creation into a single transactional service method - Align controller methods with service-based architecture - Improve file upload handling and ensure proper iteration over multipart files - Fix potential issues with partial uploads and inconsistent incident state - Improve deleteIncident logic by handling document cleanup and external storage deletion - Resolve CodeRabbit findings related to architecture, consistency, and robustness - Improve overall separation between controller, service, and persistence layers
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/team6backend/incident/service/IncidentService.java (1)
117-124:⚠️ Potential issue | 🟠 MajorDo not continue incident deletion after S3 delete failures.
Current flow logs and continues when file deletion fails, which can leave orphaned objects in external storage.
Proposed fix
`@Transactional` public void deleteIncident(Incident incident) { for (Document document : incident.getDocuments()) { - try { - s3Service.deleteFile(document.getFileKey()); - } catch (Exception e) { - log.warn("Failed to delete file from S3: " + document.getFileKey(), e); - } + s3Service.deleteFile(document.getFileKey()); } incidentRepository.delete(incident); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 117 - 124, The loop in IncidentService that calls s3Service.deleteFile(document.getFileKey()) must not allow incidentRepository.delete(incident) to run if any S3 deletion fails; modify the code in the block iterating incident.getDocuments() so that on any exception from s3Service.deleteFile you log the failure (including document.getFileKey()) and then abort by rethrowing an appropriate runtime or domain-specific exception (or return a failure result) instead of swallowing it, ensuring incidentRepository.delete(incident) is only reached when all document deletes succeed.
🧹 Nitpick comments (1)
src/main/java/org/example/team6backend/document/service/DocumentService.java (1)
52-53: Rename method parameter for clarity.
Incident incidentIdis misleading. This should be namedincidentto match the actual type and repository call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/DocumentService.java` around lines 52 - 53, Rename the misleading parameter in the method getDocumentsByIncident from incidentId to incident to reflect its actual type; update the method signature and any internal references (including the call to documentRepository.findByIncident) so the parameter name is incident throughout the getDocumentsByIncident method.
🤖 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/team6backend/document/service/DocumentService.java`:
- Around line 21-37: The uploadFile method in DocumentService can leave an
orphaned S3 object if s3Service.uploadFile(fileKey, file) succeeds but
documentRepository.save(document) fails; modify DocumentService.uploadFile to
perform compensating cleanup by deleting the uploaded S3 object (call
s3Service.deleteFile(fileKey) or equivalent) when persistence fails: after
creating the Document and before/while calling documentRepository.save(document)
wrap the save in a try/catch, and on any exception call
s3Service.deleteFile(fileKey) (catch and log any deletion error) and then
rethrow the original exception to preserve error context; reference the
uploadFile method, s3Service.uploadFile, documentRepository.save, and the
generated fileKey.
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 74-79: The file-iteration in IncidentService is inverted: inside
the loop over MultipartFile files the code uploads only when file.isEmpty();
change the condition to upload non-empty files by calling
documentService.uploadFile(savedIncident) only when !file.isEmpty(). Locate the
loop that references files, MultipartFile, file.isEmpty(),
documentService.uploadFile(...) and savedIncident and replace the conditional so
non-empty files are uploaded while empty ones are skipped.
---
Duplicate comments:
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 117-124: The loop in IncidentService that calls
s3Service.deleteFile(document.getFileKey()) must not allow
incidentRepository.delete(incident) to run if any S3 deletion fails; modify the
code in the block iterating incident.getDocuments() so that on any exception
from s3Service.deleteFile you log the failure (including document.getFileKey())
and then abort by rethrowing an appropriate runtime or domain-specific exception
(or return a failure result) instead of swallowing it, ensuring
incidentRepository.delete(incident) is only reached when all document deletes
succeed.
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 52-53: Rename the misleading parameter in the method
getDocumentsByIncident from incidentId to incident to reflect its actual type;
update the method signature and any internal references (including the call to
documentRepository.findByIncident) so the parameter name is incident throughout
the getDocumentsByIncident method.
🪄 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: 774d6f85-ce6a-4c01-8256-2555d16bafcc
📒 Files selected for processing (6)
src/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/incident/controller/IncidentController.javasrc/main/java/org/example/team6backend/incident/entity/Incident.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.javasrc/main/java/org/example/team6backend/page/PageController.javasrc/main/resources/db/migration/V8__create_document_table.sql
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/db/migration/V8__create_document_table.sql
- src/main/java/org/example/team6backend/incident/entity/Incident.java
- Added S3 cleanup on failed DB save to prevent orphan files - Guarded cleanup with upload flag - Moved incident + file handling to transactional flow - Fixed empty file upload condition
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/main/java/org/example/team6backend/document/service/DocumentService.java (1)
59-62:⚠️ Potential issue | 🟠 MajorAvoid deleting S3 before metadata delete to prevent broken document references.
At Line 60 then Line 61, if S3 deletion succeeds but DB deletion fails, the
Documentrow can remain while the file is already gone.Proposed change
public void deleteFile(Document document) { - s3Service.deleteFile(document.getFileKey()); - documentRepository.delete(document); + String fileKey = document.getFileKey(); + documentRepository.delete(document); + try { + s3Service.deleteFile(fileKey); + } catch (Exception e) { + log.warn("Document metadata deleted but failed to remove S3 object: {}", fileKey, e); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/document/service/DocumentService.java` around lines 59 - 62, The current deleteFile deletes the S3 object before removing DB metadata, which can leave a DB row referencing a missing file if DB deletion fails; in DocumentService.deleteFile, call documentRepository.delete(document) first (inside an appropriate transaction) and then call s3Service.deleteFile(document.getFileKey()), and add try/catch around the S3 delete to log or schedule retries/compensation if S3 fails so you don't lose metadata or leave silent failures.
🤖 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/team6backend/incident/service/IncidentService.java`:
- Line 81: The activity log message in IncidentService (the call to
activityLogService.log in the incident creation flow) concatenates
user.getName() and "created incident" without a space; update the log message
construction in the IncidentService method that calls
activityLogService.log("INCIDENT_CREATED", ...) so it inserts a space (e.g., by
appending " created incident" or using String.format/concat) to produce
"UserName created incident" and then pass the corrected message along with
savedIncident and user.
- Around line 74-80: Wrap the file-upload loop in IncidentService in a try/catch
and implement compensating deletes for already-uploaded files: change calls to
documentService.uploadFile(file, savedIncident) to capture a return value (e.g.,
stored key or Document id) into a list, and if any subsequent upload throws,
iterate that list and call the documentService delete method (e.g.,
documentService.deleteFile(key) or documentService.deleteDocument(id)) to remove
S3 objects and/or DB records, then rethrow or translate the exception; ensure
uploadFile’s signature is updated to return the uploaded identifier if needed
and use savedIncident as context when cleaning up.
---
Duplicate comments:
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 59-62: The current deleteFile deletes the S3 object before
removing DB metadata, which can leave a DB row referencing a missing file if DB
deletion fails; in DocumentService.deleteFile, call
documentRepository.delete(document) first (inside an appropriate transaction)
and then call s3Service.deleteFile(document.getFileKey()), and add try/catch
around the S3 delete to log or schedule retries/compensation if S3 fails so you
don't lose metadata or leave silent failures.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1ef52393-2743-4f3b-b44e-0cfb6cb9ebdb
📒 Files selected for processing (2)
src/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/incident/service/IncidentService.java
- Added cleanup of previously uploaded files if later upload fails - Prevents orphaned files in S3/MinIO - Fixed logging
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/team6backend/incident/service/IncidentService.java`:
- Around line 61-99: The createIncident method can leave uploaded S3 objects
orphaned if an exception occurs after uploads (e.g., activityLogService.log or
transaction commit); move compensating cleanup out of the upload loop and tie it
to the transaction lifecycle: after calling incidentRepository.save and starting
uploads via documentService.uploadFile (which uploads to S3 before persisting),
register a TransactionSynchronization (e.g.,
TransactionSynchronizationManager.registerSynchronization) or equivalent in
createIncident that on afterCompletion checks the transaction status and on
rollback iterates uploadedKeys and calls s3Service.deleteFile(key);
alternatively wrap the entire method (after the first save) in a try/catch that
on any exception performs the same uploadedKeys cleanup and rethrows; reference
createIncident, documentService.uploadFile, s3Service.deleteFile,
incidentRepository.save, activityLogService.log, and
TransactionSynchronization.afterCompletion when implementing.
- Around line 130-140: The deleteIncident method currently iterates
incident.getDocuments() while relying on lazy loading, which will throw
LazyInitializationException for detached entities; update deleteIncident to load
the Incident with its documents inside the `@Transactional` boundary (e.g., change
signature to deleteIncident(Long incidentId) and call
incidentRepository.findIncidentWithDocumentsById(incidentId).orElseThrow(ResourceNotFoundException::new))
or otherwise explicitly initialize the documents collection inside the
transaction before iterating; then perform the
s3Service.deleteFile(document.getFileKey()) cleanup and
incidentRepository.delete(incident) as before.
🪄 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: 5bdea702-6158-4b19-b432-35a499a99b27
📒 Files selected for processing (1)
src/main/java/org/example/team6backend/incident/service/IncidentService.java
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/main/java/org/example/team6backend/incident/service/IncidentService.java (3)
131-142:⚠️ Potential issue | 🟠 MajorLoad the incident with documents inside this transaction.
deleteIncident(Incident incident)relies onincident.getDocuments()from the caller. With the default lazy@OneToMany, passing a detachedIncidenthere will fail when the collection is accessed. PreferdeleteIncident(Long incidentId)and fetch the incident together with its documents inside this method before iterating.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 131 - 142, The current deleteIncident(Incident incident) accesses incident.getDocuments() which can fail if the Incident is detached; change the API to deleteIncident(Long incidentId) and inside that transactional method load the Incident together with its documents (e.g., via incidentRepository.findByIdWithDocuments(id) or a JPQL/criteria fetch join) before iterating, then call s3Service.deleteFile(document.getFileKey()) for each Document and finally incidentRepository.delete(incident) to remove it; ensure the method stays `@Transactional` and handle/delete missing-incident cases appropriately.
61-99:⚠️ Potential issue | 🟠 MajorTie upload rollback to transaction completion, not just the try/catch.
This cleanup only runs for exceptions thrown before the method returns. If the transaction fails during flush/commit after Line 88, the DB rolls back but the uploaded S3 objects in
uploadedKeysremain orphaned. Register rollback cleanup with the transaction lifecycle (afterCompletion) or move the external upload to an after-commit step.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 61 - 99, The current createIncident method in IncidentService collects uploadedKeys and only cleans them up in the catch block, which misses cleanup if the DB transaction later rolls back during commit; instead, register a TransactionSynchronization via TransactionSynchronizationManager.registerSynchronization inside createIncident (after saving and after each successful document upload) that in afterCompletion (or afterRollback) iterates uploadedKeys and calls s3Service.deleteFile(key) if the transaction rolled back, and remove the try/catch-only cleanup; reference IncidentService.createIncident, the uploadedKeys list, s3Service.deleteFile, documentService.uploadFile, incidentRepository.save and activityLogService.log when adding the synchronization so uploads are rolled back when the transaction fails.
131-141:⚠️ Potential issue | 🟠 MajorFail the incident delete when external file deletion fails.
Logging and continuing here can report a successful incident delete while leaving orphaned objects in S3/MinIO. That also removes the DB references you would need to retry cleanup later. Prefer collecting any delete failures and aborting
incidentRepository.delete(incident)if one occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/incident/service/IncidentService.java` around lines 131 - 141, The deleteIncident method currently logs and continues when s3Service.deleteFile(document.getFileKey()) fails, which can orphan files; update deleteIncident to collect any failures from s3Service.deleteFile for each Document on the Incident and abort the database delete if any deletion failed (e.g., accumulate exceptions or a boolean failure flag and throw a runtime or custom exception) before calling incidentRepository.delete(incident). Ensure you still run all attempted S3 deletions (don’t stop at first failure) so you can report all failing file keys, and surface the error instead of swallowing it so callers know the delete did not succeed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/main/java/org/example/team6backend/incident/service/IncidentService.java`:
- Around line 131-142: The current deleteIncident(Incident incident) accesses
incident.getDocuments() which can fail if the Incident is detached; change the
API to deleteIncident(Long incidentId) and inside that transactional method load
the Incident together with its documents (e.g., via
incidentRepository.findByIdWithDocuments(id) or a JPQL/criteria fetch join)
before iterating, then call s3Service.deleteFile(document.getFileKey()) for each
Document and finally incidentRepository.delete(incident) to remove it; ensure
the method stays `@Transactional` and handle/delete missing-incident cases
appropriately.
- Around line 61-99: The current createIncident method in IncidentService
collects uploadedKeys and only cleans them up in the catch block, which misses
cleanup if the DB transaction later rolls back during commit; instead, register
a TransactionSynchronization via
TransactionSynchronizationManager.registerSynchronization inside createIncident
(after saving and after each successful document upload) that in afterCompletion
(or afterRollback) iterates uploadedKeys and calls s3Service.deleteFile(key) if
the transaction rolled back, and remove the try/catch-only cleanup; reference
IncidentService.createIncident, the uploadedKeys list, s3Service.deleteFile,
documentService.uploadFile, incidentRepository.save and activityLogService.log
when adding the synchronization so uploads are rolled back when the transaction
fails.
- Around line 131-141: The deleteIncident method currently logs and continues
when s3Service.deleteFile(document.getFileKey()) fails, which can orphan files;
update deleteIncident to collect any failures from s3Service.deleteFile for each
Document on the Incident and abort the database delete if any deletion failed
(e.g., accumulate exceptions or a boolean failure flag and throw a runtime or
custom exception) before calling incidentRepository.delete(incident). Ensure you
still run all attempted S3 deletions (don’t stop at first failure) so you can
report all failing file keys, and surface the error instead of swallowing it so
callers know the delete did not succeed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 41d9dd62-33cc-40fd-9ac1-3a342ba5d8d2
📒 Files selected for processing (1)
src/main/java/org/example/team6backend/incident/service/IncidentService.java
This was referenced Apr 13, 2026
Merged
This was referenced Apr 22, 2026
This was referenced Apr 24, 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.
/incidents/{id}after upload and comment creation./incidents/{id}./incidents/{id}./api/incidents./incidentsin UI routes.Summary by CodeRabbit
New Features
Bug Fixes
Chores