Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Matching duplicate named entities is now an error in Assist #110050

Merged
merged 3 commits into from
Feb 9, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion homeassistant/components/conversation/default_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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))
Expand Down Expand Up @@ -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):
Expand Down
17 changes: 14 additions & 3 deletions homeassistant/components/intent/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
20 changes: 18 additions & 2 deletions homeassistant/helpers/intent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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
Expand Down
218 changes: 218 additions & 0 deletions tests/components/conversation/test_default_agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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
Loading