From dc30210237cec0eb6374eaa2720fd7f7a23f607e Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Thu, 16 Feb 2023 13:01:41 -0600 Subject: [PATCH] Use blocking in intent service calls and verify results (#88035) * Use blocking in service calls and verify result * Block for 2 seconds and update states after * Small timeout in service call to allow exceptions * Move sun test --- homeassistant/components/intent/__init__.py | 13 ++++-- homeassistant/helpers/intent.py | 44 +++++++++++++----- tests/components/conversation/test_init.py | 49 ++++++++++++--------- tests/helpers/test_intent.py | 18 +++++++- 4 files changed, 89 insertions(+), 35 deletions(-) diff --git a/homeassistant/components/intent/__init__.py b/homeassistant/components/intent/__init__.py index e02d1486af5..a52f4897d23 100644 --- a/homeassistant/components/intent/__init__.py +++ b/homeassistant/components/intent/__init__.py @@ -75,10 +75,17 @@ class OnOffIntentHandler(intent.ServiceIntentHandler): else SERVICE_CLOSE_COVER, {ATTR_ENTITY_ID: state.entity_id}, context=intent_obj.context, + blocking=True, + limit=self.service_timeout, ) - else: - # Fall back to homeassistant.turn_on/off - await super().async_call_service(intent_obj, state) + + elif not hass.services.has_service(state.domain, self.service): + raise intent.IntentHandleError( + f"Service {self.service} does not support entity {state.entity_id}" + ) + + # Fall back to homeassistant.turn_on/off + await super().async_call_service(intent_obj, state) class GetStateIntentHandler(intent.IntentHandler): diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index fb338f9f139..4e7dcc5a5a1 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -335,6 +335,10 @@ class ServiceIntentHandler(IntentHandler): vol.Optional("device_class"): vol.All(cv.ensure_list, [cv.string]), } + # We use a small timeout in service calls to (hopefully) pass validation + # checks, but not try to wait for the call to fully complete. + service_timeout: float = 0.2 + def __init__( self, intent_type: str, domain: str, service: str, speech: str | None = None ) -> None: @@ -402,7 +406,8 @@ class ServiceIntentHandler(IntentHandler): area: area_registry.AreaEntry | None = None, ) -> IntentResponse: """Complete action on matched entity states.""" - assert states + assert states, "No states" + hass = intent_obj.hass success_results: list[IntentResponseTarget] = [] response = intent_obj.create_response() @@ -419,21 +424,36 @@ class ServiceIntentHandler(IntentHandler): service_coros = [] for state in states: service_coros.append(self.async_call_service(intent_obj, state)) - success_results.append( - IntentResponseTarget( - type=IntentResponseTargetType.ENTITY, - name=state.name, - id=state.entity_id, - ), + + # Handle service calls in parallel, noting failures as they occur. + failed_results: list[IntentResponseTarget] = [] + for state, service_coro in zip(states, asyncio.as_completed(service_coros)): + target = IntentResponseTarget( + type=IntentResponseTargetType.ENTITY, + name=state.name, + id=state.entity_id, ) - # Handle service calls in parallel. - # We will need to handle partial failures here. - await asyncio.gather(*service_coros) + try: + await service_coro + success_results.append(target) + except Exception: # pylint: disable=broad-except + failed_results.append(target) + _LOGGER.exception("Service call failed for %s", state.entity_id) + + if not success_results: + # If no entities succeeded, raise an error. + failed_entity_ids = [target.id for target in failed_results] + raise IntentHandleError( + f"Failed to call {self.service} for: {failed_entity_ids}" + ) response.async_set_results( - success_results=success_results, + success_results=success_results, failed_results=failed_results ) + + # Update all states + states = [hass.states.get(state.entity_id) or state for state in states] response.async_set_states(states) if self.speech is not None: @@ -449,6 +469,8 @@ class ServiceIntentHandler(IntentHandler): self.service, {ATTR_ENTITY_ID: state.entity_id}, context=intent_obj.context, + blocking=True, + limit=self.service_timeout, ) diff --git a/tests/components/conversation/test_init.py b/tests/components/conversation/test_init.py index 7cbc6d99776..3f60d389590 100644 --- a/tests/components/conversation/test_init.py +++ b/tests/components/conversation/test_init.py @@ -1,5 +1,6 @@ """The tests for the Conversation component.""" from http import HTTPStatus +from typing import Any from unittest.mock import patch import pytest @@ -7,8 +8,9 @@ import voluptuous as vol from homeassistant.components import conversation from homeassistant.components.cover import SERVICE_OPEN_COVER +from homeassistant.components.light import DOMAIN as LIGHT_DOMAIN from homeassistant.const import ATTR_FRIENDLY_NAME -from homeassistant.core import DOMAIN as HASS_DOMAIN, Context, HomeAssistant +from homeassistant.core import Context, HomeAssistant from homeassistant.helpers import ( area_registry, device_registry, @@ -59,13 +61,15 @@ async def test_http_processing_intent( entities.async_update_entity("light.kitchen", aliases={"my cool light"}) hass.states.async_set("light.kitchen", "off") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") client = await hass_client() - data = {"text": "turn on my cool light"} + data: dict[str, Any] = {"text": "turn on my cool light"} if agent_id: data["agent_id"] = agent_id resp = await client.post("/api/conversation/process", json=data) assert resp.status == HTTPStatus.OK + assert len(calls) == 1 data = await resp.json() assert data == { @@ -105,6 +109,7 @@ async def test_http_processing_intent_target_ha_agent( entities.async_update_entity("light.kitchen", aliases={"my cool light"}) hass.states.async_set("light.kitchen", "off") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") client = await hass_client() resp = await client.post( "/api/conversation/process", @@ -112,6 +117,7 @@ async def test_http_processing_intent_target_ha_agent( ) assert resp.status == HTTPStatus.OK + assert len(calls) == 1 data = await resp.json() assert data == { @@ -153,12 +159,14 @@ async def test_http_processing_intent_entity_added( er.async_update_entity("light.kitchen", aliases={"my cool light"}) hass.states.async_set("light.kitchen", "off") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") client = await hass_client() resp = await client.post( "/api/conversation/process", json={"text": "turn on my cool light"} ) assert resp.status == HTTPStatus.OK + assert len(calls) == 1 data = await resp.json() assert data == { @@ -284,7 +292,7 @@ async def test_turn_on_intent( ) -> None: """Test calling the turn on intent.""" hass.states.async_set("light.kitchen", "off") - calls = async_mock_service(hass, HASS_DOMAIN, "turn_on") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") data = {conversation.ATTR_TEXT: sentence} if agent_id is not None: @@ -294,16 +302,16 @@ async def test_turn_on_intent( assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_on" - assert call.data == {"entity_id": "light.kitchen"} + assert call.data == {"entity_id": ["light.kitchen"]} @pytest.mark.parametrize("sentence", ("turn off kitchen", "turn kitchen off")) async def test_turn_off_intent(hass: HomeAssistant, init_components, sentence) -> None: """Test calling the turn on intent.""" hass.states.async_set("light.kitchen", "on") - calls = async_mock_service(hass, HASS_DOMAIN, "turn_off") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_off") await hass.services.async_call( "conversation", "process", {conversation.ATTR_TEXT: sentence} @@ -312,9 +320,9 @@ async def test_turn_off_intent(hass: HomeAssistant, init_components, sentence) - assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_off" - assert call.data == {"entity_id": "light.kitchen"} + assert call.data == {"entity_id": ["light.kitchen"]} async def test_http_api_no_match( @@ -706,7 +714,7 @@ async def test_prepare_fail(hass: HomeAssistant) -> None: async def test_language_region(hass: HomeAssistant, init_components) -> None: """Test calling the turn on intent.""" hass.states.async_set("light.kitchen", "off") - calls = async_mock_service(hass, HASS_DOMAIN, "turn_on") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") # Add fake region language = f"{hass.config.language}-YZ" @@ -722,9 +730,9 @@ async def test_language_region(hass: HomeAssistant, init_components) -> None: assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_on" - assert call.data == {"entity_id": "light.kitchen"} + assert call.data == {"entity_id": ["light.kitchen"]} async def test_reload_on_new_component(hass: HomeAssistant) -> None: @@ -755,7 +763,7 @@ async def test_reload_on_new_component(hass: HomeAssistant) -> None: async def test_non_default_response(hass: HomeAssistant, init_components) -> None: """Test intent response that is not the default.""" hass.states.async_set("cover.front_door", "closed") - async_mock_service(hass, "cover", SERVICE_OPEN_COVER) + calls = async_mock_service(hass, "cover", SERVICE_OPEN_COVER) agent = await conversation._get_agent_manager(hass).async_get_agent() assert isinstance(agent, conversation.DefaultAgent) @@ -768,6 +776,7 @@ async def test_non_default_response(hass: HomeAssistant, init_components) -> Non language=hass.config.language, ) ) + assert len(calls) == 1 assert result.response.speech["plain"]["speech"] == "Opened front door" @@ -792,7 +801,7 @@ async def test_turn_on_area(hass: HomeAssistant, init_components) -> None: ) hass.states.async_set("light.stove", "off") - calls = async_mock_service(hass, HASS_DOMAIN, "turn_on") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") await hass.services.async_call( "conversation", @@ -803,9 +812,9 @@ async def test_turn_on_area(hass: HomeAssistant, init_components) -> None: assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_on" - assert call.data == {"entity_id": "light.stove"} + assert call.data == {"entity_id": ["light.stove"]} basement_area = ar.async_create("basement") dr.async_update_device(device.id, area_id=basement_area.id) @@ -832,9 +841,9 @@ async def test_turn_on_area(hass: HomeAssistant, init_components) -> None: assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_on" - assert call.data == {"entity_id": "light.stove"} + assert call.data == {"entity_id": ["light.stove"]} async def test_light_area_same_name(hass: HomeAssistant, init_components) -> None: @@ -868,7 +877,7 @@ async def test_light_area_same_name(hass: HomeAssistant, init_components) -> Non ceiling_light.entity_id, "off", attributes={ATTR_FRIENDLY_NAME: "ceiling light"} ) - calls = async_mock_service(hass, HASS_DOMAIN, "turn_on") + calls = async_mock_service(hass, LIGHT_DOMAIN, "turn_on") await hass.services.async_call( "conversation", @@ -880,9 +889,9 @@ async def test_light_area_same_name(hass: HomeAssistant, init_components) -> Non # Should only turn on one light instead of all lights in the kitchen assert len(calls) == 1 call = calls[0] - assert call.domain == HASS_DOMAIN + assert call.domain == LIGHT_DOMAIN assert call.service == "turn_on" - assert call.data == {"entity_id": kitchen_light.entity_id} + assert call.data == {"entity_id": [kitchen_light.entity_id]} async def test_agent_id_validator_invalid_agent(hass: HomeAssistant) -> None: diff --git a/tests/helpers/test_intent.py b/tests/helpers/test_intent.py index 0932433baa5..b975ae4bd72 100644 --- a/tests/helpers/test_intent.py +++ b/tests/helpers/test_intent.py @@ -3,9 +3,10 @@ import pytest import voluptuous as vol +from homeassistant.components import conversation from homeassistant.components.switch import SwitchDeviceClass from homeassistant.const import ATTR_FRIENDLY_NAME -from homeassistant.core import HomeAssistant, State +from homeassistant.core import Context, HomeAssistant, State from homeassistant.helpers import ( area_registry, config_validation as cv, @@ -13,6 +14,7 @@ from homeassistant.helpers import ( entity_registry, intent, ) +from homeassistant.setup import async_setup_component class MockIntentHandler(intent.IntentHandler): @@ -154,3 +156,17 @@ def test_async_validate_slots() -> None: handler1.async_validate_slots( {"name": {"value": "kitchen"}, "probability": {"value": "0.5"}} ) + + +async def test_cant_turn_on_sun(hass: HomeAssistant) -> None: + """Test we can't turn on entities that don't support it.""" + assert await async_setup_component(hass, "homeassistant", {}) + assert await async_setup_component(hass, "conversation", {}) + assert await async_setup_component(hass, "intent", {}) + assert await async_setup_component(hass, "sun", {}) + result = await conversation.async_converse( + hass, "turn on sun", None, Context(), None + ) + + assert result.response.response_type == intent.IntentResponseType.ERROR + assert result.response.error_code == intent.IntentResponseErrorCode.FAILED_TO_HANDLE