From c1b57f2e291eadd0aac2172edfd3f860dbaa67c9 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Fri, 30 Jun 2023 14:42:47 -0400 Subject: [PATCH 01/16] Differentiate between device info types --- homeassistant/helpers/entity_platform.py | 34 ++++++++++++++++++++++++ tests/helpers/test_entity_platform.py | 7 ++--- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 66a74edf8f91a1..6d3190d7929798 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -61,6 +61,34 @@ DATA_ENTITY_PLATFORM = "entity_platform" PLATFORM_NOT_READY_BASE_WAIT_TIME = 30 # seconds +DEVICE_INFO_TYPES = ( + # link info + { + "connections", + "identifiers", + }, + # primary info + { + "connections", + "identifiers", + "entry_type", + "manufacturer", + "model", + "name", + "suggested_area", + "sw_version", + "hw_version", + "via_device", + }, + # secondary info + { + "connections", + "default_manufacturer", + "default_model", + "default_name", + }, +) + _LOGGER = getLogger(__name__) @@ -634,6 +662,12 @@ async def _async_add_entity( # noqa: C901 key # type: ignore[literal-required] ] + keys = set(processed_dev_info) + if not any(keys <= allowed for allowed in DEVICE_INFO_TYPES): + raise HomeAssistantError( + "Device info needs to be describe a device, link to existing device or provide extra information." + ) + if ( # device info that is purely meant for linking doesn't need default name any( diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index df4f4d1c6439a9..363a2c5248da09 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1846,12 +1846,13 @@ async def test_device_name_defaulting_config_entry( ) -> None: """Test setting the device name based on input info.""" device_info = { - "identifiers": {("hue", "1234")}, - "name": entity_device_name, + "connections": {(dr.CONNECTION_NETWORK_MAC, "1234")}, } if entity_device_default_name: device_info["default_name"] = entity_device_default_name + else: + device_info["name"] = entity_device_name class DeviceNameEntity(Entity): _attr_unique_id = "qwer" @@ -1874,6 +1875,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): await hass.async_block_till_done() dev_reg = dr.async_get(hass) - device = dev_reg.async_get_device({("hue", "1234")}) + device = dev_reg.async_get_device(set(), {(dr.CONNECTION_NETWORK_MAC, "1234")}) assert device is not None assert device.name == expected_device_name From fd336088b6652706f5a5aa2b0f1a246d88235ab7 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Fri, 30 Jun 2023 15:48:29 -0400 Subject: [PATCH 02/16] Update allowed fields --- homeassistant/helpers/entity_platform.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 6d3190d7929798..4a23faefcdf110 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -69,15 +69,16 @@ }, # primary info { + "configuration_url", "connections", - "identifiers", "entry_type", + "hw_version", + "identifiers", "manufacturer", "model", "name", "suggested_area", "sw_version", - "hw_version", "via_device", }, # secondary info @@ -86,6 +87,8 @@ "default_manufacturer", "default_model", "default_name", + # Used by Fritz + "via_device", }, ) From ab83a56602235a3ab0aa602de92400e3af7133c7 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 1 Jul 2023 06:07:57 -0400 Subject: [PATCH 03/16] Update homeassistant/helpers/entity_platform.py Co-authored-by: Martin Hjelmare --- homeassistant/helpers/entity_platform.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 4a23faefcdf110..dd56cfb857db6c 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -668,7 +668,7 @@ async def _async_add_entity( # noqa: C901 keys = set(processed_dev_info) if not any(keys <= allowed for allowed in DEVICE_INFO_TYPES): raise HomeAssistantError( - "Device info needs to be describe a device, link to existing device or provide extra information." + "Device info needs to either describe a device, link to existing device or provide extra information." ) if ( From 61ab9666b61fa7b66c65b08635e653bf337c735c Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 1 Jul 2023 13:23:55 -0400 Subject: [PATCH 04/16] Split up message in 2 lines --- homeassistant/helpers/entity_platform.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index dd56cfb857db6c..42aed1bc11eee9 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -668,7 +668,8 @@ async def _async_add_entity( # noqa: C901 keys = set(processed_dev_info) if not any(keys <= allowed for allowed in DEVICE_INFO_TYPES): raise HomeAssistantError( - "Device info needs to either describe a device, link to existing device or provide extra information." + "Device info needs to either describe a device, " + "link to existing device or provide extra information." ) if ( From 592de1485dc759342c1d1ea5679134a5c1aeffdd Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 1 Jul 2023 13:26:24 -0400 Subject: [PATCH 05/16] Use dict for device info types --- homeassistant/helpers/entity_platform.py | 28 ++++++++++++++---------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 42aed1bc11eee9..3634ee5594fc30 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -61,14 +61,8 @@ DATA_ENTITY_PLATFORM = "entity_platform" PLATFORM_NOT_READY_BASE_WAIT_TIME = 30 # seconds -DEVICE_INFO_TYPES = ( - # link info - { - "connections", - "identifiers", - }, - # primary info - { +DEVICE_INFO_TYPES = { + "primary": { "configuration_url", "connections", "entry_type", @@ -81,8 +75,7 @@ "sw_version", "via_device", }, - # secondary info - { + "secondary": { "connections", "default_manufacturer", "default_model", @@ -90,7 +83,11 @@ # Used by Fritz "via_device", }, -) + "link": { + "connections", + "identifiers", + }, +} _LOGGER = getLogger(__name__) @@ -666,7 +663,14 @@ async def _async_add_entity( # noqa: C901 ] keys = set(processed_dev_info) - if not any(keys <= allowed for allowed in DEVICE_INFO_TYPES): + entity_type: str | None = None + + for possible_type, allowed_keys in DEVICE_INFO_TYPES.items(): + if keys <= allowed_keys: + entity_type = possible_type + break + + if entity_type is None: raise HomeAssistantError( "Device info needs to either describe a device, " "link to existing device or provide extra information." From 409fbe783f7d633ce8a9369278a87c4eb2c3ff49 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Wed, 5 Jul 2023 21:24:39 -0400 Subject: [PATCH 06/16] Extract device info function and test error checking --- homeassistant/helpers/entity_platform.py | 164 +++++++++++------------ tests/helpers/test_entity_platform.py | 41 ++++++ 2 files changed, 123 insertions(+), 82 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 3634ee5594fc30..f92cea5061f85f 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -29,7 +29,6 @@ from homeassistant.exceptions import ( HomeAssistantError, PlatformNotReady, - RequiredParameterMissing, ) from homeassistant.generated import languages from homeassistant.setup import async_start_setup @@ -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 @@ -513,12 +511,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 ] @@ -580,7 +575,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: @@ -637,81 +631,10 @@ async def _async_add_entity( # noqa: C901 return device_info = entity.device_info - device_id = None - device = None + device: dev_reg.DeviceEntry | None = 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] - ] - - keys = set(processed_dev_info) - entity_type: str | None = None - - for possible_type, allowed_keys in DEVICE_INFO_TYPES.items(): - if keys <= allowed_keys: - entity_type = possible_type - break - - if entity_type is None: - raise HomeAssistantError( - "Device info needs to either describe a device, " - "link to existing device or provide extra information." - ) - - 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 + device = self._async_process_device_info(device_info) # An entity may suggest the entity_id by setting entity_id itself suggested_entity_id: str | None = entity.entity_id @@ -746,7 +669,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, @@ -834,6 +757,83 @@ 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.""" + processed_dev_info: DeviceInfo = {} + for key in ( + "connections", + "default_manufacturer", + "default_model", + "default_name", + "entry_type", + "hw_version", + "identifiers", + "manufacturer", + "model", + "name", + "suggested_area", + "sw_version", + "via_device", + ): + if key in device_info: + processed_dev_info[key] = device_info[key] # type: ignore [literal-required] + + 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, + ) + + keys = set(processed_dev_info) + + # If no keys or not enough info to match up, abort + if not keys or len(keys & {"connections", "identifiers"}) == 0: + return None + + device_info_type: str | None = None + + 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 + + assert self.config_entry is not None + + if ( + # device info that is purely meant for linking doesn't need default name + device_info_type != "link" + and "default_name" not in processed_dev_info + and not processed_dev_info.get("name") + ): + processed_dev_info["name"] = self.config_entry.title + + return dev_reg.async_get(self.hass).async_get_or_create( + config_entry_id=self.config_entry.entry_id, + **processed_dev_info, + ) + async def async_reset(self) -> None: """Remove all entities and reset data. diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index 363a2c5248da09..59459ff41f5deb 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1878,3 +1878,44 @@ async def async_setup_entry(hass, config_entry, async_add_entities): device = dev_reg.async_get_device(set(), {(dr.CONNECTION_NETWORK_MAC, "1234")}) assert device is not None assert device.name == expected_device_name + + +@pytest.mark.parametrize( + ("device_info"), + [ + {}, + {"name": "bla"}, + {"default_name": "bla"}, + { + "name": "bla", + "default_name": "yo", + }, + ], +) +async def test_device_type_checking( + hass: HomeAssistant, + device_info: dict, +) -> None: + """Test catching invalid device info.""" + + class DeviceNameEntity(Entity): + _attr_unique_id = "qwer" + _attr_device_info = device_info + + async def async_setup_entry(hass, config_entry, async_add_entities): + """Mock setup entry method.""" + async_add_entities([DeviceNameEntity()]) + return True + + platform = MockPlatform(async_setup_entry=async_setup_entry) + config_entry = MockConfigEntry( + title="Mock Config Entry Title", entry_id="super-mock-id" + ) + entity_platform = MockEntityPlatform( + hass, platform_name=config_entry.domain, platform=platform + ) + + assert await entity_platform.async_setup_entry(config_entry) + + dev_reg = dr.async_get(hass) + assert len(dev_reg.devices) == 0 From 4ed6c4cace994390391320eedf08ce00a31bbfc8 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Wed, 5 Jul 2023 21:34:38 -0400 Subject: [PATCH 07/16] Simplify parsing device info --- homeassistant/helpers/entity_platform.py | 59 ++++++++---------------- tests/helpers/test_entity_platform.py | 59 ++++-------------------- 2 files changed, 27 insertions(+), 91 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index f92cea5061f85f..5dfcd28128b622 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -762,43 +762,19 @@ def _async_process_device_info( self, device_info: DeviceInfo ) -> dev_reg.DeviceEntry | None: """Process a device info.""" - processed_dev_info: DeviceInfo = {} - for key in ( - "connections", - "default_manufacturer", - "default_model", - "default_name", - "entry_type", - "hw_version", - "identifiers", - "manufacturer", - "model", - "name", - "suggested_area", - "sw_version", - "via_device", - ): - if key in device_info: - processed_dev_info[key] = device_info[key] # type: ignore [literal-required] - - 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, - ) + if device_info.get("configuration_url") is not None: + if urlparse(device_info["configuration_url"]).scheme not in [ + "http", + "https", + "homeassistant", + ]: + _LOGGER.error( + "Ignoring device info with invalid configuration_url '%s'", + device_info["configuration_url"], + ) + return None - keys = set(processed_dev_info) + keys = set(device_info) # If no keys or not enough info to match up, abort if not keys or len(keys & {"connections", "identifiers"}) == 0: @@ -824,14 +800,17 @@ def _async_process_device_info( if ( # device info that is purely meant for linking doesn't need default name device_info_type != "link" - and "default_name" not in processed_dev_info - and not processed_dev_info.get("name") + and "default_name" not in device_info + and not device_info.get("name") ): - processed_dev_info["name"] = self.config_entry.title + 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, - **processed_dev_info, + **device_info, ) async def async_reset(self) -> None: diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index 59459ff41f5deb..2d6c2261fbe9d8 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1169,57 +1169,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): assert device2.model == "test-model" -async def test_device_info_invalid_url( - hass: HomeAssistant, caplog: pytest.LogCaptureFixture -) -> None: - """Test device info is forwarded correctly.""" - registry = dr.async_get(hass) - registry.async_get_or_create( - config_entry_id="123", - connections=set(), - identifiers={("hue", "via-id")}, - manufacturer="manufacturer", - model="via", - ) - - async def async_setup_entry(hass, config_entry, async_add_entities): - """Mock setup entry method.""" - async_add_entities( - [ - # Valid device info, but invalid url - MockEntity( - unique_id="qwer", - device_info={ - "identifiers": {("hue", "1234")}, - "configuration_url": "foo://192.168.0.100/config", - }, - ), - ] - ) - return True - - platform = MockPlatform(async_setup_entry=async_setup_entry) - config_entry = MockConfigEntry(entry_id="super-mock-id") - entity_platform = MockEntityPlatform( - hass, platform_name=config_entry.domain, platform=platform - ) - - assert await entity_platform.async_setup_entry(config_entry) - await hass.async_block_till_done() - - assert len(hass.states.async_entity_ids()) == 1 - - device = registry.async_get_device({("hue", "1234")}) - assert device is not None - assert device.identifiers == {("hue", "1234")} - assert device.configuration_url is None - - assert ( - "Ignoring invalid device configuration_url 'foo://192.168.0.100/config'" - in caplog.text - ) - - async def test_device_info_homeassistant_url( hass: HomeAssistant, caplog: pytest.LogCaptureFixture ) -> None: @@ -1890,6 +1839,11 @@ async def async_setup_entry(hass, config_entry, async_add_entities): "name": "bla", "default_name": "yo", }, + # Invalid configuration URL + { + "identifiers": {("hue", "1234")}, + "configuration_url": "foo://192.168.0.100/config", + }, ], ) async def test_device_type_checking( @@ -1919,3 +1873,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): dev_reg = dr.async_get(hass) assert len(dev_reg.devices) == 0 + # Entity should still be registered + ent_reg = er.async_get(hass) + assert ent_reg.async_get("test_domain.test_qwer") is not None From 2cfd1f1e2fb3f96605fc97f8d79b58f02b7b6430 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Wed, 5 Jul 2023 21:39:07 -0400 Subject: [PATCH 08/16] move checks around --- homeassistant/helpers/entity_platform.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 5dfcd28128b622..96a8de53b82e91 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -762,6 +762,12 @@ 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 not keys or len(keys & {"connections", "identifiers"}) == 0: + return None + if device_info.get("configuration_url") is not None: if urlparse(device_info["configuration_url"]).scheme not in [ "http", @@ -774,12 +780,6 @@ def _async_process_device_info( ) return None - keys = set(device_info) - - # If no keys or not enough info to match up, abort - if not keys or len(keys & {"connections", "identifiers"}) == 0: - return None - device_info_type: str | None = None for possible_type, allowed_keys in DEVICE_INFO_TYPES.items(): From 5f33f97d585c144a0d7b2eced3691b9198b66ad6 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Wed, 5 Jul 2023 21:42:53 -0400 Subject: [PATCH 09/16] Simplify more --- homeassistant/helpers/entity_platform.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 96a8de53b82e91..5f80fb349dd856 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -630,11 +630,10 @@ async def _async_add_entity( # noqa: C901 entity.add_to_platform_abort() return - device_info = entity.device_info - device: dev_reg.DeviceEntry | None = None - - if self.config_entry and device_info is not None: + 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 @@ -765,7 +764,11 @@ def _async_process_device_info( keys = set(device_info) # If no keys or not enough info to match up, abort - if not keys or len(keys & {"connections", "identifiers"}) == 0: + if len(keys & {"connections", "identifiers"}) == 0: + self.logger.error( + "Ignoring device info without identifiers or connections: %s", + device_info, + ) return None if device_info.get("configuration_url") is not None: @@ -774,7 +777,7 @@ def _async_process_device_info( "https", "homeassistant", ]: - _LOGGER.error( + self.logger.error( "Ignoring device info with invalid configuration_url '%s'", device_info["configuration_url"], ) From 1919d34d3c9cacc4bc1dff1aa65658c232171880 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Wed, 5 Jul 2023 21:45:23 -0400 Subject: [PATCH 10/16] Move error checking around --- homeassistant/helpers/entity_platform.py | 31 ++++++++++-------------- 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 5f80fb349dd856..263e3c94f8d47a 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -771,18 +771,6 @@ def _async_process_device_info( ) return None - if device_info.get("configuration_url") is not None: - if urlparse(device_info["configuration_url"]).scheme not in [ - "http", - "https", - "homeassistant", - ]: - self.logger.error( - "Ignoring device info with invalid configuration_url '%s'", - device_info["configuration_url"], - ) - return None - device_info_type: str | None = None for possible_type, allowed_keys in DEVICE_INFO_TYPES.items(): @@ -798,14 +786,21 @@ def _async_process_device_info( ) return None + if device_info.get("configuration_url") is not None: + if urlparse(device_info["configuration_url"]).scheme not in [ + "http", + "https", + "homeassistant", + ]: + self.logger.error( + "Ignoring device info with invalid configuration_url '%s'", + device_info["configuration_url"], + ) + return None + assert self.config_entry is not None - if ( - # device info that is purely meant for linking doesn't need default name - device_info_type != "link" - and "default_name" not in device_info - and not device_info.get("name") - ): + if device_info_type == "primary" and not device_info.get("name"): device_info = { **device_info, # type: ignore[misc] "name": self.config_entry.title, From 6708b350bbad0112baf345e1ffc5bcfb883fb213 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 6 Jul 2023 09:28:44 -0400 Subject: [PATCH 11/16] Fix order --- homeassistant/helpers/entity_platform.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 263e3c94f8d47a..38d8755b4757e5 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -60,6 +60,11 @@ PLATFORM_NOT_READY_BASE_WAIT_TIME = 30 # seconds DEVICE_INFO_TYPES = { + # Order is important or else link types are detected as primary + "link": { + "connections", + "identifiers", + }, "primary": { "configuration_url", "connections", @@ -81,10 +86,6 @@ # Used by Fritz "via_device", }, - "link": { - "connections", - "identifiers", - }, } _LOGGER = getLogger(__name__) From 9c3bb44089d3f6b2177206891f08570b0137122f Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 6 Jul 2023 09:36:01 -0400 Subject: [PATCH 12/16] fallback config entry title to domain --- homeassistant/helpers/entity_platform.py | 2 +- tests/helpers/test_entity_platform.py | 21 +++++++++++++-------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 38d8755b4757e5..13123b7ed0ba42 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -804,7 +804,7 @@ def _async_process_device_info( if device_info_type == "primary" and not device_info.get("name"): device_info = { **device_info, # type: ignore[misc] - "name": self.config_entry.title, + "name": self.config_entry.title or self.config_entry.domain, } return dev_reg.async_get(self.hass).async_get_or_create( diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index 2d6c2261fbe9d8..38a05e4001eb62 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1779,16 +1779,23 @@ async def async_setup_entry(hass, config_entry, async_add_entities): @pytest.mark.parametrize( - ("entity_device_name", "entity_device_default_name", "expected_device_name"), + ( + "config_entry_title", + "entity_device_name", + "entity_device_default_name", + "expected_device_name", + ), [ - (None, None, "Mock Config Entry Title"), - ("", None, "Mock Config Entry Title"), - (None, "Hello", "Hello"), - ("Mock Device Name", None, "Mock Device Name"), + ("", None, None, "test"), + ("Mock Config Entry Title", None, None, "Mock Config Entry Title"), + ("Mock Config Entry Title", "", None, "Mock Config Entry Title"), + ("Mock Config Entry Title", None, "Hello", "Hello"), + ("Mock Config Entry Title", "Mock Device Name", None, "Mock Device Name"), ], ) async def test_device_name_defaulting_config_entry( hass: HomeAssistant, + config_entry_title: str, entity_device_name: str, entity_device_default_name: str, expected_device_name: str, @@ -1813,9 +1820,7 @@ async def async_setup_entry(hass, config_entry, async_add_entities): return True platform = MockPlatform(async_setup_entry=async_setup_entry) - config_entry = MockConfigEntry( - title="Mock Config Entry Title", entry_id="super-mock-id" - ) + config_entry = MockConfigEntry(title=config_entry_title, entry_id="super-mock-id") entity_platform = MockEntityPlatform( hass, platform_name=config_entry.domain, platform=platform ) From c56794c98ffa48c89a2b0d294d319e4a83c0f0a5 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 6 Jul 2023 11:46:50 -0400 Subject: [PATCH 13/16] Remove fallback for name to config entry domain --- homeassistant/helpers/entity_platform.py | 2 +- tests/helpers/test_entity_platform.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 13123b7ed0ba42..38d8755b4757e5 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -804,7 +804,7 @@ def _async_process_device_info( if device_info_type == "primary" and not device_info.get("name"): device_info = { **device_info, # type: ignore[misc] - "name": self.config_entry.title or self.config_entry.domain, + "name": self.config_entry.title, } return dev_reg.async_get(self.hass).async_get_or_create( diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index 38a05e4001eb62..b9dae9b039673f 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1786,7 +1786,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): "expected_device_name", ), [ - ("", None, None, "test"), ("Mock Config Entry Title", None, None, "Mock Config Entry Title"), ("Mock Config Entry Title", "", None, "Mock Config Entry Title"), ("Mock Config Entry Title", None, "Hello", "Hello"), From 646e2210fa1e325d483cafa9c1f7575da41e3559 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 6 Jul 2023 14:06:31 -0400 Subject: [PATCH 14/16] Ensure mocked configuration URLs are strings --- homeassistant/helpers/entity_platform.py | 6 +++--- tests/components/fritzbox/conftest.py | 1 + tests/components/hyperion/__init__.py | 1 + tests/components/purpleair/conftest.py | 1 + 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 38d8755b4757e5..249342b27b296c 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -787,15 +787,15 @@ def _async_process_device_info( ) return None - if device_info.get("configuration_url") is not None: - if urlparse(device_info["configuration_url"]).scheme not in [ + 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'", - device_info["configuration_url"], + config_url, ) return None diff --git a/tests/components/fritzbox/conftest.py b/tests/components/fritzbox/conftest.py index 50fca4581b35f7..1fbaf48de4bfc6 100644 --- a/tests/components/fritzbox/conftest.py +++ b/tests/components/fritzbox/conftest.py @@ -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" yield fritz diff --git a/tests/components/hyperion/__init__.py b/tests/components/hyperion/__init__.py index 382a168bc4434e..3714e58479b81c 100644 --- a/tests/components/hyperion/__init__.py +++ b/tests/components/hyperion/__init__.py @@ -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}" return mock_client diff --git a/tests/components/purpleair/conftest.py b/tests/components/purpleair/conftest.py index ef48a5988a37fc..4883f79b349e07 100644 --- a/tests/components/purpleair/conftest.py +++ b/tests/components/purpleair/conftest.py @@ -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"), sensors=Mock( async_get_nearby_sensors=AsyncMock( return_value=[ From 35ab72112a4e985afb1dc8a4c2180c3db3ee1704 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 6 Jul 2023 20:46:43 -0400 Subject: [PATCH 15/16] one more test case --- tests/helpers/test_entity_platform.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/helpers/test_entity_platform.py b/tests/helpers/test_entity_platform.py index b9dae9b039673f..419f7d7de26ab2 100644 --- a/tests/helpers/test_entity_platform.py +++ b/tests/helpers/test_entity_platform.py @@ -1836,10 +1836,13 @@ async def async_setup_entry(hass, config_entry, async_add_entities): @pytest.mark.parametrize( ("device_info"), [ + # No identifiers {}, {"name": "bla"}, {"default_name": "bla"}, + # Match multiple types { + "identifiers": {("hue", "1234")}, "name": "bla", "default_name": "yo", }, @@ -1850,7 +1853,7 @@ async def async_setup_entry(hass, config_entry, async_add_entities): }, ], ) -async def test_device_type_checking( +async def test_device_type_error_checking( hass: HomeAssistant, device_info: dict, ) -> None: From 206dceeb893d22e3eaf61ea0f4e88cb565e14a40 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Fri, 7 Jul 2023 08:34:28 -0400 Subject: [PATCH 16/16] Apply suggestions from code review Co-authored-by: Erik Montnemery --- homeassistant/helpers/entity_platform.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 249342b27b296c..68e9914ecc4f98 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -60,7 +60,9 @@ PLATFORM_NOT_READY_BASE_WAIT_TIME = 30 # seconds DEVICE_INFO_TYPES = { - # Order is important or else link types are detected as primary + # 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", @@ -774,6 +776,7 @@ def _async_process_device_info( 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