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.

Sign up for GitHub

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

Differentiate between device info types #95641

Merged
merged 16 commits into from
Jul 10, 2023
Merged

Differentiate between device info types #95641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
c1b57f2
Differentiate between device info types
balloob Jun 30, 2023
fd33608
Update allowed fields
balloob Jun 30, 2023
ab83a56
Update homeassistant/helpers/entity_platform.py
balloob Jul 1, 2023
61ab966
Split up message in 2 lines
balloob Jul 1, 2023
592de14
Use dict for device info types
balloob Jul 1, 2023
409fbe7
Extract device info function and test error checking
balloob Jul 6, 2023
4ed6c4c
Simplify parsing device info
balloob Jul 6, 2023
2cfd1f1
move checks around
balloob Jul 6, 2023
5f33f97
Simplify more
balloob Jul 6, 2023
1919d34
Move error checking around
balloob Jul 6, 2023
6708b35
Fix order
balloob Jul 6, 2023
9c3bb44
fallback config entry title to domain
balloob Jul 6, 2023
c56794c
Remove fallback for name to config entry domain
balloob Jul 6, 2023
646e221
Ensure mocked configuration URLs are strings
balloob Jul 6, 2023
35ab721
one more test case
balloob Jul 7, 2023
206dcee
Apply suggestions from code review
balloob Jul 7, 2023
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
165 changes: 94 additions & 71 deletions homeassistant/helpers/entity_platform.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
from homeassistant.exceptions import (
HomeAssistantError,
PlatformNotReady,
RequiredParameterMissing,
)
from homeassistant.generated import languages
from homeassistant.setup import async_start_setup
Expand All @@ -42,14 +41,13 @@
service,
translation,
)
from .device_registry import DeviceRegistry
from .entity_registry import EntityRegistry, RegistryEntryDisabler, RegistryEntryHider
from .event import async_call_later, async_track_time_interval
from .issue_registry import IssueSeverity, async_create_issue
from .typing import UNDEFINED, ConfigType, DiscoveryInfoType

if TYPE_CHECKING:
from .entity import Entity
from .entity import DeviceInfo, Entity


SLOW_SETUP_WARNING = 10
Expand All @@ -61,6 +59,37 @@
DATA_ENTITY_PLATFORM = "entity_platform"
PLATFORM_NOT_READY_BASE_WAIT_TIME = 30 # seconds

DEVICE_INFO_TYPES = {
# Device info is categorized by finding the first device info type which has all
# the keys of the device info. The link device info type must be kept first
# to make it preferred over primary.
"link": {
"connections",
"identifiers",
},
"primary": {
"configuration_url",
"connections",
"entry_type",
"hw_version",
"identifiers",
"manufacturer",
"model",
"name",
"suggested_area",
"sw_version",
"via_device",
},
"secondary": {
"connections",
"default_manufacturer",
"default_model",
"default_name",
# Used by Fritz
"via_device",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't expect this to be set to indicate which network device is used to connect a device to the network. I guess it fits, but other integrations don't do it.

},
}

_LOGGER = getLogger(__name__)


Expand Down Expand Up @@ -485,12 +514,9 @@ async def async_add_entities(

hass = self.hass

device_registry = dev_reg.async_get(hass)
entity_registry = ent_reg.async_get(hass)
tasks = [
self._async_add_entity(
entity, update_before_add, entity_registry, device_registry
)
self._async_add_entity(entity, update_before_add, entity_registry)
for entity in new_entities
]

Expand Down Expand Up @@ -552,7 +578,6 @@ async def _async_add_entity( # noqa: C901
entity: Entity,
update_before_add: bool,
entity_registry: EntityRegistry,
device_registry: DeviceRegistry,
) -> None:
"""Add an entity to the platform."""
if entity is None:
Expand Down Expand Up @@ -608,68 +633,10 @@ async def _async_add_entity( # noqa: C901
entity.add_to_platform_abort()
return

device_info = entity.device_info
device_id = None
device = None

if self.config_entry and device_info is not None:
processed_dev_info: dict[str, str | None] = {}
for key in (
"connections",
"default_manufacturer",
"default_model",
"default_name",
"entry_type",
"identifiers",
"manufacturer",
"model",
"name",
"suggested_area",
"sw_version",
"hw_version",
"via_device",
):
if key in device_info:
processed_dev_info[key] = device_info[
key # type: ignore[literal-required]
]

if (
# device info that is purely meant for linking doesn't need default name
any(
key not in {"identifiers", "connections"}
for key in (processed_dev_info)
)
and "default_name" not in processed_dev_info
and not processed_dev_info.get("name")
):
processed_dev_info["name"] = self.config_entry.title

if "configuration_url" in device_info:
if device_info["configuration_url"] is None:
processed_dev_info["configuration_url"] = None
else:
configuration_url = str(device_info["configuration_url"])
if urlparse(configuration_url).scheme in [
"http",
"https",
"homeassistant",
]:
processed_dev_info["configuration_url"] = configuration_url
else:
_LOGGER.warning(
"Ignoring invalid device configuration_url '%s'",
configuration_url,
)

try:
device = device_registry.async_get_or_create(
config_entry_id=self.config_entry.entry_id,
**processed_dev_info, # type: ignore[arg-type]
)
device_id = device.id
except RequiredParameterMissing:
pass
if self.config_entry and (device_info := entity.device_info):
device = self._async_process_device_info(device_info)
else:
device = None

# An entity may suggest the entity_id by setting entity_id itself
suggested_entity_id: str | None = entity.entity_id
Expand Down Expand Up @@ -704,7 +671,7 @@ async def _async_add_entity( # noqa: C901
entity.unique_id,
capabilities=entity.capability_attributes,
config_entry=self.config_entry,
device_id=device_id,
device_id=device.id if device else None,
disabled_by=disabled_by,
entity_category=entity.entity_category,
get_initial_options=entity.get_initial_entity_options,
Expand Down Expand Up @@ -792,6 +759,62 @@ def remove_entity_cb() -> None:

await entity.add_to_platform_finish()

@callback
def _async_process_device_info(
self, device_info: DeviceInfo
) -> dev_reg.DeviceEntry | None:
"""Process a device info."""
keys = set(device_info)

# If no keys or not enough info to match up, abort
if len(keys & {"connections", "identifiers"}) == 0:
self.logger.error(
"Ignoring device info without identifiers or connections: %s",
device_info,
)
return None
Comment on lines +771 to +775
Copy link
Contributor

@emontnemery emontnemery Jul 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It didn't raise in the old code either, but is there a good reason to not raise here? Why do we go ahead and add an entity which has invalid device info?

I'm not suggesting we change the behavior in this PR, but it's something to consider in a follow-up I think.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I chose not to change that behavior as it would be breaking. It would probably be a small breaking change as I was pleasantly surprised the low amount of bad device info this PR found. If we decide to do this, we should do it in a follow-up PR.


device_info_type: str | None = None

# Find the first device info type which has all keys in the device info
for possible_type, allowed_keys in DEVICE_INFO_TYPES.items():
if keys <= allowed_keys:
device_info_type = possible_type
break

if device_info_type is None:
self.logger.error(
"Device info for %s needs to either describe a device, "
"link to existing device or provide extra information.",
device_info,
)
return None
Comment on lines +786 to +791
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment here, should this raise instead?


if (config_url := device_info.get("configuration_url")) is not None:
if type(config_url) is not str or urlparse(config_url).scheme not in [
"http",
"https",
"homeassistant",
]:
self.logger.error(
"Ignoring device info with invalid configuration_url '%s'",
config_url,
)
return None
Comment on lines +799 to +803
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same


assert self.config_entry is not None

if device_info_type == "primary" and not device_info.get("name"):
device_info = {
**device_info, # type: ignore[misc]
"name": self.config_entry.title,
}

return dev_reg.async_get(self.hass).async_get_or_create(
config_entry_id=self.config_entry.entry_id,
**device_info,
)

async def async_reset(self) -> None:
"""Remove all entities and reset data.

Expand Down
1 change: 1 addition & 0 deletions tests/components/fritzbox/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,4 +10,5 @@ def fritz_fixture() -> Mock:
with patch("homeassistant.components.fritzbox.Fritzhome") as fritz, patch(
"homeassistant.components.fritzbox.config_flow.Fritzhome"
):
fritz.return_value.get_prefixed_host.return_value = "http://1.2.3.4"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test used to return a Mock for this function, which resulted in passing Mock to configuration_url which triggered invalid device info, and device not to be created.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have slightly changed the behavior here. In the past, an invalid configuration URL would merely result in removal of the invalid configuration URL. With the updated logic, we will actually mark the whole device info as invalid and not create a device.

yield fritz
1 change: 1 addition & 0 deletions tests/components/hyperion/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,7 @@ def create_mock_client() -> Mock:
mock_client.instances = [
{"friendly_name": "Test instance 1", "instance": 0, "running": True}
]
mock_client.remote_url = f"http://{TEST_HOST}:{TEST_PORT_UI}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test changed?


return mock_client

Expand Down
1 change: 1 addition & 0 deletions tests/components/purpleair/conftest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def api_fixture(get_sensors_response):
"""Define a fixture to return a mocked aiopurple API object."""
return Mock(
async_check_api_key=AsyncMock(),
get_map_url=Mock(return_value="http://example.com"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this test changed?

sensors=Mock(
async_get_nearby_sensors=AsyncMock(
return_value=[
Expand Down
Loading