diff --git a/homeassistant/components/onedrive/__init__.py b/homeassistant/components/onedrive/__init__.py index 84d738e37a7..b137c7725f1 100644 --- a/homeassistant/components/onedrive/__init__.py +++ b/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) diff --git a/homeassistant/components/onedrive/backup.py b/homeassistant/components/onedrive/backup.py index a851871dbbd..38a5ab8c565 100644 --- a/homeassistant/components/onedrive/backup.py +++ b/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") diff --git a/homeassistant/components/onedrive/config_flow.py b/homeassistant/components/onedrive/config_flow.py index 3374c0369ee..bb301a94511 100644 --- a/homeassistant/components/onedrive/config_flow.py +++ b/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" diff --git a/homeassistant/components/onedrive/strings.json b/homeassistant/components/onedrive/strings.json index 14378b49ee4..89b5b48b08a 100644 --- a/homeassistant/components/onedrive/strings.json +++ b/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" }, diff --git a/tests/components/onedrive/conftest.py b/tests/components/onedrive/conftest.py index 74232f2cc39..40b61d5412a 100644 --- a/tests/components/onedrive/conftest.py +++ b/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() diff --git a/tests/components/onedrive/test_backup.py b/tests/components/onedrive/test_backup.py index 40a8def0e39..78e8964bcc8 100644 --- a/tests/components/onedrive/test_backup.py +++ b/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, diff --git a/tests/components/onedrive/test_config_flow.py b/tests/components/onedrive/test_config_flow.py index 81cd44bd041..6badce87150 100644 --- a/tests/components/onedrive/test_config_flow.py +++ b/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, diff --git a/tests/components/onedrive/test_init.py b/tests/components/onedrive/test_init.py index 4cf6f286d50..53d82c249c0 100644 --- a/tests/components/onedrive/test_init.py +++ b/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( diff --git a/tests/components/onedrive/test_services.py b/tests/components/onedrive/test_services.py index 31d2d932cd0..fe2d6d5d75c 100644 --- a/tests/components/onedrive/test_services.py +++ b/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(