From fc8af22191d054524c9e124a5fed7ebade32f16c Mon Sep 17 00:00:00 2001 From: Steven Looman Date: Tue, 23 Oct 2018 21:52:01 +0200 Subject: [PATCH] IGD review fixes (#17400) * Fix discovery-dependency for upnp * Only delete port mappings on EVENT_HOMEASSISTANT_STOP + refactoring MockDevice * Call and store local_ip from async_setup * Don't depend on http-component * Remove discovery dependency --- homeassistant/components/upnp/__init__.py | 59 +++++++---- homeassistant/components/upnp/device.py | 9 +- tests/components/upnp/test_init.py | 117 ++++++++++++++-------- 3 files changed, 118 insertions(+), 67 deletions(-) diff --git a/homeassistant/components/upnp/__init__.py b/homeassistant/components/upnp/__init__.py index 07c8d5f748e..c667327b71b 100644 --- a/homeassistant/components/upnp/__init__.py +++ b/homeassistant/components/upnp/__init__.py @@ -16,7 +16,7 @@ from homeassistant.helpers import config_validation as cv from homeassistant.helpers import dispatcher from homeassistant.helpers.typing import ConfigType from homeassistant.helpers.typing import HomeAssistantType -from homeassistant.components.discovery import DOMAIN as DISCOVERY_DOMAIN +from homeassistant.util import get_local_ip from .const import ( CONF_ENABLE_PORT_MAPPING, CONF_ENABLE_SENSORS, @@ -31,7 +31,6 @@ from .device import Device REQUIREMENTS = ['async-upnp-client==0.12.7'] -DEPENDENCIES = ['http'] NOTIFICATION_ID = 'upnp_notification' NOTIFICATION_TITLE = 'UPnP/IGD Setup' @@ -50,18 +49,37 @@ CONFIG_SCHEMA = vol.Schema({ }, extra=vol.ALLOW_EXTRA) -def _substitute_hass_ports(ports, hass_port): - """Substitute 'hass' for the hass_port.""" +def _substitute_hass_ports(ports, hass_port=None): + """ + Substitute 'hass' for the hass_port. + + This triggers a warning when hass_port is None. + """ ports = ports.copy() # substitute 'hass' for hass_port, both keys and values if CONF_HASS in ports: - ports[hass_port] = ports[CONF_HASS] + if hass_port is None: + _LOGGER.warning( + 'Could not determine Home Assistant http port, ' + 'not setting up port mapping from %s to %s. ' + 'Enable the http-component.', + CONF_HASS, ports[CONF_HASS]) + else: + ports[hass_port] = ports[CONF_HASS] del ports[CONF_HASS] for port in ports: if ports[port] == CONF_HASS: - ports[port] = hass_port + if hass_port is None: + _LOGGER.warning( + 'Could not determine Home Assistant http port, ' + 'not setting up port mapping from %s to %s. ' + 'Enable the http-component.', + port, ports[port]) + del ports[port] + else: + ports[port] = hass_port return ports @@ -74,15 +92,14 @@ async def async_setup(hass: HomeAssistantType, config: ConfigType): # ensure sane config if DOMAIN not in config: return True - - if DISCOVERY_DOMAIN not in config: - _LOGGER.warning('UPNP needs discovery, please enable it') - return False + upnp_config = config[DOMAIN] # overridden local ip - upnp_config = config[DOMAIN] if CONF_LOCAL_IP in upnp_config: hass.data[DOMAIN]['local_ip'] = upnp_config[CONF_LOCAL_IP] + else: + hass.data[DOMAIN]['local_ip'] = \ + await hass.async_add_executor_job(get_local_ip) # determine ports ports = {CONF_HASS: CONF_HASS} # default, port_mapping disabled by default @@ -119,13 +136,15 @@ async def async_setup_entry(hass: HomeAssistantType, # port mapping if data.get(CONF_ENABLE_PORT_MAPPING): - local_ip = hass.data[DOMAIN].get('local_ip') + local_ip = hass.data[DOMAIN]['local_ip'] ports = hass.data[DOMAIN]['auto_config']['ports'] _LOGGER.debug('Enabling port mappings: %s', ports) - hass_port = hass.http.server_port - ports = _substitute_hass_ports(ports, hass_port) - await device.async_add_port_mappings(ports, local_ip=local_ip) + hass_port = None + if hasattr(hass, 'http'): + hass_port = hass.http.server_port + ports = _substitute_hass_ports(ports, hass_port=hass_port) + await device.async_add_port_mappings(ports, local_ip) # sensors if data.get(CONF_ENABLE_SENSORS): @@ -135,10 +154,12 @@ async def async_setup_entry(hass: HomeAssistantType, hass.async_create_task(hass.config_entries.async_forward_entry_setup( config_entry, 'sensor')) - async def unload_entry(event): - """Unload entry on quit.""" - await async_unload_entry(hass, config_entry) - hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, unload_entry) + async def delete_port_mapping(event): + """Delete port mapping on quit.""" + if data.get(CONF_ENABLE_PORT_MAPPING): + _LOGGER.debug('Deleting port mappings') + await device.async_delete_port_mappings() + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, delete_port_mapping) return True diff --git a/homeassistant/components/upnp/device.py b/homeassistant/components/upnp/device.py index 4a444aa3087..4a0b7b61dd4 100644 --- a/homeassistant/components/upnp/device.py +++ b/homeassistant/components/upnp/device.py @@ -6,7 +6,6 @@ import aiohttp from homeassistant.helpers.aiohttp_client import async_get_clientsession from homeassistant.helpers.typing import HomeAssistantType -from homeassistant.util import get_local_ip from .const import LOGGER as _LOGGER @@ -51,15 +50,13 @@ class Device: """Get the name.""" return self._igd_device.name - async def async_add_port_mappings(self, ports, local_ip=None): + async def async_add_port_mappings(self, ports, local_ip): """Add port mappings.""" - # determine local ip, ensure sane IP - if local_ip is None: - local_ip = get_local_ip() - if local_ip == '127.0.0.1': _LOGGER.error( 'Could not create port mapping, our IP is 127.0.0.1') + + # determine local ip, ensure sane IP local_ip = IPv4Address(local_ip) # create port mappings diff --git a/tests/components/upnp/test_init.py b/tests/components/upnp/test_init.py index ce4656032a6..7f163d5bcef 100644 --- a/tests/components/upnp/test_init.py +++ b/tests/components/upnp/test_init.py @@ -9,6 +9,7 @@ from homeassistant.components.upnp.device import Device from homeassistant.const import EVENT_HOMEASSISTANT_STOP from tests.common import MockConfigEntry +from tests.common import MockDependency from tests.common import mock_coro @@ -17,7 +18,7 @@ class MockDevice(Device): def __init__(self, udn): """Initializer.""" - super().__init__(None) + super().__init__(MagicMock()) self._udn = udn self.added_port_mappings = [] self.removed_port_mappings = [] @@ -49,7 +50,15 @@ class MockDevice(Device): async def test_async_setup_no_auto_config(hass): """Test async_setup.""" # setup component, enable auto_config - await async_setup_component(hass, 'upnp') + config = { + 'discovery': {}, + # no upnp + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() assert hass.data[upnp.DOMAIN]['auto_config'] == { 'active': False, @@ -62,7 +71,15 @@ async def test_async_setup_no_auto_config(hass): async def test_async_setup_auto_config(hass): """Test async_setup.""" # setup component, enable auto_config - await async_setup_component(hass, 'upnp', {'upnp': {}, 'discovery': {}}) + config = { + 'discovery': {}, + 'upnp': {}, + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() assert hass.data[upnp.DOMAIN]['auto_config'] == { 'active': True, @@ -75,12 +92,18 @@ async def test_async_setup_auto_config(hass): async def test_async_setup_auto_config_port_mapping(hass): """Test async_setup.""" # setup component, enable auto_config - await async_setup_component(hass, 'upnp', { + config = { + 'discovery': {}, 'upnp': { 'port_mapping': True, 'ports': {'hass': 'hass'}, }, - 'discovery': {}}) + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() assert hass.data[upnp.DOMAIN]['auto_config'] == { 'active': True, @@ -93,9 +116,15 @@ async def test_async_setup_auto_config_port_mapping(hass): async def test_async_setup_auto_config_no_sensors(hass): """Test async_setup.""" # setup component, enable auto_config - await async_setup_component(hass, 'upnp', { + config = { + 'discovery': {}, 'upnp': {'sensors': False}, - 'discovery': {}}) + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() assert hass.data[upnp.DOMAIN]['auto_config'] == { 'active': True, @@ -115,18 +144,23 @@ async def test_async_setup_entry_default(hass): 'port_mapping': False, }) - # ensure hass.http is available - await async_setup_component(hass, 'upnp') + config = { + 'http': {}, + 'discovery': {}, + # no upnp + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'http', config) + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() # mock homeassistant.components.upnp.device.Device - mock_device = MagicMock() - mock_device.udn = udn - mock_device.async_add_port_mappings.return_value = mock_coro() - mock_device.async_delete_port_mappings.return_value = mock_coro() - with patch.object(Device, 'async_create_device') as mock_create_device: - mock_create_device.return_value = mock_coro( - return_value=mock_device) - with patch('homeassistant.components.upnp.device.get_local_ip', + mock_device = MockDevice(udn) + with patch.object(Device, 'async_create_device') as create_device: + create_device.return_value = mock_coro(return_value=mock_device) + with patch('homeassistant.components.upnp.get_local_ip', return_value='192.168.1.10'): assert await upnp.async_setup_entry(hass, entry) is True @@ -136,12 +170,9 @@ async def test_async_setup_entry_default(hass): hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) await hass.async_block_till_done() - # ensure cleaned up - assert udn not in hass.data[upnp.DOMAIN]['devices'] - - # ensure no port-mapping-methods called - assert len(mock_device.async_add_port_mappings.mock_calls) == 0 - assert len(mock_device.async_delete_port_mappings.mock_calls) == 0 + # ensure no port-mappings created or removed + assert not mock_device.added_port_mappings + assert not mock_device.removed_port_mappings async def test_async_setup_entry_port_mapping(hass): @@ -154,35 +185,37 @@ async def test_async_setup_entry_port_mapping(hass): 'port_mapping': True, }) - # ensure hass.http is available - await async_setup_component(hass, 'upnp', { + config = { + 'http': {}, + 'discovery': {}, 'upnp': { 'port_mapping': True, 'ports': {'hass': 'hass'}, }, - 'discovery': {}, - }) + } + with MockDependency('netdisco.discovery'), \ + patch('homeassistant.components.upnp.get_local_ip', + return_value='192.168.1.10'): + await async_setup_component(hass, 'http', config) + await async_setup_component(hass, 'upnp', config) + await hass.async_block_till_done() mock_device = MockDevice(udn) - with patch.object(Device, 'async_create_device') as mock_create_device: - mock_create_device.return_value = mock_coro(return_value=mock_device) - with patch('homeassistant.components.upnp.device.get_local_ip', - return_value='192.168.1.10'): - assert await upnp.async_setup_entry(hass, entry) is True + with patch.object(Device, 'async_create_device') as create_device: + create_device.return_value = mock_coro(return_value=mock_device) - # ensure device is stored/used - assert hass.data[upnp.DOMAIN]['devices'][udn] == mock_device + assert await upnp.async_setup_entry(hass, entry) is True - # ensure add-port-mapping-methods called - assert mock_device.added_port_mappings == [ - [8123, ip_address('192.168.1.10'), 8123] - ] + # ensure device is stored/used + assert hass.data[upnp.DOMAIN]['devices'][udn] == mock_device - hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) - await hass.async_block_till_done() + # ensure add-port-mapping-methods called + assert mock_device.added_port_mappings == [ + [8123, ip_address('192.168.1.10'), 8123] + ] - # ensure cleaned up - assert udn not in hass.data[upnp.DOMAIN]['devices'] + hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) + await hass.async_block_till_done() # ensure delete-port-mapping-methods called assert mock_device.removed_port_mappings == [8123]