refactor: improve Document and Minio classes#112
Merged
Conversation
- Replace manual getters/setters with @Getter/@Setter in Document entity - Extract getUser() and toDTO() helper methods in DocumentController - Replace generic RuntimeException with custom FileUploadException - Replace generic RuntimeException with custom MinioOperationException - Make FileMissingException static inner class - Replace System.err with log.warn in MinioService.deleteFile - Add uploadFile and deleteFile failure tests in MinioServiceTest - Remove debug System.out.println from tests
📝 WalkthroughWalkthroughRefactors document management authentication, entity boilerplate, and exception handling. Centralizes user retrieval in DocumentController with null-check, replaces manual getters/setters with Lombok annotations, introduces dedicated exception types for file operations, removes MinioService.getFile(), and adds global exception handlers for the new exceptions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
src/main/resources/application.yml (1)
25-28: Inconsistency: AuditLogController returns entities, not DTOs.The
via-dtoserialization mode suggests responses should use DTOs, but AuditLogController returnsPage<AuditLog>(entity objects directly) atsrc/main/java/org/example/team6backend/auditlog/controller/AuditLogController.java:22-39. This exposes internal entity structure in the API and contradicts the "via-dto" approach.Consider mapping AuditLog entities to DTOs before returning them, similar to how AdminController uses
userMapper.toResponsePage(users)and IncidentController uses.map(IncidentResponse::fromEntityBasic).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.yml` around lines 25 - 28, AuditLogController currently returns Page<AuditLog> (entities) while application.yml sets data.web.pageable.serialization-mode: via-dto; change AuditLogController to map entities to DTOs before returning (e.g., use an AuditLogResponse DTO with a static fromEntity(AuditLog) or add an AuditLogMapper and a method like auditLogMapper.toResponsePage(Page<AuditLog>)) so the controller returns Page<AuditLogResponse> similar to AdminController.userMapper.toResponsePage(users) or IncidentController.map(IncidentResponse::fromEntityBasic); update the controller method signature and return value to use the DTO page and ensure any serialization/tests reflect the DTO fields.src/test/java/org/example/team6backend/document/service/MinioServiceTest.java (1)
38-43: Test name promises log assertion that the body doesn't perform.The name
deleteFile_shouldLogWarning_whenMinioFailsimplies the test verifies warning logging, but it only asserts that no exception is thrown. Either rename to reflect the actual assertion (e.g.deleteFile_shouldNotThrow_whenMinioFails) or capture the log via aListAppender/MemoryAppenderand assert the warn entry.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/team6backend/document/service/MinioServiceTest.java` around lines 38 - 43, The test name deleteFile_shouldLogWarning_whenMinioFails is misleading because it doesn't assert logging; either rename the test to deleteFile_shouldNotThrow_whenMinioFails or change the test to capture and assert the warning log produced by minioService.deleteFile; to implement the latter, use a logging test appender (e.g., Logback ListAppender or MemoryAppender) attached to the logger used by MinioService, invoke minioService.deleteFile("abc") with the minioClient mocked to throw (doThrow on removeObject), then assert the appender contains a WARN entry containing the expected message; update references: MinioServiceTest, minioService, minioClient, removeObject(RemoveObjectArgs) and ReflectionTestUtils.setField("bucketName").src/main/java/org/example/team6backend/document/controller/DocumentController.java (1)
38-43: Minor: include a message with the 401 response.
new ResponseStatusException(HttpStatus.UNAUTHORIZED)produces an emptyreason. Consider adding a brief reason string for clearer logs/diagnostics, e.g.new ResponseStatusException(HttpStatus.UNAUTHORIZED, "Authentication required").🤖 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 38 - 43, The getUser(CustomUserDetails userDetails) method throws a ResponseStatusException with no reason; update the throw to include a brief reason string (e.g., "Authentication required") so logs show why a 401 was returned. Locate getUser in DocumentController and change the ResponseStatusException(HttpStatus.UNAUTHORIZED) to ResponseStatusException(HttpStatus.UNAUTHORIZED, "<brief reason>") ensuring the message is clear and concise.src/main/java/org/example/team6backend/document/service/MinioService.java (1)
67-69: Pass the throwable tolog.warnto preserve the stack trace.
e.getMessage()alone drops the cause chain, which is especially valuable when MinIO/networking errors are wrapped. SLF4J treats a trailingThrowableargument specially and logs the full stack trace.- } catch (Exception e) { - log.warn("MinIO delete failed for {}: {}", fileKey, e.getMessage()); - } + } catch (Exception e) { + log.warn("MinIO delete failed for {}", 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/MinioService.java` around lines 67 - 69, The catch block in MinioService currently logs only e.getMessage(), losing the stack trace; update the log.warn call in the catch (Exception e) block (the one referencing fileKey) to pass the Throwable as the last parameter (i.e., include e instead of e.getMessage()) so SLF4J will log the full stack trace and preserve the cause chain.src/main/java/org/example/team6backend/document/service/DocumentService.java (1)
54-63: Optional: avoid double-wrapping when the cleanup also fails or the cause is already a known type.The broad
catch (Exception e)will also wrap things likeMinioService.MinioOperationException. That's acceptable, but consider preserving an uncheckedResponseStatusException(if any future code throws one inside this block) so the framework handles it directly rather than wrapping it as a 500. Not a blocker today since nothing inside currently throwsResponseStatusException.🤖 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 54 - 63, In the catch block in DocumentService where you call minioService.deleteFile(...) and then throw new FileUploadException, preserve existing response/status exceptions and avoid double-wrapping: if the caught exception `e` is a ResponseStatusException or already a FileUploadException, rethrow it directly; when cleanup fails, log the cleanup exception (as you do) but do not replace or wrap the original `e` with the cleanup exception—always throw a FileUploadException with the original `e` as its cause (unless you rethrew it because it was a ResponseStatusException/FileUploadException). Update the catch to check for instanceof ResponseStatusException and instanceof FileUploadException before wrapping, and keep the cleanup call to minioService.deleteFile(...) and its log as-is.
🤖 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 133-137: Add a specific exception handler in
GlobalExceptionHandler for DocumentService.FileUploadException so the original
message is preserved instead of the generic "Something went wrong!"; implement a
method (e.g., handleFileUpload) annotated with
`@ExceptionHandler`(DocumentService.FileUploadException.class) that builds an
ErrorResponse using ex.getMessage(), appropriate HTTP status (e.g.,
HttpStatus.INTERNAL_SERVER_ERROR) and timestamp, and returns it in a
ResponseEntity; ensure the handler signature and returned ErrorResponse type
match the existing global handler patterns so it takes precedence over the
generic Exception handler.
In `@src/main/java/org/example/team6backend/document/service/MinioService.java`:
- Around line 72-78: The getFile method currently catches Exception and always
throws a 404; change it to handle FileMissingException and
MinioOperationException separately: call downloadFile(fileKey) inside try, catch
FileMissingException and throw new ResponseStatusException(HttpStatus.NOT_FOUND,
"File not found: " + fileKey, e), and catch MinioOperationException (or other
transport/server exceptions) and rethrow as a 503
(HttpStatus.SERVICE_UNAVAILABLE) or 500 with the original exception as the cause
so actual MinIO failures are not masked; leave other unexpected exceptions to
bubble or wrap them appropriately with their cause preserved.
In `@src/main/resources/application.yml`:
- Around line 25-28: The change to data.web.pageable.serialization-mode: via-dto
alters JSON for all Page responses; add integration tests asserting the new Page
JSON structure for AdminController, IncidentController and AuditLogController
endpoints, update API migration notes documenting the breaking change and
required frontend adjustments, and modify AuditLogController to return a DTO
(e.g., convert Page<AuditLog> to Page<AuditLogDto> using the existing mapper or
a new toDto method) so it aligns with the via-dto intent; ensure tests validate
the DTO mapping and update any controller method signatures and service mapping
logic that produce Page<AuditLog> to produce Page<AuditLogDto>.
In
`@src/test/java/org/example/team6backend/document/service/MinioServiceTest.java`:
- Line 46: The test method name contains a typo: rename the method
uploadFIle_shouldThrowMinioOperationException_whenMinioFails to
uploadFile_shouldThrowMinioOperationException_whenMinioFails in MinioServiceTest
(update the method declaration and any references to it, keeping the `@Test`
annotation and method signature intact) so the name is correct and consistent.
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/document/controller/DocumentController.java`:
- Around line 38-43: The getUser(CustomUserDetails userDetails) method throws a
ResponseStatusException with no reason; update the throw to include a brief
reason string (e.g., "Authentication required") so logs show why a 401 was
returned. Locate getUser in DocumentController and change the
ResponseStatusException(HttpStatus.UNAUTHORIZED) to
ResponseStatusException(HttpStatus.UNAUTHORIZED, "<brief reason>") ensuring the
message is clear and concise.
In
`@src/main/java/org/example/team6backend/document/service/DocumentService.java`:
- Around line 54-63: In the catch block in DocumentService where you call
minioService.deleteFile(...) and then throw new FileUploadException, preserve
existing response/status exceptions and avoid double-wrapping: if the caught
exception `e` is a ResponseStatusException or already a FileUploadException,
rethrow it directly; when cleanup fails, log the cleanup exception (as you do)
but do not replace or wrap the original `e` with the cleanup exception—always
throw a FileUploadException with the original `e` as its cause (unless you
rethrew it because it was a ResponseStatusException/FileUploadException). Update
the catch to check for instanceof ResponseStatusException and instanceof
FileUploadException before wrapping, and keep the cleanup call to
minioService.deleteFile(...) and its log as-is.
In `@src/main/java/org/example/team6backend/document/service/MinioService.java`:
- Around line 67-69: The catch block in MinioService currently logs only
e.getMessage(), losing the stack trace; update the log.warn call in the catch
(Exception e) block (the one referencing fileKey) to pass the Throwable as the
last parameter (i.e., include e instead of e.getMessage()) so SLF4J will log the
full stack trace and preserve the cause chain.
In `@src/main/resources/application.yml`:
- Around line 25-28: AuditLogController currently returns Page<AuditLog>
(entities) while application.yml sets data.web.pageable.serialization-mode:
via-dto; change AuditLogController to map entities to DTOs before returning
(e.g., use an AuditLogResponse DTO with a static fromEntity(AuditLog) or add an
AuditLogMapper and a method like auditLogMapper.toResponsePage(Page<AuditLog>))
so the controller returns Page<AuditLogResponse> similar to
AdminController.userMapper.toResponsePage(users) or
IncidentController.map(IncidentResponse::fromEntityBasic); update the controller
method signature and return value to use the DTO page and ensure any
serialization/tests reflect the DTO fields.
In
`@src/test/java/org/example/team6backend/document/service/MinioServiceTest.java`:
- Around line 38-43: The test name deleteFile_shouldLogWarning_whenMinioFails is
misleading because it doesn't assert logging; either rename the test to
deleteFile_shouldNotThrow_whenMinioFails or change the test to capture and
assert the warning log produced by minioService.deleteFile; to implement the
latter, use a logging test appender (e.g., Logback ListAppender or
MemoryAppender) attached to the logger used by MinioService, invoke
minioService.deleteFile("abc") with the minioClient mocked to throw (doThrow on
removeObject), then assert the appender contains a WARN entry containing the
expected message; update references: MinioServiceTest, minioService,
minioClient, removeObject(RemoveObjectArgs) and
ReflectionTestUtils.setField("bucketName").
🪄 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: 7b700ca2-d5a4-40b3-9d24-e26912a7d100
📒 Files selected for processing (7)
src/main/java/org/example/team6backend/document/controller/DocumentController.javasrc/main/java/org/example/team6backend/document/entity/Document.javasrc/main/java/org/example/team6backend/document/service/DocumentService.javasrc/main/java/org/example/team6backend/document/service/MinioService.javasrc/main/resources/application.ymlsrc/test/java/org/example/team6backend/document/service/DocumentServiceTest.javasrc/test/java/org/example/team6backend/document/service/MinioServiceTest.java
💤 Files with no reviewable changes (1)
- src/test/java/org/example/team6backend/document/service/DocumentServiceTest.java
- Add FileUploadException handler in GlobalExceptionHandler - Add MinioOperationException handler in GlobalExceptionHandler - Remove unused getFile() method from MinioService - Fix getFile() to only catch FileMissingException, not all exceptions
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/document/service/MinioService.java (1)
62-68:⚠️ Potential issue | 🟡 Minor
deleteFilenow silently swallows every failure — verify this is intended and clean up dead caller handling.
deleteFilenow catchesExceptionand only logs a warning, so it never throws. Two consequences worth confirming:
- Genuine MinIO outages during cleanup (e.g., orphaned objects after a DB save failure in
DocumentService.uploadFile, or rollback inIncidentService.createIncident) are reduced to a singleWARNlog line without the stack trace (e.getMessage()only). Consider passing the exception as the second arg so the cause is logged:log.warn("MinIO delete failed for {}", fileKey, e);.- Because this method can no longer throw, the surrounding
try { minioService.deleteFile(...) } catch (Exception cleanupEx) { log.warn(...) }blocks inDocumentService.uploadFileandIncidentService.createIncidentare now dead code and produce duplicate WARN entries when a delete fails. Either drop those caller-side try/catches, or havedeleteFilerethrowMinioOperationExceptionand keep the caller-side handling.🤖 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/MinioService.java` around lines 62 - 68, The deleteFile method in MinioService currently swallows all exceptions and only logs e.getMessage(), which hides stack traces and makes caller try/catch blocks dead; change deleteFile (in class MinioService, method deleteFile) to log the exception object (e.g., log.warn("MinIO delete failed for {}", fileKey, e)) and then rethrow a specific unchecked exception (e.g., MinioOperationException) so callers can handle failures, or alternatively keep deleteFile swallowing but remove the redundant try/catch in DocumentService.uploadFile and IncidentService.createIncident—pick one approach and apply consistently (either update MinioService to throw MinioOperationException and keep caller try/catches, or keep swallowing with proper logging and remove caller-side catch blocks).
🧹 Nitpick comments (1)
src/main/java/org/example/team6backend/exception/GlobalExceptionHandler.java (1)
48-60: Consider logging the underlying cause for these new 500 handlers.The new
FileUploadException/MinioOperationExceptionhandlers returnex.getMessage()to the client and bypasshandleGeneral, which means the stack trace (previously printed viaex.printStackTrace()in the catch-all) is no longer captured. For server-side failures around file storage you typically want at least the cause logged for triage. Also, surfacing rawex.getMessage()forMinioOperationExceptionmay leak internal details (bucket names, MinIO error codes); consider returning a generic message and logging the detail instead.The two handlers are also functionally identical and can be collapsed.
♻️ Suggested refactor
- `@ExceptionHandler`(DocumentService.FileUploadException.class) - public ResponseEntity<ErrorResponse> handleFileUpload(DocumentService.FileUploadException ex) { - ErrorResponse error = new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.value(), ex.getMessage(), - Instant.now()); - return new ResponseEntity<>(error, HttpStatus.INTERNAL_SERVER_ERROR); - } - - `@ExceptionHandler`(MinioService.MinioOperationException.class) - public ResponseEntity<ErrorResponse> handleMinioOperation(MinioService.MinioOperationException ex) { - ErrorResponse error = new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.value(), ex.getMessage(), - Instant.now()); - return new ResponseEntity<>(error, HttpStatus.INTERNAL_SERVER_ERROR); - } + `@ExceptionHandler`({ DocumentService.FileUploadException.class, MinioService.MinioOperationException.class }) + public ResponseEntity<ErrorResponse> handleFileOperation(RuntimeException ex) { + log.error("File operation failed", ex); + ErrorResponse error = new ErrorResponse(HttpStatus.INTERNAL_SERVER_ERROR.value(), + "File operation failed", Instant.now()); + return new ResponseEntity<>(error, HttpStatus.INTERNAL_SERVER_ERROR); + }(Requires
@Slf4jon the class.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/team6backend/exception/GlobalExceptionHandler.java` around lines 48 - 60, Collapse the two identical handlers in GlobalExceptionHandler into a single `@ExceptionHandler` that handles both DocumentService.FileUploadException and MinioService.MinioOperationException, add `@Slf4j` to the class, and replace the current direct ex.getMessage() exposure with server-side logging and a safer client message: call log.error(...) with the exception (to capture stack trace and cause) and return an ErrorResponse with a generic message (e.g., "File storage error") and HttpStatus.INTERNAL_SERVER_ERROR; keep the method name distinct such as handleFileStorageExceptions and reference the existing ErrorResponse constructor to build the response.
🤖 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/document/service/MinioService.java`:
- Around line 62-68: The deleteFile method in MinioService currently swallows
all exceptions and only logs e.getMessage(), which hides stack traces and makes
caller try/catch blocks dead; change deleteFile (in class MinioService, method
deleteFile) to log the exception object (e.g., log.warn("MinIO delete failed for
{}", fileKey, e)) and then rethrow a specific unchecked exception (e.g.,
MinioOperationException) so callers can handle failures, or alternatively keep
deleteFile swallowing but remove the redundant try/catch in
DocumentService.uploadFile and IncidentService.createIncident—pick one approach
and apply consistently (either update MinioService to throw
MinioOperationException and keep caller try/catches, or keep swallowing with
proper logging and remove caller-side catch blocks).
---
Nitpick comments:
In
`@src/main/java/org/example/team6backend/exception/GlobalExceptionHandler.java`:
- Around line 48-60: Collapse the two identical handlers in
GlobalExceptionHandler into a single `@ExceptionHandler` that handles both
DocumentService.FileUploadException and MinioService.MinioOperationException,
add `@Slf4j` to the class, and replace the current direct ex.getMessage() exposure
with server-side logging and a safer client message: call log.error(...) with
the exception (to capture stack trace and cause) and return an ErrorResponse
with a generic message (e.g., "File storage error") and
HttpStatus.INTERNAL_SERVER_ERROR; keep the method name distinct such as
handleFileStorageExceptions and reference the existing ErrorResponse constructor
to build the response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f92487ec-d6f2-4587-a207-4cfdffdf4945
📒 Files selected for processing (3)
src/main/java/org/example/team6backend/document/service/MinioService.javasrc/main/java/org/example/team6backend/exception/GlobalExceptionHandler.javasrc/test/java/org/example/team6backend/document/service/MinioServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/team6backend/document/service/MinioServiceTest.java
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
Bug Fixes
Tests