From 4cdc3de94a491fdf7fb98667666e3656b66806e5 Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Mon, 17 Feb 2025 15:38:28 +0100 Subject: [PATCH] Correct backup filename on delete or download of cloud backup (#138704) * Correct backup filename on delete or download of cloud backup * Improve tests * Address review comments --- homeassistant/components/cloud/backup.py | 43 +++++++++++------ tests/components/cloud/test_backup.py | 61 +++++++++++++++++++++--- 2 files changed, 83 insertions(+), 21 deletions(-) diff --git a/homeassistant/components/cloud/backup.py b/homeassistant/components/cloud/backup.py index 61edeccdd9c..b31fe16fbe9 100644 --- a/homeassistant/components/cloud/backup.py +++ b/homeassistant/components/cloud/backup.py @@ -11,7 +11,11 @@ from typing import Any from aiohttp import ClientError from hass_nabucasa import Cloud, CloudError from hass_nabucasa.api import CloudApiNonRetryableError -from hass_nabucasa.cloud_api import async_files_delete_file, async_files_list +from hass_nabucasa.cloud_api import ( + FilesHandlerListEntry, + async_files_delete_file, + async_files_list, +) from hass_nabucasa.files import FilesError, StorageType, calculate_b64md5 from homeassistant.components.backup import AgentBackup, BackupAgent, BackupAgentError @@ -76,11 +80,6 @@ class CloudBackupAgent(BackupAgent): self._cloud = cloud self._hass = hass - @callback - def _get_backup_filename(self) -> str: - """Return the backup filename.""" - return f"{self._cloud.client.prefs.instance_id}.tar" - async def async_download_backup( self, backup_id: str, @@ -91,13 +90,13 @@ class CloudBackupAgent(BackupAgent): :param backup_id: The ID of the backup that was returned in async_list_backups. :return: An async iterator that yields bytes. """ - if not await self.async_get_backup(backup_id): + if not (backup := await self._async_get_backup(backup_id)): raise BackupAgentError("Backup not found") try: content = await self._cloud.files.download( storage_type=StorageType.BACKUP, - filename=self._get_backup_filename(), + filename=backup["Key"], ) except CloudError as err: raise BackupAgentError(f"Failed to download backup: {err}") from err @@ -124,7 +123,7 @@ class CloudBackupAgent(BackupAgent): base64md5hash = await calculate_b64md5(open_stream, size) except FilesError as err: raise BackupAgentError(err) from err - filename = self._get_backup_filename() + filename = f"{self._cloud.client.prefs.instance_id}.tar" metadata = backup.as_dict() tries = 1 @@ -172,29 +171,34 @@ class CloudBackupAgent(BackupAgent): :param backup_id: The ID of the backup that was returned in async_list_backups. """ - if not await self.async_get_backup(backup_id): + if not (backup := await self._async_get_backup(backup_id)): return try: await async_files_delete_file( self._cloud, storage_type=StorageType.BACKUP, - filename=self._get_backup_filename(), + filename=backup["Key"], ) except (ClientError, CloudError) as err: raise BackupAgentError("Failed to delete backup") from err async def async_list_backups(self, **kwargs: Any) -> list[AgentBackup]: + """List backups.""" + backups = await self._async_list_backups() + return [AgentBackup.from_dict(backup["Metadata"]) for backup in backups] + + async def _async_list_backups(self) -> list[FilesHandlerListEntry]: """List backups.""" try: backups = await async_files_list( self._cloud, storage_type=StorageType.BACKUP ) - _LOGGER.debug("Cloud backups: %s", backups) except (ClientError, CloudError) as err: raise BackupAgentError("Failed to list backups") from err - return [AgentBackup.from_dict(backup["Metadata"]) for backup in backups] + _LOGGER.debug("Cloud backups: %s", backups) + return backups async def async_get_backup( self, @@ -202,10 +206,19 @@ class CloudBackupAgent(BackupAgent): **kwargs: Any, ) -> AgentBackup | None: """Return a backup.""" - backups = await self.async_list_backups() + if not (backup := await self._async_get_backup(backup_id)): + return None + return AgentBackup.from_dict(backup["Metadata"]) + + async def _async_get_backup( + self, + backup_id: str, + ) -> FilesHandlerListEntry | None: + """Return a backup.""" + backups = await self._async_list_backups() for backup in backups: - if backup.backup_id == backup_id: + if backup["Metadata"]["backup_id"] == backup_id: return backup return None diff --git a/tests/components/cloud/test_backup.py b/tests/components/cloud/test_backup.py index c6bb0bdad54..18793cc00bb 100644 --- a/tests/components/cloud/test_backup.py +++ b/tests/components/cloud/test_backup.py @@ -3,12 +3,12 @@ from collections.abc import AsyncGenerator, Generator from io import StringIO from typing import Any -from unittest.mock import Mock, PropertyMock, patch +from unittest.mock import ANY, Mock, PropertyMock, patch from aiohttp import ClientError from hass_nabucasa import CloudError from hass_nabucasa.api import CloudApiNonRetryableError -from hass_nabucasa.files import FilesError +from hass_nabucasa.files import FilesError, StorageType import pytest from homeassistant.components.backup import ( @@ -90,7 +90,26 @@ def mock_list_files() -> Generator[MagicMock]: "size": 34519040, "storage-type": "backup", }, - } + }, + { + "Key": "462e16810d6841228828d9dd2f9e341f.tar", + "LastModified": "2024-11-22T10:49:01.182Z", + "Size": 34519040, + "Metadata": { + "addons": [], + "backup_id": "23e64aed", + "date": "2024-11-22T11:48:48.727189+01:00", + "database_included": True, + "extra_metadata": {}, + "folders": [], + "homeassistant_included": True, + "homeassistant_version": "2024.12.0.dev0", + "name": "Core 2024.12.0.dev0", + "protected": False, + "size": 34519040, + "storage-type": "backup", + }, + }, ] yield list_files @@ -148,7 +167,21 @@ async def test_agents_list_backups( "name": "Core 2024.12.0.dev0", "failed_agent_ids": [], "with_automatic_settings": None, - } + }, + { + "addons": [], + "agents": {"cloud.cloud": {"protected": False, "size": 34519040}}, + "backup_id": "23e64aed", + "date": "2024-11-22T11:48:48.727189+01:00", + "database_included": True, + "extra_metadata": {}, + "folders": [], + "homeassistant_included": True, + "homeassistant_version": "2024.12.0.dev0", + "name": "Core 2024.12.0.dev0", + "failed_agent_ids": [], + "with_automatic_settings": None, + }, ] @@ -242,6 +275,10 @@ async def test_agents_download( resp = await client.get(f"/api/backup/download/{backup_id}?agent_id=cloud.cloud") assert resp.status == 200 assert await resp.content.read() == b"backup data" + cloud.files.download.assert_called_once_with( + filename="462e16810d6841228828d9dd2f9e341e.tar", + storage_type=StorageType.BACKUP, + ) @pytest.mark.usefixtures("cloud_logged_in", "mock_list_files") @@ -317,7 +354,14 @@ async def test_agents_upload( data={"file": StringIO(backup_data)}, ) - assert len(cloud.files.upload.mock_calls) == 1 + cloud.files.upload.assert_called_once_with( + storage_type=StorageType.BACKUP, + open_stream=ANY, + filename=f"{cloud.client.prefs.instance_id}.tar", + base64md5hash=ANY, + metadata=ANY, + size=ANY, + ) metadata = cloud.files.upload.mock_calls[-1].kwargs["metadata"] assert metadata["backup_id"] == backup_id @@ -552,6 +596,7 @@ async def test_agents_upload_wrong_size( async def test_agents_delete( hass: HomeAssistant, hass_ws_client: WebSocketGenerator, + cloud: Mock, mock_delete_file: Mock, ) -> None: """Test agent delete backup.""" @@ -568,7 +613,11 @@ async def test_agents_delete( assert response["success"] assert response["result"] == {"agent_errors": {}} - mock_delete_file.assert_called_once() + mock_delete_file.assert_called_once_with( + cloud, + filename="462e16810d6841228828d9dd2f9e341e.tar", + storage_type=StorageType.BACKUP, + ) @pytest.mark.parametrize("side_effect", [ClientError, CloudError])