From 78a336bf670c98fde18153b307ab133ca7641763 Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Thu, 8 Feb 2024 14:15:22 -0600 Subject: [PATCH 1/3] Matching duplicate named entities is now an error --- .../components/conversation/default_agent.py | 30 ++- homeassistant/components/intent/__init__.py | 17 +- homeassistant/helpers/intent.py | 20 +- .../conversation/test_default_agent.py | 218 ++++++++++++++++++ 4 files changed, 279 insertions(+), 6 deletions(-) diff --git a/homeassistant/components/conversation/default_agent.py b/homeassistant/components/conversation/default_agent.py index 52925fbc24198e..cd371ff0630146 100644 --- a/homeassistant/components/conversation/default_agent.py +++ b/homeassistant/components/conversation/default_agent.py @@ -316,6 +316,20 @@ async def async_process(self, user_input: ConversationInput) -> ConversationResu ), conversation_id, ) + except intent.DuplicateNamesMatchedError as duplicate_names_error: + # Intent was valid, but two or more entities with the same name matched. + ( + error_response_type, + error_response_args, + ) = _get_duplicate_names_matched_response(duplicate_names_error) + return _make_error_result( + language, + intent.IntentResponseErrorCode.NO_VALID_TARGETS, + self._get_error_text( + error_response_type, lang_intents, **error_response_args + ), + conversation_id, + ) except intent.IntentHandleError: # Intent was valid and entities matched constraints, but an error # occurred during handling. @@ -753,7 +767,7 @@ def _make_slot_lists(self) -> dict[str, SlotList]: if not alias.strip(): continue - entity_names.append((alias, state.name, context)) + entity_names.append((alias, alias, context)) # Default name entity_names.append((state.name, state.name, context)) @@ -992,6 +1006,20 @@ def _get_no_states_matched_response( return ErrorKey.NO_INTENT, {} +def _get_duplicate_names_matched_response( + duplicate_names_error: intent.DuplicateNamesMatchedError, +) -> tuple[ErrorKey, dict[str, Any]]: + """Return key and template arguments for error when intent returns duplicate matches.""" + + if duplicate_names_error.area: + return ErrorKey.DUPLICATE_ENTITIES_IN_AREA, { + "entity": duplicate_names_error.name, + "area": duplicate_names_error.area, + } + + return ErrorKey.DUPLICATE_ENTITIES, {"entity": duplicate_names_error.name} + + def _collect_list_references(expression: Expression, list_names: set[str]) -> None: """Collect list reference names recursively.""" if isinstance(expression, Sequence): diff --git a/homeassistant/components/intent/__init__.py b/homeassistant/components/intent/__init__.py index d032f535b06e2d..43f29b4fcad9f9 100644 --- a/homeassistant/components/intent/__init__.py +++ b/homeassistant/components/intent/__init__.py @@ -156,14 +156,18 @@ async def async_handle(self, intent_obj: intent.Intent) -> intent.IntentResponse slots = self.async_validate_slots(intent_obj.slots) # Entity name to match - entity_name: str | None = slots.get("name", {}).get("value") + name_slot = slots.get("name", {}) + entity_name: str | None = name_slot.get("value") + entity_text: str | None = name_slot.get("text") # Look up area first to fail early - area_name = slots.get("area", {}).get("value") + area_slot = slots.get("area", {}) + area_id = area_slot.get("value") + area_name = area_slot.get("text") area: ar.AreaEntry | None = None if area_name is not None: areas = ar.async_get(hass) - area = areas.async_get_area(area_name) or areas.async_get_area_by_name( + area = areas.async_get_area(area_id) or areas.async_get_area_by_name( area_name ) if area is None: @@ -205,6 +209,13 @@ async def async_handle(self, intent_obj: intent.Intent) -> intent.IntentResponse intent_obj.assistant, ) + if entity_name and (len(states) > 1): + # Multiple entities matched for the same name + raise intent.DuplicateNamesMatchedError( + name=entity_text or entity_name, + area=area_name or area_id, + ) + # Create response response = intent_obj.create_response() response.response_type = intent.IntentResponseType.QUERY_ANSWER diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index c828b86264c22e..1b44354a699a97 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -155,6 +155,17 @@ def __init__( self.device_classes = device_classes +class DuplicateNamesMatchedError(IntentError): + """Error when two or more entities with the same name matched.""" + + def __init__(self, name: str, area: str | None) -> None: + """Initialize error.""" + super().__init__() + + self.name = name + self.area = area + + def _is_device_class( state: State, entity: entity_registry.RegistryEntry | None, @@ -318,8 +329,6 @@ def async_match_states( for state, entity in states_and_entities: if _has_name(state, entity, name): yield state - break - else: # Not filtered by name for state, _entity in states_and_entities: @@ -453,6 +462,13 @@ async def async_handle(self, intent_obj: Intent) -> IntentResponse: device_classes=device_classes, ) + if entity_name and (len(states) > 1): + # Multiple entities matched for the same name + raise DuplicateNamesMatchedError( + name=entity_text or entity_name, + area=area_name or area_id, + ) + response = await self.async_handle_states(intent_obj, states, area) # Make the matched states available in the response diff --git a/tests/components/conversation/test_default_agent.py b/tests/components/conversation/test_default_agent.py index d8a256608c8d25..4b4f9ade3eb464 100644 --- a/tests/components/conversation/test_default_agent.py +++ b/tests/components/conversation/test_default_agent.py @@ -614,6 +614,115 @@ async def test_error_no_intent(hass: HomeAssistant, init_components) -> None: ) +async def test_error_duplicate_names( + hass: HomeAssistant, init_components, entity_registry: er.EntityRegistry +) -> None: + """Test error message when multiple devices have the same name (or alias).""" + kitchen_light_1 = entity_registry.async_get_or_create("light", "demo", "1234") + kitchen_light_2 = entity_registry.async_get_or_create("light", "demo", "5678") + + # Same name and alias + for light in (kitchen_light_1, kitchen_light_2): + light = entity_registry.async_update_entity( + light.entity_id, + name="kitchen light", + aliases={"overhead light"}, + ) + hass.states.async_set( + light.entity_id, + "off", + attributes={ATTR_FRIENDLY_NAME: light.name}, + ) + + # Check name and alias + for name in ("kitchen light", "overhead light"): + # command + result = await conversation.async_converse( + hass, f"turn on {name}", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + assert ( + result.response.error_code + == intent.IntentResponseErrorCode.NO_VALID_TARGETS + ) + assert ( + result.response.speech["plain"]["speech"] + == f"Sorry, there are multiple devices called {name}" + ) + + # question + result = await conversation.async_converse( + hass, f"is {name} on?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + assert ( + result.response.error_code + == intent.IntentResponseErrorCode.NO_VALID_TARGETS + ) + assert ( + result.response.speech["plain"]["speech"] + == f"Sorry, there are multiple devices called {name}" + ) + + +async def test_error_duplicate_names_in_area( + hass: HomeAssistant, + init_components, + area_registry: ar.AreaRegistry, + entity_registry: er.EntityRegistry, +) -> None: + """Test error message when multiple devices have the same name (or alias).""" + area_kitchen = area_registry.async_get_or_create("kitchen_id") + area_kitchen = area_registry.async_update(area_kitchen.id, name="kitchen") + + kitchen_light_1 = entity_registry.async_get_or_create("light", "demo", "1234") + kitchen_light_2 = entity_registry.async_get_or_create("light", "demo", "5678") + + # Same name and alias + for light in (kitchen_light_1, kitchen_light_2): + light = entity_registry.async_update_entity( + light.entity_id, + name="kitchen light", + area_id=area_kitchen.id, + aliases={"overhead light"}, + ) + hass.states.async_set( + light.entity_id, + "off", + attributes={ATTR_FRIENDLY_NAME: light.name}, + ) + + # Check name and alias + for name in ("kitchen light", "overhead light"): + # command + result = await conversation.async_converse( + hass, f"turn on {name} in {area_kitchen.name}", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + assert ( + result.response.error_code + == intent.IntentResponseErrorCode.NO_VALID_TARGETS + ) + assert ( + result.response.speech["plain"]["speech"] + == f"Sorry, there are multiple devices called {name} in the {area_kitchen.name} area" + ) + + # question + result = await conversation.async_converse( + hass, f"is {name} on in the {area_kitchen.name}?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + assert ( + result.response.error_code + == intent.IntentResponseErrorCode.NO_VALID_TARGETS + ) + assert ( + result.response.speech["plain"]["speech"] + == f"Sorry, there are multiple devices called {name} in the {area_kitchen.name} area" + ) + + async def test_no_states_matched_default_error( hass: HomeAssistant, init_components, area_registry: ar.AreaRegistry ) -> None: @@ -794,3 +903,112 @@ async def test_same_named_entities_in_different_areas( 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] + + # Targeting a duplicate name should fail + result = await conversation.async_converse( + hass, "turn on overhead light", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + + # Querying a duplicate name should also fail + result = await conversation.async_converse( + hass, "is the overhead light on?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + + # But we can still ask questions that don't rely on the name + result = await conversation.async_converse( + hass, "how many lights are on?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.QUERY_ANSWER + + +async def test_same_aliased_entities_in_different_areas( + hass: HomeAssistant, + init_components, + area_registry: ar.AreaRegistry, + entity_registry: er.EntityRegistry, +) -> None: + """Test that entities with the same alias (but different names) 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 alias, 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="kitchen overhead light", + aliases={"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="bedroom overhead light", + aliases={"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") == "overhead light" + assert result.response.intent.slots.get("name", {}).get("text") == "overhead light" + 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") == "overhead light" + assert result.response.intent.slots.get("name", {}).get("text") == "overhead light" + 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] + + # Targeting a duplicate alias should fail + result = await conversation.async_converse( + hass, "turn on overhead light", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + + # Querying a duplicate alias should also fail + result = await conversation.async_converse( + hass, "is the overhead light on?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.ERROR + + # But we can still ask questions that don't rely on the alias + result = await conversation.async_converse( + hass, "how many lights are on?", None, Context(), None + ) + assert result.response.response_type == intent.IntentResponseType.QUERY_ANSWER From fd29c051a77bf1b67dee0e58e1ceadb4b73cc0fc Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Thu, 8 Feb 2024 15:19:41 -0600 Subject: [PATCH 2/3] Update snapshot --- tests/components/conversation/snapshots/test_init.ambr | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/components/conversation/snapshots/test_init.ambr b/tests/components/conversation/snapshots/test_init.ambr index f44789414738ea..6af9d197e019ed 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': 'kitchen', + 'value': 'my cool light', }), }), 'intent': dict({ @@ -1422,7 +1422,7 @@ 'name': dict({ 'name': 'name', 'text': 'my cool light', - 'value': 'kitchen', + 'value': 'my cool light', }), }), 'intent': dict({ From f2985498e444e414335f0477aaaf9d7a455c9bdd Mon Sep 17 00:00:00 2001 From: Michael Hansen Date: Thu, 8 Feb 2024 17:24:30 -0600 Subject: [PATCH 3/3] Only use area id --- homeassistant/components/intent/__init__.py | 6 ++---- homeassistant/helpers/intent.py | 4 +--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/homeassistant/components/intent/__init__.py b/homeassistant/components/intent/__init__.py index 43f29b4fcad9f9..e960b5616cba2c 100644 --- a/homeassistant/components/intent/__init__.py +++ b/homeassistant/components/intent/__init__.py @@ -165,11 +165,9 @@ async def async_handle(self, intent_obj: intent.Intent) -> intent.IntentResponse area_id = area_slot.get("value") area_name = area_slot.get("text") area: ar.AreaEntry | None = None - if area_name is not None: + if area_id is not None: areas = ar.async_get(hass) - area = areas.async_get_area(area_id) or areas.async_get_area_by_name( - area_name - ) + area = areas.async_get_area(area_id) if area is None: raise intent.IntentHandleError(f"No area named {area_name}") diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index 1b44354a699a97..5217a55bec594c 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -425,9 +425,7 @@ async def async_handle(self, intent_obj: Intent) -> IntentResponse: area: area_registry.AreaEntry | None = None if area_id is not None: areas = area_registry.async_get(hass) - area = areas.async_get_area(area_id) or areas.async_get_area_by_name( - area_name - ) + area = areas.async_get_area(area_id) if area is None: raise IntentHandleError(f"No area named {area_name}")