From 5b37096b5f9579c55162a205cfaddb8ee897e31f Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Wed, 15 Nov 2023 19:09:49 +0100 Subject: [PATCH] Refactor config.async_log_exception (#104034) * Refactor config.async_log_exception * Improve test coverage * Make functions public --- homeassistant/bootstrap.py | 2 +- .../components/device_tracker/legacy.py | 4 +- homeassistant/components/mqtt/__init__.py | 2 +- homeassistant/components/template/config.py | 4 +- homeassistant/config.py | 73 +++++++++++------- homeassistant/helpers/check_config.py | 18 +++-- tests/helpers/test_check_config.py | 37 ++++++++-- tests/snapshots/test_config.ambr | 3 + tests/test_config.py | 74 +++++++++++++++++++ 9 files changed, 173 insertions(+), 44 deletions(-) diff --git a/homeassistant/bootstrap.py b/homeassistant/bootstrap.py index b9bb638e052..288af779fba 100644 --- a/homeassistant/bootstrap.py +++ b/homeassistant/bootstrap.py @@ -292,7 +292,7 @@ async def async_from_config_dict( try: await conf_util.async_process_ha_core_config(hass, core_config) except vol.Invalid as config_err: - conf_util.async_log_exception(config_err, "homeassistant", core_config, hass) + conf_util.async_log_schema_error(config_err, "homeassistant", core_config, hass) return None except HomeAssistantError: _LOGGER.error( diff --git a/homeassistant/components/device_tracker/legacy.py b/homeassistant/components/device_tracker/legacy.py index 7c12a2d8777..b893654e8cd 100644 --- a/homeassistant/components/device_tracker/legacy.py +++ b/homeassistant/components/device_tracker/legacy.py @@ -14,7 +14,7 @@ import voluptuous as vol from homeassistant import util from homeassistant.backports.functools import cached_property from homeassistant.components import zone -from homeassistant.config import async_log_exception, load_yaml_config_file +from homeassistant.config import async_log_schema_error, load_yaml_config_file from homeassistant.const import ( ATTR_ENTITY_ID, ATTR_GPS_ACCURACY, @@ -1006,7 +1006,7 @@ async def async_load_config( device = dev_schema(device) device["dev_id"] = cv.slugify(dev_id) except vol.Invalid as exp: - async_log_exception(exp, dev_id, devices, hass) + async_log_schema_error(exp, dev_id, devices, hass) else: result.append(Device(hass, **device)) return result diff --git a/homeassistant/components/mqtt/__init__.py b/homeassistant/components/mqtt/__init__.py index effff9fdf12..931615bf0ac 100644 --- a/homeassistant/components/mqtt/__init__.py +++ b/homeassistant/components/mqtt/__init__.py @@ -248,7 +248,7 @@ async def async_check_config_schema( except vol.Invalid as ex: integration = await async_get_integration(hass, DOMAIN) # pylint: disable-next=protected-access - message, _ = conf_util._format_config_error( + message = conf_util.format_schema_error( ex, domain, config, integration.documentation ) raise ServiceValidationError( diff --git a/homeassistant/components/template/config.py b/homeassistant/components/template/config.py index 3329f185f08..d1198b46577 100644 --- a/homeassistant/components/template/config.py +++ b/homeassistant/components/template/config.py @@ -10,7 +10,7 @@ from homeassistant.components.number import DOMAIN as NUMBER_DOMAIN from homeassistant.components.select import DOMAIN as SELECT_DOMAIN from homeassistant.components.sensor import DOMAIN as SENSOR_DOMAIN from homeassistant.components.weather import DOMAIN as WEATHER_DOMAIN -from homeassistant.config import async_log_exception, config_without_domain +from homeassistant.config import async_log_schema_error, config_without_domain from homeassistant.const import CONF_BINARY_SENSORS, CONF_SENSORS, CONF_UNIQUE_ID from homeassistant.helpers import config_validation as cv from homeassistant.helpers.trigger import async_validate_trigger_config @@ -80,7 +80,7 @@ async def async_validate_config(hass, config): hass, cfg[CONF_TRIGGER] ) except vol.Invalid as err: - async_log_exception(err, DOMAIN, cfg, hass) + async_log_schema_error(err, DOMAIN, cfg, hass) continue legacy_warn_printed = False diff --git a/homeassistant/config.py b/homeassistant/config.py index 1a4342c2e87..272ad59d1f4 100644 --- a/homeassistant/config.py +++ b/homeassistant/config.py @@ -490,21 +490,37 @@ def process_ha_config_upgrade(hass: HomeAssistant) -> None: @callback -def async_log_exception( - ex: Exception, +def async_log_schema_error( + ex: vol.Invalid, domain: str, config: dict, hass: HomeAssistant, link: str | None = None, ) -> None: - """Log an error for configuration validation. - - This method must be run in the event loop. - """ + """Log a schema validation error.""" if hass is not None: async_notify_setup_error(hass, domain, link) - message, is_friendly = _format_config_error(ex, domain, config, link) - _LOGGER.error(message, exc_info=not is_friendly and ex) + message = format_schema_error(ex, domain, config, link) + _LOGGER.error(message) + + +@callback +def async_log_config_validator_error( + ex: vol.Invalid | HomeAssistantError, + domain: str, + config: dict, + hass: HomeAssistant, + link: str | None = None, +) -> None: + """Log an error from a custom config validator.""" + if isinstance(ex, vol.Invalid): + async_log_schema_error(ex, domain, config, hass, link) + return + + if hass is not None: + async_notify_setup_error(hass, domain, link) + message = format_homeassistant_error(ex, domain, config, link) + _LOGGER.error(message, exc_info=ex) def _get_annotation(item: Any) -> tuple[str, int | str] | None: @@ -655,25 +671,24 @@ def humanize_error( @callback -def _format_config_error( - ex: Exception, domain: str, config: dict, link: str | None = None -) -> tuple[str, bool]: - """Generate log exception for configuration validation. +def format_homeassistant_error( + ex: HomeAssistantError, domain: str, config: dict, link: str | None = None +) -> str: + """Format HomeAssistantError thrown by a custom config validator.""" + message = f"Invalid config for [{domain}]: {str(ex) or repr(ex)}" - This method must be run in the event loop. - """ - is_friendly = False + if domain != CONF_CORE and link: + message += f" Please check the docs at {link}." - if isinstance(ex, vol.Invalid): - message = humanize_error(ex, domain, config, link) - is_friendly = True - else: - message = f"Invalid config for [{domain}]: {str(ex) or repr(ex)}" + return message - if domain != CONF_CORE and link: - message += f" Please check the docs at {link}." - return message, is_friendly +@callback +def format_schema_error( + ex: vol.Invalid, domain: str, config: dict, link: str | None = None +) -> str: + """Format configuration validation error.""" + return humanize_error(ex, domain, config, link) async def async_process_ha_core_config(hass: HomeAssistant, config: dict) -> None: @@ -995,7 +1010,9 @@ async def async_process_component_config( # noqa: C901 await config_validator.async_validate_config(hass, config) ) except (vol.Invalid, HomeAssistantError) as ex: - async_log_exception(ex, domain, config, hass, integration.documentation) + async_log_config_validator_error( + ex, domain, config, hass, integration.documentation + ) return None except Exception: # pylint: disable=broad-except _LOGGER.exception("Unknown error calling %s config validator", domain) @@ -1006,7 +1023,7 @@ async def async_process_component_config( # noqa: C901 try: return component.CONFIG_SCHEMA(config) # type: ignore[no-any-return] except vol.Invalid as ex: - async_log_exception(ex, domain, config, hass, integration.documentation) + async_log_schema_error(ex, domain, config, hass, integration.documentation) return None except Exception: # pylint: disable=broad-except _LOGGER.exception("Unknown error calling %s CONFIG_SCHEMA", domain) @@ -1025,7 +1042,9 @@ async def async_process_component_config( # noqa: C901 try: p_validated = component_platform_schema(p_config) except vol.Invalid as ex: - async_log_exception(ex, domain, p_config, hass, integration.documentation) + async_log_schema_error( + ex, domain, p_config, hass, integration.documentation + ) continue except Exception: # pylint: disable=broad-except _LOGGER.exception( @@ -1062,7 +1081,7 @@ async def async_process_component_config( # noqa: C901 try: p_validated = platform.PLATFORM_SCHEMA(p_config) except vol.Invalid as ex: - async_log_exception( + async_log_schema_error( ex, f"{domain}.{p_name}", p_config, diff --git a/homeassistant/helpers/check_config.py b/homeassistant/helpers/check_config.py index 441381f9994..e177d30d94d 100644 --- a/homeassistant/helpers/check_config.py +++ b/homeassistant/helpers/check_config.py @@ -15,9 +15,10 @@ from homeassistant.config import ( # type: ignore[attr-defined] CONF_PACKAGES, CORE_CONFIG_SCHEMA, YAML_CONFIG_FILE, - _format_config_error, config_per_platform, extract_domain_configs, + format_homeassistant_error, + format_schema_error, load_yaml_config_file, merge_packages_config, ) @@ -94,15 +95,20 @@ async def async_check_ha_config_file( # noqa: C901 def _pack_error( package: str, component: str, config: ConfigType, message: str ) -> None: - """Handle errors from packages: _log_pkg_error.""" + """Handle errors from packages.""" message = f"Package {package} setup failed. Component {component} {message}" domain = f"homeassistant.packages.{package}.{component}" pack_config = core_config[CONF_PACKAGES].get(package, config) result.add_warning(message, domain, pack_config) - def _comp_error(ex: Exception, domain: str, component_config: ConfigType) -> None: - """Handle errors from components: async_log_exception.""" - message = _format_config_error(ex, domain, component_config)[0] + def _comp_error( + ex: vol.Invalid | HomeAssistantError, domain: str, component_config: ConfigType + ) -> None: + """Handle errors from components.""" + if isinstance(ex, vol.Invalid): + message = format_schema_error(ex, domain, component_config) + else: + message = format_homeassistant_error(ex, domain, component_config) if domain in frontend_dependencies: result.add_error(message, domain, component_config) else: @@ -149,7 +155,7 @@ async def async_check_ha_config_file( # noqa: C901 result[CONF_CORE] = core_config except vol.Invalid as err: result.add_error( - _format_config_error(err, CONF_CORE, core_config)[0], CONF_CORE, core_config + format_schema_error(err, CONF_CORE, core_config), CONF_CORE, core_config ) core_config = {} diff --git a/tests/helpers/test_check_config.py b/tests/helpers/test_check_config.py index 6e7245603b6..8a49f23cf46 100644 --- a/tests/helpers/test_check_config.py +++ b/tests/helpers/test_check_config.py @@ -7,6 +7,7 @@ import voluptuous as vol from homeassistant.config import YAML_CONFIG_FILE from homeassistant.core import HomeAssistant +from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers.check_config import ( CheckConfigError, HomeAssistantConfig, @@ -440,12 +441,38 @@ action: assert "input_datetime" in res -async def test_config_platform_raise(hass: HomeAssistant) -> None: +@pytest.mark.parametrize( + ("exception", "errors", "warnings", "message", "config"), + [ + ( + Exception("Broken"), + 1, + 0, + "Unexpected error calling config validator: Broken", + {"value": 1}, + ), + ( + HomeAssistantError("Broken"), + 0, + 1, + "Invalid config for [bla]: Broken", + {"bla": {"value": 1}}, + ), + ], +) +async def test_config_platform_raise( + hass: HomeAssistant, + exception: Exception, + errors: int, + warnings: int, + message: str, + config: dict, +) -> None: """Test bad config validation platform.""" mock_platform( hass, "bla.config", - Mock(async_validate_config=Mock(side_effect=Exception("Broken"))), + Mock(async_validate_config=Mock(side_effect=exception)), ) files = { YAML_CONFIG_FILE: BASE_CONFIG @@ -457,11 +484,11 @@ bla: with patch("os.path.isfile", return_value=True), patch_yaml_files(files): res = await async_check_ha_config_file(hass) error = CheckConfigError( - "Unexpected error calling config validator: Broken", + message, "bla", - {"value": 1}, + config, ) - _assert_warnings_errors(res, [], [error]) + _assert_warnings_errors(res, [error] * warnings, [error] * errors) async def test_removed_yaml_support(hass: HomeAssistant) -> None: diff --git a/tests/snapshots/test_config.ambr b/tests/snapshots/test_config.ambr index 9df27e02f90..19ff83f0d08 100644 --- a/tests/snapshots/test_config.ambr +++ b/tests/snapshots/test_config.ambr @@ -305,6 +305,9 @@ Invalid config for [adr_0007_5] at /fixtures/core/config/component_validation/basic/configuration.yaml, line 44: 'no_such_option' is an invalid option for [adr_0007_5], check: adr_0007_5->no_such_option. Please check the docs at https://www.home-assistant.io/integrations/adr_0007_5 Invalid config for [adr_0007_5] at /fixtures/core/config/component_validation/basic/configuration.yaml, line 45: expected int for dictionary value 'adr_0007_5->port', got 'foo'. Please check the docs at https://www.home-assistant.io/integrations/adr_0007_5. ''', + "Invalid config for [custom_validator_ok_2] at /fixtures/core/config/component_validation/basic/configuration.yaml, line 52: required key 'host' not provided. Please check the docs at https://www.home-assistant.io/integrations/custom_validator_ok_2.", + 'Invalid config for [custom_validator_bad_1]: broken Please check the docs at https://www.home-assistant.io/integrations/custom_validator_bad_1.', + 'Unknown error calling custom_validator_bad_2 config validator', ]) # --- # name: test_package_merge_error[packages] diff --git a/tests/test_config.py b/tests/test_config.py index 2abb2f08d60..d39a1d4e907 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -295,6 +295,75 @@ async def mock_custom_validator_integrations(hass: HomeAssistant) -> list[Integr ) +@pytest.fixture +async def mock_custom_validator_integrations_with_docs( + hass: HomeAssistant, +) -> list[Integration]: + """Mock integrations with custom validator.""" + integrations = [] + + for domain in ("custom_validator_ok_1", "custom_validator_ok_2"): + + def gen_async_validate_config(domain): + schema = vol.Schema( + { + domain: vol.Schema( + { + vol.Required("host"): str, + vol.Optional("port", default=8080): int, + } + ) + }, + extra=vol.ALLOW_EXTRA, + ) + + async def async_validate_config( + hass: HomeAssistant, config: ConfigType + ) -> ConfigType: + """Validate config.""" + return schema(config) + + return async_validate_config + + integrations.append( + mock_integration( + hass, + MockModule( + domain, + partial_manifest={ + "documentation": f"https://www.home-assistant.io/integrations/{domain}" + }, + ), + ) + ) + mock_platform( + hass, + f"{domain}.config", + Mock(async_validate_config=gen_async_validate_config(domain)), + ) + + for domain, exception in [ + ("custom_validator_bad_1", HomeAssistantError("broken")), + ("custom_validator_bad_2", ValueError("broken")), + ]: + integrations.append( + mock_integration( + hass, + MockModule( + domain, + partial_manifest={ + "documentation": f"https://www.home-assistant.io/integrations/{domain}" + }, + ), + ) + ) + mock_platform( + hass, + f"{domain}.config", + Mock(async_validate_config=AsyncMock(side_effect=exception)), + ) + + async def test_create_default_config(hass: HomeAssistant) -> None: """Test creation of default config.""" assert not os.path.isfile(YAML_PATH) @@ -1683,6 +1752,7 @@ async def test_component_config_validation_error_with_docs( mock_iot_domain_integration_with_docs: Integration, mock_non_adr_0007_integration_with_docs: None, mock_adr_0007_integrations_with_docs: list[Integration], + mock_custom_validator_integrations_with_docs: list[Integration], snapshot: SnapshotAssertion, ) -> None: """Test schema error in component.""" @@ -1700,6 +1770,10 @@ async def test_component_config_validation_error_with_docs( "adr_0007_3", "adr_0007_4", "adr_0007_5", + "custom_validator_ok_1", + "custom_validator_ok_2", + "custom_validator_bad_1", + "custom_validator_bad_2", ]: integration = await async_get_integration(hass, domain) await config_util.async_process_component_config(