From 8f70ef24a22f0c03b4e1e3c4aef84cf748b56688 Mon Sep 17 00:00:00 2001 From: Subhash Chandra Date: Wed, 6 Dec 2023 06:34:17 +0530 Subject: [PATCH] feat(packages): support removing components (#5821) --- .devcontainer/devcontainer.json | 1 + esphome/config.py | 39 ++++- esphome/config_helpers.py | 28 ++- esphome/config_validation.py | 6 +- esphome/yaml_util.py | 7 +- .../component_tests/packages/test_packages.py | 164 +++++++++++++++++- 6 files changed, 240 insertions(+), 5 deletions(-) diff --git a/.devcontainer/devcontainer.json b/.devcontainer/devcontainer.json index c8f94cb6bb..7abcb43417 100644 --- a/.devcontainer/devcontainer.json +++ b/.devcontainer/devcontainer.json @@ -37,6 +37,7 @@ "!secret scalar", "!lambda scalar", "!extend scalar", + "!remove scalar", "!include_dir_named scalar", "!include_dir_list scalar", "!include_dir_merge_list scalar", diff --git a/esphome/config.py b/esphome/config.py index 6f644cee14..3461223490 100644 --- a/esphome/config.py +++ b/esphome/config.py @@ -26,7 +26,7 @@ from esphome.core import CORE, EsphomeError from esphome.helpers import indent from esphome.util import safe_print, OrderedDict -from esphome.config_helpers import Extend +from esphome.config_helpers import Extend, Remove from esphome.loader import get_component, get_platform, ComponentManifest from esphome.yaml_util import is_secret, ESPHomeDataBase, ESPForceValue from esphome.voluptuous_schema import ExtraKeysInvalid @@ -345,6 +345,12 @@ class LoadValidationStep(ConfigValidationStep): path + [CONF_ID], ) continue + if isinstance(p_id, Remove): + result.add_str_error( + f"Source for removal of ID '{p_id.value}' was not found.", + path + [CONF_ID], + ) + continue result.add_str_error("No platform specified! See 'platform' key.", path) continue # Remove temp output path and construct new one @@ -634,6 +640,35 @@ class IDPassValidationStep(ConfigValidationStep): ) +class RemoveReferenceValidationStep(ConfigValidationStep): + """ + Make sure all !remove references have been removed from the config. + Any left overs mean the merge step couldn't find corresponding previously existing id/key + """ + + def run(self, result: Config) -> None: + if result.errors: + # If result already has errors, skip this step + return + + def recursive_check_remove_tag(config: Config, path: ConfigPath = None): + path = path or [] + + if isinstance(config, Remove): + result.add_str_error( + f"Source for removal at '{'->'.join([str(p) for p in path])}' was not found.", + path, + ) + elif isinstance(config, list): + for i, item in enumerate(config): + recursive_check_remove_tag(item, path + [i]) + elif isinstance(config, dict): + for key, value in config.items(): + recursive_check_remove_tag(value, path + [key]) + + recursive_check_remove_tag(result) + + class FinalValidateValidationStep(ConfigValidationStep): """Run final_validate_schema for all components.""" @@ -771,6 +806,8 @@ def validate_config(config, command_line_substitutions) -> Config: result.add_validation_step(IDPassValidationStep()) result.add_validation_step(PinUseValidationCheck()) + result.add_validation_step(RemoveReferenceValidationStep()) + result.run_validation_steps() return result diff --git a/esphome/config_helpers.py b/esphome/config_helpers.py index e1d63775bb..ac52c6ede2 100644 --- a/esphome/config_helpers.py +++ b/esphome/config_helpers.py @@ -22,6 +22,22 @@ class Extend: return isinstance(b, Extend) and self.value == b.value +class Remove: + def __init__(self, value=None): + self.value = value + + def __str__(self): + return f"!remove {self.value}" + + def __eq__(self, b): + """ + Check if two Remove objects contain the same ID. + + Only used in unit tests. + """ + return isinstance(b, Remove) and self.value == b.value + + def read_config_file(path: str) -> str: if CORE.vscode and ( not CORE.ace or os.path.abspath(path) == os.path.abspath(CORE.config_path) @@ -48,7 +64,10 @@ def merge_config(full_old, full_new): return new res = old.copy() for k, v in new.items(): - res[k] = merge(old[k], v) if k in old else v + if isinstance(v, Remove) and k in old: + del res[k] + else: + res[k] = merge(old[k], v) if k in old else v return res if isinstance(new, list): if not isinstance(old, list): @@ -59,6 +78,7 @@ def merge_config(full_old, full_new): for i, v in enumerate(res) if CONF_ID in v and isinstance(v[CONF_ID], str) } + ids_to_delete = [] for v in new: if CONF_ID in v: new_id = v[CONF_ID] @@ -68,9 +88,15 @@ def merge_config(full_old, full_new): v[CONF_ID] = new_id res[ids[new_id]] = merge(res[ids[new_id]], v) continue + elif isinstance(new_id, Remove): + new_id = new_id.value + if new_id in ids: + ids_to_delete.append(ids[new_id]) + continue else: ids[new_id] = len(res) res.append(v) + res = [v for i, v in enumerate(res) if i not in ids_to_delete] return res if new is None: return old diff --git a/esphome/config_validation.py b/esphome/config_validation.py index eb347d0a4d..ad2ee11512 100644 --- a/esphome/config_validation.py +++ b/esphome/config_validation.py @@ -13,7 +13,7 @@ import voluptuous as vol from esphome import core import esphome.codegen as cg -from esphome.config_helpers import Extend +from esphome.config_helpers import Extend, Remove from esphome.const import ( ALLOWED_NAME_CHARS, CONF_AVAILABILITY, @@ -532,6 +532,10 @@ def declare_id(type): if isinstance(value, Extend): raise Invalid(f"Source for extension of ID '{value.value}' was not found.") + + if isinstance(value, Remove): + raise Invalid(f"Source for Removal of ID '{value.value}' was not found.") + return core.ID(validate_id_name(value), is_declaration=True, type=type) return validator diff --git a/esphome/yaml_util.py b/esphome/yaml_util.py index a954415d12..f0f755dd61 100644 --- a/esphome/yaml_util.py +++ b/esphome/yaml_util.py @@ -10,7 +10,7 @@ import yaml import yaml.constructor from esphome import core -from esphome.config_helpers import read_config_file, Extend +from esphome.config_helpers import read_config_file, Extend, Remove from esphome.core import ( EsphomeError, IPAddress, @@ -362,6 +362,10 @@ class ESPHomeLoader(FastestAvailableSafeLoader): def construct_extend(self, node): return Extend(str(node.value)) + @_add_data_ref + def construct_remove(self, node): + return Remove(str(node.value)) + ESPHomeLoader.add_constructor("tag:yaml.org,2002:int", ESPHomeLoader.construct_yaml_int) ESPHomeLoader.add_constructor( @@ -394,6 +398,7 @@ ESPHomeLoader.add_constructor( ESPHomeLoader.add_constructor("!lambda", ESPHomeLoader.construct_lambda) ESPHomeLoader.add_constructor("!force", ESPHomeLoader.construct_force) ESPHomeLoader.add_constructor("!extend", ESPHomeLoader.construct_extend) +ESPHomeLoader.add_constructor("!remove", ESPHomeLoader.construct_remove) def load_yaml(fname, clear_secrets=True): diff --git a/tests/component_tests/packages/test_packages.py b/tests/component_tests/packages/test_packages.py index 0e24d78f5c..01cf55872c 100644 --- a/tests/component_tests/packages/test_packages.py +++ b/tests/component_tests/packages/test_packages.py @@ -20,7 +20,7 @@ from esphome.const import ( CONF_WIFI, ) from esphome.components.packages import do_packages_pass -from esphome.config_helpers import Extend +from esphome.config_helpers import Extend, Remove import esphome.config_validation as cv # Test strings @@ -349,3 +349,165 @@ def test_package_merge_by_missing_id(): actual = do_packages_pass(config) assert actual == expected + + +def test_package_list_remove_by_id(): + """ + Ensures that components with matching IDs are removed correctly. + + In this test, two sensors are defined in a package, and one of them is removed at the top level. + """ + config = { + CONF_PACKAGES: { + "package_sensors": { + CONF_SENSOR: [ + { + CONF_ID: TEST_SENSOR_ID_1, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_1, + }, + { + CONF_ID: TEST_SENSOR_ID_2, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + }, + ] + }, + # "package2": { + # CONF_SENSOR: [ + # { + # CONF_ID: Remove(TEST_SENSOR_ID_1), + # } + # ], + # }, + }, + CONF_SENSOR: [ + { + CONF_ID: Remove(TEST_SENSOR_ID_1), + }, + ], + } + + expected = { + CONF_SENSOR: [ + { + CONF_ID: TEST_SENSOR_ID_2, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + }, + ] + } + + actual = do_packages_pass(config) + assert actual == expected + + +def test_multiple_package_list_remove_by_id(): + """ + Ensures that components with matching IDs are removed correctly. + + In this test, two sensors are defined in a package, and one of them is removed in another package. + """ + config = { + CONF_PACKAGES: { + "package_sensors": { + CONF_SENSOR: [ + { + CONF_ID: TEST_SENSOR_ID_1, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_1, + }, + { + CONF_ID: TEST_SENSOR_ID_2, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + }, + ] + }, + "package2": { + CONF_SENSOR: [ + { + CONF_ID: Remove(TEST_SENSOR_ID_1), + } + ], + }, + }, + } + + expected = { + CONF_SENSOR: [ + { + CONF_ID: TEST_SENSOR_ID_2, + CONF_PLATFORM: TEST_SENSOR_PLATFORM_1, + CONF_NAME: TEST_SENSOR_NAME_2, + }, + ] + } + + actual = do_packages_pass(config) + assert actual == expected + + +def test_package_dict_remove_by_id(basic_wifi, basic_esphome): + """ + Ensures that components with missing IDs are removed from dict. + """ + """ + Ensures that the top-level configuration takes precedence over duplicate keys defined in a package. + + In this test, CONF_SSID should be overwritten by that defined in the top-level config. + """ + config = { + CONF_ESPHOME: basic_esphome, + CONF_PACKAGES: {"network": {CONF_WIFI: basic_wifi}}, + CONF_WIFI: Remove(), + } + + expected = { + CONF_ESPHOME: basic_esphome, + } + + actual = do_packages_pass(config) + assert actual == expected + + +def test_package_remove_by_missing_id(): + """ + Ensures that components with missing IDs are not merged. + """ + + config = { + CONF_PACKAGES: { + "sensors": { + CONF_SENSOR: [ + {CONF_ID: TEST_SENSOR_ID_1, CONF_FILTERS: [{CONF_MULTIPLY: 42.0}]}, + ] + } + }, + "missing_key": Remove(), + CONF_SENSOR: [ + {CONF_ID: TEST_SENSOR_ID_1, CONF_FILTERS: [{CONF_MULTIPLY: 10.0}]}, + {CONF_ID: Remove(TEST_SENSOR_ID_2), CONF_FILTERS: [{CONF_OFFSET: 146.0}]}, + ], + } + + expected = { + "missing_key": Remove(), + CONF_SENSOR: [ + { + CONF_ID: TEST_SENSOR_ID_1, + CONF_FILTERS: [{CONF_MULTIPLY: 42.0}], + }, + { + CONF_ID: TEST_SENSOR_ID_1, + CONF_FILTERS: [{CONF_MULTIPLY: 10.0}], + }, + { + CONF_ID: Remove(TEST_SENSOR_ID_2), + CONF_FILTERS: [{CONF_OFFSET: 146.0}], + }, + ], + } + + actual = do_packages_pass(config) + assert actual == expected