From 5f311d281e78ef38b621eddfc41ab1459754afe1 Mon Sep 17 00:00:00 2001 From: Clyde Stubbs <2366188+clydebarrow@users.noreply.github.com> Date: Mon, 29 Jun 2026 07:17:01 +1000 Subject: [PATCH] [esphome] Warn when a YAML merge (`<<:`) drops a key (#17246) Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- esphome/config.py | 19 +++++++ esphome/const.py | 1 + esphome/core/config.py | 2 + esphome/yaml_util.py | 27 +++++++++ script/ci-custom.py | 2 +- tests/unit_tests/test_config_normalization.py | 55 +++++++++++++++++++ tests/unit_tests/test_yaml_util.py | 45 +++++++++++++++ 7 files changed, 150 insertions(+), 1 deletion(-) diff --git a/esphome/config.py b/esphome/config.py index 33e687137f..fc8f46909f 100644 --- a/esphome/config.py +++ b/esphome/config.py @@ -20,6 +20,7 @@ from esphome.const import ( CONF_ESPHOME, CONF_EXTERNAL_COMPONENTS, CONF_ID, + CONF_MERGE_WARNINGS, CONF_MIN_VERSION, CONF_PACKAGES, CONF_PLATFORM, @@ -1184,6 +1185,24 @@ def validate_config( ) return result + # Warn about any keys silently dropped by `<<` merge includes (shallow, + # first-wins). The esphome: section is now known, so we can honor its + # `merge_warnings:` opt-out. Always drain the queue to keep it from leaking + # into a later run. + if (dropped := yaml_util.take_dropped_merge_keys()) and ( + not isinstance(esphome_conf := config[CONF_ESPHOME], dict) + or esphome_conf.get(CONF_MERGE_WARNINGS, True) + ): + for key, location in dict.fromkeys(dropped): + _LOGGER.warning( + "Key '%s' (%s) was dropped while processing a '<<' merge because it " + "is already defined. Merge keys don't combine sections - the first " + "definition wins. Use 'packages:' to merge sections, or set " + "'esphome: { merge_warnings: false }' to silence this.", + key, + location, + ) + # Snapshot the user's config before any schema validation defaults are # applied. preload_core_config and later validation steps rewrite entries # in-place with defaulted values; deep-copying here preserves the diff --git a/esphome/const.py b/esphome/const.py index 3ca7b2e618..5fa6f00b59 100644 --- a/esphome/const.py +++ b/esphome/const.py @@ -613,6 +613,7 @@ CONF_MEASUREMENT_SEQUENCE_NUMBER = "measurement_sequence_number" CONF_MEDIA_PLAYER = "media_player" CONF_MEDIUM = "medium" CONF_MEMORY_BLOCKS = "memory_blocks" +CONF_MERGE_WARNINGS = "merge_warnings" CONF_MESSAGE = "message" CONF_METHANE = "methane" CONF_METHOD = "method" diff --git a/esphome/core/config.py b/esphome/core/config.py index 59c96035b8..0670fde0ff 100644 --- a/esphome/core/config.py +++ b/esphome/core/config.py @@ -26,6 +26,7 @@ from esphome.const import ( CONF_INCLUDES, CONF_INCLUDES_C, CONF_LIBRARIES, + CONF_MERGE_WARNINGS, CONF_MIN_VERSION, CONF_NAME, CONF_NAME_ADD_MAC_SUFFIX, @@ -316,6 +317,7 @@ CONFIG_SCHEMA = cv.All( cv.Optional(CONF_INCLUDES_C, default=[]): cv.ensure_list(valid_include), cv.Optional(CONF_LIBRARIES, default=[]): cv.ensure_list(cv.string_strict), cv.Optional(CONF_NAME_ADD_MAC_SUFFIX, default=False): cv.boolean, + cv.Optional(CONF_MERGE_WARNINGS, default=True): cv.boolean, cv.Optional(CONF_DEBUG_SCHEDULER, default=False): cv.boolean, cv.Optional(CONF_PROJECT): cv.Schema( { diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index bfe1fb0136..0009cde551 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -51,6 +51,29 @@ _load_listeners: list[Callable[[Path], None]] = [] DocumentPath = list[str | int] +# Key under CORE.data used to accumulate keys that a `<<` merge silently +# dropped. The warning is emitted later (see esphome.config.validate_config), +# because the esphome: option that suppresses it isn't known while parsing. +_MERGE_WARNINGS_KEY = "yaml_dropped_merge_keys" + + +def _record_dropped_merge_key(parent_file: Path, key: Any) -> None: + """Record a mapping key that a ``<<`` merge silently dropped. + + Merge keys follow the YAML spec's shallow, first-wins semantics: a key that + already exists in the mapping (or came from an earlier merge) is discarded + rather than deep-merged the way ``packages:`` would combine it. We collect + these so a single warning can be emitted once the config is loaded. + """ + esp_range = getattr(key, "esp_range", None) + location = str(esp_range.start_mark) if esp_range is not None else str(parent_file) + CORE.data.setdefault(_MERGE_WARNINGS_KEY, []).append((str(key), location)) + + +def take_dropped_merge_keys() -> list[tuple[str, str]]: + """Return and clear the keys dropped during ``<<`` merges so far.""" + return CORE.data.pop(_MERGE_WARNINGS_KEY, []) + class SensitiveStr(str): """Marker subclass for validated strings that should be masked in @@ -551,6 +574,10 @@ class ESPHomeLoaderMixin: # is expected to contain mapping nodes and each of these nodes is merged in # turn according to its order in the sequence. Keys in mapping nodes earlier # in the sequence override keys specified in later mapping nodes." + # + # This is a silent shallow drop (unlike `packages:`, which deep-merges). + # Record it so a warning can be emitted after the config loads. + _record_dropped_merge_key(self.name, key) continue pairs.append((key, value)) # Add key node to seen keys, for sequence merge values. diff --git a/script/ci-custom.py b/script/ci-custom.py index 6c5ad5bb69..4568732b88 100755 --- a/script/ci-custom.py +++ b/script/ci-custom.py @@ -555,7 +555,7 @@ def lint_constants_usage(): # Maximum allowed CONF_ constants in esphome/const.py. # This file is frozen — new constants go in esphome/components/const/__init__.py. # Decrease this number when constants are moved out of const.py. -CONST_PY_MAX_CONF = 1013 +CONST_PY_MAX_CONF = 1014 @lint_content_check(include=["esphome/const.py"]) diff --git a/tests/unit_tests/test_config_normalization.py b/tests/unit_tests/test_config_normalization.py index 4ec17b3c7c..a06b2da621 100644 --- a/tests/unit_tests/test_config_normalization.py +++ b/tests/unit_tests/test_config_normalization.py @@ -1,6 +1,7 @@ """Unit tests for esphome.config module.""" from collections.abc import Generator +import logging from pathlib import Path from unittest.mock import MagicMock, Mock, patch @@ -113,3 +114,57 @@ def test_ota_with_platform_list_and_captive_portal(fixtures_dir: Path) -> None: platforms = {p.get("platform") for p in result["ota"]} assert "esphome" in platforms, f"Expected esphome platform in {platforms}" assert "web_server" in platforms, f"Expected web_server platform in {platforms}" + + +def _write_merge_conflict_config(tmp_path: Path, *, suppress: bool) -> Path: + """Create a config where two `<<` includes both define `logger:`. + + The second `logger:` is dropped by the shallow merge. Returns the main file. + """ + (tmp_path / "a.yaml").write_text("logger:\n level: DEBUG\n") + (tmp_path / "b.yaml").write_text("logger:\n level: INFO\n") + esphome_section = "esphome:\n name: test\n" + if suppress: + esphome_section += " merge_warnings: false\n" + main = tmp_path / "main.yaml" + main.write_text(f"{esphome_section}<<: !include a.yaml\n<<: !include b.yaml\n") + return main + + +def test_validate_config_warns_on_dropped_merge_key( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """By default, a `<<` merge that drops a key logs a warning.""" + main = _write_merge_conflict_config(tmp_path, suppress=False) + CORE.config_path = main + raw_config = yaml_util.load_yaml(main) + + with caplog.at_level(logging.WARNING, logger="esphome.config"): + config.validate_config(raw_config, {}) + + assert any( + "was dropped while processing a '<<' merge" in record.message + and "logger" in record.message + for record in caplog.records + ) + # The queue is drained so the warning cannot leak into a later run. + assert yaml_util.take_dropped_merge_keys() == [] + + +def test_validate_config_suppresses_merge_warning( + tmp_path: Path, caplog: pytest.LogCaptureFixture +) -> None: + """`esphome: merge_warnings: false` hides the warning but still drains the queue.""" + main = _write_merge_conflict_config(tmp_path, suppress=True) + CORE.config_path = main + raw_config = yaml_util.load_yaml(main) + + with caplog.at_level(logging.WARNING, logger="esphome.config"): + config.validate_config(raw_config, {}) + + assert not any( + "was dropped while processing a '<<' merge" in record.message + for record in caplog.records + ) + # The queue is drained even when the warning is suppressed. + assert yaml_util.take_dropped_merge_keys() == [] diff --git a/tests/unit_tests/test_yaml_util.py b/tests/unit_tests/test_yaml_util.py index 6be090b869..fa1c0fcce2 100644 --- a/tests/unit_tests/test_yaml_util.py +++ b/tests/unit_tests/test_yaml_util.py @@ -1395,3 +1395,48 @@ def test_dump__redaction_flag_does_not_leak_between_calls() -> None: assert "\\033[8m" in redacted assert "\\033[8m" not in raw assert "\\033[8m" in redacted_again + + +@pytest.fixture(autouse=True) +def clear_dropped_merge_keys() -> None: + """Reset the dropped-merge-key queue between tests.""" + core.CORE.data.pop(yaml_util._MERGE_WARNINGS_KEY, None) + yield + core.CORE.data.pop(yaml_util._MERGE_WARNINGS_KEY, None) + + +def test_merge_include_records_dropped_keys(tmp_path: Path) -> None: + """A `<<` merge that overlaps an existing key records it (shallow first-wins).""" + (tmp_path / "a.yaml").write_text("api:\n reboot_timeout: 5min\n") + (tmp_path / "b.yaml").write_text("api:\n password: secret\n") + test_yaml = tmp_path / "test.yaml" + test_yaml.write_text("<<: !include a.yaml\n<<: !include b.yaml\n") + + with patch.object(core.CORE, "config_path", test_yaml): + result = yaml_util.load_yaml(test_yaml) + + # First definition wins; the second `api` block is dropped entirely. + assert result["api"] == {"reboot_timeout": "5min"} + + dropped = yaml_util.take_dropped_merge_keys() + assert len(dropped) == 1 + key, location = dropped[0] + assert key == "api" + assert "b.yaml" in location + # Queue is drained after being taken. + assert yaml_util.take_dropped_merge_keys() == [] + + +def test_merge_include_no_overlap_records_nothing(tmp_path: Path) -> None: + """A `<<` merge with distinct top-level keys drops nothing.""" + (tmp_path / "a.yaml").write_text("api:\n reboot_timeout: 5min\n") + (tmp_path / "b.yaml").write_text("logger:\n level: DEBUG\n") + test_yaml = tmp_path / "test.yaml" + test_yaml.write_text("<<: !include a.yaml\n<<: !include b.yaml\n") + + with patch.object(core.CORE, "config_path", test_yaml): + result = yaml_util.load_yaml(test_yaml) + + assert result["api"] == {"reboot_timeout": "5min"} + assert result["logger"] == {"level": "DEBUG"} + assert yaml_util.take_dropped_merge_keys() == []