diff --git a/esphome/components/lvgl/widgets/arc.py b/esphome/components/lvgl/widgets/arc.py index 9eaf3dadce..ac993cc382 100644 --- a/esphome/components/lvgl/widgets/arc.py +++ b/esphome/components/lvgl/widgets/arc.py @@ -77,8 +77,11 @@ class ArcType(NumberType): # start_angle and end_angle are mapped to bg_start_angle and bg_end_angle prop = str(prop) if prop.endswith("_angle"): - prop = "bg_" + prop - await w.set_property(prop, config, processor=validator) + await w.set_property( + "bg_" + prop, await validator.process(config.get(prop)) + ) + else: + await w.set_property(prop, config, processor=validator) if CONF_ADJUSTABLE in config: if not config[CONF_ADJUSTABLE]: lv_obj.remove_style(w.obj, nullptr, LV_PART.KNOB) diff --git a/esphome/components/packages/__init__.py b/esphome/components/packages/__init__.py index 3f3df75351..97a5309480 100644 --- a/esphome/components/packages/__init__.py +++ b/esphome/components/packages/__init__.py @@ -8,8 +8,11 @@ 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, ) from esphome.components.substitutions.jinja import has_jinja @@ -359,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 @@ -516,7 +526,12 @@ def do_packages_pass( if CONF_PACKAGES not in config: return config - substitutions = UserDict(config.pop(CONF_SUBSTITUTIONS, {})) + with cv.prepend_path(CONF_SUBSTITUTIONS): + substitutions = UserDict( + resolve_substitutions_block( + config.pop(CONF_SUBSTITUTIONS, {}), command_line_substitutions + ) + ) processor = _PackageProcessor( substitutions, command_line_substitutions, skip_update ) diff --git a/esphome/components/substitutions/__init__.py b/esphome/components/substitutions/__init__.py index 94aebbbfe3..8bbccffca1 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) @@ -414,6 +464,34 @@ def _warn_unresolved_variables(errors: ErrList) -> None: ) +def resolve_substitutions_block( + substitutions: Any, + command_line_substitutions: dict[str, Any] | None, +) -> dict[str, Any]: + """Resolve a deferred ``substitutions: !include file.yaml`` and validate the shape. + + The caller is responsible for wrapping the call in + ``cv.prepend_path(CONF_SUBSTITUTIONS)`` for error reporting. + ``command_line_substitutions`` seeds the filename context so + ``substitutions: !include ${var}.yaml`` can reference CLI-provided vars. + """ + if isinstance(substitutions, IncludeFile): + # Single-shot resolution — matches ``_walk_packages`` for the + # ``packages: !include`` entry point. Chained includes (an include that + # itself loads another ``!include`` at the top level) are not supported. + substitutions, _ = resolve_include( + substitutions, + [], + ContextVars(command_line_substitutions or {}), + strict_undefined=False, + ) + if not isinstance(substitutions, dict): + raise cv.Invalid( + f"Substitutions must be a key to value mapping, got {type(substitutions)}" + ) + return substitutions + + def do_substitution_pass( config: OrderedDict, command_line_substitutions: dict[str, Any] | None = None ) -> OrderedDict: @@ -429,10 +507,9 @@ def do_substitution_pass( # Use merge_dicts_ordered to preserve OrderedDict type for move_to_end() substitutions = config.pop(CONF_SUBSTITUTIONS, {}) with cv.prepend_path(CONF_SUBSTITUTIONS): - if not isinstance(substitutions, dict): - raise cv.Invalid( - f"Substitutions must be a key to value mapping, got {type(substitutions)}" - ) + substitutions = resolve_substitutions_block( + substitutions, command_line_substitutions + ) substitutions = merge_dicts_ordered( substitutions, command_line_substitutions or {} ) diff --git a/esphome/core/component.h b/esphome/core/component.h index 717ca36257..67db5423af 100644 --- a/esphome/core/component.h +++ b/esphome/core/component.h @@ -601,7 +601,7 @@ class Component { */ class PollingComponent : public Component { public: - PollingComponent() : PollingComponent(1) {} + PollingComponent() : PollingComponent(SCHEDULER_DONT_RUN) {} /** Initialize this polling component with the given update interval in ms. * diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index cd91c4d8cb..0bd339efa9 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/fixtures/substitutions/15-substitutions_as_include.approved.yaml b/tests/unit_tests/fixtures/substitutions/15-substitutions_as_include.approved.yaml new file mode 100644 index 0000000000..14aa707def --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/15-substitutions_as_include.approved.yaml @@ -0,0 +1,5 @@ +substitutions: + wifi_password: sub_password +wifi: + ssid: main_ssid + password: sub_password diff --git a/tests/unit_tests/fixtures/substitutions/15-substitutions_as_include.input.yaml b/tests/unit_tests/fixtures/substitutions/15-substitutions_as_include.input.yaml new file mode 100644 index 0000000000..5909e7bf4f --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/15-substitutions_as_include.input.yaml @@ -0,0 +1,5 @@ +substitutions: !include 15-substitutions_inc.yaml + +wifi: + ssid: main_ssid + password: $wifi_password diff --git a/tests/unit_tests/fixtures/substitutions/15-substitutions_inc.yaml b/tests/unit_tests/fixtures/substitutions/15-substitutions_inc.yaml new file mode 100644 index 0000000000..44d9a4b9ef --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/15-substitutions_inc.yaml @@ -0,0 +1 @@ +wifi_password: sub_password diff --git a/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.approved.yaml b/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.approved.yaml new file mode 100644 index 0000000000..14aa707def --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.approved.yaml @@ -0,0 +1,5 @@ +substitutions: + wifi_password: sub_password +wifi: + ssid: main_ssid + password: sub_password diff --git a/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.input.yaml b/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.input.yaml new file mode 100644 index 0000000000..a2e72f33a2 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/16-substitutions_as_include_with_packages.input.yaml @@ -0,0 +1,9 @@ +substitutions: !include 15-substitutions_inc.yaml + +packages: + wifi_pkg: + wifi: + password: $wifi_password + +wifi: + ssid: main_ssid diff --git a/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.approved.yaml b/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.approved.yaml new file mode 100644 index 0000000000..f1fd5fb078 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.approved.yaml @@ -0,0 +1,6 @@ +substitutions: + subs_file: 15-substitutions_inc + wifi_password: sub_password +wifi: + ssid: main_ssid + password: sub_password diff --git a/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.input.yaml b/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.input.yaml new file mode 100644 index 0000000000..3248504b46 --- /dev/null +++ b/tests/unit_tests/fixtures/substitutions/17-substitutions_include_cli_var.input.yaml @@ -0,0 +1,8 @@ +command_line_substitutions: + subs_file: 15-substitutions_inc + +substitutions: !include ${subs_file}.yaml + +wifi: + ssid: main_ssid + password: $wifi_password diff --git a/tests/unit_tests/test_substitutions.py b/tests/unit_tests/test_substitutions.py index 01c669e542..3599e703d9 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,90 @@ 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: + """`substitutions: !include list.yaml` where the file holds a list raises cv.Invalid. + + Locks in the shape check that runs after the deferred IncludeFile has been + resolved. + """ + parent = tmp_path / "main.yaml" + parent.write_text("") + + def loader(path: Path): + return ["not", "a", "mapping"] + + include = yaml_util.IncludeFile(parent, "subs.yaml", None, loader) + config = OrderedDict({CONF_SUBSTITUTIONS: include}) + + with pytest.raises( + cv.Invalid, match="Substitutions must be a key to value mapping" + ): + substitutions.do_substitution_pass(config) + + +def test_do_packages_pass_included_substitutions_must_be_mapping( + tmp_path: Path, +) -> None: + """`substitutions: !include list.yaml` alongside `packages:` raises cv.Invalid. + + Without the shape check, ``UserDict(...)`` would surface a low-level + ``TypeError``; the explicit ``cv.Invalid`` points at the substitutions path. + """ + parent = tmp_path / "main.yaml" + parent.write_text("") + + def loader(path: Path): + return ["not", "a", "mapping"] + + include = yaml_util.IncludeFile(parent, "subs.yaml", None, loader) + config = OrderedDict( + { + CONF_SUBSTITUTIONS: include, + "packages": {"noop": {"wifi": {"ssid": "main"}}}, + } + ) + + with pytest.raises( + cv.Invalid, match="Substitutions must be a key to value mapping" + ): + do_packages_pass(config) + + def test_resolve_package_undefined_var_in_include_filename(tmp_path: Path) -> None: """An undefined substitution in a package include filename raises cv.Invalid.