Fix prefer internal URL for LOQED webhooks (#178667)

homeassistant/components/loqed/coordinator.py

@@ -133,7 +133,9 @@ class LoqedDataCoordinator(DataUpdateCoordinator[StatusMessage]):
             )
         else:
             webhook_url = webhook.async_generate_url(
-                self.hass, self.config_entry.data[CONF_WEBHOOK_ID]
+                self.hass,
+                self.config_entry.data[CONF_WEBHOOK_ID],
+                prefer_external=False,
             )
 
         _LOGGER.debug("Webhook URL: %s", webhook_url)
@@ -151,20 +153,46 @@ class LoqedDataCoordinator(DataUpdateCoordinator[StatusMessage]):
 
             _LOGGER.debug("Webhook got index %s", webhook_index)
 
+        if webhook_index:
+            await self._remove_stale_webhooks(webhook_id, webhook_index, webhooks)
+
+    async def _remove_stale_webhooks(
+        self, webhook_id: str, webhook_index: int, webhooks: list[dict]
+    ) -> None:
+        cloudhook_url = self.config_entry.data.get(CONF_CLOUDHOOK_URL)
+
+        for existing_webhook in webhooks:
+            url = existing_webhook["url"]
+            index = existing_webhook["id"]
+            is_integration_webhook = url.endswith(f"/{webhook_id}") or (
+                cloudhook_url and url == cloudhook_url
+            )
+
+            if is_integration_webhook and webhook_index != index:
+                _LOGGER.debug("Removing stale webhook with URL: %s", url)
+                try:
+                    await self.lock.deleteWebhook(index)
+                except (TimeoutError, aiohttp.ClientError) as err:
+                    _LOGGER.warning(
+                        "Could not remove stale webhook from LOQED bridge: %s", err
+                    )
+
     async def remove_webhooks(self) -> None:
         """Remove webhook from LOQED bridge."""
         webhook_id = self.config_entry.data[CONF_WEBHOOK_ID]
 
-        if CONF_CLOUDHOOK_URL in self.config_entry.data:
-            webhook_url = self.config_entry.data[CONF_CLOUDHOOK_URL]
-        else:
-            webhook_url = webhook.async_generate_url(self.hass, webhook_id)
-
-        _LOGGER.debug("Webhook URL: %s", webhook_url)
-
         try:
             webhooks = await self.lock.getWebhooks()
 
+            if CONF_CLOUDHOOK_URL in self.config_entry.data:
+                webhook_url = self.config_entry.data[CONF_CLOUDHOOK_URL]
+            else:
+                webhook_url = next(
+                    (x["url"] for x in webhooks if x["url"].endswith(f"/{webhook_id}")),
+                    None,
+                )
+                _LOGGER.debug("Webhook URL: %s", webhook_url)
+
             webhook_index = next(
                 (x["id"] for x in webhooks if x["url"] == webhook_url), None
             )

tests/components/loqed/test_init.py

@@ -3,7 +3,7 @@
 from datetime import timedelta
 import json
 from typing import Any
-from unittest.mock import AsyncMock, patch
+from unittest.mock import AsyncMock, call, patch
 
 import aiohttp
 from freezegun.api import FrozenDateTimeFactory
@@ -18,7 +18,12 @@ from homeassistant.exceptions import ConfigEntryNotReady
 from homeassistant.helpers.network import get_url
 from homeassistant.setup import async_setup_component
 
-from tests.common import MockConfigEntry, async_fire_time_changed, async_load_fixture
+from tests.common import (
+    MockConfigEntry,
+    async_fire_time_changed,
+    async_load_fixture,
+    async_load_json_object_fixture,
+)
 from tests.typing import ClientSessionGenerator
 
 
