From 4d394a1255a3261bbaf112cc3efcf74fe9c17e39 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 18:45:14 -0700 Subject: [PATCH 1/8] refactor(feature-toggles): Code coverage and housekeeping Changes: - Add missing test coverage - Fix some MyPy error - Use NumPy docstrings - Use *Error naming convention for exceptions --- .../utilities/feature_toggles/__init__.py | 4 +-- .../feature_toggles/appconfig_fetcher.py | 34 ++++++++++++------- .../feature_toggles/configuration_store.py | 6 ++-- .../utilities/feature_toggles/exceptions.py | 2 +- .../utilities/feature_toggles/schema.py | 4 +-- .../feature_toggles/test_feature_toggles.py | 21 ++++++++++++ .../feature_toggles/test_schema_validation.py | 34 +++++++++---------- 7 files changed, 68 insertions(+), 37 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/__init__.py b/aws_lambda_powertools/utilities/feature_toggles/__init__.py index 378f7e23f48..04237d63812 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/__init__.py +++ b/aws_lambda_powertools/utilities/feature_toggles/__init__.py @@ -2,12 +2,12 @@ """ from .appconfig_fetcher import AppConfigFetcher from .configuration_store import ConfigurationStore -from .exceptions import ConfigurationException +from .exceptions import ConfigurationError from .schema import ACTION, SchemaValidator from .schema_fetcher import SchemaFetcher __all__ = [ - "ConfigurationException", + "ConfigurationError", "ConfigurationStore", "ACTION", "SchemaValidator", diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py index 177d4ed0ae9..83d29a9807e 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py @@ -5,7 +5,7 @@ from aws_lambda_powertools.utilities.parameters import AppConfigProvider, GetParameterError, TransformParameterError -from .exceptions import ConfigurationException +from .exceptions import ConfigurationError from .schema_fetcher import SchemaFetcher logger = logging.getLogger(__name__) @@ -25,12 +25,18 @@ def __init__( ): """This class fetches JSON schemas from AWS AppConfig - Args: - environment (str): what appconfig environment to use 'dev/test' etc. - service (str): what service name to use from the supplied environment - configuration_name (str): what configuration to take from the environment & service combination - cache_seconds (int): cache expiration time, how often to call AppConfig to fetch latest configuration - config (Optional[Config]): boto3 client configuration + Parameters + ---------- + environment: str + what appconfig environment to use 'dev/test' etc. + service: str + what service name to use from the supplied environment + configuration_name: str + what configuration to take from the environment & service combination + cache_seconds: int + cache expiration time, how often to call AppConfig to fetch latest configuration + config: Optional[Config] + boto3 client configuration """ super().__init__(configuration_name, cache_seconds) self._logger = logger @@ -39,11 +45,15 @@ def __init__( def get_json_configuration(self) -> Dict[str, Any]: """Get configuration string from AWs AppConfig and return the parsed JSON dictionary - Raises: - ConfigurationException: Any validation error or appconfig error that can occur + Raises + ------ + ConfigurationException + Any validation error or appconfig error that can occur - Returns: - Dict[str, Any]: parsed JSON dictionary + Returns + ------- + Dict[str, Any] + parsed JSON dictionary """ try: return self._conf_store.get( @@ -54,4 +64,4 @@ def get_json_configuration(self) -> Dict[str, Any]: except (GetParameterError, TransformParameterError) as exc: error_str = f"unable to get AWS AppConfig configuration file, exception={str(exc)}" self._logger.error(error_str) - raise ConfigurationException(error_str) + raise ConfigurationError(error_str) diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py index e5404477375..0a516b505bf 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py @@ -2,7 +2,7 @@ from typing import Any, Dict, List, Optional from . import schema -from .exceptions import ConfigurationException +from .exceptions import ConfigurationError from .schema_fetcher import SchemaFetcher logger = logging.getLogger(__name__) @@ -122,7 +122,7 @@ def get_feature_toggle( try: toggles_dict: Dict[str, Any] = self.get_configuration() - except ConfigurationException: + except ConfigurationError: logger.error("unable to get feature toggles JSON, returning provided value_if_missing value") # noqa: E501 return value_if_missing @@ -167,7 +167,7 @@ def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, A rules_context = {} try: toggles_dict: Dict[str, Any] = self.get_configuration() - except ConfigurationException: + except ConfigurationError: logger.error("unable to get feature toggles JSON") # noqa: E501 return [] ret_list = [] diff --git a/aws_lambda_powertools/utilities/feature_toggles/exceptions.py b/aws_lambda_powertools/utilities/feature_toggles/exceptions.py index 9bbb5f200ba..d87f9a39dec 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/exceptions.py +++ b/aws_lambda_powertools/utilities/feature_toggles/exceptions.py @@ -1,2 +1,2 @@ -class ConfigurationException(Exception): +class ConfigurationError(Exception): """When a a configuration store raises an exception on config retrieval or parsing""" diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_toggles/schema.py index 58e75fabfc2..d57e247d7e0 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema.py @@ -1,7 +1,7 @@ from enum import Enum from typing import Any, Dict -from .exceptions import ConfigurationException +from .exceptions import ConfigurationError FEATURES_KEY = "features" RULES_KEY = "rules" @@ -27,7 +27,7 @@ def __init__(self, logger: object): def _raise_conf_exc(self, error_str: str) -> None: self._logger.error(error_str) - raise ConfigurationException(error_str) + raise ConfigurationError(error_str) def _validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 27f89eb1513..656313733b6 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -3,9 +3,11 @@ import pytest # noqa: F401 from botocore.config import Config +from aws_lambda_powertools.utilities.feature_toggles import ConfigurationError from aws_lambda_powertools.utilities.feature_toggles.appconfig_fetcher import AppConfigFetcher from aws_lambda_powertools.utilities.feature_toggles.configuration_store import ConfigurationStore from aws_lambda_powertools.utilities.feature_toggles.schema import ACTION +from aws_lambda_powertools.utilities.parameters import GetParameterError @pytest.fixture(scope="module") @@ -420,3 +422,22 @@ def test_multiple_features_only_some_enabled(mocker, config): rules_context={"tenant_id": "6", "username": "a"} ) assert enabled_list == expected_value + + +def test_app_config_get_parameter_err(mocker): + # GIVEN an appconfig with a missing config + mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") + mocked_get_conf.side_effect = GetParameterError() + app_conf_fetcher = AppConfigFetcher( + environment="env", + service="service", + configuration_name="conf", + cache_seconds=1, + ) + + # WHEN calling get_json_configuration + with pytest.raises(ConfigurationError) as err: + app_conf_fetcher.get_json_configuration() + + # THEN raise ConfigurationException error + assert "AWS AppConfig configuration" in str(err.value) diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 3b024c854b9..6abc9a9bef9 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -2,7 +2,7 @@ import pytest # noqa: F401 -from aws_lambda_powertools.utilities.feature_toggles.exceptions import ConfigurationException +from aws_lambda_powertools.utilities.feature_toggles.exceptions import ConfigurationError from aws_lambda_powertools.utilities.feature_toggles.schema import ( ACTION, CONDITION_ACTION, @@ -24,17 +24,17 @@ def test_invalid_features_dict(): schema = {} # empty dict validator = SchemaValidator(logger) - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) schema = [] # invalid type - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # invalid features key schema = {FEATURES_KEY: []} - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) @@ -48,27 +48,27 @@ def test_invalid_feature_dict(): # invalid feature type, not dict schema = {FEATURES_KEY: {"my_feature": []}} validator = SchemaValidator(logger) - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # empty feature dict schema = {FEATURES_KEY: {"my_feature": {}}} - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}} - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean #2 schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: 5}}} - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # invalid rules type, not list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}} - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) @@ -97,7 +97,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(logger) - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # rules RULE_DEFAULT_VALUE is not bool @@ -114,7 +114,7 @@ def test_invalid_rule(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # missing conditions list @@ -131,7 +131,7 @@ def test_invalid_rule(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # condition list is empty @@ -145,7 +145,7 @@ def test_invalid_rule(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # condition is invalid type, not list @@ -159,7 +159,7 @@ def test_invalid_rule(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) @@ -180,7 +180,7 @@ def test_invalid_condition(): } } validator = SchemaValidator(logger) - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # missing condition key and value @@ -198,7 +198,7 @@ def test_invalid_condition(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) # invalid condition key type, not string @@ -220,7 +220,7 @@ def test_invalid_condition(): } } } - with pytest.raises(ConfigurationException): + with pytest.raises(ConfigurationError): validator.validate_json_schema(schema) From f3b478166ee1cde3cec28cd01728dcb07e34f9e7 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 19:06:04 -0700 Subject: [PATCH 2/8] chore: use abstractmethod and wrap long lines --- .../feature_toggles/appconfig_fetcher.py | 2 +- .../feature_toggles/configuration_store.py | 98 ++++++++++++------- .../feature_toggles/schema_fetcher.py | 18 ++-- .../feature_toggles/test_feature_toggles.py | 2 +- 4 files changed, 73 insertions(+), 47 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py index 83d29a9807e..ae7c6c90e51 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py @@ -47,7 +47,7 @@ def get_json_configuration(self) -> Dict[str, Any]: Raises ------ - ConfigurationException + ConfigurationError Any validation error or appconfig error that can occur Returns diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py index 0a516b505bf..cc061696c82 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py @@ -49,15 +49,18 @@ def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], rules_contex context_value, ): logger.debug( - f"rule did not match action, rule_name={rule_name}, rule_default_value={rule_default_value}, feature_name={feature_name}, context_value={str(context_value)}" # noqa: E501 + f"rule did not match action, rule_name={rule_name}, rule_default_value={rule_default_value}, " + f"feature_name={feature_name}, context_value={str(context_value)} " ) # context doesn't match condition return False # if we got here, all conditions match logger.debug( - f"rule matched, rule_name={rule_name}, rule_default_value={rule_default_value}, feature_name={feature_name}" # noqa: E501 + f"rule matched, rule_name={rule_name}, rule_default_value={rule_default_value}, " + f"feature_name={feature_name}" ) return True + return False def _handle_rules( self, @@ -73,27 +76,33 @@ def _handle_rules( return rule_default_value # no rule matched, return default value of feature logger.debug( - f"no rule matched, returning default value of feature, feature_default_value={feature_default_value}, feature_name={feature_name}" # noqa: E501 + f"no rule matched, returning default value of feature, feature_default_value={feature_default_value}, " + f"feature_name={feature_name}" ) return feature_default_value + return False def get_configuration(self) -> Dict[str, Any]: """Get configuration string from AWs AppConfig and returned the parsed JSON dictionary - Raises: - ConfigurationException: Any validation error or appconfig error that can occur + Raises + ------ + ConfigurationError + Any validation error or appconfig error that can occur - Returns: - Dict[str, Any]: parsed JSON dictionary + Returns + ------ + Dict[str, Any] + parsed JSON dictionary """ - schema: Dict[ + config: Dict[ str, Any ] = ( self._schema_fetcher.get_json_configuration() ) # parse result conf as JSON, keep in cache for self.max_age seconds # validate schema - self._schema_validator.validate_json_schema(schema) - return schema + self._schema_validator.validate_json_schema(config) + return config def get_feature_toggle( self, *, feature_name: str, rules_context: Optional[Dict[str, Any]] = None, value_if_missing: bool @@ -101,21 +110,27 @@ def get_feature_toggle( """get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. see below for explanation. - Args: - feature_name (str): feature name that you wish to fetch - rules_context (Optional[Dict[str, Any]]): dict of attributes that you would like to match the rules - against, can be {'tenant_id: 'X', 'username':' 'Y', 'region': 'Z'} etc. - value_if_missing (bool): this will be the returned value in case the feature toggle doesn't exist in - the schema or there has been an error while fetching the - configuration from appconfig - - Returns: - bool: calculated feature toggle value. several possibilities: - 1. if the feature doesn't appear in the schema or there has been an error fetching the - configuration -> error/warning log would appear and value_if_missing is returned - 2. feature exists and has no rules or no rules have matched -> return feature_default_value of - the defined feature - 3. feature exists and a rule matches -> rule_default_value of rule is returned + Parameters + ---------- + feature_name: str + feature name that you wish to fetch + rules_context: Optional[Dict[str, Any]] + dict of attributes that you would like to match the rules + against, can be {'tenant_id: 'X', 'username':' 'Y', 'region': 'Z'} etc. + value_if_missing: bool + this will be the returned value in case the feature toggle doesn't exist in + the schema or there has been an error while fetching the + configuration from appconfig + + Returns + ---------- + bool + calculated feature toggle value. several possibilities: + 1. if the feature doesn't appear in the schema or there has been an error fetching the + configuration -> error/warning log would appear and value_if_missing is returned + 2. feature exists and has no rules or no rules have matched -> return feature_default_value of + the defined feature + 3. feature exists and a rule matches -> rule_default_value of rule is returned """ if rules_context is None: rules_context = {} @@ -123,13 +138,14 @@ def get_feature_toggle( try: toggles_dict: Dict[str, Any] = self.get_configuration() except ConfigurationError: - logger.error("unable to get feature toggles JSON, returning provided value_if_missing value") # noqa: E501 + logger.error("unable to get feature toggles JSON, returning provided value_if_missing value") return value_if_missing feature: Dict[str, Dict] = toggles_dict.get(schema.FEATURES_KEY, {}).get(feature_name, None) if feature is None: logger.warning( - f"feature does not appear in configuration, using provided value_if_missing, feature_name={feature_name}, value_if_missing={value_if_missing}" # noqa: E501 + f"feature does not appear in configuration, using provided value_if_missing, " + f"feature_name={feature_name}, value_if_missing={value_if_missing}" ) return value_if_missing @@ -138,13 +154,14 @@ def get_feature_toggle( if not rules_list: # not rules but has a value logger.debug( - f"no rules found, returning feature default value, feature_name={feature_name}, default_value={feature_default_value}" # noqa: E501 + f"no rules found, returning feature default value, feature_name={feature_name}, " + f"default_value={feature_default_value}" ) return feature_default_value # look for first rule match logger.debug( f"looking for rule match, feature_name={feature_name}, feature_default_value={feature_default_value}" - ) # noqa: E501 + ) return self._handle_rules( feature_name=feature_name, rules_context=rules_context, @@ -153,15 +170,20 @@ def get_feature_toggle( ) def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, Any]] = None) -> List[str]: - """Get all enabled feature toggles while also taking into account rule_context (when a feature has defined rules) - - Args: - rules_context (Optional[Dict[str, Any]]): dict of attributes that you would like to match the rules - against, can be {'tenant_id: 'X', 'username':' 'Y', 'region': 'Z'} etc. - - Returns: - List[str]: a list of all features name that are enabled by also taking into account - rule_context (when a feature has defined rules) + """Get all enabled feature toggles while also taking into account rule_context + (when a feature has defined rules) + + Parameters + ---------- + rules_context: Optional[Dict[str, Any]] + dict of attributes that you would like to match the rules + against, can be `{'tenant_id: 'X', 'username':' 'Y', 'region': 'Z'}` etc. + + Returns + ---------- + List[str] + a list of all features name that are enabled by also taking into account + rule_context (when a feature has defined rules) """ if rules_context is None: rules_context = {} diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py index 37dee63f7f2..89fffe1221d 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py @@ -1,4 +1,4 @@ -from abc import ABC, abstractclassmethod +from abc import ABC, abstractmethod from typing import Any, Dict @@ -7,14 +7,18 @@ def __init__(self, configuration_name: str, cache_seconds: int): self.configuration_name = configuration_name self._cache_seconds = cache_seconds - @abstractclassmethod + @abstractmethod def get_json_configuration(self) -> Dict[str, Any]: """Get configuration string from any configuration storing service and return the parsed JSON dictionary - Raises: - ConfigurationException: Any error that can occur during schema fetch or JSON parse + Raises + ------ + ConfigurationError + Any error that can occur during schema fetch or JSON parse - Returns: - Dict[str, Any]: parsed JSON dictionary + Returns + ------- + Dict[str, Any] + parsed JSON dictionary """ - return None + return NotImplemented # pragma: no cover diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 656313733b6..5624c4604ce 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -439,5 +439,5 @@ def test_app_config_get_parameter_err(mocker): with pytest.raises(ConfigurationError) as err: app_conf_fetcher.get_json_configuration() - # THEN raise ConfigurationException error + # THEN raise ConfigurationError error assert "AWS AppConfig configuration" in str(err.value) From abaace429ad63d3e15d3b6db85f46cddce461699 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 22:07:29 -0700 Subject: [PATCH 3/8] test: fix test by adding config --- tests/functional/feature_toggles/test_feature_toggles.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 5624c4604ce..ad7565ca606 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -424,7 +424,7 @@ def test_multiple_features_only_some_enabled(mocker, config): assert enabled_list == expected_value -def test_app_config_get_parameter_err(mocker): +def test_app_config_get_parameter_err(mocker, config): # GIVEN an appconfig with a missing config mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") mocked_get_conf.side_effect = GetParameterError() @@ -433,6 +433,7 @@ def test_app_config_get_parameter_err(mocker): service="service", configuration_name="conf", cache_seconds=1, + config=config, ) # WHEN calling get_json_configuration From 64c1ec8cce2aeb4fd8e5c09fb9804473bd8784f8 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 22:49:00 -0700 Subject: [PATCH 4/8] test(feature-toggles): Add some missing internal tests --- .../feature_toggles/test_feature_toggles.py | 35 +++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index ad7565ca606..627e4648384 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -1,9 +1,9 @@ from typing import Dict, List -import pytest # noqa: F401 +import pytest from botocore.config import Config -from aws_lambda_powertools.utilities.feature_toggles import ConfigurationError +from aws_lambda_powertools.utilities.feature_toggles import ConfigurationError, schema from aws_lambda_powertools.utilities.feature_toggles.appconfig_fetcher import AppConfigFetcher from aws_lambda_powertools.utilities.feature_toggles.configuration_store import ConfigurationStore from aws_lambda_powertools.utilities.feature_toggles.schema import ACTION @@ -442,3 +442,34 @@ def test_app_config_get_parameter_err(mocker, config): # THEN raise ConfigurationError error assert "AWS AppConfig configuration" in str(err.value) + + +def test_match_by_action_no_matching_action(mocker, config): + # GIVEN an unsupported action + conf_store = init_configuration_store(mocker, {}, config) + # WHEN calling _match_by_action + result = conf_store._match_by_action("Foo", None, "foo") + # THEN default to False + assert result is False + + +def test_match_by_action_attribute_error(mocker, config): + # GIVEN a startswith action and 2 integer + conf_store = init_configuration_store(mocker, {}, config) + # WHEN calling _match_by_action + result = conf_store._match_by_action(ACTION.STARTSWITH.value, 1, 100) + # THEN swallow the AttributeError and return False + assert result is False + + +def test_is_rule_matched_no_matches(mocker, config): + # GIVEN an empty list of conditions + rule = {schema.CONDITIONS_KEY: []} + rules_context = {} + conf_store = init_configuration_store(mocker, {}, config) + + # WHEN calling _is_rule_matched + result = conf_store._is_rule_matched("feature_name", rule, rules_context) + + # THEN return False + assert result is False From 577b5cc346403dbc3f81d9ed157dd25fcd3c197f Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 23:05:04 -0700 Subject: [PATCH 5/8] test(feature-toggles): Remaining configuration_store tests --- .../feature_toggles/configuration_store.py | 5 +- .../feature_toggles/test_feature_toggles.py | 46 +++++++++++++++---- 2 files changed, 41 insertions(+), 10 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py index cc061696c82..fc6d62619f8 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py @@ -187,11 +187,13 @@ def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, A """ if rules_context is None: rules_context = {} + try: toggles_dict: Dict[str, Any] = self.get_configuration() except ConfigurationError: - logger.error("unable to get feature toggles JSON") # noqa: E501 + logger.error("unable to get feature toggles JSON") return [] + ret_list = [] features: Dict[str, Any] = toggles_dict.get(schema.FEATURES_KEY, {}) for feature_name, feature_dict_def in features.items(): @@ -210,4 +212,5 @@ def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, A ): self._logger.debug(f"feature's calculated value is True, feature_name={feature_name}") ret_list.append(feature_name) + return ret_list diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 627e4648384..bb4b8f24dfc 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -30,6 +30,18 @@ def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> Confi return conf_store +def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigFetcher: + mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") + mocked_get_conf.side_effect = side_effect + return AppConfigFetcher( + environment="env", + service="service", + configuration_name="conf", + cache_seconds=1, + config=config, + ) + + # this test checks that we get correct value of feature that exists in the schema. # we also don't send an empty rules_context dict in this case def test_toggles_rule_does_not_match(mocker, config): @@ -424,17 +436,33 @@ def test_multiple_features_only_some_enabled(mocker, config): assert enabled_list == expected_value +def test_get_feature_toggle_handles_error(mocker, config): + # GIVEN a schema fetch that raises a ConfigurationError + schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) + conf_store = ConfigurationStore(schema_fetcher) + + # WHEN calling get_feature_toggle + toggle = conf_store.get_feature_toggle(feature_name="Foo", value_if_missing=False) + + # THEN handle the error and return the value_if_missing + assert toggle is False + + +def test_get_all_enabled_feature_toggles_handles_error(mocker, config): + # GIVEN a schema fetch that raises a ConfigurationError + schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) + conf_store = ConfigurationStore(schema_fetcher) + + # WHEN calling get_all_enabled_feature_toggles + toggles = conf_store.get_all_enabled_feature_toggles(rules_context=None) + + # THEN handle the error and return an empty list + assert toggles == [] + + def test_app_config_get_parameter_err(mocker, config): # GIVEN an appconfig with a missing config - mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") - mocked_get_conf.side_effect = GetParameterError() - app_conf_fetcher = AppConfigFetcher( - environment="env", - service="service", - configuration_name="conf", - cache_seconds=1, - config=config, - ) + app_conf_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) # WHEN calling get_json_configuration with pytest.raises(ConfigurationError) as err: From 0bbc9c6a8a7c2409abf608477595e4ad95d8edc2 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Fri, 16 Jul 2021 23:47:15 -0700 Subject: [PATCH 6/8] test(feature-toggles): Add schema tests --- .../utilities/feature_toggles/schema.py | 9 +-- .../feature_toggles/test_schema_validation.py | 66 +++++++++++++++++++ 2 files changed, 71 insertions(+), 4 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_toggles/schema.py index d57e247d7e0..9d995ab59e4 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema.py @@ -1,4 +1,5 @@ from enum import Enum +from logging import Logger from typing import Any, Dict from .exceptions import ConfigurationError @@ -22,7 +23,7 @@ class ACTION(str, Enum): class SchemaValidator: - def __init__(self, logger: object): + def __init__(self, logger: Logger): self._logger = logger def _raise_conf_exc(self, error_str: str) -> None: @@ -47,7 +48,7 @@ def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: self._raise_conf_exc(f"feature rule is not a dictionary, feature_name={feature_name}") rule_name = rule.get(RULE_NAME_KEY) if not rule_name or rule_name is None or not isinstance(rule_name, str): - self._raise_conf_exc(f"invalid rule_name, feature_name={feature_name}") + return self._raise_conf_exc(f"invalid rule_name, feature_name={feature_name}") rule_default_value = rule.get(RULE_DEFAULT_VALUE) if rule_default_value is None or not isinstance(rule_default_value, bool): self._raise_conf_exc(f"invalid rule_default_value, rule_name={rule_name}") @@ -76,8 +77,8 @@ def _validate_feature(self, feature_name: str, feature_dict_def: Dict[str, Any]) def validate_json_schema(self, schema: Dict[str, Any]) -> None: if not isinstance(schema, dict): self._raise_conf_exc("invalid AWS AppConfig JSON schema detected, root schema is not a dictionary") - features_dict: Dict = schema.get(FEATURES_KEY) + features_dict = schema.get(FEATURES_KEY) if not isinstance(features_dict, dict): - self._raise_conf_exc("invalid AWS AppConfig JSON schema detected, missing features dictionary") + return self._raise_conf_exc("invalid AWS AppConfig JSON schema detected, missing features dictionary") for feature_name, feature_dict_def in features_dict.items(): self._validate_feature(feature_name, feature_dict_def) diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 6abc9a9bef9..184f448322a 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -262,3 +262,69 @@ def test_valid_condition_all_actions(): }, } validator.validate_json_schema(schema) + + +def test_validate_condition_invalid_condition_type(): + # GIVEN an invalid condition type of empty dict + validator = SchemaValidator(logger) + condition = {} + + # WHEN calling _validate_condition + with pytest.raises(ConfigurationError) as err: + validator._validate_condition("foo", condition) + + # THEN raise ConfigurationError + assert "invalid condition type" in str(err) + + +def test_validate_condition_invalid_condition_action(): + # GIVEN an invalid condition action of foo + validator = SchemaValidator(logger) + condition = {"action": "foo"} + + # WHEN calling _validate_condition + with pytest.raises(ConfigurationError) as err: + validator._validate_condition("foo", condition) + + # THEN raise ConfigurationError + assert "invalid action value" in str(err) + + +def test_validate_condition_invalid_condition_key(): + # GIVEN a configuration with a missing "key" + validator = SchemaValidator(logger) + condition = {"action": ACTION.EQUALS.value} + + # WHEN calling _validate_condition + with pytest.raises(ConfigurationError) as err: + validator._validate_condition("foo", condition) + + # THEN raise ConfigurationError + assert "invalid key value" in str(err) + + +def test_validate_condition_missing_condition_value(): + # GIVEN a configuration with a missing condition value + validator = SchemaValidator(logger) + condition = {"action": ACTION.EQUALS.value, "key": "Foo"} + + # WHEN calling _validate_condition + with pytest.raises(ConfigurationError) as err: + validator._validate_condition("foo", condition) + + # THEN raise ConfigurationError + assert "missing condition value" in str(err) + + +def test_validate_rule_invalid_rule_name(): + # GIVEN a rule_name not in the rule dict + validator = SchemaValidator(logger) + rule_name = "invalid_rule_name" + rule = {"missing": ""} + + # WHEN calling _validate_rule + with pytest.raises(ConfigurationError) as err: + validator._validate_rule(rule_name, rule) + + # THEN raise ConfigurationError + assert "invalid rule_name" in str(err) From f00b9d83eb088069450695234b9110d260199a50 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sat, 17 Jul 2021 10:54:32 -0700 Subject: [PATCH 7/8] fix(feature-toggles): More MyPy fixes and Docstrings --- .../feature_toggles/configuration_store.py | 36 +++++++++---------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py index fc6d62619f8..72d00bb9c03 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py @@ -1,5 +1,5 @@ import logging -from typing import Any, Dict, List, Optional +from typing import Any, Dict, List, Optional, cast from . import schema from .exceptions import ConfigurationError @@ -12,8 +12,10 @@ class ConfigurationStore: def __init__(self, schema_fetcher: SchemaFetcher): """constructor - Args: - schema_fetcher (SchemaFetcher): A schema JSON fetcher, can be AWS AppConfig, Hashicorp Consul etc. + Parameters + ---------- + schema_fetcher: SchemaFetcher + A schema JSON fetcher, can be AWS AppConfig, Hashicorp Consul etc. """ self._logger = logger self._schema_fetcher = schema_fetcher @@ -39,12 +41,12 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], rules_context: Dict[str, Any]) -> bool: rule_name = rule.get(schema.RULE_NAME_KEY, "") rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) - conditions: Dict[str, str] = rule.get(schema.CONDITIONS_KEY) + conditions = cast(List[Dict], rule.get(schema.CONDITIONS_KEY)) for condition in conditions: - context_value = rules_context.get(condition.get(schema.CONDITION_KEY)) + context_value = rules_context.get(str(condition.get(schema.CONDITION_KEY))) if not self._match_by_action( - condition.get(schema.CONDITION_ACTION), + condition.get(schema.CONDITION_ACTION, ""), condition.get(schema.CONDITION_VALUE), context_value, ): @@ -73,7 +75,7 @@ def _handle_rules( for rule in rules: rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) if self._is_rule_matched(feature_name, rule, rules_context): - return rule_default_value + return bool(rule_default_value) # no rule matched, return default value of feature logger.debug( f"no rule matched, returning default value of feature, feature_default_value={feature_default_value}, " @@ -95,11 +97,8 @@ def get_configuration(self) -> Dict[str, Any]: Dict[str, Any] parsed JSON dictionary """ - config: Dict[ - str, Any - ] = ( - self._schema_fetcher.get_json_configuration() - ) # parse result conf as JSON, keep in cache for self.max_age seconds + # parse result conf as JSON, keep in cache for self.max_age seconds + config = self._schema_fetcher.get_json_configuration() # validate schema self._schema_validator.validate_json_schema(config) return config @@ -107,8 +106,9 @@ def get_configuration(self) -> Dict[str, Any]: def get_feature_toggle( self, *, feature_name: str, rules_context: Optional[Dict[str, Any]] = None, value_if_missing: bool ) -> bool: - """get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. - see below for explanation. + """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. + + See below for explanation. Parameters ---------- @@ -123,7 +123,7 @@ def get_feature_toggle( configuration from appconfig Returns - ---------- + ------ bool calculated feature toggle value. several possibilities: 1. if the feature doesn't appear in the schema or there has been an error fetching the @@ -157,7 +157,7 @@ def get_feature_toggle( f"no rules found, returning feature default value, feature_name={feature_name}, " f"default_value={feature_default_value}" ) - return feature_default_value + return bool(feature_default_value) # look for first rule match logger.debug( f"looking for rule match, feature_name={feature_name}, feature_default_value={feature_default_value}" @@ -165,8 +165,8 @@ def get_feature_toggle( return self._handle_rules( feature_name=feature_name, rules_context=rules_context, - feature_default_value=feature_default_value, - rules=rules_list, + feature_default_value=bool(feature_default_value), + rules=cast(List, rules_list), ) def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, Any]] = None) -> List[str]: From af64ca7301934f004ef55c5c696aaa536306b637 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Sat, 17 Jul 2021 11:04:58 -0700 Subject: [PATCH 8/8] feat(mypy): move pretty option to makefile --- Makefile | 2 +- .../utilities/idempotency/persistence/dynamodb.py | 2 +- mypy.ini | 1 - 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index e14ccf8dde7..e098615b86c 100644 --- a/Makefile +++ b/Makefile @@ -85,4 +85,4 @@ changelog: docker run -v "${PWD}":/workdir quay.io/git-chglog/git-chglog $$(git describe --abbrev=0 --tag).. > TMP_CHANGELOG.md mypy: - poetry run mypy aws_lambda_powertools + poetry run mypy --pretty aws_lambda_powertools diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py b/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py index ae3a1be490f..dc00334277e 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/dynamodb.py @@ -154,7 +154,7 @@ def _update_record(self, data_record: DataRecord): "ExpressionAttributeNames": expression_attr_names, } - self.table.update_item(**kwargs) # type: ignore + self.table.update_item(**kwargs) def _delete_record(self, data_record: DataRecord) -> None: logger.debug(f"Deleting record for idempotency key: {data_record.idempotency_key}") diff --git a/mypy.ini b/mypy.ini index 66bed405f59..2436d7074d2 100644 --- a/mypy.ini +++ b/mypy.ini @@ -4,7 +4,6 @@ warn_unused_configs=True no_implicit_optional=True warn_redundant_casts=True warn_unused_ignores=True -pretty = True show_column_numbers = True show_error_codes = True show_error_context = True