Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds S3/MinIO-backed storage: Report gains persisted S3 keys; S3 client and bucket-init config added; service generates/uploads PDFs and optional files, persists S3 keys, exposes downloads and listing; controller and UI updated for multipart uploads and new endpoints. (≈34 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant HomeController
participant ReportService
participant iText as iText_PDF
participant S3Client
participant S3 as S3_Storage
participant Database
Client->>HomeController: POST /reports/add (form + optional file)
HomeController->>ReportService: saveReport(CreateReport, MultipartFile)
ReportService->>iText_PDF: Generate PDF bytes
iText_PDF-->>ReportService: PDF bytes
ReportService->>S3Client: PUT reports/pdf/{uuid}.pdf (PDF bytes)
S3Client->>S3_Storage: Store object
S3_Storage-->>S3Client: OK (object key)
alt file provided
ReportService->>S3Client: PUT reports/files/{uuid}.{ext} (file bytes)
S3Client->>S3_Storage: Store object
S3_Storage-->>S3Client: OK (object key)
end
ReportService->>Database: Save Report(entity with s3KeyPdf,s3KeyFile)
Database-->>ReportService: Persisted
ReportService-->>HomeController: Complete
HomeController-->>Client: Redirect/Response
sequenceDiagram
participant Client
participant HomeController
participant ReportService
participant Database
participant S3Client
participant S3 as S3_Storage
Client->>HomeController: GET /reports/{uuid}/download/pdf
HomeController->>ReportService: downloadPdf(UUID)
ReportService->>Database: Find report by UUID
Database-->>ReportService: Report (s3KeyPdf)
ReportService->>S3Client: GET object (s3KeyPdf)
S3Client->>S3_Storage: Retrieve object
S3_Storage-->>S3Client: File bytes
S3Client-->>ReportService: bytes
ReportService-->>HomeController: ResponseEntity<byte[]> (application/pdf)
HomeController-->>Client: PDF file download
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 6
🤖 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/crimearchive/config/S3Config.java`:
- Around line 45-60: The createBucket ApplicationRunner currently swallows
non-404 S3Exception and only logs to stderr, allowing startup to succeed; change
the catch in S3Config.createBucket so that for non-404 S3Exception you re-throw
the exception (or wrap it in a RuntimeException) instead of just logging,
ensuring the ApplicationRunner fails and Spring stops startup on bucket
access/permission/credential errors.
In `@src/main/java/org/example/crimearchive/controllers/HomeController.java`:
- Around line 44-46: The getAllReports endpoint in HomeController currently
returns Report entities directly (HomeController.getAllReports ->
ResponseEntity<List<Report>>), which exposes internal S3 object names via
Report.getS3KeyPdf() and Report.getS3KeyFile(); fix by either creating and
returning a ReportDTO with only public fields from HomeController.getAllReports
(map Report -> ReportDTO before ResponseEntity.ok) or annotate the Report
accessors getS3KeyPdf and getS3KeyFile with `@JsonIgnore` so those fields are not
serialized in the JSON response. Ensure mapping is applied consistently wherever
Reports are serialized.
In `@src/main/java/org/example/crimearchive/service/ReportService.java`:
- Around line 89-103: In ReportService, update downloadPdf (and similarly
downloadFile) to throw ResponseStatusException(HttpStatus.NOT_FOUND, "...")
instead of RuntimeException when simpleRepository.findById(uuid) returns empty
or when report.getS3KeyPdf() is null, and wrap or catch S3 client errors (e.g.,
S3Exception/NoSuchKey thrown by s3Client.getObjectAsBytes) to rethrow as
ResponseStatusException(HttpStatus.NOT_FOUND, "...") so missing S3 objects
return 404; ensure you add the import for
org.springframework.web.server.ResponseStatusException and HttpStatus and apply
the same pattern in the downloadFile method.
- Around line 42-86: Mark saveReport as transactional (add `@Transactional` on the
method or class) and ensure any S3 uploads are cleaned up if the DB save fails:
after uploading PDF (s3KeyPdf) and optional file (s3KeyFile) via
s3Client.putObject, wrap the simpleRepository.save(…) call in a try/catch (or
use transaction rollback listener) and on failure call s3Client.deleteObject for
s3KeyPdf and s3KeyFile if they were uploaded; also propagate the original
exception as IOException so callers see the failure. Ensure you reference the
existing variables s3KeyPdf, s3KeyFile, s3Client.putObject and
simpleRepository.save when implementing the compensating deletes.
- Line 57: The S3 key construction in ReportService (variables s3KeyPdf and the
use of file.getOriginalFilename()) embeds user input; change to generate opaque
UUID-only keys (e.g., "reports/pdf/{UUID}.pdf" and "reports/uploads/{UUID}") and
stop persisting original filenames into the key. Instead persist a separate
sanitized display name field in the report entity (store the original filename
after sanitizing/truncating) and use that display field when showing download
names. Also ensure the code path that sets the Content-Disposition header (the
code around where the filename is read for the header) performs proper
sanitization/quoting and length truncation before inserting into the header to
prevent header injection and overly long values.
In `@src/main/resources/application.properties`:
- Around line 6-9: Remove hardcoded MinIO endpoint and credentials from
application.properties: delete or blank the values for minio.endpoint,
minio.access-key, and minio.secret-key in the shared default file and instead
read them from environment variables (e.g., MINIO_ENDPOINT, MINIO_ACCESS_KEY,
MINIO_SECRET_KEY) or place them into a local-only Spring profile (e.g.,
application-local.properties) that is not checked into source control; keep
minio.bucket if it is a safe default, otherwise also move it to env/profile and
ensure the code that loads these values (the Spring property binding for
minio.endpoint/minio.access-key/minio.secret-key) still resolves from env or
active profile.
🪄 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: b3aaa37e-ea16-4418-8a21-77a6775f84a8
📒 Files selected for processing (7)
src/main/java/org/example/crimearchive/bevis/Report.javasrc/main/java/org/example/crimearchive/config/S3Config.javasrc/main/java/org/example/crimearchive/controllers/HomeController.javasrc/main/java/org/example/crimearchive/mapper/ReportMapper.javasrc/main/java/org/example/crimearchive/service/ReportService.javasrc/main/resources/application.propertiessrc/main/resources/templates/private.html
src/main/java/org/example/crimearchive/controllers/HomeController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/crimearchive/service/ReportService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/crimearchive/service/ReportService.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/crimearchive/service/ReportService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/example/crimearchive/service/ReportService.java (2)
112-117:⚠️ Potential issue | 🟠 MajorS3
getObjectAsBytesexceptions will result in HTTP 500 instead of 404.If the S3 object doesn't exist or is inaccessible,
s3Client.getObjectAsBytes()throwsS3Exception(orNoSuchKeyException), which isn't caught and will propagate as a 500 error. Wrap in try-catch and convert toResponseStatusExceptionwith 404.Proposed fix
+import software.amazon.awssdk.services.s3.model.S3Exception; ... - ResponseBytes<GetObjectResponse> objectBytes = s3Client.getObjectAsBytes( - GetObjectRequest.builder() - .bucket(bucket) - .key(report.getS3KeyPdf()) - .build() - ); + ResponseBytes<GetObjectResponse> objectBytes; + try { + objectBytes = s3Client.getObjectAsBytes( + GetObjectRequest.builder() + .bucket(bucket) + .key(report.getS3KeyPdf()) + .build() + ); + } catch (S3Exception e) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND, + "PDF-filen kunde inte hämtas från lagring"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/crimearchive/service/ReportService.java` around lines 112 - 117, Wrap the S3 download call in ReportService that invokes s3Client.getObjectAsBytes(...) in a try-catch and convert S3 errors to a 404 ResponseStatusException: catch S3Exception (and NoSuchKeyException if used) around the getObjectAsBytes call and throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Report PDF not found for key: " + report.getS3KeyPdf(), e) so missing/inaccessible objects return 404 instead of propagating a 500.
134-143:⚠️ Potential issue | 🟠 MajorSame S3 exception issue; also missing
Content-Typeheader.
- The
s3Client.getObjectAsBytes()call has the same unhandled exception issue as indownloadPdf.- The response doesn't set a
Content-Type, which may cause browsers to mishandle the file. Consider usingMediaType.APPLICATION_OCTET_STREAMas a fallback or retrieving the content type from the S3 object metadata.Proposed fix
- ResponseBytes<GetObjectResponse> objectBytes = s3Client.getObjectAsBytes( - GetObjectRequest.builder() - .bucket(bucket) - .key(report.getS3KeyFile()) - .build() - ); - - return ResponseEntity.ok() - .header("Content-Disposition", "attachment; filename=fil") - .body(objectBytes.asByteArray()); + ResponseBytes<GetObjectResponse> objectBytes; + try { + objectBytes = s3Client.getObjectAsBytes( + GetObjectRequest.builder() + .bucket(bucket) + .key(report.getS3KeyFile()) + .build() + ); + } catch (S3Exception e) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND, + "Filen kunde inte hämtas från lagring"); + } + + String contentType = objectBytes.response().contentType(); + return ResponseEntity.ok() + .contentType(MediaType.parseMediaType( + contentType != null ? contentType : MediaType.APPLICATION_OCTET_STREAM_VALUE)) + .header("Content-Disposition", "attachment; filename=fil") + .body(objectBytes.asByteArray());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/crimearchive/service/ReportService.java` around lines 134 - 143, Wrap the s3Client.getObjectAsBytes(...) call in the same exception handling used in downloadPdf so S3 errors are caught and translated to a meaningful HTTP response; specifically guard the call that produces ResponseBytes<GetObjectResponse> objectBytes and handle S3Exceptions and IOExceptions around objectBytes.asByteArray(). Also set an explicit Content-Type on the ResponseEntity (use MediaType.APPLICATION_OCTET_STREAM as a fallback or retrieve the content type from the S3 object's metadata/GetObjectResponse) and fix the Content-Disposition header (currently "attachment; filename=fil") to use the real filename (e.g., from report.getS3KeyFile() or a report.getFilename() field) when building the ResponseEntity.
🧹 Nitpick comments (1)
src/main/java/org/example/crimearchive/service/ReportService.java (1)
52-61: iTextDocumentresource not properly closed on failure.If an exception occurs between
document.open()(line 56) anddocument.close()(line 61), the document may not be properly released. Use try-finally or move close to a finally block.Proposed fix with proper resource handling
try { ByteArrayOutputStream pdfStream = new ByteArrayOutputStream(); Document document = new Document(); - PdfWriter.getInstance(document, pdfStream); - document.open(); - document.add(new Paragraph("Brottsanmälan")); - document.add(new Paragraph("Namn: " + report.name())); - document.add(new Paragraph("Brottstyp: " + report.event())); - document.add(new Paragraph("Datum: " + LocalDateTime.now())); - document.close(); + try { + PdfWriter.getInstance(document, pdfStream); + document.open(); + document.add(new Paragraph("Brottsanmälan")); + document.add(new Paragraph("Namn: " + report.name())); + document.add(new Paragraph("Brottstyp: " + report.event())); + document.add(new Paragraph("Datum: " + LocalDateTime.now())); + } finally { + document.close(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/crimearchive/service/ReportService.java` around lines 52 - 61, The try block in ReportService that creates and writes the iText Document (variables pdfStream and document, and call PdfWriter.getInstance(...)) doesn't guarantee document.close() if an exception is thrown; wrap document creation/open/write in a try-finally (or use try-with-resources if using a compatible iText AutoCloseable Document) so that document.close() is always called in the finally block, and ensure pdfStream is also closed/handled after use; update the block around PdfWriter.getInstance, document.open(), document.add(...), and document.close() to guarantee proper closing on failure.
🤖 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/crimearchive/controllers/HomeController.java`:
- Around line 37-43: The saveReport method in HomeController currently lets
IOException from reportService.saveReport propagate, causing HTTP 500; modify
HomeController.saveReport to catch IOException around the
reportService.saveReport(newReport, file) call, add a user-friendly error
message to the model (or flash attributes) and return an appropriate view (e.g.,
an error page or the report form view) instead of throwing; ensure you reference
the same parameters (CreateReport newReport and MultipartFile file) and keep the
successful path returning "redirect:/".
In `@src/main/java/org/example/crimearchive/service/ReportService.java`:
- Around line 90-98: The catch block in ReportService that performs S3 cleanup
(references: variables s3KeyPdf, s3KeyFile and calls
s3Client.deleteObject(DeleteObjectRequest.builder()...)) can throw and mask the
original exception e; update this block to perform each s3Client.deleteObject
call inside its own try-catch (or a single try that catches and logs/suppresses
cleanup exceptions) so that any exception thrown during deletion does not
overwrite or replace the original exception e — ensure the original exception is
rethrown (or logged/propagated) after cleanup attempts and any cleanup errors
are handled separately.
---
Duplicate comments:
In `@src/main/java/org/example/crimearchive/service/ReportService.java`:
- Around line 112-117: Wrap the S3 download call in ReportService that invokes
s3Client.getObjectAsBytes(...) in a try-catch and convert S3 errors to a 404
ResponseStatusException: catch S3Exception (and NoSuchKeyException if used)
around the getObjectAsBytes call and throw new
ResponseStatusException(HttpStatus.NOT_FOUND, "Report PDF not found for key: " +
report.getS3KeyPdf(), e) so missing/inaccessible objects return 404 instead of
propagating a 500.
- Around line 134-143: Wrap the s3Client.getObjectAsBytes(...) call in the same
exception handling used in downloadPdf so S3 errors are caught and translated to
a meaningful HTTP response; specifically guard the call that produces
ResponseBytes<GetObjectResponse> objectBytes and handle S3Exceptions and
IOExceptions around objectBytes.asByteArray(). Also set an explicit Content-Type
on the ResponseEntity (use MediaType.APPLICATION_OCTET_STREAM as a fallback or
retrieve the content type from the S3 object's metadata/GetObjectResponse) and
fix the Content-Disposition header (currently "attachment; filename=fil") to use
the real filename (e.g., from report.getS3KeyFile() or a report.getFilename()
field) when building the ResponseEntity.
---
Nitpick comments:
In `@src/main/java/org/example/crimearchive/service/ReportService.java`:
- Around line 52-61: The try block in ReportService that creates and writes the
iText Document (variables pdfStream and document, and call
PdfWriter.getInstance(...)) doesn't guarantee document.close() if an exception
is thrown; wrap document creation/open/write in a try-finally (or use
try-with-resources if using a compatible iText AutoCloseable Document) so that
document.close() is always called in the finally block, and ensure pdfStream is
also closed/handled after use; update the block around PdfWriter.getInstance,
document.open(), document.add(...), and document.close() to guarantee proper
closing on failure.
🪄 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: 1d294d4c-50b4-4fbe-ba9a-88b83efd0e79
📒 Files selected for processing (4)
src/main/java/org/example/crimearchive/DTO/ReportResponse.javasrc/main/java/org/example/crimearchive/config/S3Config.javasrc/main/java/org/example/crimearchive/controllers/HomeController.javasrc/main/java/org/example/crimearchive/service/ReportService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/crimearchive/DTO/ReportResponse.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/crimearchive/config/S3Config.java
src/main/java/org/example/crimearchive/controllers/HomeController.java
Outdated
Show resolved
Hide resolved
src/main/java/org/example/crimearchive/service/ReportService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/org/example/crimearchive/controllers/HomeController.java (1)
57-65: Add exception handling for S3 client operations in the service layer.The
downloadPdfanddownloadFilemethods (lines 112-122 and 134-139 in ReportService) calls3Client.getObjectAsBytes()without catching S3Exception or SdkClientException. While NOT_FOUND cases are properly handled viaResponseStatusException, infrastructure errors (network failures, bucket access issues) will propagate as unhandled 500 errors, potentially exposing stack traces.Wrap S3 calls in try-catch blocks to return appropriate error responses (e.g., 503 Service Unavailable for infrastructure failures), or add a
@ControllerAdviceto handle S3 exceptions globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/crimearchive/controllers/HomeController.java` around lines 57 - 65, ReportService currently calls s3Client.getObjectAsBytes() from methods downloadPdf and downloadFile without catching S3Exception or SdkClientException, so infrastructure errors bubble up; modify the ReportService implementations of downloadPdf(UUID) and downloadFile(UUID) to wrap the s3Client.getObjectAsBytes(...) calls in try-catch blocks catching software.amazon.awssdk.services.s3.model.S3Exception and software.amazon.awssdk.core.exception.SdkClientException, map S3Exception (e.g., 404 already handled) and other SDK/infrastructure errors to an appropriate ResponseStatusException (for example HttpStatus.SERVICE_UNAVAILABLE with a clear message) or rethrow a custom exception that a new `@ControllerAdvice` handles and converts to 503, and ensure the controller methods downloadPdf and downloadFile still return the same ResponseEntity<byte[]> from reportService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/org/example/crimearchive/controllers/HomeController.java`:
- Around line 57-65: ReportService currently calls s3Client.getObjectAsBytes()
from methods downloadPdf and downloadFile without catching S3Exception or
SdkClientException, so infrastructure errors bubble up; modify the ReportService
implementations of downloadPdf(UUID) and downloadFile(UUID) to wrap the
s3Client.getObjectAsBytes(...) calls in try-catch blocks catching
software.amazon.awssdk.services.s3.model.S3Exception and
software.amazon.awssdk.core.exception.SdkClientException, map S3Exception (e.g.,
404 already handled) and other SDK/infrastructure errors to an appropriate
ResponseStatusException (for example HttpStatus.SERVICE_UNAVAILABLE with a clear
message) or rethrow a custom exception that a new `@ControllerAdvice` handles and
converts to 503, and ensure the controller methods downloadPdf and downloadFile
still return the same ResponseEntity<byte[]> from reportService.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 16828447-75d3-42da-a474-8707c131841e
📒 Files selected for processing (1)
src/main/java/org/example/crimearchive/controllers/HomeController.java
…ument its attached to the pdf
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit