From cac0c04a91239eeb8c6ee912608b1e5b1cf52185 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 17 Oct 2021 17:32:02 -1000 Subject: [PATCH] Avoid setting up harmony websocket from discovery (#57589) --- homeassistant/components/harmony/__init__.py | 13 ++------- .../components/harmony/config_flow.py | 25 ++++++++++------ homeassistant/components/harmony/data.py | 29 +++++++++++++------ .../components/harmony/manifest.json | 2 +- requirements_all.txt | 2 +- requirements_test_all.txt | 2 +- tests/components/harmony/test_config_flow.py | 29 ++++++++++++++++--- 7 files changed, 66 insertions(+), 36 deletions(-) diff --git a/homeassistant/components/harmony/__init__.py b/homeassistant/components/harmony/__init__.py index 00a0feca4f7..4ec610f1f75 100644 --- a/homeassistant/components/harmony/__init__.py +++ b/homeassistant/components/harmony/__init__.py @@ -1,12 +1,10 @@ """The Logitech Harmony Hub integration.""" -import asyncio import logging from homeassistant.components.remote import ATTR_ACTIVITY, ATTR_DELAY_SECS from homeassistant.config_entries import ConfigEntry from homeassistant.const import CONF_HOST, CONF_NAME, EVENT_HOMEASSISTANT_STOP from homeassistant.core import HomeAssistant, callback -from homeassistant.exceptions import ConfigEntryNotReady from homeassistant.helpers import entity_registry from homeassistant.helpers.dispatcher import async_dispatcher_send @@ -34,13 +32,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: address = entry.data[CONF_HOST] name = entry.data[CONF_NAME] data = HarmonyData(hass, address, name, entry.unique_id) - try: - connected_ok = await data.connect() - except (asyncio.TimeoutError, ValueError, AttributeError) as err: - raise ConfigEntryNotReady from err - - if not connected_ok: - raise ConfigEntryNotReady + await data.connect() await _migrate_old_unique_ids(hass, entry.entry_id, data) @@ -51,8 +43,7 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: cancel_stop = hass.bus.async_listen(EVENT_HOMEASSISTANT_STOP, _async_on_stop) - hass.data.setdefault(DOMAIN, {}) - hass.data[DOMAIN][entry.entry_id] = { + hass.data.setdefault(DOMAIN, {})[entry.entry_id] = { HARMONY_DATA: data, CANCEL_LISTENER: cancel_listener, CANCEL_STOP: cancel_stop, diff --git a/homeassistant/components/harmony/config_flow.py b/homeassistant/components/harmony/config_flow.py index b1e71ac2dab..1c735b0747d 100644 --- a/homeassistant/components/harmony/config_flow.py +++ b/homeassistant/components/harmony/config_flow.py @@ -1,7 +1,10 @@ """Config flow for Logitech Harmony Hub integration.""" +import asyncio import logging from urllib.parse import urlparse +from aioharmony.hubconnector_websocket import HubConnector +import aiohttp import voluptuous as vol from homeassistant import config_entries, exceptions @@ -94,16 +97,20 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): CONF_NAME: friendly_name, } - harmony = await get_harmony_client_if_available(parsed_url.hostname) - - if harmony: - unique_id = find_unique_id_for_remote(harmony) - await self.async_set_unique_id(unique_id) - self._abort_if_unique_id_configured( - updates={CONF_HOST: self.harmony_config[CONF_HOST]} - ) - self.harmony_config[UNIQUE_ID] = unique_id + connector = HubConnector(parsed_url.hostname, asyncio.Queue()) + try: + remote_id = await connector.get_remote_id() + except aiohttp.ClientError: + return self.async_abort(reason="cannot_connect") + finally: + await connector.async_close_session() + unique_id = str(remote_id) + await self.async_set_unique_id(str(unique_id)) + self._abort_if_unique_id_configured( + updates={CONF_HOST: self.harmony_config[CONF_HOST]} + ) + self.harmony_config[UNIQUE_ID] = unique_id return await self.async_step_link() async def async_step_link(self, user_input=None): diff --git a/homeassistant/components/harmony/data.py b/homeassistant/components/harmony/data.py index 6fdf18df612..78377265c07 100644 --- a/homeassistant/components/harmony/data.py +++ b/homeassistant/components/harmony/data.py @@ -1,6 +1,7 @@ """Harmony data object which contains the Harmony Client.""" from __future__ import annotations +import asyncio from collections.abc import Iterable import logging @@ -8,6 +9,8 @@ from aioharmony.const import ClientCallbackType, SendCommandDevice import aioharmony.exceptions as aioexc from aioharmony.harmonyapi import HarmonyAPI as HarmonyClient +from homeassistant.exceptions import ConfigEntryNotReady + from .const import ACTIVITY_POWER_OFF from .subscriber import HarmonySubscriberMixin @@ -109,16 +112,24 @@ class HarmonyData(HarmonySubscriberMixin): ip_address=self._address, callbacks=ClientCallbackType(**callbacks) ) + connected = False try: - if not await self._client.connect(): - _LOGGER.warning("%s: Unable to connect to HUB", self._name) - await self._client.close() - return False - except aioexc.TimeOut: - _LOGGER.warning("%s: Connection timed-out", self._name) - return False - - return True + connected = await self._client.connect() + except (asyncio.TimeoutError, aioexc.TimeOut) as err: + await self._client.close() + raise ConfigEntryNotReady( + f"{self._name}: Connection timed-out to {self._address}:8088" + ) from err + except (ValueError, AttributeError) as err: + await self._client.close() + raise ConfigEntryNotReady( + f"{self._name}: Error {err} while connected HUB at: {self._address}:8088" + ) from err + if not connected: + await self._client.close() + raise ConfigEntryNotReady( + f"{self._name}: Unable to connect to HUB at: {self._address}:8088" + ) async def shutdown(self): """Close connection on shutdown.""" diff --git a/homeassistant/components/harmony/manifest.json b/homeassistant/components/harmony/manifest.json index f35f4e99303..d1b1073ebad 100644 --- a/homeassistant/components/harmony/manifest.json +++ b/homeassistant/components/harmony/manifest.json @@ -2,7 +2,7 @@ "domain": "harmony", "name": "Logitech Harmony Hub", "documentation": "https://www.home-assistant.io/integrations/harmony", - "requirements": ["aioharmony==0.2.7"], + "requirements": ["aioharmony==0.2.8"], "codeowners": [ "@ehendrix23", "@bramkragten", diff --git a/requirements_all.txt b/requirements_all.txt index ad08e06a848..b05c90d243e 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -176,7 +176,7 @@ aiogithubapi==21.8.0 aioguardian==1.0.8 # homeassistant.components.harmony -aioharmony==0.2.7 +aioharmony==0.2.8 # homeassistant.components.homekit_controller aiohomekit==0.6.3 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 2a7205a1893..3d0715314ef 100644 --- a/requirements_test_all.txt +++ b/requirements_test_all.txt @@ -118,7 +118,7 @@ aioflo==0.4.1 aioguardian==1.0.8 # homeassistant.components.harmony -aioharmony==0.2.7 +aioharmony==0.2.8 # homeassistant.components.homekit_controller aiohomekit==0.6.3 diff --git a/tests/components/harmony/test_config_flow.py b/tests/components/harmony/test_config_flow.py index be0d78242ac..9195af40cf1 100644 --- a/tests/components/harmony/test_config_flow.py +++ b/tests/components/harmony/test_config_flow.py @@ -1,6 +1,8 @@ """Test the Logitech Harmony Hub config flow.""" from unittest.mock import AsyncMock, MagicMock, patch +import aiohttp + from homeassistant import config_entries, data_entry_flow from homeassistant.components.harmony.config_flow import CannotConnect from homeassistant.components.harmony.const import DOMAIN, PREVIOUS_ACTIVE_ACTIVITY @@ -49,11 +51,9 @@ async def test_user_form(hass): async def test_form_ssdp(hass): """Test we get the form with ssdp source.""" - harmonyapi = _get_mock_harmonyapi(connect=True) - with patch( - "homeassistant.components.harmony.util.HarmonyAPI", - return_value=harmonyapi, + "homeassistant.components.harmony.config_flow.HubConnector.get_remote_id", + return_value=1234, ): result = await hass.config_entries.flow.async_init( DOMAIN, @@ -75,6 +75,8 @@ async def test_form_ssdp(hass): assert progress[0]["flow_id"] == result["flow_id"] assert progress[0]["context"]["confirm_only"] is True + harmonyapi = _get_mock_harmonyapi(connect=True) + with patch( "homeassistant.components.harmony.util.HarmonyAPI", return_value=harmonyapi, @@ -94,6 +96,25 @@ async def test_form_ssdp(hass): assert len(mock_setup_entry.mock_calls) == 1 +async def test_form_ssdp_fails_to_get_remote_id(hass): + """Test we abort if we cannot get the remote id.""" + + with patch( + "homeassistant.components.harmony.config_flow.HubConnector.get_remote_id", + side_effect=aiohttp.ClientError, + ): + result = await hass.config_entries.flow.async_init( + DOMAIN, + context={"source": config_entries.SOURCE_SSDP}, + data={ + "friendlyName": "Harmony Hub", + "ssdp_location": "http://192.168.1.12:8088/description", + }, + ) + assert result["type"] == "abort" + assert result["reason"] == "cannot_connect" + + async def test_form_ssdp_aborts_before_checking_remoteid_if_host_known(hass): """Test we abort without connecting if the host is already known."""