Remove file description dependency in onedrive (#162012)

homeassistant/components/onedrive/__init__.py

@@ -14,7 +14,6 @@ from onedrive_personal_sdk.exceptions import (
     NotFoundError,
     OneDriveException,
 )
-from onedrive_personal_sdk.models.items import ItemUpdate
 
 from homeassistant.const import CONF_ACCESS_TOKEN, Platform
 from homeassistant.core import HomeAssistant
@@ -72,15 +71,6 @@ async def async_setup_entry(hass: HomeAssistant, entry: OneDriveConfigEntry) ->
             entry, data={**entry.data, CONF_FOLDER_ID: backup_folder.id}
         )
 
-    # write instance id to description
-    if backup_folder.description != (instance_id := await async_get_instance_id(hass)):
-        await _handle_item_operation(
-            lambda: client.update_drive_item(
-                backup_folder.id, ItemUpdate(description=instance_id)
-            ),
-            folder_name,
-        )
-
     # update in case folder was renamed manually inside OneDrive
     if backup_folder.name != entry.data[CONF_FOLDER_NAME]:
         hass.config_entries.async_update_entry(
@@ -122,7 +112,11 @@ async def async_unload_entry(hass: HomeAssistant, entry: OneDriveConfigEntry) ->
 
 
 async def _migrate_backup_files(client: OneDriveClient, backup_folder_id: str) -> None:
-    """Migrate backup files to metadata version 2."""
+    """Migrate backup files from metadata version 1 to version 2.
+
+    Version 1: Backup metadata was stored in the backup file's description field.
+    Version 2: Backup metadata is stored in a separate .metadata.json file.
+    """
     files = await client.list_drive_items(backup_folder_id)
     for file in files:
         if file.description and '"metadata_version": 1' in (
@@ -131,24 +125,11 @@ async def _migrate_backup_files(client: OneDriveClient, backup_folder_id: str) -
             metadata = loads(metadata_json)
             del metadata["metadata_version"]
             metadata_filename = file.name.rsplit(".", 1)[0] + ".metadata.json"
-            metadata_file = await client.upload_file(
+            await client.upload_file(
                 backup_folder_id,
                 metadata_filename,
                 dumps(metadata),
             )
-            metadata_description = {
-                "metadata_version": 2,
-                "backup_id": metadata["backup_id"],
-                "backup_file_id": file.id,
-            }
-            await client.update_drive_item(
-                path_or_id=metadata_file.id,
-                data=ItemUpdate(description=dumps(metadata_description)),
-            )
-            await client.update_drive_item(
-                path_or_id=file.id,
-                data=ItemUpdate(description=""),
-            )
             _LOGGER.debug("Migrated backup file %s", file.name)
 
 

homeassistant/components/onedrive/backup.py

@@ -3,10 +3,7 @@
 from __future__ import annotations
 
 from collections.abc import AsyncIterator, Callable, Coroutine
-from dataclasses import dataclass
 from functools import wraps
-from html import unescape
-from json import dumps, loads
 import logging
 from time import time
 from typing import Any, Concatenate
@@ -18,7 +15,6 @@ from onedrive_personal_sdk.exceptions import (
     HashMismatchError,
     OneDriveException,
 )
-from onedrive_personal_sdk.models.items import ItemUpdate
 from onedrive_personal_sdk.models.upload import FileInfo
 
 from homeassistant.components.backup import (
@@ -30,6 +26,8 @@ from homeassistant.components.backup import (
 )
 from homeassistant.core import HomeAssistant, callback
 from homeassistant.helpers.aiohttp_client import async_get_clientsession
+from homeassistant.helpers.json import json_dumps
+from homeassistant.util.json import json_loads_object
 
 from .const import CONF_DELETE_PERMANENTLY, DATA_BACKUP_AGENT_LISTENERS, DOMAIN
 from .coordinator import OneDriveConfigEntry
@@ -38,7 +36,6 @@ _LOGGER = logging.getLogger(__name__)
 MAX_CHUNK_SIZE = 60 * 1024 * 1024  # largest chunk possible, must be <= 60 MiB
 TARGET_CHUNKS = 20
 TIMEOUT = ClientTimeout(connect=10, total=43200)  # 12 hours
-METADATA_VERSION = 2
 CACHE_TTL = 300
 
 
@@ -104,13 +101,10 @@ def handle_backup_errors[_R, **P](
     return wrapper
 
 
-@dataclass(kw_only=True)
-class OneDriveBackup:
-    """Define a OneDrive backup."""
-
-    backup: AgentBackup
-    backup_file_id: str
-    metadata_file_id: str
+def suggested_filenames(backup: AgentBackup) -> tuple[str, str]:
+    """Return the suggested filenames for the backup and metadata."""
+    base_name = suggested_filename(backup).rsplit(".", 1)[0]
+    return f"{base_name}.tar", f"{base_name}.metadata.json"
 
 
 class OneDriveBackupAgent(BackupAgent):
@@ -129,7 +123,7 @@ class OneDriveBackupAgent(BackupAgent):
         self.name = entry.title
         assert entry.unique_id
         self.unique_id = entry.unique_id
-        self._backup_cache: dict[str, OneDriveBackup] = {}
+        self._cache_backup_metadata: dict[str, AgentBackup] = {}
         self._cache_expiration = time()
 
     @handle_backup_errors
@@ -137,12 +131,11 @@ class OneDriveBackupAgent(BackupAgent):
         self, backup_id: str, **kwargs: Any
     ) -> AsyncIterator[bytes]:
         """Download a backup file."""
-        backups = await self._list_cached_backups()
-        if backup_id not in backups:
-            raise BackupNotFound(f"Backup {backup_id} not found")
+        backup = await self._find_backup_by_id(backup_id)
+        backup_filename, _ = suggested_filenames(backup)
 
         stream = await self._client.download_drive_item(
-            backups[backup_id].backup_file_id, timeout=TIMEOUT
+            f"{self._folder_id}:/{backup_filename}:", timeout=TIMEOUT
         )
         return stream.iter_chunked(1024)
 
@@ -155,9 +148,9 @@ class OneDriveBackupAgent(BackupAgent):
         **kwargs: Any,
     ) -> None:
         """Upload a backup."""
-        filename = suggested_filename(backup)
+        backup_filename, metadata_filename = suggested_filenames(backup)
         file = FileInfo(
-            filename,
+            backup_filename,
             backup.size,
             self._folder_id,
             await open_stream(),
@@ -173,7 +166,7 @@ class OneDriveBackupAgent(BackupAgent):
         upload_chunk_size = max(upload_chunk_size, 320 * 1024)
 
         try:
-            backup_file = await LargeFileUploadClient.upload(
+            await LargeFileUploadClient.upload(
                 self._token_function,
                 file,
                 upload_chunk_size=upload_chunk_size,
@@ -185,35 +178,27 @@ class OneDriveBackupAgent(BackupAgent):
                 "Hash validation failed, backup file might be corrupt"
             ) from err
 
-        # store metadata in metadata file
-        description = dumps(backup.as_dict())
-        _LOGGER.debug("Creating metadata: %s", description)
-        metadata_filename = filename.rsplit(".", 1)[0] + ".metadata.json"
+        _LOGGER.debug("Uploaded backup to %s", backup_filename)
+
+        # Store metadata in separate metadata file (just backup.as_dict(), no extra fields)
+        metadata_content = json_dumps(backup.as_dict())
         try:
-            metadata_file = await self._client.upload_file(
+            await self._client.upload_file(
                 self._folder_id,
                 metadata_filename,
-                description,
+                metadata_content,
             )
         except OneDriveException:
-            await self._client.delete_drive_item(backup_file.id)
+            # Clean up the backup file if metadata upload fails
+            _LOGGER.debug(
+                "Uploading metadata failed, deleting backup file %s", backup_filename
+            )
+            await self._client.delete_drive_item(
+                f"{self._folder_id}:/{backup_filename}:"
+            )
             raise
 
-        # add metadata to the metadata file
-        metadata_description = {
-            "metadata_version": METADATA_VERSION,
-            "backup_id": backup.backup_id,
-            "backup_file_id": backup_file.id,
-        }
-        try:
-            await self._client.update_drive_item(
-                path_or_id=metadata_file.id,
-                data=ItemUpdate(description=dumps(metadata_description)),
-            )
-        except OneDriveException:
-            await self._client.delete_drive_item(backup_file.id)
-            await self._client.delete_drive_item(metadata_file.id)
-            raise
+        _LOGGER.debug("Uploaded metadata file %s", metadata_filename)
         self._cache_expiration = time()
 
     @handle_backup_errors
@@ -223,66 +208,63 @@ class OneDriveBackupAgent(BackupAgent):
         **kwargs: Any,
     ) -> None:
         """Delete a backup file."""
-        backups = await self._list_cached_backups()
-        if backup_id not in backups:
-            raise BackupNotFound(f"Backup {backup_id} not found")
-
-        backup = backups[backup_id]
+        backup = await self._find_backup_by_id(backup_id)
+        backup_filename, metadata_filename = suggested_filenames(backup)
 
         delete_permanently = self._entry.options.get(CONF_DELETE_PERMANENTLY, False)
 
-        await self._client.delete_drive_item(backup.backup_file_id, delete_permanently)
         await self._client.delete_drive_item(
-            backup.metadata_file_id, delete_permanently
+            f"{self._folder_id}:/{backup_filename}:", delete_permanently
         )
+        await self._client.delete_drive_item(
+            f"{self._folder_id}:/{metadata_filename}:", delete_permanently
+        )
+
+        _LOGGER.debug("Deleted backup %s", backup_filename)
         self._cache_expiration = time()
 
     @handle_backup_errors
     async def async_list_backups(self, **kwargs: Any) -> list[AgentBackup]:
         """List backups."""
-        return [
-            backup.backup for backup in (await self._list_cached_backups()).values()
-        ]
+        return list((await self._list_cached_metadata_files()).values())
 
     @handle_backup_errors
     async def async_get_backup(self, backup_id: str, **kwargs: Any) -> AgentBackup:
         """Return a backup."""
-        backups = await self._list_cached_backups()
-        if backup_id not in backups:
-            raise BackupNotFound(f"Backup {backup_id} not found")
-        return backups[backup_id].backup
+        return await self._find_backup_by_id(backup_id)
 
-    async def _list_cached_backups(self) -> dict[str, OneDriveBackup]:
-        """List backups with a cache."""
+    async def _list_cached_metadata_files(self) -> dict[str, AgentBackup]:
+        """List metadata files with a cache."""
         if time() <= self._cache_expiration:
-            return self._backup_cache
+            return self._cache_backup_metadata
 
-        items = await self._client.list_drive_items(self._folder_id)
-
-        async def download_backup_metadata(item_id: str) -> AgentBackup | None:
+        async def _download_metadata(item_id: str) -> AgentBackup | None:
+            """Download metadata file."""
             try:
                 metadata_stream = await self._client.download_drive_item(item_id)
             except OneDriveException as err:
                 _LOGGER.warning("Error downloading metadata for %s: %s", item_id, err)
                 return None
-            metadata_json = loads(await metadata_stream.read())
-            return AgentBackup.from_dict(metadata_json)
 
-        backups: dict[str, OneDriveBackup] = {}
+            return AgentBackup.from_dict(
+                json_loads_object(await metadata_stream.read())
+            )
+
+        items = await self._client.list_drive_items(self._folder_id)
+        metadata_files: dict[str, AgentBackup] = {}
         for item in items:
-            if item.description and f'"metadata_version": {METADATA_VERSION}' in (
-                metadata_description_json := unescape(item.description)
-            ):
-                backup = await download_backup_metadata(item.id)
-                if backup is None:
-                    continue
-                metadata_description = loads(metadata_description_json)
-                backups[backup.backup_id] = OneDriveBackup(
-                    backup=backup,
-                    backup_file_id=metadata_description["backup_file_id"],
-                    metadata_file_id=item.id,
-                )
+            if item.name and item.name.endswith(".metadata.json"):
+                if metadata := await _download_metadata(item.id):
+                    metadata_files[metadata.backup_id] = metadata
 
+        self._cache_backup_metadata = metadata_files
         self._cache_expiration = time() + CACHE_TTL
-        self._backup_cache = backups
-        return backups
+        return self._cache_backup_metadata
+
+    async def _find_backup_by_id(self, backup_id: str) -> AgentBackup:
+        """Find a backup by its backup ID on remote."""
+        metadata_files = await self._list_cached_metadata_files()
+        if backup := metadata_files.get(backup_id):
+            return backup
+
+        raise BackupNotFound(f"Backup {backup_id} not found")

homeassistant/components/onedrive/config_flow.py

@@ -129,9 +129,6 @@ class OneDriveConfigFlow(AbstractOAuth2FlowHandler, domain=DOMAIN):
             except OneDriveException:
                 self.logger.debug("Failed to create folder", exc_info=True)
                 errors["base"] = "folder_creation_error"
-            else:
-                if folder.description and folder.description != instance_id:
-                    errors[CONF_FOLDER_NAME] = "folder_already_in_use"
             if not errors:
                 title = (
                     f"{self.approot.created_by.user.display_name}'s OneDrive"

homeassistant/components/onedrive/strings.json

@@ -22,7 +22,6 @@
       "default": "[%key:common::config_flow::create_entry::authenticated%]"
     },
     "error": {
-      "folder_already_in_use": "Folder already used for backups from another Home Assistant instance",
       "folder_creation_error": "Failed to create folder",
       "folder_rename_error": "Failed to rename folder"
     },

tests/components/onedrive/conftest.py

@@ -1,7 +1,6 @@
 """Fixtures for OneDrive tests."""
 
 from collections.abc import AsyncIterator, Generator
-from html import escape
 from json import dumps
 import time
 from unittest.mock import AsyncMock, MagicMock, patch
@@ -146,7 +145,6 @@ def mock_folder() -> Folder:
         name="name",
         size=0,
         child_count=0,
-        description="9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0",
         parent_reference=ItemParentReference(
             drive_id="mock_drive_id", id="id", path="path"
         ),
@@ -182,9 +180,9 @@ def mock_backup_file() -> File:
 def mock_metadata_file() -> File:
     """Return a mocked metadata file."""
     return File(
-        id="id",
-        name="23e64aec.tar",
-        size=34519040,
+        id="metadata_id",
+        name="23e64aec.metadata.json",
+        size=1024,
         parent_reference=ItemParentReference(
             drive_id="mock_drive_id", id="id", path="path"
         ),
@@ -192,15 +190,6 @@ def mock_metadata_file() -> File:
             quick_xor_hash="hash",
         ),
         mime_type="application/x-tar",
-        description=escape(
-            dumps(
-                {
-                    "metadata_version": 2,
-                    "backup_id": "23e64aec",
-                    "backup_file_id": "id",
-                }
-            )
-        ),
         created_by=IDENTITY_SET,
     )
 
@@ -227,6 +216,7 @@ def mock_onedrive_client(
             yield b"backup data"
 
         async def read(self) -> bytes:
+            # Metadata contains just the backup metadata (no backup_file_id)
             return dumps(BACKUP_METADATA).encode()
 
     client.download_drive_item.return_value = MockStreamReader()

tests/components/onedrive/test_backup.py

@@ -209,7 +209,10 @@ async def test_agents_upload(
     assert resp.status == 201
     assert f"Uploading backup {test_backup.backup_id}" in caplog.text
     mock_large_file_upload_client.assert_called_once()
-    mock_onedrive_client.update_drive_item.assert_called_once()
+    # upload_file should be called for the metadata file
+    mock_onedrive_client.upload_file.assert_called_once()
+    # update_drive_item should not be called (no description updates)
+    assert mock_onedrive_client.update_drive_item.call_count == 0
 
 
 async def test_agents_upload_corrupt_upload(
@@ -284,42 +287,6 @@ async def test_agents_upload_metadata_upload_failed(
     assert mock_onedrive_client.update_drive_item.call_count == 0
 
 
-async def test_agents_upload_metadata_metadata_failed(
-    hass_client: ClientSessionGenerator,
-    caplog: pytest.LogCaptureFixture,
-    mock_onedrive_client: MagicMock,
-    mock_large_file_upload_client: AsyncMock,
-    mock_config_entry: MockConfigEntry,
-) -> None:
-    """Test metadata upload on file description update."""
-    client = await hass_client()
-    test_backup = AgentBackup.from_dict(BACKUP_METADATA)
-    mock_onedrive_client.update_drive_item.side_effect = OneDriveException("test")
-
-    with (
-        patch(
-            "homeassistant.components.backup.manager.BackupManager.async_get_backup",
-        ) as fetch_backup,
-        patch(
-            "homeassistant.components.backup.manager.read_backup",
-            return_value=test_backup,
-        ),
-        patch("pathlib.Path.open") as mocked_open,
-    ):
-        mocked_open.return_value.read = Mock(side_effect=[b"test", b""])
-        fetch_backup.return_value = test_backup
-        resp = await client.post(
-            f"/api/backup/upload?agent_id={DOMAIN}.{mock_config_entry.unique_id}",
-            data={"file": StringIO("test")},
-        )
-
-    assert resp.status == 201
-    assert f"Uploading backup {test_backup.backup_id}" in caplog.text
-    mock_large_file_upload_client.assert_called_once()
-    assert mock_onedrive_client.update_drive_item.call_count == 1
-    assert mock_onedrive_client.delete_drive_item.call_count == 2
-
-
 async def test_agents_download(
     hass_client: ClientSessionGenerator,
     mock_onedrive_client: MagicMock,

tests/components/onedrive/test_config_flow.py

@@ -4,7 +4,7 @@ from http import HTTPStatus
 from unittest.mock import AsyncMock, MagicMock
 
 from onedrive_personal_sdk.exceptions import OneDriveException
-from onedrive_personal_sdk.models.items import AppRoot, Folder, ItemUpdate
+from onedrive_personal_sdk.models.items import AppRoot, ItemUpdate
 import pytest
 
 from homeassistant import config_entries
@@ -141,49 +141,6 @@ async def test_full_flow_with_owner_not_found(
     mock_onedrive_client.reset_mock()
 
 
-@pytest.mark.usefixtures("current_request_with_host")
-async def test_folder_already_in_use(
-    hass: HomeAssistant,
-    hass_client_no_auth: ClientSessionGenerator,
-    aioclient_mock: AiohttpClientMocker,
-    mock_setup_entry: AsyncMock,
-    mock_onedrive_client: MagicMock,
-    mock_instance_id: AsyncMock,
-    mock_folder: Folder,
-) -> None:
-    """Ensure a folder that is already in use is not allowed."""
-
-    mock_folder.description = "1234"
-
-    result = await hass.config_entries.flow.async_init(
-        DOMAIN, context={"source": config_entries.SOURCE_USER}
-    )
-    await _do_get_token(hass, result, hass_client_no_auth, aioclient_mock)
-    result = await hass.config_entries.flow.async_configure(result["flow_id"])
-
-    assert result["type"] is FlowResultType.FORM
-    result = await hass.config_entries.flow.async_configure(
-        result["flow_id"], {CONF_FOLDER_NAME: "myFolder"}
-    )
-
-    assert result["type"] is FlowResultType.FORM
-    assert result["errors"] == {CONF_FOLDER_NAME: "folder_already_in_use"}
-
-    # clear error and try again
-    mock_onedrive_client.create_folder.return_value.description = mock_instance_id
-
-    result = await hass.config_entries.flow.async_configure(
-        result["flow_id"], {CONF_FOLDER_NAME: "myFolder"}
-    )
-    assert result["type"] is FlowResultType.CREATE_ENTRY
-    assert result["title"] == "John Doe's OneDrive"
-    assert result["result"].unique_id == "mock_drive_id"
-    assert result["data"][CONF_TOKEN][CONF_ACCESS_TOKEN] == "mock-access-token"
-    assert result["data"][CONF_TOKEN]["refresh_token"] == "mock-refresh-token"
-    assert result["data"][CONF_FOLDER_NAME] == "myFolder"
-    assert result["data"][CONF_FOLDER_ID] == "my_folder_id"
-
-
 @pytest.mark.usefixtures("current_request_with_host")
 async def test_error_during_folder_creation(
     hass: HomeAssistant,

tests/components/onedrive/test_init.py

@@ -11,7 +11,7 @@ from onedrive_personal_sdk.exceptions import (
     NotFoundError,
     OneDriveException,
 )
-from onedrive_personal_sdk.models.items import AppRoot, Drive, File, Folder, ItemUpdate
+from onedrive_personal_sdk.models.items import AppRoot, Drive, File, Folder
 import pytest
 from syrupy.assertion import SnapshotAssertion
 
@@ -28,7 +28,7 @@ from homeassistant.helpers.config_entry_oauth2_flow import (
 )
 
 from . import setup_integration
-from .const import BACKUP_METADATA, INSTANCE_ID
+from .const import BACKUP_METADATA
 
 from tests.common import MockConfigEntry
 
@@ -128,38 +128,27 @@ async def test_get_integration_folder_creation_error(
     assert "Failed to get backups_123 folder" in caplog.text
 
 
-async def test_update_instance_id_description(
-    hass: HomeAssistant,
-    mock_config_entry: MockConfigEntry,
-    mock_onedrive_client: MagicMock,
-    mock_folder: Folder,
-) -> None:
-    """Test we write the instance id to the folder."""
-    mock_folder.description = ""
-    await setup_integration(hass, mock_config_entry)
-    await hass.async_block_till_done()
-
-    mock_onedrive_client.update_drive_item.assert_called_with(
-        mock_folder.id, ItemUpdate(description=INSTANCE_ID)
-    )
-
-
 async def test_migrate_metadata_files(
     hass: HomeAssistant,
     mock_config_entry: MockConfigEntry,
     mock_onedrive_client: MagicMock,
     mock_backup_file: File,
 ) -> None:
-    """Test migration of metadata files."""
+    """Test migration of metadata files from v1 to v2.
+
+    V1 stored metadata in the backup file's description field.
+    V2 stores metadata in a separate .metadata.json file.
+    """
     mock_backup_file.description = escape(
         dumps({**BACKUP_METADATA, "metadata_version": 1})
     )
     await setup_integration(hass, mock_config_entry)
     await hass.async_block_till_done()
 
+    # Should upload a new metadata file
     mock_onedrive_client.upload_file.assert_called_once()
-    assert mock_onedrive_client.update_drive_item.call_count == 2
-    assert mock_onedrive_client.update_drive_item.call_args[1]["data"].description == ""
+    # No longer updates descriptions (we don't rely on them anymore)
+    assert mock_onedrive_client.update_drive_item.call_count == 0
 
 
 async def test_migrate_metadata_files_errors(

tests/components/onedrive/test_services.py

@@ -93,7 +93,7 @@ async def test_upload_service(
 
     assert response
     assert response["files"]
-    assert cast(list[dict[str, Any]], response["files"])[0]["id"] == "id"
+    assert cast(list[dict[str, Any]], response["files"])[0]["id"] == "metadata_id"
 
 
 async def test_upload_service_no_response(