Skip to content

feat: added support for prune for S3 buckets#3463

Open
harshit078 wants to merge 5 commits intotolgee:mainfrom
harshit078:harshit.singh/add-prune-error
Open

feat: added support for prune for S3 buckets#3463
harshit078 wants to merge 5 commits intotolgee:mainfrom
harshit078:harshit.singh/add-prune-error

Conversation

@harshit078
Copy link

@harshit078 harshit078 commented Feb 8, 2026

Description

  • This PR fixes Handle unsupported prune for S3 buckets better #3339
  • Added specific error handling for pruning failures for s3 bucket
  • Introduced a new message constant CANNOT_PRUNE_CONTENT_STORAGE for better error reporting.
  • Updated the error translation logic to include the new error case.

Summary by CodeRabbit

  • Bug Fixes

    • Users now receive a specific, descriptive error when content storage pruning fails during content-delivery configuration, returning a clear Bad Request response.
  • Tests

    • Added test coverage to ensure the new error response is returned when pruning the content storage fails.
  • Localization

    • Added translation mapping for the new error code so the message can be displayed in the UI.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 8, 2026

📝 Walkthrough

Walkthrough

Catches 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

Cohort / File(s) Summary
Controller
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt
Wraps contentDeliveryUploader.upload(exporter.id) in a try/catch for FileStoreException and throws BadRequestException with either Message.CANNOT_PRUNE_CONTENT_STORAGE or Message.CANNOT_STORE_FILE_TO_CONTENT_STORAGE based on exception message.
Tests
backend/app/src/test/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigControllerTest.kt
Adds test returns specific error when prune fails that mocks pruneDirectory to throw FileStoreException and asserts HTTP 400 with JSON error code cannot_prune_content_storage.
Constants
backend/data/src/main/kotlin/io/tolgee/constants/Message.kt
Adds new enum constant CANNOT_PRUNE_CONTENT_STORAGE.
Frontend translation
webapp/src/translationTools/useErrorTranslation.ts
Maps 'cannot_prune_content_storage' error code to the corresponding translation key.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

"I thumped a log and sniffed the prune,
A twitch, a catch, and oh — a tune,
From storage's groan to UI light,
I hopped the warning into sight,
🥕 clearer paths from night to noon."

🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'feat: added support for prune for S3 buckets' is misleading—the actual changes add error handling for prune failures, not support for pruning S3 buckets. Revise title to accurately reflect the changes, such as 'feat: add specific error handling for S3 prune failures' or 'feat: improve error messaging for unsupported S3 pruning'.
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check ✅ Passed The PR successfully addresses issue #3339 by implementing specific error handling for pruning failures, introducing CANNOT_PRUNE_CONTENT_STORAGE message, and updating error translation logic as required.
Out of Scope Changes check ✅ Passed All changes are directly scoped to issue #3339: backend exception handling for prune failures, new error message constant, error translation mapping, and corresponding test coverage.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

No actionable comments were generated in the recent review. 🎉

🧹 Recent nitpick comments
backend/api/src/main/kotlin/io/tolgee/api/v2/controllers/contentDelivery/ContentDeliveryConfigController.kt (1)

124-131: Previous issues resolved; consider logging the original exception.

Both past review concerns (missing catch, indiscriminate error mapping) are addressed — nice work.

Two minor points:

  1. Original cause is swallowed — passing e as the cause of BadRequestException (if supported) or logging it at warn/error level would preserve debuggability on the server side.
  2. String-based detection is fragilee.message?.contains("prune") silently degrades to the generic error if the upstream message wording changes. If FileStoreException could be extended with a structured cause/code, that would be more robust, but this is fine for now.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @Test annotation, 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 to FileStoreException instead 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.

@harshit078
Copy link
Author

Hey @Anty0, the PR is ready to review. Thanks !

@Anty0 Anty0 self-requested a review February 9, 2026 12:43
@Anty0
Copy link
Collaborator

Anty0 commented Feb 9, 2026

Hey @harshit078, the linter and build failed. Please take a look at the failed CI jobs. Let me know if you need any help! ^^

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-prune FileStoreException.

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 storeFile throws a FileStoreException (without "prune" in the message) and assert the response code is cannot_store_file_to_content_storage.

@harshit078
Copy link
Author

@Anty0 , resolved the tests and comments. PR ready to be reviewed again. Thanks !

@Anty0
Copy link
Collaborator

Anty0 commented Feb 10, 2026

The CI pipeline is still failing. Please take a look. Thanks! ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handle unsupported prune for S3 buckets better

2 participants