Fix Hue homekit discovery (#37694)

Co-authored-by: Franck Nijhof <git@frenck.dev>

homeassistant/components/hue/__init__.py

@@ -161,11 +161,37 @@ async def async_setup_entry(
     config = bridge.api.config
 
     # For backwards compat
+    unique_id = normalize_bridge_id(config.bridgeid)
     if entry.unique_id is None:
-        hass.config_entries.async_update_entry(
+        hass.config_entries.async_update_entry(entry, unique_id=unique_id)
-            entry, unique_id=normalize_bridge_id(config.bridgeid)
+
+    # For recovering from bug where we incorrectly assumed homekit ID = bridge ID
+    elif entry.unique_id != unique_id:
+        # Find entries with this unique ID
+        other_entry = next(
+            (
+                entry
+                for entry in hass.config_entries.async_entries(DOMAIN)
+                if entry.unique_id == unique_id
+            ),
+            None,
         )
 
+        if other_entry is None:
+            # If no other entry, update unique ID of this entry ID.
+            hass.config_entries.async_update_entry(entry, unique_id=unique_id)
+
+        elif other_entry.source == config_entries.SOURCE_IGNORE:
+            # There is another entry but it is ignored, delete that one and update this one
+            hass.async_create_task(
+                hass.config_entries.async_remove(other_entry.entry_id)
+            )
+            hass.config_entries.async_update_entry(entry, unique_id=unique_id)
+        else:
+            # There is another entry that already has the right unique ID. Delete this entry
+            hass.async_create_task(hass.config_entries.async_remove(entry.entry_id))
+            return False
+
     device_registry = await dr.async_get_registry(hass)
     device_registry.async_get_or_create(
         config_entry_id=entry.entry_id,

homeassistant/components/hue/config_flow.py

@@ -203,18 +203,6 @@ class HueFlowHandler(config_entries.ConfigFlow, domain=DOMAIN):
         self.bridge = bridge
         return await self.async_step_link()
 
-    async def async_step_homekit(self, homekit_info):
-        """Handle HomeKit discovery."""
-        bridge = self._async_get_bridge(
-            homekit_info["host"], homekit_info["properties"]["id"]
-        )
-
-        await self.async_set_unique_id(bridge.id)
-        self._abort_if_unique_id_configured(updates={CONF_HOST: bridge.host})
-
-        self.bridge = bridge
-        return await self.async_step_link()
-
     async def async_step_import(self, import_info):
         """Import a new bridge as a config entry.
 

tests/components/hue/test_config_flow.py

@@ -538,8 +538,10 @@ async def test_creating_entry_removes_entries_for_same_host_or_bridge(hass):
     assert new_entry.unique_id == "id-1234"
 
 
-async def test_bridge_homekit(hass):
+async def test_bridge_homekit(hass, aioclient_mock):
     """Test a bridge being discovered via HomeKit."""
+    aioclient_mock.get(URL_NUPNP, json=[{"internalipaddress": "1.2.3.4", "id": "bla"}])
+
     result = await hass.config_entries.flow.async_init(
         const.DOMAIN,
         context={"source": "homekit"},
@@ -552,7 +554,7 @@ async def test_bridge_homekit(hass):
     )
 
     assert result["type"] == "form"
-    assert result["step_id"] == "link"
+    assert result["step_id"] == "init"
 
 
 async def test_bridge_import_already_configured(hass):
@@ -609,24 +611,6 @@ async def test_ssdp_discovery_update_configuration(hass):
     assert entry.data["host"] == "1.1.1.1"
 
 
-async def test_homekit_discovery_update_configuration(hass):
-    """Test if a discovered bridge is configured and updated with new host."""
-    entry = MockConfigEntry(
-        domain="hue", unique_id="aabbccddeeff", data={"host": "0.0.0.0"}
-    )
-    entry.add_to_hass(hass)
-
-    result = await hass.config_entries.flow.async_init(
-        const.DOMAIN,
-        context={"source": "homekit"},
-        data={"host": "1.1.1.1", "properties": {"id": "aa:bb:cc:dd:ee:ff"}},
-    )
-
-    assert result["type"] == "abort"
-    assert result["reason"] == "already_configured"
-    assert entry.data["host"] == "1.1.1.1"
-
-
 async def test_options_flow(hass):
     """Test options config flow."""
     entry = MockConfigEntry(

tests/components/hue/test_init.py

@@ -1,11 +1,23 @@
 """Test Hue setup process."""
 from unittest.mock import Mock
 
+import pytest
+
+from homeassistant import config_entries
 from homeassistant.components import hue
 from homeassistant.setup import async_setup_component
 
 from tests.async_mock import AsyncMock, patch
-from tests.common import MockConfigEntry, mock_coro
+from tests.common import MockConfigEntry
+
+
+@pytest.fixture
+def mock_bridge_setup():
+    """Mock bridge setup."""
+    with patch.object(hue, "HueBridge") as mock_bridge:
+        mock_bridge.return_value.async_setup = AsyncMock(return_value=True)
+        mock_bridge.return_value.api.config = Mock(bridgeid="mock-id")
+        yield mock_bridge.return_value
 
 
 async def test_setup_with_no_config(hass):
@@ -23,7 +35,7 @@ async def test_setup_defined_hosts_known_auth(hass):
     """Test we don't initiate a config entry if config bridge is known."""
     MockConfigEntry(domain="hue", data={"host": "0.0.0.0"}).add_to_hass(hass)
 
-    with patch.object(hue, "async_setup_entry", return_value=mock_coro(True)):
+    with patch.object(hue, "async_setup_entry", return_value=True):
         assert (
             await async_setup_component(
                 hass,
@@ -143,43 +155,71 @@ async def test_config_passed_to_config_entry(hass):
     }
 
 
-async def test_unload_entry(hass):
+async def test_unload_entry(hass, mock_bridge_setup):
     """Test being able to unload an entry."""
     entry = MockConfigEntry(domain=hue.DOMAIN, data={"host": "0.0.0.0"})
     entry.add_to_hass(hass)
 
-    with patch.object(hue, "HueBridge") as mock_bridge, patch(
-        "homeassistant.helpers.device_registry.async_get_registry",
-        return_value=mock_coro(Mock()),
-    ):
-        mock_bridge.return_value.async_setup = AsyncMock(return_value=True)
-        mock_bridge.return_value.api.config = Mock(bridgeid="aabbccddeeff")
     assert await async_setup_component(hass, hue.DOMAIN, {}) is True
+    assert len(mock_bridge_setup.mock_calls) == 1
 
-    assert len(mock_bridge.return_value.mock_calls) == 1
+    mock_bridge_setup.async_reset = AsyncMock(return_value=True)
-
-    mock_bridge.return_value.async_reset = AsyncMock(return_value=True)
     assert await hue.async_unload_entry(hass, entry)
-    assert len(mock_bridge.return_value.async_reset.mock_calls) == 1
+    assert len(mock_bridge_setup.async_reset.mock_calls) == 1
     assert hass.data[hue.DOMAIN] == {}
 
 
-async def test_setting_unique_id(hass):
+async def test_setting_unique_id(hass, mock_bridge_setup):
     """Test we set unique ID if not set yet."""
     entry = MockConfigEntry(domain=hue.DOMAIN, data={"host": "0.0.0.0"})
     entry.add_to_hass(hass)
-
-    with patch.object(hue, "HueBridge") as mock_bridge, patch(
-        "homeassistant.helpers.device_registry.async_get_registry",
-        return_value=mock_coro(Mock()),
-    ):
-        mock_bridge.return_value.async_setup = AsyncMock(return_value=True)
-        mock_bridge.return_value.api.config = Mock(bridgeid="mock-id")
     assert await async_setup_component(hass, hue.DOMAIN, {}) is True
-
     assert entry.unique_id == "mock-id"
 
 
+async def test_fixing_unique_id_no_other(hass, mock_bridge_setup):
+    """Test we set unique ID if not set yet."""
+    entry = MockConfigEntry(
+        domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id"
+    )
+    entry.add_to_hass(hass)
+    assert await async_setup_component(hass, hue.DOMAIN, {}) is True
+    assert entry.unique_id == "mock-id"
+
+
+async def test_fixing_unique_id_other_ignored(hass, mock_bridge_setup):
+    """Test we set unique ID if not set yet."""
+    MockConfigEntry(
+        domain=hue.DOMAIN,
+        data={"host": "0.0.0.0"},
+        unique_id="mock-id",
+        source=config_entries.SOURCE_IGNORE,
+    ).add_to_hass(hass)
+    entry = MockConfigEntry(
+        domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id",
+    )
+    entry.add_to_hass(hass)
+    assert await async_setup_component(hass, hue.DOMAIN, {}) is True
+    await hass.async_block_till_done()
+    assert entry.unique_id == "mock-id"
+    assert hass.config_entries.async_entries() == [entry]
+
+
+async def test_fixing_unique_id_other_correct(hass, mock_bridge_setup):
+    """Test we remove config entry if another one has correct ID."""
+    correct_entry = MockConfigEntry(
+        domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="mock-id",
+    )
+    correct_entry.add_to_hass(hass)
+    entry = MockConfigEntry(
+        domain=hue.DOMAIN, data={"host": "0.0.0.0"}, unique_id="invalid-id",
+    )
+    entry.add_to_hass(hass)
+    assert await async_setup_component(hass, hue.DOMAIN, {}) is True
+    await hass.async_block_till_done()
+    assert hass.config_entries.async_entries() == [correct_entry]
+
+
 async def test_security_vuln_check(hass):
     """Test that we report security vulnerabilities."""
     assert await async_setup_component(hass, "persistent_notification", {})