mirror of
https://github.com/esphome/esphome.git
synced 2026-05-20 17:52:00 +08:00
[packages] Fix false deprecation warning and wrong error paths in nested packages (#15605)
This commit is contained in:
@@ -45,6 +45,18 @@ def is_remote_package(package_config: dict) -> bool:
|
||||
return CONF_URL in package_config
|
||||
|
||||
|
||||
def is_package_definition(value: object) -> bool:
|
||||
"""Returns True if the value looks like a package definition rather than a config fragment.
|
||||
|
||||
Package definitions are IncludeFile objects, git URL shorthand strings, or
|
||||
remote package dicts (containing a ``url:`` key). Config fragments are
|
||||
plain dicts that represent component configuration.
|
||||
"""
|
||||
return isinstance(value, (yaml_util.IncludeFile, str)) or (
|
||||
isinstance(value, dict) and is_remote_package(value)
|
||||
)
|
||||
|
||||
|
||||
def valid_package_contents(package_config: dict) -> dict:
|
||||
"""Validate that a package looks like a plausible ESPHome config fragment.
|
||||
|
||||
@@ -318,11 +330,11 @@ def _walk_packages(
|
||||
if not isinstance(packages, dict):
|
||||
_walk_package_list(packages, callback, context)
|
||||
elif (result := _walk_package_dict(packages, callback, context)) is not None:
|
||||
if not validate_deprecated:
|
||||
if not validate_deprecated or any(
|
||||
is_package_definition(v) for v in packages.values()
|
||||
):
|
||||
raise result
|
||||
# Fallback: treat the dict as a single deprecated package.
|
||||
# Note: this catches *any* cv.Invalid from the callback, which may
|
||||
# mask real validation errors in named package dicts.
|
||||
# This block can be removed once the single-package
|
||||
# deprecation period (2026.7.0) is over.
|
||||
config[CONF_PACKAGES] = [packages]
|
||||
@@ -461,6 +473,9 @@ class _PackageProcessor:
|
||||
self, package_config: dict | str, context_vars: ContextVars | None
|
||||
) -> dict:
|
||||
"""Resolve a single package and recurse into any nested packages."""
|
||||
from_remote = isinstance(package_config, dict) and is_remote_package(
|
||||
package_config
|
||||
)
|
||||
package_config = self.resolve_package(package_config, context_vars)
|
||||
self.collect_substitutions(package_config)
|
||||
|
||||
@@ -470,7 +485,18 @@ class _PackageProcessor:
|
||||
# Push context from !include vars on the package root and on the packages key
|
||||
context_vars = push_context(package_config, context_vars)
|
||||
context_vars = push_context(package_config[CONF_PACKAGES], context_vars)
|
||||
return _walk_packages(package_config, self.process_package, context_vars)
|
||||
# Disable the deprecated single-package fallback for remote
|
||||
# packages. _process_remote_package returns dicts with
|
||||
# already-resolved values that is_package_definition cannot
|
||||
# distinguish from config fragments, so the fallback would
|
||||
# always fire and mask real errors with wrong paths
|
||||
# (packages->0 instead of packages-><name>).
|
||||
return _walk_packages(
|
||||
package_config,
|
||||
self.process_package,
|
||||
context_vars,
|
||||
validate_deprecated=not from_remote,
|
||||
)
|
||||
|
||||
|
||||
def do_packages_pass(
|
||||
|
||||
@@ -1,11 +1,18 @@
|
||||
"""Tests for the packages component."""
|
||||
|
||||
import logging
|
||||
from pathlib import Path
|
||||
from unittest.mock import MagicMock, patch
|
||||
|
||||
import pytest
|
||||
|
||||
from esphome.components.packages import CONFIG_SCHEMA, do_packages_pass, merge_packages
|
||||
from esphome.components.packages import (
|
||||
CONFIG_SCHEMA,
|
||||
_walk_packages,
|
||||
do_packages_pass,
|
||||
is_package_definition,
|
||||
merge_packages,
|
||||
)
|
||||
from esphome.components.substitutions import do_substitution_pass
|
||||
import esphome.config as config_module
|
||||
from esphome.config import resolve_extend_remove
|
||||
@@ -37,7 +44,7 @@ from esphome.const import (
|
||||
)
|
||||
from esphome.core import CORE
|
||||
from esphome.util import OrderedDict
|
||||
from esphome.yaml_util import add_context
|
||||
from esphome.yaml_util import IncludeFile, add_context
|
||||
|
||||
# Test strings
|
||||
TEST_DEVICE_NAME = "test_device_name"
|
||||
@@ -79,6 +86,44 @@ def packages_pass(config):
|
||||
return config
|
||||
|
||||
|
||||
_INCLUDE_FILE = "INCLUDE_FILE"
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
("value", "expected"),
|
||||
[
|
||||
# IncludeFile objects are package definitions
|
||||
(_INCLUDE_FILE, True),
|
||||
# Git URL shorthand strings are package definitions
|
||||
("github://esphome/firmware/base.yaml@main", True),
|
||||
# Remote package dicts (with url key) are package definitions
|
||||
({"url": "https://github.com/esphome/firmware", "file": "base.yaml"}, True),
|
||||
# Plain config dicts are NOT package definitions (they are config fragments)
|
||||
({"wifi": {"ssid": "test"}}, False),
|
||||
# None is not a package definition
|
||||
(None, False),
|
||||
# Lists are not package definitions
|
||||
([{"wifi": {"ssid": "test"}}], False),
|
||||
# Empty dicts are not package definitions
|
||||
({}, False),
|
||||
],
|
||||
ids=[
|
||||
"include_file",
|
||||
"git_shorthand",
|
||||
"remote_package",
|
||||
"config_fragment",
|
||||
"none",
|
||||
"list",
|
||||
"empty_dict",
|
||||
],
|
||||
)
|
||||
def test_is_package_definition(value: object, expected: bool) -> None:
|
||||
"""Test that is_package_definition correctly identifies package definitions."""
|
||||
if value is _INCLUDE_FILE:
|
||||
value = MagicMock(spec=IncludeFile)
|
||||
assert is_package_definition(value) is expected
|
||||
|
||||
|
||||
def test_package_unused(basic_esphome, basic_wifi) -> None:
|
||||
"""
|
||||
Ensures do_package_pass does not change a config if packages aren't used.
|
||||
@@ -1107,6 +1152,134 @@ def test_invalid_package_contents_masked_by_deprecation(
|
||||
do_packages_pass(config)
|
||||
|
||||
|
||||
def test_named_dict_with_include_files_no_false_deprecation_warning(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""Package errors in named dicts must not trigger the deprecated fallback."""
|
||||
good_include = MagicMock(spec=IncludeFile)
|
||||
bad_include = MagicMock(spec=IncludeFile)
|
||||
|
||||
config = {
|
||||
CONF_PACKAGES: {
|
||||
"good_pkg": good_include,
|
||||
"bad_pkg": bad_include,
|
||||
},
|
||||
}
|
||||
|
||||
call_count = 0
|
||||
|
||||
def failing_callback(package_config: dict, context: object) -> dict:
|
||||
nonlocal call_count
|
||||
call_count += 1
|
||||
if call_count == 1:
|
||||
# First package processes fine
|
||||
return {CONF_WIFI: {CONF_SSID: "test"}}
|
||||
# Second package has an error (e.g. jinja syntax error)
|
||||
raise cv.Invalid("simulated jinja error in bad_pkg")
|
||||
|
||||
with (
|
||||
caplog.at_level(logging.WARNING),
|
||||
pytest.raises(cv.Invalid, match="simulated jinja error"),
|
||||
):
|
||||
_walk_packages(config, failing_callback)
|
||||
|
||||
# Must NOT emit the deprecated single-package warning
|
||||
assert "deprecated" not in caplog.text.lower()
|
||||
|
||||
|
||||
def test_validate_deprecated_false_raises_directly(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""With validate_deprecated=False, errors raise directly without fallback.
|
||||
|
||||
This is the codepath used for remote packages where _process_remote_package
|
||||
returns already-resolved dicts that is_package_definition cannot detect.
|
||||
"""
|
||||
config = {
|
||||
CONF_PACKAGES: {
|
||||
"pkg_a": {CONF_WIFI: {CONF_SSID: "test"}},
|
||||
"pkg_b": {CONF_WIFI: {CONF_SSID: "test2"}},
|
||||
},
|
||||
}
|
||||
|
||||
call_count = 0
|
||||
|
||||
def failing_callback(package_config: dict, context: object) -> dict:
|
||||
nonlocal call_count
|
||||
call_count += 1
|
||||
if call_count == 1:
|
||||
return package_config
|
||||
raise cv.Invalid("nested error")
|
||||
|
||||
with (
|
||||
caplog.at_level(logging.WARNING),
|
||||
pytest.raises(cv.Invalid, match="nested error"),
|
||||
):
|
||||
_walk_packages(config, failing_callback, validate_deprecated=False)
|
||||
|
||||
assert "deprecated" not in caplog.text.lower()
|
||||
|
||||
|
||||
def test_error_on_first_declared_package_still_detected() -> None:
|
||||
"""When the first declared package errors, it's the last processed in reverse.
|
||||
|
||||
All other entries are already resolved to dicts, but the failing entry
|
||||
retains its original IncludeFile value since assignment was skipped.
|
||||
"""
|
||||
config = {
|
||||
CONF_PACKAGES: {
|
||||
"first_pkg": MagicMock(spec=IncludeFile),
|
||||
"second_pkg": MagicMock(spec=IncludeFile),
|
||||
"third_pkg": MagicMock(spec=IncludeFile),
|
||||
},
|
||||
}
|
||||
|
||||
call_count = 0
|
||||
|
||||
def fail_on_last(package_config: dict, context: object) -> dict:
|
||||
nonlocal call_count
|
||||
call_count += 1
|
||||
# Reverse iteration: third_pkg (1), second_pkg (2), first_pkg (3)
|
||||
if call_count < 3:
|
||||
return {CONF_WIFI: {CONF_SSID: "test"}}
|
||||
raise cv.Invalid("error in first_pkg")
|
||||
|
||||
with pytest.raises(cv.Invalid, match="error in first_pkg"):
|
||||
_walk_packages(config, fail_on_last)
|
||||
|
||||
|
||||
def test_deprecated_single_package_fallback_still_works(
|
||||
caplog: pytest.LogCaptureFixture,
|
||||
) -> None:
|
||||
"""The deprecated single-package form still falls back at the top level.
|
||||
|
||||
When a dict's values are plain config fragments (not package definitions)
|
||||
and the callback fails, the deprecated fallback wraps the dict in a list
|
||||
and retries with a deprecation warning.
|
||||
"""
|
||||
config = {
|
||||
CONF_PACKAGES: {
|
||||
CONF_WIFI: {CONF_SSID: "test", CONF_PASSWORD: "secret"},
|
||||
},
|
||||
}
|
||||
|
||||
attempt = 0
|
||||
|
||||
def fail_then_succeed(package_config: dict, context: object) -> dict:
|
||||
nonlocal attempt
|
||||
attempt += 1
|
||||
if attempt == 1:
|
||||
# First attempt: treating as named dict fails
|
||||
raise cv.Invalid("not a valid package")
|
||||
# Second attempt: after fallback wraps as list, succeeds
|
||||
return package_config
|
||||
|
||||
with caplog.at_level(logging.WARNING):
|
||||
_walk_packages(config, fail_then_succeed)
|
||||
|
||||
assert "deprecated" in caplog.text.lower()
|
||||
|
||||
|
||||
def test_merge_packages_invalid_nested_type_raises() -> None:
|
||||
"""Invalid nested packages type during merge raises cv.Invalid."""
|
||||
config = {
|
||||
|
||||
Reference in New Issue
Block a user