From 56309a2fc27dff876e73349bc1a8d21d81d3e1e3 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Thu, 2 Apr 2020 15:09:12 +0000 Subject: [PATCH] Ensure harmony hub is ready before importing If the harmony hub was not ready for connection or was busy when importing from yaml, the import validation would fail would not be retried. To mitigate this scenario we now do the validation in async_setup_platform which allows us to raise PlatformNotReady so we can retry later. --- .../components/harmony/config_flow.py | 58 +++++++------------ homeassistant/components/harmony/remote.py | 27 ++++++++- homeassistant/components/harmony/util.py | 44 +++++++++++++- tests/components/harmony/test_config_flow.py | 17 +++--- 4 files changed, 95 insertions(+), 51 deletions(-) diff --git a/homeassistant/components/harmony/config_flow.py b/homeassistant/components/harmony/config_flow.py index ff7b47d60107e4..9d9c9dfb8e97ac 100644 --- a/homeassistant/components/harmony/config_flow.py +++ b/homeassistant/components/harmony/config_flow.py @@ -2,11 +2,9 @@ import logging from urllib.parse import urlparse -import aioharmony.exceptions as harmony_exceptions -from aioharmony.harmonyapi import HarmonyAPI import voluptuous as vol -from homeassistant import config_entries, core, exceptions +from homeassistant import config_entries, exceptions from homeassistant.components import ssdp from homeassistant.components.remote import ( ATTR_ACTIVITY, @@ -17,7 +15,11 @@ from homeassistant.core import callback from .const import DOMAIN, UNIQUE_ID -from .util import find_unique_id_for_remote +from .util import ( + find_best_name_for_remote, + find_unique_id_for_remote, + get_harmony_client_if_available, +) _LOGGER = logging.getLogger(__name__) @@ -26,43 +28,19 @@ ) -async def get_harmony_client_if_available(hass: core.HomeAssistant, ip_address): - """Connect to a harmony hub and fetch info.""" - harmony = HarmonyAPI(ip_address=ip_address) - - try: - if not await harmony.connect(): - await harmony.close() - return None - except harmony_exceptions.TimeOut: - return None - - await harmony.close() - - return harmony - - -async def validate_input(hass: core.HomeAssistant, data): +async def validate_input(data): """Validate the user input allows us to connect. Data has the keys from DATA_SCHEMA with values provided by the user. """ - harmony = await get_harmony_client_if_available(hass, data[CONF_HOST]) + harmony = await get_harmony_client_if_available(data[CONF_HOST]) if not harmony: raise CannotConnect - unique_id = find_unique_id_for_remote(harmony) - - # As a last resort we get the name from the harmony client - # in the event a name was not provided. harmony.name is - # usually the ip address but it can be an empty string. - if CONF_NAME not in data or data[CONF_NAME] is None or data[CONF_NAME] == "": - data[CONF_NAME] = harmony.name - return { - CONF_NAME: data[CONF_NAME], + CONF_NAME: find_best_name_for_remote(data, harmony), CONF_HOST: data[CONF_HOST], - UNIQUE_ID: unique_id, + UNIQUE_ID: find_unique_id_for_remote(harmony), } @@ -82,7 +60,7 @@ async def async_step_user(self, user_input=None): if user_input is not None: try: - validated = await validate_input(self.hass, user_input) + validated = await validate_input(user_input) except CannotConnect: errors["base"] = "cannot_connect" except Exception: # pylint: disable=broad-except @@ -116,9 +94,7 @@ async def async_step_ssdp(self, discovery_info): CONF_NAME: friendly_name, } - harmony = await get_harmony_client_if_available( - self.hass, self.harmony_config[CONF_HOST] - ) + harmony = await get_harmony_client_if_available(parsed_url.hostname) if harmony: unique_id = find_unique_id_for_remote(harmony) @@ -150,9 +126,15 @@ async def async_step_link(self, user_input=None): }, ) - async def async_step_import(self, user_input): + async def async_step_import(self, validated_input): """Handle import.""" - return await self.async_step_user(user_input) + await self.async_set_unique_id(validated_input[UNIQUE_ID]) + self._abort_if_unique_id_configured() + # Everything was validated in remote async_setup_platform + # all we do now is create. + return await self._async_create_entry_from_valid_input( + validated_input, validated_input + ) @staticmethod @callback diff --git a/homeassistant/components/harmony/remote.py b/homeassistant/components/harmony/remote.py index 024af9b15804bd..5af8d1eb65a31a 100644 --- a/homeassistant/components/harmony/remote.py +++ b/homeassistant/components/harmony/remote.py @@ -24,6 +24,7 @@ from homeassistant.config_entries import SOURCE_IMPORT, ConfigEntry from homeassistant.const import ATTR_ENTITY_ID, CONF_HOST, CONF_NAME from homeassistant.core import HomeAssistant +from homeassistant.exceptions import PlatformNotReady import homeassistant.helpers.config_validation as cv from homeassistant.helpers.dispatcher import async_dispatcher_connect @@ -33,6 +34,13 @@ HARMONY_OPTIONS_UPDATE, SERVICE_CHANGE_CHANNEL, SERVICE_SYNC, + UNIQUE_ID, +) +from .util import ( + find_best_name_for_remote, + find_matching_config_entries_for_host, + find_unique_id_for_remote, + get_harmony_client_if_available, ) _LOGGER = logging.getLogger(__name__) @@ -51,6 +59,7 @@ extra=vol.ALLOW_EXTRA, ) + HARMONY_SYNC_SCHEMA = vol.Schema({vol.Optional(ATTR_ENTITY_ID): cv.entity_ids}) HARMONY_CHANGE_CHANNEL_SCHEMA = vol.Schema( @@ -68,9 +77,25 @@ async def async_setup_platform(hass, config, async_add_entities, discovery_info= # Now handled by ssdp in the config flow return + if find_matching_config_entries_for_host(hass, config[CONF_HOST]): + return + + # We do the validation to verify we can connect + # so we can raise PlatformNotReady to force + # a retry so we can avoid a scenario where the config + # entry cannot be created via import because hub + # is not yet ready. + harmony = await get_harmony_client_if_available(config[CONF_HOST]) + if not harmony: + raise PlatformNotReady + + validated_config = config.copy() + validated_config[UNIQUE_ID] = find_unique_id_for_remote(harmony) + validated_config[CONF_NAME] = find_best_name_for_remote(config, harmony) + hass.async_create_task( hass.config_entries.flow.async_init( - DOMAIN, context={"source": SOURCE_IMPORT}, data=config + DOMAIN, context={"source": SOURCE_IMPORT}, data=validated_config ) ) diff --git a/homeassistant/components/harmony/util.py b/homeassistant/components/harmony/util.py index 5f7e46510f9f95..69ed44cb7dac10 100644 --- a/homeassistant/components/harmony/util.py +++ b/homeassistant/components/harmony/util.py @@ -1,8 +1,13 @@ """The Logitech Harmony Hub integration utils.""" -from aioharmony.harmonyapi import HarmonyAPI as HarmonyClient +import aioharmony.exceptions as harmony_exceptions +from aioharmony.harmonyapi import HarmonyAPI +from homeassistant.const import CONF_HOST, CONF_NAME -def find_unique_id_for_remote(harmony: HarmonyClient): +from .const import DOMAIN + + +def find_unique_id_for_remote(harmony: HarmonyAPI): """Find the unique id for both websocket and xmpp clients.""" websocket_unique_id = harmony.hub_config.info.get("activeRemoteId") if websocket_unique_id is not None: @@ -10,3 +15,38 @@ def find_unique_id_for_remote(harmony: HarmonyClient): # fallback to the xmpp unique id if websocket is not available return harmony.config["global"]["timeStampHash"].split(";")[-1] + + +def find_best_name_for_remote(data: dict, harmony: HarmonyAPI): + """Find the best name from config or fallback to the remote.""" + # As a last resort we get the name from the harmony client + # in the event a name was not provided. harmony.name is + # usually the ip address but it can be an empty string. + if CONF_NAME not in data or data[CONF_NAME] is None or data[CONF_NAME] == "": + return harmony.name + + return data[CONF_NAME] + + +async def get_harmony_client_if_available(ip_address: str): + """Connect to a harmony hub and fetch info.""" + harmony = HarmonyAPI(ip_address=ip_address) + + try: + if not await harmony.connect(): + await harmony.close() + return None + except harmony_exceptions.TimeOut: + return None + + await harmony.close() + + return harmony + + +def find_matching_config_entries_for_host(hass, host): + """Search existing config entries for one matching the host.""" + for entry in hass.config_entries.async_entries(DOMAIN): + if entry.data[CONF_HOST] == host: + return entry + return None diff --git a/tests/components/harmony/test_config_flow.py b/tests/components/harmony/test_config_flow.py index 18c0825b6a1dcb..30421756d22514 100644 --- a/tests/components/harmony/test_config_flow.py +++ b/tests/components/harmony/test_config_flow.py @@ -25,8 +25,7 @@ async def test_user_form(hass): harmonyapi = _get_mock_harmonyapi(connect=True) with patch( - "homeassistant.components.harmony.config_flow.HarmonyAPI", - return_value=harmonyapi, + "homeassistant.components.harmony.util.HarmonyAPI", return_value=harmonyapi, ), patch( "homeassistant.components.harmony.async_setup", return_value=True ) as mock_setup, patch( @@ -53,8 +52,7 @@ async def test_form_import(hass): harmonyapi = _get_mock_harmonyapi(connect=True) with patch( - "homeassistant.components.harmony.config_flow.HarmonyAPI", - return_value=harmonyapi, + "homeassistant.components.harmony.util.HarmonyAPI", return_value=harmonyapi, ), patch( "homeassistant.components.harmony.async_setup", return_value=True ) as mock_setup, patch( @@ -68,9 +66,11 @@ async def test_form_import(hass): "name": "friend", "activity": "Watch TV", "delay_secs": 0.9, + "unique_id": "555234534543", }, ) + assert result["result"].unique_id == "555234534543" assert result["type"] == "create_entry" assert result["title"] == "friend" assert result["data"] == { @@ -94,8 +94,7 @@ async def test_form_ssdp(hass): harmonyapi = _get_mock_harmonyapi(connect=True) with patch( - "homeassistant.components.harmony.config_flow.HarmonyAPI", - return_value=harmonyapi, + "homeassistant.components.harmony.util.HarmonyAPI", return_value=harmonyapi, ): result = await hass.config_entries.flow.async_init( DOMAIN, @@ -114,8 +113,7 @@ async def test_form_ssdp(hass): } with patch( - "homeassistant.components.harmony.config_flow.HarmonyAPI", - return_value=harmonyapi, + "homeassistant.components.harmony.util.HarmonyAPI", return_value=harmonyapi, ), patch( "homeassistant.components.harmony.async_setup", return_value=True ) as mock_setup, patch( @@ -141,8 +139,7 @@ async def test_form_cannot_connect(hass): ) with patch( - "homeassistant.components.harmony.config_flow.HarmonyAPI", - side_effect=CannotConnect, + "homeassistant.components.harmony.util.HarmonyAPI", side_effect=CannotConnect, ): result2 = await hass.config_entries.flow.async_configure( result["flow_id"],