Rework MQTT config merging and adding defaults

homeassistant/components/mqtt/__init__.py

@@ -24,7 +24,7 @@
     SERVICE_RELOAD,
 )
 from homeassistant.core import HassJob, HomeAssistant, ServiceCall, callback
-from homeassistant.exceptions import TemplateError, Unauthorized
+from homeassistant.exceptions import ConfigEntryError, TemplateError, Unauthorized
 from homeassistant.helpers import config_validation as cv, event, template
 from homeassistant.helpers.device_registry import DeviceEntry
 from homeassistant.helpers.dispatcher import async_dispatcher_connect
@@ -45,11 +45,7 @@
     publish,
     subscribe,
 )
-from .config_integration import (
-    CONFIG_SCHEMA_ENTRY,
-    DEFAULT_VALUES,
-    PLATFORM_CONFIG_SCHEMA_BASE,
-)
+from .config_integration import CONFIG_SCHEMA_ENTRY, PLATFORM_CONFIG_SCHEMA_BASE
 from .const import (  # noqa: F401
     ATTR_PAYLOAD,
     ATTR_QOS,
@@ -102,8 +98,6 @@
 SERVICE_PUBLISH = "publish"
 SERVICE_DUMP = "dump"
 
-MANDATORY_DEFAULT_VALUES = (CONF_PORT, CONF_DISCOVERY_PREFIX)
-
 ATTR_TOPIC_TEMPLATE = "topic_template"
 ATTR_PAYLOAD_TEMPLATE = "payload_template"
 
@@ -193,50 +187,6 @@ 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"
-            ),
-            entry.data.keys() - filtered_data.keys(),
-        )
-        hass.config_entries.async_update_entry(entry, data=filtered_data)
-
-
-async def _async_auto_mend_config(
-    hass: HomeAssistant, entry: ConfigEntry, yaml_config: dict[str, Any]
-) -> None:
-    """Mends config fetched from config entry and adds missing values.
-
-    This mends incomplete migration from old version of HA Core.
-    """
-    entry_updated = False
-    entry_config = {**entry.data}
-    for key in MANDATORY_DEFAULT_VALUES:
-        if key not in entry_config:
-            entry_config[key] = DEFAULT_VALUES[key]
-            entry_updated = True
-
-    if entry_updated:
-        hass.config_entries.async_update_entry(entry, data=entry_config)
-
-
-def _merge_extended_config(entry: ConfigEntry, conf: ConfigType) -> dict[str, Any]:
-    """Merge advanced options in configuration.yaml config with config entry."""
-    # Add default values
-    conf = {**DEFAULT_VALUES, **conf}
-    return {**conf, **entry.data}
-
-
 async def _async_config_entry_updated(hass: HomeAssistant, entry: ConfigEntry) -> None:
     """Handle signals of config entry being updated.
 
@@ -245,42 +195,28 @@ async def _async_config_entry_updated(hass: HomeAssistant, entry: ConfigEntry) -
     await hass.config_entries.async_reload(entry.entry_id)
 
 
-async def async_fetch_config(
-    hass: HomeAssistant, entry: ConfigEntry
-) -> dict[str, Any] | None:
+async def async_fetch_config(hass: HomeAssistant, entry: ConfigEntry) -> dict[str, Any]:
     """Fetch fresh MQTT yaml config from the hass config."""
     mqtt_data = get_mqtt_data(hass)
     hass_config = await conf_util.async_hass_config_yaml(hass)
     mqtt_data.config = PLATFORM_CONFIG_SCHEMA_BASE(hass_config.get(DOMAIN, {}))
+    try:
+        entry_config = CONFIG_SCHEMA_ENTRY(dict(entry.data))
+    except vol.MultipleInvalid as ex:
+        raise ConfigEntryError(
+            f"The MQTT config entry is invalid, please correct it: {ex}"
+        ) from ex
 
-    # Remove unknown keys from config entry data
-    _filter_entry_config(hass, entry)
-
-    # Add missing defaults to migrate older config entries
-    await _async_auto_mend_config(hass, entry, mqtt_data.config or {})
-    # Bail out if broker setting is missing
-    if CONF_BROKER not in entry.data:
-        _LOGGER.error("MQTT broker is not configured, please configure it")
-        return None
-
-    # If user doesn't have configuration.yaml config, generate default values
-    # for options not in config entry data
-    if (conf := mqtt_data.config) is None:
-        conf = CONFIG_SCHEMA_ENTRY(dict(entry.data))
-
-    # Merge advanced configuration values from configuration.yaml
-    conf = _merge_extended_config(entry, conf)
-    return conf
+    # Merge yaml config for manual MQTT items with entry data
+    return {**mqtt_data.config, **entry_config}
 
 
 async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool:
     """Load a config entry."""
     mqtt_data = get_mqtt_data(hass, True)
 
-    # Fetch configuration and add missing defaults for basic options
-    if (conf := await async_fetch_config(hass, entry)) is None:
-        # Bail out
-        return False
+    # Fetch configuration and add default values
+    conf = await async_fetch_config(hass, entry)
 
     await async_create_certificate_temp_files(hass, dict(entry.data))
     mqtt_data.client = MQTT(hass, entry, conf)

homeassistant/components/mqtt/config_integration.py

@@ -65,17 +65,6 @@
 
 DEFAULT_TLS_PROTOCOL = "auto"
 
-DEFAULT_VALUES = {
-    CONF_BIRTH_MESSAGE: DEFAULT_BIRTH,
-    CONF_DISCOVERY: DEFAULT_DISCOVERY,
-    CONF_DISCOVERY_PREFIX: DEFAULT_PREFIX,
-    CONF_PORT: DEFAULT_PORT,
-    CONF_PROTOCOL: DEFAULT_PROTOCOL,
-    CONF_TRANSPORT: DEFAULT_TRANSPORT,
-    CONF_WILL_MESSAGE: DEFAULT_WILL,
-    CONF_KEEPALIVE: DEFAULT_KEEPALIVE,
-}
-
 PLATFORM_CONFIG_SCHEMA_BASE = vol.Schema(
     {
         Platform.ALARM_CONTROL_PANEL.value: vol.All(
@@ -169,9 +158,11 @@
 CONFIG_SCHEMA_ENTRY = vol.Schema(
     {
         vol.Optional(CONF_CLIENT_ID): cv.string,
-        vol.Optional(CONF_KEEPALIVE): vol.All(vol.Coerce(int), vol.Range(min=15)),
-        vol.Optional(CONF_BROKER): cv.string,
-        vol.Optional(CONF_PORT): cv.port,
+        vol.Optional(CONF_KEEPALIVE, default=DEFAULT_KEEPALIVE): vol.All(
+            vol.Coerce(int), vol.Range(min=15)
+        ),
+        vol.Required(CONF_BROKER): cv.string,
+        vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port,
         vol.Optional(CONF_USERNAME): cv.string,
         vol.Optional(CONF_PASSWORD): cv.string,
         vol.Optional(CONF_CERTIFICATE): str,
@@ -180,13 +171,17 @@
             CONF_CLIENT_CERT, "client_key_auth", msg=CLIENT_KEY_AUTH_MSG
         ): str,
         vol.Optional(CONF_TLS_INSECURE): cv.boolean,
-        vol.Optional(CONF_PROTOCOL): vol.All(cv.string, vol.In(SUPPORTED_PROTOCOLS)),
-        vol.Optional(CONF_WILL_MESSAGE): valid_birth_will,
-        vol.Optional(CONF_BIRTH_MESSAGE): valid_birth_will,
-        vol.Optional(CONF_DISCOVERY): cv.boolean,
+        vol.Optional(CONF_PROTOCOL, default=DEFAULT_PROTOCOL): vol.All(
+            cv.string, vol.In(SUPPORTED_PROTOCOLS)
+        ),
+        vol.Optional(CONF_WILL_MESSAGE, default=DEFAULT_WILL): valid_birth_will,
+        vol.Optional(CONF_BIRTH_MESSAGE, default=DEFAULT_BIRTH): valid_birth_will,
+        vol.Optional(CONF_DISCOVERY, default=DEFAULT_DISCOVERY): cv.boolean,
         # discovery_prefix must be a valid publish topic because if no
         # state topic is specified, it will be created with the given prefix.
-        vol.Optional(CONF_DISCOVERY_PREFIX): valid_publish_topic,
+        vol.Optional(
+            CONF_DISCOVERY_PREFIX, default=DEFAULT_PREFIX
+        ): valid_publish_topic,
         vol.Optional(CONF_TRANSPORT, default=DEFAULT_TRANSPORT): vol.All(
             cv.string, vol.In([TRANSPORT_TCP, TRANSPORT_WEBSOCKETS])
         ),
@@ -195,32 +190,6 @@
     }
 )
 
-CONFIG_SCHEMA_BASE = PLATFORM_CONFIG_SCHEMA_BASE.extend(
-    {
-        vol.Optional(CONF_CLIENT_ID): cv.string,
-        vol.Optional(CONF_KEEPALIVE): vol.All(vol.Coerce(int), vol.Range(min=15)),
-        vol.Optional(CONF_BROKER): cv.string,
-        vol.Optional(CONF_PORT): cv.port,
-        vol.Optional(CONF_USERNAME): cv.string,
-        vol.Optional(CONF_PASSWORD): cv.string,
-        vol.Optional(CONF_CERTIFICATE): vol.Any("auto", cv.isfile),
-        vol.Inclusive(
-            CONF_CLIENT_KEY, "client_key_auth", msg=CLIENT_KEY_AUTH_MSG
-        ): cv.isfile,
-        vol.Inclusive(
-            CONF_CLIENT_CERT, "client_key_auth", msg=CLIENT_KEY_AUTH_MSG
-        ): cv.isfile,
-        vol.Optional(CONF_TLS_INSECURE): cv.boolean,
-        vol.Optional(CONF_PROTOCOL): vol.All(cv.string, vol.In(SUPPORTED_PROTOCOLS)),
-        vol.Optional(CONF_WILL_MESSAGE): valid_birth_will,
-        vol.Optional(CONF_BIRTH_MESSAGE): valid_birth_will,
-        vol.Optional(CONF_DISCOVERY): cv.boolean,
-        # discovery_prefix must be a valid publish topic because if no
-        # state topic is specified, it will be created with the given prefix.
-        vol.Optional(CONF_DISCOVERY_PREFIX): valid_publish_topic,
-    }
-)
-
 DEPRECATED_CONFIG_KEYS = [
     CONF_BIRTH_MESSAGE,
     CONF_BROKER,

homeassistant/components/mqtt/models.py

@@ -289,7 +289,7 @@ class MqttData:
     """Keep the MQTT entry data."""
 
     client: MQTT | None = None
-    config: ConfigType | None = None
+    config: ConfigType = field(default_factory=dict)
     debug_info_entities: dict[str, EntityDebugInfo] = field(default_factory=dict)
     debug_info_triggers: dict[tuple[str, str], TriggerDebugInfo] = field(
         default_factory=dict

tests/components/mqtt/test_config_flow.py

@@ -183,15 +183,14 @@ async def test_user_connection_works(
     assert result["type"] == "form"
 
     result = await hass.config_entries.flow.async_configure(
-        result["flow_id"], {"broker": "127.0.0.1", "advanced_options": False}
+        result["flow_id"], {"broker": "127.0.0.1"}
     )
 
     assert result["type"] == "create_entry"
     assert result["result"].data == {
         "broker": "127.0.0.1",
         "port": 1883,
         "discovery": True,
-        "discovery_prefix": "homeassistant",
     }
     # Check we tried the connection
     assert len(mock_try_connection.mock_calls) == 1
@@ -231,7 +230,6 @@ async def test_user_v5_connection_works(
     assert result["result"].data == {
         "broker": "another-broker",
         "discovery": True,
-        "discovery_prefix": "homeassistant",
         "port": 2345,
         "protocol": "5",
     }
@@ -283,15 +281,14 @@ async def test_manual_config_set(
     assert result["type"] == "form"
 
     result = await hass.config_entries.flow.async_configure(
-        result["flow_id"], {"broker": "127.0.0.1"}
+        result["flow_id"], {"broker": "127.0.0.1", "port": "1883"}
     )
 
     assert result["type"] == "create_entry"
     assert result["result"].data == {
         "broker": "127.0.0.1",
         "port": 1883,
         "discovery": True,
-        "discovery_prefix": "homeassistant",
     }
     # Check we tried the connection, with precedence for config entry settings
     mock_try_connection.assert_called_once_with(
@@ -395,7 +392,6 @@ async def test_hassio_confirm(
         "username": "mock-user",
         "password": "mock-pass",
         "discovery": True,
-        "discovery_prefix": "homeassistant",
     }
     # Check we tried the connection
     assert len(mock_try_connection_success.mock_calls)

tests/components/mqtt/test_diagnostics.py

@@ -31,6 +31,8 @@
         "retain": False,
         "topic": "homeassistant/status",
     },
+    "ws_headers": {},
+    "ws_path": "/",
 }
 
 
@@ -265,6 +267,7 @@ async def test_redact_diagnostics(
         "name_by_user": None,
     }
 
+    await get_diagnostics_for_config_entry(hass, hass_client, config_entry)
     assert await get_diagnostics_for_config_entry(hass, hass_client, config_entry) == {
         "connected": True,
         "devices": [expected_device],

tests/components/mqtt/test_init.py

@@ -2256,34 +2256,29 @@ async def test_mqtt_subscribes_topics_on_connect(
     mqtt_client_mock.subscribe.assert_any_call("still/pending", 1)
 
 
-async def test_update_incomplete_entry(
+async def test_default_entry_setting_are_applied(
     hass: HomeAssistant,
     device_registry: dr.DeviceRegistry,
     mqtt_mock_entry_no_yaml_config: MqttMockHAClientGenerator,
     mqtt_client_mock: MqttMockPahoClient,
     caplog: pytest.LogCaptureFixture,
 ) -> None:
-    """Test if the MQTT component loads when config entry data is incomplete."""
+    """Test if the MQTT component loads when config entry data not has all default settings."""
     data = (
         '{ "device":{"identifiers":["0AFFD2"]},'
         '  "state_topic": "foobar/sensor",'
         '  "unique_id": "unique" }'
     )
 
-    # Config entry data is incomplete
+    # Config entry data is incomplete but valid according the schema
     entry = hass.config_entries.async_entries(mqtt.DOMAIN)[0]
     entry.data = {"broker": "test-broker", "port": 1234}
     await mqtt_mock_entry_no_yaml_config()
     await hass.async_block_till_done()
 
-    # Config entry data should now be updated
-    assert dict(entry.data) == {
-        "broker": "test-broker",
-        "port": 1234,
-        "discovery_prefix": "homeassistant",
-    }
-
     # Discover a device to verify the entry was setup correctly
+    # The discovery prefix should be the default
+    # And that the default settings were merged
     async_fire_mqtt_message(hass, "homeassistant/sensor/bla/config", data)
     await hass.async_block_till_done()
 
@@ -2297,12 +2292,15 @@ async def test_fail_no_broker(
     mqtt_client_mock: MqttMockPahoClient,
     caplog: pytest.LogCaptureFixture,
 ) -> None:
-    """Test if the MQTT component loads when broker configuration is missing."""
+    """Test the MQTT entry setup when broker configuration is missing."""
     # Config entry data is incomplete
     entry = MockConfigEntry(domain=mqtt.DOMAIN, data={})
     entry.add_to_hass(hass)
     assert not await hass.config_entries.async_setup(entry.entry_id)
-    assert "MQTT broker is not configured, please configure it" in caplog.text
+    assert (
+        "The MQTT config entry is invalid, please correct it: required key not provided @ data['broker']"
+        in caplog.text
+    )
 
 
 @pytest.mark.no_fail_on_log_exception
@@ -3312,7 +3310,7 @@ async def test_setup_manual_items_with_unique_ids(
     assert bool("Platform mqtt does not generate unique IDs." in caplog.text) != unique
 
 
-async def test_remove_unknown_conf_entry_options(
+async def test_fail_with_unknown_conf_entry_options(
     hass: HomeAssistant,
     mqtt_client_mock: MqttMockPahoClient,
     caplog: pytest.LogCaptureFixture,
@@ -3331,14 +3329,9 @@ async def test_remove_unknown_conf_entry_options(
     )
 
     entry.add_to_hass(hass)
-    assert await hass.config_entries.async_setup(entry.entry_id)
-    await hass.async_block_till_done()
+    assert await hass.config_entries.async_setup(entry.entry_id) is False
 
-    assert mqtt.client.CONF_PROTOCOL not in entry.data
-    assert (
-        "The following unsupported configuration options were removed from the "
-        "MQTT config entry: {'old_option'}"
-    ) in caplog.text
+    assert ("extra keys not allowed @ data['old_option']") in caplog.text
 
 
 @patch("homeassistant.components.mqtt.PLATFORMS", [Platform.LIGHT])