From 3ba8a8224365b65d8e6fdcf900925ad18f375e95 Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Sun, 5 Nov 2023 03:08:04 +0100 Subject: [PATCH] Differentiate between warnings and errors in check_config helper (#102902) * Differentiate between warnings and errors in check_config helper * Update tests * Treat configuration errors in frontend and its dependencies as errors * Improve test coverage * Address review comments * Improve test coverage * Improve test coverage * Address review comments * Add comment --- homeassistant/components/config/core.py | 16 +- homeassistant/helpers/check_config.py | 75 +++++++-- homeassistant/scripts/check_config.py | 22 ++- tests/components/config/test_core.py | 39 ++++- tests/helpers/test_check_config.py | 199 ++++++++++++++++++------ tests/scripts/test_check_config.py | 14 +- 6 files changed, 290 insertions(+), 75 deletions(-) diff --git a/homeassistant/components/config/core.py b/homeassistant/components/config/core.py index 9771e12f1d6..4c64028874d 100644 --- a/homeassistant/components/config/core.py +++ b/homeassistant/components/config/core.py @@ -7,9 +7,8 @@ import voluptuous as vol from homeassistant.components import websocket_api from homeassistant.components.http import HomeAssistantView, require_admin from homeassistant.components.sensor import async_update_suggested_units -from homeassistant.config import async_check_ha_config_file from homeassistant.core import HomeAssistant -from homeassistant.helpers import config_validation as cv +from homeassistant.helpers import check_config, config_validation as cv from homeassistant.helpers.aiohttp_client import async_get_clientsession from homeassistant.util import location, unit_system @@ -31,11 +30,18 @@ class CheckConfigView(HomeAssistantView): @require_admin async def post(self, request): """Validate configuration and return results.""" - errors = await async_check_ha_config_file(request.app["hass"]) - state = "invalid" if errors else "valid" + res = await check_config.async_check_ha_config_file(request.app["hass"]) - return self.json({"result": state, "errors": errors}) + state = "invalid" if res.errors else "valid" + + return self.json( + { + "result": state, + "errors": res.error_str or None, + "warnings": res.warning_str or None, + } + ) @websocket_api.require_admin diff --git a/homeassistant/helpers/check_config.py b/homeassistant/helpers/check_config.py index a5e68cb877d..64e57b09d59 100644 --- a/homeassistant/helpers/check_config.py +++ b/homeassistant/helpers/check_config.py @@ -48,6 +48,7 @@ class HomeAssistantConfig(OrderedDict): """Initialize HA config.""" super().__init__() self.errors: list[CheckConfigError] = [] + self.warnings: list[CheckConfigError] = [] def add_error( self, @@ -55,15 +56,30 @@ class HomeAssistantConfig(OrderedDict): domain: str | None = None, config: ConfigType | None = None, ) -> Self: - """Add a single error.""" + """Add an error.""" self.errors.append(CheckConfigError(str(message), domain, config)) return self @property def error_str(self) -> str: - """Return errors as a string.""" + """Concatenate all errors to a string.""" return "\n".join([err.message for err in self.errors]) + def add_warning( + self, + message: str, + domain: str | None = None, + config: ConfigType | None = None, + ) -> Self: + """Add a warning.""" + self.warnings.append(CheckConfigError(str(message), domain, config)) + return self + + @property + def warning_str(self) -> str: + """Concatenate all warnings to a string.""" + return "\n".join([err.message for err in self.warnings]) + async def async_check_ha_config_file( # noqa: C901 hass: HomeAssistant, @@ -82,11 +98,36 @@ async def async_check_ha_config_file( # noqa: C901 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_error(message, domain, pack_config) + result.add_warning(message, domain, pack_config) def _comp_error(ex: Exception, domain: str, config: ConfigType) -> None: """Handle errors from components: async_log_exception.""" - result.add_error(_format_config_error(ex, domain, config)[0], domain, config) + if domain in frontend_dependencies: + result.add_error( + _format_config_error(ex, domain, config)[0], domain, config + ) + else: + result.add_warning( + _format_config_error(ex, domain, config)[0], domain, config + ) + + async def _get_integration( + hass: HomeAssistant, domain: str + ) -> loader.Integration | None: + """Get an integration.""" + integration: loader.Integration | None = None + try: + integration = await async_get_integration_with_requirements(hass, domain) + except loader.IntegrationNotFound as ex: + # We get this error if an integration is not found. In recovery mode and + # safe mode, this currently happens for all custom integrations. Don't + # show errors for a missing integration in recovery mode or safe mode to + # not confuse the user. + if not hass.config.recovery_mode and not hass.config.safe_mode: + result.add_warning(f"Integration error: {domain} - {ex}") + except RequirementsNotFound as ex: + result.add_warning(f"Integration error: {domain} - {ex}") + return integration # Load configuration.yaml config_path = hass.config.path(YAML_CONFIG_FILE) @@ -122,22 +163,22 @@ async def async_check_ha_config_file( # noqa: C901 # Filter out repeating config sections components = {key.partition(" ")[0] for key in config} + frontend_dependencies: set[str] = set() + if "frontend" in components or "default_config" in components: + frontend = await _get_integration(hass, "frontend") + if frontend: + await frontend.resolve_dependencies() + frontend_dependencies = frontend.all_dependencies | {"frontend"} + # Process and validate config for domain in components: - try: - integration = await async_get_integration_with_requirements(hass, domain) - except loader.IntegrationNotFound as ex: - if not hass.config.recovery_mode and not hass.config.safe_mode: - result.add_error(f"Integration error: {domain} - {ex}") - continue - except RequirementsNotFound as ex: - result.add_error(f"Integration error: {domain} - {ex}") + if not (integration := await _get_integration(hass, domain)): continue try: component = integration.get_component() except ImportError as ex: - result.add_error(f"Component error: {domain} - {ex}") + result.add_warning(f"Component error: {domain} - {ex}") continue # Check if the integration has a custom config validator @@ -216,14 +257,18 @@ async def async_check_ha_config_file( # noqa: C901 ) platform = p_integration.get_platform(domain) except loader.IntegrationNotFound as ex: + # We get this error if an integration is not found. In recovery mode and + # safe mode, this currently happens for all custom integrations. Don't + # show errors for a missing integration in recovery mode or safe mode to + # not confuse the user. if not hass.config.recovery_mode and not hass.config.safe_mode: - result.add_error(f"Platform error {domain}.{p_name} - {ex}") + result.add_warning(f"Platform error {domain}.{p_name} - {ex}") continue except ( RequirementsNotFound, ImportError, ) as ex: - result.add_error(f"Platform error {domain}.{p_name} - {ex}") + result.add_warning(f"Platform error {domain}.{p_name} - {ex}") continue # Validate platform specific schema diff --git a/homeassistant/scripts/check_config.py b/homeassistant/scripts/check_config.py index 9a63c73590b..25922ab1f81 100644 --- a/homeassistant/scripts/check_config.py +++ b/homeassistant/scripts/check_config.py @@ -40,6 +40,7 @@ PATCHES: dict[str, Any] = {} C_HEAD = "bold" ERROR_STR = "General Errors" +WARNING_STR = "General Warnings" def color(the_color, *args, reset=None): @@ -116,6 +117,18 @@ def run(script_args: list) -> int: dump_dict(config, reset="red") print(color("reset")) + if res["warn"]: + print(color("bold_white", "Incorrect config")) + for domain, config in res["warn"].items(): + domain_info.append(domain) + print( + " ", + color("bold_yellow", domain + ":"), + color("yellow", "", reset="yellow"), + ) + dump_dict(config, reset="yellow") + print(color("reset")) + if domain_info: if "all" in domain_info: print(color("bold_white", "Successful config (all)")) @@ -160,7 +173,8 @@ def check(config_dir, secrets=False): res: dict[str, Any] = { "yaml_files": OrderedDict(), # yaml_files loaded "secrets": OrderedDict(), # secret cache and secrets loaded - "except": OrderedDict(), # exceptions raised (with config) + "except": OrderedDict(), # critical exceptions raised (with config) + "warn": OrderedDict(), # non critical exceptions raised (with config) #'components' is a HomeAssistantConfig # noqa: E265 "secret_cache": {}, } @@ -215,6 +229,12 @@ def check(config_dir, secrets=False): if err.config: res["except"].setdefault(domain, []).append(err.config) + for err in res["components"].warnings: + domain = err.domain or WARNING_STR + res["warn"].setdefault(domain, []).append(err.message) + if err.config: + res["warn"].setdefault(domain, []).append(err.config) + except Exception as err: # pylint: disable=broad-except print(color("red", "Fatal error while loading config:"), str(err)) res["except"].setdefault(ERROR_STR, []).append(str(err)) diff --git a/tests/components/config/test_core.py b/tests/components/config/test_core.py index fa7f33858a6..bd21e5e7d30 100644 --- a/tests/components/config/test_core.py +++ b/tests/components/config/test_core.py @@ -1,6 +1,6 @@ """Test core config.""" from http import HTTPStatus -from unittest.mock import patch +from unittest.mock import Mock, patch import pytest @@ -37,9 +37,14 @@ async def test_validate_config_ok( client = await hass_client() + no_error = Mock() + no_error.errors = None + no_error.error_str = "" + no_error.warning_str = "" + with patch( - "homeassistant.components.config.core.async_check_ha_config_file", - return_value=None, + "homeassistant.components.config.core.check_config.async_check_ha_config_file", + return_value=no_error, ): resp = await client.post("/api/config/core/check_config") @@ -47,10 +52,16 @@ async def test_validate_config_ok( result = await resp.json() assert result["result"] == "valid" assert result["errors"] is None + assert result["warnings"] is None + + error_warning = Mock() + error_warning.errors = ["beer"] + error_warning.error_str = "beer" + error_warning.warning_str = "milk" with patch( - "homeassistant.components.config.core.async_check_ha_config_file", - return_value="beer", + "homeassistant.components.config.core.check_config.async_check_ha_config_file", + return_value=error_warning, ): resp = await client.post("/api/config/core/check_config") @@ -58,6 +69,24 @@ async def test_validate_config_ok( result = await resp.json() assert result["result"] == "invalid" assert result["errors"] == "beer" + assert result["warnings"] == "milk" + + warning = Mock() + warning.errors = None + warning.error_str = "" + warning.warning_str = "milk" + + with patch( + "homeassistant.components.config.core.check_config.async_check_ha_config_file", + return_value=warning, + ): + resp = await client.post("/api/config/core/check_config") + + assert resp.status == HTTPStatus.OK + result = await resp.json() + assert result["result"] == "valid" + assert result["errors"] is None + assert result["warnings"] == "milk" async def test_validate_config_requires_admin( diff --git a/tests/helpers/test_check_config.py b/tests/helpers/test_check_config.py index 973dec7381e..73d6433315e 100644 --- a/tests/helpers/test_check_config.py +++ b/tests/helpers/test_check_config.py @@ -2,10 +2,13 @@ import logging from unittest.mock import Mock, patch +import pytest + from homeassistant.config import YAML_CONFIG_FILE from homeassistant.core import HomeAssistant from homeassistant.helpers.check_config import ( CheckConfigError, + HomeAssistantConfig, async_check_ha_config_file, ) import homeassistant.helpers.config_validation as cv @@ -40,6 +43,28 @@ def log_ha_config(conf): _LOGGER.debug("error[%s] = %s", cnt, err) +def _assert_warnings_errors( + res: HomeAssistantConfig, + expected_warnings: list[CheckConfigError], + expected_errors: list[CheckConfigError], +) -> None: + assert len(res.warnings) == len(expected_warnings) + assert len(res.errors) == len(expected_errors) + + expected_warning_str = "" + expected_error_str = "" + + for idx, expected_warning in enumerate(expected_warnings): + assert res.warnings[idx] == expected_warning + expected_warning_str += expected_warning.message + assert res.warning_str == expected_warning_str + + for idx, expected_error in enumerate(expected_errors): + assert res.errors[idx] == expected_error + expected_error_str += expected_error.message + assert res.error_str == expected_error_str + + async def test_bad_core_config(hass: HomeAssistant) -> None: """Test a bad core config setup.""" files = {YAML_CONFIG_FILE: BAD_CORE_CONFIG} @@ -47,13 +72,12 @@ async def test_bad_core_config(hass: HomeAssistant) -> None: res = await async_check_ha_config_file(hass) log_ha_config(res) - assert isinstance(res.errors[0].message, str) - assert res.errors[0].domain == "homeassistant" - assert res.errors[0].config == {"unit_system": "bad"} - - # Only 1 error expected - res.errors.pop(0) - assert not res.errors + error = CheckConfigError( + "not a valid value for dictionary value @ data['unit_system']", + "homeassistant", + {"unit_system": "bad"}, + ) + _assert_warnings_errors(res, [], [error]) async def test_config_platform_valid(hass: HomeAssistant) -> None: @@ -65,7 +89,7 @@ async def test_config_platform_valid(hass: HomeAssistant) -> None: assert res.keys() == {"homeassistant", "light"} assert res["light"] == [{"platform": "demo"}] - assert not res.errors + _assert_warnings_errors(res, [], []) async def test_component_platform_not_found(hass: HomeAssistant) -> None: @@ -77,13 +101,10 @@ async def test_component_platform_not_found(hass: HomeAssistant) -> None: log_ha_config(res) assert res.keys() == {"homeassistant"} - assert res.errors[0] == CheckConfigError( + warning = CheckConfigError( "Integration error: beer - Integration 'beer' not found.", None, None ) - - # Only 1 error expected - res.errors.pop(0) - assert not res.errors + _assert_warnings_errors(res, [warning], []) async def test_component_requirement_not_found(hass: HomeAssistant) -> None: @@ -98,7 +119,7 @@ async def test_component_requirement_not_found(hass: HomeAssistant) -> None: log_ha_config(res) assert res.keys() == {"homeassistant"} - assert res.errors[0] == CheckConfigError( + warning = CheckConfigError( ( "Integration error: test_custom_component - Requirements for" " test_custom_component not found: ['any']." @@ -106,10 +127,7 @@ async def test_component_requirement_not_found(hass: HomeAssistant) -> None: None, None, ) - - # Only 1 error expected - res.errors.pop(0) - assert not res.errors + _assert_warnings_errors(res, [warning], []) async def test_component_not_found_recovery_mode(hass: HomeAssistant) -> None: @@ -122,7 +140,7 @@ async def test_component_not_found_recovery_mode(hass: HomeAssistant) -> None: log_ha_config(res) assert res.keys() == {"homeassistant"} - assert not res.errors + _assert_warnings_errors(res, [], []) async def test_component_not_found_safe_mode(hass: HomeAssistant) -> None: @@ -135,7 +153,55 @@ async def test_component_not_found_safe_mode(hass: HomeAssistant) -> None: log_ha_config(res) assert res.keys() == {"homeassistant"} - assert not res.errors + _assert_warnings_errors(res, [], []) + + +async def test_component_import_error(hass: HomeAssistant) -> None: + """Test errors if component with a requirement not found not found.""" + # Make sure they don't exist + files = {YAML_CONFIG_FILE: BASE_CONFIG + "light:"} + with patch( + "homeassistant.loader.Integration.get_component", + side_effect=ImportError("blablabla"), + ), patch("os.path.isfile", return_value=True), patch_yaml_files(files): + res = await async_check_ha_config_file(hass) + log_ha_config(res) + + assert res.keys() == {"homeassistant"} + warning = CheckConfigError( + "Component error: light - blablabla", + None, + None, + ) + _assert_warnings_errors(res, [warning], []) + + +@pytest.mark.parametrize( + ("component", "errors", "warnings", "message"), + [ + ("frontend", 1, 0, "[blah] is an invalid option for [frontend]"), + ("http", 1, 0, "[blah] is an invalid option for [http]"), + ("logger", 0, 1, "[blah] is an invalid option for [logger]"), + ], +) +async def test_component_schema_error( + hass: HomeAssistant, component: str, errors: int, warnings: int, message: str +) -> None: + """Test schema error in component.""" + # Make sure they don't exist + files = {YAML_CONFIG_FILE: BASE_CONFIG + f"frontend:\n{component}:\n blah:"} + hass.config.safe_mode = True + with patch("os.path.isfile", return_value=True), patch_yaml_files(files): + res = await async_check_ha_config_file(hass) + log_ha_config(res) + + assert len(res.errors) == errors + assert len(res.warnings) == warnings + + for err in res.errors: + assert message in err.message + for warn in res.warnings: + assert message in warn.message async def test_component_platform_not_found_2(hass: HomeAssistant) -> None: @@ -149,13 +215,10 @@ async def test_component_platform_not_found_2(hass: HomeAssistant) -> None: assert res.keys() == {"homeassistant", "light"} assert res["light"] == [] - assert res.errors[0] == CheckConfigError( + warning = CheckConfigError( "Platform error light.beer - Integration 'beer' not found.", None, None ) - - # Only 1 error expected - res.errors.pop(0) - assert not res.errors + _assert_warnings_errors(res, [warning], []) async def test_platform_not_found_recovery_mode(hass: HomeAssistant) -> None: @@ -170,7 +233,7 @@ async def test_platform_not_found_recovery_mode(hass: HomeAssistant) -> None: assert res.keys() == {"homeassistant", "light"} assert res["light"] == [] - assert not res.errors + _assert_warnings_errors(res, [], []) async def test_platform_not_found_safe_mode(hass: HomeAssistant) -> None: @@ -185,7 +248,47 @@ async def test_platform_not_found_safe_mode(hass: HomeAssistant) -> None: assert res.keys() == {"homeassistant", "light"} assert res["light"] == [] - assert not res.errors + _assert_warnings_errors(res, [], []) + + +async def test_component_config_platform_import_error(hass: HomeAssistant) -> None: + """Test errors if config platform fails to import.""" + # Make sure they don't exist + files = {YAML_CONFIG_FILE: BASE_CONFIG + "light:\n platform: beer"} + with patch( + "homeassistant.loader.Integration.get_platform", + side_effect=ImportError("blablabla"), + ), patch("os.path.isfile", return_value=True), patch_yaml_files(files): + res = await async_check_ha_config_file(hass) + log_ha_config(res) + + assert res.keys() == {"homeassistant"} + error = CheckConfigError( + "Error importing config platform light: blablabla", + None, + None, + ) + _assert_warnings_errors(res, [], [error]) + + +async def test_component_platform_import_error(hass: HomeAssistant) -> None: + """Test errors if component or platform not found.""" + # Make sure they don't exist + files = {YAML_CONFIG_FILE: BASE_CONFIG + "light:\n platform: demo"} + with patch( + "homeassistant.loader.Integration.get_platform", + side_effect=[None, ImportError("blablabla")], + ), patch("os.path.isfile", return_value=True), patch_yaml_files(files): + res = await async_check_ha_config_file(hass) + log_ha_config(res) + + assert res.keys() == {"homeassistant", "light"} + warning = CheckConfigError( + "Platform error light.demo - blablabla", + None, + None, + ) + _assert_warnings_errors(res, [warning], []) async def test_package_invalid(hass: HomeAssistant) -> None: @@ -195,27 +298,32 @@ async def test_package_invalid(hass: HomeAssistant) -> None: res = await async_check_ha_config_file(hass) log_ha_config(res) - assert res.errors[0].domain == "homeassistant.packages.p1.group" - assert res.errors[0].config == {"group": ["a"]} - # Only 1 error expected - res.errors.pop(0) - assert not res.errors - assert res.keys() == {"homeassistant"} + warning = CheckConfigError( + ( + "Package p1 setup failed. Component group cannot be merged. Expected a " + "dict." + ), + "homeassistant.packages.p1.group", + {"group": ["a"]}, + ) + _assert_warnings_errors(res, [warning], []) -async def test_bootstrap_error(hass: HomeAssistant) -> None: - """Test a valid platform setup.""" + +async def test_missing_included_file(hass: HomeAssistant) -> None: + """Test missing included file.""" files = {YAML_CONFIG_FILE: BASE_CONFIG + "automation: !include no.yaml"} with patch("os.path.isfile", return_value=True), patch_yaml_files(files): res = await async_check_ha_config_file(hass) log_ha_config(res) - assert res.errors[0].domain is None + assert len(res.errors) == 1 + assert len(res.warnings) == 0 - # Only 1 error expected - res.errors.pop(0) - assert not res.errors + assert res.errors[0].message.startswith("Error loading") + assert res.errors[0].domain is None + assert res.errors[0].config is None async def test_automation_config_platform(hass: HomeAssistant) -> None: @@ -251,6 +359,7 @@ action: res = await async_check_ha_config_file(hass) assert len(res.get("automation", [])) == 1 assert len(res.errors) == 0 + assert len(res.warnings) == 0 assert "input_datetime" in res @@ -270,11 +379,12 @@ bla: } with patch("os.path.isfile", return_value=True), patch_yaml_files(files): res = await async_check_ha_config_file(hass) - assert len(res.errors) == 1 - err = res.errors[0] - assert err.domain == "bla" - assert err.message == "Unexpected error calling config validator: Broken" - assert err.config == {"value": 1} + error = CheckConfigError( + "Unexpected error calling config validator: Broken", + "bla", + {"value": 1}, + ) + _assert_warnings_errors(res, [], [error]) async def test_removed_yaml_support(hass: HomeAssistant) -> None: @@ -292,3 +402,4 @@ async def test_removed_yaml_support(hass: HomeAssistant) -> None: log_ha_config(res) assert res.keys() == {"homeassistant"} + _assert_warnings_errors(res, [], []) diff --git a/tests/scripts/test_check_config.py b/tests/scripts/test_check_config.py index e410dd672ce..06dff1e0869 100644 --- a/tests/scripts/test_check_config.py +++ b/tests/scripts/test_check_config.py @@ -49,6 +49,7 @@ def test_bad_core_config(mock_is_file, event_loop, mock_hass_config_yaml: None) res = check_config.check(get_test_config_dir()) assert res["except"].keys() == {"homeassistant"} assert res["except"]["homeassistant"][1] == {"unit_system": "bad"} + assert res["warn"] == {} @pytest.mark.parametrize("hass_config_yaml", [BASE_CONFIG + "light:\n platform: demo"]) @@ -62,6 +63,7 @@ def test_config_platform_valid( assert res["except"] == {} assert res["secret_cache"] == {} assert res["secrets"] == {} + assert res["warn"] == {} assert len(res["yaml_files"]) == 1 @@ -87,9 +89,10 @@ def test_component_platform_not_found( # Make sure they don't exist res = check_config.check(get_test_config_dir()) assert res["components"].keys() == platforms - assert res["except"] == {check_config.ERROR_STR: [error]} + assert res["except"] == {} assert res["secret_cache"] == {} assert res["secrets"] == {} + assert res["warn"] == {check_config.WARNING_STR: [error]} assert len(res["yaml_files"]) == 1 @@ -123,6 +126,7 @@ def test_secrets(mock_is_file, event_loop, mock_hass_config_yaml: None) -> None: get_test_config_dir("secrets.yaml"): {"http_pw": "http://google.com"} } assert res["secrets"] == {"http_pw": "http://google.com"} + assert res["warn"] == {} assert normalize_yaml_files(res) == [ ".../configuration.yaml", ".../secrets.yaml", @@ -136,13 +140,12 @@ def test_package_invalid(mock_is_file, event_loop, mock_hass_config_yaml: None) """Test an invalid package.""" res = check_config.check(get_test_config_dir()) - assert res["except"].keys() == {"homeassistant.packages.p1.group"} - assert res["except"]["homeassistant.packages.p1.group"][1] == {"group": ["a"]} - assert len(res["except"]) == 1 + assert res["except"] == {} assert res["components"].keys() == {"homeassistant"} - assert len(res["components"]) == 1 assert res["secret_cache"] == {} assert res["secrets"] == {} + assert res["warn"].keys() == {"homeassistant.packages.p1.group"} + assert res["warn"]["homeassistant.packages.p1.group"][1] == {"group": ["a"]} assert len(res["yaml_files"]) == 1 @@ -158,4 +161,5 @@ def test_bootstrap_error(event_loop, mock_hass_config_yaml: None) -> None: assert res["components"] == {} # No components, load failed assert res["secret_cache"] == {} assert res["secrets"] == {} + assert res["warn"] == {} assert res["yaml_files"] == {}