Handle non-existing backup file #5590
Conversation
|
Warning Rate limit exceeded@agners has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 0 minutes and 38 seconds before requesting another review. ⌛ 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. 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThe changes introduce a new exception Changes
Sequence DiagramsequenceDiagram
participant API as Backup API
participant Manager as Backup Manager
participant Filesystem as File System
API->>Manager: Remove Backup
Manager->>Filesystem: Attempt to Unlink Backup File
alt File Not Found
Filesystem-->>Manager: FileNotFoundError
Manager->>Manager: Raise BackupFileNotFoundError
Manager-->>API: BackupFileNotFoundError
API->>API: Generate 404 Error Response
else File Successfully Removed
Filesystem-->>Manager: File Removed Successfully
Manager-->>API: Backup Removed
end
This sequence diagram illustrates the new error handling flow when attempting to remove a backup file that does not exist, demonstrating how the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
supervisor/backups/manager.py (1)
Line range hint
1773-1774: Simplify error handling logic.The error handling for
OSErrorcan be simplified by removing redundant error checks since the error is re-raised in both cases.- with pytest.raises(OSError): - coresys.backups.remove(backup) - assert coresys.core.healthy is True - - err.errno = errno.EBADMSG - with pytest.raises(OSError): - coresys.backups.remove(backup) + err.errno = errno.EBADMSG + with pytest.raises(OSError): + coresys.backups.remove(backup)Also applies to: 1778-1779
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
supervisor/api/backups.py(5 hunks)supervisor/backups/backup.py(2 hunks)supervisor/backups/manager.py(3 hunks)supervisor/exceptions.py(1 hunks)supervisor/misc/tasks.py(2 hunks)supervisor/resolution/fixups/system_clear_full_backup.py(2 hunks)tests/api/test_backups.py(2 hunks)tests/backups/test_manager.py(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
supervisor/exceptions.py (6)
Pattern */**(html|markdown|md): - For instructional content in documentation, use a direct and authoritative tone. Avoid expressions of politeness such as 'may' or 'please', and ensure the goal of the instruction is fronted.
- Apply the Microsoft Style Guide to ensure documentation maintains clarity and conciseness.
- In step-by-step instructions, front the location phrase in the instructional sentence.
- In step-by-step instructions, front the 'goal' in the instructional sentence.
- In step-by-step instructions, if in doubt what to front, front the 'goal' before the location phrase in the instructional sentence.
- do not hyphenate terms like 'top-right' or 'bottom-left' with 'corner'
Pattern */**(html|markdown|md): - Use bold to mark UI strings.
- If "" are used to mark UI strings, replace them by bold.
Pattern */**(html|markdown|md): - Be brief in your replies and don't add fluff like "thank you for..." and "Please let me know if"
Pattern */**(html|markdown|md): - Use sentence-style capitalization also in headings.
Pattern */**(html|markdown|md): do not comment on HTML used for icons
Pattern */**(html|markdown|md): Avoid flagging inline HTML for embedding videos in future reviews for this repository.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build i386 supervisor
- GitHub Check: Build amd64 supervisor
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
- GitHub Check: Run tests Python 3.13.1
🔇 Additional comments (8)
supervisor/exceptions.py (1)
662-664: LGTM! Well-structured exception class.The new
BackupFileNotFoundErrorexception is appropriately placed in the hierarchy and follows the established pattern.supervisor/api/backups.py (1)
324-325: LGTM! Consistent error handling.The error handling in
_background_backup_taskandremovemethods is well-implemented and consistent, properly convertingBackupFileNotFoundErrorto appropriate API errors.Also applies to: 467-470
supervisor/resolution/fixups/system_clear_full_backup.py (1)
36-39: LGTM! Appropriate error handling for cleanup task.The error handling is well-implemented:
- Catches
BackupFileNotFoundErrorwithout interrupting the cleanup process- Uses debug level logging which is appropriate for this scenario
supervisor/misc/tasks.py (1)
372-375: LGTM! Well-implemented error handling.The error handling in the cleanup task is appropriate:
- Properly catches
BackupFileNotFoundError- Uses debug level logging
- Allows the cleanup process to continue
supervisor/backups/backup.py (1)
Line range hint
521-525: LGTM! Improved error handling specificity.Good change to use a dedicated
BackupFileNotFoundErrorexception for better error handling when a backup file is missing. This makes the error more specific and easier to handle by consumers.supervisor/backups/manager.py (1)
296-304: LGTM! Consistent error handling for missing backup files.Good change to use
BackupFileNotFoundErrorwhen a backup file is missing during removal. This aligns with the error handling in theopenmethod.tests/api/test_backups.py (1)
506-513: LGTM! Good test coverage for missing backup file.The test case properly verifies that attempting to remove a non-existent backup file returns a 404 status code and includes the correct error message.
tests/backups/test_manager.py (1)
2080-2099: LGTM! Comprehensive test for file not found error.Good test case that verifies the
BackupFileNotFoundErroris raised when attempting to remove a non-existent backup file. The test properly mocks the file system error and checks the exception.
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
agners
force-pushed
the
handle-non-existing-backup-file
branch
from
314c7d3 to
55d01f9
Compare
agners
force-pushed
the
handle-non-existing-backup-file
branch
from
55d01f9 to
03bd728
Compare
Proposed change
Currently handling if the backup tar-file of a previously scanned tar file vanished is rather inconsistent. Most API's return with 400 error code, but error handling varies from simple error message in logs (e.g. when deleting) up to printing a stack trace (e.g. restoring with password).
This PR consistently uses error 404 when the backup file vanished.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
New Features
BackupFileNotFoundErrorexception to improve error handling for backup operations.Bug Fixes
Tests
These updates enhance the robustness of backup functionalities by providing clearer error messages and improved handling of missing backup files.