From d99637e51bc37b22599973beb1a5f4501d76f5cd Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Sun, 14 Apr 2019 05:25:45 +0200 Subject: [PATCH] Deprecate implicit state_topic for MQTT discovery (#22998) * Deprecate implicit state_topic for MQTT discovery * Lint * Add comments * Modernize tests --- homeassistant/components/mqtt/camera.py | 7 +- homeassistant/components/mqtt/climate.py | 10 +- homeassistant/components/mqtt/discovery.py | 37 ++- tests/components/mqtt/test_discovery.py | 269 +++++++++++++++------ 4 files changed, 233 insertions(+), 90 deletions(-) diff --git a/homeassistant/components/mqtt/camera.py b/homeassistant/components/mqtt/camera.py index 0449bf79ca7..49679fc5583 100644 --- a/homeassistant/components/mqtt/camera.py +++ b/homeassistant/components/mqtt/camera.py @@ -14,8 +14,7 @@ from homeassistant.helpers.dispatcher import async_dispatcher_connect from homeassistant.helpers.typing import ConfigType, HomeAssistantType from . import ( - ATTR_DISCOVERY_HASH, CONF_STATE_TOPIC, CONF_UNIQUE_ID, MqttDiscoveryUpdate, - subscription) + ATTR_DISCOVERY_HASH, CONF_UNIQUE_ID, MqttDiscoveryUpdate, subscription) from .discovery import MQTT_DISCOVERY_NEW, clear_discovery_hash _LOGGER = logging.getLogger(__name__) @@ -42,8 +41,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): """Discover and add a MQTT camera.""" try: discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH) - # state_topic is implicitly set by MQTT discovery, remove it - discovery_payload.pop(CONF_STATE_TOPIC, None) config = PLATFORM_SCHEMA(discovery_payload) await _async_setup_entity(config, async_add_entities, discovery_hash) @@ -85,8 +82,6 @@ class MqttCamera(MqttDiscoveryUpdate, Camera): async def discovery_update(self, discovery_payload): """Handle updated discovery message.""" - # state_topic is implicitly set by MQTT discovery, remove it - discovery_payload.pop(CONF_STATE_TOPIC, None) config = PLATFORM_SCHEMA(discovery_payload) self._config = config await self._subscribe_topics() diff --git a/homeassistant/components/mqtt/climate.py b/homeassistant/components/mqtt/climate.py index 6a8c4d83995..fb443b29c61 100644 --- a/homeassistant/components/mqtt/climate.py +++ b/homeassistant/components/mqtt/climate.py @@ -24,9 +24,9 @@ from homeassistant.helpers.dispatcher import async_dispatcher_connect from homeassistant.helpers.typing import ConfigType, HomeAssistantType from . import ( - ATTR_DISCOVERY_HASH, CONF_QOS, CONF_RETAIN, CONF_STATE_TOPIC, - CONF_UNIQUE_ID, MQTT_BASE_PLATFORM_SCHEMA, MqttAttributes, - MqttAvailability, MqttDiscoveryUpdate, MqttEntityDeviceInfo, subscription) + ATTR_DISCOVERY_HASH, CONF_QOS, CONF_RETAIN, CONF_UNIQUE_ID, + MQTT_BASE_PLATFORM_SCHEMA, MqttAttributes, MqttAvailability, + MqttDiscoveryUpdate, MqttEntityDeviceInfo, subscription) from .discovery import MQTT_DISCOVERY_NEW, clear_discovery_hash _LOGGER = logging.getLogger(__name__) @@ -161,8 +161,6 @@ async def async_setup_entry(hass, config_entry, async_add_entities): """Discover and add a MQTT climate device.""" try: discovery_hash = discovery_payload.pop(ATTR_DISCOVERY_HASH) - # state_topic is implicitly set by MQTT discovery, remove it - discovery_payload.pop(CONF_STATE_TOPIC, None) config = PLATFORM_SCHEMA(discovery_payload) await _async_setup_entity(hass, config, async_add_entities, config_entry, discovery_hash) @@ -225,8 +223,6 @@ class MqttClimate(MqttAttributes, MqttAvailability, MqttDiscoveryUpdate, async def discovery_update(self, discovery_payload): """Handle updated discovery message.""" - # state_topic is implicitly set by MQTT discovery, remove it - discovery_payload.pop(CONF_STATE_TOPIC, None) config = PLATFORM_SCHEMA(discovery_payload) self._config = config self._setup_from_config(config) diff --git a/homeassistant/components/mqtt/discovery.py b/homeassistant/components/mqtt/discovery.py index 20b707eec17..4c1427d7e15 100644 --- a/homeassistant/components/mqtt/discovery.py +++ b/homeassistant/components/mqtt/discovery.py @@ -19,21 +19,30 @@ TOPIC_MATCHER = re.compile( r'(?:(?P[a-zA-Z0-9_-]+)/)?(?P[a-zA-Z0-9_-]+)/config') SUPPORTED_COMPONENTS = [ - 'binary_sensor', 'camera', 'cover', 'fan', - 'light', 'sensor', 'switch', 'lock', 'climate', - 'alarm_control_panel', 'vacuum'] - -CONFIG_ENTRY_COMPONENTS = [ + 'alarm_control_panel', 'binary_sensor', 'camera', + 'climate', 'cover', + 'fan', 'light', 'lock', 'sensor', 'switch', - 'climate', + 'vacuum', +] + +CONFIG_ENTRY_COMPONENTS = [ 'alarm_control_panel', + 'binary_sensor', + 'camera', + 'climate', + 'cover', 'fan', + 'light', + 'lock', + 'sensor', + 'switch', 'vacuum', ] @@ -44,6 +53,14 @@ DEPRECATED_PLATFORM_TO_SCHEMA = { } } +# These components require state_topic to be set. +# If not specified, infer state_topic from discovery topic. +IMPLICIT_STATE_TOPIC_COMPONENTS = [ + 'alarm_control_panel', + 'binary_sensor', + 'sensor', +] + ALREADY_DISCOVERED = 'mqtt_discovered_components' DATA_CONFIG_ENTRY_LOCK = 'mqtt_config_entry_lock' @@ -258,10 +275,16 @@ async def async_start(hass: HomeAssistantType, discovery_topic, hass_config, platform, schema) payload[CONF_PLATFORM] = 'mqtt' - if CONF_STATE_TOPIC not in payload: + if (CONF_STATE_TOPIC not in payload and + component in IMPLICIT_STATE_TOPIC_COMPONENTS): + # state_topic not specified, infer from discovery topic payload[CONF_STATE_TOPIC] = '{}/{}/{}{}/state'.format( discovery_topic, component, '%s/' % node_id if node_id else '', object_id) + _LOGGER.warning('implicit %s is deprecated, add "%s":"%s" to ' + '%s discovery message', + CONF_STATE_TOPIC, CONF_STATE_TOPIC, + payload[CONF_STATE_TOPIC], topic) payload[ATTR_DISCOVERY_HASH] = discovery_hash diff --git a/tests/components/mqtt/test_discovery.py b/tests/components/mqtt/test_discovery.py index ffc385021d7..ba72db52a8f 100644 --- a/tests/components/mqtt/test_discovery.py +++ b/tests/components/mqtt/test_discovery.py @@ -1,5 +1,4 @@ """The tests for the MQTT discovery.""" -import asyncio from unittest.mock import patch from homeassistant.components import mqtt @@ -10,8 +9,7 @@ from homeassistant.const import STATE_OFF, STATE_ON from tests.common import MockConfigEntry, async_fire_mqtt_message, mock_coro -@asyncio.coroutine -def test_subscribing_config_topic(hass, mqtt_mock): +async def test_subscribing_config_topic(hass, mqtt_mock): """Test setting up discovery.""" entry = MockConfigEntry(domain=mqtt.DOMAIN, data={ mqtt.CONF_BROKER: 'test-broker' @@ -19,7 +17,7 @@ def test_subscribing_config_topic(hass, mqtt_mock): hass_config = {} discovery_topic = 'homeassistant' - yield from async_start(hass, discovery_topic, hass_config, entry) + await async_start(hass, discovery_topic, hass_config, entry) assert mqtt_mock.async_subscribe.called call_args = mqtt_mock.async_subscribe.mock_calls[0][1] @@ -27,57 +25,57 @@ def test_subscribing_config_topic(hass, mqtt_mock): assert call_args[2] == 0 -@patch('homeassistant.components.mqtt.discovery.async_load_platform') -@asyncio.coroutine -def test_invalid_topic(mock_load_platform, hass, mqtt_mock): +async def test_invalid_topic(hass, mqtt_mock): """Test sending to invalid topic.""" - entry = MockConfigEntry(domain=mqtt.DOMAIN, data={ - mqtt.CONF_BROKER: 'test-broker' - }) + with patch('homeassistant.components.mqtt.discovery.async_load_platform')\ + as mock_load_platform: + entry = MockConfigEntry(domain=mqtt.DOMAIN, data={ + mqtt.CONF_BROKER: 'test-broker' + }) - mock_load_platform.return_value = mock_coro() - yield from async_start(hass, 'homeassistant', {}, entry) + mock_load_platform.return_value = mock_coro() + await async_start(hass, 'homeassistant', {}, entry) - async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/not_config', - '{}') - yield from hass.async_block_till_done() - assert not mock_load_platform.called + async_fire_mqtt_message( + hass, 'homeassistant/binary_sensor/bla/not_config', '{}') + await hass.async_block_till_done() + assert not mock_load_platform.called -@patch('homeassistant.components.mqtt.discovery.async_load_platform') -@asyncio.coroutine -def test_invalid_json(mock_load_platform, hass, mqtt_mock, caplog): +async def test_invalid_json(hass, mqtt_mock, caplog): """Test sending in invalid JSON.""" - entry = MockConfigEntry(domain=mqtt.DOMAIN, data={ - mqtt.CONF_BROKER: 'test-broker' - }) + with patch('homeassistant.components.mqtt.discovery.async_load_platform')\ + as mock_load_platform: + entry = MockConfigEntry(domain=mqtt.DOMAIN, data={ + mqtt.CONF_BROKER: 'test-broker' + }) - mock_load_platform.return_value = mock_coro() - yield from async_start(hass, 'homeassistant', {}, entry) + mock_load_platform.return_value = mock_coro() + await async_start(hass, 'homeassistant', {}, entry) - async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/config', - 'not json') - yield from hass.async_block_till_done() - assert 'Unable to parse JSON' in caplog.text - assert not mock_load_platform.called + async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/config', + 'not json') + await hass.async_block_till_done() + assert 'Unable to parse JSON' in caplog.text + assert not mock_load_platform.called -@patch('homeassistant.components.mqtt.discovery.async_load_platform') -@asyncio.coroutine -def test_only_valid_components(mock_load_platform, hass, mqtt_mock, caplog): +async def test_only_valid_components(hass, mqtt_mock, caplog): """Test for a valid component.""" - entry = MockConfigEntry(domain=mqtt.DOMAIN) + with patch('homeassistant.components.mqtt.discovery.async_load_platform')\ + as mock_load_platform: + entry = MockConfigEntry(domain=mqtt.DOMAIN) - invalid_component = "timer" + invalid_component = "timer" - mock_load_platform.return_value = mock_coro() - yield from async_start(hass, 'homeassistant', {}, entry) + mock_load_platform.return_value = mock_coro() + await async_start(hass, 'homeassistant', {}, entry) - async_fire_mqtt_message(hass, 'homeassistant/{}/bla/config'.format( - invalid_component - ), '{}') + async_fire_mqtt_message(hass, 'homeassistant/{}/bla/config'.format( + invalid_component + ), '{}') - yield from hass.async_block_till_done() + await hass.async_block_till_done() assert 'Component {} is not supported'.format( invalid_component @@ -86,16 +84,15 @@ def test_only_valid_components(mock_load_platform, hass, mqtt_mock, caplog): assert not mock_load_platform.called -@asyncio.coroutine -def test_correct_config_discovery(hass, mqtt_mock, caplog): +async def test_correct_config_discovery(hass, mqtt_mock, caplog): """Test sending in correct JSON.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/config', '{ "name": "Beer" }') - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('binary_sensor.beer') @@ -104,17 +101,16 @@ def test_correct_config_discovery(hass, mqtt_mock, caplog): assert ('binary_sensor', 'bla') in hass.data[ALREADY_DISCOVERED] -@asyncio.coroutine -def test_discover_fan(hass, mqtt_mock, caplog): +async def test_discover_fan(hass, mqtt_mock, caplog): """Test discovering an MQTT fan.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) async_fire_mqtt_message(hass, 'homeassistant/fan/bla/config', ('{ "name": "Beer",' ' "command_topic": "test_topic" }')) - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('fan.beer') @@ -123,12 +119,11 @@ def test_discover_fan(hass, mqtt_mock, caplog): assert ('fan', 'bla') in hass.data[ALREADY_DISCOVERED] -@asyncio.coroutine -def test_discover_climate(hass, mqtt_mock, caplog): +async def test_discover_climate(hass, mqtt_mock, caplog): """Test discovering an MQTT climate component.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) data = ( '{ "name": "ClimateTest",' @@ -137,7 +132,7 @@ def test_discover_climate(hass, mqtt_mock, caplog): ) async_fire_mqtt_message(hass, 'homeassistant/climate/bla/config', data) - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('climate.ClimateTest') @@ -146,12 +141,11 @@ def test_discover_climate(hass, mqtt_mock, caplog): assert ('climate', 'bla') in hass.data[ALREADY_DISCOVERED] -@asyncio.coroutine -def test_discover_alarm_control_panel(hass, mqtt_mock, caplog): +async def test_discover_alarm_control_panel(hass, mqtt_mock, caplog): """Test discovering an MQTT alarm control panel component.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) data = ( '{ "name": "AlarmControlPanelTest",' @@ -161,7 +155,7 @@ def test_discover_alarm_control_panel(hass, mqtt_mock, caplog): async_fire_mqtt_message( hass, 'homeassistant/alarm_control_panel/bla/config', data) - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('alarm_control_panel.AlarmControlPanelTest') @@ -170,16 +164,15 @@ def test_discover_alarm_control_panel(hass, mqtt_mock, caplog): assert ('alarm_control_panel', 'bla') in hass.data[ALREADY_DISCOVERED] -@asyncio.coroutine -def test_discovery_incl_nodeid(hass, mqtt_mock, caplog): +async def test_discovery_incl_nodeid(hass, mqtt_mock, caplog): """Test sending in correct JSON with optional node_id included.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/my_node_id/bla' '/config', '{ "name": "Beer" }') - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('binary_sensor.beer') @@ -188,18 +181,17 @@ def test_discovery_incl_nodeid(hass, mqtt_mock, caplog): assert ('binary_sensor', 'my_node_id bla') in hass.data[ALREADY_DISCOVERED] -@asyncio.coroutine -def test_non_duplicate_discovery(hass, mqtt_mock, caplog): +async def test_non_duplicate_discovery(hass, mqtt_mock, caplog): """Test for a non duplicate component.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/config', '{ "name": "Beer" }') async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/config', '{ "name": "Beer" }') - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('binary_sensor.beer') state_duplicate = hass.states.get('binary_sensor.beer1') @@ -211,12 +203,11 @@ def test_non_duplicate_discovery(hass, mqtt_mock, caplog): 'binary_sensor bla' in caplog.text -@asyncio.coroutine -def test_discovery_expansion(hass, mqtt_mock, caplog): +async def test_discovery_expansion(hass, mqtt_mock, caplog): """Test expansion of abbreviated discovery payload.""" entry = MockConfigEntry(domain=mqtt.DOMAIN) - yield from async_start(hass, 'homeassistant', {}, entry) + await async_start(hass, 'homeassistant', {}, entry) data = ( '{ "~": "some/base/topic",' @@ -235,7 +226,7 @@ def test_discovery_expansion(hass, mqtt_mock, caplog): async_fire_mqtt_message( hass, 'homeassistant/switch/bla/config', data) - yield from hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('switch.DiscoveryExpansionTest1') assert state is not None @@ -245,8 +236,146 @@ def test_discovery_expansion(hass, mqtt_mock, caplog): async_fire_mqtt_message(hass, 'test_topic/some/base/topic', 'ON') - yield from hass.async_block_till_done() - yield from hass.async_block_till_done() + await hass.async_block_till_done() + await hass.async_block_till_done() state = hass.states.get('switch.DiscoveryExpansionTest1') assert state.state == STATE_ON + + +async def test_implicit_state_topic_alarm(hass, mqtt_mock, caplog): + """Test implicit state topic for alarm_control_panel.""" + entry = MockConfigEntry(domain=mqtt.DOMAIN) + + await async_start(hass, 'homeassistant', {}, entry) + + data = ( + '{ "name": "Test1",' + ' "command_topic": "homeassistant/alarm_control_panel/bla/cmnd"' + '}' + ) + + async_fire_mqtt_message( + hass, 'homeassistant/alarm_control_panel/bla/config', data) + await hass.async_block_till_done() + assert ( + 'implicit state_topic is deprecated, add ' + '"state_topic":"homeassistant/alarm_control_panel/bla/state"' + in caplog.text) + + state = hass.states.get('alarm_control_panel.Test1') + assert state is not None + assert state.name == 'Test1' + assert ('alarm_control_panel', 'bla') in hass.data[ALREADY_DISCOVERED] + assert state.state == 'unknown' + + async_fire_mqtt_message( + hass, 'homeassistant/alarm_control_panel/bla/state', 'armed_away') + await hass.async_block_till_done() + await hass.async_block_till_done() + + state = hass.states.get('alarm_control_panel.Test1') + assert state.state == 'armed_away' + + +async def test_implicit_state_topic_binary_sensor(hass, mqtt_mock, caplog): + """Test implicit state topic for binary_sensor.""" + entry = MockConfigEntry(domain=mqtt.DOMAIN) + + await async_start(hass, 'homeassistant', {}, entry) + + data = ( + '{ "name": "Test1"' + '}' + ) + + async_fire_mqtt_message( + hass, 'homeassistant/binary_sensor/bla/config', data) + await hass.async_block_till_done() + assert ( + 'implicit state_topic is deprecated, add ' + '"state_topic":"homeassistant/binary_sensor/bla/state"' + in caplog.text) + + state = hass.states.get('binary_sensor.Test1') + assert state is not None + assert state.name == 'Test1' + assert ('binary_sensor', 'bla') in hass.data[ALREADY_DISCOVERED] + assert state.state == 'off' + + async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/state', + 'ON') + await hass.async_block_till_done() + await hass.async_block_till_done() + + state = hass.states.get('binary_sensor.Test1') + assert state.state == 'on' + + +async def test_implicit_state_topic_sensor(hass, mqtt_mock, caplog): + """Test implicit state topic for sensor.""" + entry = MockConfigEntry(domain=mqtt.DOMAIN) + + await async_start(hass, 'homeassistant', {}, entry) + + data = ( + '{ "name": "Test1"' + '}' + ) + + async_fire_mqtt_message( + hass, 'homeassistant/sensor/bla/config', data) + await hass.async_block_till_done() + assert ( + 'implicit state_topic is deprecated, add ' + '"state_topic":"homeassistant/sensor/bla/state"' + in caplog.text) + + state = hass.states.get('sensor.Test1') + assert state is not None + assert state.name == 'Test1' + assert ('sensor', 'bla') in hass.data[ALREADY_DISCOVERED] + assert state.state == 'unknown' + + async_fire_mqtt_message(hass, 'homeassistant/sensor/bla/state', + '1234') + await hass.async_block_till_done() + await hass.async_block_till_done() + + state = hass.states.get('sensor.Test1') + assert state.state == '1234' + + +async def test_no_implicit_state_topic_switch(hass, mqtt_mock, caplog): + """Test no implicit state topic for switch.""" + entry = MockConfigEntry(domain=mqtt.DOMAIN) + + await async_start(hass, 'homeassistant', {}, entry) + + data = ( + '{ "name": "Test1",' + ' "command_topic": "cmnd"' + '}' + ) + + async_fire_mqtt_message( + hass, 'homeassistant/switch/bla/config', data) + await hass.async_block_till_done() + assert ( + 'implicit state_topic is deprecated' + not in caplog.text) + + state = hass.states.get('switch.Test1') + assert state is not None + assert state.name == 'Test1' + assert ('switch', 'bla') in hass.data[ALREADY_DISCOVERED] + assert state.state == 'off' + assert state.attributes['assumed_state'] is True + + async_fire_mqtt_message(hass, 'homeassistant/switch/bla/state', + 'ON') + await hass.async_block_till_done() + await hass.async_block_till_done() + + state = hass.states.get('switch.Test1') + assert state.state == 'off'