IGD review fixes

homeassistant/components/upnp/__init__.py

@@ -16,7 +16,7 @@
 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 @@
 
 
 REQUIREMENTS = ['async-upnp-client==0.12.7']
-DEPENDENCIES = ['http']
 
 NOTIFICATION_ID = 'upnp_notification'
 NOTIFICATION_TITLE = 'UPnP/IGD Setup'
@@ -50,18 +49,37 @@
 }, 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
 

homeassistant/components/upnp/device.py

@@ -6,7 +6,6 @@
 
 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 @@ def name(self):
         """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

tests/components/upnp/test_init.py

@@ -9,6 +9,7 @@
 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 @@ async def _async_delete_port_mapping(self, external_port):
 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]