Improve package schema validation (#108125)

* Add failing tests for package config validation error wrapping

* Wrap package schema validation errors in HomeAssistantError

* Fix yamllint errors

* Rework package merge validation

Ignore invalid package definitions instead of failing startup.
Output error messages with locations if possible when a package
definition has errors.

* Ruff format

* Fix linter errors

* Move package_definition_schema to module scope

* Move inner function to module level

* Merge exception handlers

Merge exception handlers for config schema validation and package merge
to avoid untested code branches

* Fix long lines and doc strings

* More minor changes to exception handler

---------

Co-authored-by: Erik Montnemery <erik@montnemery.com>

homeassistant/config.py

@@ -212,9 +212,11 @@ def _filter_bad_internal_external_urls(conf: dict) -> dict:
     return conf
 
 
-PACKAGES_CONFIG_SCHEMA = cv.schema_with_slug_keys(  # Package names are slugs
-    vol.Schema({cv.string: vol.Any(dict, list, None)})  # Component config
-)
+# Schema for all packages element
+PACKAGES_CONFIG_SCHEMA = vol.Schema({cv.string: vol.Any(dict, list)})
+
+# Schema for individual package definition
+PACKAGE_DEFINITION_SCHEMA = vol.Schema({cv.string: vol.Any(dict, list, None)})
 
 CUSTOMIZE_DICT_SCHEMA = vol.Schema(
     {
@@ -499,7 +501,17 @@ async def async_hass_config_yaml(hass: HomeAssistant) -> dict:
         config.pop(invalid_domain)
 
     core_config = config.get(CONF_CORE, {})
-    await merge_packages_config(hass, config, core_config.get(CONF_PACKAGES, {}))
+    try:
+        await merge_packages_config(hass, config, core_config.get(CONF_PACKAGES, {}))
+    except vol.Invalid as exc:
+        suffix = ""
+        if annotation := find_annotation(config, [CONF_CORE, CONF_PACKAGES] + exc.path):
+            suffix = f" at {_relpath(hass, annotation[0])}, line {annotation[1]}"
+        _LOGGER.error(
+            "Invalid package configuration '%s'%s: %s", CONF_PACKAGES, suffix, exc
+        )
+        core_config[CONF_PACKAGES] = {}
+
     return config
 
 
@@ -938,7 +950,7 @@ async def async_process_ha_core_config(hass: HomeAssistant, config: dict) -> Non
 
 
 def _log_pkg_error(
-    hass: HomeAssistant, package: str, component: str, config: dict, message: str
+    hass: HomeAssistant, package: str, component: str | None, config: dict, message: str
 ) -> None:
     """Log an error while merging packages."""
     message_prefix = f"Setup of package '{package}'"
@@ -996,6 +1008,12 @@ def _identify_config_schema(module: ComponentProtocol) -> str | None:
     return None
 
 
+def _validate_package_definition(name: str, conf: Any) -> None:
+    """Validate basic package definition properties."""
+    cv.slug(name)
+    PACKAGE_DEFINITION_SCHEMA(conf)
+
+
 def _recursive_merge(conf: dict[str, Any], package: dict[str, Any]) -> str | None:
     """Merge package into conf, recursively."""
     duplicate_key: str | None = None
@@ -1023,12 +1041,33 @@ async def merge_packages_config(
     config: dict,
     packages: dict[str, Any],
     _log_pkg_error: Callable[
-        [HomeAssistant, str, str, dict, str], None
+        [HomeAssistant, str, str | None, dict, str], None
     ] = _log_pkg_error,
 ) -> dict:
-    """Merge packages into the top-level configuration. Mutate config."""
+    """Merge packages into the top-level configuration.
+
+    Ignores packages that cannot be setup. Mutates config. Raises
+    vol.Invalid if whole package config is invalid.
+    """
+
     PACKAGES_CONFIG_SCHEMA(packages)
+
+    invalid_packages = []
     for pack_name, pack_conf in packages.items():
+        try:
+            _validate_package_definition(pack_name, pack_conf)
+        except vol.Invalid as exc:
+            _log_pkg_error(
+                hass,
+                pack_name,
+                None,
+                config,
+                f"Invalid package definition '{pack_name}': {str(exc)}. Package "
+                f"will not be initialized",
+            )
+            invalid_packages.append(pack_name)
+            continue
+
         for comp_name, comp_conf in pack_conf.items():
             if comp_name == CONF_CORE:
                 continue
@@ -1123,6 +1162,9 @@ async def merge_packages_config(
                     f"integration '{comp_name}' has duplicate key '{duplicate_key}'",
                 )
 
+    for pack_name in invalid_packages:
+        packages.pop(pack_name, {})
+
     return config
 
 

homeassistant/helpers/check_config.py

@@ -96,13 +96,13 @@ async def async_check_ha_config_file(  # noqa: C901
     def _pack_error(
         hass: HomeAssistant,
         package: str,
-        component: str,
+        component: str | None,
         config: ConfigType,
         message: str,
     ) -> None:
         """Handle errors from packages."""
         message = f"Setup of package '{package}' failed: {message}"
-        domain = f"homeassistant.packages.{package}.{component}"
+        domain = f"homeassistant.packages.{package}{'.' + component if component is not None else ''}"
         pack_config = core_config[CONF_PACKAGES].get(package, config)
         result.add_warning(message, domain, pack_config)
 
@@ -157,10 +157,15 @@ async def async_check_ha_config_file(  # noqa: C901
         return result.add_error(f"Error loading {config_path}: {err}")
 
     # Extract and validate core [homeassistant] config
+    core_config = config.pop(CONF_CORE, {})
     try:
-        core_config = config.pop(CONF_CORE, {})
         core_config = CORE_CONFIG_SCHEMA(core_config)
         result[CONF_CORE] = core_config
+
+        # Merge packages
+        await merge_packages_config(
+            hass, config, core_config.get(CONF_PACKAGES, {}), _pack_error
+        )
     except vol.Invalid as err:
         result.add_error(
             format_schema_error(hass, err, CONF_CORE, core_config),
@@ -168,11 +173,6 @@ async def async_check_ha_config_file(  # noqa: C901
             core_config,
         )
         core_config = {}
-
-    # Merge packages
-    await merge_packages_config(
-        hass, config, core_config.get(CONF_PACKAGES, {}), _pack_error
-    )
     core_config.pop(CONF_PACKAGES, None)
 
     # Filter out repeating config sections

tests/fixtures/core/config/package_schema_errors/packages_is_a_list/configuration.yaml

@@ -0,0 +1,5 @@
+homeassistant:
+  packages:
+    - should_be_a_dict_not_a_list
+    - this_also_fails:
+      iot_domain: [{ "some": "yay" }]

tests/fixtures/core/config/package_schema_errors/packages_is_a_value/configuration.yaml

@@ -0,0 +1,2 @@
+homeassistant:
+  packages: "Hello"

tests/fixtures/core/config/package_schema_errors/packages_is_null/configuration.yaml

@@ -0,0 +1,3 @@
+homeassistant:
+  name: Ensure Correct Line Numbers
+  packages: null

tests/fixtures/core/config/package_schema_validation/packages_dict/configuration.yaml

@@ -0,0 +1,6 @@
+homeassistant:
+  packages:
+    should_be_a_dict:
+      - platform: template
+    this_works:
+      iot_domain: [{ "some": "yay" }]

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_dict/configuration.yaml

@@ -0,0 +1,2 @@
+homeassistant:
+  packages: !include_dir_merge_named packages/

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_dict/packages/expected_dict.yaml

@@ -0,0 +1,2 @@
+should_be_a_dict:
+  - platform: template

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_dict/packages/this_works.yaml

@@ -0,0 +1,2 @@
+this_works:
+  iot_domain: [{ "some": "yay" }]

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_slug/configuration.yaml

@@ -0,0 +1,2 @@
+homeassistant:
+  packages: !include_dir_merge_named packages/

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_slug/packages/expected_slug.yaml

@@ -0,0 +1,3 @@
+this is not a slug but it should be one:
+  switch:
+    - platform: template

tests/fixtures/core/config/package_schema_validation/packages_include_dir_named_slug/packages/this_works.yaml

@@ -0,0 +1,2 @@
+this_works:
+  iot_domain: [{ "some": "yay" }]

tests/fixtures/core/config/package_schema_validation/packages_slug/configuration.yaml

@@ -0,0 +1,7 @@
+homeassistant:
+  packages:
+    this is not a slug but it should be one:
+      switch:
+        - platform: template
+    this_works:
+      iot_domain: [{ "some": "yay" }]

tests/helpers/test_check_config.py

@@ -374,7 +374,7 @@ async def test_platform_import_error(hass: HomeAssistant) -> None:
 
 
 async def test_package_invalid(hass: HomeAssistant) -> None:
-    """Test a valid platform setup."""
+    """Test a platform setup with an invalid package config."""
     files = {YAML_CONFIG_FILE: BASE_CONFIG + '  packages:\n    p1:\n      group: ["a"]'}
     with patch("os.path.isfile", return_value=True), patch_yaml_files(files):
         res = await async_check_ha_config_file(hass)
@@ -393,6 +393,72 @@ async def test_package_invalid(hass: HomeAssistant) -> None:
         _assert_warnings_errors(res, [warning], [])
 
 
+async def test_package_definition_invalid_slug_keys(hass: HomeAssistant) -> None:
+    """Test a platform setup with a broken package: keys must be slugs."""
+    files = {
+        YAML_CONFIG_FILE: BASE_CONFIG
+        + '  packages:\n    not a slug:\n      group: ["a"]'
+    }
+    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.keys() == {"homeassistant"}
+
+        warning = CheckConfigError(
+            (
+                "Setup of package 'not a slug' failed: Invalid package definition 'not a slug': invalid slug not a "
+                "slug (try not_a_slug). Package will not be initialized"
+            ),
+            "homeassistant.packages.not a slug",
+            {"group": ["a"]},
+        )
+        _assert_warnings_errors(res, [warning], [])
+
+
+async def test_package_definition_invalid_dict(hass: HomeAssistant) -> None:
+    """Test a platform setup with a broken package: packages must be dicts."""
+    files = {
+        YAML_CONFIG_FILE: BASE_CONFIG
+        + '  packages:\n    not_a_dict:\n      - group: ["a"]'
+    }
+    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.keys() == {"homeassistant"}
+
+        warning = CheckConfigError(
+            (
+                "Setup of package 'not_a_dict' failed: Invalid package definition 'not_a_dict': expected a "
+                "dictionary. Package will not be initialized"
+            ),
+            "homeassistant.packages.not_a_dict",
+            [{"group": ["a"]}],
+        )
+        _assert_warnings_errors(res, [warning], [])
+
+
+async def test_package_schema_invalid(hass: HomeAssistant) -> None:
+    """Test an invalid platform config because of severely broken packages section."""
+    files = {
+        YAML_CONFIG_FILE: "homeassistant:\n  packages:\n    - must\n    - not\n    - be\n    - a\n    - list"
+    }
+    with patch("os.path.isfile", return_value=True), patch_yaml_files(files):
+        res = await async_check_ha_config_file(hass)
+        log_ha_config(res)
+
+        error = CheckConfigError(
+            (
+                f"Invalid config for 'homeassistant' at {YAML_CONFIG_FILE}, line 2:"
+                " expected a dictionary for dictionary value 'packages', got ['must', 'not', 'be', 'a', 'list']"
+            ),
+            "homeassistant",
+            {"packages": ["must", "not", "be", "a", "list"]},
+        )
+        _assert_warnings_errors(res, [], [error])
+
+
 async def test_missing_included_file(hass: HomeAssistant) -> None:
     """Test missing included file."""
     files = {YAML_CONFIG_FILE: BASE_CONFIG + "automation: !include no.yaml"}

tests/snapshots/test_config.ambr

@@ -451,3 +451,38 @@
     ''',
   ])
 # ---
+# name: test_individual_packages_schema_validation_errors[packages_dict]
+  list([
+    "Setup of package 'should_be_a_dict' at configuration.yaml, line 3 failed: Invalid package definition 'should_be_a_dict': expected a dictionary. Package will not be initialized",
+  ])
+# ---
+# name: test_individual_packages_schema_validation_errors[packages_slug]
+  list([
+    "Setup of package 'this is not a slug but it should be one' at configuration.yaml, line 3 failed: Invalid package definition 'this is not a slug but it should be one': invalid slug this is not a slug but it should be one (try this_is_not_a_slug_but_it_should_be_one). Package will not be initialized",
+  ])
+# ---
+# name: test_individual_packages_schema_validation_errors[packages_include_dir_named_dict]
+  list([
+    "Setup of package 'should_be_a_dict' at packages/expected_dict.yaml, line 1 failed: Invalid package definition 'should_be_a_dict': expected a dictionary. Package will not be initialized",
+  ])
+# ---
+# name: test_individual_packages_schema_validation_errors[packages_include_dir_named_slug]
+  list([
+    "Setup of package 'this is not a slug but it should be one' at packages/expected_slug.yaml, line 1 failed: Invalid package definition 'this is not a slug but it should be one': invalid slug this is not a slug but it should be one (try this_is_not_a_slug_but_it_should_be_one). Package will not be initialized",
+  ])
+# ---
+# name: test_packages_schema_validation_error[packages_is_a_list]
+  list([
+    "Invalid package configuration 'packages' at configuration.yaml, line 2: expected a dictionary",
+  ])
+# ---
+# name: test_packages_schema_validation_error[packages_is_a_value]
+  list([
+    "Invalid package configuration 'packages' at configuration.yaml, line 2: expected a dictionary",
+  ])
+# ---
+# name: test_packages_schema_validation_error[packages_is_null]
+  list([
+    "Invalid package configuration 'packages' at configuration.yaml, line 3: expected a dictionary",
+  ])
+# ---

tests/test_config.py

@@ -2233,6 +2233,79 @@ async def test_yaml_error(
     assert error_records == snapshot
 
 
+@pytest.mark.parametrize(
+    "config_dir",
+    [
+        "packages_dict",
+        "packages_slug",
+        "packages_include_dir_named_dict",
+        "packages_include_dir_named_slug",
+    ],
+)
+async def test_individual_packages_schema_validation_errors(
+    hass: HomeAssistant,
+    caplog: pytest.LogCaptureFixture,
+    config_dir: str,
+    mock_iot_domain_integration: Integration,
+    snapshot: SnapshotAssertion,
+) -> None:
+    """Tests syntactic errors in individual packages."""
+
+    base_path = os.path.dirname(__file__)
+    hass.config.config_dir = os.path.join(
+        base_path, "fixtures", "core", "config", "package_schema_validation", config_dir
+    )
+
+    config = await config_util.async_hass_config_yaml(hass)
+
+    error_records = [
+        record.message
+        for record in caplog.get_records("call")
+        if record.levelno == logging.ERROR
+    ]
+    assert error_records == snapshot
+
+    assert len(config["iot_domain"]) == 1
+
+
+@pytest.mark.parametrize(
+    "config_dir",
+    [
+        "packages_is_a_list",
+        "packages_is_a_value",
+        "packages_is_null",
+    ],
+)
+async def test_packages_schema_validation_error(
+    hass: HomeAssistant,
+    caplog: pytest.LogCaptureFixture,
+    config_dir: str,
+    snapshot: SnapshotAssertion,
+) -> None:
+    """Ensure that global package schema validation errors are logged."""
+
+    base_path = os.path.dirname(__file__)
+    hass.config.config_dir = os.path.join(
+        base_path,
+        "fixtures",
+        "core",
+        "config",
+        "package_schema_errors",
+        config_dir,
+    )
+
+    config = await config_util.async_hass_config_yaml(hass)
+
+    error_records = [
+        record.message
+        for record in caplog.get_records("call")
+        if record.levelno == logging.ERROR
+    ]
+    assert error_records == snapshot
+
+    assert len(config[config_util.CONF_CORE][config_util.CONF_PACKAGES]) == 0
+
+
 def test_extract_domain_configs() -> None:
     """Test the extraction of domain configuration."""
     config = {