Extend backup upload API with file name parameter #5568
Conversation
Add a query parameter which allows to specify the file name on upload. All locations will store the backup with the same file name.
📝 WalkthroughWalkthroughThe pull request introduces an enhancement to the backup upload functionality across multiple files in the supervisor system. The changes allow specifying a custom filename when uploading a backup file. This modification spans the API layer, backup manager, and includes a corresponding test case to validate the new feature. The implementation provides more flexibility in handling backup file names while maintaining the existing core backup processes. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BackupAPI
participant BackupManager
participant FileSystem
Client->>BackupAPI: Upload backup with filename
BackupAPI->>BackupManager: import_backup(tar_file, filename)
BackupManager->>FileSystem: Determine tar file path
alt Filename provided
BackupManager->>FileSystem: Use custom filename
else No filename
BackupManager->>FileSystem: Use default slug
end
BackupManager->>FileSystem: Rename and store backup
BackupManager-->>BackupAPI: Backup imported
BackupAPI-->>Client: Upload successful
✨ Finishing Touches
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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
supervisor/api/backups.py(3 hunks)supervisor/backups/manager.py(3 hunks)tests/api/test_backups.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (4)
tests/api/test_backups.py (1)
719-740: LGTM! Comprehensive test coverage for the new filename parameter.The test thoroughly verifies:
- Successful upload with custom filename
- Correct storage of the backup file
- Proper mapping in backup locations
- Consistent backup slug
supervisor/api/backups.py (1)
29-29: LGTM!The import of
ATTR_FILENAMEconstant is correctly placed and used in the upload method.supervisor/backups/manager.py (2)
354-354: LGTM!The filename parameter is correctly typed and follows Python's parameter ordering conventions.
366-370: LGTM!The implementation:
- Safely extracts the filename using Path.name
- Maintains backward compatibility
- Preserves existing error handling
Also applies to: 372-372, 381-381
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
supervisor/api/backups.py (1)
488-491:⚠️ Potential issueAdd validation for the filename parameter.
The filename from the query parameter should be validated to prevent security issues such as path traversal attacks and ensure it's a valid backup filename.
The existing
RE_BACKUP_FILENAMEpattern should be used to validate the filename before using it.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supervisor/api/backups.py(3 hunks)tests/api/test_backups.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/api/test_backups.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run tests Python 3.13.1
- GitHub Check: Build armv7 supervisor
- GitHub Check: Build armhf supervisor
- GitHub Check: Build aarch64 supervisor
🔇 Additional comments (2)
supervisor/api/backups.py (2)
29-29: LGTM!The constant import is correctly placed and follows the project's naming conventions.
518-521: LGTM!The
import_backupcall is correctly modified to include the filename parameter while maintaining code style consistency and proper async operation protection.
| filename: str | None = None | ||
| if ATTR_FILENAME in request.query: | ||
| filename = request.query.get(ATTR_FILENAME) | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Documentation update needed for the filename parameter
The codebase review confirms that the new filename parameter is not documented. The API endpoints are documented through docstrings, but the new parameter is missing from:
- The endpoint's docstring
- Any general API documentation
This makes it harder for users to discover and properly use this new feature.
🔗 Analysis chain
Consider updating the API documentation.
This new feature introduces a query parameter that should be documented to help users understand:
- The purpose and usage of the filename parameter
- The filename format requirements
- Any security considerations
Let's check if there's existing API documentation that needs to be updated:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for API documentation files
fd -e md -e rst . | grep -i "api\|doc\|backup"
Length of output: 48
Script:
#!/bin/bash
# Search for documentation and API schema files using multiple approaches
echo "=== Searching for documentation files ==="
find . -type f -name "*.md" -o -name "*.rst" -o -name "*.txt" | grep -i "doc\|api\|backup"
echo -e "\n=== Searching for API schema files ==="
find . -type f -name "*.json" -o -name "*.yaml" -o -name "*.yml" | grep -i "api\|schema"
echo -e "\n=== Searching file contents for API documentation ==="
rg -l "API" --type md --type rst
Length of output: 35678
Script:
#!/bin/bash
# Check README content and search for backup API documentation
echo "=== README.md content ==="
cat README.md | grep -A 5 -i "api\|backup"
echo -e "\n=== Searching for backup API documentation in Python files ==="
rg -l "backup.*api" --type py
echo -e "\n=== Checking for API docstrings in backup endpoints ==="
rg -A 5 '"""' supervisor/api/backups.py
Length of output: 5865
| @@ -484,6 +485,10 @@ async def upload(self, request: web.Request): | |||
| if location and location != LOCATION_CLOUD_BACKUP: | |||
| tmp_path = location.local_where | |||
|
|
|||
| filename: str | None = None | |||
| if ATTR_FILENAME in request.query: | |||
There was a problem hiding this comment.
We need to validate this using the same RE_BACKUP_FILENAME regex we used here:
supervisor/supervisor/api/backups.py
Line 104 in 61a2101
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
mdegat01
added
missing-documentation
Proposed change
Add a query parameter which allows to specify the file name on upload. All locations will store the backup with the same file name.
Type of change
Additional information
Checklist
ruff format supervisor tests)If API endpoints or add-on configuration are added/changed:
Summary by CodeRabbit
Release Notes
New Features
Tests
These updates provide users with greater flexibility when uploading backups, enabling the definition of custom filenames during the backup process.