From 9f469c08d14d04b6ce364822f821706dfd05d130 Mon Sep 17 00:00:00 2001 From: G Johansson Date: Fri, 6 Sep 2024 15:35:38 +0200 Subject: [PATCH] Code quality improvement on local_file (#125165) * Code quality improvement on local_file * Fix * No translation * Review comments --- homeassistant/components/local_file/camera.py | 86 +++++++----------- .../components/local_file/services.yaml | 9 +- .../components/local_file/strings.json | 9 +- tests/components/local_file/test_camera.py | 91 ++++++++++++++++--- 4 files changed, 121 insertions(+), 74 deletions(-) diff --git a/homeassistant/components/local_file/camera.py b/homeassistant/components/local_file/camera.py index 1306751f1a9..74d887b613f 100644 --- a/homeassistant/components/local_file/camera.py +++ b/homeassistant/components/local_file/camera.py @@ -12,13 +12,14 @@ from homeassistant.components.camera import ( PLATFORM_SCHEMA as CAMERA_PLATFORM_SCHEMA, Camera, ) -from homeassistant.const import ATTR_ENTITY_ID, CONF_FILE_PATH, CONF_NAME -from homeassistant.core import HomeAssistant, ServiceCall -from homeassistant.helpers import config_validation as cv +from homeassistant.const import CONF_FILE_PATH, CONF_NAME +from homeassistant.core import HomeAssistant +from homeassistant.exceptions import PlatformNotReady, ServiceValidationError +from homeassistant.helpers import config_validation as cv, entity_platform from homeassistant.helpers.entity_platform import AddEntitiesCallback from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType -from .const import DATA_LOCAL_FILE, DEFAULT_NAME, DOMAIN, SERVICE_UPDATE_FILE_PATH +from .const import DEFAULT_NAME, SERVICE_UPDATE_FILE_PATH _LOGGER = logging.getLogger(__name__) @@ -29,57 +30,45 @@ PLATFORM_SCHEMA = CAMERA_PLATFORM_SCHEMA.extend( } ) -CAMERA_SERVICE_UPDATE_FILE_PATH = vol.Schema( - { - vol.Required(ATTR_ENTITY_ID): cv.comp_entity_ids, - vol.Required(CONF_FILE_PATH): cv.string, - } -) + +def check_file_path_access(file_path: str) -> bool: + """Check that filepath given is readable.""" + if not os.access(file_path, os.R_OK): + return False + return True -def setup_platform( +async def async_setup_platform( hass: HomeAssistant, config: ConfigType, - add_entities: AddEntitiesCallback, + async_add_entities: AddEntitiesCallback, discovery_info: DiscoveryInfoType | None = None, ) -> None: """Set up the Camera that works with local files.""" - if DATA_LOCAL_FILE not in hass.data: - hass.data[DATA_LOCAL_FILE] = [] + file_path: str = config[CONF_FILE_PATH] - file_path = config[CONF_FILE_PATH] - camera = LocalFile(config[CONF_NAME], file_path) - hass.data[DATA_LOCAL_FILE].append(camera) - - def update_file_path_service(call: ServiceCall) -> None: - """Update the file path.""" - file_path = call.data[CONF_FILE_PATH] - entity_ids = call.data[ATTR_ENTITY_ID] - cameras = hass.data[DATA_LOCAL_FILE] - - for camera in cameras: - if camera.entity_id in entity_ids: - camera.update_file_path(file_path) - - hass.services.register( - DOMAIN, + platform = entity_platform.async_get_current_platform() + platform.async_register_entity_service( SERVICE_UPDATE_FILE_PATH, - update_file_path_service, - schema=CAMERA_SERVICE_UPDATE_FILE_PATH, + { + vol.Required(CONF_FILE_PATH): cv.string, + }, + "update_file_path", ) - add_entities([camera]) + if not await hass.async_add_executor_job(check_file_path_access, file_path): + raise PlatformNotReady(f"File path {file_path} is not readable") + + async_add_entities([LocalFile(config[CONF_NAME], file_path)]) class LocalFile(Camera): """Representation of a local file camera.""" - def __init__(self, name, file_path): + def __init__(self, name: str, file_path: str) -> None: """Initialize Local File Camera component.""" super().__init__() - - self._name = name - self.check_file_path_access(file_path) + self._attr_name = name self._file_path = file_path # Set content type of local file content, _ = mimetypes.guess_type(file_path) @@ -96,30 +85,21 @@ class LocalFile(Camera): except FileNotFoundError: _LOGGER.warning( "Could not read camera %s image from file: %s", - self._name, + self.name, self._file_path, ) return None - def check_file_path_access(self, file_path): - """Check that filepath given is readable.""" - if not os.access(file_path, os.R_OK): - _LOGGER.warning( - "Could not read camera %s image from file: %s", self._name, file_path - ) - - def update_file_path(self, file_path): + async def update_file_path(self, file_path: str) -> None: """Update the file_path.""" - self.check_file_path_access(file_path) + if not await self.hass.async_add_executor_job( + check_file_path_access, file_path + ): + raise ServiceValidationError(f"Path {file_path} is not accessible") self._file_path = file_path self.schedule_update_ha_state() @property - def name(self): - """Return the name of this camera.""" - return self._name - - @property - def extra_state_attributes(self): + def extra_state_attributes(self) -> dict[str, str]: """Return the camera state attributes.""" return {"file_path": self._file_path} diff --git a/homeassistant/components/local_file/services.yaml b/homeassistant/components/local_file/services.yaml index 5fc0b11f4c2..1b3000e663e 100644 --- a/homeassistant/components/local_file/services.yaml +++ b/homeassistant/components/local_file/services.yaml @@ -1,10 +1,9 @@ update_file_path: + target: + entity: + integration: local_file + domain: camera fields: - entity_id: - required: true - selector: - entity: - domain: camera file_path: required: true example: "/config/www/images/image.jpg" diff --git a/homeassistant/components/local_file/strings.json b/homeassistant/components/local_file/strings.json index 0db5d709c69..801d85ce1e0 100644 --- a/homeassistant/components/local_file/strings.json +++ b/homeassistant/components/local_file/strings.json @@ -4,15 +4,16 @@ "name": "Updates file path", "description": "Use this action to change the file displayed by the camera.", "fields": { - "entity_id": { - "name": "Entity", - "description": "Name of the entity_id of the camera to update." - }, "file_path": { "name": "File path", "description": "The full path to the new image file to be displayed." } } } + }, + "exceptions": { + "file_path_not_accessible": { + "message": "Path {file_path} is not accessible" + } } } diff --git a/tests/components/local_file/test_camera.py b/tests/components/local_file/test_camera.py index 4455d47469c..132212df0ec 100644 --- a/tests/components/local_file/test_camera.py +++ b/tests/components/local_file/test_camera.py @@ -6,7 +6,9 @@ from unittest import mock import pytest from homeassistant.components.local_file.const import DOMAIN, SERVICE_UPDATE_FILE_PATH +from homeassistant.const import ATTR_ENTITY_ID, CONF_FILE_PATH from homeassistant.core import HomeAssistant +from homeassistant.exceptions import ServiceValidationError from homeassistant.setup import async_setup_component from tests.typing import ClientSessionGenerator @@ -71,9 +73,45 @@ async def test_file_not_readable( ) await hass.async_block_till_done() - assert "Could not read" in caplog.text - assert "config_test" in caplog.text - assert "mock.file" in caplog.text + assert "File path mock.file is not readable;" in caplog.text + + +async def test_file_not_readable_after_setup( + hass: HomeAssistant, + hass_client: ClientSessionGenerator, + caplog: pytest.LogCaptureFixture, +) -> None: + """Test a warning is shown setup when file is not readable.""" + with ( + mock.patch("os.path.isfile", mock.Mock(return_value=True)), + mock.patch("os.access", mock.Mock(return_value=True)), + mock.patch( + "homeassistant.components.local_file.camera.mimetypes.guess_type", + mock.Mock(return_value=(None, None)), + ), + ): + await async_setup_component( + hass, + "camera", + { + "camera": { + "name": "config_test", + "platform": "local_file", + "file_path": "mock.file", + } + }, + ) + await hass.async_block_till_done() + + client = await hass_client() + + with mock.patch( + "homeassistant.components.local_file.camera.open", side_effect=FileNotFoundError + ): + resp = await client.get("/api/camera_proxy/camera.config_test") + + assert resp.status == HTTPStatus.INTERNAL_SERVER_ERROR + assert "Could not read camera config_test image from file: mock.file" in caplog.text async def test_camera_content_type( @@ -100,13 +138,23 @@ async def test_camera_content_type( "platform": "local_file", "file_path": "/path/to/image", } - - await async_setup_component( - hass, - "camera", - {"camera": [cam_config_jpg, cam_config_png, cam_config_svg, cam_config_noext]}, - ) - await hass.async_block_till_done() + with ( + mock.patch("os.path.isfile", mock.Mock(return_value=True)), + mock.patch("os.access", mock.Mock(return_value=True)), + ): + await async_setup_component( + hass, + "camera", + { + "camera": [ + cam_config_jpg, + cam_config_png, + cam_config_svg, + cam_config_noext, + ] + }, + ) + await hass.async_block_till_done() client = await hass_client() @@ -169,8 +217,12 @@ async def test_update_file_path(hass: HomeAssistant) -> None: service_data = {"entity_id": "camera.local_file", "file_path": "new/path.jpg"} - await hass.services.async_call(DOMAIN, SERVICE_UPDATE_FILE_PATH, service_data) - await hass.async_block_till_done() + await hass.services.async_call( + DOMAIN, + SERVICE_UPDATE_FILE_PATH, + service_data, + blocking=True, + ) state = hass.states.get("camera.local_file") assert state.attributes.get("file_path") == "new/path.jpg" @@ -178,3 +230,18 @@ async def test_update_file_path(hass: HomeAssistant) -> None: # Check that local_file_camera_2 file_path is still as configured state = hass.states.get("camera.local_file_camera_2") assert state.attributes.get("file_path") == "mock/path_2.jpg" + + # Assert it fails if file is not readable + service_data = { + ATTR_ENTITY_ID: "camera.local_file", + CONF_FILE_PATH: "new/path2.jpg", + } + with pytest.raises( + ServiceValidationError, match="Path new/path2.jpg is not accessible" + ): + await hass.services.async_call( + DOMAIN, + SERVICE_UPDATE_FILE_PATH, + service_data, + blocking=True, + )