From 1750f54da4ee9baa1ac383665a1f67f9a2698393 Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Wed, 7 Feb 2024 15:13:42 -0600 Subject: [PATCH] Assist fixes (#109889) * Don't pass entity ids in hassil slot lists * Use first completed response * Add more tests --- homeassistant/components/climate/intent.py | 34 ++++-- .../components/conversation/default_agent.py | 36 +++--- homeassistant/components/intent/__init__.py | 7 +- homeassistant/helpers/intent.py | 15 ++- tests/components/climate/test_intent.py | 70 ++++++++++- .../conversation/snapshots/test_init.ambr | 8 +- .../conversation/test_default_agent.py | 93 +++++++++++++- tests/components/conversation/test_trigger.py | 115 +++++++++++++++++- tests/components/intent/test_init.py | 25 ++++ 9 files changed, 354 insertions(+), 49 deletions(-) diff --git a/homeassistant/components/climate/intent.py b/homeassistant/components/climate/intent.py index 4152fb5ee2d..db263451f0b 100644 --- a/homeassistant/components/climate/intent.py +++ b/homeassistant/components/climate/intent.py @@ -1,4 +1,5 @@ """Intents for the client integration.""" + from __future__ import annotations import voluptuous as vol @@ -36,24 +37,34 @@ class GetTemperatureIntent(intent.IntentHandler): if not entities: raise intent.IntentHandleError("No climate entities") - if "area" in slots: - # Filter by area - area_name = slots["area"]["value"] + name_slot = slots.get("name", {}) + entity_name: str | None = name_slot.get("value") + entity_text: str | None = name_slot.get("text") + + area_slot = slots.get("area", {}) + area_id = area_slot.get("value") + + if area_id: + # Filter by area and optionally name + area_name = area_slot.get("text") for maybe_climate in intent.async_match_states( - hass, area_name=area_name, domains=[DOMAIN] + hass, name=entity_name, area_name=area_id, domains=[DOMAIN] ): climate_state = maybe_climate break if climate_state is None: - raise intent.IntentHandleError(f"No climate entity in area {area_name}") + raise intent.NoStatesMatchedError( + name=entity_text or entity_name, + area=area_name or area_id, + domains={DOMAIN}, + device_classes=None, + ) climate_entity = component.get_entity(climate_state.entity_id) - elif "name" in slots: + elif entity_name: # Filter by name - entity_name = slots["name"]["value"] - for maybe_climate in intent.async_match_states( hass, name=entity_name, domains=[DOMAIN] ): @@ -61,7 +72,12 @@ class GetTemperatureIntent(intent.IntentHandler): break if climate_state is None: - raise intent.IntentHandleError(f"No climate entity named {entity_name}") + raise intent.NoStatesMatchedError( + name=entity_name, + area=None, + domains={DOMAIN}, + device_classes=None, + ) climate_entity = component.get_entity(climate_state.entity_id) else: diff --git a/homeassistant/components/conversation/default_agent.py b/homeassistant/components/conversation/default_agent.py index fb33d87e107..52925fbc241 100644 --- a/homeassistant/components/conversation/default_agent.py +++ b/homeassistant/components/conversation/default_agent.py @@ -223,22 +223,22 @@ class DefaultAgent(AbstractConversationAgent): # Check if a trigger matched if isinstance(result, SentenceTriggerResult): # Gather callback responses in parallel - trigger_responses = await asyncio.gather( - *( - self._trigger_sentences[trigger_id].callback( - result.sentence, trigger_result - ) - for trigger_id, trigger_result in result.matched_triggers.items() + trigger_callbacks = [ + self._trigger_sentences[trigger_id].callback( + result.sentence, trigger_result ) - ) + for trigger_id, trigger_result in result.matched_triggers.items() + ] # Use last non-empty result as response. # # There may be multiple copies of a trigger running when editing in # the UI, so it's critical that we filter out empty responses here. response_text: str | None = None - for trigger_response in trigger_responses: - response_text = response_text or trigger_response + for trigger_future in asyncio.as_completed(trigger_callbacks): + if trigger_response := await trigger_future: + response_text = trigger_response + break # Convert to conversation result response = intent.IntentResponse(language=language) @@ -724,7 +724,12 @@ class DefaultAgent(AbstractConversationAgent): if async_should_expose(self.hass, DOMAIN, state.entity_id) ] - # Gather exposed entity names + # Gather exposed entity names. + # + # NOTE: We do not pass entity ids in here because multiple entities may + # have the same name. The intent matcher doesn't gather all matching + # values for a list, just the first. So we will need to match by name no + # matter what. entity_names = [] for state in states: # Checked against "requires_context" and "excludes_context" in hassil @@ -740,7 +745,7 @@ class DefaultAgent(AbstractConversationAgent): if not entity: # Default name - entity_names.append((state.name, state.entity_id, context)) + entity_names.append((state.name, state.name, context)) continue if entity.aliases: @@ -748,12 +753,15 @@ class DefaultAgent(AbstractConversationAgent): if not alias.strip(): continue - entity_names.append((alias, state.entity_id, context)) + entity_names.append((alias, state.name, context)) # Default name - entity_names.append((state.name, state.entity_id, context)) + entity_names.append((state.name, state.name, context)) - # Expose all areas + # Expose all areas. + # + # We pass in area id here with the expectation that no two areas will + # share the same name or alias. areas = ar.async_get(self.hass) area_names = [] for area in areas.async_list_areas(): diff --git a/homeassistant/components/intent/__init__.py b/homeassistant/components/intent/__init__.py index 5756b78b4de..d032f535b06 100644 --- a/homeassistant/components/intent/__init__.py +++ b/homeassistant/components/intent/__init__.py @@ -1,4 +1,5 @@ """The Intent integration.""" + from __future__ import annotations import logging @@ -155,7 +156,7 @@ class GetStateIntentHandler(intent.IntentHandler): slots = self.async_validate_slots(intent_obj.slots) # Entity name to match - name: str | None = slots.get("name", {}).get("value") + entity_name: str | None = slots.get("name", {}).get("value") # Look up area first to fail early area_name = slots.get("area", {}).get("value") @@ -186,7 +187,7 @@ class GetStateIntentHandler(intent.IntentHandler): states = list( intent.async_match_states( hass, - name=name, + name=entity_name, area=area, domains=domains, device_classes=device_classes, @@ -197,7 +198,7 @@ class GetStateIntentHandler(intent.IntentHandler): _LOGGER.debug( "Found %s state(s) that matched: name=%s, area=%s, domains=%s, device_classes=%s, assistant=%s", len(states), - name, + entity_name, area, domains, device_classes, diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index d932332b4c0..c828b86264c 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -403,11 +403,11 @@ class ServiceIntentHandler(IntentHandler): slots = self.async_validate_slots(intent_obj.slots) name_slot = slots.get("name", {}) - entity_id: str | None = name_slot.get("value") - entity_name: str | None = name_slot.get("text") - if entity_id == "all": + entity_name: str | None = name_slot.get("value") + entity_text: str | None = name_slot.get("text") + if entity_name == "all": # Don't match on name if targeting all entities - entity_id = None + entity_name = None # Look up area first to fail early area_slot = slots.get("area", {}) @@ -436,7 +436,7 @@ class ServiceIntentHandler(IntentHandler): states = list( async_match_states( hass, - name=entity_id, + name=entity_name, area=area, domains=domains, device_classes=device_classes, @@ -447,7 +447,7 @@ class ServiceIntentHandler(IntentHandler): if not states: # No states matched constraints raise NoStatesMatchedError( - name=entity_name or entity_id, + name=entity_text or entity_name, area=area_name or area_id, domains=domains, device_classes=device_classes, @@ -455,6 +455,9 @@ class ServiceIntentHandler(IntentHandler): response = await self.async_handle_states(intent_obj, states, area) + # Make the matched states available in the response + response.async_set_states(matched_states=states, unmatched_states=[]) + return response async def async_handle_states( diff --git a/tests/components/climate/test_intent.py b/tests/components/climate/test_intent.py index 6473eca1b88..e4f92759793 100644 --- a/tests/components/climate/test_intent.py +++ b/tests/components/climate/test_intent.py @@ -1,4 +1,5 @@ """Test climate intents.""" + from collections.abc import Generator from unittest.mock import patch @@ -135,8 +136,10 @@ async def test_get_temperature( # Add climate entities to different areas: # climate_1 => living room # climate_2 => bedroom + # nothing in office living_room_area = area_registry.async_create(name="Living Room") bedroom_area = area_registry.async_create(name="Bedroom") + office_area = area_registry.async_create(name="Office") entity_registry.async_update_entity( climate_1.entity_id, area_id=living_room_area.id @@ -158,7 +161,7 @@ async def test_get_temperature( hass, "test", climate_intent.INTENT_GET_TEMPERATURE, - {"area": {"value": "Bedroom"}}, + {"area": {"value": bedroom_area.name}}, ) assert response.response_type == intent.IntentResponseType.QUERY_ANSWER assert len(response.matched_states) == 1 @@ -179,6 +182,52 @@ async def test_get_temperature( state = response.matched_states[0] assert state.attributes["current_temperature"] == 22.0 + # Check area with no climate entities + with pytest.raises(intent.NoStatesMatchedError) as error: + response = await intent.async_handle( + hass, + "test", + climate_intent.INTENT_GET_TEMPERATURE, + {"area": {"value": office_area.name}}, + ) + + # Exception should contain details of what we tried to match + assert isinstance(error.value, intent.NoStatesMatchedError) + assert error.value.name is None + assert error.value.area == office_area.name + assert error.value.domains == {DOMAIN} + assert error.value.device_classes is None + + # Check wrong name + with pytest.raises(intent.NoStatesMatchedError) as error: + response = await intent.async_handle( + hass, + "test", + climate_intent.INTENT_GET_TEMPERATURE, + {"name": {"value": "Does not exist"}}, + ) + + assert isinstance(error.value, intent.NoStatesMatchedError) + assert error.value.name == "Does not exist" + assert error.value.area is None + assert error.value.domains == {DOMAIN} + assert error.value.device_classes is None + + # Check wrong name with area + with pytest.raises(intent.NoStatesMatchedError) as error: + response = await intent.async_handle( + hass, + "test", + climate_intent.INTENT_GET_TEMPERATURE, + {"name": {"value": "Climate 1"}, "area": {"value": bedroom_area.name}}, + ) + + assert isinstance(error.value, intent.NoStatesMatchedError) + assert error.value.name == "Climate 1" + assert error.value.area == bedroom_area.name + assert error.value.domains == {DOMAIN} + assert error.value.device_classes is None + async def test_get_temperature_no_entities( hass: HomeAssistant, @@ -216,19 +265,28 @@ async def test_get_temperature_no_state( climate_1.entity_id, area_id=living_room_area.id ) - with patch("homeassistant.core.StateMachine.get", return_value=None), pytest.raises( - intent.IntentHandleError + with ( + patch("homeassistant.core.StateMachine.get", return_value=None), + pytest.raises(intent.IntentHandleError), ): await intent.async_handle( hass, "test", climate_intent.INTENT_GET_TEMPERATURE, {} ) - with patch( - "homeassistant.core.StateMachine.async_all", return_value=[] - ), pytest.raises(intent.IntentHandleError): + with ( + patch("homeassistant.core.StateMachine.async_all", return_value=[]), + pytest.raises(intent.NoStatesMatchedError) as error, + ): await intent.async_handle( hass, "test", climate_intent.INTENT_GET_TEMPERATURE, {"area": {"value": "Living Room"}}, ) + + # Exception should contain details of what we tried to match + assert isinstance(error.value, intent.NoStatesMatchedError) + assert error.value.name is None + assert error.value.area == "Living Room" + assert error.value.domains == {DOMAIN} + assert error.value.device_classes is None diff --git a/tests/components/conversation/snapshots/test_init.ambr b/tests/components/conversation/snapshots/test_init.ambr index 034bfafc1f5..f4478941473 100644 --- a/tests/components/conversation/snapshots/test_init.ambr +++ b/tests/components/conversation/snapshots/test_init.ambr @@ -1397,7 +1397,7 @@ 'name': dict({ 'name': 'name', 'text': 'my cool light', - 'value': 'light.kitchen', + 'value': 'kitchen', }), }), 'intent': dict({ @@ -1422,7 +1422,7 @@ 'name': dict({ 'name': 'name', 'text': 'my cool light', - 'value': 'light.kitchen', + 'value': 'kitchen', }), }), 'intent': dict({ @@ -1572,7 +1572,7 @@ 'name': dict({ 'name': 'name', 'text': 'test light', - 'value': 'light.demo_1234', + 'value': 'test light', }), }), 'intent': dict({ @@ -1604,7 +1604,7 @@ 'name': dict({ 'name': 'name', 'text': 'test light', - 'value': 'light.demo_1234', + 'value': 'test light', }), }), 'intent': dict({ diff --git a/tests/components/conversation/test_default_agent.py b/tests/components/conversation/test_default_agent.py index 0cf343a3e20..d8a256608c8 100644 --- a/tests/components/conversation/test_default_agent.py +++ b/tests/components/conversation/test_default_agent.py @@ -101,7 +101,7 @@ async def test_exposed_areas( device_registry.async_update_device(kitchen_device.id, area_id=area_kitchen.id) kitchen_light = entity_registry.async_get_or_create("light", "demo", "1234") - entity_registry.async_update_entity( + kitchen_light = entity_registry.async_update_entity( kitchen_light.entity_id, device_id=kitchen_device.id ) hass.states.async_set( @@ -109,7 +109,7 @@ async def test_exposed_areas( ) bedroom_light = entity_registry.async_get_or_create("light", "demo", "5678") - entity_registry.async_update_entity( + bedroom_light = entity_registry.async_update_entity( bedroom_light.entity_id, area_id=area_bedroom.id ) hass.states.async_set( @@ -206,14 +206,14 @@ async def test_unexposed_entities_skipped( # Both lights are in the kitchen exposed_light = entity_registry.async_get_or_create("light", "demo", "1234") - entity_registry.async_update_entity( + exposed_light = entity_registry.async_update_entity( exposed_light.entity_id, area_id=area_kitchen.id, ) hass.states.async_set(exposed_light.entity_id, "off") unexposed_light = entity_registry.async_get_or_create("light", "demo", "5678") - entity_registry.async_update_entity( + unexposed_light = entity_registry.async_update_entity( unexposed_light.entity_id, area_id=area_kitchen.id, ) @@ -336,7 +336,9 @@ async def test_device_area_context( light_entity = entity_registry.async_get_or_create( "light", "demo", f"{area.name}-light-{i}" ) - entity_registry.async_update_entity(light_entity.entity_id, area_id=area.id) + light_entity = entity_registry.async_update_entity( + light_entity.entity_id, area_id=area.id + ) hass.states.async_set( light_entity.entity_id, "off", @@ -692,7 +694,7 @@ async def test_empty_aliases( names = slot_lists["name"] assert len(names.values) == 1 - assert names.values[0].value_out == kitchen_light.entity_id + assert names.values[0].value_out == kitchen_light.name assert names.values[0].text_in.text == kitchen_light.name @@ -713,3 +715,82 @@ async def test_all_domains_loaded(hass: HomeAssistant, init_components) -> None: result.response.speech["plain"]["speech"] == "Sorry, I am not aware of any device called test light" ) + + +async def test_same_named_entities_in_different_areas( + hass: HomeAssistant, + init_components, + area_registry: ar.AreaRegistry, + entity_registry: er.EntityRegistry, +) -> None: + """Test that entities with the same name in different areas can be targeted.""" + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") + + area_bedroom = area_registry.async_get_or_create("bedroom_id") + area_bedroom = area_registry.async_update(area_bedroom.id, name="bedroom") + + # Both lights have the same name, but are in different areas + kitchen_light = entity_registry.async_get_or_create("light", "demo", "1234") + kitchen_light = entity_registry.async_update_entity( + kitchen_light.entity_id, + area_id=area_kitchen.id, + name="overhead light", + ) + hass.states.async_set( + kitchen_light.entity_id, + "off", + attributes={ATTR_FRIENDLY_NAME: kitchen_light.name}, + ) + + bedroom_light = entity_registry.async_get_or_create("light", "demo", "5678") + bedroom_light = entity_registry.async_update_entity( + bedroom_light.entity_id, + area_id=area_bedroom.id, + name="overhead light", + ) + hass.states.async_set( + bedroom_light.entity_id, + "off", + attributes={ATTR_FRIENDLY_NAME: bedroom_light.name}, + ) + + # Target kitchen light + calls = async_mock_service(hass, "light", "turn_on") + result = await conversation.async_converse( + hass, "turn on overhead light in the kitchen", None, Context(), None + ) + await hass.async_block_till_done() + + assert len(calls) == 1 + assert result.response.response_type == intent.IntentResponseType.ACTION_DONE + assert result.response.intent is not None + assert ( + result.response.intent.slots.get("name", {}).get("value") == kitchen_light.name + ) + assert ( + result.response.intent.slots.get("name", {}).get("text") == kitchen_light.name + ) + assert len(result.response.matched_states) == 1 + assert result.response.matched_states[0].entity_id == kitchen_light.entity_id + assert calls[0].data.get("entity_id") == [kitchen_light.entity_id] + + # Target bedroom light + calls.clear() + result = await conversation.async_converse( + hass, "turn on overhead light in the bedroom", None, Context(), None + ) + await hass.async_block_till_done() + + assert len(calls) == 1 + assert result.response.response_type == intent.IntentResponseType.ACTION_DONE + assert result.response.intent is not None + assert ( + result.response.intent.slots.get("name", {}).get("value") == bedroom_light.name + ) + assert ( + result.response.intent.slots.get("name", {}).get("text") == bedroom_light.name + ) + assert len(result.response.matched_states) == 1 + assert result.response.matched_states[0].entity_id == bedroom_light.entity_id + assert calls[0].data.get("entity_id") == [bedroom_light.entity_id] diff --git a/tests/components/conversation/test_trigger.py b/tests/components/conversation/test_trigger.py index 74df1b7f8a6..78c30c05e7b 100644 --- a/tests/components/conversation/test_trigger.py +++ b/tests/components/conversation/test_trigger.py @@ -1,4 +1,7 @@ """Test conversation triggers.""" + +import logging + import pytest import voluptuous as vol @@ -70,7 +73,7 @@ async def test_if_fires_on_event(hass: HomeAssistant, calls, setup_comp) -> None async def test_response(hass: HomeAssistant, setup_comp) -> None: - """Test the firing of events.""" + """Test the conversation response action.""" response = "I'm sorry, Dave. I'm afraid I can't do that" assert await async_setup_component( hass, @@ -100,6 +103,116 @@ async def test_response(hass: HomeAssistant, setup_comp) -> None: assert service_response["response"]["speech"]["plain"]["speech"] == response +async def test_response_same_sentence(hass: HomeAssistant, calls, setup_comp) -> None: + """Test the conversation response action with multiple triggers using the same sentence.""" + assert await async_setup_component( + hass, + "automation", + { + "automation": [ + { + "trigger": { + "id": "trigger1", + "platform": "conversation", + "command": ["test sentence"], + }, + "action": [ + # Add delay so this response will not be the first + {"delay": "0:0:0.100"}, + { + "service": "test.automation", + "data_template": {"data": "{{ trigger }}"}, + }, + {"set_conversation_response": "response 2"}, + ], + }, + { + "trigger": { + "id": "trigger2", + "platform": "conversation", + "command": ["test sentence"], + }, + "action": {"set_conversation_response": "response 1"}, + }, + ] + }, + ) + + service_response = await hass.services.async_call( + "conversation", + "process", + {"text": "test sentence"}, + blocking=True, + return_response=True, + ) + await hass.async_block_till_done() + + # Should only get first response + assert service_response["response"]["speech"]["plain"]["speech"] == "response 1" + + # Service should still have been called + assert len(calls) == 1 + assert calls[0].data["data"] == { + "alias": None, + "id": "trigger1", + "idx": "0", + "platform": "conversation", + "sentence": "test sentence", + "slots": {}, + "details": {}, + } + + +async def test_response_same_sentence_with_error( + hass: HomeAssistant, calls, setup_comp, caplog: pytest.LogCaptureFixture +) -> None: + """Test the conversation response action with multiple triggers using the same sentence and an error.""" + caplog.set_level(logging.ERROR) + assert await async_setup_component( + hass, + "automation", + { + "automation": [ + { + "trigger": { + "id": "trigger1", + "platform": "conversation", + "command": ["test sentence"], + }, + "action": [ + # Add delay so this will not finish first + {"delay": "0:0:0.100"}, + {"service": "fake_domain.fake_service"}, + ], + }, + { + "trigger": { + "id": "trigger2", + "platform": "conversation", + "command": ["test sentence"], + }, + "action": {"set_conversation_response": "response 1"}, + }, + ] + }, + ) + + service_response = await hass.services.async_call( + "conversation", + "process", + {"text": "test sentence"}, + blocking=True, + return_response=True, + ) + await hass.async_block_till_done() + + # Should still get first response + assert service_response["response"]["speech"]["plain"]["speech"] == "response 1" + + # Error should have been logged + assert "Error executing script" in caplog.text + + async def test_subscribe_trigger_does_not_interfere_with_responses( hass: HomeAssistant, setup_comp, hass_ws_client: WebSocketGenerator ) -> None: diff --git a/tests/components/intent/test_init.py b/tests/components/intent/test_init.py index d80add2a441..4c327a237c7 100644 --- a/tests/components/intent/test_init.py +++ b/tests/components/intent/test_init.py @@ -1,4 +1,5 @@ """Tests for Intent component.""" + import pytest from homeassistant.components.cover import SERVICE_OPEN_COVER @@ -225,6 +226,30 @@ async def test_turn_on_multiple_intent(hass: HomeAssistant) -> None: assert call.data == {"entity_id": ["light.test_lights_2"]} +async def test_turn_on_all(hass: HomeAssistant) -> None: + """Test HassTurnOn intent with "all" name.""" + result = await async_setup_component(hass, "homeassistant", {}) + result = await async_setup_component(hass, "intent", {}) + assert result + + hass.states.async_set("light.test_light", "off") + hass.states.async_set("light.test_light_2", "off") + calls = async_mock_service(hass, "light", SERVICE_TURN_ON) + + await intent.async_handle(hass, "test", "HassTurnOn", {"name": {"value": "all"}}) + await hass.async_block_till_done() + + # All lights should be on now + assert len(calls) == 2 + entity_ids = set() + for call in calls: + assert call.domain == "light" + assert call.service == "turn_on" + entity_ids.update(call.data.get("entity_id", [])) + + assert entity_ids == {"light.test_light", "light.test_light_2"} + + async def test_get_state_intent( hass: HomeAssistant, area_registry: ar.AreaRegistry,