Fix/s3 fallback#60
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughS3Service methods now catch Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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: 2
🧹 Nitpick comments (5)
src/main/java/backendlab/team4you/s3/S3Controller.java (2)
35-67: Prefer a centralized@RestControllerAdviceover per-endpoint try/catch.The same
catch (FileStorageConfigurationException e) → 500 + Swedish messagepattern is duplicated three times here, and any other controller that ends up callingS3Service(e.g.,CaseFileController) will need the same boilerplate. Centralizing in a@RestControllerAdviceeliminates the duplication, ensures consistent behavior across all controllers, and gives a single place for logging.`@RestControllerAdvice` public class FileStorageExceptionHandler { private static final Logger log = LoggerFactory.getLogger(FileStorageExceptionHandler.class); `@ExceptionHandler`(FileStorageConfigurationException.class) public ResponseEntity<String> handle(FileStorageConfigurationException e) { log.error("File storage failure", e); return ResponseEntity.internalServerError().body(e.getMessage()); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Controller.java` around lines 35 - 67, Replace the repeated try/catch blocks in S3Controller methods uploadFile, downloadFile, and deleteFile by removing catches for FileStorageConfigurationException and instead create a centralized `@RestControllerAdvice` named FileStorageExceptionHandler with an `@ExceptionHandler`(FileStorageConfigurationException.class) method (e.g., handle) that logs the exception and returns ResponseEntity.internalServerError().body(e.getMessage()) so all controllers (including CaseFileController) get consistent 500 handling and logging for FileStorageConfigurationException.
46-46: Loosened return type — considerResponseEntity<Object>or a sealed response shape.Changing
ResponseEntity<byte[]>toResponseEntity<?>works but loses information for API documentation tools (Swagger/OpenAPI) and makes the contract harder to reason about.ResponseEntity<Object>is more idiomatic for mixed body types, or alternatively keepResponseEntity<byte[]>and have the error path return an empty body / use@ResponseStatuswith a global handler (see the@RestControllerAdvicesuggestion).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Controller.java` at line 46, The method signature for downloadFile currently uses ResponseEntity<?> which hides the response contract; update the signature of downloadFile to a more explicit type such as ResponseEntity<Object> (or revert to ResponseEntity<byte[]> if the success path always returns bytes) and adjust error handling accordingly—e.g., keep ResponseEntity<byte[]> and return an empty body / use `@ResponseStatus` with your global `@RestControllerAdvice`, or change to ResponseEntity<Object> and ensure both the success (byte[] payload) and error (error DTO or message) paths are properly typed so OpenAPI/Swagger and callers see a clear contract.src/test/java/backendlab/team4you/s3/S3ServiceTest.java (1)
74-105: Optional: import the exception type and assert on the cause.The fully-qualified class name in
assertThrowscalls is verbose. Adding an import would be cleaner. Additionally, asserting that the originalS3Exceptionis preserved asgetCause()would protect against accidental loss of the underlying SDK failure during refactors.♻️ Suggested cleanup
+import backendlab.team4you.exceptions.FileStorageConfigurationException; @@ - assertThrows(backendlab.team4you.exceptions.FileStorageConfigurationException.class, - () -> s3Service.uploadFile("test.txt", "Hello".getBytes(), "text/plain")); + FileStorageConfigurationException ex = assertThrows(FileStorageConfigurationException.class, + () -> s3Service.uploadFile("test.txt", "Hello".getBytes(), "text/plain")); + assertInstanceOf(S3Exception.class, ex.getCause());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/s3/S3ServiceTest.java` around lines 74 - 105, Replace the fully-qualified exception class in the three tests in S3ServiceTest (uploadFile_shouldThrowFileStorageConfigurationException_whenS3Fails, downloadFile_shouldThrowFileStorageConfigurationException_whenS3Fails, deleteFile_shouldThrowFileStorageConfigurationException_whenS3Fails) with an import of backendlab.team4you.exceptions.FileStorageConfigurationException and, after asserting the exception is thrown, assert that ex.getCause() is an instance of S3Exception (or that the cause message matches "S3 error") to ensure the original S3Exception is preserved as the cause.src/main/java/backendlab/team4you/s3/S3Service.java (2)
34-74: Exception choice misrepresents the failure mode.
FileStorageConfigurationExceptionsemantically implies a misconfiguration, butS3Exceptioncovers many transient/runtime failures (network, throttling, auth expiry, server-side errors, etc.). Callers reading the type will be misled, and operationally a 5xx caused by throttling will be classified as a configuration problem.Consider introducing a more general
FileStorageException(or similar) for runtime failures, and reservingFileStorageConfigurationExceptionfor genuine configuration issues. At minimum, the message should preserve more detail from the underlyingS3Exception(status code / AWS error code) rather than only the key.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Service.java` around lines 34 - 74, The catch blocks in upload/download/delete (methods: the putObject block, downloadFile, deleteFile) wrongly wrap all S3Exception cases as FileStorageConfigurationException; introduce a new runtime exception (e.g., FileStorageException) for transient/runtime S3 errors and reserve FileStorageConfigurationException only for genuine config validation failures, then update the catch clauses to throw FileStorageException instead, constructing the message to include S3Exception.getMessage(), status code and AWS error code (or e.awsErrorDetails()) and pass the original exception as the cause so callers can inspect details.
76-93: Inconsistent error translation inuploadFileIfAbsent.
uploadFile,downloadFile, anddeleteFilenow translateS3Exception→FileStorageConfigurationException, butuploadFileIfAbsentstill rethrows the rawS3Exceptionfor any non-409/412 status. Callers of this method will see a different exception type from the same SDK failure, which makes uniform handling at the controller / service boundary harder.♻️ Suggested alignment
} catch (S3Exception exception) { if (exception.statusCode() == 409 || exception.statusCode() == 412) { throw new FileKeyConflictException(key, exception); } - throw exception; + throw new FileStorageConfigurationException("Kunde inte ladda upp filen: " + key, exception); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/s3/S3Service.java` around lines 76 - 93, uploadFileIfAbsent currently catches S3Exception only to map 409/412 into FileKeyConflictException and rethrows all other S3Exception instances directly, causing inconsistent exception types compared to uploadFile, downloadFile, and deleteFile; change uploadFileIfAbsent to catch S3Exception from s3Client.putObject and, after mapping 409/412 to FileKeyConflictException (as it already does), wrap all other S3Exception instances in FileStorageConfigurationException (the same translation used by uploadFile/downloadFile/deleteFile) so callers always receive consistent exceptions for SDK 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/backendlab/team4you/s3/S3Controller.java`:
- Line 39: The error response in S3Controller currently returns a Swedish string
("Kunde inte ladda upp filen: " + key) while success messages are English;
change the endpoint to return a consistent structured JSON error object (e.g.,
Map or a small ErrorResponse DTO with fields "error" and "key") and use English
messages (e.g., { "error": "Could not upload file", "key": key }) so clients can
localize; update the ResponseEntity.internalServerError().body(...) call to
return the ErrorResponse (or Map.of("error", "...", "key", key)) and ensure any
other error branches in S3Controller use the same structure.
- Around line 35-67: The three catch blocks in S3Controller (in methods
uploadFile, downloadFile, deleteFile) currently swallow
FileStorageConfigurationException; update each catch to log the exception before
returning the 500 response (use the controller's logger and call logger.error
with a clear message and the exception object so stacktrace and cause are
preserved), keeping the existing Swedish response body; ensure the logger used
is the class logger (e.g., LoggerFactory.getLogger(S3Controller.class)) and
include contextual info like the key and operation in the log message.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/s3/S3Controller.java`:
- Around line 35-67: Replace the repeated try/catch blocks in S3Controller
methods uploadFile, downloadFile, and deleteFile by removing catches for
FileStorageConfigurationException and instead create a centralized
`@RestControllerAdvice` named FileStorageExceptionHandler with an
`@ExceptionHandler`(FileStorageConfigurationException.class) method (e.g., handle)
that logs the exception and returns
ResponseEntity.internalServerError().body(e.getMessage()) so all controllers
(including CaseFileController) get consistent 500 handling and logging for
FileStorageConfigurationException.
- Line 46: The method signature for downloadFile currently uses
ResponseEntity<?> which hides the response contract; update the signature of
downloadFile to a more explicit type such as ResponseEntity<Object> (or revert
to ResponseEntity<byte[]> if the success path always returns bytes) and adjust
error handling accordingly—e.g., keep ResponseEntity<byte[]> and return an empty
body / use `@ResponseStatus` with your global `@RestControllerAdvice`, or change to
ResponseEntity<Object> and ensure both the success (byte[] payload) and error
(error DTO or message) paths are properly typed so OpenAPI/Swagger and callers
see a clear contract.
In `@src/main/java/backendlab/team4you/s3/S3Service.java`:
- Around line 34-74: The catch blocks in upload/download/delete (methods: the
putObject block, downloadFile, deleteFile) wrongly wrap all S3Exception cases as
FileStorageConfigurationException; introduce a new runtime exception (e.g.,
FileStorageException) for transient/runtime S3 errors and reserve
FileStorageConfigurationException only for genuine config validation failures,
then update the catch clauses to throw FileStorageException instead,
constructing the message to include S3Exception.getMessage(), status code and
AWS error code (or e.awsErrorDetails()) and pass the original exception as the
cause so callers can inspect details.
- Around line 76-93: uploadFileIfAbsent currently catches S3Exception only to
map 409/412 into FileKeyConflictException and rethrows all other S3Exception
instances directly, causing inconsistent exception types compared to uploadFile,
downloadFile, and deleteFile; change uploadFileIfAbsent to catch S3Exception
from s3Client.putObject and, after mapping 409/412 to FileKeyConflictException
(as it already does), wrap all other S3Exception instances in
FileStorageConfigurationException (the same translation used by
uploadFile/downloadFile/deleteFile) so callers always receive consistent
exceptions for SDK failures.
In `@src/test/java/backendlab/team4you/s3/S3ServiceTest.java`:
- Around line 74-105: Replace the fully-qualified exception class in the three
tests in S3ServiceTest
(uploadFile_shouldThrowFileStorageConfigurationException_whenS3Fails,
downloadFile_shouldThrowFileStorageConfigurationException_whenS3Fails,
deleteFile_shouldThrowFileStorageConfigurationException_whenS3Fails) with an
import of backendlab.team4you.exceptions.FileStorageConfigurationException and,
after asserting the exception is thrown, assert that ex.getCause() is an
instance of S3Exception (or that the cause message matches "S3 error") to ensure
the original S3Exception is preserved as the cause.
🪄 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: f52fe721-29a9-41ba-b16c-49aff8eb5cc7
📒 Files selected for processing (3)
src/main/java/backendlab/team4you/s3/S3Controller.javasrc/main/java/backendlab/team4you/s3/S3Service.javasrc/test/java/backendlab/team4you/s3/S3ServiceTest.java
2 participants
Summary by CodeRabbit