Improve backup file naming in Synology DSM backup agent (#137278)

* improve backup file naming

* use built-in suggested_filename

homeassistant/components/synology_dsm/backup.py

@@ -10,7 +10,12 @@ from aiohttp import StreamReader
 from synology_dsm.api.file_station import SynoFileStation
 from synology_dsm.exceptions import SynologyDSMAPIErrorException
 
-from homeassistant.components.backup import AgentBackup, BackupAgent, BackupAgentError
+from homeassistant.components.backup import (
+    AgentBackup,
+    BackupAgent,
+    BackupAgentError,
+    suggested_filename,
+)
 from homeassistant.config_entries import ConfigEntry
 from homeassistant.core import HomeAssistant, callback
 from homeassistant.helpers.aiohttp_client import ChunkAsyncStreamIterator
@@ -28,6 +33,15 @@ from .models import SynologyDSMData
 LOGGER = logging.getLogger(__name__)
 
 
+def suggested_filenames(backup: AgentBackup) -> tuple[str, str]:
+    """Suggest filenames for the backup.
+
+    returns a tuple of tar_filename and meta_filename
+    """
+    base_name = suggested_filename(backup).rsplit(".", 1)[0]
+    return (f"{base_name}.tar", f"{base_name}_meta.json")
+
+
 async def async_get_backup_agents(
     hass: HomeAssistant,
 ) -> list[BackupAgent]:
@@ -95,6 +109,19 @@ class SynologyDSMBackupAgent(BackupAgent):
             assert self.api.file_station
         return self.api.file_station
 
+    async def _async_suggested_filenames(
+        self,
+        backup_id: str,
+    ) -> tuple[str, str]:
+        """Suggest filenames for the backup.
+
+        :param backup_id: The ID of the backup that was returned in async_list_backups.
+        :return: A tuple of tar_filename and meta_filename
+        """
+        if (backup := await self.async_get_backup(backup_id)) is None:
+            raise BackupAgentError("Backup not found")
+        return suggested_filenames(backup)
+
     async def async_download_backup(
         self,
         backup_id: str,
@@ -105,10 +132,12 @@ class SynologyDSMBackupAgent(BackupAgent):
         :param backup_id: The ID of the backup that was returned in async_list_backups.
         :return: An async iterator that yields bytes.
         """
+        (filename_tar, _) = await self._async_suggested_filenames(backup_id)
+
         try:
             resp = await self._file_station.download_file(
                 path=self.path,
-                filename=f"{backup_id}.tar",
+                filename=filename_tar,
             )
         except SynologyDSMAPIErrorException as err:
             raise BackupAgentError("Failed to download backup") from err
@@ -131,11 +160,13 @@ class SynologyDSMBackupAgent(BackupAgent):
         :param backup: Metadata about the backup that should be uploaded.
         """
 
+        (filename_tar, filename_meta) = suggested_filenames(backup)
+
         # upload backup.tar file first
         try:
             await self._file_station.upload_file(
                 path=self.path,
-                filename=f"{backup.backup_id}.tar",
+                filename=filename_tar,
                 source=await open_stream(),
                 create_parents=True,
             )
@@ -146,7 +177,7 @@ class SynologyDSMBackupAgent(BackupAgent):
         try:
             await self._file_station.upload_file(
                 path=self.path,
-                filename=f"{backup.backup_id}_meta.json",
+                filename=filename_meta,
                 source=json_dumps(backup.as_dict()).encode(),
             )
         except SynologyDSMAPIErrorException as err:
@@ -161,7 +192,15 @@ class SynologyDSMBackupAgent(BackupAgent):
 
         :param backup_id: The ID of the backup that was returned in async_list_backups.
         """
-        for filename in (f"{backup_id}.tar", f"{backup_id}_meta.json"):
+        try:
+            (filename_tar, filename_meta) = await self._async_suggested_filenames(
+                backup_id
+            )
+        except BackupAgentError:
+            # backup meta data could not be found, so we can't delete the backup
+            return
+
+        for filename in (filename_tar, filename_meta):
             try:
                 await self._file_station.delete_file(path=self.path, filename=filename)
             except SynologyDSMAPIErrorException as err:

tests/components/synology_dsm/test_backup.py

@@ -36,6 +36,8 @@ from .consts import HOST, MACS, PASSWORD, PORT, SERIAL, USE_SSL, USERNAME
 from tests.common import MockConfigEntry
 from tests.typing import ClientSessionGenerator, WebSocketGenerator
 
+BASE_FILENAME = "Automatic_backup_2025.2.0.dev0_-_2025-01-09_20.14_35457323"
+
 
 class MockStreamReaderChunked(MockStreamReader):
     """Mock a stream reader with simulated chunked data."""
@@ -46,14 +48,14 @@ class MockStreamReaderChunked(MockStreamReader):
 
 
 async def _mock_download_file(path: str, filename: str) -> MockStreamReader:
-    if filename == "abcd12ef_meta.json":
+    if filename == f"{BASE_FILENAME}_meta.json":
         return MockStreamReader(
             b'{"addons":[],"backup_id":"abcd12ef","date":"2025-01-09T20:14:35.457323+01:00",'
             b'"database_included":true,"extra_metadata":{"instance_id":"36b3b7e984da43fc89f7bafb2645fa36",'
             b'"with_automatic_settings":true},"folders":[],"homeassistant_included":true,'
             b'"homeassistant_version":"2025.2.0.dev0","name":"Automatic backup 2025.2.0.dev0","protected":true,"size":13916160}'
         )
-    if filename == "abcd12ef.tar":
+    if filename == f"{BASE_FILENAME}.tar":
         return MockStreamReaderChunked(b"backup data")
     raise MockStreamReaderChunked(b"")
 
@@ -61,22 +63,22 @@ async def _mock_download_file(path: str, filename: str) -> MockStreamReader:
 async def _mock_download_file_meta_ok_tar_missing(
     path: str, filename: str
 ) -> MockStreamReader:
-    if filename == "abcd12ef_meta.json":
+    if filename == f"{BASE_FILENAME}_meta.json":
         return MockStreamReader(
             b'{"addons":[],"backup_id":"abcd12ef","date":"2025-01-09T20:14:35.457323+01:00",'
             b'"database_included":true,"extra_metadata":{"instance_id":"36b3b7e984da43fc89f7bafb2645fa36",'
             b'"with_automatic_settings":true},"folders":[],"homeassistant_included":true,'
             b'"homeassistant_version":"2025.2.0.dev0","name":"Automatic backup 2025.2.0.dev0","protected":true,"size":13916160}'
         )
-    if filename == "abcd12ef.tar":
+    if filename == f"{BASE_FILENAME}.tar":
-        raise SynologyDSMAPIErrorException("api", "404", "not found")
+        raise SynologyDSMAPIErrorException("api", "900", [{"code": 408}])
     raise MockStreamReaderChunked(b"")
 
 
 async def _mock_download_file_meta_defect(path: str, filename: str) -> MockStreamReader:
-    if filename == "abcd12ef_meta.json":
+    if filename == f"{BASE_FILENAME}_meta.json":
         return MockStreamReader(b"im not a json")
-    if filename == "abcd12ef.tar":
+    if filename == f"{BASE_FILENAME}.tar":
         return MockStreamReaderChunked(b"backup data")
     raise MockStreamReaderChunked(b"")
 
@@ -84,7 +86,6 @@ async def _mock_download_file_meta_defect(path: str, filename: str) -> MockStrea
 @pytest.fixture
 def mock_dsm_with_filestation():
     """Mock a successful service with filestation support."""
-
     with patch("homeassistant.components.synology_dsm.common.SynologyDSM") as dsm:
         dsm.login = AsyncMock(return_value=True)
         dsm.update = AsyncMock(return_value=True)
@@ -115,14 +116,14 @@ def mock_dsm_with_filestation():
                     SynoFileFile(
                         additional=None,
                         is_dir=False,
-                        name="abcd12ef_meta.json",
+                        name=f"{BASE_FILENAME}_meta.json",
-                        path="/ha_backup/my_backup_path/abcd12ef_meta.json",
+                        path=f"/ha_backup/my_backup_path/{BASE_FILENAME}_meta.json",
                     ),
                     SynoFileFile(
                         additional=None,
                         is_dir=False,
-                        name="abcd12ef.tar",
+                        name=f"{BASE_FILENAME}.tar",
-                        path="/ha_backup/my_backup_path/abcd12ef.tar",
+                        path=f"/ha_backup/my_backup_path/{BASE_FILENAME}.tar",
                     ),
                 ]
             ),
@@ -522,6 +523,7 @@ async def test_agents_upload(
         protected=True,
         size=0,
     )
+    base_filename = "Test_-_1970-01-01_00.00_00000000"
 
     with (
         patch(
@@ -544,9 +546,9 @@ async def test_agents_upload(
     assert f"Uploading backup {backup_id}" in caplog.text
     mock: AsyncMock = setup_dsm_with_filestation.file.upload_file
     assert len(mock.mock_calls) == 2
-    assert mock.call_args_list[0].kwargs["filename"] == "test-backup.tar"
+    assert mock.call_args_list[0].kwargs["filename"] == f"{base_filename}.tar"
     assert mock.call_args_list[0].kwargs["path"] == "/ha_backup/my_backup_path"
-    assert mock.call_args_list[1].kwargs["filename"] == "test-backup_meta.json"
+    assert mock.call_args_list[1].kwargs["filename"] == f"{base_filename}_meta.json"
     assert mock.call_args_list[1].kwargs["path"] == "/ha_backup/my_backup_path"
 
 
@@ -572,6 +574,7 @@ async def test_agents_upload_error(
         protected=True,
         size=0,
     )
+    base_filename = "Test_-_1970-01-01_00.00_00000000"
 
     # fail to upload the tar file
     with (
@@ -599,7 +602,7 @@ async def test_agents_upload_error(
     assert "Failed to upload backup" in caplog.text
     mock: AsyncMock = setup_dsm_with_filestation.file.upload_file
     assert len(mock.mock_calls) == 1
-    assert mock.call_args_list[0].kwargs["filename"] == "test-backup.tar"
+    assert mock.call_args_list[0].kwargs["filename"] == f"{base_filename}.tar"
     assert mock.call_args_list[0].kwargs["path"] == "/ha_backup/my_backup_path"
 
     # fail to upload the meta json file
@@ -630,9 +633,9 @@ async def test_agents_upload_error(
     assert "Failed to upload backup" in caplog.text
     mock: AsyncMock = setup_dsm_with_filestation.file.upload_file
     assert len(mock.mock_calls) == 3
-    assert mock.call_args_list[1].kwargs["filename"] == "test-backup.tar"
+    assert mock.call_args_list[1].kwargs["filename"] == f"{base_filename}.tar"
     assert mock.call_args_list[1].kwargs["path"] == "/ha_backup/my_backup_path"
-    assert mock.call_args_list[2].kwargs["filename"] == "test-backup_meta.json"
+    assert mock.call_args_list[2].kwargs["filename"] == f"{base_filename}_meta.json"
     assert mock.call_args_list[2].kwargs["path"] == "/ha_backup/my_backup_path"
 
 
@@ -657,9 +660,9 @@ async def test_agents_delete(
     assert response["result"] == {"agent_errors": {}}
     mock: AsyncMock = setup_dsm_with_filestation.file.delete_file
     assert len(mock.mock_calls) == 2
-    assert mock.call_args_list[0].kwargs["filename"] == "abcd12ef.tar"
+    assert mock.call_args_list[0].kwargs["filename"] == f"{BASE_FILENAME}.tar"
     assert mock.call_args_list[0].kwargs["path"] == "/ha_backup/my_backup_path"
-    assert mock.call_args_list[1].kwargs["filename"] == "abcd12ef_meta.json"
+    assert mock.call_args_list[1].kwargs["filename"] == f"{BASE_FILENAME}_meta.json"
     assert mock.call_args_list[1].kwargs["path"] == "/ha_backup/my_backup_path"
 
 
@@ -672,6 +675,9 @@ async def test_agents_delete_not_existing(
     client = await hass_ws_client(hass)
     backup_id = "ef34ab12"
 
+    setup_dsm_with_filestation.file.download_file = (
+        _mock_download_file_meta_ok_tar_missing
+    )
     setup_dsm_with_filestation.file.delete_file = AsyncMock(
         side_effect=SynologyDSMAPIErrorException(
             "api",
@@ -740,5 +746,5 @@ async def test_agents_delete_error(
     assert f"Failed to delete backup: {expected_log}" in caplog.text
     mock: AsyncMock = setup_dsm_with_filestation.file.delete_file
     assert len(mock.mock_calls) == 1
-    assert mock.call_args_list[0].kwargs["filename"] == "abcd12ef.tar"
+    assert mock.call_args_list[0].kwargs["filename"] == f"{BASE_FILENAME}.tar"
     assert mock.call_args_list[0].kwargs["path"] == "/ha_backup/my_backup_path"