Add location to backup download and remove APIs #5482
Conversation
📝 WalkthroughWalkthroughThe changes introduce a new schema for removing backups in the Changes
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: 0
🧹 Outside diff range and nitpick comments (5)
supervisor/api/backups.py (2)
422-431: Consider enhancing error handling for location validation.While the implementation is correct, consider adding more specific error messages when location validation fails to help with debugging.
if ATTR_LOCATION in body: self._validate_cloud_backup_location(request, body[ATTR_LOCATION]) - locations = [self._location_to_mount(name) for name in body[ATTR_LOCATION]] + try: + locations = [self._location_to_mount(name) for name in body[ATTR_LOCATION]] + except APIError as err: + raise APIError(f"Invalid backup location: {err}") from err
437-444: Consider adding debug logging for location selection.The location handling logic is correct, but adding debug logging would help track which location is being used for downloads.
location = request.query.get(ATTR_LOCATION, backup.location) or None self._validate_cloud_backup_location(request, location) +_LOGGER.debug("Downloading backup %s from location %s", backup.slug, location) if location not in backup.all_locations: raise APIError(f"Backup {backup.slug} is not in location {location}")tests/api/test_backups.py (2)
752-772: Consider adding test for removing from multiple locations.The test covers the basic case well, but consider adding a test case for removing a backup from multiple locations simultaneously.
@pytest.mark.usefixtures("tmp_supervisor_data") async def test_remove_backup_from_multiple_locations(api_client: TestClient, coresys: CoreSys): """Test removing a backup from multiple locations.""" backup_file = get_fixture_path("backup_example.tar") location_1 = Path(copy(backup_file, coresys.config.path_backup)) location_2 = Path(copy(backup_file, coresys.config.path_core_backup)) location_3 = Path(copy(backup_file, coresys.config.path_mounts / "backup_test")) await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) resp = await api_client.delete( "/backups/7fed74c8", json={"location": [".cloud_backup", "backup_test"]} ) assert resp.status == 200 assert location_1.exists() assert not location_2.exists() assert not location_3.exists() assert coresys.backups.get("7fed74c8")
774-802: Consider testing content integrity after download.The test verifies the download succeeds but doesn't verify the content integrity of the downloaded backup.
with out_file.open("wb") as out: out.write(await resp.read()) out_backup = Backup(coresys, out_file, "out", None) await out_backup.load() - assert backup == out_backup + # Verify backup content integrity + assert backup.slug == out_backup.slug + assert backup.name == out_backup.name + assert backup.date == out_backup.date + assert backup.compressed == out_backup.compressed + assert backup.protected == out_backup.protected + assert backup.homeassistant_version == out_backup.homeassistant_versiontests/backups/test_manager.py (1)
Line range hint
1980-1995: Consider adding error case test.The test covers the success case well, but consider adding a test for error handling when removal fails.
@pytest.mark.usefixtures("tmp_supervisor_data") async def test_backup_remove_multiple_locations_error(coresys: CoreSys): """Test error handling when removing backup from multiple locations.""" backup_file = get_fixture_path("backup_example.tar") location_1 = Path(copy(backup_file, coresys.config.path_backup)) location_2 = Path(copy(backup_file, coresys.config.path_core_backup)) await coresys.backups.reload() assert (backup := coresys.backups.get("7fed74c8")) with patch("pathlib.Path.unlink", side_effect=OSError("Test error")): assert not coresys.backups.remove(backup) assert location_1.exists() assert location_2.exists() assert coresys.backups.get("7fed74c8")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
supervisor/api/backups.py(2 hunks)tests/api/test_backups.py(1 hunks)tests/backups/test_manager.py(2 hunks)
🔇 Additional comments (3)
supervisor/api/backups.py (1)
129-135: LGTM! Schema definition is well-structured.
The schema correctly validates the optional location field and reuses the existing _ensure_list helper for consistent list handling.
tests/api/test_backups.py (1)
805-811: LGTM! Error handling test is comprehensive.
The test properly verifies the error case for attempting to download from an invalid location.
tests/backups/test_manager.py (1)
Line range hint 1997-2014: LGTM! Test case is well-structured.
The test thoroughly verifies the partial removal functionality while ensuring the backup remains accessible from other locations.
| location = request.query.get(ATTR_LOCATION, backup.location) or None | ||
| self._validate_cloud_backup_location(request, location) | ||
| if location not in backup.all_locations: | ||
| raise APIError(f"Backup {backup.slug} is not in location {location}") |
There was a problem hiding this comment.
Should that be an APINotFound error maybe? 🤔 I mean it is query string dependent, so not REST, maybe 404 is not appropriate then 🤔
There was a problem hiding this comment.
Yea that was my thought process. The resource is the backup not the location so I left that as a 400, it felt more like invalid input. I could go either way though I suppose.
Proposed change
Add location option to download and remove backup API to support cloud backup.
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
Bug Fixes
Tests