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

@@ -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,
+    ATTR_DISCOVERY_HASH, CONF_UNIQUE_ID, MqttDiscoveryUpdate, subscription)
-    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()

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,
+    ATTR_DISCOVERY_HASH, CONF_QOS, CONF_RETAIN, CONF_UNIQUE_ID,
-    CONF_UNIQUE_ID, MQTT_BASE_PLATFORM_SCHEMA, MqttAttributes,
+    MQTT_BASE_PLATFORM_SCHEMA, MqttAttributes, MqttAvailability,
-    MqttAvailability, MqttDiscoveryUpdate, MqttEntityDeviceInfo, subscription)
+    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)

homeassistant/components/mqtt/discovery.py

@@ -19,21 +19,30 @@ TOPIC_MATCHER = re.compile(
     r'(?:(?P<node_id>[a-zA-Z0-9_-]+)/)?(?P<object_id>[a-zA-Z0-9_-]+)/config')
 
 SUPPORTED_COMPONENTS = [
-    'binary_sensor', 'camera', 'cover', 'fan',
+    'alarm_control_panel',
-    'light', 'sensor', 'switch', 'lock', 'climate',
-    'alarm_control_panel', 'vacuum']
-
-CONFIG_ENTRY_COMPONENTS = [
     '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
 

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
+async def test_subscribing_config_topic(hass, mqtt_mock):
-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')
+async def test_invalid_topic(hass, mqtt_mock):
-@asyncio.coroutine
-def test_invalid_topic(mock_load_platform, hass, mqtt_mock):
     """Test sending to invalid topic."""
+    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)
+        await async_start(hass, 'homeassistant', {}, entry)
 
-    async_fire_mqtt_message(hass, 'homeassistant/binary_sensor/bla/not_config',
+        async_fire_mqtt_message(
-                            '{}')
+            hass, 'homeassistant/binary_sensor/bla/not_config', '{}')
-    yield from hass.async_block_till_done()
+        await hass.async_block_till_done()
         assert not mock_load_platform.called
 
 
-@patch('homeassistant.components.mqtt.discovery.async_load_platform')
+async def test_invalid_json(hass, mqtt_mock, caplog):
-@asyncio.coroutine
-def test_invalid_json(mock_load_platform, hass, mqtt_mock, caplog):
     """Test sending in invalid JSON."""
+    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)
+        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()
+        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')
+async def test_only_valid_components(hass, mqtt_mock, caplog):
-@asyncio.coroutine
-def test_only_valid_components(mock_load_platform, hass, mqtt_mock, caplog):
     """Test for a valid component."""
+    with patch('homeassistant.components.mqtt.discovery.async_load_platform')\
+            as mock_load_platform:
         entry = MockConfigEntry(domain=mqtt.DOMAIN)
 
         invalid_component = "timer"
 
         mock_load_platform.return_value = mock_coro()
-    yield from async_start(hass, 'homeassistant', {}, entry)
+        await async_start(hass, 'homeassistant', {}, entry)
 
         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
+async def test_correct_config_discovery(hass, mqtt_mock, caplog):
-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
+async def test_discover_fan(hass, mqtt_mock, caplog):
-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
+async def test_discover_climate(hass, mqtt_mock, caplog):
-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
+async def test_discover_alarm_control_panel(hass, mqtt_mock, caplog):
-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
+async def test_discovery_incl_nodeid(hass, mqtt_mock, caplog):
-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
+async def test_non_duplicate_discovery(hass, mqtt_mock, caplog):
-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
+async def test_discovery_expansion(hass, mqtt_mock, caplog):
-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()
+    await hass.async_block_till_done()
-    yield from 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'