From c377cf1ce0478dd793be9daaca37c8298f817929 Mon Sep 17 00:00:00 2001 From: Jan Bouwhuis Date: Thu, 19 Oct 2023 12:06:33 +0200 Subject: [PATCH] Do not fail mqtt entry on single platform config schema error (#101373) * Do not fail mqtt entry on platform config * Raise on reload with invalid config * Do not store issues * Follow up --- homeassistant/components/mqtt/__init__.py | 49 ++++++++ .../components/mqtt/alarm_control_panel.py | 27 ++--- .../components/mqtt/config_integration.py | 6 +- homeassistant/components/mqtt/mixins.py | 113 ++++++++++++++++++ homeassistant/components/mqtt/models.py | 3 + homeassistant/components/mqtt/strings.json | 4 + .../mqtt/test_alarm_control_panel.py | 109 +++++++++++++++-- tests/components/mqtt/test_discovery.py | 19 +++ tests/components/mqtt/test_init.py | 28 ++++- 9 files changed, 320 insertions(+), 38 deletions(-) diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index 7caeb2b51f7..3d3bb486c02 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -29,9 +29,14 @@ from homeassistant.helpers import config_validation as cv, event as ev, template from homeassistant.helpers.device_registry import DeviceEntry from homeassistant.helpers.dispatcher import async_dispatcher_connect from homeassistant.helpers.entity_platform import async_get_platforms +from homeassistant.helpers.issue_registry import ( + async_delete_issue, + async_get as async_get_issue_registry, +) from homeassistant.helpers.reload import async_integration_yaml_config from homeassistant.helpers.service import async_register_admin_service from homeassistant.helpers.typing import ConfigType +from homeassistant.loader import async_get_integration # Loading the config flow file will register the flow from . import debug_info, discovery @@ -209,6 +214,41 @@ async def _async_config_entry_updated(hass: HomeAssistant, entry: ConfigEntry) - await hass.config_entries.async_reload(entry.entry_id) +@callback +def _async_remove_mqtt_issues(hass: HomeAssistant, mqtt_data: MqttData) -> None: + """Unregister open config issues.""" + issue_registry = async_get_issue_registry(hass) + open_issues = [ + issue_id + for (domain, issue_id), issue_entry in issue_registry.issues.items() + if domain == DOMAIN and issue_entry.translation_key == "invalid_platform_config" + ] + for issue in open_issues: + async_delete_issue(hass, DOMAIN, issue) + + +async def async_check_config_schema( + hass: HomeAssistant, config_yaml: ConfigType +) -> None: + """Validate manually configured MQTT items.""" + mqtt_data = get_mqtt_data(hass) + mqtt_config: list[dict[str, list[ConfigType]]] = config_yaml[DOMAIN] + for mqtt_config_item in mqtt_config: + for domain, config_items in mqtt_config_item.items(): + if (schema := mqtt_data.reload_schema.get(domain)) is None: + continue + for config in config_items: + try: + schema(config) + except vol.Invalid as ex: + integration = await async_get_integration(hass, DOMAIN) + # pylint: disable-next=protected-access + message, _ = conf_util._format_config_error( + ex, domain, config, integration.documentation + ) + raise HomeAssistantError(message) from ex + + async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: """Load a config entry.""" conf: dict[str, Any] @@ -373,6 +413,12 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: "Error reloading manually configured MQTT items, " "check your configuration.yaml" ) + # Check the schema before continuing reload + await async_check_config_schema(hass, config_yaml) + + # Remove repair issues + _async_remove_mqtt_issues(hass, mqtt_data) + mqtt_data.config = config_yaml.get(DOMAIN, {}) # Reload the modern yaml platforms @@ -594,4 +640,7 @@ async def async_unload_entry(hass: HomeAssistant, entry: ConfigEntry) -> bool: if subscriptions := mqtt_client.subscriptions: mqtt_data.subscriptions_to_restore = subscriptions + # Remove repair issues + _async_remove_mqtt_issues(hass, mqtt_data) + return True diff --git a/homeassistant/components/mqtt/alarm_control_panel.py b/homeassistant/components/mqtt/alarm_control_panel.py index a960367ad11..1eb210bf99e 100644 --- a/homeassistant/components/mqtt/alarm_control_panel.py +++ b/homeassistant/components/mqtt/alarm_control_panel.py @@ -1,7 +1,6 @@ """Control a MQTT alarm.""" from __future__ import annotations -import functools import logging import voluptuous as vol @@ -27,7 +26,7 @@ from homeassistant.const import ( from homeassistant.core import HomeAssistant, callback import homeassistant.helpers.config_validation as cv from homeassistant.helpers.entity_platform import AddEntitiesCallback -from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType +from homeassistant.helpers.typing import ConfigType from . import subscription from .config import DEFAULT_RETAIN, MQTT_BASE_SCHEMA @@ -44,7 +43,7 @@ from .debug_info import log_messages from .mixins import ( MQTT_ENTITY_COMMON_SCHEMA, MqttEntity, - async_setup_entry_helper, + async_mqtt_entry_helper, write_state_on_attr_change, ) from .models import MqttCommandTemplate, MqttValueTemplate, ReceiveMessage @@ -133,21 +132,15 @@ async def async_setup_entry( async_add_entities: AddEntitiesCallback, ) -> None: """Set up MQTT alarm control panel through YAML and through MQTT discovery.""" - setup = functools.partial( - _async_setup_entity, hass, async_add_entities, config_entry=config_entry + await async_mqtt_entry_helper( + hass, + config_entry, + MqttAlarm, + alarm.DOMAIN, + async_add_entities, + DISCOVERY_SCHEMA, + PLATFORM_SCHEMA_MODERN, ) - await async_setup_entry_helper(hass, alarm.DOMAIN, setup, DISCOVERY_SCHEMA) - - -async def _async_setup_entity( - hass: HomeAssistant, - async_add_entities: AddEntitiesCallback, - config: ConfigType, - config_entry: ConfigEntry, - discovery_data: DiscoveryInfoType | None = None, -) -> None: - """Set up the MQTT Alarm Control Panel platform.""" - async_add_entities([MqttAlarm(hass, config, config_entry, discovery_data)]) class MqttAlarm(MqttEntity, alarm.AlarmControlPanelEntity): diff --git a/homeassistant/components/mqtt/config_integration.py b/homeassistant/components/mqtt/config_integration.py index 79e977a90cd..975ddfe6386 100644 --- a/homeassistant/components/mqtt/config_integration.py +++ b/homeassistant/components/mqtt/config_integration.py @@ -15,7 +15,6 @@ from homeassistant.const import ( from homeassistant.helpers import config_validation as cv from . import ( - alarm_control_panel as alarm_control_panel_platform, binary_sensor as binary_sensor_platform, button as button_platform, camera as camera_platform, @@ -56,10 +55,7 @@ DEFAULT_TLS_PROTOCOL = "auto" CONFIG_SCHEMA_BASE = vol.Schema( { - Platform.ALARM_CONTROL_PANEL.value: vol.All( - cv.ensure_list, - [alarm_control_panel_platform.PLATFORM_SCHEMA_MODERN], # type: ignore[has-type] # noqa: E501 - ), + Platform.ALARM_CONTROL_PANEL.value: vol.All(cv.ensure_list, [dict]), Platform.BINARY_SENSOR.value: vol.All( cv.ensure_list, [binary_sensor_platform.PLATFORM_SCHEMA_MODERN], # type: ignore[has-type] diff --git a/homeassistant/components/mqtt/mixins.py b/homeassistant/components/mqtt/mixins.py index a01691f0601..1138663c851 100644 --- a/homeassistant/components/mqtt/mixins.py +++ b/homeassistant/components/mqtt/mixins.py @@ -9,6 +9,7 @@ import logging from typing import TYPE_CHECKING, Any, Protocol, cast, final import voluptuous as vol +import yaml from homeassistant.config_entries import ConfigEntry from homeassistant.const import ( @@ -53,6 +54,7 @@ from homeassistant.helpers.event import ( async_track_device_registry_updated_event, async_track_entity_registry_updated_event, ) +from homeassistant.helpers.issue_registry import IssueSeverity, async_create_issue from homeassistant.helpers.typing import ( UNDEFINED, ConfigType, @@ -337,6 +339,117 @@ async def async_setup_entry_helper( await _async_setup_entities() +async def async_mqtt_entry_helper( + hass: HomeAssistant, + entry: ConfigEntry, + entity_class: type[MqttEntity], + domain: str, + async_add_entities: AddEntitiesCallback, + discovery_schema: vol.Schema, + platform_schema_modern: vol.Schema, +) -> None: + """Set up entity, automation or tag creation dynamically through MQTT discovery.""" + mqtt_data = get_mqtt_data(hass) + + async def async_discover(discovery_payload: MQTTDiscoveryPayload) -> None: + """Discover and add an MQTT entity, automation or tag.""" + if not mqtt_config_entry_enabled(hass): + _LOGGER.warning( + ( + "MQTT integration is disabled, skipping setup of discovered item " + "MQTT %s, payload %s" + ), + domain, + discovery_payload, + ) + return + discovery_data = discovery_payload.discovery_data + try: + config: DiscoveryInfoType = discovery_schema(discovery_payload) + async_add_entities([entity_class(hass, config, entry, discovery_data)]) + except vol.Invalid as err: + discovery_hash = discovery_data[ATTR_DISCOVERY_HASH] + clear_discovery_hash(hass, discovery_hash) + async_dispatcher_send( + hass, MQTT_DISCOVERY_DONE.format(discovery_hash), None + ) + async_handle_schema_error(discovery_payload, err) + except Exception: + discovery_hash = discovery_data[ATTR_DISCOVERY_HASH] + clear_discovery_hash(hass, discovery_hash) + async_dispatcher_send( + hass, MQTT_DISCOVERY_DONE.format(discovery_hash), None + ) + raise + + mqtt_data.reload_dispatchers.append( + async_dispatcher_connect( + hass, MQTT_DISCOVERY_NEW.format(domain, "mqtt"), async_discover + ) + ) + + async def _async_setup_entities() -> None: + """Set up MQTT items from configuration.yaml.""" + mqtt_data = get_mqtt_data(hass) + if not (config_yaml := mqtt_data.config): + return + yaml_configs: list[ConfigType] = [ + config + for config_item in config_yaml + for config_domain, configs in config_item.items() + for config in configs + if config_domain == domain + ] + entities: list[Entity] = [] + for yaml_config in yaml_configs: + try: + config = platform_schema_modern(yaml_config) + entities.append(entity_class(hass, config, entry, None)) + except vol.Invalid as ex: + error = str(ex) + config_file = getattr(yaml_config, "__config_file__", "?") + line = getattr(yaml_config, "__line__", "?") + issue_id = hex(hash(frozenset(yaml_config.items()))) + yaml_config_str = yaml.dump(dict(yaml_config)) + learn_more_url = ( + f"https://www.home-assistant.io/integrations/{domain}.mqtt/" + ) + async_create_issue( + hass, + DOMAIN, + issue_id, + issue_domain=domain, + is_fixable=False, + severity=IssueSeverity.ERROR, + learn_more_url=learn_more_url, + translation_placeholders={ + "domain": domain, + "config_file": config_file, + "line": line, + "config": yaml_config_str, + "error": error, + }, + translation_key="invalid_platform_config", + ) + _LOGGER.error( + "%s for manual configured MQTT %s item, in %s, line %s Got %s", + error, + domain, + config_file, + line, + yaml_config, + ) + + async_add_entities(entities) + + # When reloading we check manual configured items against the schema + # before reloading + mqtt_data.reload_schema[domain] = platform_schema_modern + # discover manual configured MQTT items + mqtt_data.reload_handlers[domain] = _async_setup_entities + await _async_setup_entities() + + def init_entity_id_from_config( hass: HomeAssistant, entity: Entity, config: ConfigType, entity_id_format: str ) -> None: diff --git a/homeassistant/components/mqtt/models.py b/homeassistant/components/mqtt/models.py index 23faa726e09..53442d35cef 100644 --- a/homeassistant/components/mqtt/models.py +++ b/homeassistant/components/mqtt/models.py @@ -11,6 +11,8 @@ from enum import StrEnum import logging from typing import TYPE_CHECKING, Any, TypedDict +import voluptuous as vol + from homeassistant.const import ATTR_ENTITY_ID, ATTR_NAME from homeassistant.core import CALLBACK_TYPE, HomeAssistant, callback from homeassistant.helpers import template @@ -343,6 +345,7 @@ class MqttData: reload_handlers: dict[str, Callable[[], Coroutine[Any, Any, None]]] = field( default_factory=dict ) + reload_schema: dict[str, vol.Schema] = field(default_factory=dict) state_write_requests: EntityTopicState = field(default_factory=EntityTopicState) subscriptions_to_restore: list[Subscription] = field(default_factory=list) tags: dict[str, dict[str, MQTTTagScanner]] = field(default_factory=dict) diff --git a/homeassistant/components/mqtt/strings.json b/homeassistant/components/mqtt/strings.json index b28f16cb404..9082e034e02 100644 --- a/homeassistant/components/mqtt/strings.json +++ b/homeassistant/components/mqtt/strings.json @@ -19,6 +19,10 @@ "deprecated_climate_aux_property": { "title": "MQTT entities with auxiliary heat support found", "description": "Entity `{entity_id}` has auxiliary heat support enabled, which has been deprecated for MQTT climate devices. Please adjust your configuration and remove deprecated config options from your configuration and restart Home Assistant to fix this issue." + }, + "invalid_platform_config": { + "title": "Invalid configured MQTT {domain} item", + "description": "Home Assistant detected an invalid config for a manual configured item.\n\nPlatform domain: **{domain}**\nConfiguration file: **{config_file}**\nNear line: **{line}**\nConfiguration found:\n```yaml\n{config}\n```\nError: **{error}**.\n\nMake sure the configuration is valid and [reload](/developer-tools/yaml) the manually configured MQTT items or restart Home Assistant to fix this issue." } }, "config": { diff --git a/tests/components/mqtt/test_alarm_control_panel.py b/tests/components/mqtt/test_alarm_control_panel.py index 7532319854a..0d5c9ee2e8d 100644 --- a/tests/components/mqtt/test_alarm_control_panel.py +++ b/tests/components/mqtt/test_alarm_control_panel.py @@ -22,6 +22,7 @@ from homeassistant.const import ( SERVICE_ALARM_ARM_VACATION, SERVICE_ALARM_DISARM, SERVICE_ALARM_TRIGGER, + SERVICE_RELOAD, STATE_ALARM_ARMED_AWAY, STATE_ALARM_ARMED_CUSTOM_BYPASS, STATE_ALARM_ARMED_HOME, @@ -36,6 +37,7 @@ from homeassistant.const import ( Platform, ) from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from .test_common import ( help_custom_config, @@ -184,11 +186,9 @@ async def test_fail_setup_without_state_or_command_topic( hass: HomeAssistant, mqtt_mock_entry: MqttMockHAClientGenerator, valid ) -> None: """Test for failing setup with no state or command topic.""" - if valid: - await mqtt_mock_entry() - return - with pytest.raises(AssertionError): - await mqtt_mock_entry() + assert await mqtt_mock_entry() + state = hass.states.get(f"{alarm_control_panel.DOMAIN}.test") + assert (state is not None) == valid @pytest.mark.parametrize("hass_config", [DEFAULT_CONFIG]) @@ -306,15 +306,13 @@ async def test_supported_features( valid: bool, ) -> None: """Test conditional enablement of supported features.""" + assert await mqtt_mock_entry() + state = hass.states.get("alarm_control_panel.test") if valid: - await mqtt_mock_entry() - assert ( - hass.states.get("alarm_control_panel.test").attributes["supported_features"] - == expected_features - ) + assert state is not None + assert state.attributes["supported_features"] == expected_features else: - with pytest.raises(AssertionError): - await mqtt_mock_entry() + assert state is None @pytest.mark.parametrize( @@ -1269,3 +1267,90 @@ async def test_skipped_async_ha_write_state( """Test a write state command is only called when there is change.""" await mqtt_mock_entry() await help_test_skipped_async_ha_write_state(hass, topic, payload1, payload2) + + +@pytest.mark.parametrize( + "hass_config", + [ + { + "mqtt": [ + { + "alarm_control_panel": { + "name": "test", + "invalid_topic": "test-topic", + } + }, + ] + } + ], +) +async def test_reload_after_invalid_config( + hass: HomeAssistant, + mqtt_mock_entry: MqttMockHAClientGenerator, + caplog: pytest.LogCaptureFixture, +) -> None: + """Test reloading yaml config fails.""" + with patch( + "homeassistant.components.mqtt.async_delete_issue" + ) as mock_async_remove_issue: + assert await mqtt_mock_entry() + assert hass.states.get("alarm_control_panel.test") is None + assert ( + "extra keys not allowed @ data['invalid_topic'] for " + "manual configured MQTT alarm_control_panel item, " + "in ?, line ? Got {'name': 'test', 'invalid_topic': 'test-topic'}" + in caplog.text + ) + + # Reload with an valid config + valid_config = { + "mqtt": [ + { + "alarm_control_panel": { + "name": "test", + "command_topic": "test-topic", + "state_topic": "alarm/state", + } + }, + ] + } + with patch( + "homeassistant.config.load_yaml_config_file", return_value=valid_config + ): + await hass.services.async_call( + "mqtt", + SERVICE_RELOAD, + {}, + blocking=True, + ) + await hass.async_block_till_done() + + # Test the config is loaded now and that the existing issue is removed + assert hass.states.get("alarm_control_panel.test") is not None + assert mock_async_remove_issue.call_count == 1 + + # Reload with an invalid config + invalid_config = { + "mqtt": [ + { + "alarm_control_panel": { + "name": "test", + "command_topic": "test-topic", + "invalid_option": "should_fail", + } + }, + ] + } + with patch( + "homeassistant.config.load_yaml_config_file", return_value=invalid_config + ), pytest.raises(HomeAssistantError): + await hass.services.async_call( + "mqtt", + SERVICE_RELOAD, + {}, + blocking=True, + ) + await hass.async_block_till_done() + + # Make sure the config is loaded now + assert hass.states.get("alarm_control_panel.test") is not None diff --git a/tests/components/mqtt/test_discovery.py b/tests/components/mqtt/test_discovery.py index d2f6350a801..863a79fce70 100644 --- a/tests/components/mqtt/test_discovery.py +++ b/tests/components/mqtt/test_discovery.py @@ -145,6 +145,25 @@ async def test_discovery_schema_error( assert "AttributeError: Attribute abc not found" in caplog.text +@patch("homeassistant.components.mqtt.PLATFORMS", [Platform.ALARM_CONTROL_PANEL]) +async def test_invalid_config( + hass: HomeAssistant, + mqtt_mock_entry: MqttMockHAClientGenerator, + caplog: pytest.LogCaptureFixture, +) -> None: + """Test sending in JSON that violates the platform schema.""" + await mqtt_mock_entry() + async_fire_mqtt_message( + hass, + "homeassistant/alarm_control_panel/bla/config", + '{"name": "abc", "state_topic": "home/alarm", ' + '"command_topic": "home/alarm/set", ' + '"qos": "some_invalid_value"}', + ) + await hass.async_block_till_done() + assert "Error 'expected int for dictionary value @ data['qos']'" in caplog.text + + async def test_only_valid_components( hass: HomeAssistant, mqtt_mock_entry: MqttMockHAClientGenerator, diff --git a/tests/components/mqtt/test_init.py b/tests/components/mqtt/test_init.py index 3cb1188dccf..2c6ee5d1b20 100644 --- a/tests/components/mqtt/test_init.py +++ b/tests/components/mqtt/test_init.py @@ -3547,14 +3547,21 @@ async def test_publish_or_subscribe_without_valid_config_entry( @patch( "homeassistant.components.mqtt.PLATFORMS", - ["tag", Platform.LIGHT], + [Platform.ALARM_CONTROL_PANEL, Platform.LIGHT], ) @pytest.mark.parametrize( "hass_config", [ { "mqtt": { - "light": [{"name": "test", "command_topic": "test-topic"}], + "alarm_control_panel": [ + { + "name": "test", + "state_topic": "home/alarm", + "command_topic": "home/alarm/set", + }, + ], + "light": [{"name": "test", "command_topic": "test-topic_new"}], } } ], @@ -3568,10 +3575,18 @@ async def test_disabling_and_enabling_entry( await mqtt_mock_entry() entry = hass.config_entries.async_entries(mqtt.DOMAIN)[0] assert entry.state is ConfigEntryState.LOADED - # Late discovery of a mqtt entity/tag + # Late discovery of a mqtt entity config_tag = '{"topic": "0AFFD2/tag_scanned", "value_template": "{{ value_json.PN532.UID }}"}' - config_light = '{"name": "test2", "command_topic": "test-topic_new"}' + config_alarm_control_panel = '{"name": "test_new", "state_topic": "home/alarm", "command_topic": "home/alarm/set"}' + config_light = '{"name": "test_new", "command_topic": "test-topic_new"}' + + # Discovery of mqtt tag async_fire_mqtt_message(hass, "homeassistant/tag/abc/config", config_tag) + + # Late discovery of mqtt entities + async_fire_mqtt_message( + hass, "homeassistant/alarm_control_panel/abc/config", config_alarm_control_panel + ) async_fire_mqtt_message(hass, "homeassistant/light/abc/config", config_light) # Disable MQTT config entry @@ -3585,6 +3600,10 @@ async def test_disabling_and_enabling_entry( "MQTT integration is disabled, skipping setup of discovered item MQTT tag" in caplog.text ) + assert ( + "MQTT integration is disabled, skipping setup of discovered item MQTT alarm_control_panel" + in caplog.text + ) assert ( "MQTT integration is disabled, skipping setup of discovered item MQTT light" in caplog.text @@ -3601,6 +3620,7 @@ async def test_disabling_and_enabling_entry( assert new_mqtt_config_entry.state is ConfigEntryState.LOADED assert hass.states.get("light.test") is not None + assert hass.states.get("alarm_control_panel.test") is not None @patch("homeassistant.components.mqtt.PLATFORMS", [Platform.LIGHT])