Adjust thread router discovery typing (#98439)

* Adjust thread router discovery typing

* Adjust debug logs

homeassistant/components/thread/discovery.py

@@ -31,11 +31,11 @@ TYPE_PTR = 12
 class ThreadRouterDiscoveryData:
     """Thread router discovery data."""
 
-    addresses: list[str] | None
+    addresses: list[str]
     border_agent_id: str | None
     brand: str | None
-    extended_address: str | None
+    extended_address: str
-    extended_pan_id: str | None
+    extended_pan_id: str
     model_name: str | None
     network_name: str | None
     server: str | None
@@ -46,6 +46,8 @@ class ThreadRouterDiscoveryData:
 
 def async_discovery_data_from_service(
     service: AsyncServiceInfo,
+    ext_addr: bytes,
+    ext_pan_id: bytes,
 ) -> ThreadRouterDiscoveryData:
     """Get a ThreadRouterDiscoveryData from an AsyncServiceInfo."""
 
@@ -64,8 +66,6 @@ def async_discovery_data_from_service(
     service_properties = cast(dict[bytes, bytes | None], service.properties)
 
     border_agent_id = service_properties.get(b"id")
-    ext_addr = service_properties.get(b"xa")
-    ext_pan_id = service_properties.get(b"xp")
     model_name = try_decode(service_properties.get(b"mn"))
     network_name = try_decode(service_properties.get(b"nn"))
     server = service.server
@@ -90,8 +90,8 @@ def async_discovery_data_from_service(
         addresses=service.parsed_addresses(),
         border_agent_id=border_agent_id.hex() if border_agent_id is not None else None,
         brand=brand,
-        extended_address=ext_addr.hex() if ext_addr is not None else None,
+        extended_address=ext_addr.hex(),
-        extended_pan_id=ext_pan_id.hex() if ext_pan_id is not None else None,
+        extended_pan_id=ext_pan_id.hex(),
         model_name=model_name,
         network_name=network_name,
         server=server,
@@ -121,7 +121,19 @@ def async_read_zeroconf_cache(aiozc: AsyncZeroconf) -> list[ThreadRouterDiscover
             # data is not fully in the cache, so ignore for now
             continue
 
-        results.append(async_discovery_data_from_service(info))
+        # Service properties are always bytes if they are set from the network.
+        # For legacy backwards compatibility zeroconf allows properties to be set
+        # as strings but we never do that so we can safely cast here.
+        service_properties = cast(dict[bytes, bytes | None], info.properties)
+
+        if not (xa := service_properties.get(b"xa")):
+            _LOGGER.debug("Ignoring record without xa %s", info)
+            continue
+        if not (xp := service_properties.get(b"xp")):
+            _LOGGER.debug("Ignoring record without xp %s", info)
+            continue
+
+        results.append(async_discovery_data_from_service(info, xa, xp))
 
     return results
 
@@ -182,18 +194,22 @@ class ThreadRouterDiscovery:
             # as strings but we never do that so we can safely cast here.
             service_properties = cast(dict[bytes, bytes | None], service.properties)
 
+            # We need xa and xp, bail out if either is missing
             if not (xa := service_properties.get(b"xa")):
-                _LOGGER.debug("_add_update_service failed to find xa in %s", service)
+                _LOGGER.info(
+                    "Discovered unsupported Thread router without extended address: %s",
+                    service,
+                )
+                return
+            if not (xp := service_properties.get(b"xp")):
+                _LOGGER.info(
+                    "Discovered unsupported Thread router without extended pan ID: %s",
+                    service,
+                )
                 return
 
-            # We use the extended mac address as key, bail out if it's missing
+            data = async_discovery_data_from_service(service, xa, xp)
-            try:
+            extended_mac_address = xa.hex()
-                extended_mac_address = xa.hex()
-            except UnicodeDecodeError as err:
-                _LOGGER.debug("_add_update_service failed to parse service %s", err)
-                return
-
-            data = async_discovery_data_from_service(service)
             if name in self._known_routers and self._known_routers[name] == (
                 extended_mac_address,
                 data,

tests/components/thread/__init__.py

@@ -150,6 +150,7 @@ ROUTER_DISCOVERY_HASS_MISSING_DATA = {
     "properties": {
         b"rv": b"1",
         b"id": b"#\x0cj\x1a\xc5\x7foK\xe2b\xac\xf3.^\xf5,",
+        # vn is missing
         b"mn": b"OpenThreadBorderRouter",
         b"nn": b"OpenThread HC",
         b"xp": b"\xe6\x0f\xc7\xc1\x86!,\xe5",
@@ -167,7 +168,7 @@ ROUTER_DISCOVERY_HASS_MISSING_DATA = {
 }
 
 
-ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA = {
+ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XA = {
     "type_": "_meshcop._udp.local.",
     "name": "HomeAssistant OpenThreadBorderRouter #0BBF._meshcop._udp.local.",
     "addresses": [b"\xc0\xa8\x00s"],
@@ -195,6 +196,34 @@ ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA = {
 }
 
 
+ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XP = {
+    "type_": "_meshcop._udp.local.",
+    "name": "HomeAssistant OpenThreadBorderRouter #0BBF._meshcop._udp.local.",
+    "addresses": [b"\xc0\xa8\x00s"],
+    "port": 49153,
+    "weight": 0,
+    "priority": 0,
+    "server": "core-silabs-multiprotocol.local.",
+    "properties": {
+        b"rv": b"1",
+        b"id": b"#\x0cj\x1a\xc5\x7foK\xe2b\xac\xf3.^\xf5,",
+        b"vn": b"HomeAssistant",
+        b"mn": b"OpenThreadBorderRouter",
+        b"nn": b"OpenThread HC",
+        b"tv": b"1.3.0",
+        b"xa": b"\xae\xeb/YKW\x0b\xbf",
+        b"sb": b"\x00\x00\x01\xb1",
+        b"at": b"\x00\x00\x00\x00\x00\x01\x00\x00",
+        b"pt": b"\x8f\x06Q~",
+        b"sq": b"3",
+        b"bb": b"\xf0\xbf",
+        b"dn": b"DefaultDomain",
+        b"omr": b"@\xfd \xbe\x89IZ\x00\x01",
+    },
+    "interface_index": None,
+}
+
+
 ROUTER_DISCOVERY_HASS_NO_ACTIVE_TIMESTAMP = {
     "type_": "_meshcop._udp.local.",
     "name": "HomeAssistant OpenThreadBorderRouter #0BBF._meshcop._udp.local.",

tests/components/thread/test_diagnostics.py

@@ -106,6 +106,48 @@ TEST_ZEROCONF_RECORD_4 = ServiceInfo(
 # Make sure this generates an invalid DNSPointer
 TEST_ZEROCONF_RECORD_4.name = "office._meshcop._udp.lo\x00cal."
 
+# This has no XA
+TEST_ZEROCONF_RECORD_5 = ServiceInfo(
+    type_="_meshcop._udp.local.",
+    name="bad_1._meshcop._udp.local.",
+    addresses=["127.0.0.1", "fe80::10ed:6406:4ee9:85e0"],
+    port=8080,
+    properties={
+        "rv": "1",
+        "vn": "Apple",
+        "nn": "OpenThread HC",
+        "xp": "\xe6\x0f\xc7\xc1\x86!,\xe5",
+        "tv": "1.2.0",
+        "sb": "\x00\x00\x01\xb1",
+        "at": "\x00\x00\x00\x00\x00\x01\x00\x00",
+        "pt": "\x8f\x06Q~",
+        "sq": "3",
+        "bb": "\xf0\xbf",
+        "dn": "DefaultDomain",
+    },
+)
+
+# This has no XP
+TEST_ZEROCONF_RECORD_6 = ServiceInfo(
+    type_="_meshcop._udp.local.",
+    name="bad_2._meshcop._udp.local.",
+    addresses=["127.0.0.1", "fe80::10ed:6406:4ee9:85e0"],
+    port=8080,
+    properties={
+        "rv": "1",
+        "vn": "Apple",
+        "nn": "OpenThread HC",
+        "tv": "1.2.0",
+        "xa": "\xae\xeb/YKW\x0b\xbf",
+        "sb": "\x00\x00\x01\xb1",
+        "at": "\x00\x00\x00\x00\x00\x01\x00\x00",
+        "pt": "\x8f\x06Q~",
+        "sq": "3",
+        "bb": "\xf0\xbf",
+        "dn": "DefaultDomain",
+    },
+)
+
 
 @dataclasses.dataclass
 class MockRoute:
@@ -177,6 +219,24 @@ async def test_diagnostics(
             TEST_ZEROCONF_RECORD_4.dns_pointer(created=now),
         ]
     )
+    # Test for record without xa
+    cache.async_add_records(
+        [
+            *TEST_ZEROCONF_RECORD_5.dns_addresses(created=now),
+            TEST_ZEROCONF_RECORD_5.dns_service(created=now),
+            TEST_ZEROCONF_RECORD_5.dns_text(created=now),
+            TEST_ZEROCONF_RECORD_5.dns_pointer(created=now),
+        ]
+    )
+    # Test for record without xp
+    cache.async_add_records(
+        [
+            *TEST_ZEROCONF_RECORD_6.dns_addresses(created=now),
+            TEST_ZEROCONF_RECORD_6.dns_service(created=now),
+            TEST_ZEROCONF_RECORD_6.dns_text(created=now),
+            TEST_ZEROCONF_RECORD_6.dns_pointer(created=now),
+        ]
+    )
     assert await async_setup_component(hass, DOMAIN, {})
     await hass.async_block_till_done()
 

tests/components/thread/test_discovery.py

@@ -16,7 +16,8 @@ from . import (
     ROUTER_DISCOVERY_HASS_BAD_DATA,
     ROUTER_DISCOVERY_HASS_BAD_STATE_BITMAP,
     ROUTER_DISCOVERY_HASS_MISSING_DATA,
-    ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA,
+    ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XA,
+    ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XP,
     ROUTER_DISCOVERY_HASS_NO_ACTIVE_TIMESTAMP,
     ROUTER_DISCOVERY_HASS_NO_STATE_BITMAP,
     ROUTER_DISCOVERY_HASS_STATE_BITMAP_NOT_ACTIVE,
@@ -152,7 +153,7 @@ async def test_discover_routers(hass: HomeAssistant, mock_async_zeroconf: None)
 async def test_discover_routers_unconfigured(
     hass: HomeAssistant, mock_async_zeroconf: None, data, unconfigured
 ) -> None:
-    """Test discovering thread routers with bad or missing vendor mDNS data."""
+    """Test discovering thread routers and setting the unconfigured flag."""
     mock_async_zeroconf.async_add_service_listener = AsyncMock()
     mock_async_zeroconf.async_remove_service_listener = AsyncMock()
     mock_async_zeroconf.async_get_service_info = AsyncMock()
@@ -195,7 +196,7 @@ async def test_discover_routers_unconfigured(
 @pytest.mark.parametrize(
     "data", (ROUTER_DISCOVERY_HASS_BAD_DATA, ROUTER_DISCOVERY_HASS_MISSING_DATA)
 )
-async def test_discover_routers_bad_data(
+async def test_discover_routers_bad_or_missing_optional_data(
     hass: HomeAssistant, mock_async_zeroconf: None, data
 ) -> None:
     """Test discovering thread routers with bad or missing vendor mDNS data."""
@@ -238,8 +239,15 @@ async def test_discover_routers_bad_data(
     )
 
 
-async def test_discover_routers_missing_mandatory_data(
+@pytest.mark.parametrize(
-    hass: HomeAssistant, mock_async_zeroconf: None
+    "service",
+    [
+        ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XA,
+        ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA_XP,
+    ],
+)
+async def test_discover_routers_bad_or_missing_mandatory_data(
+    hass: HomeAssistant, mock_async_zeroconf: None, service
 ) -> None:
     """Test discovering thread routers with missing mandatory mDNS data."""
     mock_async_zeroconf.async_add_service_listener = AsyncMock()
@@ -261,12 +269,12 @@ async def test_discover_routers_missing_mandatory_data(
 
     # Discover a service with missing mandatory data
     mock_async_zeroconf.async_get_service_info.return_value = AsyncServiceInfo(
-        **ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA
+        **service
     )
     listener.add_service(
         None,
-        ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA["type_"],
+        service["type_"],
-        ROUTER_DISCOVERY_HASS_MISSING_MANDATORY_DATA["name"],
+        service["name"],
     )
     await hass.async_block_till_done()
     router_discovered_removed.assert_not_called()