From 5b55c7da5fbaeffd3aa61f32dcb40a21ba192b6c Mon Sep 17 00:00:00 2001 From: Erik Montnemery Date: Tue, 5 Dec 2023 18:08:11 +0100 Subject: [PATCH] Remove logic converting empty or falsy YAML to empty dict (#103912) * Correct logic converting empty YAML to empty dict * Modify according to github comments * Add load_yaml_dict helper * Update check_config script * Update tests --- homeassistant/components/blueprint/models.py | 3 +- .../components/lovelace/dashboard.py | 6 ++- homeassistant/components/notify/legacy.py | 6 +-- .../components/python_script/__init__.py | 4 +- homeassistant/components/tts/legacy.py | 8 ++-- homeassistant/config.py | 10 ++--- homeassistant/helpers/service.py | 6 ++- homeassistant/scripts/check_config.py | 2 +- homeassistant/util/yaml/__init__.py | 11 ++++- homeassistant/util/yaml/loader.py | 40 ++++++++++++++----- script/hassfest/services.py | 6 +-- tests/components/automation/test_init.py | 2 +- tests/components/lovelace/test_cast.py | 2 +- tests/components/lovelace/test_dashboard.py | 8 ++-- tests/components/lovelace/test_resources.py | 2 +- .../components/lovelace/test_system_health.py | 2 +- tests/components/samsungtv/test_trigger.py | 2 +- tests/components/webostv/test_trigger.py | 2 +- tests/components/zwave_js/test_trigger.py | 4 +- tests/util/yaml/test_init.py | 33 +++++++++++++++ 20 files changed, 112 insertions(+), 47 deletions(-) diff --git a/homeassistant/components/blueprint/models.py b/homeassistant/components/blueprint/models.py index ddf57aa6eee..63a1c1b45f0 100644 --- a/homeassistant/components/blueprint/models.py +++ b/homeassistant/components/blueprint/models.py @@ -215,7 +215,7 @@ class DomainBlueprints: def _load_blueprint(self, blueprint_path) -> Blueprint: """Load a blueprint.""" try: - blueprint_data = yaml.load_yaml(self.blueprint_folder / blueprint_path) + blueprint_data = yaml.load_yaml_dict(self.blueprint_folder / blueprint_path) except FileNotFoundError as err: raise FailedToLoad( self.domain, @@ -225,7 +225,6 @@ class DomainBlueprints: except HomeAssistantError as err: raise FailedToLoad(self.domain, blueprint_path, err) from err - assert isinstance(blueprint_data, dict) return Blueprint( blueprint_data, expected_domain=self.domain, path=blueprint_path ) diff --git a/homeassistant/components/lovelace/dashboard.py b/homeassistant/components/lovelace/dashboard.py index e1641451221..d935ad9bff5 100644 --- a/homeassistant/components/lovelace/dashboard.py +++ b/homeassistant/components/lovelace/dashboard.py @@ -14,7 +14,7 @@ from homeassistant.const import CONF_FILENAME from homeassistant.core import callback from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers import collection, storage -from homeassistant.util.yaml import Secrets, load_yaml +from homeassistant.util.yaml import Secrets, load_yaml_dict from .const import ( CONF_ICON, @@ -201,7 +201,9 @@ class LovelaceYAML(LovelaceConfig): is_updated = self._cache is not None try: - config = load_yaml(self.path, Secrets(Path(self.hass.config.config_dir))) + config = load_yaml_dict( + self.path, Secrets(Path(self.hass.config.config_dir)) + ) except FileNotFoundError: raise ConfigNotFound from None diff --git a/homeassistant/components/notify/legacy.py b/homeassistant/components/notify/legacy.py index 93b6833edc6..7c78bfc44d3 100644 --- a/homeassistant/components/notify/legacy.py +++ b/homeassistant/components/notify/legacy.py @@ -17,7 +17,7 @@ from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType from homeassistant.loader import async_get_integration, bind_hass from homeassistant.setup import async_prepare_setup_platform, async_start_setup from homeassistant.util import slugify -from homeassistant.util.yaml import load_yaml +from homeassistant.util.yaml import load_yaml_dict from .const import ( ATTR_DATA, @@ -280,8 +280,8 @@ class BaseNotificationService: # Load service descriptions from notify/services.yaml integration = await async_get_integration(hass, DOMAIN) services_yaml = integration.file_path / "services.yaml" - self.services_dict = cast( - dict, await hass.async_add_executor_job(load_yaml, str(services_yaml)) + self.services_dict = await hass.async_add_executor_job( + load_yaml_dict, str(services_yaml) ) async def async_register_services(self) -> None: diff --git a/homeassistant/components/python_script/__init__.py b/homeassistant/components/python_script/__init__.py index 10751d28c06..098603b9494 100644 --- a/homeassistant/components/python_script/__init__.py +++ b/homeassistant/components/python_script/__init__.py @@ -27,7 +27,7 @@ from homeassistant.helpers.typing import ConfigType from homeassistant.loader import bind_hass from homeassistant.util import raise_if_invalid_filename import homeassistant.util.dt as dt_util -from homeassistant.util.yaml.loader import load_yaml +from homeassistant.util.yaml.loader import load_yaml_dict _LOGGER = logging.getLogger(__name__) @@ -120,7 +120,7 @@ def discover_scripts(hass): # Load user-provided service descriptions from python_scripts/services.yaml services_yaml = os.path.join(path, "services.yaml") if os.path.exists(services_yaml): - services_dict = load_yaml(services_yaml) + services_dict = load_yaml_dict(services_yaml) else: services_dict = {} diff --git a/homeassistant/components/tts/legacy.py b/homeassistant/components/tts/legacy.py index a52bcb802ab..05be2e284e3 100644 --- a/homeassistant/components/tts/legacy.py +++ b/homeassistant/components/tts/legacy.py @@ -6,7 +6,7 @@ from collections.abc import Coroutine, Mapping from functools import partial import logging from pathlib import Path -from typing import TYPE_CHECKING, Any, cast +from typing import TYPE_CHECKING, Any import voluptuous as vol @@ -31,7 +31,7 @@ import homeassistant.helpers.config_validation as cv from homeassistant.helpers.service import async_set_service_schema from homeassistant.helpers.typing import ConfigType, DiscoveryInfoType from homeassistant.setup import async_prepare_setup_platform -from homeassistant.util.yaml import load_yaml +from homeassistant.util.yaml import load_yaml_dict from .const import ( ATTR_CACHE, @@ -104,8 +104,8 @@ async def async_setup_legacy( # Load service descriptions from tts/services.yaml services_yaml = Path(__file__).parent / "services.yaml" - services_dict = cast( - dict, await hass.async_add_executor_job(load_yaml, str(services_yaml)) + services_dict = await hass.async_add_executor_job( + load_yaml_dict, str(services_yaml) ) async def async_setup_platform( diff --git a/homeassistant/config.py b/homeassistant/config.py index 6dd8bc21471..95dd42737a0 100644 --- a/homeassistant/config.py +++ b/homeassistant/config.py @@ -66,7 +66,7 @@ from .loader import ComponentProtocol, Integration, IntegrationNotFound from .requirements import RequirementsNotFound, async_get_integration_with_requirements from .util.package import is_docker_env from .util.unit_system import get_unit_system, validate_unit_system -from .util.yaml import SECRET_YAML, Secrets, load_yaml +from .util.yaml import SECRET_YAML, Secrets, YamlTypeError, load_yaml_dict _LOGGER = logging.getLogger(__name__) @@ -476,15 +476,15 @@ def load_yaml_config_file( This method needs to run in an executor. """ - conf_dict = load_yaml(config_path, secrets) - - if not isinstance(conf_dict, dict): + try: + conf_dict = load_yaml_dict(config_path, secrets) + except YamlTypeError as exc: msg = ( f"The configuration file {os.path.basename(config_path)} " "does not contain a dictionary" ) _LOGGER.error(msg) - raise HomeAssistantError(msg) + raise HomeAssistantError(msg) from exc # Convert values to dictionaries if they are None for key, value in conf_dict.items(): diff --git a/homeassistant/helpers/service.py b/homeassistant/helpers/service.py index 32f51a924f7..2ada25bd4cd 100644 --- a/homeassistant/helpers/service.py +++ b/homeassistant/helpers/service.py @@ -42,7 +42,7 @@ from homeassistant.exceptions import ( UnknownUser, ) from homeassistant.loader import Integration, async_get_integrations, bind_hass -from homeassistant.util.yaml import load_yaml +from homeassistant.util.yaml import load_yaml_dict from homeassistant.util.yaml.loader import JSON_TYPE from . import ( @@ -542,7 +542,9 @@ def _load_services_file(hass: HomeAssistant, integration: Integration) -> JSON_T try: return cast( JSON_TYPE, - _SERVICES_SCHEMA(load_yaml(str(integration.file_path / "services.yaml"))), + _SERVICES_SCHEMA( + load_yaml_dict(str(integration.file_path / "services.yaml")) + ), ) except FileNotFoundError: _LOGGER.warning( diff --git a/homeassistant/scripts/check_config.py b/homeassistant/scripts/check_config.py index 0e00d0b75f2..dcccdbccf40 100644 --- a/homeassistant/scripts/check_config.py +++ b/homeassistant/scripts/check_config.py @@ -32,7 +32,7 @@ REQUIREMENTS = ("colorlog==6.7.0",) _LOGGER = logging.getLogger(__name__) MOCKS: dict[str, tuple[str, Callable]] = { "load": ("homeassistant.util.yaml.loader.load_yaml", yaml_loader.load_yaml), - "load*": ("homeassistant.config.load_yaml", yaml_loader.load_yaml), + "load*": ("homeassistant.config.load_yaml_dict", yaml_loader.load_yaml_dict), "secrets": ("homeassistant.util.yaml.loader.secret_yaml", yaml_loader.secret_yaml), } diff --git a/homeassistant/util/yaml/__init__.py b/homeassistant/util/yaml/__init__.py index b3f1b7ecd43..fe4f01677cd 100644 --- a/homeassistant/util/yaml/__init__.py +++ b/homeassistant/util/yaml/__init__.py @@ -2,7 +2,14 @@ from .const import SECRET_YAML from .dumper import dump, save_yaml from .input import UndefinedSubstitution, extract_inputs, substitute -from .loader import Secrets, load_yaml, parse_yaml, secret_yaml +from .loader import ( + Secrets, + YamlTypeError, + load_yaml, + load_yaml_dict, + parse_yaml, + secret_yaml, +) from .objects import Input __all__ = [ @@ -11,7 +18,9 @@ __all__ = [ "dump", "save_yaml", "Secrets", + "YamlTypeError", "load_yaml", + "load_yaml_dict", "secret_yaml", "parse_yaml", "UndefinedSubstitution", diff --git a/homeassistant/util/yaml/loader.py b/homeassistant/util/yaml/loader.py index 4a14afb53b2..60e917a6a99 100644 --- a/homeassistant/util/yaml/loader.py +++ b/homeassistant/util/yaml/loader.py @@ -36,6 +36,10 @@ _DictT = TypeVar("_DictT", bound=dict) _LOGGER = logging.getLogger(__name__) +class YamlTypeError(HomeAssistantError): + """Raised by load_yaml_dict if top level data is not a dict.""" + + class Secrets: """Store secrets while loading YAML.""" @@ -211,7 +215,7 @@ class SafeLineLoader(PythonSafeLoader): LoaderType = FastSafeLoader | PythonSafeLoader -def load_yaml(fname: str, secrets: Secrets | None = None) -> JSON_TYPE: +def load_yaml(fname: str, secrets: Secrets | None = None) -> JSON_TYPE | None: """Load a YAML file.""" try: with open(fname, encoding="utf-8") as conf_file: @@ -221,6 +225,20 @@ def load_yaml(fname: str, secrets: Secrets | None = None) -> JSON_TYPE: raise HomeAssistantError(exc) from exc +def load_yaml_dict(fname: str, secrets: Secrets | None = None) -> dict: + """Load a YAML file and ensure the top level is a dict. + + Raise if the top level is not a dict. + Return an empty dict if the file is empty. + """ + loaded_yaml = load_yaml(fname, secrets) + if loaded_yaml is None: + loaded_yaml = {} + if not isinstance(loaded_yaml, dict): + raise YamlTypeError(f"YAML file {fname} does not contain a dict") + return loaded_yaml + + def parse_yaml( content: str | TextIO | StringIO, secrets: Secrets | None = None ) -> JSON_TYPE: @@ -255,12 +273,7 @@ def _parse_yaml( secrets: Secrets | None = None, ) -> JSON_TYPE: """Load a YAML file.""" - # If configuration file is empty YAML returns None - # We convert that to an empty dict - return ( - yaml.load(content, Loader=lambda stream: loader(stream, secrets)) # type: ignore[arg-type] - or NodeDictClass() - ) + return yaml.load(content, Loader=lambda stream: loader(stream, secrets)) # type: ignore[arg-type] @overload @@ -309,7 +322,10 @@ def _include_yaml(loader: LoaderType, node: yaml.nodes.Node) -> JSON_TYPE: """ fname = os.path.join(os.path.dirname(loader.get_name()), node.value) try: - return _add_reference(load_yaml(fname, loader.secrets), loader, node) + loaded_yaml = load_yaml(fname, loader.secrets) + if loaded_yaml is None: + loaded_yaml = NodeDictClass() + return _add_reference(loaded_yaml, loader, node) except FileNotFoundError as exc: raise HomeAssistantError( f"{node.start_mark}: Unable to read file {fname}." @@ -339,7 +355,10 @@ def _include_dir_named_yaml(loader: LoaderType, node: yaml.nodes.Node) -> NodeDi filename = os.path.splitext(os.path.basename(fname))[0] if os.path.basename(fname) == SECRET_YAML: continue - mapping[filename] = load_yaml(fname, loader.secrets) + loaded_yaml = load_yaml(fname, loader.secrets) + if loaded_yaml is None: + continue + mapping[filename] = loaded_yaml return _add_reference(mapping, loader, node) @@ -364,9 +383,10 @@ def _include_dir_list_yaml( """Load multiple files from directory as a list.""" loc = os.path.join(os.path.dirname(loader.get_name()), node.value) return [ - load_yaml(f, loader.secrets) + loaded_yaml for f in _find_files(loc, "*.yaml") if os.path.basename(f) != SECRET_YAML + and (loaded_yaml := load_yaml(f, loader.secrets)) is not None ] diff --git a/script/hassfest/services.py b/script/hassfest/services.py index 4a826f7cad9..580294705cf 100644 --- a/script/hassfest/services.py +++ b/script/hassfest/services.py @@ -13,7 +13,7 @@ from voluptuous.humanize import humanize_error from homeassistant.const import CONF_SELECTOR from homeassistant.exceptions import HomeAssistantError from homeassistant.helpers import config_validation as cv, selector, service -from homeassistant.util.yaml import load_yaml +from homeassistant.util.yaml import load_yaml_dict from .model import Config, Integration @@ -107,7 +107,7 @@ def grep_dir(path: pathlib.Path, glob_pattern: str, search_pattern: str) -> bool def validate_services(config: Config, integration: Integration) -> None: """Validate services.""" try: - data = load_yaml(str(integration.path / "services.yaml")) + data = load_yaml_dict(str(integration.path / "services.yaml")) except FileNotFoundError: # Find if integration uses services has_services = grep_dir( @@ -122,7 +122,7 @@ def validate_services(config: Config, integration: Integration) -> None: ) return except HomeAssistantError: - integration.add_error("services", "Unable to load services.yaml") + integration.add_error("services", "Invalid services.yaml") return try: diff --git a/tests/components/automation/test_init.py b/tests/components/automation/test_init.py index 6d83b00517d..359303c51fd 100644 --- a/tests/components/automation/test_init.py +++ b/tests/components/automation/test_init.py @@ -1102,7 +1102,7 @@ async def test_reload_automation_when_blueprint_changes( autospec=True, return_value=config, ), patch( - "homeassistant.components.blueprint.models.yaml.load_yaml", + "homeassistant.components.blueprint.models.yaml.load_yaml_dict", autospec=True, return_value=blueprint_config, ): diff --git a/tests/components/lovelace/test_cast.py b/tests/components/lovelace/test_cast.py index e67ab3f841a..4181d73c4d3 100644 --- a/tests/components/lovelace/test_cast.py +++ b/tests/components/lovelace/test_cast.py @@ -44,7 +44,7 @@ async def mock_yaml_dashboard(hass): ) with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={ "title": "YAML Title", "views": [ diff --git a/tests/components/lovelace/test_dashboard.py b/tests/components/lovelace/test_dashboard.py index 05bc7f372b8..a772b37f047 100644 --- a/tests/components/lovelace/test_dashboard.py +++ b/tests/components/lovelace/test_dashboard.py @@ -141,7 +141,7 @@ async def test_lovelace_from_yaml( events = async_capture_events(hass, const.EVENT_LOVELACE_UPDATED) with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"hello": "yo"}, ): await client.send_json({"id": 7, "type": "lovelace/config"}) @@ -154,7 +154,7 @@ async def test_lovelace_from_yaml( # Fake new data to see we fire event with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"hello": "yo2"}, ): await client.send_json({"id": 8, "type": "lovelace/config", "force": True}) @@ -245,7 +245,7 @@ async def test_dashboard_from_yaml( events = async_capture_events(hass, const.EVENT_LOVELACE_UPDATED) with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"hello": "yo"}, ): await client.send_json( @@ -260,7 +260,7 @@ async def test_dashboard_from_yaml( # Fake new data to see we fire event with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"hello": "yo2"}, ): await client.send_json( diff --git a/tests/components/lovelace/test_resources.py b/tests/components/lovelace/test_resources.py index f7830f03ed6..4a280eccfda 100644 --- a/tests/components/lovelace/test_resources.py +++ b/tests/components/lovelace/test_resources.py @@ -38,7 +38,7 @@ async def test_yaml_resources_backwards( ) -> None: """Test defining resources in YAML ll config (legacy).""" with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"resources": RESOURCE_EXAMPLES}, ): assert await async_setup_component( diff --git a/tests/components/lovelace/test_system_health.py b/tests/components/lovelace/test_system_health.py index 7a39bc4605d..72e7adb3a13 100644 --- a/tests/components/lovelace/test_system_health.py +++ b/tests/components/lovelace/test_system_health.py @@ -39,7 +39,7 @@ async def test_system_health_info_yaml(hass: HomeAssistant) -> None: assert await async_setup_component(hass, "lovelace", {"lovelace": {"mode": "YAML"}}) await hass.async_block_till_done() with patch( - "homeassistant.components.lovelace.dashboard.load_yaml", + "homeassistant.components.lovelace.dashboard.load_yaml_dict", return_value={"views": [{"cards": []}]}, ): info = await get_system_health_info(hass, "lovelace") diff --git a/tests/components/samsungtv/test_trigger.py b/tests/components/samsungtv/test_trigger.py index 27f6d7a8e51..12af639b251 100644 --- a/tests/components/samsungtv/test_trigger.py +++ b/tests/components/samsungtv/test_trigger.py @@ -57,7 +57,7 @@ async def test_turn_on_trigger_device_id( assert calls[0].data["some"] == device.id assert calls[0].data["id"] == 0 - with patch("homeassistant.config.load_yaml", return_value={}): + with patch("homeassistant.config.load_yaml_dict", return_value={}): await hass.services.async_call(automation.DOMAIN, SERVICE_RELOAD, blocking=True) calls.clear() diff --git a/tests/components/webostv/test_trigger.py b/tests/components/webostv/test_trigger.py index 9cbf8768dd5..74573e2185b 100644 --- a/tests/components/webostv/test_trigger.py +++ b/tests/components/webostv/test_trigger.py @@ -60,7 +60,7 @@ async def test_webostv_turn_on_trigger_device_id( assert calls[0].data["some"] == device.id assert calls[0].data["id"] == 0 - with patch("homeassistant.config.load_yaml", return_value={}): + with patch("homeassistant.config.load_yaml_dict", return_value={}): await hass.services.async_call(automation.DOMAIN, SERVICE_RELOAD, blocking=True) calls.clear() diff --git a/tests/components/zwave_js/test_trigger.py b/tests/components/zwave_js/test_trigger.py index 25553489b4e..26b9459cfc2 100644 --- a/tests/components/zwave_js/test_trigger.py +++ b/tests/components/zwave_js/test_trigger.py @@ -272,7 +272,7 @@ async def test_zwave_js_value_updated( clear_events() - with patch("homeassistant.config.load_yaml", return_value={}): + with patch("homeassistant.config.load_yaml_dict", return_value={}): await hass.services.async_call(automation.DOMAIN, SERVICE_RELOAD, blocking=True) @@ -834,7 +834,7 @@ async def test_zwave_js_event( clear_events() - with patch("homeassistant.config.load_yaml", return_value={}): + with patch("homeassistant.config.load_yaml_dict", return_value={}): await hass.services.async_call(automation.DOMAIN, SERVICE_RELOAD, blocking=True) diff --git a/tests/util/yaml/test_init.py b/tests/util/yaml/test_init.py index c4e5c58e235..1e31d8c6955 100644 --- a/tests/util/yaml/test_init.py +++ b/tests/util/yaml/test_init.py @@ -134,6 +134,7 @@ def test_include_yaml( [ ({"/test/one.yaml": "one", "/test/two.yaml": "two"}, ["one", "two"]), ({"/test/one.yaml": "1", "/test/two.yaml": "2"}, [1, 2]), + ({"/test/one.yaml": "1", "/test/two.yaml": None}, [1]), ], ) def test_include_dir_list( @@ -190,6 +191,10 @@ def test_include_dir_list_recursive( {"/test/first.yaml": "1", "/test/second.yaml": "2"}, {"first": 1, "second": 2}, ), + ( + {"/test/first.yaml": "1", "/test/second.yaml": None}, + {"first": 1}, + ), ], ) def test_include_dir_named( @@ -249,6 +254,10 @@ def test_include_dir_named_recursive( {"/test/first.yaml": "- 1", "/test/second.yaml": "- 2\n- 3"}, [1, 2, 3], ), + ( + {"/test/first.yaml": "- 1", "/test/second.yaml": None}, + [1], + ), ], ) def test_include_dir_merge_list( @@ -311,6 +320,13 @@ def test_include_dir_merge_list_recursive( }, {"key1": 1, "key2": 2, "key3": 3}, ), + ( + { + "/test/first.yaml": "key1: 1", + "/test/second.yaml": None, + }, + {"key1": 1}, + ), ], ) def test_include_dir_merge_named( @@ -686,3 +702,20 @@ def test_string_used_as_vol_schema(try_both_loaders) -> None: schema({"key_1": "value_1", "key_2": "value_2"}) with pytest.raises(vol.Invalid): schema({"key_1": "value_2", "key_2": "value_1"}) + + +@pytest.mark.parametrize( + ("hass_config_yaml", "expected_data"), [("", {}), ("bla:", {"bla": None})] +) +def test_load_yaml_dict( + try_both_loaders, mock_hass_config_yaml: None, expected_data: Any +) -> None: + """Test item without a key.""" + assert yaml.load_yaml_dict(YAML_CONFIG_FILE) == expected_data + + +@pytest.mark.parametrize("hass_config_yaml", ["abc", "123", "[]"]) +def test_load_yaml_dict_fail(try_both_loaders, mock_hass_config_yaml: None) -> None: + """Test item without a key.""" + with pytest.raises(yaml_loader.YamlTypeError): + yaml_loader.load_yaml_dict(YAML_CONFIG_FILE)