Remove old roborock map storage (home-assistant#157379)

Author: allenporter

homeassistant/components/roborock/__init__.py

@@ -33,7 +33,7 @@
     RoborockWashingMachineUpdateCoordinator,
     RoborockWetDryVacUpdateCoordinator,
 )
-from .roborock_storage import CacheStore, async_remove_map_storage
+from .roborock_storage import CacheStore, async_cleanup_map_storage
 
 SCAN_INTERVAL = timedelta(seconds=30)
 
@@ -42,6 +42,7 @@
 
 async def async_setup_entry(hass: HomeAssistant, entry: RoborockConfigEntry) -> bool:
     """Set up roborock from a config entry."""
+    await async_cleanup_map_storage(hass, entry.entry_id)
 
     user_data = UserData.from_dict(entry.data[CONF_USER_DATA])
     user_params = UserParams(
@@ -245,6 +246,5 @@ async def async_unload_entry(hass: HomeAssistant, entry: RoborockConfigEntry) ->
 
 async def async_remove_entry(hass: HomeAssistant, entry: RoborockConfigEntry) -> None:
     """Handle removal of an entry."""
-    await async_remove_map_storage(hass, entry.entry_id)
     store = CacheStore(hass, entry.entry_id)
     await store.async_remove()

homeassistant/components/roborock/roborock_storage.py

@@ -25,22 +25,22 @@ def _storage_path_prefix(hass: HomeAssistant, entry_id: str) -> Path:
     return Path(hass.config.path(STORAGE_PATH)) / entry_id
 
 
-async def async_remove_map_storage(hass: HomeAssistant, entry_id: str) -> None:
-    """Remove all map storage associated with a config entry.
+async def async_cleanup_map_storage(hass: HomeAssistant, entry_id: str) -> None:
+    """Remove map storage in the old format, if any.
 
     This removes all on-disk map files for the given config entry. This is the
     old format that was replaced by the `CacheStore` implementation.
     """
 
     def remove(path_prefix: Path) -> None:
         try:
-            if path_prefix.exists():
+            if path_prefix.exists() and path_prefix.is_dir():
+                _LOGGER.debug("Removing maps from disk store: %s", path_prefix)
                 shutil.rmtree(path_prefix, ignore_errors=True)
         except OSError as err:
             _LOGGER.error("Unable to remove map files in %s: %s", path_prefix, err)
 
     path_prefix = _storage_path_prefix(hass, entry_id)
-    _LOGGER.debug("Removing maps from disk store: %s", path_prefix)
     await hass.async_add_executor_job(remove, path_prefix)
 
 

tests/components/roborock/test_init.py

@@ -52,25 +52,77 @@ async def test_reauth_started(
 
 
 @pytest.mark.parametrize("platforms", [[Platform.IMAGE]])
-async def test_oserror_remove_image(
+@pytest.mark.parametrize(
+    ("exists", "is_dir", "rmtree_called"),
+    [
+        (True, True, True),
+        (False, False, False),
+        (True, False, False),
+    ],
+    ids=[
+        "old_storage_removed",
+        "new_storage_ignored",
+        "no_existing_storage",
+    ],
+)
+async def test_remove_old_storage_directory(
     hass: HomeAssistant,
-    setup_entry: MockConfigEntry,
+    mock_roborock_entry: MockConfigEntry,
     storage_path: pathlib.Path,
     hass_client: ClientSessionGenerator,
     caplog: pytest.LogCaptureFixture,
+    exists: bool,
+    is_dir: bool,
+    rmtree_called: bool,
 ) -> None:
-    """Test that we gracefully handle failing to remove old map storage."""
+    """Test cleanup of old old map storage."""
+    with (
+        patch(
+            "homeassistant.components.roborock.roborock_storage.Path.exists",
+            return_value=exists,
+        ),
+        patch(
+            "homeassistant.components.roborock.roborock_storage.Path.is_dir",
+            return_value=is_dir,
+        ),
+        patch(
+            "homeassistant.components.roborock.roborock_storage.shutil.rmtree",
+        ) as mock_rmtree,
+    ):
+        await hass.config_entries.async_setup(mock_roborock_entry.entry_id)
+        await hass.async_block_till_done()
+        assert mock_roborock_entry.state is ConfigEntryState.LOADED
 
+    assert mock_rmtree.called == rmtree_called
+
+
+@pytest.mark.parametrize("platforms", [[Platform.IMAGE]])
+async def test_oserror_remove_storage_directory(
+    hass: HomeAssistant,
+    mock_roborock_entry: MockConfigEntry,
+    storage_path: pathlib.Path,
+    hass_client: ClientSessionGenerator,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """Test that we gracefully handle failing to remove old map storage."""
     with (
         patch(
             "homeassistant.components.roborock.roborock_storage.Path.exists",
+            return_value=True,
+        ),
+        patch(
+            "homeassistant.components.roborock.roborock_storage.Path.is_dir",
+            return_value=True,
         ),
         patch(
             "homeassistant.components.roborock.roborock_storage.shutil.rmtree",
             side_effect=OSError,
         ) as mock_rmtree,
     ):
-        await hass.config_entries.async_remove(setup_entry.entry_id)
+        await hass.config_entries.async_setup(mock_roborock_entry.entry_id)
+        await hass.async_block_till_done()
+        assert mock_roborock_entry.state is ConfigEntryState.LOADED
+
     assert mock_rmtree.called
     assert "Unable to remove map files" in caplog.text