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

@@ -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

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])