Improve error message when a script fails to validate (#84438)

homeassistant/components/automation/config.py

@@ -211,6 +211,7 @@ async def _try_async_validate_config_item(
 
 async def async_validate_config_item(
     hass: HomeAssistant,
+    config_key: str,
     config: dict[str, Any],
 ) -> AutomationConfig | None:
     """Validate config item, called by EditAutomationConfigView."""

homeassistant/components/config/__init__.py

@@ -130,7 +130,7 @@ class BaseEditConfigView(HomeAssistantView):
             # We just validate, we don't store that data because
             # we don't want to store the defaults.
             if self.data_validator:
-                await self.data_validator(hass, data)
+                await self.data_validator(hass, config_key, data)
             else:
                 self.data_schema(data)
         except (vol.Invalid, HomeAssistantError) as err:

homeassistant/components/script/__init__.py

@@ -7,10 +7,9 @@ import logging
 from typing import Any, cast
 
 import voluptuous as vol
-from voluptuous.humanize import humanize_error
 
 from homeassistant.components import websocket_api
-from homeassistant.components.blueprint import CONF_USE_BLUEPRINT, BlueprintInputs
+from homeassistant.components.blueprint import CONF_USE_BLUEPRINT
 from homeassistant.const import (
     ATTR_ENTITY_ID,
     ATTR_MODE,
@@ -53,7 +52,7 @@ from homeassistant.helpers.typing import ConfigType
 from homeassistant.loader import bind_hass
 from homeassistant.util.dt import parse_datetime
 
-from .config import ScriptConfig, async_validate_config_item
+from .config import ScriptConfig
 from .const import (
     ATTR_LAST_ACTION,
     ATTR_LAST_TRIGGERED,
@@ -249,32 +248,11 @@ async def _prepare_script_config(
     """Parse configuration and prepare script entity configuration."""
     script_configs: list[ScriptEntityConfig] = []
 
-    conf: dict[str, dict[str, Any] | BlueprintInputs] = config[DOMAIN]
+    conf: dict[str, ConfigType] = config[DOMAIN]
 
     for key, config_block in conf.items():
-        raw_blueprint_inputs = None
+        raw_config = cast(ScriptConfig, config_block).raw_config
-        raw_config = None
+        raw_blueprint_inputs = cast(ScriptConfig, config_block).raw_blueprint_inputs
-
-        if isinstance(config_block, BlueprintInputs):
-            blueprint_inputs = config_block
-            raw_blueprint_inputs = blueprint_inputs.config_with_inputs
-
-            try:
-                raw_config = blueprint_inputs.async_substitute()
-                config_block = cast(
-                    dict[str, Any],
-                    await async_validate_config_item(hass, raw_config),
-                )
-            except vol.Invalid as err:
-                LOGGER.error(
-                    "Blueprint %s generated invalid script with input %s: %s",
-                    blueprint_inputs.blueprint.name,
-                    blueprint_inputs.inputs,
-                    humanize_error(config_block, err),
-                )
-                continue
-        else:
-            raw_config = cast(ScriptConfig, config_block).raw_config
 
         script_configs.append(
             ScriptEntityConfig(config_block, key, raw_blueprint_inputs, raw_config)

homeassistant/components/script/config.py

@@ -1,14 +1,19 @@
 """Config validation helper for the script integration."""
+from __future__ import annotations
+
+from collections.abc import Mapping
 from contextlib import suppress
+from typing import Any
 
 import voluptuous as vol
+from voluptuous.humanize import humanize_error
 
 from homeassistant.components.blueprint import (
-    BlueprintInputs,
+    BlueprintException,
     is_blueprint_instance_config,
 )
 from homeassistant.components.trace import TRACE_CONFIG_SCHEMA
-from homeassistant.config import async_log_exception, config_without_domain
+from homeassistant.config import config_without_domain
 from homeassistant.const import (
     CONF_ALIAS,
     CONF_DEFAULT,
@@ -19,6 +24,7 @@ from homeassistant.const import (
     CONF_SEQUENCE,
     CONF_VARIABLES,
 )
+from homeassistant.core import HomeAssistant
 from homeassistant.exceptions import HomeAssistantError
 from homeassistant.helpers import config_per_platform, config_validation as cv
 from homeassistant.helpers.script import (
@@ -27,6 +33,8 @@ from homeassistant.helpers.script import (
     make_script_schema,
 )
 from homeassistant.helpers.selector import validate_selector
+from homeassistant.helpers.typing import ConfigType
+from homeassistant.util.yaml.input import UndefinedSubstitution
 
 from .const import (
     CONF_ADVANCED,
@@ -65,45 +73,132 @@ SCRIPT_ENTITY_SCHEMA = make_script_schema(
 )
 
 
-async def async_validate_config_item(hass, config, full_config=None):
+async def _async_validate_config_item(
+    hass: HomeAssistant, object_id: str, config: ConfigType, warn_on_errors: bool
+) -> ScriptConfig:
     """Validate config item."""
+    raw_config = None
+    raw_blueprint_inputs = None
+    uses_blueprint = False
+    with suppress(ValueError):  # Invalid config
+        raw_config = dict(config)
+
+    def _log_invalid_script(
+        err: Exception,
+        script_name: str,
+        problem: str,
+        data: Any,
+    ) -> None:
+        """Log an error about invalid script."""
+        if not warn_on_errors:
+            return
+
+        if uses_blueprint:
+            LOGGER.error(
+                "Blueprint '%s' generated invalid script with inputs %s: %s",
+                blueprint_inputs.blueprint.name,
+                blueprint_inputs.inputs,
+                humanize_error(data, err) if isinstance(err, vol.Invalid) else err,
+            )
+            return
+
+        LOGGER.error(
+            "%s %s and has been disabled: %s",
+            script_name,
+            problem,
+            humanize_error(data, err) if isinstance(err, vol.Invalid) else err,
+        )
+        return
+
     if is_blueprint_instance_config(config):
+        uses_blueprint = True
         blueprints = async_get_blueprints(hass)
-        return await blueprints.async_inputs_from_config(config)
+        try:
+            blueprint_inputs = await blueprints.async_inputs_from_config(config)
+        except BlueprintException as err:
+            if warn_on_errors:
+                LOGGER.error(
+                    "Failed to generate script from blueprint: %s",
+                    err,
+                )
+            raise
 
-    config = SCRIPT_ENTITY_SCHEMA(config)
+        raw_blueprint_inputs = blueprint_inputs.config_with_inputs
-    config[CONF_SEQUENCE] = await async_validate_actions_config(
-        hass, config[CONF_SEQUENCE]
-    )
 
-    return config
+        try:
+            config = blueprint_inputs.async_substitute()
+            raw_config = dict(config)
+        except UndefinedSubstitution as err:
+            if warn_on_errors:
+                LOGGER.error(
+                    "Blueprint '%s' failed to generate script with inputs %s: %s",
+                    blueprint_inputs.blueprint.name,
+                    blueprint_inputs.inputs,
+                    err,
+                )
+            raise HomeAssistantError from err
+
+    script_name = f"Script with object id '{object_id}'"
+    if isinstance(config, Mapping):
+        if CONF_ALIAS in config:
+            script_name = f"Script with alias '{config[CONF_ALIAS]}'"
+
+    try:
+        cv.slug(object_id)
+    except vol.Invalid as err:
+        _log_invalid_script(err, script_name, "has invalid object id", object_id)
+        raise
+    try:
+        validated_config = SCRIPT_ENTITY_SCHEMA(config)
+    except vol.Invalid as err:
+        _log_invalid_script(err, script_name, "could not be validated", config)
+        raise
+
+    try:
+        validated_config[CONF_SEQUENCE] = await async_validate_actions_config(
+            hass, validated_config[CONF_SEQUENCE]
+        )
+    except (
+        vol.Invalid,
+        HomeAssistantError,
+    ) as err:
+        _log_invalid_script(
+            err, script_name, "failed to setup actions", validated_config
+        )
+        raise
+
+    script_config = ScriptConfig(validated_config)
+    script_config.raw_blueprint_inputs = raw_blueprint_inputs
+    script_config.raw_config = raw_config
+    return script_config
 
 
 class ScriptConfig(dict):
     """Dummy class to allow adding attributes."""
 
-    raw_config = None
+    raw_config: ConfigType | None = None
+    raw_blueprint_inputs: ConfigType | None = None
 
 
-async def _try_async_validate_config_item(hass, object_id, config, full_config=None):
+async def _try_async_validate_config_item(
+    hass: HomeAssistant,
+    object_id: str,
+    config: ConfigType,
+) -> ScriptConfig | None:
     """Validate config item."""
-    raw_config = None
-    with suppress(ValueError):  # Invalid config
-        raw_config = dict(config)
-
     try:
-        cv.slug(object_id)
+        return await _async_validate_config_item(hass, object_id, config, True)
-        config = await async_validate_config_item(hass, config, full_config)
+    except (vol.Invalid, HomeAssistantError):
-    except (vol.Invalid, HomeAssistantError) as ex:
-        async_log_exception(ex, DOMAIN, full_config or config, hass)
         return None
 
-    if isinstance(config, BlueprintInputs):
-        return config
 
-    config = ScriptConfig(config)
+async def async_validate_config_item(
-    config.raw_config = raw_config
+    hass: HomeAssistant,
-    return config
+    object_id: str,
+    config: dict[str, Any],
+) -> ScriptConfig | None:
+    """Validate config item, called by EditScriptConfigView."""
+    return await _async_validate_config_item(hass, object_id, config, False)
 
 
 async def async_validate_config(hass, config):
@@ -114,7 +209,7 @@ async def async_validate_config(hass, config):
             if object_id in scripts:
                 LOGGER.warning("Duplicate script detected with name: '%s'", object_id)
                 continue
-            cfg = await _try_async_validate_config_item(hass, object_id, cfg, config)
+            cfg = await _try_async_validate_config_item(hass, object_id, cfg)
             if cfg is not None:
                 scripts[object_id] = cfg
 

tests/components/config/test_script.py

@@ -71,6 +71,36 @@ async def test_update_script_config(
     assert new_data["moon"] == {"alias": "Moon updated", "sequence": []}
 
 
+@pytest.mark.parametrize("script_config", ({},))
+async def test_update_script_config_with_error(
+    hass: HomeAssistant, hass_client, hass_config_store, caplog
+):
+    """Test updating script config with errors."""
+    with patch.object(config, "SECTIONS", ["script"]):
+        await async_setup_component(hass, "config", {})
+
+    assert sorted(hass.states.async_entity_ids("script")) == []
+
+    client = await hass_client()
+
+    orig_data = {"sun": {}, "moon": {}}
+    hass_config_store["scripts.yaml"] = orig_data
+
+    resp = await client.post(
+        "/api/config/script/config/moon",
+        data=json.dumps({}),
+    )
+    await hass.async_block_till_done()
+    assert sorted(hass.states.async_entity_ids("script")) == []
+
+    assert resp.status != HTTPStatus.OK
+    result = await resp.json()
+    validation_error = "required key not provided @ data['sequence']"
+    assert result == {"message": f"Message malformed: {validation_error}"}
+    # Assert the validation error is not logged
+    assert validation_error not in caplog.text
+
+
 @pytest.mark.parametrize("script_config", ({},))
 async def test_update_remove_key_script_config(
     hass: HomeAssistant, hass_client, hass_config_store

tests/components/script/test_init.py

@@ -38,6 +38,7 @@ from homeassistant.helpers.script import (
 )
 from homeassistant.helpers.service import async_get_all_descriptions
 from homeassistant.setup import async_setup_component
+from homeassistant.util import yaml
 import homeassistant.util.dt as dt_util
 
 from tests.common import async_fire_time_changed, async_mock_service, mock_restore_cache
@@ -166,6 +167,67 @@ async def test_setup_with_invalid_configs(hass, value):
     assert len(hass.states.async_entity_ids("script")) == 0
 
 
+@pytest.mark.parametrize(
+    "object_id, broken_config, problem, details",
+    (
+        (
+            "Bad Script",
+            {},
+            "has invalid object id",
+            "invalid slug Bad Script",
+        ),
+        (
+            "bad_script",
+            {},
+            "could not be validated",
+            "required key not provided @ data['sequence']",
+        ),
+        (
+            "bad_script",
+            {
+                "sequence": {
+                    "condition": "state",
+                    # The UUID will fail being resolved to en entity_id
+                    "entity_id": "abcdabcdabcdabcdabcdabcdabcdabcd",
+                    "state": "blah",
+                },
+            },
+            "failed to setup actions",
+            "Unknown entity registry entry abcdabcdabcdabcdabcdabcdabcdabcd.",
+        ),
+    ),
+)
+async def test_bad_config_validation(
+    hass: HomeAssistant, caplog, object_id, broken_config, problem, details
+):
+    """Test bad script configuration which can be detected during validation."""
+    assert await async_setup_component(
+        hass,
+        script.DOMAIN,
+        {
+            script.DOMAIN: {
+                object_id: {"alias": "bad_script", **broken_config},
+                "good_script": {
+                    "alias": "good_script",
+                    "sequence": {
+                        "service": "test.automation",
+                        "entity_id": "hello.world",
+                    },
+                },
+            }
+        },
+    )
+
+    # Check we get the expected error message
+    assert (
+        f"Script with alias 'bad_script' {problem} and has been disabled: {details}"
+        in caplog.text
+    )
+
+    # Make sure one bad automation does not prevent other automations from setting up
+    assert hass.states.async_entity_ids("script") == ["script.good_script"]
+
+
 @pytest.mark.parametrize("running", ["no", "same", "different"])
 async def test_reload_service(hass, running):
     """Verify the reload service."""
@@ -1248,3 +1310,81 @@ async def test_script_service_changed_entity_id(hass: HomeAssistant) -> None:
 
     assert len(calls) == 2
     assert calls[1].data["entity_id"] == "script.custom_entity_id_2"
+
+
+@pytest.mark.parametrize(
+    "blueprint_inputs, problem, details",
+    (
+        (
+            # No input
+            {},
+            "Failed to generate script from blueprint",
+            "Missing input service_to_call",
+        ),
+        (
+            # Missing input
+            {"a_number": 5},
+            "Failed to generate script from blueprint",
+            "Missing input service_to_call",
+        ),
+        (
+            # Wrong input
+            {
+                "trigger_event": "blueprint_event",
+                "service_to_call": {"dict": "not allowed"},
+                "a_number": 5,
+            },
+            "Blueprint 'Call service' generated invalid script",
+            "value should be a string for dictionary value @ data['sequence'][0]['service']",
+        ),
+    ),
+)
+async def test_blueprint_script_bad_config(
+    hass, caplog, blueprint_inputs, problem, details
+):
+    """Test blueprint script with bad inputs."""
+    assert await async_setup_component(
+        hass,
+        script.DOMAIN,
+        {
+            script.DOMAIN: {
+                "test_script": {
+                    "use_blueprint": {
+                        "path": "test_service.yaml",
+                        "input": blueprint_inputs,
+                    }
+                }
+            }
+        },
+    )
+    assert problem in caplog.text
+    assert details in caplog.text
+
+
+async def test_blueprint_script_fails_substitution(hass, caplog):
+    """Test blueprint script with bad inputs."""
+    with patch(
+        "homeassistant.components.blueprint.models.BlueprintInputs.async_substitute",
+        side_effect=yaml.UndefinedSubstitution("blah"),
+    ):
+        assert await async_setup_component(
+            hass,
+            script.DOMAIN,
+            {
+                script.DOMAIN: {
+                    "test_script": {
+                        "use_blueprint": {
+                            "path": "test_service.yaml",
+                            "input": {
+                                "service_to_call": "test.automation",
+                            },
+                        }
+                    }
+                }
+            },
+        )
+    assert (
+        "Blueprint 'Call service' failed to generate script with inputs "
+        "{'service_to_call': 'test.automation'}: No substitution found for input blah"
+        in caplog.text
+    )