From d5fad335992c185fe668574574240f109cd5bc69 Mon Sep 17 00:00:00 2001 From: Rohan Kapoor Date: Fri, 8 Feb 2019 02:14:50 -0800 Subject: [PATCH] Add better handling of deprecated configs (#20565) * Add better handling of deprecated configs * Embed the call to has_at_most_one_key in deprecated * Add tests for checking the deprecated logs * Add thoroughly documented tests * Always check has_at_most_one_key * Fix typing * Move logging helpers to homea new logging helper * Lint * Rename to KeywordMessage instead of BraceMessage * Remove unneeded KeywordStyleAdapter * Lint * Use dict directly rather than dict.keys() when creating set * Patch the version in unit tests, update logging and use parse_version * Re-add KeywordStyleAdapter and fix tests * Lint * Lint --- homeassistant/components/freedns/__init__.py | 32 +- homeassistant/helpers/config_validation.py | 111 ++++++- homeassistant/helpers/logging.py | 49 +++ tests/components/freedns/test_init.py | 2 +- tests/helpers/test_config_validation.py | 333 ++++++++++++++++++- 5 files changed, 487 insertions(+), 40 deletions(-) create mode 100644 homeassistant/helpers/logging.py diff --git a/homeassistant/components/freedns/__init__.py b/homeassistant/components/freedns/__init__.py index ec38bb59cc7..7da51cd42e4 100644 --- a/homeassistant/components/freedns/__init__.py +++ b/homeassistant/components/freedns/__init__.py @@ -13,7 +13,7 @@ import async_timeout import voluptuous as vol from homeassistant.const import (CONF_URL, CONF_ACCESS_TOKEN, - CONF_UPDATE_INTERVAL) + CONF_UPDATE_INTERVAL, CONF_SCAN_INTERVAL) import homeassistant.helpers.config_validation as cv _LOGGER = logging.getLogger(__name__) @@ -26,21 +26,31 @@ TIMEOUT = 10 UPDATE_URL = 'https://freedns.afraid.org/dynamic/update.php' CONFIG_SCHEMA = vol.Schema({ - DOMAIN: vol.Schema({ - vol.Exclusive(CONF_URL, DOMAIN): cv.string, - vol.Exclusive(CONF_ACCESS_TOKEN, DOMAIN): cv.string, - vol.Optional(CONF_UPDATE_INTERVAL, default=DEFAULT_INTERVAL): vol.All( - cv.time_period, cv.positive_timedelta), - - }) + DOMAIN: vol.All( + vol.Schema({ + vol.Exclusive(CONF_URL, DOMAIN): cv.string, + vol.Exclusive(CONF_ACCESS_TOKEN, DOMAIN): cv.string, + vol.Optional(CONF_UPDATE_INTERVAL): + vol.All(cv.time_period, cv.positive_timedelta), + vol.Optional(CONF_SCAN_INTERVAL): + vol.All(cv.time_period, cv.positive_timedelta), + }), + cv.deprecated( + CONF_UPDATE_INTERVAL, + replacement_key=CONF_SCAN_INTERVAL, + invalidation_version='1.0.0', + default=DEFAULT_INTERVAL + ) + ) }, extra=vol.ALLOW_EXTRA) async def async_setup(hass, config): """Initialize the FreeDNS component.""" - url = config[DOMAIN].get(CONF_URL) - auth_token = config[DOMAIN].get(CONF_ACCESS_TOKEN) - update_interval = config[DOMAIN].get(CONF_UPDATE_INTERVAL) + conf = config[DOMAIN] + url = conf.get(CONF_URL) + auth_token = conf.get(CONF_ACCESS_TOKEN) + update_interval = conf[CONF_SCAN_INTERVAL] session = hass.helpers.aiohttp_client.async_get_clientsession() diff --git a/homeassistant/helpers/config_validation.py b/homeassistant/helpers/config_validation.py index 3a4b9ced0ab..3b01a01fc96 100644 --- a/homeassistant/helpers/config_validation.py +++ b/homeassistant/helpers/config_validation.py @@ -1,27 +1,29 @@ """Helpers for config validation using voluptuous.""" -from datetime import (timedelta, datetime as datetime_sys, - time as time_sys, date as date_sys) +import inspect +import logging import os import re -from urllib.parse import urlparse +from datetime import (timedelta, datetime as datetime_sys, + time as time_sys, date as date_sys) from socket import _GLOBAL_DEFAULT_TIMEOUT -import logging -import inspect -from typing import Any, Union, TypeVar, Callable, Sequence, Dict +from typing import Any, Union, TypeVar, Callable, Sequence, Dict, Optional +from urllib.parse import urlparse import voluptuous as vol +from pkg_resources import parse_version +import homeassistant.util.dt as dt_util from homeassistant.const import ( CONF_PLATFORM, CONF_SCAN_INTERVAL, TEMP_CELSIUS, TEMP_FAHRENHEIT, CONF_ALIAS, CONF_ENTITY_ID, CONF_VALUE_TEMPLATE, WEEKDAYS, CONF_CONDITION, CONF_BELOW, CONF_ABOVE, CONF_TIMEOUT, SUN_EVENT_SUNSET, SUN_EVENT_SUNRISE, CONF_UNIT_SYSTEM_IMPERIAL, CONF_UNIT_SYSTEM_METRIC, - ENTITY_MATCH_ALL, CONF_ENTITY_NAMESPACE) + ENTITY_MATCH_ALL, CONF_ENTITY_NAMESPACE, __version__) from homeassistant.core import valid_entity_id, split_entity_id from homeassistant.exceptions import TemplateError -import homeassistant.util.dt as dt_util -from homeassistant.util import slugify as util_slugify from homeassistant.helpers import template as template_helper +from homeassistant.helpers.logging import KeywordStyleAdapter +from homeassistant.util import slugify as util_slugify # pylint: disable=invalid-name @@ -67,6 +69,22 @@ def has_at_least_one_key(*keys: str) -> Callable: return validate +def has_at_most_one_key(*keys: str) -> Callable: + """Validate that zero keys exist or one key exists.""" + def validate(obj: Dict) -> Dict: + """Test zero keys exist or one key exists in dict.""" + if not isinstance(obj, dict): + raise vol.Invalid('expected dictionary') + + if len(set(keys) & set(obj)) > 1: + raise vol.Invalid( + 'must contain at most one of {}.'.format(', '.join(keys)) + ) + return obj + + return validate + + def boolean(value: Any) -> bool: """Validate and coerce a boolean value.""" if isinstance(value, str): @@ -520,18 +538,79 @@ def ensure_list_csv(value: Any) -> Sequence: return ensure_list(value) -def deprecated(key): - """Log key as deprecated.""" +def deprecated(key: str, + replacement_key: Optional[str] = None, + invalidation_version: Optional[str] = None, + default: Optional[Any] = None): + """ + Log key as deprecated and provide a replacement (if exists). + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema moving the value from key to replacement_key + - Processes schema changing nothing if only replacement_key provided + - No warning if only replacement_key provided + - No warning if neither key nor replacement_key are provided + - Adds replacement_key with default value in this case + - Once the invalidation_version is crossed, raises vol.Invalid if key + is detected + """ module_name = inspect.getmodule(inspect.stack()[1][0]).__name__ - def validator(config): + if replacement_key and invalidation_version: + warning = ("The '{key}' option (with value '{value}') is" + " deprecated, please replace it with '{replacement_key}'." + " This option will become invalid in version" + " {invalidation_version}") + elif replacement_key: + warning = ("The '{key}' option (with value '{value}') is" + " deprecated, please replace it with '{replacement_key}'") + elif invalidation_version: + warning = ("The '{key}' option (with value '{value}') is" + " deprecated, please remove it from your configuration." + " This option will become invalid in version" + " {invalidation_version}") + else: + warning = ("The '{key}' option (with value '{value}') is" + " deprecated, please remove it from your configuration") + + def check_for_invalid_version(value: Optional[Any]): + """Raise error if current version has reached invalidation.""" + if not invalidation_version: + return + + if parse_version(__version__) >= parse_version(invalidation_version): + raise vol.Invalid( + warning.format( + key=key, + value=value, + replacement_key=replacement_key, + invalidation_version=invalidation_version + ) + ) + + def validator(config: Dict): """Check if key is in config and log warning.""" if key in config: - logging.getLogger(module_name).warning( - "The '%s' option (with value '%s') is deprecated, please " - "remove it from your configuration.", key, config[key]) + value = config[key] + check_for_invalid_version(value) + KeywordStyleAdapter(logging.getLogger(module_name)).warning( + warning, + key=key, + value=value, + replacement_key=replacement_key, + invalidation_version=invalidation_version + ) + if replacement_key: + config.pop(key) + else: + value = default + if (replacement_key + and replacement_key not in config + and value is not None): + config[replacement_key] = value - return config + return has_at_most_one_key(key, replacement_key)(config) return validator diff --git a/homeassistant/helpers/logging.py b/homeassistant/helpers/logging.py new file mode 100644 index 00000000000..ea596eb3c15 --- /dev/null +++ b/homeassistant/helpers/logging.py @@ -0,0 +1,49 @@ +"""Helpers for logging allowing more advanced logging styles to be used.""" +import inspect +import logging + + +class KeywordMessage: + """ + Represents a logging message with keyword arguments. + + Adapted from: https://stackoverflow.com/a/24683360/2267718 + """ + + def __init__(self, fmt, args, kwargs): + """Initialize a new BraceMessage object.""" + self._fmt = fmt + self._args = args + self._kwargs = kwargs + + def __str__(self): + """Convert the object to a string for logging.""" + return str(self._fmt).format(*self._args, **self._kwargs) + + +class KeywordStyleAdapter(logging.LoggerAdapter): + """Represents an adapter wrapping the logger allowing KeywordMessages.""" + + def __init__(self, logger, extra=None): + """Initialize a new StyleAdapter for the provided logger.""" + super(KeywordStyleAdapter, self).__init__(logger, extra or {}) + + def log(self, level, msg, *args, **kwargs): + """Log the message provided at the appropriate level.""" + if self.isEnabledFor(level): + msg, log_kwargs = self.process(msg, kwargs) + self.logger._log( # pylint: disable=protected-access + level, KeywordMessage(msg, args, kwargs), (), **log_kwargs + ) + + def process(self, msg, kwargs): + """Process the keyward args in preparation for logging.""" + return ( + msg, + { + k: kwargs[k] + for k in inspect.getfullargspec( + self.logger._log # pylint: disable=protected-access + ).args[1:] if k in kwargs + } + ) diff --git a/tests/components/freedns/test_init.py b/tests/components/freedns/test_init.py index b8e38e9c3a8..784926912cd 100644 --- a/tests/components/freedns/test_init.py +++ b/tests/components/freedns/test_init.py @@ -40,7 +40,7 @@ def test_setup(hass, aioclient_mock): result = yield from async_setup_component(hass, freedns.DOMAIN, { freedns.DOMAIN: { 'access_token': ACCESS_TOKEN, - 'update_interval': UPDATE_INTERVAL, + 'scan_interval': UPDATE_INTERVAL, } }) assert result diff --git a/tests/helpers/test_config_validation.py b/tests/helpers/test_config_validation.py index 119725b06dd..cefde564035 100644 --- a/tests/helpers/test_config_validation.py +++ b/tests/helpers/test_config_validation.py @@ -5,6 +5,7 @@ import os from socket import _GLOBAL_DEFAULT_TIMEOUT from unittest.mock import Mock, patch +import homeassistant import pytest import voluptuous as vol @@ -275,7 +276,6 @@ def test_time_period(): {}, {'wrong_key': -10} ) for value in options: - with pytest.raises(vol.MultipleInvalid): schema(value) @@ -489,26 +489,323 @@ def test_datetime(): schema('2016-11-23T18:59:08') -def test_deprecated(caplog): - """Test deprecation log.""" - schema = vol.Schema({ +@pytest.fixture +def schema(): + """Create a schema used for testing deprecation.""" + return vol.Schema({ 'venus': cv.boolean, - 'mars': cv.boolean + 'mars': cv.boolean, + 'jupiter': cv.boolean }) + + +@pytest.fixture +def version(monkeypatch): + """Patch the version used for testing to 0.5.0.""" + monkeypatch.setattr(homeassistant.const, '__version__', '0.5.0') + + +def test_deprecated_with_no_optionals(caplog, schema): + """ + Test deprecation behaves correctly when optional params are None. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema without changing any values + - No warning or difference in output if key is not provided + """ deprecated_schema = vol.All( cv.deprecated('mars'), schema ) - deprecated_schema({'venus': True}) - # pylint: disable=len-as-condition - assert len(caplog.records) == 0 - - deprecated_schema({'mars': True}) + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) assert len(caplog.records) == 1 assert caplog.records[0].name == __name__ assert ("The 'mars' option (with value 'True') is deprecated, " - "please remove it from your configuration.") in caplog.text + "please remove it from your configuration") in caplog.text + assert test_data == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + +def test_deprecated_with_replacement_key(caplog, schema): + """ + Test deprecation behaves correctly when only a replacement key is provided. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema moving the value from key to replacement_key + - Processes schema changing nothing if only replacement_key provided + - No warning if only replacement_key provided + - No warning or difference in output if neither key nor + replacement_key are provided + """ + deprecated_schema = vol.All( + cv.deprecated('mars', replacement_key='jupiter'), + schema + ) + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'") in caplog.text + assert {'jupiter': True} == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'jupiter': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + +def test_deprecated_with_invalidation_version(caplog, schema, version): + """ + Test deprecation behaves correctly with only an invalidation_version. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema without changing any values + - No warning or difference in output if key is not provided + - Once the invalidation_version is crossed, raises vol.Invalid if key + is detected + """ + deprecated_schema = vol.All( + cv.deprecated('mars', invalidation_version='1.0.0'), + schema + ) + + message = ("The 'mars' option (with value 'True') is deprecated, " + "please remove it from your configuration. " + "This option will become invalid in version 1.0.0") + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert message in caplog.text + assert test_data == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'venus': False} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + invalidated_schema = vol.All( + cv.deprecated('mars', invalidation_version='0.1.0'), + schema + ) + test_data = {'mars': True} + with pytest.raises(vol.MultipleInvalid) as exc_info: + invalidated_schema(test_data) + assert ("The 'mars' option (with value 'True') is deprecated, " + "please remove it from your configuration. This option will " + "become invalid in version 0.1.0") == str(exc_info.value) + + +def test_deprecated_with_replacement_key_and_invalidation_version( + caplog, schema, version +): + """ + Test deprecation behaves with a replacement key & invalidation_version. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema moving the value from key to replacement_key + - Processes schema changing nothing if only replacement_key provided + - No warning if only replacement_key provided + - No warning or difference in output if neither key nor + replacement_key are provided + - Once the invalidation_version is crossed, raises vol.Invalid if key + is detected + """ + deprecated_schema = vol.All( + cv.deprecated( + 'mars', replacement_key='jupiter', invalidation_version='1.0.0' + ), + schema + ) + + warning = ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'. This option will become " + "invalid in version 1.0.0") + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert warning in caplog.text + assert {'jupiter': True} == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'jupiter': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + invalidated_schema = vol.All( + cv.deprecated( + 'mars', replacement_key='jupiter', invalidation_version='0.1.0' + ), + schema + ) + test_data = {'mars': True} + with pytest.raises(vol.MultipleInvalid) as exc_info: + invalidated_schema(test_data) + assert ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'. This option will become " + "invalid in version 0.1.0") == str(exc_info.value) + + +def test_deprecated_with_default(caplog, schema): + """ + Test deprecation behaves correctly with a default value. + + This is likely a scenario that would never occur. + + Expected behavior: + - Behaves identically as when the default value was not present + """ + deprecated_schema = vol.All( + cv.deprecated('mars', default=False), + schema + ) + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert caplog.records[0].name == __name__ + assert ("The 'mars' option (with value 'True') is deprecated, " + "please remove it from your configuration") in caplog.text + assert test_data == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + +def test_deprecated_with_replacement_key_and_default(caplog, schema): + """ + Test deprecation behaves correctly when only a replacement key is provided. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema moving the value from key to replacement_key + - Processes schema changing nothing if only replacement_key provided + - No warning if only replacement_key provided + - No warning if neither key nor replacement_key are provided + - Adds replacement_key with default value in this case + """ + deprecated_schema = vol.All( + cv.deprecated('mars', replacement_key='jupiter', default=False), + schema + ) + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'") in caplog.text + assert {'jupiter': True} == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'jupiter': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert {'venus': True, 'jupiter': False} == output + + +def test_deprecated_with_replacement_key_invalidation_version_default( + caplog, schema, version +): + """ + Test deprecation with a replacement key, invalidation_version & default. + + Expected behavior: + - Outputs the appropriate deprecation warning if key is detected + - Processes schema moving the value from key to replacement_key + - Processes schema changing nothing if only replacement_key provided + - No warning if only replacement_key provided + - No warning if neither key nor replacement_key are provided + - Adds replacement_key with default value in this case + - Once the invalidation_version is crossed, raises vol.Invalid if key + is detected + """ + deprecated_schema = vol.All( + cv.deprecated( + 'mars', replacement_key='jupiter', invalidation_version='1.0.0', + default=False + ), + schema + ) + + test_data = {'mars': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 1 + assert ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'. This option will become " + "invalid in version 1.0.0") in caplog.text + assert {'jupiter': True} == output + + caplog.clear() + assert len(caplog.records) == 0 + + test_data = {'jupiter': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert test_data == output + + test_data = {'venus': True} + output = deprecated_schema(test_data.copy()) + assert len(caplog.records) == 0 + assert {'venus': True, 'jupiter': False} == output + + invalidated_schema = vol.All( + cv.deprecated( + 'mars', replacement_key='jupiter', invalidation_version='0.1.0' + ), + schema + ) + test_data = {'mars': True} + with pytest.raises(vol.MultipleInvalid) as exc_info: + invalidated_schema(test_data) + assert ("The 'mars' option (with value 'True') is deprecated, " + "please replace it with 'jupiter'. This option will become " + "invalid in version 0.1.0") == str(exc_info.value) def test_key_dependency(): @@ -530,6 +827,18 @@ def test_key_dependency(): schema(value) +def test_has_at_most_one_key(): + """Test has_at_most_one_key validator.""" + schema = vol.Schema(cv.has_at_most_one_key('beer', 'soda')) + + for value in (None, [], {'beer': None, 'soda': None}): + with pytest.raises(vol.MultipleInvalid): + schema(value) + + for value in ({}, {'beer': None}, {'soda': None}): + schema(value) + + def test_has_at_least_one_key(): """Test has_at_least_one_key validator.""" schema = vol.Schema(cv.has_at_least_one_key('beer', 'soda')) @@ -582,7 +891,7 @@ def test_matches_regex(): schema(" nrtd ") test_str = "This is a test including uiae." - assert(schema(test_str) == test_str) + assert (schema(test_str) == test_str) def test_is_regex():