From 9d0a252ca73ea1b002bf13c7d77f652d5542794b Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Fri, 22 Jul 2022 13:36:43 +0200 Subject: [PATCH] Improve handling of MQTT config entry data (#72691) * Improve handling of MQTT config entry data * Add test * Add warning * Adjust tests --- homeassistant/components/mqtt/__init__.py | 31 ++++++ tests/components/mqtt/test_diagnostics.py | 2 +- tests/components/mqtt/test_discovery.py | 18 +++- tests/components/mqtt/test_init.py | 122 ++++++++++++---------- tests/conftest.py | 31 ++++-- 5 files changed, 135 insertions(+), 69 deletions(-) diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index 394e4af3e2e..2bea1a593d1 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -107,6 +107,16 @@ CONNECTION_SUCCESS = "connection_success" CONNECTION_FAILED = "connection_failed" CONNECTION_FAILED_RECOVERABLE = "connection_failed_recoverable" +CONFIG_ENTRY_CONFIG_KEYS = [ + CONF_BIRTH_MESSAGE, + CONF_BROKER, + CONF_DISCOVERY, + CONF_PASSWORD, + CONF_PORT, + CONF_USERNAME, + CONF_WILL_MESSAGE, +] + CONFIG_SCHEMA = vol.Schema( { DOMAIN: vol.All( @@ -185,6 +195,23 @@ async def async_setup(hass: HomeAssistant, config: ConfigType) -> bool: return True +def _filter_entry_config(hass: HomeAssistant, entry: ConfigEntry) -> None: + """Remove unknown keys from config entry data. + + Extra keys may have been added when importing MQTT yaml configuration. + """ + filtered_data = { + k: entry.data[k] for k in CONFIG_ENTRY_CONFIG_KEYS if k in entry.data + } + if entry.data.keys() != filtered_data.keys(): + _LOGGER.warning( + "The following unsupported configuration options were removed from the " + "MQTT config entry: %s. Add them to configuration.yaml if they are needed", + entry.data.keys() - filtered_data.keys(), + ) + hass.config_entries.async_update_entry(entry, data=filtered_data) + + def _merge_basic_config( hass: HomeAssistant, entry: ConfigEntry, yaml_config: dict[str, Any] ) -> None: @@ -243,6 +270,10 @@ async def async_fetch_config(hass: HomeAssistant, entry: ConfigEntry) -> dict | mqtt_config = CONFIG_SCHEMA_BASE(hass_config.get(DOMAIN, {})) hass.data[DATA_MQTT_CONFIG] = mqtt_config + # Remove unknown keys from config entry data + _filter_entry_config(hass, entry) + + # Merge basic configuration, and add missing defaults for basic options _merge_basic_config(hass, entry, hass.data.get(DATA_MQTT_CONFIG, {})) # Bail out if broker setting is missing if CONF_BROKER not in entry.data: diff --git a/tests/components/mqtt/test_diagnostics.py b/tests/components/mqtt/test_diagnostics.py index 8cc5d0b1070..7c486f9af74 100644 --- a/tests/components/mqtt/test_diagnostics.py +++ b/tests/components/mqtt/test_diagnostics.py @@ -158,7 +158,7 @@ async def test_entry_diagnostics( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", diff --git a/tests/components/mqtt/test_discovery.py b/tests/components/mqtt/test_discovery.py index d185d3334d0..e4c6f44883a 100644 --- a/tests/components/mqtt/test_discovery.py +++ b/tests/components/mqtt/test_discovery.py @@ -46,7 +46,7 @@ def entity_reg(hass): @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_DISCOVERY: False}], ) async def test_subscribing_config_topic(hass, mqtt_mock_entry_no_yaml_config): @@ -1238,19 +1238,27 @@ async def test_no_implicit_state_topic_switch( @patch("homeassistant.components.mqtt.PLATFORMS", [Platform.BINARY_SENSOR]) @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", - mqtt.CONF_DISCOVERY_PREFIX: "my_home/homeassistant/register", } ], ) async def test_complex_discovery_topic_prefix( - hass, mqtt_mock_entry_no_yaml_config, caplog + hass, mqtt_mock_entry_with_yaml_config, caplog ): """Tests handling of discovery topic prefix with multiple slashes.""" - await mqtt_mock_entry_no_yaml_config() + assert await async_setup_component( + hass, + mqtt.DOMAIN, + { + mqtt.DOMAIN: { + mqtt.CONF_DISCOVERY_PREFIX: "my_home/homeassistant/register", + } + }, + ) + async_fire_mqtt_message( hass, ("my_home/homeassistant/register/binary_sensor/node1/object1/config"), diff --git a/tests/components/mqtt/test_init.py b/tests/components/mqtt/test_init.py index cec6038ae04..1f263d40fc2 100644 --- a/tests/components/mqtt/test_init.py +++ b/tests/components/mqtt/test_init.py @@ -1249,7 +1249,7 @@ async def test_unsubscribe_race(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_ @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_DISCOVERY: False}], ) async def test_restore_subscriptions_on_reconnect( @@ -1272,7 +1272,7 @@ async def test_restore_subscriptions_on_reconnect( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_DISCOVERY: False}], ) async def test_restore_all_active_subscriptions_on_reconnect( @@ -1486,19 +1486,19 @@ async def test_setup_manual_mqtt_empty_platform(hass, caplog): @patch("homeassistant.components.mqtt.PLATFORMS", []) -async def test_setup_mqtt_client_protocol(hass): +async def test_setup_mqtt_client_protocol(hass, mqtt_mock_entry_with_yaml_config): """Test MQTT client protocol setup.""" - entry = MockConfigEntry( - domain=mqtt.DOMAIN, - data={ - mqtt.CONF_BROKER: "test-broker", - mqtt.config_integration.CONF_PROTOCOL: "3.1", - }, - ) - entry.add_to_hass(hass) with patch("paho.mqtt.client.Client") as mock_client: + assert await async_setup_component( + hass, + mqtt.DOMAIN, + { + mqtt.DOMAIN: { + mqtt.config_integration.CONF_PROTOCOL: "3.1", + } + }, + ) mock_client.on_connect(return_value=0) - assert await mqtt.async_setup_entry(hass, entry) await hass.async_block_till_done() # check if protocol setup was correctly @@ -1563,9 +1563,17 @@ async def test_setup_raises_ConfigEntryNotReady_if_no_connect_broker(hass, caplo assert "Failed to connect to MQTT server due to exception:" in caplog.text -@pytest.mark.parametrize("insecure", [None, False, True]) +@pytest.mark.parametrize( + "config, insecure_param", + [ + ({"certificate": "auto"}, "not set"), + ({"certificate": "auto", "tls_insecure": False}, False), + ({"certificate": "auto", "tls_insecure": True}, True), + ], +) +@patch("homeassistant.components.mqtt.PLATFORMS", []) async def test_setup_uses_certificate_on_certificate_set_to_auto_and_insecure( - hass, insecure + hass, config, insecure_param, mqtt_mock_entry_with_yaml_config ): """Test setup uses bundled certs when certificate is set to auto and insecure.""" calls = [] @@ -1577,18 +1585,14 @@ async def test_setup_uses_certificate_on_certificate_set_to_auto_and_insecure( def mock_tls_insecure_set(insecure_param): insecure_check["insecure"] = insecure_param - config_item_data = {mqtt.CONF_BROKER: "test-broker", "certificate": "auto"} - if insecure is not None: - config_item_data["tls_insecure"] = insecure with patch("paho.mqtt.client.Client") as mock_client: mock_client().tls_set = mock_tls_set mock_client().tls_insecure_set = mock_tls_insecure_set - entry = MockConfigEntry( - domain=mqtt.DOMAIN, - data=config_item_data, + assert await async_setup_component( + hass, + mqtt.DOMAIN, + {mqtt.DOMAIN: config}, ) - entry.add_to_hass(hass) - assert await mqtt.async_setup_entry(hass, entry) await hass.async_block_till_done() assert calls @@ -1596,19 +1600,14 @@ async def test_setup_uses_certificate_on_certificate_set_to_auto_and_insecure( import certifi expectedCertificate = certifi.where() - # assert mock_mqtt.mock_calls[0][1][2]["certificate"] == expectedCertificate assert calls[0][0] == expectedCertificate # test if insecure is set - assert ( - insecure_check["insecure"] == insecure - if insecure is not None - else insecure_check["insecure"] == "not set" - ) + assert insecure_check["insecure"] == insecure_param -async def test_setup_without_tls_config_uses_tlsv1_under_python36(hass): - """Test setup defaults to TLSv1 under python3.6.""" +async def test_tls_version(hass, mqtt_mock_entry_with_yaml_config): + """Test setup defaults for tls.""" calls = [] def mock_tls_set(certificate, certfile=None, keyfile=None, tls_version=None): @@ -1616,28 +1615,19 @@ async def test_setup_without_tls_config_uses_tlsv1_under_python36(hass): with patch("paho.mqtt.client.Client") as mock_client: mock_client().tls_set = mock_tls_set - entry = MockConfigEntry( - domain=mqtt.DOMAIN, - data={"certificate": "auto", mqtt.CONF_BROKER: "test-broker"}, + assert await async_setup_component( + hass, + mqtt.DOMAIN, + {mqtt.DOMAIN: {"certificate": "auto"}}, ) - entry.add_to_hass(hass) - assert await mqtt.async_setup_entry(hass, entry) await hass.async_block_till_done() assert calls - - import sys - - if sys.hexversion >= 0x03060000: - expectedTlsVersion = ssl.PROTOCOL_TLS # pylint: disable=no-member - else: - expectedTlsVersion = ssl.PROTOCOL_TLSv1 - - assert calls[0][3] == expectedTlsVersion + assert calls[0][3] == ssl.PROTOCOL_TLS @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", @@ -1670,7 +1660,7 @@ async def test_custom_birth_message( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", @@ -1705,7 +1695,7 @@ async def test_default_birth_message( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_BIRTH_MESSAGE: {}}], ) async def test_no_birth_message(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_config): @@ -1719,7 +1709,7 @@ async def test_no_birth_message(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_ @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", @@ -1733,7 +1723,7 @@ async def test_no_birth_message(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_ ], ) async def test_delayed_birth_message( - hass, mqtt_client_mock, mqtt_config, mqtt_mock_entry_no_yaml_config + hass, mqtt_client_mock, mqtt_config_entry_data, mqtt_mock_entry_no_yaml_config ): """Test sending birth message does not happen until Home Assistant starts.""" mqtt_mock = await mqtt_mock_entry_no_yaml_config() @@ -1743,7 +1733,7 @@ async def test_delayed_birth_message( await hass.async_block_till_done() - entry = MockConfigEntry(domain=mqtt.DOMAIN, data=mqtt_config) + entry = MockConfigEntry(domain=mqtt.DOMAIN, data=mqtt_config_entry_data) entry.add_to_hass(hass) assert await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() @@ -1780,7 +1770,7 @@ async def test_delayed_birth_message( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", @@ -1816,7 +1806,7 @@ async def test_default_will_message( @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [{mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_WILL_MESSAGE: {}}], ) async def test_no_will_message(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_config): @@ -1827,7 +1817,7 @@ async def test_no_will_message(hass, mqtt_client_mock, mqtt_mock_entry_no_yaml_c @pytest.mark.parametrize( - "mqtt_config", + "mqtt_config_entry_data", [ { mqtt.CONF_BROKER: "mock-broker", @@ -2922,3 +2912,29 @@ async def test_setup_manual_items_with_unique_ids( assert hass.states.get("light.test1") is not None assert (hass.states.get("light.test2") is not None) == unique assert bool("Platform mqtt does not generate unique IDs." in caplog.text) != unique + + +async def test_remove_unknown_conf_entry_options(hass, mqtt_client_mock, caplog): + """Test unknown keys in config entry data is removed.""" + mqtt_config_entry_data = { + mqtt.CONF_BROKER: "mock-broker", + mqtt.CONF_BIRTH_MESSAGE: {}, + mqtt.client.CONF_PROTOCOL: mqtt.const.PROTOCOL_311, + } + + entry = MockConfigEntry( + data=mqtt_config_entry_data, + domain=mqtt.DOMAIN, + title="MQTT", + ) + + entry.add_to_hass(hass) + assert await hass.config_entries.async_setup(entry.entry_id) + await hass.async_block_till_done() + + assert mqtt.client.CONF_PROTOCOL not in entry.data + assert ( + "The following unsupported configuration options were removed from the " + "MQTT config entry: {'protocol'}. Add them to configuration.yaml if they " + "are needed" + ) in caplog.text diff --git a/tests/conftest.py b/tests/conftest.py index ee2e903e88d..dc5f3069332 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -501,7 +501,7 @@ def fail_on_log_exception(request, monkeypatch): @pytest.fixture -def mqtt_config(): +def mqtt_config_entry_data(): """Fixture to allow overriding MQTT config.""" return None @@ -553,7 +553,7 @@ def mqtt_client_mock(hass): async def mqtt_mock( hass, mqtt_client_mock, - mqtt_config, + mqtt_config_entry_data, mqtt_mock_entry_no_yaml_config, ): """Fixture to mock MQTT component.""" @@ -561,15 +561,18 @@ async def mqtt_mock( @asynccontextmanager -async def _mqtt_mock_entry(hass, mqtt_client_mock, mqtt_config): +async def _mqtt_mock_entry(hass, mqtt_client_mock, mqtt_config_entry_data): """Fixture to mock a delayed setup of the MQTT config entry.""" - if mqtt_config is None: - mqtt_config = {mqtt.CONF_BROKER: "mock-broker", mqtt.CONF_BIRTH_MESSAGE: {}} + if mqtt_config_entry_data is None: + mqtt_config_entry_data = { + mqtt.CONF_BROKER: "mock-broker", + mqtt.CONF_BIRTH_MESSAGE: {}, + } await hass.async_block_till_done() entry = MockConfigEntry( - data=mqtt_config, + data=mqtt_config_entry_data, domain=mqtt.DOMAIN, title="MQTT", ) @@ -613,7 +616,9 @@ async def _mqtt_mock_entry(hass, mqtt_client_mock, mqtt_config): @pytest.fixture -async def mqtt_mock_entry_no_yaml_config(hass, mqtt_client_mock, mqtt_config): +async def mqtt_mock_entry_no_yaml_config( + hass, mqtt_client_mock, mqtt_config_entry_data +): """Set up an MQTT config entry without MQTT yaml config.""" async def _async_setup_config_entry(hass, entry): @@ -626,12 +631,16 @@ async def mqtt_mock_entry_no_yaml_config(hass, mqtt_client_mock, mqtt_config): """Set up the MQTT config entry.""" return await mqtt_mock_entry(_async_setup_config_entry) - async with _mqtt_mock_entry(hass, mqtt_client_mock, mqtt_config) as mqtt_mock_entry: + async with _mqtt_mock_entry( + hass, mqtt_client_mock, mqtt_config_entry_data + ) as mqtt_mock_entry: yield _setup_mqtt_entry @pytest.fixture -async def mqtt_mock_entry_with_yaml_config(hass, mqtt_client_mock, mqtt_config): +async def mqtt_mock_entry_with_yaml_config( + hass, mqtt_client_mock, mqtt_config_entry_data +): """Set up an MQTT config entry with MQTT yaml config.""" async def _async_do_not_setup_config_entry(hass, entry): @@ -642,7 +651,9 @@ async def mqtt_mock_entry_with_yaml_config(hass, mqtt_client_mock, mqtt_config): """Set up the MQTT config entry.""" return await mqtt_mock_entry(_async_do_not_setup_config_entry) - async with _mqtt_mock_entry(hass, mqtt_client_mock, mqtt_config) as mqtt_mock_entry: + async with _mqtt_mock_entry( + hass, mqtt_client_mock, mqtt_config_entry_data + ) as mqtt_mock_entry: yield _setup_mqtt_entry