feat: added support for prune for S3 buckets#3463
feat: added support for prune for S3 buckets#3463harshit078 wants to merge 5 commits intotolgee:mainfrom
Conversation
📝 WalkthroughWalkthroughCatches FileStoreException thrown during contentDeliveryUploader.upload in the ContentDeliveryConfigController POST handler and converts it to a BadRequestException with a new Message code; adds the new message constant, a test for the failure case, and maps the new error code to a frontend translation. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (UI)
participant API as API (ContentDeliveryConfigController)
participant Uploader as ContentDeliveryUploader
participant Store as FileStore (S3 / storage)
Client->>API: POST /content-delivery-configs (publish with prune)
API->>Uploader: upload(exporter.id)
Uploader->>Store: pruneDirectory / upload operations
alt prune succeeds
Store-->>Uploader: success
Uploader-->>API: success
API-->>Client: 200 OK
else prune fails (FileStoreException)
Store-->>Uploader: throws FileStoreException
Uploader-->>API: propagates exception
API->>API: catch FileStoreException -> map message -> throw BadRequest(Message.CANNOT_PRUNE_CONTENT_STORAGE or CANNOT_STORE_FILE_TO_CONTENT_STORAGE)
API-->>Client: 400 Bad Request (code: cannot_prune_content_storage)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt`:
- Around line 120-126: The try block around
contentDeliveryUploader.upload(exporter.id) is missing a catch and references an
undeclared variable e; add a catch for the specific exception thrown by the
upload (catch (FileStoreException e)) and move the logic that checks
e.message?.contains(...) into that catch to throw either
BadRequestException(Message.CANNOT_PRUNE_CONTENT_STORAGE) or
BadRequestException(Message.CANNOT_STORE_FILE_TO_CONTENT_STORAGE) accordingly;
also add the required imports for
io.tolgee.component.fileStorage.FileStoreException,
io.tolgee.exceptions.BadRequestException and io.tolgee.constants.Message so the
types resolve.
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt`:
- Around line 102-115: The test `returns specific error when prune fails` uses
FileStoreException but lacks its import; add the import for
io.tolgee.exceptions.FileStoreException to the top of
ContentDeliveryConfigControllerTest.kt so the usage in the test
(FileStoreException(...) in the mocked.pruneDirectory stub) compiles.
🧹 Nitpick comments (2)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt (1)
102-102: Minor: Missing blank line before@Test.There should be a blank line between the closing brace of the previous test (line 101) and this
@Testannotation, consistent with the rest of the file.backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt (1)
122-125: Consider adding structured error classification toFileStoreExceptioninstead of message matching.The code currently checks
e.message?.contains("prune")to distinguish error types, which works because prune error messages consistently include that word. However, this pattern is fragile: if the upstream exception message changes, the error classification silently breaks. The exception class could be more robust by adding a typed error code or enum field (e.g.,errorType: FileStorageErrorType) to make error classification explicit and resilient to message wording changes.
.../main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt
Outdated
Show resolved
Hide resolved
...t/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt
Show resolved
Hide resolved
|
Hey @Anty0, the PR is ready to review. Thanks ! |
|
Hey @harshit078, the linter and build failed. Please take a look at the failed CI jobs. Let me know if you need any help! ^^ |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt`:
- Around line 124-128: The catch currently maps every FileStoreException from
contentDeliveryUploader.upload(exporter.id) to
Message.CANNOT_PRUNE_CONTENT_STORAGE; update the handler in
ContentDeliveryConfigController so you inspect the FileStoreException (e.g.,
e.message or a specific error code on the exception) and only throw
BadRequestException(Message.CANNOT_PRUNE_CONTENT_STORAGE) when the exception
indicates a prune-specific failure (message contains "prune" or matching code);
for other FileStoreException cases throw a different, accurate
BadRequestException (e.g., Message.CANNOT_STORE_CONTENT or a generic storage
error) or rethrow the original exception so storage/write errors are not
misreported as prune failures.
In
`@backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt`:
- Line 11: The test imports the wrong FileStoreException package; update the
import in ContentDeliveryConfigControllerTest to use the correct class location
(FileStoreException from io.tolgee.component.fileStorage) to match the
controller; locate the import statement referencing FileStoreException and
replace the io.tolgee.exceptions path with io.tolgee.component.fileStorage so
both the test and the controller use the same exception class.
🧹 Nitpick comments (1)
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt (1)
104-117: Test looks good, but consider adding a test for non-pruneFileStoreException.The test correctly validates the prune failure path. However, if you adopt the fix to distinguish prune vs. non-prune errors in the controller, you should also add a test where
storeFilethrows aFileStoreException(without "prune" in the message) and assert the response code iscannot_store_file_to_content_storage.
.../main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt
Show resolved
Hide resolved
...t/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt
Outdated
Show resolved
Hide resolved
|
@Anty0 , resolved the tests and comments. PR ready to be reviewed again. Thanks ! |
|
The CI pipeline is still failing. Please take a look. Thanks! ^^ |
Description
CANNOT_PRUNE_CONTENT_STORAGEfor better error reporting.Summary by CodeRabbit
Bug Fixes
Tests
Localization