@@ -71,6 +76,131 @@ async def test_setup_webhook_in_bridge(
     lock.registerWebhook.assert_called_with(f"{get_url(hass)}/api/webhook/Webhook_id")
 
 
+async def test_webhook_prefers_internal_url(
+    hass: HomeAssistant, config_entry: MockConfigEntry, lock: loqed.Lock
+) -> None:
+    """Test webhook actually prefers internal url."""
+    await hass.config.async_update(
+        internal_url="http://192.168.1.10:8123",
+        external_url="https://this-is-external-url.hass.nabu.casa",
+    )
+
+    config_entry.add_to_hass(hass)
+
+    lock_status = await async_load_json_object_fixture(hass, "status_ok.json", DOMAIN)
+
+    webhooks_fixture = json.loads(
+        await async_load_fixture(hass, "get_all_webhooks.json", DOMAIN)
+    )
+    webhooks_fixture[0]["url"] = f"{hass.config.internal_url}/api/webhook/Webhook_id"
+
+    lock.getWebhooks = AsyncMock(side_effect=[[], webhooks_fixture])
+
+    with (
+        patch("loqedAPI.loqed.LoqedAPI.async_get_lock", return_value=lock),
+        patch(
+            "loqedAPI.loqed.LoqedAPI.async_get_lock_details", return_value=lock_status
+        ),
+    ):
+        await hass.config_entries.async_setup(config_entry.entry_id)
+        await hass.async_block_till_done()
+
+    lock.registerWebhook.assert_called_with(
+        f"{hass.config.internal_url}/api/webhook/Webhook_id"
+    )
+
+
+async def test_ensure_webhooks_removes_stale_webhooks(
+    hass: HomeAssistant, config_entry: MockConfigEntry, lock: loqed.Lock
+) -> None:
+    """Test that stale webhooks with the same ID but different URL are removed."""
+    await hass.config.async_update(
+        internal_url="http://192.168.1.10:8123",
+        external_url="https://this-is-external-url.hass.nabu.casa",
+    )
+
+    config_entry.add_to_hass(hass)
+    webhook_id = config_entry.data[CONF_WEBHOOK_ID]
+
+    lock_status = await async_load_json_object_fixture(hass, "status_ok.json", DOMAIN)
+
+    stale_webhook = {
+        "id": 14,
+        "url": f"192.168.14.233/api/webhook/{webhook_id}",
+    }
+    another_stale_webhook = {
+        "id": 4,
+        "url": f"{hass.config.external_url}/api/webhook/{webhook_id}",
+    }
+    new_webhook = {
+        "id": 15,
+        "url": f"{hass.config.internal_url}/api/webhook/{webhook_id}",
+    }
+
+    lock.getWebhooks = AsyncMock(
+        side_effect=[
+            [stale_webhook, another_stale_webhook],
+            [stale_webhook, another_stale_webhook, new_webhook],
+        ]
+    )
+
+    with (
+        patch("loqedAPI.loqed.LoqedAPI.async_get_lock", return_value=lock),
+        patch(
+            "loqedAPI.loqed.LoqedAPI.async_get_lock_details", return_value=lock_status
+        ),
+    ):
+        await hass.config_entries.async_setup(config_entry.entry_id)
+        await hass.async_block_till_done()
+
+    lock.registerWebhook.assert_called_once()
+    assert lock.deleteWebhook.call_count == 2
+    lock.deleteWebhook.assert_has_calls([call(14), call(4)], any_order=True)
+
+
+async def test_ensure_webhooks_handles_bridge_error_on_cleanup(
+    hass: HomeAssistant,
+    config_entry: MockConfigEntry,
+    lock: loqed.Lock,
+    caplog: pytest.LogCaptureFixture,
+) -> None:
+    """Test that stale webhook cleanup handles bridge connection errors smoothly."""
+    await hass.config.async_update(internal_url="http://192.168.1.10:8123")
+
+    config_entry.add_to_hass(hass)
+    webhook_id = config_entry.data[CONF_WEBHOOK_ID]
+
+    lock_status = await async_load_json_object_fixture(hass, "status_ok.json", DOMAIN)
+
+    stale_webhook = {
+        "id": 14,
+        "url": f"https://this-is-external-url.hass.nabu.casa/api/webhook/{webhook_id}",
+    }
+    new_webhook = {
+        "id": 15,
+        "url": f"{hass.config.internal_url}/api/webhook/{webhook_id}",
+    }
+
+    lock.getWebhooks = AsyncMock(
+        side_effect=[[stale_webhook], [stale_webhook, new_webhook]]
+    )
+
+    lock.deleteWebhook = AsyncMock(side_effect=aiohttp.ClientError)
+
+    with (
+        patch("loqedAPI.loqed.LoqedAPI.async_get_lock", return_value=lock),
+        patch(
+            "loqedAPI.loqed.LoqedAPI.async_get_lock_details", return_value=lock_status
+        ),
+    ):
+        await hass.config_entries.async_setup(config_entry.entry_id)
+        await hass.async_block_till_done()
+
+    lock.deleteWebhook.assert_called_once_with(14)
+
+    assert "Could not remove stale webhook from LOQED bridge" in caplog.text
+
+
 async def test_cannot_connect_to_bridge_will_retry(
     hass: HomeAssistant, config_entry: MockConfigEntry, lock: loqed.Lock
 ) -> None: