diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index 252a24061a0..97a53094800 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -8,7 +8,9 @@ from typing import Any from esphome import git, yaml_util from esphome.components.substitutions import ( ContextVars, + ErrList, push_context, + raise_first_undefined, resolve_include, resolve_substitutions_block, substitute, @@ -360,12 +362,19 @@ def _substitute_package_definition( if isinstance(package_config, str) or ( isinstance(package_config, dict) and is_remote_package(package_config) ): + # Collect undefined-variable errors (rather than raising strict) so the + # path walked through a remote-package dict is preserved and the user + # sees which field (url / path / ref / ...) referenced the undefined + # variable. + errors: ErrList = [] package_config = substitute( item=package_config, path=[], parent_context=context_vars or ContextVars(), strict_undefined=False, + errors=errors, ) + raise_first_undefined(errors, package_config, "package definition") return package_config diff --git a/esphome/components/substitutions/__init__.py b/esphome/components/substitutions/__init__.py index 7aace9bb648..0144c13c017 100644 --- a/esphome/components/substitutions/__init__.py +++ b/esphome/components/substitutions/__init__.py @@ -30,6 +30,56 @@ ErrList = list[tuple[UndefinedError, SubstitutionPath, Any]] jinja = Jinja() +def raise_first_undefined( + errors: ErrList, + source: Any, + context_label: str, +) -> None: + """If *errors* is non-empty, raise ``cv.Invalid`` for the first undefined variable. + + The raised error names the missing variable, the path walked into *source* + (for nested dicts, e.g. ``url`` or ``ref``), and the YAML source location + when *source* carries one. Only the first error is surfaced; the user will + re-run after fixing it and any remaining undefined variables will be + reported then. + + ``context_label`` is the noun describing where the undefined variable + appeared (e.g. ``"package definition"``). + """ + if not errors: + return + err, err_path, err_value = errors[0] + if len(errors) > 1: + # Log any further undefined variables so debug-level output covers + # the full set, even though only the first is surfaced to the user. + extras = ", ".join( + f"{e.message} at '{'->'.join(str(p) for p in p_path)}'" + for e, p_path, _ in errors[1:] + ) + _LOGGER.debug("Additional undefined variables in %s: %s", context_label, extras) + # Prefer the location of the offending scalar (e.g. the `url:` value) over + # the enclosing package-definition dict so the message points at the exact + # line/column that carries the undefined variable. + location_node = ( + err_value + if isinstance(err_value, ESPHomeDataBase) and err_value.esp_range is not None + else source + ) + location = "" + if ( + isinstance(location_node, ESPHomeDataBase) + and location_node.esp_range is not None + ): + mark = location_node.esp_range.start_mark + # DocumentLocation.line/column are 0-based (from the YAML Mark). Render + # as 1-based to match config.line_info() and editor line numbering. + location = f" (in {mark.document} {mark.line + 1}:{mark.column + 1})" + field = f" at '{'->'.join(str(p) for p in err_path)}'" if err_path else "" + raise cv.Invalid( + f"Undefined variable in {context_label}{field}: {err.message}{location}" + ) + + def validate_substitution_key(value: Any) -> str: """Validate and normalize a substitution key, stripping a leading ``$`` if present.""" value = cv.string(value) diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index cd91c4d8cb1..0bd339efa9c 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -2,18 +2,20 @@ import logging from pathlib import Path +import re from unittest.mock import MagicMock, patch import pytest from esphome.components.packages import ( CONFIG_SCHEMA, + _substitute_package_definition, _walk_packages, do_packages_pass, is_package_definition, merge_packages, ) -from esphome.components.substitutions import do_substitution_pass +from esphome.components.substitutions import ContextVars, do_substitution_pass import esphome.config as config_module from esphome.config import resolve_extend_remove from esphome.config_helpers import Extend, Remove @@ -44,7 +46,7 @@ from esphome.const import ( ) from esphome.core import CORE from esphome.util import OrderedDict -from esphome.yaml_util import IncludeFile, add_context +from esphome.yaml_util import IncludeFile, add_context, load_yaml # Test strings TEST_DEVICE_NAME = "test_device_name" @@ -1399,3 +1401,85 @@ def test_raw_config_contains_merged_esphome_from_package(tmp_path) -> None: "CORE.raw_config should contain esphome section after package merge" ) assert CORE.raw_config[CONF_ESPHOME][CONF_NAME] == TEST_DEVICE_NAME + + +# --------------------------------------------------------------------------- +# _substitute_package_definition +# --------------------------------------------------------------------------- + + +def test_substitute_package_definition_local_dict_returned_unchanged() -> None: + """A plain local config dict is not substituted and is returned as-is.""" + pkg = {CONF_WIFI: {CONF_SSID: "test"}} + result = _substitute_package_definition(pkg, ContextVars()) + assert result is pkg + + +def test_substitute_package_definition_string_resolved_with_context() -> None: + """A string package definition has its variables substituted.""" + ctx = ContextVars({"variant": "esp32"}) + result = _substitute_package_definition("device-${variant}.yaml", ctx) + assert result == "device-esp32.yaml" + + +def test_substitute_package_definition_undefined_in_string() -> None: + """An undefined variable in a package URL string raises cv.Invalid.""" + with pytest.raises(cv.Invalid, match="Undefined variable in package definition"): + _substitute_package_definition( + "github://org/repo/${undefined_var}/pkg.yaml", ContextVars() + ) + + +def test_substitute_package_definition_undefined_in_remote_dict_field() -> None: + """An undefined variable inside a remote-dict field names the offending field.""" + with pytest.raises(cv.Invalid) as exc_info: + _substitute_package_definition( + {CONF_URL: "github://${typo}/repo"}, ContextVars() + ) + err = str(exc_info.value) + assert "'typo' is undefined" in err + assert CONF_URL in err + + +def test_substitute_package_definition_undefined_in_remote_dict_non_first_field() -> ( + None +): + """The field path joins correctly for non-first dict fields (e.g. ``ref``).""" + with pytest.raises(cv.Invalid) as exc_info: + _substitute_package_definition( + { + CONF_URL: "github://org/repo", + CONF_REF: "branch-${branch_typo}", + }, + ContextVars(), + ) + err = str(exc_info.value) + assert "'branch_typo' is undefined" in err + assert CONF_REF in err + + +def test_substitute_package_definition_includes_source_location(tmp_path: Path) -> None: + """A package loaded from YAML surfaces file/line/col in the cv.Invalid message. + + Line/column are rendered 1-based (matching config.line_info() and editor + line numbering) and point at the offending scalar, not the enclosing dict. + """ + yaml_file = tmp_path / "main.yaml" + yaml_file.write_text( + "packages:\n broken: github://org/repo/${undefined_var}/pkg.yaml\n" + ) + config = load_yaml(yaml_file) + package_config = config[CONF_PACKAGES]["broken"] + + with pytest.raises(cv.Invalid) as exc_info: + _substitute_package_definition(package_config, ContextVars()) + + err = str(exc_info.value) + assert "main.yaml" in err + # The offending value lives on line 2 (1-based). Column depends on the YAML + # loader, so we only pin line and check that a 1-based column is present. + match = re.search(r"main\.yaml (\d+):(\d+)", err) + assert match, err + line, col = int(match.group(1)), int(match.group(2)) + assert line == 2, f"expected 1-based line 2, got {line} (err={err!r})" + assert col >= 1, f"expected 1-based column ≥ 1, got {col} (err={err!r})" diff --git a/tests/unit_tests/test_substitutions.py b/tests/unit_tests/test_substitutions.py index 71bbd9db86d..3599e703d9d 100644 --- a/tests/unit_tests/test_substitutions.py +++ b/tests/unit_tests/test_substitutions.py @@ -14,6 +14,7 @@ from esphome.components.packages import ( do_packages_pass, merge_packages, ) +from esphome.components.substitutions.jinja import UndefinedError from esphome.config import resolve_extend_remove from esphome.config_helpers import Extend, merge_config import esphome.config_validation as cv @@ -675,6 +676,39 @@ def test_include_filename_substitution_undefined_var(tmp_path: Path) -> None: substitutions.do_substitution_pass(config) +def test_raise_first_undefined_logs_extras_at_debug( + caplog: pytest.LogCaptureFixture, +) -> None: + """Only the first undefined error is raised; extras are logged at debug.""" + errors: substitutions.ErrList = [ + (UndefinedError("'a' is undefined"), ["url"], None), + (UndefinedError("'b' is undefined"), ["ref"], None), + (UndefinedError("'c' is undefined"), ["path"], None), + ] + + with ( + caplog.at_level(logging.DEBUG, logger="esphome.components.substitutions"), + pytest.raises(cv.Invalid) as exc_info, + ): + substitutions.raise_first_undefined(errors, None, "package definition") + + # First error is surfaced as the cv.Invalid message. + raised = str(exc_info.value) + assert "'a' is undefined" in raised + assert "'b' is undefined" not in raised + assert "'c' is undefined" not in raised + + # Remaining errors are captured via debug logging for troubleshooting. + assert "Additional undefined variables in package definition" in caplog.text + assert "'b' is undefined at 'ref'" in caplog.text + assert "'c' is undefined at 'path'" in caplog.text + + +def test_raise_first_undefined_noop_on_empty() -> None: + """An empty errors list is a no-op — no exception, no log.""" + substitutions.raise_first_undefined([], None, "package definition") + + def test_do_substitution_pass_included_substitutions_must_be_mapping( tmp_path: Path, ) -> None: