Fix swallowed exceptions in action handlers for ColorExtractor (#181537)

homeassistant/components/color_extractor/services.py

@@ -1,6 +1,7 @@
 """Module for color_extractor (RGB extraction from images) component."""
 
 import asyncio
+from http import HTTPStatus
 import io
 import logging
 from typing import Any
@@ -17,7 +18,7 @@ from homeassistant.components.light import (
 )
 from homeassistant.const import SERVICE_TURN_ON
 from homeassistant.core import HomeAssistant, ServiceCall, SupportsResponse, callback
-from homeassistant.exceptions import ServiceValidationError
+from homeassistant.exceptions import HomeAssistantError, ServiceValidationError
 from homeassistant.helpers import aiohttp_client, config_validation as cv
 
 from .const import ATTR_PATH, ATTR_URL, DOMAIN, SERVICE_GET_COLOR
@@ -67,17 +68,14 @@ def _get_color(file_handler: io.BytesIO | str) -> tuple[int, int, int]:
 
 async def _async_extract_color_from_url(
     hass: HomeAssistant, url: str
-) -> tuple[int, int, int] | None:
+) -> tuple[int, int, int]:
     """Handle call for URL based image."""
     if not hass.config.is_allowed_external_url(url):
-        _LOGGER.error(
-            (
-                "External URL '%s' is not allowed, please add to"
-                " 'allowlist_external_urls'"
-            ),
-            url,
+        raise ServiceValidationError(
+            translation_domain=DOMAIN,
+            translation_key="url_not_allowed",
+            translation_placeholders={"url": url},
         )
-        return None
 
     _LOGGER.debug("Getting predominant RGB from image URL '%s'", url)
 
@@ -85,14 +83,30 @@ async def _async_extract_color_from_url(
     try:
         session = aiohttp_client.async_get_clientsession(hass)
 
-        async with asyncio.timeout(10):
-            response = await session.get(url)
+        async with asyncio.timeout(10), session.get(url) as response:
+            if response.status != HTTPStatus.OK:
+                raise HomeAssistantError(
+                    translation_domain=DOMAIN,
+                    translation_key="http_error",
+                    translation_placeholders={
+                        "url": url,
+                        "status": str(response.status),
+                    },
+                )
+            content = await response.read()
 
-    except (TimeoutError, aiohttp.ClientError) as err:
-        _LOGGER.error("Failed to get ColorThief image due to HTTPError: %s", err)
-        return None
-
-    content = await response.content.read()
+    except TimeoutError as err:
+        raise HomeAssistantError(
+            translation_domain=DOMAIN,
+            translation_key="timeout",
+            translation_placeholders={"url": url},
+        ) from err
+    except aiohttp.ClientError as err:
+        raise HomeAssistantError(
+            translation_domain=DOMAIN,
+            translation_key="fetch_failed",
+            translation_placeholders={"url": url, "error": str(err)},
+        ) from err
 
     with io.BytesIO(content) as _file:
         _file.name = "color_extractor.jpg"
@@ -103,14 +117,14 @@ async def _async_extract_color_from_url(
 
 def _extract_color_from_path(
     hass: HomeAssistant, file_path: str
-) -> tuple[int, int, int] | None:
+) -> tuple[int, int, int]:
     """Handle call for local file based image."""
     if not hass.config.is_allowed_path(file_path):
-        _LOGGER.error(
-            "File path '%s' is not allowed, please add to 'allowlist_external_dirs'",
-            file_path,
+        raise ServiceValidationError(
+            translation_domain=DOMAIN,
+            translation_key="path_not_allowed",
+            translation_placeholders={"file_path": file_path},
         )
-        return None
 
     _LOGGER.debug("Getting predominant RGB from file path '%s'", file_path)
 
@@ -137,22 +151,21 @@ async def async_handle_service(service_call: ServiceCall) -> None:
                 _extract_color_from_path, service_call.hass, image_reference
             )
 
-    # pylint: disable-next=home-assistant-action-swallowed-exception
     except UnidentifiedImageError as ex:
-        _LOGGER.error(
-            "Bad image from %s '%s' provided, are you sure it's an image? %s",
-            image_type,
-            image_reference,
-            ex,
-        )
-        return
+        raise ServiceValidationError(
+            translation_domain=DOMAIN,
+            translation_key="invalid_image",
+            translation_placeholders={
+                "image_type": image_type,
+                "image_reference": image_reference,
+            },
+        ) from ex
 
-    if color:
-        service_data[ATTR_RGB_COLOR] = color
+    service_data[ATTR_RGB_COLOR] = color
 
-        await service_call.hass.services.async_call(
-            LIGHT_DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
-        )
+    await service_call.hass.services.async_call(
+        LIGHT_DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+    )
 
 
 async def async_handle_get_color(
@@ -186,16 +199,6 @@ async def async_handle_get_color(
             },
         ) from ex
 
-    if color is None:
-        raise ServiceValidationError(
-            translation_domain=DOMAIN,
-            translation_key="invalid_image",
-            translation_placeholders={
-                "image_type": image_type,
-                "image_reference": image_reference,
-            },
-        )
-
     return {"color": color}
 
 

homeassistant/components/color_extractor/strings.json

@@ -7,8 +7,23 @@
     }
   },
   "exceptions": {
+    "fetch_failed": {
+      "message": "Failed to fetch the image from {url}: {error}"
+    },
+    "http_error": {
+      "message": "Failed to fetch the image from {url}: the server responded with HTTP status {status}."
+    },
     "invalid_image": {
       "message": "Bad image {image_reference} from {image_type} provided, are you sure it's an image?"
+    },
+    "path_not_allowed": {
+      "message": "Path {file_path} is not allowed, add it to allowlist_external_dirs."
+    },
+    "timeout": {
+      "message": "Timed out fetching the image from {url}."
+    },
+    "url_not_allowed": {
+      "message": "URL {url} is not allowed, add it to allowlist_external_urls."
     }
   },
   "services": {

tests/components/color_extractor/test_services.py

@@ -26,7 +26,7 @@ from homeassistant.components.light import (
 )
 from homeassistant.const import ATTR_ENTITY_ID, STATE_OFF, STATE_ON
 from homeassistant.core import HomeAssistant
-from homeassistant.exceptions import ServiceValidationError
+from homeassistant.exceptions import HomeAssistantError, ServiceValidationError
 from homeassistant.setup import async_setup_component
 from homeassistant.util import color as color_util
 
@@ -175,13 +175,22 @@ async def test_url_success(
 async def test_url_not_allowed(
     hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration
 ) -> None:
-    """Test that a not allowed external URL fails to turn light on."""
+    """Test that a not allowed external URL raises and doesn't turn light on."""
     service_data = {
         ATTR_URL: "http://denied.com/images/logo.png",
         ATTR_ENTITY_ID: LIGHT_ENTITY,
     }
 
-    await _async_execute_service(hass, service_data)
+    with pytest.raises(ServiceValidationError) as exc_info:
+        await hass.services.async_call(
+            DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "url_not_allowed"
+    assert exc_info.value.translation_placeholders == {
+        "url": "http://denied.com/images/logo.png"
+    }
 
     # Light has not been modified due to failure
     state = hass.states.get(LIGHT_ENTITY)
@@ -189,10 +198,24 @@ async def test_url_not_allowed(
     assert state.state == STATE_OFF
 
 
+@pytest.mark.parametrize(
+    ("exc", "translation_key", "placeholder_keys"),
+    [
+        pytest.param(
+            aiohttp.ClientError, "fetch_failed", {"url", "error"}, id="client_error"
+        ),
+        pytest.param(TimeoutError, "timeout", {"url"}, id="timeout"),
+    ],
+)
+@pytest.mark.usefixtures("setup_integration")
 async def test_url_exception(
-    hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration
+    hass: HomeAssistant,
+    aioclient_mock: AiohttpClientMocker,
+    exc: type[Exception],
+    translation_key: str,
+    placeholder_keys: set[str],
 ) -> None:
-    """Test that a HTTPError fails to turn light on."""
+    """Test that a failed image download raises and doesn't turn light on."""
     service_data = {
         ATTR_URL: "http://example.com/images/logo.png",
         ATTR_ENTITY_ID: LIGHT_ENTITY,
@@ -201,10 +224,19 @@ async def test_url_exception(
     # Don't let the URL not being allowed sway our exception test
     hass.config.allowlist_external_urls.add("http://example.com/images/")
 
-    # Mock the HTTP Response with an HTTPError
-    aioclient_mock.get(url=service_data[ATTR_URL], exc=aiohttp.ClientError)
+    aioclient_mock.get(url=service_data[ATTR_URL], exc=exc)
 
-    await _async_execute_service(hass, service_data)
+    with pytest.raises(HomeAssistantError) as exc_info:
+        await hass.services.async_call(
+            DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == translation_key
+    placeholders = exc_info.value.translation_placeholders
+    assert placeholders is not None
+    assert set(placeholders) == placeholder_keys
+    assert placeholders["url"] == service_data[ATTR_URL]
 
     # Light has not been modified due to failure
     state = hass.states.get(LIGHT_ENTITY)
@@ -212,10 +244,18 @@ async def test_url_exception(
     assert state.state == STATE_OFF
 
 
+@pytest.mark.parametrize(
+    "status",
+    [
+        pytest.param(400, id="bad_request"),
+        pytest.param(304, id="not_modified"),
+    ],
+)
+@pytest.mark.usefixtures("setup_integration")
 async def test_url_error(
-    hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, setup_integration
+    hass: HomeAssistant, aioclient_mock: AiohttpClientMocker, status: int
 ) -> None:
-    """Test that a HTTP Error (non 200) doesn't turn light on."""
+    """Test that a non-OK HTTP status raises and doesn't turn light on."""
     service_data = {
         ATTR_URL: "http://example.com/images/logo.png",
         ATTR_ENTITY_ID: LIGHT_ENTITY,
@@ -224,10 +264,26 @@ async def test_url_error(
     # Don't let the URL not being allowed sway our exception test
     hass.config.allowlist_external_urls.add("http://example.com/images/")
 
-    # Mock the HTTP Response with a 400 Bad Request error
-    aioclient_mock.get(url=service_data[ATTR_URL], status=400)
+    aioclient_mock.get(url=service_data[ATTR_URL], status=status)
 
-    await _async_execute_service(hass, service_data)
+    # The body of a non-OK response must not be downloaded at all
+    with (
+        patch(
+            "tests.test_util.aiohttp.AiohttpClientMockResponse.read",
+            side_effect=AssertionError("body read for a non-OK response"),
+        ),
+        pytest.raises(HomeAssistantError) as exc_info,
+    ):
+        await hass.services.async_call(
+            DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "http_error"
+    assert exc_info.value.translation_placeholders == {
+        "url": service_data[ATTR_URL],
+        "status": str(status),
+    }
 
     # Light has not been modified due to failure
     state = hass.states.get(LIGHT_ENTITY)
@@ -298,7 +354,7 @@ async def test_file(hass: HomeAssistant, setup_integration) -> None:
 @patch("os.path.isfile", Mock(return_value=True))
 @patch("os.access", Mock(return_value=True))
 async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None:
-    """Test file service fails for images in disallowed dirs."""
+    """Test file service raises for images in disallowed dirs."""
     service_data = {
         ATTR_PATH: "/path/to/a/dir/not/allowed/image.png",
         ATTR_ENTITY_ID: LIGHT_ENTITY,
@@ -311,12 +367,16 @@ async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None:
     assert state
     assert state.state == STATE_OFF
 
-    # Mock the file handler read with our 1x1 base64 encoded fixture image
-    with patch(
-        "homeassistant.components.color_extractor.services._get_file", _get_file_mock
-    ):
-        await hass.services.async_call(DOMAIN, SERVICE_TURN_ON, service_data)
-        await hass.async_block_till_done()
+    with pytest.raises(ServiceValidationError) as exc_info:
+        await hass.services.async_call(
+            DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "path_not_allowed"
+    assert exc_info.value.translation_placeholders == {
+        "file_path": "/path/to/a/dir/not/allowed/image.png"
+    }
 
     state = hass.states.get(LIGHT_ENTITY)
 
@@ -326,6 +386,61 @@ async def test_file_denied_dir(hass: HomeAssistant, setup_integration) -> None:
     assert state.state == STATE_OFF
 
 
+@pytest.mark.parametrize(
+    ("image_attr", "image_reference", "image_type"),
+    [
+        pytest.param(ATTR_PATH, "/opt/not_an_image.txt", "file path", id="file_path"),
+        pytest.param(
+            ATTR_URL, "http://example.com/images/not_an_image.txt", "URL", id="url"
+        ),
+    ],
+)
+@pytest.mark.usefixtures("setup_integration")
+@patch("os.path.isfile", Mock(return_value=True))
+@patch("os.access", Mock(return_value=True))
+async def test_turn_on_invalid_image(
+    hass: HomeAssistant,
+    aioclient_mock: AiohttpClientMocker,
+    image_attr: str,
+    image_reference: str,
+    image_type: str,
+) -> None:
+    """Test that the turn_on service raises a ServiceValidationError when given an invalid image."""
+    service_data = {
+        image_attr: image_reference,
+        ATTR_ENTITY_ID: LIGHT_ENTITY,
+    }
+
+    hass.config.allowlist_external_dirs.add("/opt/")
+    hass.config.allowlist_external_urls.add("http://example.com/images/")
+    aioclient_mock.get(
+        url="http://example.com/images/not_an_image.txt", content=b"not an image"
+    )
+
+    with (
+        patch(
+            "homeassistant.components.color_extractor.services._get_file",
+            Mock(return_value=io.BytesIO(b"not an image")),
+        ),
+        pytest.raises(ServiceValidationError) as exc_info,
+    ):
+        await hass.services.async_call(
+            DOMAIN, SERVICE_TURN_ON, service_data, blocking=True
+        )
+
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "invalid_image"
+    assert exc_info.value.translation_placeholders == {
+        "image_type": image_type,
+        "image_reference": image_reference,
+    }
+
+    # The light must stay untouched when the image cannot be read
+    state = hass.states.get(LIGHT_ENTITY)
+    assert state
+    assert state.state == STATE_OFF
+
+
 @patch("os.path.isfile", Mock(return_value=True))
 @patch("os.access", Mock(return_value=True))
 async def test_get_color_service(hass: HomeAssistant, setup_integration) -> None:
@@ -390,19 +505,13 @@ async def test_get_color_service_not_allowed_path(
         ATTR_PATH: "/opt/not_an_image.txt",
     }
 
-    with (
-        patch(
-            "homeassistant.components.color_extractor.services._get_file",
-            Mock(side_effect=UnidentifiedImageError("Cannot identify image file")),
-        ),
-        pytest.raises(ServiceValidationError) as exc_info,
-    ):
+    with pytest.raises(ServiceValidationError) as exc_info:
         await hass.services.async_call(
             DOMAIN, SERVICE_GET_COLOR, service_data, blocking=True, return_response=True
         )
 
-    assert exc_info.value.translation_key == "invalid_image"
+    assert exc_info.value.translation_domain == DOMAIN
+    assert exc_info.value.translation_key == "path_not_allowed"
     assert exc_info.value.translation_placeholders == {
-        "image_type": "file path",
-        "image_reference": "/opt/not_an_image.txt",
+        "file_path": "/opt/not_an_image.txt",
     }