From 59ce31f44fbf6ee81e2eae5e6d639a814619f42a Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Thu, 30 May 2019 04:33:26 -0700 Subject: [PATCH] No longer allow invalid slugs or extra keys (#24176) * No longer allow slugs * Lint * Remove HASchema * Lint * Fix tests --- homeassistant/bootstrap.py | 46 -------- homeassistant/helpers/config_validation.py | 120 +------------------- tests/components/mqtt/test_legacy_vacuum.py | 4 - tests/components/mqtt/test_state_vacuum.py | 19 +--- tests/helpers/test_config_validation.py | 28 ----- tests/test_setup.py | 66 ----------- 6 files changed, 8 insertions(+), 275 deletions(-) diff --git a/homeassistant/bootstrap.py b/homeassistant/bootstrap.py index 96417c54b127..79e5ec248ae1 100644 --- a/homeassistant/bootstrap.py +++ b/homeassistant/bootstrap.py @@ -17,7 +17,6 @@ from homeassistant.util.logging import AsyncHandler from homeassistant.util.package import async_get_user_site, is_virtual_env from homeassistant.util.yaml import clear_secret_cache from homeassistant.exceptions import HomeAssistantError -from homeassistant.helpers import config_validation as cv _LOGGER = logging.getLogger(__name__) @@ -101,51 +100,6 @@ async def async_from_config_dict(config: Dict[str, Any], "upgrade Python.", "Python version", "python_version" ) - # TEMP: warn users for invalid slugs - # Remove after 0.94 or 1.0 - if cv.INVALID_SLUGS_FOUND or cv.INVALID_ENTITY_IDS_FOUND: - msg = [] - - if cv.INVALID_ENTITY_IDS_FOUND: - msg.append( - "Your configuration contains invalid entity ID references. " - "Please find and update the following. " - "This will become a breaking change." - ) - msg.append('\n'.join('- {} -> {}'.format(*item) - for item - in cv.INVALID_ENTITY_IDS_FOUND.items())) - - if cv.INVALID_SLUGS_FOUND: - msg.append( - "Your configuration contains invalid slugs. " - "Please find and update the following. " - "This will become a breaking change." - ) - msg.append('\n'.join('- {} -> {}'.format(*item) - for item in cv.INVALID_SLUGS_FOUND.items())) - - hass.components.persistent_notification.async_create( - '\n\n'.join(msg), "Config Warning", "config_warning" - ) - - # TEMP: warn users of invalid extra keys - # Remove after 0.92 - if cv.INVALID_EXTRA_KEYS_FOUND: - msg = [] - msg.append( - "Your configuration contains extra keys " - "that the platform does not support (but were silently " - "accepted before 0.88). Please find and remove the following." - "This will become a breaking change." - ) - msg.append('\n'.join('- {}'.format(it) - for it in cv.INVALID_EXTRA_KEYS_FOUND)) - - hass.components.persistent_notification.async_create( - '\n\n'.join(msg), "Config Warning", "config_warning" - ) - return hass diff --git a/homeassistant/helpers/config_validation.py b/homeassistant/helpers/config_validation.py index 9282770de1a8..7ec6d1771782 100644 --- a/homeassistant/helpers/config_validation.py +++ b/homeassistant/helpers/config_validation.py @@ -1,6 +1,5 @@ """Helpers for config validation using voluptuous.""" import inspect -import json import logging import os import re @@ -17,7 +16,7 @@ from pkg_resources import parse_version import homeassistant.util.dt as dt_util from homeassistant.const import ( CONF_ABOVE, CONF_ALIAS, CONF_BELOW, CONF_CONDITION, CONF_ENTITY_ID, - CONF_ENTITY_NAMESPACE, CONF_NAME, CONF_PLATFORM, CONF_SCAN_INTERVAL, + CONF_ENTITY_NAMESPACE, CONF_PLATFORM, CONF_SCAN_INTERVAL, CONF_UNIT_SYSTEM_IMPERIAL, CONF_UNIT_SYSTEM_METRIC, CONF_VALUE_TEMPLATE, CONF_TIMEOUT, ENTITY_MATCH_ALL, SUN_EVENT_SUNRISE, SUN_EVENT_SUNSET, TEMP_CELSIUS, TEMP_FAHRENHEIT, WEEKDAYS, __version__) @@ -29,13 +28,6 @@ from homeassistant.util import slugify as util_slugify # pylint: disable=invalid-name TIME_PERIOD_ERROR = "offset {} should be format 'HH:MM' or 'HH:MM:SS'" -OLD_SLUG_VALIDATION = r'^[a-z0-9_]+$' -OLD_ENTITY_ID_VALIDATION = r"^(\w+)\.(\w+)$" -# Keep track of invalid slugs and entity ids found so we can create a -# persistent notification. Rare temporary exception to use a global. -INVALID_SLUGS_FOUND = {} -INVALID_ENTITY_IDS_FOUND = {} -INVALID_EXTRA_KEYS_FOUND = [] # Home Assistant types @@ -176,17 +168,6 @@ def entity_id(value: Any) -> str: value = string(value).lower() if valid_entity_id(value): return value - if re.match(OLD_ENTITY_ID_VALIDATION, value): - # To ease the breaking change, we allow old slugs for now - # Remove after 0.94 or 1.0 - fixed = '.'.join(util_slugify(part) for part in value.split('.', 1)) - INVALID_ENTITY_IDS_FOUND[value] = fixed - logging.getLogger(__name__).warning( - "Found invalid entity_id %s, please update with %s. This " - "will become a breaking change.", - value, fixed - ) - return value raise vol.Invalid('Entity ID {} is an invalid entity id'.format(value)) @@ -377,21 +358,7 @@ def schema_with_slug_keys(value_schema: Union[T, Callable]) -> Callable: raise vol.Invalid('expected dictionary') for key in value.keys(): - try: - slug(key) - except vol.Invalid: - # To ease the breaking change, we allow old slugs for now - # Remove after 0.94 or 1.0 - if re.match(OLD_SLUG_VALIDATION, key): - fixed = util_slugify(key) - INVALID_SLUGS_FOUND[key] = fixed - logging.getLogger(__name__).warning( - "Found invalid slug %s, please update with %s. This " - "will be come a breaking change.", - key, fixed - ) - else: - raise + slug(key) return schema(value) return verify @@ -656,88 +623,7 @@ def key_dependency(key, dependency): # Schemas -class HASchema(vol.Schema): - """Schema class that allows us to mark PREVENT_EXTRA errors as warnings.""" - - def __call__(self, data): - """Override __call__ to mark PREVENT_EXTRA as warning.""" - try: - return super().__call__(data) - except vol.Invalid as orig_err: - if self.extra != vol.PREVENT_EXTRA: - raise - - # orig_error is of type vol.MultipleInvalid (see super __call__) - assert isinstance(orig_err, vol.MultipleInvalid) - # pylint: disable=no-member - # If it fails with PREVENT_EXTRA, try with ALLOW_EXTRA - self.extra = vol.ALLOW_EXTRA - # In case it still fails the following will raise - try: - validated = super().__call__(data) - finally: - self.extra = vol.PREVENT_EXTRA - - # This is a legacy config, print warning - extra_key_errs = [err.path[-1] for err in orig_err.errors - if err.error_message == 'extra keys not allowed'] - - if not extra_key_errs: - # This should not happen (all errors should be extra key - # errors). Let's raise the original error anyway. - raise orig_err - - WHITELIST = [ - re.compile(CONF_NAME), - re.compile(CONF_PLATFORM), - re.compile('.*_topic'), - ] - - msg = "Your configuration contains extra keys " \ - "that the platform does not support.\n" \ - "Please remove " - submsg = ', '.join('[{}]'.format(err) for err in - extra_key_errs) - submsg += '. ' - - # Add file+line information, if available - if hasattr(data, '__config_file__'): - submsg += " (See {}, line {}). ".format( - data.__config_file__, data.__line__) - - # Add configuration source information, if available - if hasattr(data, '__configuration_source__'): - submsg += "\nConfiguration source: {}. ".format( - data.__configuration_source__) - redacted_data = {} - - # Print configuration causing the error, but filter any potentially - # sensitive data - for k, v in data.items(): - if (any(regex.match(k) for regex in WHITELIST) or - k in extra_key_errs): - redacted_data[k] = v - else: - redacted_data[k] = '' - submsg += "\nOffending data: {}".format( - json.dumps(redacted_data)) - - msg += submsg - logging.getLogger(__name__).warning(msg) - INVALID_EXTRA_KEYS_FOUND.append(submsg) - - # Return legacy validated config - return validated - - def extend(self, schema, required=None, extra=None): - """Extend this schema and convert it to HASchema if necessary.""" - ret = super().extend(schema, required=required, extra=extra) - if extra is not None: - return ret - return HASchema(ret.schema, required=required, extra=self.extra) - - -PLATFORM_SCHEMA = HASchema({ +PLATFORM_SCHEMA = vol.Schema({ vol.Required(CONF_PLATFORM): string, vol.Optional(CONF_ENTITY_NAMESPACE): string, vol.Optional(CONF_SCAN_INTERVAL): time_period diff --git a/tests/components/mqtt/test_legacy_vacuum.py b/tests/components/mqtt/test_legacy_vacuum.py index 8beceb7d6606..f8bef1755409 100644 --- a/tests/components/mqtt/test_legacy_vacuum.py +++ b/tests/components/mqtt/test_legacy_vacuum.py @@ -612,7 +612,6 @@ async def test_setting_attribute_via_mqtt_json_message(hass, mqtt_mock): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -629,7 +628,6 @@ async def test_update_with_json_attrs_not_dict(hass, mqtt_mock, caplog): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -647,7 +645,6 @@ async def test_update_with_json_attrs_bad_json(hass, mqtt_mock, caplog): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -766,7 +763,6 @@ async def test_entity_device_info_update(hass, mqtt_mock): config = { 'platform': 'mqtt', 'name': 'Test 1', - 'state_topic': 'test-topic', 'command_topic': 'test-command-topic', 'device': { 'identifiers': ['helloworld'], diff --git a/tests/components/mqtt/test_state_vacuum.py b/tests/components/mqtt/test_state_vacuum.py index ecd63a1dcdc9..588a808ecfb4 100644 --- a/tests/components/mqtt/test_state_vacuum.py +++ b/tests/components/mqtt/test_state_vacuum.py @@ -331,8 +331,7 @@ async def test_discovery_removal_vacuum(hass, mqtt_mock): data = ( '{ "name": "Beer",' - ' "command_topic": "test_topic",' - ' "component": "state" }' + ' "command_topic": "test_topic"}' ) async_fire_mqtt_message(hass, 'homeassistant/vacuum/bla/config', @@ -357,13 +356,11 @@ async def test_discovery_broken(hass, mqtt_mock, caplog): data1 = ( '{ "name": "Beer",' - ' "command_topic": "test_topic#",' - ' "component": "state" }' + ' "command_topic": "test_topic#"}' ) data2 = ( '{ "name": "Milk",' - ' "command_topic": "test_topic",' - ' "component": "state" }' + ' "command_topic": "test_topic"}' ) async_fire_mqtt_message(hass, 'homeassistant/vacuum/bla/config', @@ -391,13 +388,11 @@ async def test_discovery_update_vacuum(hass, mqtt_mock): data1 = ( '{ "name": "Beer",' - ' "command_topic": "test_topic",' - '"component": "state" }' + ' "command_topic": "test_topic"}' ) data2 = ( '{ "name": "Milk",' - ' "command_topic": "test_topic",' - ' "component": "state"}' + ' "command_topic": "test_topic"}' ) async_fire_mqtt_message(hass, 'homeassistant/vacuum/bla/config', @@ -425,7 +420,6 @@ async def test_setting_attribute_via_mqtt_json_message(hass, mqtt_mock): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -442,7 +436,6 @@ async def test_update_with_json_attrs_not_dict(hass, mqtt_mock, caplog): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -460,7 +453,6 @@ async def test_update_with_json_attrs_bad_json(hass, mqtt_mock, caplog): vacuum.DOMAIN: { 'platform': 'mqtt', 'name': 'test', - 'state_topic': 'test-topic', 'json_attributes_topic': 'attr-topic' } }) @@ -579,7 +571,6 @@ async def test_entity_device_info_update(hass, mqtt_mock): config = { 'platform': 'mqtt', 'name': 'Test 1', - 'state_topic': 'test-topic', 'command_topic': 'test-command-topic', 'device': { 'identifiers': ['helloworld'], diff --git a/tests/helpers/test_config_validation.py b/tests/helpers/test_config_validation.py index e0bd509d3304..4b65904b8b26 100644 --- a/tests/helpers/test_config_validation.py +++ b/tests/helpers/test_config_validation.py @@ -943,34 +943,6 @@ def test_comp_entity_ids(): schema(invalid) -def test_schema_with_slug_keys_allows_old_slugs(caplog): - """Test schema with slug keys allowing old slugs.""" - schema = cv.schema_with_slug_keys(str) - - with patch.dict(cv.INVALID_SLUGS_FOUND, clear=True): - for value in ('_world', 'wow__yeah'): - caplog.clear() - # Will raise if not allowing old slugs - schema({value: 'yo'}) - assert "Found invalid slug {}".format(value) in caplog.text - - assert len(cv.INVALID_SLUGS_FOUND) == 2 - - -def test_entity_id_allow_old_validation(caplog): - """Test schema allowing old entity_ids.""" - schema = vol.Schema(cv.entity_id) - - with patch.dict(cv.INVALID_ENTITY_IDS_FOUND, clear=True): - for value in ('hello.__world', 'great.wow__yeah'): - caplog.clear() - # Will raise if not allowing old entity ID - schema(value) - assert "Found invalid entity_id {}".format(value) in caplog.text - - assert len(cv.INVALID_ENTITY_IDS_FOUND) == 2 - - def test_uuid4_hex(caplog): """Test uuid validation.""" schema = vol.Schema(cv.uuid4_hex) diff --git a/tests/test_setup.py b/tests/test_setup.py index 1dae51966beb..410d97b288d4 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -108,37 +108,6 @@ class TestSetup: 'platform_conf.whatever', MockPlatform(platform_schema=platform_schema)) - with assert_setup_component(1): - assert setup.setup_component(self.hass, 'platform_conf', { - 'platform_conf': { - 'platform': 'whatever', - 'hello': 'world', - 'invalid': 'extra', - } - }) - assert caplog.text.count('Your configuration contains ' - 'extra keys') == 1 - - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - - with assert_setup_component(2): - assert setup.setup_component(self.hass, 'platform_conf', { - 'platform_conf': { - 'platform': 'whatever', - 'hello': 'world', - }, - 'platform_conf 2': { - 'platform': 'whatever', - 'invalid': True - } - }) - assert caplog.text.count('Your configuration contains ' - 'extra keys') == 2 - - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - with assert_setup_component(0): assert setup.setup_component(self.hass, 'platform_conf', { 'platform_conf': { @@ -206,21 +175,6 @@ class TestSetup: MockPlatform('whatever', platform_schema=platform_schema)) - with assert_setup_component(1): - assert setup.setup_component(self.hass, 'platform_conf', { - # fail: no extra keys allowed in platform schema - 'platform_conf': { - 'platform': 'whatever', - 'hello': 'world', - 'invalid': 'extra', - } - }) - assert caplog.text.count('Your configuration contains ' - 'extra keys') == 1 - - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - with assert_setup_component(1): assert setup.setup_component(self.hass, 'platform_conf', { # pass @@ -235,9 +189,6 @@ class TestSetup: } }) - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - def test_validate_platform_config_3(self, caplog): """Test fallback to component PLATFORM_SCHEMA.""" component_schema = PLATFORM_SCHEMA_BASE.extend({ @@ -258,20 +209,6 @@ class TestSetup: MockPlatform('whatever', platform_schema=platform_schema)) - with assert_setup_component(1): - assert setup.setup_component(self.hass, 'platform_conf', { - 'platform_conf': { - 'platform': 'whatever', - 'hello': 'world', - 'invalid': 'extra', - } - }) - assert caplog.text.count('Your configuration contains ' - 'extra keys') == 1 - - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - with assert_setup_component(1): assert setup.setup_component(self.hass, 'platform_conf', { # pass @@ -286,9 +223,6 @@ class TestSetup: } }) - self.hass.data.pop(setup.DATA_SETUP) - self.hass.config.components.remove('platform_conf') - def test_validate_platform_config_4(self): """Test entity_namespace in PLATFORM_SCHEMA.""" component_schema = PLATFORM_SCHEMA_BASE