From 9890170026254ebfdbaee2f89ef6febfb0e6d2e1 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 26 Jul 2021 10:30:03 +0200 Subject: [PATCH 01/45] docs(feature_flags): initial skeleton --- docs/utilities/feature_flags.md | 24 ++++++++++++++++++++++++ mkdocs.yml | 1 + 2 files changed, 25 insertions(+) create mode 100644 docs/utilities/feature_flags.md diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md new file mode 100644 index 00000000000..b7b18b82797 --- /dev/null +++ b/docs/utilities/feature_flags.md @@ -0,0 +1,24 @@ +--- +title: Feature flags +description: Utility +--- + +The feature flags utility provides a simple rule engine to define when one or multiple features should be enabled depending on the input. + +!!! tip "For simpler use cases where a feature is simply on or off for all users, use [Parameters](parameters.md) utility instead." + +## Key features + +* Define simple feature flags to dynamically decide when to enable a feature + + +## TODO + +* Review param names and UX + - Deep dive on naming - AppConfigFetcher vs ConfigurationStore vs SchemaFetcher + - How can we make it easier to get started +* Getting started + - Sample Infrastructure + - Quickest way to start --- defaults, built-ins +* Advanced section + - Bring your own diff --git a/mkdocs.yml b/mkdocs.yml index 7ee0fd56236..94dc9980cf1 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -25,6 +25,7 @@ nav: - utilities/data_classes.md - utilities/parser.md - utilities/idempotency.md + - utilities/feature_flags.md theme: name: material From 0b2abc74fd1a851e431859ae99584039e3480e82 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 13:58:18 +0200 Subject: [PATCH 02/45] docs(feature_flags): initial sections --- docs/utilities/feature_flags.md | 53 ++++++++++++++++++++++++++++++++- 1 file changed, 52 insertions(+), 1 deletion(-) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index b7b18b82797..39e1aaf4245 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -7,18 +7,69 @@ The feature flags utility provides a simple rule engine to define when one or mu !!! tip "For simpler use cases where a feature is simply on or off for all users, use [Parameters](parameters.md) utility instead." +## Terminology + +> Explain the difference between static vs dynamic feature flags + ## Key features +> TODO: Revisit once getting started and advanced sections are complete + * Define simple feature flags to dynamically decide when to enable a feature +* Fetch one or all feature flags enabled for a given application context +* Bring your own configuration store + +## Getting started +### IAM Permissions + +This utility requires additional permissions to work as expected. + +Provider | Function/Method | IAM Permission +------------------------------------------------- | ------------------------------------------------- | --------------------------------------------------------------------------------- +App Config | `AppConfigProvider.get_app_config`, `get_app_config` | `appconfig:GetConfiguration` + +### Creating feature flags + +> NOTE: Explain schema, provide sample boto3 script and CFN to create one + +#### Rules + + +### Fetching a single feature flag + +### Fetching all feature flags + +### Advanced + +#### Adjusting cache TTL + +### Partially enabling features + +### Bring your own store provider + +## Testing your code + +> NOTE: Share example on how customers can unit test their feature flags ## TODO * Review param names and UX - - Deep dive on naming - AppConfigFetcher vs ConfigurationStore vs SchemaFetcher + - Deep dive on naming - ConfigurationStore vs SchemaFetcher vs Schema - How can we make it easier to get started * Getting started - Sample Infrastructure - Quickest way to start --- defaults, built-ins * Advanced section - Bring your own + +## Changes + +Potential changes to be validated when docs are in a better shape + +- [ ] `rules_context` to `context` +- [ ] `ConfigurationStore` to `FeatureFlags` +- [ ] `SchemaFetcher` to `StoreProvider` +- [ ] Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.) +- [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given SchemaFetcher) +- [ ] From 566a2f97a2188a5eff8d66d6553a3d5704f43656 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 14:49:16 +0200 Subject: [PATCH 03/45] docs: add terminology section --- docs/utilities/feature_flags.md | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 39e1aaf4245..a962506c66e 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -9,7 +9,15 @@ The feature flags utility provides a simple rule engine to define when one or mu ## Terminology -> Explain the difference between static vs dynamic feature flags +Feature flags are used to modify a system behaviour without having to change their code. These flags can be static or dynamic. + +**Static feature flags** are commonly used for long-lived behaviours that will rarely change, for example `TRACER_ENABLED=True`. These are better suited for [Parameters utility](parameters.md). + +**Dynamic feature flags** are typically used for experiments where you'd want to enable a feature for a limited set of customers, for example A/B testing and Canary releases. These are better suited for this utility, as you can create multiple conditions on whether a feature flag should be `True` or `False`. + +That being said, be mindful that feature flags can increase your application complexity over time if you're not careful; use them sparingly. + +!!! tip "Read [this article](https://martinfowler.com/articles/feature-toggles.html){target="_blank"} for more details on different types of feature flags and trade-offs" ## Key features From e9f87dc1bee6420495db411ef4ce3228400617e7 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 14:53:40 +0200 Subject: [PATCH 04/45] docs: add IAM permission --- docs/utilities/feature_flags.md | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index a962506c66e..8e15f4310b2 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -30,11 +30,7 @@ That being said, be mindful that feature flags can increase your application com ## Getting started ### IAM Permissions -This utility requires additional permissions to work as expected. - -Provider | Function/Method | IAM Permission -------------------------------------------------- | ------------------------------------------------- | --------------------------------------------------------------------------------- -App Config | `AppConfigProvider.get_app_config`, `get_app_config` | `appconfig:GetConfiguration` +By default, this utility provides AWS AppConfig as a configuration store. As such, you IAM Role needs permission - `appconfig:GetConfiguration` - to fetch feature flags from AppConfig. ### Creating feature flags From 619c547a36f74365afd37b2d65c5ac153e6d2b06 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 15:24:08 +0200 Subject: [PATCH 05/45] refactor: rules_context to context --- .../feature_toggles/configuration_store.py | 28 +++++------ docs/utilities/feature_flags.md | 17 ++----- .../feature_toggles/test_feature_toggles.py | 50 +++++++------------ 3 files changed, 35 insertions(+), 60 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py index 72d00bb9c03..c2a30f5a9b5 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py @@ -38,13 +38,13 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any self._logger.error(f"caught exception while matching action, action={action}, exception={str(exc)}") return False - def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], rules_context: Dict[str, Any]) -> bool: + def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], context: Dict[str, Any]) -> bool: rule_name = rule.get(schema.RULE_NAME_KEY, "") rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) conditions = cast(List[Dict], rule.get(schema.CONDITIONS_KEY)) for condition in conditions: - context_value = rules_context.get(str(condition.get(schema.CONDITION_KEY))) + context_value = context.get(str(condition.get(schema.CONDITION_KEY))) if not self._match_by_action( condition.get(schema.CONDITION_ACTION, ""), condition.get(schema.CONDITION_VALUE), @@ -68,13 +68,13 @@ def _handle_rules( self, *, feature_name: str, - rules_context: Dict[str, Any], + context: Dict[str, Any], feature_default_value: bool, rules: List[Dict[str, Any]], ) -> bool: for rule in rules: rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) - if self._is_rule_matched(feature_name, rule, rules_context): + if self._is_rule_matched(feature_name, rule, context): return bool(rule_default_value) # no rule matched, return default value of feature logger.debug( @@ -104,7 +104,7 @@ def get_configuration(self) -> Dict[str, Any]: return config def get_feature_toggle( - self, *, feature_name: str, rules_context: Optional[Dict[str, Any]] = None, value_if_missing: bool + self, *, feature_name: str, 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. @@ -114,7 +114,7 @@ def get_feature_toggle( ---------- feature_name: str feature name that you wish to fetch - rules_context: Optional[Dict[str, Any]] + 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 @@ -132,8 +132,8 @@ def get_feature_toggle( the defined feature 3. feature exists and a rule matches -> rule_default_value of rule is returned """ - if rules_context is None: - rules_context = {} + if context is None: + context = {} try: toggles_dict: Dict[str, Any] = self.get_configuration() @@ -164,18 +164,18 @@ def get_feature_toggle( ) return self._handle_rules( feature_name=feature_name, - rules_context=rules_context, + context=context, 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]: + def get_all_enabled_feature_toggles(self, *, 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) Parameters ---------- - rules_context: Optional[Dict[str, Any]] + 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. @@ -185,8 +185,8 @@ def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, A 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 = {} + if context is None: + context = {} try: toggles_dict: Dict[str, Any] = self.get_configuration() @@ -206,7 +206,7 @@ def get_all_enabled_feature_toggles(self, *, rules_context: Optional[Dict[str, A ret_list.append(feature_name) elif self._handle_rules( feature_name=feature_name, - rules_context=rules_context, + context=context, feature_default_value=feature_default_value, rules=rules_list, ): diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 8e15f4310b2..8e253e14e87 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -56,24 +56,15 @@ By default, this utility provides AWS AppConfig as a configuration store. As suc > NOTE: Share example on how customers can unit test their feature flags -## TODO - -* Review param names and UX - - Deep dive on naming - ConfigurationStore vs SchemaFetcher vs Schema - - How can we make it easier to get started -* Getting started - - Sample Infrastructure - - Quickest way to start --- defaults, built-ins -* Advanced section - - Bring your own - ## Changes Potential changes to be validated when docs are in a better shape -- [ ] `rules_context` to `context` +- [x] ~~`rules_context` to `context`~~ - [ ] `ConfigurationStore` to `FeatureFlags` - [ ] `SchemaFetcher` to `StoreProvider` - [ ] Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.) - [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given SchemaFetcher) -- [ ] +- [ ] Review why we're testing a private method(`is_rule_matched`) + +**Q: Why is `get_configuration()` public?** diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index bb4b8f24dfc..13e6f5f7207 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -43,7 +43,7 @@ def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigFe # 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 +# we also don't send an empty context dict in this case def test_toggles_rule_does_not_match(mocker, config): expected_value = True mocked_app_config_schema = { @@ -68,7 +68,7 @@ def test_toggles_rule_does_not_match(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle(feature_name="my_feature", rules_context={}, value_if_missing=False) + toggle = conf_store.get_feature_toggle(feature_name="my_feature", context={}, value_if_missing=False) assert toggle == expected_value @@ -79,18 +79,18 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): mocked_app_config_schema = {"features": {"my_fake_feature": {"feature_default_value": True}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle(feature_name="my_feature", rules_context={}, value_if_missing=expected_value) + toggle = conf_store.get_feature_toggle(feature_name="my_feature", context={}, value_if_missing=expected_value) assert toggle == expected_value -# check that feature match works when they are no rules and we send rules_context. +# check that feature match works when they are no rules and we send context. # default value is False but the feature has a True default_value. def test_toggles_no_rules(mocker, config): expected_value = True mocked_app_config_schema = {"features": {"my_feature": {"feature_default_value": expected_value}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( - feature_name="my_feature", rules_context={"tenant_id": "6", "username": "a"}, value_if_missing=False + feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False ) assert toggle == expected_value @@ -120,9 +120,7 @@ def test_toggles_conditions_no_match(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "6", "username": "a"}, # rule will not match - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False ) assert toggle == expected_value @@ -160,7 +158,7 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( feature_name="my_feature", - rules_context={ + context={ "tenant_id": tenant_id_val, "username": username_val, }, @@ -201,7 +199,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( - feature_name="my_feature", rules_context={"tenant_id": "6", "username": "a"}, value_if_missing=False + feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False ) assert toggle == expected_val @@ -262,29 +260,23 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) # match first rule toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "6", "username": "abcd"}, - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "6", "username": "abcd"}, value_if_missing=False ) assert toggle == expected_value_first_check # match second rule toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "4446", "username": "az"}, - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "4446", "username": "az"}, value_if_missing=False ) assert toggle == expected_value_second_check # match no rule toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "11114446", "username": "ab"}, - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, value_if_missing=False ) assert toggle == expected_value_third_check # feature doesn't exist toggle = conf_store.get_feature_toggle( feature_name="my_fake_feature", - rules_context={"tenant_id": "11114446", "username": "ab"}, + context={"tenant_id": "11114446", "username": "ab"}, value_if_missing=expected_value_fourth_case, ) assert toggle == expected_value_fourth_case @@ -315,9 +307,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "6", "username": "a"}, # rule will match - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False ) assert toggle == expected_value @@ -346,9 +336,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.get_feature_toggle( - feature_name="my_feature", - rules_context={"tenant_id": "6", "username": "a"}, # rule will not match - value_if_missing=False, + feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False ) assert toggle == expected_value @@ -379,9 +367,7 @@ def test_multiple_features_enabled(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles( - rules_context={"tenant_id": "6", "username": "a"} - ) + enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles(context={"tenant_id": "6", "username": "a"}) assert enabled_list == expected_value @@ -430,9 +416,7 @@ def test_multiple_features_only_some_enabled(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles( - rules_context={"tenant_id": "6", "username": "a"} - ) + enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles(context={"tenant_id": "6", "username": "a"}) assert enabled_list == expected_value @@ -454,7 +438,7 @@ def test_get_all_enabled_feature_toggles_handles_error(mocker, config): conf_store = ConfigurationStore(schema_fetcher) # WHEN calling get_all_enabled_feature_toggles - toggles = conf_store.get_all_enabled_feature_toggles(rules_context=None) + toggles = conf_store.get_all_enabled_feature_toggles(context=None) # THEN handle the error and return an empty list assert toggles == [] From b274eed21b41fa96ded856550c5697fbe462bbc4 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 15:55:49 +0200 Subject: [PATCH 06/45] refactor: AppConfig params to match Parameters --- .../feature_toggles/appconfig_fetcher.py | 20 +++++++++---------- .../feature_toggles/schema_fetcher.py | 4 ++-- docs/utilities/feature_flags.md | 5 +++-- .../feature_toggles/test_feature_toggles.py | 8 ++++---- 4 files changed, 19 insertions(+), 18 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py index 3501edfd0d3..4963eda3052 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py @@ -18,8 +18,8 @@ class AppConfigFetcher(SchemaFetcher): def __init__( self, environment: str, - service: str, - configuration_name: str, + application: str, + name: str, cache_seconds: int, config: Optional[Config] = None, ): @@ -28,19 +28,19 @@ def __init__( 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 + Appconfig environment, e.g. 'dev/test' etc. + application: str + AppConfig application name, e.g. 'powertools' + name: str + AppConfig configuration name e.g. `my_conf` 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) + super().__init__(name, cache_seconds) self._logger = logger - self._conf_store = AppConfigProvider(environment=environment, application=service, config=config) + self._conf_store = AppConfigProvider(environment=environment, application=application, config=config) def get_json_configuration(self) -> Dict[str, Any]: """Get configuration string from AWs AppConfig and return the parsed JSON dictionary @@ -60,7 +60,7 @@ def get_json_configuration(self) -> Dict[str, Any]: return cast( dict, self._conf_store.get( - name=self.configuration_name, + name=self.name, transform=TRANSFORM_TYPE, max_age=self._cache_seconds, ), diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py index 89fffe1221d..4b566c6398d 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py @@ -4,12 +4,12 @@ class SchemaFetcher(ABC): def __init__(self, configuration_name: str, cache_seconds: int): - self.configuration_name = configuration_name + self.name = configuration_name self._cache_seconds = cache_seconds @abstractmethod def get_json_configuration(self) -> Dict[str, Any]: - """Get configuration string from any configuration storing service and return the parsed JSON dictionary + """Get configuration string from any configuration storing application and return the parsed JSON dictionary Raises ------ diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 8e253e14e87..410daf5e143 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -66,5 +66,6 @@ Potential changes to be validated when docs are in a better shape - [ ] Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.) - [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given SchemaFetcher) - [ ] Review why we're testing a private method(`is_rule_matched`) - -**Q: Why is `get_configuration()` public?** +- [x] AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`) +- [ ] Review `get_configuration`, `get_json_configuration` +- [ ] Review `get_feature` in favour of `get_feature_toggle` diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 13e6f5f7207..b0b65a44278 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -21,8 +21,8 @@ def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> Confi app_conf_fetcher = AppConfigFetcher( environment="test_env", - service="test_app", - configuration_name="test_conf_name", + application="test_app", + name="test_conf_name", cache_seconds=600, config=config, ) @@ -35,8 +35,8 @@ def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigFe mocked_get_conf.side_effect = side_effect return AppConfigFetcher( environment="env", - service="service", - configuration_name="conf", + application="application", + name="conf", cache_seconds=1, config=config, ) From 4d397a76bf3adc664b0aa6b7ed13b3173697d4fb Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 16:36:48 +0200 Subject: [PATCH 07/45] refactor: rename ConfigurationStore, SchemaFetcher --- .../utilities/feature_toggles/__init__.py | 12 ++++++------ .../{appconfig_fetcher.py => appconfig.py} | 4 ++-- .../{schema_fetcher.py => base.py} | 2 +- ...configuration_store.py => feature_flags.py} | 8 ++++---- docs/utilities/feature_flags.md | 6 +++--- .../feature_toggles/test_feature_toggles.py | 18 +++++++++--------- 6 files changed, 25 insertions(+), 25 deletions(-) rename aws_lambda_powertools/utilities/feature_toggles/{appconfig_fetcher.py => appconfig.py} (96%) rename aws_lambda_powertools/utilities/feature_toggles/{schema_fetcher.py => base.py} (96%) rename aws_lambda_powertools/utilities/feature_toggles/{configuration_store.py => feature_flags.py} (98%) diff --git a/aws_lambda_powertools/utilities/feature_toggles/__init__.py b/aws_lambda_powertools/utilities/feature_toggles/__init__.py index 04237d63812..2d048a6600b 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/__init__.py +++ b/aws_lambda_powertools/utilities/feature_toggles/__init__.py @@ -1,16 +1,16 @@ """Advanced feature toggles utility """ -from .appconfig_fetcher import AppConfigFetcher -from .configuration_store import ConfigurationStore +from .appconfig import AppConfigStore +from .base import StoreProvider from .exceptions import ConfigurationError +from .feature_flags import FeatureFlags from .schema import ACTION, SchemaValidator -from .schema_fetcher import SchemaFetcher __all__ = [ "ConfigurationError", - "ConfigurationStore", + "FeatureFlags", "ACTION", "SchemaValidator", - "AppConfigFetcher", - "SchemaFetcher", + "AppConfigStore", + "StoreProvider", ] diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig.py similarity index 96% rename from aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py rename to aws_lambda_powertools/utilities/feature_toggles/appconfig.py index 4963eda3052..2f1fee6c49c 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig.py @@ -5,8 +5,8 @@ from aws_lambda_powertools.utilities.parameters import AppConfigProvider, GetParameterError, TransformParameterError +from .base import StoreProvider from .exceptions import ConfigurationError -from .schema_fetcher import SchemaFetcher logger = logging.getLogger(__name__) @@ -14,7 +14,7 @@ TRANSFORM_TYPE = "json" -class AppConfigFetcher(SchemaFetcher): +class AppConfigStore(StoreProvider): def __init__( self, environment: str, diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py b/aws_lambda_powertools/utilities/feature_toggles/base.py similarity index 96% rename from aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py rename to aws_lambda_powertools/utilities/feature_toggles/base.py index 4b566c6398d..5e92fd7a898 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema_fetcher.py +++ b/aws_lambda_powertools/utilities/feature_toggles/base.py @@ -2,7 +2,7 @@ from typing import Any, Dict -class SchemaFetcher(ABC): +class StoreProvider(ABC): def __init__(self, configuration_name: str, cache_seconds: int): self.name = configuration_name self._cache_seconds = cache_seconds diff --git a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py similarity index 98% rename from aws_lambda_powertools/utilities/feature_toggles/configuration_store.py rename to aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index c2a30f5a9b5..36f5d852ce6 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/configuration_store.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -2,19 +2,19 @@ from typing import Any, Dict, List, Optional, cast from . import schema +from .base import StoreProvider from .exceptions import ConfigurationError -from .schema_fetcher import SchemaFetcher logger = logging.getLogger(__name__) -class ConfigurationStore: - def __init__(self, schema_fetcher: SchemaFetcher): +class FeatureFlags: + def __init__(self, schema_fetcher: StoreProvider): """constructor Parameters ---------- - schema_fetcher: SchemaFetcher + schema_fetcher: StoreProvider A schema JSON fetcher, can be AWS AppConfig, Hashicorp Consul etc. """ self._logger = logger diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 410daf5e143..90d7ad429c6 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -61,10 +61,10 @@ By default, this utility provides AWS AppConfig as a configuration store. As suc Potential changes to be validated when docs are in a better shape - [x] ~~`rules_context` to `context`~~ -- [ ] `ConfigurationStore` to `FeatureFlags` -- [ ] `SchemaFetcher` to `StoreProvider` +- [x] `ConfigurationStore` to `FeatureFlags` +- [x] `StoreProvider` to `StoreProvider` - [ ] Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.) -- [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given SchemaFetcher) +- [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given StoreProvider) - [ ] Review why we're testing a private method(`is_rule_matched`) - [x] AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`) - [ ] Review `get_configuration`, `get_json_configuration` diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index b0b65a44278..45b2485190e 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -4,8 +4,8 @@ from botocore.config import Config 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.appconfig import AppConfigStore +from aws_lambda_powertools.utilities.feature_toggles.feature_flags import FeatureFlags from aws_lambda_powertools.utilities.feature_toggles.schema import ACTION from aws_lambda_powertools.utilities.parameters import GetParameterError @@ -15,25 +15,25 @@ def config(): return Config(region_name="us-east-1") -def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> ConfigurationStore: +def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> FeatureFlags: mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") mocked_get_conf.return_value = mock_schema - app_conf_fetcher = AppConfigFetcher( + app_conf_fetcher = AppConfigStore( environment="test_env", application="test_app", name="test_conf_name", cache_seconds=600, config=config, ) - conf_store: ConfigurationStore = ConfigurationStore(schema_fetcher=app_conf_fetcher) + conf_store: FeatureFlags = FeatureFlags(schema_fetcher=app_conf_fetcher) return conf_store -def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigFetcher: +def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigStore: mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") mocked_get_conf.side_effect = side_effect - return AppConfigFetcher( + return AppConfigStore( environment="env", application="application", name="conf", @@ -423,7 +423,7 @@ def test_multiple_features_only_some_enabled(mocker, config): 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) + conf_store = FeatureFlags(schema_fetcher) # WHEN calling get_feature_toggle toggle = conf_store.get_feature_toggle(feature_name="Foo", value_if_missing=False) @@ -435,7 +435,7 @@ def test_get_feature_toggle_handles_error(mocker, config): 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) + conf_store = FeatureFlags(schema_fetcher) # WHEN calling get_all_enabled_feature_toggles toggles = conf_store.get_all_enabled_feature_toggles(context=None) From a70bcd8e9a7c1b50dafe25462ef263edae83c383 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 16:39:19 +0200 Subject: [PATCH 08/45] refactor: rename schema_fetcher param --- .../utilities/feature_toggles/feature_flags.py | 8 ++++---- tests/functional/feature_toggles/test_feature_toggles.py | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index 36f5d852ce6..e1b9f0b4ef9 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -9,16 +9,16 @@ class FeatureFlags: - def __init__(self, schema_fetcher: StoreProvider): + def __init__(self, store: StoreProvider): """constructor Parameters ---------- - schema_fetcher: StoreProvider + store: StoreProvider A schema JSON fetcher, can be AWS AppConfig, Hashicorp Consul etc. """ self._logger = logger - self._schema_fetcher = schema_fetcher + self._store = store self._schema_validator = schema.SchemaValidator(self._logger) def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: @@ -98,7 +98,7 @@ def get_configuration(self) -> Dict[str, Any]: parsed JSON dictionary """ # parse result conf as JSON, keep in cache for self.max_age seconds - config = self._schema_fetcher.get_json_configuration() + config = self._store.get_json_configuration() # validate schema self._schema_validator.validate_json_schema(config) return config diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 45b2485190e..4eb4005b9ef 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -26,7 +26,7 @@ def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> Featu cache_seconds=600, config=config, ) - conf_store: FeatureFlags = FeatureFlags(schema_fetcher=app_conf_fetcher) + conf_store: FeatureFlags = FeatureFlags(store=app_conf_fetcher) return conf_store From 649d98705d3367a3fe4257b3f0990584b83dce26 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 16:49:49 +0200 Subject: [PATCH 09/45] refactor: rename get_feature_toggle to evaluate --- .../feature_toggles/feature_flags.py | 21 ++++---- docs/utilities/feature_flags.md | 8 +-- .../feature_toggles/test_feature_toggles.py | 52 ++++++++----------- 3 files changed, 35 insertions(+), 46 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index e1b9f0b4ef9..0fd96756855 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -103,9 +103,7 @@ def get_configuration(self) -> Dict[str, Any]: self._schema_validator.validate_json_schema(config) return config - def get_feature_toggle( - self, *, feature_name: str, context: Optional[Dict[str, Any]] = None, value_if_missing: bool - ) -> bool: + def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. See below for explanation. @@ -117,10 +115,9 @@ def get_feature_toggle( 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 + default: bool + default value if feature flag doesn't exist in the schema, + or there has been an error while fetching the configuration from appconfig Returns ------ @@ -138,16 +135,16 @@ 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") - return value_if_missing + logger.error("unable to get feature toggles JSON, returning provided default value") + return default 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, " - f"feature_name={feature_name}, value_if_missing={value_if_missing}" + f"feature does not appear in configuration, using provided default, " + f"feature_name={feature_name}, default={default}" ) - return value_if_missing + return default rules_list = feature.get(schema.RULES_KEY) feature_default_value = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 90d7ad429c6..2a5c58f2751 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -63,9 +63,11 @@ Potential changes to be validated when docs are in a better shape - [x] ~~`rules_context` to `context`~~ - [x] `ConfigurationStore` to `FeatureFlags` - [x] `StoreProvider` to `StoreProvider` -- [ ] Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.) +- [x] ~~Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.)~~ +- [x] ~~AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`)~~ +- [x] ~~Rename `value_if_missing` param to `default` to match Python consistency (e.g. `os.getenv("VAR", default=False)`)~~ +- [x] ~~Review `get_feature` in favour of `evaluate`~~ - [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given StoreProvider) - [ ] Review why we're testing a private method(`is_rule_matched`) -- [x] AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`) - [ ] Review `get_configuration`, `get_json_configuration` -- [ ] Review `get_feature` in favour of `get_feature_toggle` +- [ ] Review potentially redundant logging diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 4eb4005b9ef..e4564e6a042 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -68,18 +68,18 @@ def test_toggles_rule_does_not_match(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle(feature_name="my_feature", context={}, value_if_missing=False) + toggle = conf_store.evaluate(feature_name="my_feature", context={}, default=False) assert toggle == expected_value # this test checks that if you try to get a feature that doesn't exist in the schema, -# you get the default value of False that was sent to the get_feature_toggle API +# you get the default value of False that was sent to the evaluate API def test_toggles_no_conditions_feature_does_not_exist(mocker, config): expected_value = False mocked_app_config_schema = {"features": {"my_fake_feature": {"feature_default_value": True}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle(feature_name="my_feature", context={}, value_if_missing=expected_value) + toggle = conf_store.evaluate(feature_name="my_feature", context={}, default=expected_value) assert toggle == expected_value @@ -89,9 +89,7 @@ def test_toggles_no_rules(mocker, config): expected_value = True mocked_app_config_schema = {"features": {"my_feature": {"feature_default_value": expected_value}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False - ) + toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -119,9 +117,7 @@ def test_toggles_conditions_no_match(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False - ) + toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -156,13 +152,13 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( + toggle = conf_store.evaluate( feature_name="my_feature", context={ "tenant_id": tenant_id_val, "username": username_val, }, - value_if_missing=True, + default=True, ) assert toggle == expected_value @@ -198,9 +194,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False - ) + toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_val @@ -259,25 +253,25 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) # match first rule - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "abcd"}, value_if_missing=False + toggle = conf_store.evaluate( + feature_name="my_feature", context={"tenant_id": "6", "username": "abcd"}, default=False ) assert toggle == expected_value_first_check # match second rule - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "4446", "username": "az"}, value_if_missing=False + toggle = conf_store.evaluate( + feature_name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False ) assert toggle == expected_value_second_check # match no rule - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, value_if_missing=False + toggle = conf_store.evaluate( + feature_name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, default=False ) assert toggle == expected_value_third_check # feature doesn't exist - toggle = conf_store.get_feature_toggle( + toggle = conf_store.evaluate( feature_name="my_fake_feature", context={"tenant_id": "11114446", "username": "ab"}, - value_if_missing=expected_value_fourth_case, + default=expected_value_fourth_case, ) assert toggle == expected_value_fourth_case @@ -306,9 +300,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False - ) + toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -335,9 +327,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.get_feature_toggle( - feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, value_if_missing=False - ) + toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -425,10 +415,10 @@ def test_get_feature_toggle_handles_error(mocker, config): schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) conf_store = FeatureFlags(schema_fetcher) - # WHEN calling get_feature_toggle - toggle = conf_store.get_feature_toggle(feature_name="Foo", value_if_missing=False) + # WHEN calling evaluate + toggle = conf_store.evaluate(feature_name="Foo", default=False) - # THEN handle the error and return the value_if_missing + # THEN handle the error and return the default assert toggle is False From 38c7bd1f6ad723b91bb3183d20f113131d787b29 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 17:13:32 +0200 Subject: [PATCH 10/45] chore: improve all_enabled feature tests --- tests/functional/feature_toggles/test_feature_toggles.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index e4564e6a042..80152f8f9d7 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -354,6 +354,9 @@ def test_multiple_features_enabled(mocker, config): "my_feature2": { "feature_default_value": True, }, + "my_feature3": { + "feature_default_value": False, + }, }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) From d9020188d16b3846a7048f22a156762480a7dc23 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 17:16:12 +0200 Subject: [PATCH 11/45] refactor: get_all_enabled_feature_toggles to get_enabled_features --- .../utilities/feature_toggles/feature_flags.py | 2 +- docs/utilities/feature_flags.md | 3 ++- tests/functional/feature_toggles/test_feature_toggles.py | 8 ++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index 0fd96756855..ca03222d75a 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -166,7 +166,7 @@ def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = Non rules=cast(List, rules_list), ) - def get_all_enabled_feature_toggles(self, *, context: Optional[Dict[str, Any]] = None) -> List[str]: + def get_enabled_features(self, *, 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) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 2a5c58f2751..1f97104ceed 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -66,7 +66,8 @@ Potential changes to be validated when docs are in a better shape - [x] ~~Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.)~~ - [x] ~~AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`)~~ - [x] ~~Rename `value_if_missing` param to `default` to match Python consistency (e.g. `os.getenv("VAR", default=False)`)~~ -- [x] ~~Review `get_feature` in favour of `evaluate`~~ +- [x] ~~Rename `get_feature` to `evaluate`~~ +- [x] ~~Rename `get_all_enabled_feature_toggles` to `get_enabled_features`~~ - [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given StoreProvider) - [ ] Review why we're testing a private method(`is_rule_matched`) - [ ] Review `get_configuration`, `get_json_configuration` diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 80152f8f9d7..5ff96b93ecb 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -360,7 +360,7 @@ def test_multiple_features_enabled(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles(context={"tenant_id": "6", "username": "a"}) + enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) assert enabled_list == expected_value @@ -409,7 +409,7 @@ def test_multiple_features_only_some_enabled(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_all_enabled_feature_toggles(context={"tenant_id": "6", "username": "a"}) + enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) assert enabled_list == expected_value @@ -430,8 +430,8 @@ def test_get_all_enabled_feature_toggles_handles_error(mocker, config): schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) conf_store = FeatureFlags(schema_fetcher) - # WHEN calling get_all_enabled_feature_toggles - toggles = conf_store.get_all_enabled_feature_toggles(context=None) + # WHEN calling get_enabled_features + toggles = conf_store.get_enabled_features(context=None) # THEN handle the error and return an empty list assert toggles == [] From ce16548406ebaf7e5f37032bc2570946c93a3bf9 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 17:39:26 +0200 Subject: [PATCH 12/45] refactor: remove redundant logger, add newlines --- .../utilities/feature_toggles/appconfig.py | 5 +-- .../feature_toggles/feature_flags.py | 21 ++++----- .../utilities/feature_toggles/schema.py | 43 +++++++++++-------- docs/utilities/feature_flags.md | 3 +- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig.py b/aws_lambda_powertools/utilities/feature_toggles/appconfig.py index 2f1fee6c49c..343405e1675 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_toggles/appconfig.py @@ -10,7 +10,6 @@ logger = logging.getLogger(__name__) - TRANSFORM_TYPE = "json" @@ -66,6 +65,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 ConfigurationError(error_str) + raise ConfigurationError("Unable to get AWS AppConfig configuration file") from exc diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index ca03222d75a..8f59f75f516 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -35,7 +35,7 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any func = mapping_by_action.get(action, lambda a, b: False) return func(context_value, condition_value) except Exception as exc: - self._logger.error(f"caught exception while matching action, action={action}, exception={str(exc)}") + self._logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") return False def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], context: Dict[str, Any]) -> bool: @@ -135,12 +135,12 @@ def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = Non try: toggles_dict: Dict[str, Any] = self.get_configuration() except ConfigurationError: - logger.error("unable to get feature toggles JSON, returning provided default value") + logger.debug("Unable to get feature toggles JSON, returning provided default value") return default feature: Dict[str, Dict] = toggles_dict.get(schema.FEATURES_KEY, {}).get(feature_name, None) if feature is None: - logger.warning( + logger.debug( f"feature does not appear in configuration, using provided default, " f"feature_name={feature_name}, default={default}" ) @@ -149,7 +149,7 @@ def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = Non rules_list = feature.get(schema.RULES_KEY) feature_default_value = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) if not rules_list: - # not rules but has a value + # no rules but value logger.debug( f"no rules found, returning feature default value, feature_name={feature_name}, " f"default_value={feature_default_value}" @@ -185,13 +185,14 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L if context is None: context = {} + features_enabled: List[str] = [] + try: toggles_dict: Dict[str, Any] = self.get_configuration() except ConfigurationError: - logger.error("unable to get feature toggles JSON") - return [] + logger.debug("unable to get feature toggles JSON") + return features_enabled - ret_list = [] features: Dict[str, Any] = toggles_dict.get(schema.FEATURES_KEY, {}) for feature_name, feature_dict_def in features.items(): rules_list = feature_dict_def.get(schema.RULES_KEY, []) @@ -200,7 +201,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L self._logger.debug( f"feature is enabled by default and has no defined rules, feature_name={feature_name}" ) - ret_list.append(feature_name) + features_enabled.append(feature_name) elif self._handle_rules( feature_name=feature_name, context=context, @@ -208,6 +209,6 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L rules=rules_list, ): self._logger.debug(f"feature's calculated value is True, feature_name={feature_name}") - ret_list.append(feature_name) + features_enabled.append(feature_name) - return ret_list + return features_enabled diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_toggles/schema.py index 9d995ab59e4..9453467702f 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema.py @@ -26,59 +26,68 @@ class SchemaValidator: def __init__(self, logger: Logger): self._logger = logger - def _raise_conf_exc(self, error_str: str) -> None: - self._logger.error(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): - self._raise_conf_exc(f"invalid condition type, not a dictionary, rule_name={rule_name}") + raise ConfigurationError(f"invalid condition type, not a dictionary, rule_name={rule_name}") + action = condition.get(CONDITION_ACTION, "") if action not in [ACTION.EQUALS.value, ACTION.STARTSWITH.value, ACTION.ENDSWITH.value, ACTION.CONTAINS.value]: - self._raise_conf_exc(f"invalid action value, rule_name={rule_name}, action={action}") + raise ConfigurationError(f"invalid action value, rule_name={rule_name}, action={action}") + key = condition.get(CONDITION_KEY, "") if not key or not isinstance(key, str): - self._raise_conf_exc(f"invalid key value, key has to be a non empty string, rule_name={rule_name}") + raise ConfigurationError(f"Invalid key value, key has to be a non empty string, rule_name={rule_name}") + value = condition.get(CONDITION_VALUE, "") if not value: - self._raise_conf_exc(f"missing condition value, rule_name={rule_name}") + raise ConfigurationError(f"Missing condition value, rule_name={rule_name}") def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: if not rule or not isinstance(rule, dict): - self._raise_conf_exc(f"feature rule is not a dictionary, feature_name={feature_name}") + raise ConfigurationError(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): - return self._raise_conf_exc(f"invalid rule_name, feature_name={feature_name}") + raise ConfigurationError(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}") + raise ConfigurationError(f"Invalid rule_default_value, rule_name={rule_name}") + conditions = rule.get(CONDITIONS_KEY, {}) if not conditions or not isinstance(conditions, list): - self._raise_conf_exc(f"invalid condition, rule_name={rule_name}") + raise ConfigurationError(f"Invalid condition, rule_name={rule_name}") + # validate conditions for condition in conditions: self._validate_condition(rule_name, condition) def _validate_feature(self, feature_name: str, feature_dict_def: Dict[str, Any]) -> None: if not feature_dict_def or not isinstance(feature_dict_def, dict): - self._raise_conf_exc(f"invalid AWS AppConfig JSON schema detected, feature {feature_name} is invalid") + raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {feature_name} is invalid") + feature_default_value = feature_dict_def.get(FEATURE_DEFAULT_VAL_KEY) if feature_default_value is None or not isinstance(feature_default_value, bool): - self._raise_conf_exc(f"missing feature_default_value for feature, feature_name={feature_name}") + raise ConfigurationError(f"Missing feature_default_value for feature, feature_name={feature_name}") + # validate rules rules = feature_dict_def.get(RULES_KEY, []) if not rules: return + if not isinstance(rules, list): - self._raise_conf_exc(f"feature rules is not a list, feature_name={feature_name}") + raise ConfigurationError(f"Feature rules is not a list, feature_name={feature_name}") + for rule in rules: self._validate_rule(feature_name, rule) 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") + raise ConfigurationError("invalid AWS AppConfig JSON schema detected, root schema is not a dictionary") + features_dict = schema.get(FEATURES_KEY) if not isinstance(features_dict, dict): - return self._raise_conf_exc("invalid AWS AppConfig JSON schema detected, missing features dictionary") + raise ConfigurationError("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/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 1f97104ceed..93c63544085 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -68,7 +68,8 @@ Potential changes to be validated when docs are in a better shape - [x] ~~Rename `value_if_missing` param to `default` to match Python consistency (e.g. `os.getenv("VAR", default=False)`)~~ - [x] ~~Rename `get_feature` to `evaluate`~~ - [x] ~~Rename `get_all_enabled_feature_toggles` to `get_enabled_features`~~ +- [x] ~~Review redundant logging~~ - [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given StoreProvider) - [ ] Review why we're testing a private method(`is_rule_matched`) - [ ] Review `get_configuration`, `get_json_configuration` -- [ ] Review potentially redundant logging +- [ ] Consider fine grained ConfigurationError instead of a single ConfigurationError From 2f5ccd3e6b22ff157d12e725b127b3f045141daf Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 30 Jul 2021 17:50:48 +0200 Subject: [PATCH 13/45] chore: make method static --- aws_lambda_powertools/utilities/feature_toggles/schema.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_toggles/schema.py index 9453467702f..22221f02a7f 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema.py @@ -26,7 +26,8 @@ class SchemaValidator: def __init__(self, logger: Logger): self._logger = logger - def _validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: + @staticmethod + def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): raise ConfigurationError(f"invalid condition type, not a dictionary, rule_name={rule_name}") From 0d44e4d9771259184eec810559506fef9e119e7b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Sun, 1 Aug 2021 13:20:19 +0200 Subject: [PATCH 14/45] refactor(tests): simplify with pytest raise match str --- .../feature_toggles/test_schema_validation.py | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-) diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 184f448322a..7e369037042 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -296,11 +296,9 @@ def test_validate_condition_invalid_condition_key(): 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) + with pytest.raises(ConfigurationError, match="Invalid key value"): + validator._validate_condition("foo", condition) def test_validate_condition_missing_condition_value(): @@ -309,12 +307,9 @@ def test_validate_condition_missing_condition_value(): condition = {"action": ACTION.EQUALS.value, "key": "Foo"} # WHEN calling _validate_condition - with pytest.raises(ConfigurationError) as err: + with pytest.raises(ConfigurationError, match="Missing condition value"): 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 @@ -323,8 +318,6 @@ def test_validate_rule_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) + with pytest.raises(ConfigurationError, match="Invalid rule_name"): + validator._validate_rule(rule_name, rule) From 6100d38f5f6f2d35dcb9eb06510512b5d3a6892b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Sun, 1 Aug 2021 14:04:32 +0200 Subject: [PATCH 15/45] refactor: rename feature_name to name --- .../feature_toggles/feature_flags.py | 30 +++++++--------- .../feature_toggles/test_feature_toggles.py | 34 ++++++++----------- 2 files changed, 26 insertions(+), 38 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index 8f59f75f516..7de79603bb3 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -52,14 +52,13 @@ def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], context: Dic ): logger.debug( 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)} " + f"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}, " - f"feature_name={feature_name}" + f"rule matched, rule_name={rule_name}, rule_default_value={rule_default_value}, " f"name={feature_name}" ) return True return False @@ -79,7 +78,7 @@ def _handle_rules( # 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}, " - f"feature_name={feature_name}" + f"name={feature_name}" ) return feature_default_value return False @@ -103,14 +102,14 @@ def get_configuration(self) -> Dict[str, Any]: self._schema_validator.validate_json_schema(config) return config - def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: + def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. See below for explanation. Parameters ---------- - feature_name: str + name: str feature name that you wish to fetch context: Optional[Dict[str, Any]] dict of attributes that you would like to match the rules @@ -138,11 +137,10 @@ def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = Non logger.debug("Unable to get feature toggles JSON, returning provided default value") return default - feature: Dict[str, Dict] = toggles_dict.get(schema.FEATURES_KEY, {}).get(feature_name, None) + feature: Dict[str, Dict] = toggles_dict.get(schema.FEATURES_KEY, {}).get(name, None) if feature is None: logger.debug( - f"feature does not appear in configuration, using provided default, " - f"feature_name={feature_name}, default={default}" + f"feature does not appear in configuration, using provided default, " f"name={name}, default={default}" ) return default @@ -151,16 +149,14 @@ def evaluate(self, *, feature_name: str, context: Optional[Dict[str, Any]] = Non if not rules_list: # no rules but value logger.debug( - f"no rules found, returning feature default value, feature_name={feature_name}, " + f"no rules found, returning feature default value, name={name}, " f"default_value={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}" - ) + logger.debug(f"looking for rule match, name={name}, feature_default_value={feature_default_value}") return self._handle_rules( - feature_name=feature_name, + feature_name=name, context=context, feature_default_value=bool(feature_default_value), rules=cast(List, rules_list), @@ -198,9 +194,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L rules_list = feature_dict_def.get(schema.RULES_KEY, []) feature_default_value = feature_dict_def.get(schema.FEATURE_DEFAULT_VAL_KEY) if feature_default_value and not rules_list: - self._logger.debug( - f"feature is enabled by default and has no defined rules, feature_name={feature_name}" - ) + self._logger.debug(f"feature is enabled by default and has no defined rules, name={feature_name}") features_enabled.append(feature_name) elif self._handle_rules( feature_name=feature_name, @@ -208,7 +202,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L feature_default_value=feature_default_value, rules=rules_list, ): - self._logger.debug(f"feature's calculated value is True, feature_name={feature_name}") + self._logger.debug(f"feature's calculated value is True, name={feature_name}") features_enabled.append(feature_name) return features_enabled diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 5ff96b93ecb..28d88956161 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -68,7 +68,7 @@ def test_toggles_rule_does_not_match(mocker, config): } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={}, default=False) assert toggle == expected_value @@ -79,7 +79,7 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): mocked_app_config_schema = {"features": {"my_fake_feature": {"feature_default_value": True}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={}, default=expected_value) + toggle = conf_store.evaluate(name="my_feature", context={}, default=expected_value) assert toggle == expected_value @@ -89,7 +89,7 @@ def test_toggles_no_rules(mocker, config): expected_value = True mocked_app_config_schema = {"features": {"my_feature": {"feature_default_value": expected_value}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -117,7 +117,7 @@ def test_toggles_conditions_no_match(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -153,7 +153,7 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate( - feature_name="my_feature", + name="my_feature", context={ "tenant_id": tenant_id_val, "username": username_val, @@ -194,7 +194,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_val @@ -253,23 +253,17 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) # match first rule - toggle = conf_store.evaluate( - feature_name="my_feature", context={"tenant_id": "6", "username": "abcd"}, default=False - ) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "abcd"}, default=False) assert toggle == expected_value_first_check # match second rule - toggle = conf_store.evaluate( - feature_name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False - ) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False) assert toggle == expected_value_second_check # match no rule - toggle = conf_store.evaluate( - feature_name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, default=False - ) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, default=False) assert toggle == expected_value_third_check # feature doesn't exist toggle = conf_store.evaluate( - feature_name="my_fake_feature", + name="my_fake_feature", context={"tenant_id": "11114446", "username": "ab"}, default=expected_value_fourth_case, ) @@ -300,7 +294,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -327,7 +321,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): }, } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(feature_name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -419,7 +413,7 @@ def test_get_feature_toggle_handles_error(mocker, config): conf_store = FeatureFlags(schema_fetcher) # WHEN calling evaluate - toggle = conf_store.evaluate(feature_name="Foo", default=False) + toggle = conf_store.evaluate(name="Foo", default=False) # THEN handle the error and return the default assert toggle is False @@ -474,7 +468,7 @@ def test_is_rule_matched_no_matches(mocker, config): conf_store = init_configuration_store(mocker, {}, config) # WHEN calling _is_rule_matched - result = conf_store._is_rule_matched("feature_name", rule, rules_context) + result = conf_store._is_rule_matched("name", rule, rules_context) # THEN return False assert result is False From 7370464ca29a7b6c704988aebc073c600f4d5cab Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:03:51 +0200 Subject: [PATCH 16/45] refactor: remove redundant logger; rename method to validate --- .../feature_toggles/feature_flags.py | 39 ++++++++--- .../utilities/feature_toggles/schema.py | 9 ++- .../feature_toggles/test_schema_validation.py | 64 +++++++++---------- 3 files changed, 65 insertions(+), 47 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py index 7de79603bb3..bc7b164ec25 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py @@ -1,5 +1,5 @@ import logging -from typing import Any, Dict, List, Optional, cast +from typing import Any, Dict, List, Optional, Union, cast from . import schema from .base import StoreProvider @@ -19,7 +19,7 @@ def __init__(self, store: StoreProvider): """ self._logger = logger self._store = store - self._schema_validator = schema.SchemaValidator(self._logger) + self._schema_validator = schema.SchemaValidator() def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: if not context_value: @@ -83,7 +83,7 @@ def _handle_rules( return feature_default_value return False - def get_configuration(self) -> Dict[str, Any]: + def get_configuration(self) -> Union[Dict[str, Dict], Dict]: """Get configuration string from AWs AppConfig and returned the parsed JSON dictionary Raises @@ -93,14 +93,33 @@ def get_configuration(self) -> Dict[str, Any]: Returns ------ - Dict[str, Any] + Dict[str, Dict] parsed JSON dictionary + + { + "my_feature": { + "feature_default_value": True, + "rules": [ + { + "rule_name": "tenant id equals 345345435", + "value_when_applies": False, + "conditions": [ + { + "action": "EQUALS", + "key": "tenant_id", + "value": "345345435", + } + ], + }, + ], + } + } """ # parse result conf as JSON, keep in cache for self.max_age seconds config = self._store.get_json_configuration() # validate schema - self._schema_validator.validate_json_schema(config) - return config + self._schema_validator.validate(config) + return config.get(schema.FEATURES_KEY, {}) def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. @@ -132,12 +151,12 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau context = {} try: - toggles_dict: Dict[str, Any] = self.get_configuration() + features = self.get_configuration() except ConfigurationError: logger.debug("Unable to get feature toggles JSON, returning provided default value") return default - feature: Dict[str, Dict] = toggles_dict.get(schema.FEATURES_KEY, {}).get(name, None) + feature = features.get(name) if feature is None: logger.debug( f"feature does not appear in configuration, using provided default, " f"name={name}, default={default}" @@ -153,6 +172,7 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau f"default_value={feature_default_value}" ) return bool(feature_default_value) + # look for first rule match logger.debug(f"looking for rule match, name={name}, feature_default_value={feature_default_value}") return self._handle_rules( @@ -184,12 +204,11 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L features_enabled: List[str] = [] try: - toggles_dict: Dict[str, Any] = self.get_configuration() + features: Dict[str, Any] = self.get_configuration() except ConfigurationError: logger.debug("unable to get feature toggles JSON") return features_enabled - features: Dict[str, Any] = toggles_dict.get(schema.FEATURES_KEY, {}) for feature_name, feature_dict_def in features.items(): rules_list = feature_dict_def.get(schema.RULES_KEY, []) feature_default_value = feature_dict_def.get(schema.FEATURE_DEFAULT_VAL_KEY) diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_toggles/schema.py index 22221f02a7f..5e814a5d596 100644 --- a/aws_lambda_powertools/utilities/feature_toggles/schema.py +++ b/aws_lambda_powertools/utilities/feature_toggles/schema.py @@ -1,9 +1,11 @@ +import logging from enum import Enum -from logging import Logger from typing import Any, Dict from .exceptions import ConfigurationError +logger = logging.getLogger(__name__) + FEATURES_KEY = "features" RULES_KEY = "rules" FEATURE_DEFAULT_VAL_KEY = "feature_default_value" @@ -23,9 +25,6 @@ class ACTION(str, Enum): class SchemaValidator: - def __init__(self, logger: Logger): - self._logger = logger - @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): @@ -82,7 +81,7 @@ def _validate_feature(self, feature_name: str, feature_dict_def: Dict[str, Any]) for rule in rules: self._validate_rule(feature_name, rule) - def validate_json_schema(self, schema: Dict[str, Any]) -> None: + def validate(self, schema: Dict[str, Any]) -> None: if not isinstance(schema, dict): raise ConfigurationError("invalid AWS AppConfig JSON schema detected, root schema is not a dictionary") diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 7e369037042..922143eb6b9 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -23,64 +23,64 @@ def test_invalid_features_dict(): schema = {} # empty dict - validator = SchemaValidator(logger) + validator = SchemaValidator() with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) schema = [] # invalid type with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # invalid features key schema = {FEATURES_KEY: []} with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) def test_empty_features_not_fail(): schema = {FEATURES_KEY: {}} - validator = SchemaValidator(logger) - validator.validate_json_schema(schema) + validator = SchemaValidator() + validator.validate(schema) def test_invalid_feature_dict(): # invalid feature type, not dict schema = {FEATURES_KEY: {"my_feature": []}} - validator = SchemaValidator(logger) + validator = SchemaValidator() with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # empty feature dict schema = {FEATURES_KEY: {"my_feature": {}}} with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}} with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean #2 schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: 5}}} with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # invalid rules type, not list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}} with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) def test_valid_feature_dict(): # no rules list at all schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}}} - validator = SchemaValidator(logger) - validator.validate_json_schema(schema) + validator = SchemaValidator() + validator.validate(schema) # empty rules list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}}} - validator.validate_json_schema(schema) + validator.validate(schema) def test_invalid_rule(): @@ -96,9 +96,9 @@ def test_invalid_rule(): } } } - validator = SchemaValidator(logger) + validator = SchemaValidator() with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # rules RULE_DEFAULT_VALUE is not bool schema = { @@ -115,7 +115,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # missing conditions list schema = { @@ -132,7 +132,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # condition list is empty schema = { @@ -146,7 +146,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # condition is invalid type, not list schema = { @@ -160,7 +160,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) def test_invalid_condition(): @@ -179,9 +179,9 @@ def test_invalid_condition(): } } } - validator = SchemaValidator(logger) + validator = SchemaValidator() with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # missing condition key and value schema = { @@ -199,7 +199,7 @@ def test_invalid_condition(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) # invalid condition key type, not string schema = { @@ -221,11 +221,11 @@ def test_invalid_condition(): } } with pytest.raises(ConfigurationError): - validator.validate_json_schema(schema) + validator.validate(schema) def test_valid_condition_all_actions(): - validator = SchemaValidator(logger) + validator = SchemaValidator() schema = { FEATURES_KEY: { "my_feature": { @@ -261,12 +261,12 @@ def test_valid_condition_all_actions(): } }, } - validator.validate_json_schema(schema) + validator.validate(schema) def test_validate_condition_invalid_condition_type(): # GIVEN an invalid condition type of empty dict - validator = SchemaValidator(logger) + validator = SchemaValidator() condition = {} # WHEN calling _validate_condition @@ -279,7 +279,7 @@ def test_validate_condition_invalid_condition_type(): def test_validate_condition_invalid_condition_action(): # GIVEN an invalid condition action of foo - validator = SchemaValidator(logger) + validator = SchemaValidator() condition = {"action": "foo"} # WHEN calling _validate_condition @@ -292,7 +292,7 @@ def test_validate_condition_invalid_condition_action(): def test_validate_condition_invalid_condition_key(): # GIVEN a configuration with a missing "key" - validator = SchemaValidator(logger) + validator = SchemaValidator() condition = {"action": ACTION.EQUALS.value} # WHEN calling _validate_condition @@ -303,7 +303,7 @@ def test_validate_condition_invalid_condition_key(): def test_validate_condition_missing_condition_value(): # GIVEN a configuration with a missing condition value - validator = SchemaValidator(logger) + validator = SchemaValidator() condition = {"action": ACTION.EQUALS.value, "key": "Foo"} # WHEN calling _validate_condition @@ -313,7 +313,7 @@ def test_validate_condition_missing_condition_value(): def test_validate_rule_invalid_rule_name(): # GIVEN a rule_name not in the rule dict - validator = SchemaValidator(logger) + validator = SchemaValidator() rule_name = "invalid_rule_name" rule = {"missing": ""} From d50650c077eec141511870f259a6d35a65653029 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:05:32 +0200 Subject: [PATCH 17/45] refactor: rename to feature_flags --- .../{feature_toggles => feature_flags}/__init__.py | 0 .../{feature_toggles => feature_flags}/appconfig.py | 0 .../utilities/{feature_toggles => feature_flags}/base.py | 0 .../{feature_toggles => feature_flags}/exceptions.py | 0 .../{feature_toggles => feature_flags}/feature_flags.py | 0 .../{feature_toggles => feature_flags}/schema.py | 0 tests/functional/feature_toggles/test_feature_toggles.py | 8 ++++---- .../functional/feature_toggles/test_schema_validation.py | 4 ++-- 8 files changed, 6 insertions(+), 6 deletions(-) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/__init__.py (100%) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/appconfig.py (100%) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/base.py (100%) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/exceptions.py (100%) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/feature_flags.py (100%) rename aws_lambda_powertools/utilities/{feature_toggles => feature_flags}/schema.py (100%) diff --git a/aws_lambda_powertools/utilities/feature_toggles/__init__.py b/aws_lambda_powertools/utilities/feature_flags/__init__.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/__init__.py rename to aws_lambda_powertools/utilities/feature_flags/__init__.py diff --git a/aws_lambda_powertools/utilities/feature_toggles/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/appconfig.py rename to aws_lambda_powertools/utilities/feature_flags/appconfig.py diff --git a/aws_lambda_powertools/utilities/feature_toggles/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/base.py rename to aws_lambda_powertools/utilities/feature_flags/base.py diff --git a/aws_lambda_powertools/utilities/feature_toggles/exceptions.py b/aws_lambda_powertools/utilities/feature_flags/exceptions.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/exceptions.py rename to aws_lambda_powertools/utilities/feature_flags/exceptions.py diff --git a/aws_lambda_powertools/utilities/feature_toggles/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/feature_flags.py rename to aws_lambda_powertools/utilities/feature_flags/feature_flags.py diff --git a/aws_lambda_powertools/utilities/feature_toggles/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py similarity index 100% rename from aws_lambda_powertools/utilities/feature_toggles/schema.py rename to aws_lambda_powertools/utilities/feature_flags/schema.py diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 28d88956161..dc137172f11 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -3,10 +3,10 @@ import pytest from botocore.config import Config -from aws_lambda_powertools.utilities.feature_toggles import ConfigurationError, schema -from aws_lambda_powertools.utilities.feature_toggles.appconfig import AppConfigStore -from aws_lambda_powertools.utilities.feature_toggles.feature_flags import FeatureFlags -from aws_lambda_powertools.utilities.feature_toggles.schema import ACTION +from aws_lambda_powertools.utilities.feature_flags import ConfigurationError, schema +from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore +from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags +from aws_lambda_powertools.utilities.feature_flags.schema import ACTION from aws_lambda_powertools.utilities.parameters import GetParameterError diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 922143eb6b9..7dd0a39545b 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -2,8 +2,8 @@ import pytest # noqa: F401 -from aws_lambda_powertools.utilities.feature_toggles.exceptions import ConfigurationError -from aws_lambda_powertools.utilities.feature_toggles.schema import ( +from aws_lambda_powertools.utilities.feature_flags.exceptions import ConfigurationError +from aws_lambda_powertools.utilities.feature_flags.schema import ( ACTION, CONDITION_ACTION, CONDITION_KEY, From 0c0a4f76edc958adb7ce7d6da45b6c4ee37fef36 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:16:55 +0200 Subject: [PATCH 18/45] refactor(schema-validator): accept schema in constructor --- .../utilities/feature_flags/feature_flags.py | 7 +- .../utilities/feature_flags/schema.py | 9 ++- .../feature_toggles/test_schema_validation.py | 66 ++++++++++--------- 3 files changed, 44 insertions(+), 38 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index bc7b164ec25..ec56f8b9616 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -19,7 +19,6 @@ def __init__(self, store: StoreProvider): """ self._logger = logger self._store = store - self._schema_validator = schema.SchemaValidator() def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: if not context_value: @@ -117,8 +116,10 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: """ # parse result conf as JSON, keep in cache for self.max_age seconds config = self._store.get_json_configuration() - # validate schema - self._schema_validator.validate(config) + + validator = schema.SchemaValidator(schema=config) + validator.validate() + return config.get(schema.FEATURES_KEY, {}) def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 5e814a5d596..58b01aa204d 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -25,6 +25,9 @@ class ACTION(str, Enum): class SchemaValidator: + def __init__(self, schema: Dict[str, Any]): + self.schema = schema + @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): @@ -81,11 +84,11 @@ def _validate_feature(self, feature_name: str, feature_dict_def: Dict[str, Any]) for rule in rules: self._validate_rule(feature_name, rule) - def validate(self, schema: Dict[str, Any]) -> None: - if not isinstance(schema, dict): + def validate(self) -> None: + if not isinstance(self.schema, dict): raise ConfigurationError("invalid AWS AppConfig JSON schema detected, root schema is not a dictionary") - features_dict = schema.get(FEATURES_KEY) + features_dict = self.schema.get(FEATURES_KEY) if not isinstance(features_dict, dict): raise ConfigurationError("invalid AWS AppConfig JSON schema detected, missing features dictionary") diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 7dd0a39545b..61ac11024a7 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -19,68 +19,70 @@ logger = logging.getLogger(__name__) +EMPTY_SCHEMA = {"": ""} + def test_invalid_features_dict(): schema = {} # empty dict - validator = SchemaValidator() + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() schema = [] # invalid type with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # invalid features key schema = {FEATURES_KEY: []} with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() def test_empty_features_not_fail(): schema = {FEATURES_KEY: {}} - validator = SchemaValidator() - validator.validate(schema) + validator = SchemaValidator(schema) + validator.validate() def test_invalid_feature_dict(): # invalid feature type, not dict schema = {FEATURES_KEY: {"my_feature": []}} - validator = SchemaValidator() + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # empty feature dict schema = {FEATURES_KEY: {"my_feature": {}}} with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}} with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean #2 schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: 5}}} with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # invalid rules type, not list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}} with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() def test_valid_feature_dict(): # no rules list at all schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}}} - validator = SchemaValidator() - validator.validate(schema) + validator = SchemaValidator(schema) + validator.validate() # empty rules list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}}} - validator.validate(schema) + validator.validate() def test_invalid_rule(): @@ -96,9 +98,9 @@ def test_invalid_rule(): } } } - validator = SchemaValidator() + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # rules RULE_DEFAULT_VALUE is not bool schema = { @@ -115,7 +117,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # missing conditions list schema = { @@ -132,7 +134,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # condition list is empty schema = { @@ -146,7 +148,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # condition is invalid type, not list schema = { @@ -160,7 +162,7 @@ def test_invalid_rule(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() def test_invalid_condition(): @@ -179,9 +181,9 @@ def test_invalid_condition(): } } } - validator = SchemaValidator() + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # missing condition key and value schema = { @@ -199,7 +201,7 @@ def test_invalid_condition(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() # invalid condition key type, not string schema = { @@ -221,11 +223,10 @@ def test_invalid_condition(): } } with pytest.raises(ConfigurationError): - validator.validate(schema) + validator.validate() def test_valid_condition_all_actions(): - validator = SchemaValidator() schema = { FEATURES_KEY: { "my_feature": { @@ -261,12 +262,13 @@ def test_valid_condition_all_actions(): } }, } - validator.validate(schema) + validator = SchemaValidator(schema) + validator.validate() def test_validate_condition_invalid_condition_type(): # GIVEN an invalid condition type of empty dict - validator = SchemaValidator() + validator = SchemaValidator(EMPTY_SCHEMA) condition = {} # WHEN calling _validate_condition @@ -279,7 +281,7 @@ def test_validate_condition_invalid_condition_type(): def test_validate_condition_invalid_condition_action(): # GIVEN an invalid condition action of foo - validator = SchemaValidator() + validator = SchemaValidator(EMPTY_SCHEMA) condition = {"action": "foo"} # WHEN calling _validate_condition @@ -292,7 +294,7 @@ def test_validate_condition_invalid_condition_action(): def test_validate_condition_invalid_condition_key(): # GIVEN a configuration with a missing "key" - validator = SchemaValidator() + validator = SchemaValidator(EMPTY_SCHEMA) condition = {"action": ACTION.EQUALS.value} # WHEN calling _validate_condition @@ -303,7 +305,7 @@ def test_validate_condition_invalid_condition_key(): def test_validate_condition_missing_condition_value(): # GIVEN a configuration with a missing condition value - validator = SchemaValidator() + validator = SchemaValidator(EMPTY_SCHEMA) condition = {"action": ACTION.EQUALS.value, "key": "Foo"} # WHEN calling _validate_condition @@ -313,7 +315,7 @@ def test_validate_condition_missing_condition_value(): def test_validate_rule_invalid_rule_name(): # GIVEN a rule_name not in the rule dict - validator = SchemaValidator() + validator = SchemaValidator(EMPTY_SCHEMA) rule_name = "invalid_rule_name" rule = {"missing": ""} From 0e15b4bb63f471eee6af3d0488ca0c6537e066d3 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:26:05 +0200 Subject: [PATCH 19/45] refactor(schema-validator): private fn signature name --- .../utilities/feature_flags/schema.py | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 58b01aa204d..dfcfdf647fd 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -1,6 +1,6 @@ import logging from enum import Enum -from typing import Any, Dict +from typing import Any, Dict, Optional from .exceptions import ConfigurationError @@ -28,6 +28,9 @@ class SchemaValidator: def __init__(self, schema: Dict[str, Any]): self.schema = schema + if not isinstance(self.schema, dict): + raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): @@ -65,32 +68,29 @@ def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: for condition in conditions: self._validate_condition(rule_name, condition) - def _validate_feature(self, feature_name: str, feature_dict_def: Dict[str, Any]) -> None: - if not feature_dict_def or not isinstance(feature_dict_def, dict): - raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {feature_name} is invalid") + def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: + if not feature or not isinstance(feature, dict): + raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {name} is invalid") - feature_default_value = feature_dict_def.get(FEATURE_DEFAULT_VAL_KEY) + feature_default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) if feature_default_value is None or not isinstance(feature_default_value, bool): - raise ConfigurationError(f"Missing feature_default_value for feature, feature_name={feature_name}") + raise ConfigurationError(f"Missing feature_default_value for feature, feature_name={name}") # validate rules - rules = feature_dict_def.get(RULES_KEY, []) + rules = feature.get(RULES_KEY, []) if not rules: return if not isinstance(rules, list): - raise ConfigurationError(f"Feature rules is not a list, feature_name={feature_name}") + raise ConfigurationError(f"Feature rules is not a list, feature_name={name}") for rule in rules: - self._validate_rule(feature_name, rule) + self._validate_rule(name, rule) def validate(self) -> None: - if not isinstance(self.schema, dict): - raise ConfigurationError("invalid AWS AppConfig JSON schema detected, root schema is not a dictionary") - - features_dict = self.schema.get(FEATURES_KEY) - if not isinstance(features_dict, dict): - raise ConfigurationError("invalid AWS AppConfig JSON schema detected, missing features dictionary") + features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) + if not isinstance(features, dict): + raise ConfigurationError(f"'features' key must be present, schema={self.schema}") - for feature_name, feature_dict_def in features_dict.items(): - self._validate_feature(feature_name, feature_dict_def) + for name, feature in features.items(): + self._validate_feature(name, feature) From 7eb8a679c7790e7177ace2c7969b0f2ca95c4bd6 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:45:38 +0200 Subject: [PATCH 20/45] revert: re-add root schema validation under validate --- aws_lambda_powertools/utilities/feature_flags/schema.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index dfcfdf647fd..30d3ea5d1ad 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -28,8 +28,9 @@ class SchemaValidator: def __init__(self, schema: Dict[str, Any]): self.schema = schema - if not isinstance(self.schema, dict): - raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + @staticmethod + def _is_dict_and_non_empty(value: Optional[Dict]): + return not value or not isinstance(value, dict) @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: @@ -88,6 +89,9 @@ def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: self._validate_rule(name, rule) def validate(self) -> None: + if self._is_dict_and_non_empty(self.schema): + raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) if not isinstance(features, dict): raise ConfigurationError(f"'features' key must be present, schema={self.schema}") From 9e757fa108f7af342ccf8659c5ec7ed94215291d Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:51:08 +0200 Subject: [PATCH 21/45] fix: regression on empty schemas --- aws_lambda_powertools/utilities/feature_flags/schema.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 30d3ea5d1ad..5ba41526c40 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -30,7 +30,7 @@ def __init__(self, schema: Dict[str, Any]): @staticmethod def _is_dict_and_non_empty(value: Optional[Dict]): - return not value or not isinstance(value, dict) + return value is not None or isinstance(value, dict) @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: @@ -71,6 +71,7 @@ def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: if not feature or not isinstance(feature, dict): + # if self._is_dict_and_non_empty(feature): raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {name} is invalid") feature_default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) @@ -89,7 +90,7 @@ def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: self._validate_rule(name, rule) def validate(self) -> None: - if self._is_dict_and_non_empty(self.schema): + if not self._is_dict_and_non_empty(self.schema): raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) From 3a71fdfcca6c0eb0868e49ea862eba7dc64ef29b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 17:51:08 +0200 Subject: [PATCH 22/45] fix: regression on empty schemas --- aws_lambda_powertools/utilities/feature_flags/schema.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 30d3ea5d1ad..55a7261e2c3 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -30,7 +30,7 @@ def __init__(self, schema: Dict[str, Any]): @staticmethod def _is_dict_and_non_empty(value: Optional[Dict]): - return not value or not isinstance(value, dict) + return not value or isinstance(value, dict) @staticmethod def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: @@ -71,6 +71,7 @@ def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: if not feature or not isinstance(feature, dict): + # if self._is_dict_and_non_empty(feature): raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {name} is invalid") feature_default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) @@ -89,7 +90,7 @@ def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: self._validate_rule(name, rule) def validate(self) -> None: - if self._is_dict_and_non_empty(self.schema): + if not self._is_dict_and_non_empty(self.schema): raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) From bf4bdd63b2f98c25de997a330f1d00f323315ae7 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 20:39:39 +0200 Subject: [PATCH 23/45] refactor(schema-validator): initial change to classes --- .../utilities/feature_flags/base.py | 6 + .../utilities/feature_flags/schema.py | 144 ++++++++++++------ .../feature_toggles/test_schema_validation.py | 16 +- 3 files changed, 111 insertions(+), 55 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py index 5e92fd7a898..f1f7c02217f 100644 --- a/aws_lambda_powertools/utilities/feature_flags/base.py +++ b/aws_lambda_powertools/utilities/feature_flags/base.py @@ -22,3 +22,9 @@ def get_json_configuration(self) -> Dict[str, Any]: parsed JSON dictionary """ return NotImplemented # pragma: no cover + + +class BaseValidator(ABC): + @abstractmethod + def validate(self): + return NotImplemented # pragma: no cover diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 55a7261e2c3..77bf82c1869 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -1,7 +1,8 @@ import logging from enum import Enum -from typing import Any, Dict, Optional +from typing import Any, Dict, List, Optional +from .base import BaseValidator from .exceptions import ConfigurationError logger = logging.getLogger(__name__) @@ -28,74 +29,117 @@ class SchemaValidator: def __init__(self, schema: Dict[str, Any]): self.schema = schema - @staticmethod - def _is_dict_and_non_empty(value: Optional[Dict]): - return not value or isinstance(value, dict) + def validate(self) -> None: + if not isinstance(self.schema, dict): + raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + + features = Features(schema=self.schema) + features.validate() + + +class Features(BaseValidator): + def __init__(self, schema): + self.schema = schema + self.features: Optional[Dict[str, Dict]] = None + + if isinstance(self.schema, dict): + self.features = self.schema.get(FEATURES_KEY) + + def validate(self): + if not isinstance(self.features, dict): + raise ConfigurationError(f"'features' key must be a dictionary, schema={self.schema}") + + for name, feature in self.features.items(): + self.validate_feature(name, feature) + rules = FeatureRules(feature=feature, feature_name=name) + rules.validate() @staticmethod - def _validate_condition(rule_name: str, condition: Dict[str, str]) -> None: - if not condition or not isinstance(condition, dict): - raise ConfigurationError(f"invalid condition type, not a dictionary, rule_name={rule_name}") + def validate_feature(name, feature): + if not feature or not isinstance(feature, dict): + raise ConfigurationError(f"Feature must be a non-empty dictionary, feature={name}") - action = condition.get(CONDITION_ACTION, "") - if action not in [ACTION.EQUALS.value, ACTION.STARTSWITH.value, ACTION.ENDSWITH.value, ACTION.CONTAINS.value]: - raise ConfigurationError(f"invalid action value, rule_name={rule_name}, action={action}") + default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) + if default_value is None or not isinstance(default_value, bool): + raise ConfigurationError(f"'feature_default_value' boolean key must be present, feature_name={name}") - key = condition.get(CONDITION_KEY, "") - if not key or not isinstance(key, str): - raise ConfigurationError(f"Invalid key value, key has to be a non empty string, rule_name={rule_name}") - value = condition.get(CONDITION_VALUE, "") - if not value: - raise ConfigurationError(f"Missing condition value, rule_name={rule_name}") +class FeatureRules(BaseValidator): + def __init__(self, feature: Dict[str, Any], feature_name: str): + self.feature = feature + self.feature_name = feature_name + self.rules: Optional[List[Dict]] = self.feature.get(RULES_KEY) + + def validate(self): + if not isinstance(self.rules, list): + raise ConfigurationError(f"Feature rules is not a list, feature_name={self.feature_name}") + + if not self.rules: + logger.debug("Rules are empty, ignoring validation") + return - def _validate_rule(self, feature_name: str, rule: Dict[str, Any]) -> None: + for rule in self.rules: + self.validate_rule(rule, self.feature) + conditions = FeatureRuleConditions(rule=rule, rule_name=rule.get(RULE_NAME_KEY)) + conditions.validate() + + def validate_rule(self, rule, feature_name): if not rule or not isinstance(rule, dict): - raise ConfigurationError(f"Feature rule is not a dictionary, feature_name={feature_name}") + raise ConfigurationError(f"Feature rule must be a dictionary, feature_name={feature_name}") + + self.validate_rule_name(rule, feature_name) + self.validate_rule_default_value(rule) + @staticmethod + def validate_rule_name(rule, feature_name): rule_name = rule.get(RULE_NAME_KEY) if not rule_name or rule_name is None or not isinstance(rule_name, str): - raise ConfigurationError(f"Invalid rule_name, feature_name={feature_name}") + raise ConfigurationError( + f"'rule_name' key must be present and have a non-empty string, feature_name={feature_name}" + ) + @staticmethod + def validate_rule_default_value(rule): + rule_name = rule.get(RULE_NAME_KEY) rule_default_value = rule.get(RULE_DEFAULT_VALUE) if rule_default_value is None or not isinstance(rule_default_value, bool): - raise ConfigurationError(f"Invalid rule_default_value, rule_name={rule_name}") - - conditions = rule.get(CONDITIONS_KEY, {}) - if not conditions or not isinstance(conditions, list): - raise ConfigurationError(f"Invalid condition, rule_name={rule_name}") + raise ConfigurationError(f"'rule_default_value' key must have be bool, rule_name={rule_name}") - # validate conditions - for condition in conditions: - self._validate_condition(rule_name, condition) - def _validate_feature(self, name: str, feature: Dict[str, Any]) -> None: - if not feature or not isinstance(feature, dict): - # if self._is_dict_and_non_empty(feature): - raise ConfigurationError(f"Invalid AWS AppConfig JSON schema detected, feature {name} is invalid") +class FeatureRuleConditions(BaseValidator): + def __init__(self, rule: Dict[str, Any], rule_name: str): + self.conditions = rule.get(CONDITIONS_KEY, {}) + self.rule_name = rule_name - feature_default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) - if feature_default_value is None or not isinstance(feature_default_value, bool): - raise ConfigurationError(f"Missing feature_default_value for feature, feature_name={name}") + def validate(self): + if not self.conditions or not isinstance(self.conditions, list): + raise ConfigurationError(f"Invalid condition, rule_name={self.rule_name}") - # validate rules - rules = feature.get(RULES_KEY, []) - if not rules: - return + for condition in self.conditions: + self._validate_condition(rule_name=self.rule_name, condition=condition) - if not isinstance(rules, list): - raise ConfigurationError(f"Feature rules is not a list, feature_name={name}") + def _validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: + if not condition or not isinstance(condition, dict): + raise ConfigurationError(f"invalid condition type, not a dictionary, rule_name={rule_name}") - for rule in rules: - self._validate_rule(name, rule) + self._validate_condition_action(condition=condition, rule_name=rule_name) + self._validate_condition_key(condition=condition, rule_name=rule_name) + self._validate_condition_value(condition=condition, rule_name=rule_name) - def validate(self) -> None: - if not self._is_dict_and_non_empty(self.schema): - raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + @staticmethod + def _validate_condition_action(condition: Dict[str, Any], rule_name: str): + action = condition.get(CONDITION_ACTION, "") + if action not in ACTION.__members__: + raise ConfigurationError(f"invalid action value, rule_name={rule_name}, action={action}") - features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) - if not isinstance(features, dict): - raise ConfigurationError(f"'features' key must be present, schema={self.schema}") + @staticmethod + def _validate_condition_key(condition: Dict[str, Any], rule_name: str): + key = condition.get(CONDITION_KEY, "") + if not key or not isinstance(key, str): + raise ConfigurationError(f"Invalid key value, key has to be a non empty string, rule_name={rule_name}") - for name, feature in features.items(): - self._validate_feature(name, feature) + @staticmethod + def _validate_condition_value(condition: Dict[str, Any], rule_name: str): + value = condition.get(CONDITION_VALUE, "") + if not value: + raise ConfigurationError(f"Missing condition value, rule_name={rule_name}") diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 61ac11024a7..de24fa3e1c9 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -14,6 +14,7 @@ RULE_DEFAULT_VALUE, RULE_NAME_KEY, RULES_KEY, + FeatureRules, SchemaValidator, ) @@ -315,11 +316,16 @@ def test_validate_condition_missing_condition_value(): def test_validate_rule_invalid_rule_name(): # GIVEN a rule_name not in the rule dict - validator = SchemaValidator(EMPTY_SCHEMA) - rule_name = "invalid_rule_name" - rule = {"missing": ""} + feature = { + "feature_default_value": True, + "rules": [ + {"invalid_rule_name": "tenant id equals 345345435"}, + ], + } + rule = feature[RULES_KEY][0] # WHEN calling _validate_rule # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Invalid rule_name"): - validator._validate_rule(rule_name, rule) + with pytest.raises(ConfigurationError, match="'rule_name' key must be present*"): + rules_validator = FeatureRules(feature=feature, feature_name="my_feature") + rules_validator.validate_rule_name(rule=rule, feature_name="my_feature") From 7fa8ec27f5c33fa532f1b2f9b9e81e11c93a1c7c Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 20:44:35 +0200 Subject: [PATCH 24/45] refactor(schema-validator): rename each Validator --- .../utilities/feature_flags/schema.py | 12 ++++++------ .../feature_toggles/test_schema_validation.py | 4 ++-- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 77bf82c1869..af72bc415e4 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -33,11 +33,11 @@ def validate(self) -> None: if not isinstance(self.schema, dict): raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") - features = Features(schema=self.schema) + features = FeaturesValidator(schema=self.schema) features.validate() -class Features(BaseValidator): +class FeaturesValidator(BaseValidator): def __init__(self, schema): self.schema = schema self.features: Optional[Dict[str, Dict]] = None @@ -51,7 +51,7 @@ def validate(self): for name, feature in self.features.items(): self.validate_feature(name, feature) - rules = FeatureRules(feature=feature, feature_name=name) + rules = RulesValidator(feature=feature, feature_name=name) rules.validate() @staticmethod @@ -64,7 +64,7 @@ def validate_feature(name, feature): raise ConfigurationError(f"'feature_default_value' boolean key must be present, feature_name={name}") -class FeatureRules(BaseValidator): +class RulesValidator(BaseValidator): def __init__(self, feature: Dict[str, Any], feature_name: str): self.feature = feature self.feature_name = feature_name @@ -80,7 +80,7 @@ def validate(self): for rule in self.rules: self.validate_rule(rule, self.feature) - conditions = FeatureRuleConditions(rule=rule, rule_name=rule.get(RULE_NAME_KEY)) + conditions = ConditionsValidator(rule=rule, rule_name=rule.get(RULE_NAME_KEY)) conditions.validate() def validate_rule(self, rule, feature_name): @@ -106,7 +106,7 @@ def validate_rule_default_value(rule): raise ConfigurationError(f"'rule_default_value' key must have be bool, rule_name={rule_name}") -class FeatureRuleConditions(BaseValidator): +class ConditionsValidator(BaseValidator): def __init__(self, rule: Dict[str, Any], rule_name: str): self.conditions = rule.get(CONDITIONS_KEY, {}) self.rule_name = rule_name diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index de24fa3e1c9..291966b2c5e 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -14,7 +14,7 @@ RULE_DEFAULT_VALUE, RULE_NAME_KEY, RULES_KEY, - FeatureRules, + RulesValidator, SchemaValidator, ) @@ -327,5 +327,5 @@ def test_validate_rule_invalid_rule_name(): # WHEN calling _validate_rule # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="'rule_name' key must be present*"): - rules_validator = FeatureRules(feature=feature, feature_name="my_feature") + rules_validator = RulesValidator(feature=feature, feature_name="my_feature") rules_validator.validate_rule_name(rule=rule, feature_name="my_feature") From 3b5066422c81cc7f1e8ddb3bf552230b0d6d879a Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 20:47:22 +0200 Subject: [PATCH 25/45] refactor(schema-validator): rename Action to RuleAction --- .../utilities/feature_flags/__init__.py | 4 +-- .../utilities/feature_flags/feature_flags.py | 8 ++--- .../utilities/feature_flags/schema.py | 4 +-- .../feature_toggles/test_feature_toggles.py | 36 +++++++++---------- .../feature_toggles/test_schema_validation.py | 18 +++++----- 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/__init__.py b/aws_lambda_powertools/utilities/feature_flags/__init__.py index 2d048a6600b..96e3ccee0e1 100644 --- a/aws_lambda_powertools/utilities/feature_flags/__init__.py +++ b/aws_lambda_powertools/utilities/feature_flags/__init__.py @@ -4,12 +4,12 @@ from .base import StoreProvider from .exceptions import ConfigurationError from .feature_flags import FeatureFlags -from .schema import ACTION, SchemaValidator +from .schema import RuleAction, SchemaValidator __all__ = [ "ConfigurationError", "FeatureFlags", - "ACTION", + "RuleAction", "SchemaValidator", "AppConfigStore", "StoreProvider", diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index ec56f8b9616..861f23025fa 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -24,10 +24,10 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any if not context_value: return False mapping_by_action = { - schema.ACTION.EQUALS.value: lambda a, b: a == b, - schema.ACTION.STARTSWITH.value: lambda a, b: a.startswith(b), - schema.ACTION.ENDSWITH.value: lambda a, b: a.endswith(b), - schema.ACTION.CONTAINS.value: lambda a, b: a in b, + schema.RuleAction.EQUALS.value: lambda a, b: a == b, + schema.RuleAction.STARTSWITH.value: lambda a, b: a.startswith(b), + schema.RuleAction.ENDSWITH.value: lambda a, b: a.endswith(b), + schema.RuleAction.CONTAINS.value: lambda a, b: a in b, } try: diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index af72bc415e4..5887c61c1e1 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -18,7 +18,7 @@ CONDITION_ACTION = "action" -class ACTION(str, Enum): +class RuleAction(str, Enum): EQUALS = "EQUALS" STARTSWITH = "STARTSWITH" ENDSWITH = "ENDSWITH" @@ -129,7 +129,7 @@ def _validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None @staticmethod def _validate_condition_action(condition: Dict[str, Any], rule_name: str): action = condition.get(CONDITION_ACTION, "") - if action not in ACTION.__members__: + if action not in RuleAction.__members__: raise ConfigurationError(f"invalid action value, rule_name={rule_name}, action={action}") @staticmethod diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index dc137172f11..295e7623dac 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -6,7 +6,7 @@ from aws_lambda_powertools.utilities.feature_flags import ConfigurationError, schema from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags -from aws_lambda_powertools.utilities.feature_flags.schema import ACTION +from aws_lambda_powertools.utilities.feature_flags.schema import RuleAction from aws_lambda_powertools.utilities.parameters import GetParameterError @@ -56,7 +56,7 @@ def test_toggles_rule_does_not_match(mocker, config): "value_when_applies": False, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "345345435", } @@ -106,7 +106,7 @@ def test_toggles_conditions_no_match(mocker, config): "value_when_applies": False, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "345345435", } @@ -136,12 +136,12 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) "value_when_applies": expected_value, "conditions": [ { - "action": ACTION.EQUALS.value, # this rule will match, it has multiple conditions + "action": RuleAction.EQUALS.value, # this rule will match, it has multiple conditions "key": "tenant_id", "value": tenant_id_val, }, { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "username", "value": username_val, }, @@ -178,12 +178,12 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf "value_when_applies": False, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "645654", }, { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "username", "value": "a", }, @@ -214,12 +214,12 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ "value_when_applies": expected_value_first_check, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "6", }, { - "action": ACTION.STARTSWITH.value, + "action": RuleAction.STARTSWITH.value, "key": "username", "value": "a", }, @@ -230,17 +230,17 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ "value_when_applies": expected_value_second_check, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "4446", }, { - "action": ACTION.STARTSWITH.value, + "action": RuleAction.STARTSWITH.value, "key": "username", "value": "a", }, { - "action": ACTION.ENDSWITH.value, + "action": RuleAction.ENDSWITH.value, "key": "username", "value": "z", }, @@ -283,7 +283,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): "value_when_applies": expected_value, "conditions": [ { - "action": ACTION.CONTAINS.value, + "action": RuleAction.CONTAINS.value, "key": "tenant_id", "value": ["6", "2"], } @@ -310,7 +310,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): "value_when_applies": True, "conditions": [ { - "action": ACTION.CONTAINS.value, + "action": RuleAction.CONTAINS.value, "key": "tenant_id", "value": ["8", "2"], } @@ -337,7 +337,7 @@ def test_multiple_features_enabled(mocker, config): "value_when_applies": True, "conditions": [ { - "action": ACTION.CONTAINS.value, + "action": RuleAction.CONTAINS.value, "key": "tenant_id", "value": ["6", "2"], } @@ -370,7 +370,7 @@ def test_multiple_features_only_some_enabled(mocker, config): "value_when_applies": True, "conditions": [ { - "action": ACTION.CONTAINS.value, + "action": RuleAction.CONTAINS.value, "key": "tenant_id", "value": ["6", "2"], } @@ -392,7 +392,7 @@ def test_multiple_features_only_some_enabled(mocker, config): "value_when_applies": False, "conditions": [ { - "action": ACTION.EQUALS.value, + "action": RuleAction.EQUALS.value, "key": "tenant_id", "value": "7", } @@ -456,7 +456,7 @@ 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) + result = conf_store._match_by_action(RuleAction.STARTSWITH.value, 1, 100) # THEN swallow the AttributeError and return False assert result is False diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 291966b2c5e..41d0b5d3bdb 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -4,7 +4,6 @@ from aws_lambda_powertools.utilities.feature_flags.exceptions import ConfigurationError from aws_lambda_powertools.utilities.feature_flags.schema import ( - ACTION, CONDITION_ACTION, CONDITION_KEY, CONDITION_VALUE, @@ -14,6 +13,7 @@ RULE_DEFAULT_VALUE, RULE_NAME_KEY, RULES_KEY, + RuleAction, RulesValidator, SchemaValidator, ) @@ -195,7 +195,7 @@ def test_invalid_condition(): { RULE_NAME_KEY: "tenant id equals 345345435", RULE_DEFAULT_VALUE: False, - CONDITIONS_KEY: {CONDITION_ACTION: ACTION.EQUALS.value}, + CONDITIONS_KEY: {CONDITION_ACTION: RuleAction.EQUALS.value}, }, ], } @@ -214,7 +214,7 @@ def test_invalid_condition(): RULE_NAME_KEY: "tenant id equals 345345435", RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: { - CONDITION_ACTION: ACTION.EQUALS.value, + CONDITION_ACTION: RuleAction.EQUALS.value, CONDITION_KEY: 5, CONDITION_VALUE: "a", }, @@ -238,22 +238,22 @@ def test_valid_condition_all_actions(): RULE_DEFAULT_VALUE: True, CONDITIONS_KEY: [ { - CONDITION_ACTION: ACTION.EQUALS.value, + CONDITION_ACTION: RuleAction.EQUALS.value, CONDITION_KEY: "tenant_id", CONDITION_VALUE: "645654", }, { - CONDITION_ACTION: ACTION.STARTSWITH.value, + CONDITION_ACTION: RuleAction.STARTSWITH.value, CONDITION_KEY: "username", CONDITION_VALUE: "a", }, { - CONDITION_ACTION: ACTION.ENDSWITH.value, + CONDITION_ACTION: RuleAction.ENDSWITH.value, CONDITION_KEY: "username", CONDITION_VALUE: "a", }, { - CONDITION_ACTION: ACTION.CONTAINS.value, + CONDITION_ACTION: RuleAction.CONTAINS.value, CONDITION_KEY: "username", CONDITION_VALUE: ["a", "b"], }, @@ -296,7 +296,7 @@ def test_validate_condition_invalid_condition_action(): def test_validate_condition_invalid_condition_key(): # GIVEN a configuration with a missing "key" validator = SchemaValidator(EMPTY_SCHEMA) - condition = {"action": ACTION.EQUALS.value} + condition = {"action": RuleAction.EQUALS.value} # WHEN calling _validate_condition # THEN raise ConfigurationError @@ -307,7 +307,7 @@ def test_validate_condition_invalid_condition_key(): def test_validate_condition_missing_condition_value(): # GIVEN a configuration with a missing condition value validator = SchemaValidator(EMPTY_SCHEMA) - condition = {"action": ACTION.EQUALS.value, "key": "Foo"} + condition = {"action": RuleAction.EQUALS.value, "key": "Foo"} # WHEN calling _validate_condition with pytest.raises(ConfigurationError, match="Missing condition value"): From d22226f7137d07aca04f437cb12d8f01a261bd39 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 2 Aug 2021 21:04:23 +0200 Subject: [PATCH 26/45] refactor(tests): use public methods --- .../utilities/feature_flags/schema.py | 20 +++---- .../feature_toggles/test_schema_validation.py | 60 ++++++++++++------- 2 files changed, 48 insertions(+), 32 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 5887c61c1e1..cc7216d8dde 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -116,30 +116,30 @@ def validate(self): raise ConfigurationError(f"Invalid condition, rule_name={self.rule_name}") for condition in self.conditions: - self._validate_condition(rule_name=self.rule_name, condition=condition) + self.validate_condition(rule_name=self.rule_name, condition=condition) - def _validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: + def validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): - raise ConfigurationError(f"invalid condition type, not a dictionary, rule_name={rule_name}") + raise ConfigurationError(f"Invalid condition type, not a dictionary, rule_name={rule_name}") - self._validate_condition_action(condition=condition, rule_name=rule_name) - self._validate_condition_key(condition=condition, rule_name=rule_name) - self._validate_condition_value(condition=condition, rule_name=rule_name) + self.validate_condition_action(condition=condition, rule_name=rule_name) + self.validate_condition_key(condition=condition, rule_name=rule_name) + self.validate_condition_value(condition=condition, rule_name=rule_name) @staticmethod - def _validate_condition_action(condition: Dict[str, Any], rule_name: str): + def validate_condition_action(condition: Dict[str, Any], rule_name: str): action = condition.get(CONDITION_ACTION, "") if action not in RuleAction.__members__: - raise ConfigurationError(f"invalid action value, rule_name={rule_name}, action={action}") + raise ConfigurationError(f"Invalid action value, rule_name={rule_name}, action={action}") @staticmethod - def _validate_condition_key(condition: Dict[str, Any], rule_name: str): + def validate_condition_key(condition: Dict[str, Any], rule_name: str): key = condition.get(CONDITION_KEY, "") if not key or not isinstance(key, str): raise ConfigurationError(f"Invalid key value, key has to be a non empty string, rule_name={rule_name}") @staticmethod - def _validate_condition_value(condition: Dict[str, Any], rule_name: str): + def validate_condition_value(condition: Dict[str, Any], rule_name: str): value = condition.get(CONDITION_VALUE, "") if not value: raise ConfigurationError(f"Missing condition value, rule_name={rule_name}") diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 41d0b5d3bdb..8329dd68987 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -13,6 +13,7 @@ RULE_DEFAULT_VALUE, RULE_NAME_KEY, RULES_KEY, + ConditionsValidator, RuleAction, RulesValidator, SchemaValidator, @@ -269,49 +270,64 @@ def test_valid_condition_all_actions(): def test_validate_condition_invalid_condition_type(): # GIVEN an invalid condition type of empty dict - validator = SchemaValidator(EMPTY_SCHEMA) - condition = {} - - # WHEN calling _validate_condition - with pytest.raises(ConfigurationError) as err: - validator._validate_condition("foo", condition) + rule_conditions = {"conditions": {}} + condition = rule_conditions[CONDITIONS_KEY] + rule_name = "dummy" + # WHEN calling validate_condition # THEN raise ConfigurationError - assert "invalid condition type" in str(err) + with pytest.raises(ConfigurationError, match="Invalid condition type"): + validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) + validator.validate_condition(condition=condition, rule_name=rule_name) def test_validate_condition_invalid_condition_action(): # GIVEN an invalid condition action of foo - validator = SchemaValidator(EMPTY_SCHEMA) - condition = {"action": "foo"} - - # WHEN calling _validate_condition - with pytest.raises(ConfigurationError) as err: - validator._validate_condition("foo", condition) + rule_conditions = { + "conditions": [{"action": "INVALID", "key": "tenant_id", "value": "12345"}], + } + condition = rule_conditions[CONDITIONS_KEY][0] + rule_name = "dummy" + # WHEN calling validate_condition # THEN raise ConfigurationError - assert "invalid action value" in str(err) + with pytest.raises(ConfigurationError, match="Invalid action value"): + validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) + validator.validate_condition_action(condition=condition, rule_name=rule_name) def test_validate_condition_invalid_condition_key(): # GIVEN a configuration with a missing "key" - validator = SchemaValidator(EMPTY_SCHEMA) - condition = {"action": RuleAction.EQUALS.value} + rule_conditions = { + "conditions": [{"action": RuleAction.EQUALS.value, "value": "12345"}], + } + condition = rule_conditions[CONDITIONS_KEY][0] + rule_name = "dummy" - # WHEN calling _validate_condition + # WHEN calling validate_condition # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="Invalid key value"): - validator._validate_condition("foo", condition) + validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) + validator.validate_condition_key(condition=condition, rule_name=rule_name) def test_validate_condition_missing_condition_value(): # GIVEN a configuration with a missing condition value - validator = SchemaValidator(EMPTY_SCHEMA) - condition = {"action": RuleAction.EQUALS.value, "key": "Foo"} + rule_conditions = { + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + } + ], + } + condition = rule_conditions[CONDITIONS_KEY][0] + rule_name = "dummy" - # WHEN calling _validate_condition + # WHEN calling validate_condition with pytest.raises(ConfigurationError, match="Missing condition value"): - validator._validate_condition("foo", condition) + validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) + validator.validate_condition_value(condition=condition, rule_name=rule_name) def test_validate_rule_invalid_rule_name(): From cd561a19aeedcaacf88fbcc6cfb836dbcf237563 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Mon, 2 Aug 2021 16:21:22 -0700 Subject: [PATCH 27/45] From dd2c1f866ae85967ea3b344798ed76be50c16ff4 Mon Sep 17 00:00:00 2001 From: Michael Brewer Date: Mon, 2 Aug 2021 16:22:30 -0700 Subject: [PATCH 28/45] test(feature-flags): review issues with pr --- .../utilities/feature_flags/feature_flags.py | 4 ++-- .../utilities/feature_flags/schema.py | 13 +++++------- .../feature_toggles/test_feature_toggles.py | 8 +++---- .../feature_toggles/test_schema_validation.py | 21 +++++++++++++++---- 4 files changed, 28 insertions(+), 18 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 861f23025fa..3d8100bd366 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -57,7 +57,7 @@ def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], context: Dic return False # if we got here, all conditions match logger.debug( - f"rule matched, rule_name={rule_name}, rule_default_value={rule_default_value}, " f"name={feature_name}" + f"rule matched, rule_name={rule_name}, rule_default_value={rule_default_value}, name={feature_name}" ) return True return False @@ -160,7 +160,7 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau feature = features.get(name) if feature is None: logger.debug( - f"feature does not appear in configuration, using provided default, " f"name={name}, default={default}" + f"feature does not appear in configuration, using provided default, name={name}, default={default}" ) return default diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index cc7216d8dde..09a54b11dda 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -38,12 +38,9 @@ def validate(self) -> None: class FeaturesValidator(BaseValidator): - def __init__(self, schema): + def __init__(self, schema: Dict): self.schema = schema - self.features: Optional[Dict[str, Dict]] = None - - if isinstance(self.schema, dict): - self.features = self.schema.get(FEATURES_KEY) + self.features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) def validate(self): if not isinstance(self.features, dict): @@ -71,13 +68,13 @@ def __init__(self, feature: Dict[str, Any], feature_name: str): self.rules: Optional[List[Dict]] = self.feature.get(RULES_KEY) def validate(self): - if not isinstance(self.rules, list): - raise ConfigurationError(f"Feature rules is not a list, feature_name={self.feature_name}") - if not self.rules: logger.debug("Rules are empty, ignoring validation") return + if not isinstance(self.rules, list): + raise ConfigurationError(f"Feature rules is not a list, feature_name={self.feature_name}") + for rule in self.rules: self.validate_rule(rule, self.feature) conditions = ConditionsValidator(rule=rule, rule_name=rule.get(RULE_NAME_KEY)) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 295e7623dac..88cf455d0db 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -279,7 +279,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): "feature_default_value": False, "rules": [ { - "rule_name": "tenant id is contained in [6,2] ", + "rule_name": "tenant id is contained in [6, 2]", "value_when_applies": expected_value, "conditions": [ { @@ -306,7 +306,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): "feature_default_value": expected_value, "rules": [ { - "rule_name": "tenant id is contained in [6,2] ", + "rule_name": "tenant id is contained in [8, 2]", "value_when_applies": True, "conditions": [ { @@ -333,7 +333,7 @@ def test_multiple_features_enabled(mocker, config): "feature_default_value": False, "rules": [ { - "rule_name": "tenant id is contained in [6,2] ", + "rule_name": "tenant id is contained in [6, 2]", "value_when_applies": True, "conditions": [ { @@ -366,7 +366,7 @@ def test_multiple_features_only_some_enabled(mocker, config): "feature_default_value": False, "rules": [ { - "rule_name": "tenant id is contained in [6,2] ", + "rule_name": "tenant id is contained in [6, 2]", "value_when_applies": True, "conditions": [ { diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 8329dd68987..6495a00e727 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -33,11 +33,13 @@ def test_invalid_features_dict(): schema = [] # invalid type + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() # invalid features key schema = {FEATURES_KEY: []} + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -57,33 +59,38 @@ def test_invalid_feature_dict(): # empty feature dict schema = {FEATURES_KEY: {"my_feature": {}}} + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}} + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean #2 schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: 5}}} + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() # invalid rules type, not list schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}} + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() def test_valid_feature_dict(): - # no rules list at all - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}}} + # empty rules list + schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}}} validator = SchemaValidator(schema) validator.validate() - # empty rules list - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}}} + # no rules list at all + schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}}} + validator = SchemaValidator(schema) validator.validate() @@ -118,6 +125,7 @@ def test_invalid_rule(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -135,6 +143,7 @@ def test_invalid_rule(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -149,6 +158,7 @@ def test_invalid_rule(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -163,6 +173,7 @@ def test_invalid_rule(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -202,6 +213,7 @@ def test_invalid_condition(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -224,6 +236,7 @@ def test_invalid_condition(): } } } + validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() From b500acb70f244e15e9b8aaef3dfbaf84d90309bf Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 07:12:21 +0200 Subject: [PATCH 29/45] refactor(tests): use public static methods --- .../utilities/feature_flags/schema.py | 9 ++-- .../feature_toggles/test_schema_validation.py | 51 +++++-------------- 2 files changed, 17 insertions(+), 43 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 09a54b11dda..0ec8335b989 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -115,13 +115,14 @@ def validate(self): for condition in self.conditions: self.validate_condition(rule_name=self.rule_name, condition=condition) - def validate_condition(self, rule_name: str, condition: Dict[str, str]) -> None: + @staticmethod + def validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): raise ConfigurationError(f"Invalid condition type, not a dictionary, rule_name={rule_name}") - self.validate_condition_action(condition=condition, rule_name=rule_name) - self.validate_condition_key(condition=condition, rule_name=rule_name) - self.validate_condition_value(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_action(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_key(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @staticmethod def validate_condition_action(condition: Dict[str, Any], rule_name: str): diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 6495a00e727..42f253abfab 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -283,78 +283,51 @@ def test_valid_condition_all_actions(): def test_validate_condition_invalid_condition_type(): # GIVEN an invalid condition type of empty dict - rule_conditions = {"conditions": {}} - condition = rule_conditions[CONDITIONS_KEY] - rule_name = "dummy" + condition = {} # WHEN calling validate_condition # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="Invalid condition type"): - validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) - validator.validate_condition(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition(condition=condition, rule_name="dummy") def test_validate_condition_invalid_condition_action(): # GIVEN an invalid condition action of foo - rule_conditions = { - "conditions": [{"action": "INVALID", "key": "tenant_id", "value": "12345"}], - } - condition = rule_conditions[CONDITIONS_KEY][0] - rule_name = "dummy" + condition = {"action": "INVALID", "key": "tenant_id", "value": "12345"} # WHEN calling validate_condition # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="Invalid action value"): - validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) - validator.validate_condition_action(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_action(condition=condition, rule_name="dummy") def test_validate_condition_invalid_condition_key(): # GIVEN a configuration with a missing "key" - rule_conditions = { - "conditions": [{"action": RuleAction.EQUALS.value, "value": "12345"}], - } - condition = rule_conditions[CONDITIONS_KEY][0] - rule_name = "dummy" + condition = {"action": RuleAction.EQUALS.value, "value": "12345"} # WHEN calling validate_condition # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="Invalid key value"): - validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) - validator.validate_condition_key(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_key(condition=condition, rule_name="dummy") def test_validate_condition_missing_condition_value(): # GIVEN a configuration with a missing condition value - rule_conditions = { - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - } - ], + condition = { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", } - condition = rule_conditions[CONDITIONS_KEY][0] - rule_name = "dummy" # WHEN calling validate_condition with pytest.raises(ConfigurationError, match="Missing condition value"): - validator = ConditionsValidator(rule=rule_conditions, rule_name=rule_name) - validator.validate_condition_value(condition=condition, rule_name=rule_name) + ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy") def test_validate_rule_invalid_rule_name(): # GIVEN a rule_name not in the rule dict - feature = { - "feature_default_value": True, - "rules": [ - {"invalid_rule_name": "tenant id equals 345345435"}, - ], - } - rule = feature[RULES_KEY][0] + rule = {"invalid_rule_name": "tenant id equals 345345435"} # WHEN calling _validate_rule # THEN raise ConfigurationError with pytest.raises(ConfigurationError, match="'rule_name' key must be present*"): - rules_validator = RulesValidator(feature=feature, feature_name="my_feature") - rules_validator.validate_rule_name(rule=rule, feature_name="my_feature") + RulesValidator.validate_rule_name(rule=rule, feature_name="dummy") From 327a4aeb63dd8f4ea2a7158ee59d0413860829c5 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 07:17:37 +0200 Subject: [PATCH 30/45] refactor(schema): rename feature_default_value to default --- .../utilities/feature_flags/schema.py | 2 +- .../feature_toggles/test_feature_toggles.py | 32 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 0ec8335b989..fd3aec7ff99 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -9,7 +9,7 @@ FEATURES_KEY = "features" RULES_KEY = "rules" -FEATURE_DEFAULT_VAL_KEY = "feature_default_value" +FEATURE_DEFAULT_VAL_KEY = "default" CONDITIONS_KEY = "conditions" RULE_NAME_KEY = "rule_name" RULE_DEFAULT_VALUE = "value_when_applies" diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 88cf455d0db..52d72b0c3d5 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -49,7 +49,7 @@ def test_toggles_rule_does_not_match(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": expected_value, + "default": expected_value, "rules": [ { "rule_name": "tenant id equals 345345435", @@ -76,7 +76,7 @@ def test_toggles_rule_does_not_match(mocker, config): # you get the default value of False that was sent to the evaluate API def test_toggles_no_conditions_feature_does_not_exist(mocker, config): expected_value = False - mocked_app_config_schema = {"features": {"my_fake_feature": {"feature_default_value": True}}} + mocked_app_config_schema = {"features": {"my_fake_feature": {"default": True}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate(name="my_feature", context={}, default=expected_value) @@ -87,7 +87,7 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): # default value is False but the feature has a True default_value. def test_toggles_no_rules(mocker, config): expected_value = True - mocked_app_config_schema = {"features": {"my_feature": {"feature_default_value": expected_value}}} + mocked_app_config_schema = {"features": {"my_feature": {"default": expected_value}}} conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -99,7 +99,7 @@ def test_toggles_conditions_no_match(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": expected_value, + "default": expected_value, "rules": [ { "rule_name": "tenant id equals 345345435", @@ -129,7 +129,7 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": True, + "default": True, "rules": [ { "rule_name": "tenant id equals 6 and username is a", @@ -171,7 +171,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": expected_val, + "default": expected_val, "rules": [ { "rule_name": "tenant id equals 645654 and username is a", # rule will not match @@ -207,7 +207,7 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": expected_value_third_check, + "default": expected_value_third_check, "rules": [ { "rule_name": "tenant id equals 6 and username startswith a", @@ -276,7 +276,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": False, + "default": False, "rules": [ { "rule_name": "tenant id is contained in [6, 2]", @@ -303,7 +303,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": expected_value, + "default": expected_value, "rules": [ { "rule_name": "tenant id is contained in [8, 2]", @@ -330,7 +330,7 @@ def test_multiple_features_enabled(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { - "feature_default_value": False, + "default": False, "rules": [ { "rule_name": "tenant id is contained in [6, 2]", @@ -346,10 +346,10 @@ def test_multiple_features_enabled(mocker, config): ], }, "my_feature2": { - "feature_default_value": True, + "default": True, }, "my_feature3": { - "feature_default_value": False, + "default": False, }, }, } @@ -363,7 +363,7 @@ def test_multiple_features_only_some_enabled(mocker, config): mocked_app_config_schema = { "features": { "my_feature": { # rule will match here, feature is enabled due to rule match - "feature_default_value": False, + "default": False, "rules": [ { "rule_name": "tenant id is contained in [6, 2]", @@ -379,13 +379,13 @@ def test_multiple_features_only_some_enabled(mocker, config): ], }, "my_feature2": { - "feature_default_value": True, + "default": True, }, "my_feature3": { - "feature_default_value": False, + "default": False, }, "my_feature4": { # rule will not match here, feature is enabled by default - "feature_default_value": True, + "default": True, "rules": [ { "rule_name": "tenant id equals 7", From 4bfe2a87a7464955ae1562103ff523a2b708acd2 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 07:19:07 +0200 Subject: [PATCH 31/45] refactor(schema): rename value_when_applies to when_match --- .../utilities/feature_flags/schema.py | 2 +- .../feature_toggles/test_feature_toggles.py | 22 +++++++++---------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index fd3aec7ff99..aabfd8c2b7f 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -12,7 +12,7 @@ FEATURE_DEFAULT_VAL_KEY = "default" CONDITIONS_KEY = "conditions" RULE_NAME_KEY = "rule_name" -RULE_DEFAULT_VALUE = "value_when_applies" +RULE_DEFAULT_VALUE = "when_match" CONDITION_KEY = "key" CONDITION_VALUE = "value" CONDITION_ACTION = "action" diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 52d72b0c3d5..78455005146 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -53,7 +53,7 @@ def test_toggles_rule_does_not_match(mocker, config): "rules": [ { "rule_name": "tenant id equals 345345435", - "value_when_applies": False, + "when_match": False, "conditions": [ { "action": RuleAction.EQUALS.value, @@ -103,7 +103,7 @@ def test_toggles_conditions_no_match(mocker, config): "rules": [ { "rule_name": "tenant id equals 345345435", - "value_when_applies": False, + "when_match": False, "conditions": [ { "action": RuleAction.EQUALS.value, @@ -133,7 +133,7 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) "rules": [ { "rule_name": "tenant id equals 6 and username is a", - "value_when_applies": expected_value, + "when_match": expected_value, "conditions": [ { "action": RuleAction.EQUALS.value, # this rule will match, it has multiple conditions @@ -175,7 +175,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf "rules": [ { "rule_name": "tenant id equals 645654 and username is a", # rule will not match - "value_when_applies": False, + "when_match": False, "conditions": [ { "action": RuleAction.EQUALS.value, @@ -211,7 +211,7 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ "rules": [ { "rule_name": "tenant id equals 6 and username startswith a", - "value_when_applies": expected_value_first_check, + "when_match": expected_value_first_check, "conditions": [ { "action": RuleAction.EQUALS.value, @@ -227,7 +227,7 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ }, { "rule_name": "tenant id equals 4446 and username startswith a and endswith z", - "value_when_applies": expected_value_second_check, + "when_match": expected_value_second_check, "conditions": [ { "action": RuleAction.EQUALS.value, @@ -280,7 +280,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): "rules": [ { "rule_name": "tenant id is contained in [6, 2]", - "value_when_applies": expected_value, + "when_match": expected_value, "conditions": [ { "action": RuleAction.CONTAINS.value, @@ -307,7 +307,7 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): "rules": [ { "rule_name": "tenant id is contained in [8, 2]", - "value_when_applies": True, + "when_match": True, "conditions": [ { "action": RuleAction.CONTAINS.value, @@ -334,7 +334,7 @@ def test_multiple_features_enabled(mocker, config): "rules": [ { "rule_name": "tenant id is contained in [6, 2]", - "value_when_applies": True, + "when_match": True, "conditions": [ { "action": RuleAction.CONTAINS.value, @@ -367,7 +367,7 @@ def test_multiple_features_only_some_enabled(mocker, config): "rules": [ { "rule_name": "tenant id is contained in [6, 2]", - "value_when_applies": True, + "when_match": True, "conditions": [ { "action": RuleAction.CONTAINS.value, @@ -389,7 +389,7 @@ def test_multiple_features_only_some_enabled(mocker, config): "rules": [ { "rule_name": "tenant id equals 7", - "value_when_applies": False, + "when_match": False, "conditions": [ { "action": RuleAction.EQUALS.value, From 4aba5070d97e027b92f60b9bf978f633e9f0184b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 08:46:22 +0200 Subject: [PATCH 32/45] refactor(schema): use new rules dict over list --- .../utilities/feature_flags/feature_flags.py | 17 +-- .../utilities/feature_flags/schema.py | 36 +++---- .../feature_toggles/test_feature_toggles.py | 102 ++++++++---------- .../feature_toggles/test_schema_validation.py | 38 ++++--- 4 files changed, 98 insertions(+), 95 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 3d8100bd366..b9a59f9f9f6 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -37,8 +37,9 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any self._logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") return False - def _is_rule_matched(self, feature_name: str, rule: Dict[str, Any], context: Dict[str, Any]) -> bool: - rule_name = rule.get(schema.RULE_NAME_KEY, "") + def _is_rule_matched( + self, rule_name: str, feature_name: str, rule: Dict[str, Any], context: Dict[str, Any] + ) -> bool: rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) conditions = cast(List[Dict], rule.get(schema.CONDITIONS_KEY)) @@ -68,11 +69,11 @@ def _handle_rules( feature_name: str, context: Dict[str, Any], feature_default_value: bool, - rules: List[Dict[str, Any]], + rules: Dict[str, Any], ) -> bool: - for rule in rules: + for rule_name, rule in rules.items(): rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) - if self._is_rule_matched(feature_name, rule, context): + if self._is_rule_matched(rule_name=rule_name, feature_name=feature_name, rule=rule, context=context): return bool(rule_default_value) # no rule matched, return default value of feature logger.debug( @@ -211,16 +212,16 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L return features_enabled for feature_name, feature_dict_def in features.items(): - rules_list = feature_dict_def.get(schema.RULES_KEY, []) + rules = feature_dict_def.get(schema.RULES_KEY, {}) feature_default_value = feature_dict_def.get(schema.FEATURE_DEFAULT_VAL_KEY) - if feature_default_value and not rules_list: + if feature_default_value and not rules: self._logger.debug(f"feature is enabled by default and has no defined rules, name={feature_name}") features_enabled.append(feature_name) elif self._handle_rules( feature_name=feature_name, context=context, feature_default_value=feature_default_value, - rules=rules_list, + rules=rules, ): self._logger.debug(f"feature's calculated value is True, name={feature_name}") features_enabled.append(feature_name) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index aabfd8c2b7f..6eb9ea022b6 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -48,7 +48,7 @@ def validate(self): for name, feature in self.features.items(): self.validate_feature(name, feature) - rules = RulesValidator(feature=feature, feature_name=name) + rules = RulesValidator(feature=feature) rules.validate() @staticmethod @@ -62,50 +62,48 @@ def validate_feature(name, feature): class RulesValidator(BaseValidator): - def __init__(self, feature: Dict[str, Any], feature_name: str): + def __init__(self, feature: Dict[str, Any]): self.feature = feature - self.feature_name = feature_name - self.rules: Optional[List[Dict]] = self.feature.get(RULES_KEY) + self.feature_name = next(iter(self.feature)) + self.rules: Optional[Dict] = self.feature.get(RULES_KEY) def validate(self): if not self.rules: logger.debug("Rules are empty, ignoring validation") return - if not isinstance(self.rules, list): - raise ConfigurationError(f"Feature rules is not a list, feature_name={self.feature_name}") + if not isinstance(self.rules, dict): + raise ConfigurationError(f"Feature rules must be a dictionary, feature_name={self.feature_name}") - for rule in self.rules: - self.validate_rule(rule, self.feature) - conditions = ConditionsValidator(rule=rule, rule_name=rule.get(RULE_NAME_KEY)) + for rule_name, rule in self.rules.items(): + self.validate_rule(rule=rule, rule_name=rule_name, feature_name=self.feature_name) + conditions = ConditionsValidator(rule=rule, rule_name=rule_name) conditions.validate() - def validate_rule(self, rule, feature_name): + def validate_rule(self, rule, rule_name, feature_name): if not rule or not isinstance(rule, dict): raise ConfigurationError(f"Feature rule must be a dictionary, feature_name={feature_name}") - self.validate_rule_name(rule, feature_name) - self.validate_rule_default_value(rule) + self.validate_rule_name(rule_name=rule_name, feature_name=feature_name) + self.validate_rule_default_value(rule=rule, rule_name=rule_name) @staticmethod - def validate_rule_name(rule, feature_name): - rule_name = rule.get(RULE_NAME_KEY) - if not rule_name or rule_name is None or not isinstance(rule_name, str): + def validate_rule_name(rule_name: str, feature_name: str): + if not rule_name or not isinstance(rule_name, str): raise ConfigurationError( f"'rule_name' key must be present and have a non-empty string, feature_name={feature_name}" ) @staticmethod - def validate_rule_default_value(rule): - rule_name = rule.get(RULE_NAME_KEY) + def validate_rule_default_value(rule: Dict, rule_name: str): rule_default_value = rule.get(RULE_DEFAULT_VALUE) - if rule_default_value is None or not isinstance(rule_default_value, bool): + if not isinstance(rule_default_value, bool): raise ConfigurationError(f"'rule_default_value' key must have be bool, rule_name={rule_name}") class ConditionsValidator(BaseValidator): def __init__(self, rule: Dict[str, Any], rule_name: str): - self.conditions = rule.get(CONDITIONS_KEY, {}) + self.conditions: List[Dict[str, Any]] = rule.get(CONDITIONS_KEY, {}) self.rule_name = rule_name def validate(self): diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 78455005146..08c53690028 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -50,9 +50,8 @@ def test_toggles_rule_does_not_match(mocker, config): "features": { "my_feature": { "default": expected_value, - "rules": [ - { - "rule_name": "tenant id equals 345345435", + "rules": { + "tenant id equals 345345435": { "when_match": False, "conditions": [ { @@ -61,10 +60,10 @@ def test_toggles_rule_does_not_match(mocker, config): "value": "345345435", } ], - }, - ], + } + }, } - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) @@ -100,9 +99,8 @@ def test_toggles_conditions_no_match(mocker, config): "features": { "my_feature": { "default": expected_value, - "rules": [ - { - "rule_name": "tenant id equals 345345435", + "rules": { + "tenant id equals 345345435": { "when_match": False, "conditions": [ { @@ -111,10 +109,10 @@ def test_toggles_conditions_no_match(mocker, config): "value": "345345435", } ], - }, - ], + } + }, } - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) @@ -130,9 +128,8 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) "features": { "my_feature": { "default": True, - "rules": [ - { - "rule_name": "tenant id equals 6 and username is a", + "rules": { + "tenant id equals 6 and username is a": { "when_match": expected_value, "conditions": [ { @@ -146,8 +143,8 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) "value": username_val, }, ], - }, - ], + } + }, } }, } @@ -172,9 +169,9 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf "features": { "my_feature": { "default": expected_val, - "rules": [ - { - "rule_name": "tenant id equals 645654 and username is a", # rule will not match + "rules": { + # rule will not match + "tenant id equals 645654 and username is a": { "when_match": False, "conditions": [ { @@ -188,10 +185,10 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf "value": "a", }, ], - }, - ], + } + }, } - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) @@ -208,9 +205,8 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ "features": { "my_feature": { "default": expected_value_third_check, - "rules": [ - { - "rule_name": "tenant id equals 6 and username startswith a", + "rules": { + "tenant id equals 6 and username startswith a": { "when_match": expected_value_first_check, "conditions": [ { @@ -225,8 +221,7 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ }, ], }, - { - "rule_name": "tenant id equals 4446 and username startswith a and endswith z", + "tenant id equals 4446 and username startswith a and endswith z": { "when_match": expected_value_second_check, "conditions": [ { @@ -246,9 +241,9 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ }, ], }, - ], + }, } - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) @@ -277,9 +272,8 @@ def test_toggles_match_rule_with_contains_action(mocker, config): "features": { "my_feature": { "default": False, - "rules": [ - { - "rule_name": "tenant id is contained in [6, 2]", + "rules": { + "tenant id is contained in [6, 2]": { "when_match": expected_value, "conditions": [ { @@ -288,10 +282,10 @@ def test_toggles_match_rule_with_contains_action(mocker, config): "value": ["6", "2"], } ], - }, - ], + } + }, } - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) @@ -331,9 +325,8 @@ def test_multiple_features_enabled(mocker, config): "features": { "my_feature": { "default": False, - "rules": [ - { - "rule_name": "tenant id is contained in [6, 2]", + "rules": { + "tenant id is contained in [6, 2]": { "when_match": True, "conditions": [ { @@ -342,8 +335,8 @@ def test_multiple_features_enabled(mocker, config): "value": ["6", "2"], } ], - }, - ], + } + }, }, "my_feature2": { "default": True, @@ -351,7 +344,7 @@ def test_multiple_features_enabled(mocker, config): "my_feature3": { "default": False, }, - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) @@ -364,9 +357,8 @@ def test_multiple_features_only_some_enabled(mocker, config): "features": { "my_feature": { # rule will match here, feature is enabled due to rule match "default": False, - "rules": [ - { - "rule_name": "tenant id is contained in [6, 2]", + "rules": { + "tenant id is contained in [6, 2]": { "when_match": True, "conditions": [ { @@ -375,8 +367,8 @@ def test_multiple_features_only_some_enabled(mocker, config): "value": ["6", "2"], } ], - }, - ], + } + }, }, "my_feature2": { "default": True, @@ -384,11 +376,11 @@ def test_multiple_features_only_some_enabled(mocker, config): "my_feature3": { "default": False, }, - "my_feature4": { # rule will not match here, feature is enabled by default + # rule will not match here, feature is enabled by default + "my_feature4": { "default": True, - "rules": [ - { - "rule_name": "tenant id equals 7", + "rules": { + "tenant id equals 7": { "when_match": False, "conditions": [ { @@ -397,10 +389,10 @@ def test_multiple_features_only_some_enabled(mocker, config): "value": "7", } ], - }, - ], + } + }, }, - }, + } } conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) @@ -468,7 +460,7 @@ def test_is_rule_matched_no_matches(mocker, config): conf_store = init_configuration_store(mocker, {}, config) # WHEN calling _is_rule_matched - result = conf_store._is_rule_matched("name", rule, rules_context) + result = conf_store._is_rule_matched(rule_name="dummy", feature_name="dummy", rule=rule, context=rules_context) # THEN return False assert result is False diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 42f253abfab..a8730e1ad7f 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -246,9 +246,8 @@ def test_valid_condition_all_actions(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 645654 and username is a", + RULES_KEY: { + "tenant id equals 645654 and username is a": { RULE_DEFAULT_VALUE: True, CONDITIONS_KEY: [ { @@ -272,10 +271,10 @@ def test_valid_condition_all_actions(): CONDITION_VALUE: ["a", "b"], }, ], - }, - ], + } + }, } - }, + } } validator = SchemaValidator(schema) validator.validate() @@ -323,11 +322,24 @@ def test_validate_condition_missing_condition_value(): ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy") -def test_validate_rule_invalid_rule_name(): - # GIVEN a rule_name not in the rule dict - rule = {"invalid_rule_name": "tenant id equals 345345435"} +def test_new_rule_format(): + new_features = { + "my_feature": { + "default": True, + "rules": { + "tenant id equals 345345435": { + "when_match": False, + "conditions": [ + { + "action": "EQUALS", + "key": "tenant_id", + "value": "345345435", + } + ], + }, + }, + } + } - # WHEN calling _validate_rule - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="'rule_name' key must be present*"): - RulesValidator.validate_rule_name(rule=rule, feature_name="dummy") + rules = RulesValidator(feature=new_features) + rules.validate() From 387da38937a97c5bd9af5f8fa79d3b99c2568e6d Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 08:52:53 +0200 Subject: [PATCH 33/45] refactor(tests): conf_store to feature flags --- .../feature_toggles/test_feature_toggles.py | 78 ++++++++++--------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 08c53690028..a5f25b1d1fc 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -15,7 +15,7 @@ def config(): return Config(region_name="us-east-1") -def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> FeatureFlags: +def init_feature_flags(mocker, mock_schema: Dict, config: Config) -> FeatureFlags: mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") mocked_get_conf.return_value = mock_schema @@ -26,8 +26,8 @@ def init_configuration_store(mocker, mock_schema: Dict, config: Config) -> Featu cache_seconds=600, config=config, ) - conf_store: FeatureFlags = FeatureFlags(store=app_conf_fetcher) - return conf_store + feature_flags: FeatureFlags = FeatureFlags(store=app_conf_fetcher) + return feature_flags def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigStore: @@ -66,8 +66,8 @@ def test_toggles_rule_does_not_match(mocker, config): } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={}, default=False) assert toggle == expected_value @@ -77,8 +77,8 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): expected_value = False mocked_app_config_schema = {"features": {"my_fake_feature": {"default": True}}} - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={}, default=expected_value) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={}, default=expected_value) assert toggle == expected_value @@ -87,8 +87,8 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): def test_toggles_no_rules(mocker, config): expected_value = True mocked_app_config_schema = {"features": {"my_feature": {"default": expected_value}}} - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -114,8 +114,8 @@ def test_toggles_conditions_no_match(mocker, config): } } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -148,8 +148,8 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) } }, } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate( + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( name="my_feature", context={ "tenant_id": tenant_id_val, @@ -190,8 +190,8 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf } } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_val @@ -246,18 +246,20 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) # match first rule - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "abcd"}, default=False) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "abcd"}, default=False) assert toggle == expected_value_first_check # match second rule - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "4446", "username": "az"}, default=False) assert toggle == expected_value_second_check # match no rule - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, default=False) + toggle = feature_flags.evaluate( + name="my_feature", context={"tenant_id": "11114446", "username": "ab"}, default=False + ) assert toggle == expected_value_third_check # feature doesn't exist - toggle = conf_store.evaluate( + toggle = feature_flags.evaluate( name="my_fake_feature", context={"tenant_id": "11114446", "username": "ab"}, default=expected_value_fourth_case, @@ -287,8 +289,8 @@ def test_toggles_match_rule_with_contains_action(mocker, config): } } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -314,8 +316,8 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): } }, } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - toggle = conf_store.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -346,8 +348,8 @@ def test_multiple_features_enabled(mocker, config): }, } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + enabled_list: List[str] = feature_flags.get_enabled_features(context={"tenant_id": "6", "username": "a"}) assert enabled_list == expected_value @@ -394,18 +396,18 @@ def test_multiple_features_only_some_enabled(mocker, config): }, } } - conf_store = init_configuration_store(mocker, mocked_app_config_schema, config) - enabled_list: List[str] = conf_store.get_enabled_features(context={"tenant_id": "6", "username": "a"}) + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + enabled_list: List[str] = feature_flags.get_enabled_features(context={"tenant_id": "6", "username": "a"}) 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 = FeatureFlags(schema_fetcher) + feature_flags = FeatureFlags(schema_fetcher) # WHEN calling evaluate - toggle = conf_store.evaluate(name="Foo", default=False) + toggle = feature_flags.evaluate(name="Foo", default=False) # THEN handle the error and return the default assert toggle is False @@ -414,10 +416,10 @@ def test_get_feature_toggle_handles_error(mocker, config): 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 = FeatureFlags(schema_fetcher) + feature_flags = FeatureFlags(schema_fetcher) # WHEN calling get_enabled_features - toggles = conf_store.get_enabled_features(context=None) + toggles = feature_flags.get_enabled_features(context=None) # THEN handle the error and return an empty list assert toggles == [] @@ -437,18 +439,18 @@ def test_app_config_get_parameter_err(mocker, config): def test_match_by_action_no_matching_action(mocker, config): # GIVEN an unsupported action - conf_store = init_configuration_store(mocker, {}, config) + feature_flags = init_feature_flags(mocker, {}, config) # WHEN calling _match_by_action - result = conf_store._match_by_action("Foo", None, "foo") + result = feature_flags._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) + feature_flags = init_feature_flags(mocker, {}, config) # WHEN calling _match_by_action - result = conf_store._match_by_action(RuleAction.STARTSWITH.value, 1, 100) + result = feature_flags._match_by_action(RuleAction.STARTSWITH.value, 1, 100) # THEN swallow the AttributeError and return False assert result is False @@ -457,10 +459,10 @@ 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) + feature_flags = init_feature_flags(mocker, {}, config) # WHEN calling _is_rule_matched - result = conf_store._is_rule_matched(rule_name="dummy", feature_name="dummy", rule=rule, context=rules_context) + result = feature_flags._is_rule_matched(rule_name="dummy", feature_name="dummy", rule=rule, context=rules_context) # THEN return False assert result is False From a8728144f831dceeb3879a741aa4c9f514c443ac Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 09:23:26 +0200 Subject: [PATCH 34/45] refactor: remove RULE_KEY, exception & rule test consistency --- .../utilities/feature_flags/schema.py | 23 ++--- .../feature_toggles/test_schema_validation.py | 99 ++++++++----------- 2 files changed, 55 insertions(+), 67 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 6eb9ea022b6..06a4eccf344 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -11,7 +11,6 @@ RULES_KEY = "rules" FEATURE_DEFAULT_VAL_KEY = "default" CONDITIONS_KEY = "conditions" -RULE_NAME_KEY = "rule_name" RULE_DEFAULT_VALUE = "when_match" CONDITION_KEY = "key" CONDITION_VALUE = "value" @@ -80,19 +79,18 @@ def validate(self): conditions = ConditionsValidator(rule=rule, rule_name=rule_name) conditions.validate() - def validate_rule(self, rule, rule_name, feature_name): + @staticmethod + def validate_rule(rule, rule_name, feature_name): if not rule or not isinstance(rule, dict): raise ConfigurationError(f"Feature rule must be a dictionary, feature_name={feature_name}") - self.validate_rule_name(rule_name=rule_name, feature_name=feature_name) - self.validate_rule_default_value(rule=rule, rule_name=rule_name) + RulesValidator.validate_rule_name(rule_name=rule_name, feature_name=feature_name) + RulesValidator.validate_rule_default_value(rule=rule, rule_name=rule_name) @staticmethod def validate_rule_name(rule_name: str, feature_name: str): if not rule_name or not isinstance(rule_name, str): - raise ConfigurationError( - f"'rule_name' key must be present and have a non-empty string, feature_name={feature_name}" - ) + raise ConfigurationError(f"Rule name key must have a non-empty string, feature_name={feature_name}") @staticmethod def validate_rule_default_value(rule: Dict, rule_name: str): @@ -116,7 +114,7 @@ def validate(self): @staticmethod def validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): - raise ConfigurationError(f"Invalid condition type, not a dictionary, rule_name={rule_name}") + raise ConfigurationError(f"Feature rule condition must be a dictionary, rule_name={rule_name}") ConditionsValidator.validate_condition_action(condition=condition, rule_name=rule_name) ConditionsValidator.validate_condition_key(condition=condition, rule_name=rule_name) @@ -126,16 +124,19 @@ def validate_condition(rule_name: str, condition: Dict[str, str]) -> None: def validate_condition_action(condition: Dict[str, Any], rule_name: str): action = condition.get(CONDITION_ACTION, "") if action not in RuleAction.__members__: - raise ConfigurationError(f"Invalid action value, rule_name={rule_name}, action={action}") + allowed_values = [_action.value for _action in RuleAction] + raise ConfigurationError( + f"'action' value must be either {allowed_values}, rule_name={rule_name}, action={action}" + ) @staticmethod def validate_condition_key(condition: Dict[str, Any], rule_name: str): key = condition.get(CONDITION_KEY, "") if not key or not isinstance(key, str): - raise ConfigurationError(f"Invalid key value, key has to be a non empty string, rule_name={rule_name}") + raise ConfigurationError(f"'key' value must be a non empty string, rule_name={rule_name}") @staticmethod def validate_condition_value(condition: Dict[str, Any], rule_name: str): value = condition.get(CONDITION_VALUE, "") if not value: - raise ConfigurationError(f"Missing condition value, rule_name={rule_name}") + raise ConfigurationError(f"'value' key must not be empty, rule_name={rule_name}") diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index a8730e1ad7f..948142a158b 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -11,7 +11,6 @@ FEATURE_DEFAULT_VAL_KEY, FEATURES_KEY, RULE_DEFAULT_VALUE, - RULE_NAME_KEY, RULES_KEY, ConditionsValidator, RuleAction, @@ -116,12 +115,11 @@ def test_invalid_rule(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 345345435", + RULES_KEY: { + "tenant id equals 345345435": { RULE_DEFAULT_VALUE: "False", - }, - ], + } + }, } } } @@ -134,12 +132,11 @@ def test_invalid_rule(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 345345435", + RULES_KEY: { + "tenant id equals 345345435": { RULE_DEFAULT_VALUE: False, - }, - ], + } + }, } } } @@ -152,9 +149,9 @@ def test_invalid_rule(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - {RULE_NAME_KEY: "tenant id equals 345345435", RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: []}, - ], + RULES_KEY: { + "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: []}, + }, } } } @@ -167,9 +164,9 @@ def test_invalid_rule(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - {RULE_NAME_KEY: "tenant id equals 345345435", RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {}}, - ], + RULES_KEY: { + "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {}}, + }, } } } @@ -184,13 +181,12 @@ def test_invalid_condition(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 345345435", + RULES_KEY: { + "tenant id equals 345345435": { RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {CONDITION_ACTION: "stuff", CONDITION_KEY: "a", CONDITION_VALUE: "a"}, - }, - ], + } + }, } } } @@ -203,13 +199,12 @@ def test_invalid_condition(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 345345435", + RULES_KEY: { + "tenant id equals 345345435": { RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {CONDITION_ACTION: RuleAction.EQUALS.value}, - }, - ], + } + }, } } } @@ -222,17 +217,16 @@ def test_invalid_condition(): FEATURES_KEY: { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - { - RULE_NAME_KEY: "tenant id equals 345345435", + RULES_KEY: { + "tenant id equals 345345435": { RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: { CONDITION_ACTION: RuleAction.EQUALS.value, CONDITION_KEY: 5, CONDITION_VALUE: "a", }, - }, - ], + } + }, } } } @@ -286,7 +280,7 @@ def test_validate_condition_invalid_condition_type(): # WHEN calling validate_condition # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Invalid condition type"): + with pytest.raises(ConfigurationError, match="Feature rule condition must be a dictionary"): ConditionsValidator.validate_condition(condition=condition, rule_name="dummy") @@ -296,7 +290,7 @@ def test_validate_condition_invalid_condition_action(): # WHEN calling validate_condition # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Invalid action value"): + with pytest.raises(ConfigurationError, match="'action' value must be either"): ConditionsValidator.validate_condition_action(condition=condition, rule_name="dummy") @@ -306,7 +300,7 @@ def test_validate_condition_invalid_condition_key(): # WHEN calling validate_condition # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Invalid key value"): + with pytest.raises(ConfigurationError, match="'key' value must be a non empty string"): ConditionsValidator.validate_condition_key(condition=condition, rule_name="dummy") @@ -318,28 +312,21 @@ def test_validate_condition_missing_condition_value(): } # WHEN calling validate_condition - with pytest.raises(ConfigurationError, match="Missing condition value"): + with pytest.raises(ConfigurationError, match="'value' key must not be empty"): ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy") -def test_new_rule_format(): - new_features = { - "my_feature": { - "default": True, - "rules": { - "tenant id equals 345345435": { - "when_match": False, - "conditions": [ - { - "action": "EQUALS", - "key": "tenant_id", - "value": "345345435", - } - ], - }, - }, - } - } +def test_validate_rule_invalid_rule_type(): + # GIVEN an invalid rule type of empty list + # WHEN calling validate_rule + # THEN raise ConfigurationError + with pytest.raises(ConfigurationError, match="Feature rule must be a dictionary"): + RulesValidator.validate_rule(rule=[], rule_name="dummy", feature_name="dummy") - rules = RulesValidator(feature=new_features) - rules.validate() + +def test_validate_rule_invalid_rule_name(): + # GIVEN a rule name is empty + # WHEN calling validate_rule_name + # THEN raise ConfigurationError + with pytest.raises(ConfigurationError, match="Rule name key must have a non-empty string"): + RulesValidator.validate_rule_name(rule_name="", feature_name="dummy") From 1f6269683fbf0c8ef3ceee7ae9198c072e577510 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 10:02:53 +0200 Subject: [PATCH 35/45] refactor(schema): remove 'features' key --- .../utilities/feature_flags/feature_flags.py | 12 +- .../utilities/feature_flags/schema.py | 9 +- .../feature_toggles/test_feature_toggles.py | 402 +++++++++--------- .../feature_toggles/test_schema_validation.py | 254 +++++------ 4 files changed, 302 insertions(+), 375 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index b9a59f9f9f6..038d684f420 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -121,7 +121,7 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: validator = schema.SchemaValidator(schema=config) validator.validate() - return config.get(schema.FEATURES_KEY, {}) + return config def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. @@ -154,8 +154,8 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau try: features = self.get_configuration() - except ConfigurationError: - logger.debug("Unable to get feature toggles JSON, returning provided default value") + except ConfigurationError as err: + logger.debug(f"Unable to get feature toggles JSON, returning provided default value, reason={err}") return default feature = features.get(name) @@ -165,9 +165,9 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau ) return default - rules_list = feature.get(schema.RULES_KEY) + rules = feature.get(schema.RULES_KEY) feature_default_value = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) - if not rules_list: + if not rules: # no rules but value logger.debug( f"no rules found, returning feature default value, name={name}, " @@ -181,7 +181,7 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau feature_name=name, context=context, feature_default_value=bool(feature_default_value), - rules=cast(List, rules_list), + rules=rules, ) def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> List[str]: diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 06a4eccf344..d38e4269625 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -7,7 +7,6 @@ logger = logging.getLogger(__name__) -FEATURES_KEY = "features" RULES_KEY = "rules" FEATURE_DEFAULT_VAL_KEY = "default" CONDITIONS_KEY = "conditions" @@ -30,7 +29,7 @@ def __init__(self, schema: Dict[str, Any]): def validate(self) -> None: if not isinstance(self.schema, dict): - raise ConfigurationError(f"Schema must be a dictionary, schema={str(self.schema)}") + raise ConfigurationError(f"Features must be a dictionary, schema={str(self.schema)}") features = FeaturesValidator(schema=self.schema) features.validate() @@ -39,13 +38,9 @@ def validate(self) -> None: class FeaturesValidator(BaseValidator): def __init__(self, schema: Dict): self.schema = schema - self.features: Optional[Dict[str, Dict]] = self.schema.get(FEATURES_KEY) def validate(self): - if not isinstance(self.features, dict): - raise ConfigurationError(f"'features' key must be a dictionary, schema={self.schema}") - - for name, feature in self.features.items(): + for name, feature in self.schema.items(): self.validate_feature(name, feature) rules = RulesValidator(feature=feature) rules.validate() diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index a5f25b1d1fc..1a0f02e5b1b 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -47,22 +47,20 @@ def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigSt def test_toggles_rule_does_not_match(mocker, config): expected_value = True mocked_app_config_schema = { - "features": { - "my_feature": { - "default": expected_value, - "rules": { - "tenant id equals 345345435": { - "when_match": False, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "345345435", - } - ], - } - }, - } + "my_feature": { + "default": expected_value, + "rules": { + "tenant id equals 345345435": { + "when_match": False, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "345345435", + } + ], + } + }, } } @@ -86,7 +84,7 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): # default value is False but the feature has a True default_value. def test_toggles_no_rules(mocker, config): expected_value = True - mocked_app_config_schema = {"features": {"my_feature": {"default": expected_value}}} + mocked_app_config_schema = {"my_feature": {"default": expected_value}} feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) assert toggle == expected_value @@ -96,22 +94,20 @@ def test_toggles_no_rules(mocker, config): def test_toggles_conditions_no_match(mocker, config): expected_value = True mocked_app_config_schema = { - "features": { - "my_feature": { - "default": expected_value, - "rules": { - "tenant id equals 345345435": { - "when_match": False, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "345345435", - } - ], - } - }, - } + "my_feature": { + "default": expected_value, + "rules": { + "tenant id equals 345345435": { + "when_match": False, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "345345435", + } + ], + } + }, } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) @@ -125,28 +121,26 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) tenant_id_val = "6" username_val = "a" mocked_app_config_schema = { - "features": { - "my_feature": { - "default": True, - "rules": { - "tenant id equals 6 and username is a": { - "when_match": expected_value, - "conditions": [ - { - "action": RuleAction.EQUALS.value, # this rule will match, it has multiple conditions - "key": "tenant_id", - "value": tenant_id_val, - }, - { - "action": RuleAction.EQUALS.value, - "key": "username", - "value": username_val, - }, - ], - } - }, - } - }, + "my_feature": { + "default": True, + "rules": { + "tenant id equals 6 and username is a": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.EQUALS.value, # this rule will match, it has multiple conditions + "key": "tenant_id", + "value": tenant_id_val, + }, + { + "action": RuleAction.EQUALS.value, + "key": "username", + "value": username_val, + }, + ], + } + }, + } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) toggle = feature_flags.evaluate( @@ -166,28 +160,26 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, config): expected_val = True mocked_app_config_schema = { - "features": { - "my_feature": { - "default": expected_val, - "rules": { - # rule will not match - "tenant id equals 645654 and username is a": { - "when_match": False, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "645654", - }, - { - "action": RuleAction.EQUALS.value, - "key": "username", - "value": "a", - }, - ], - } - }, - } + "my_feature": { + "default": expected_val, + "rules": { + # rule will not match + "tenant id equals 645654 and username is a": { + "when_match": False, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "645654", + }, + { + "action": RuleAction.EQUALS.value, + "key": "username", + "value": "a", + }, + ], + } + }, } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) @@ -202,47 +194,45 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ expected_value_third_check = False expected_value_fourth_case = False mocked_app_config_schema = { - "features": { - "my_feature": { - "default": expected_value_third_check, - "rules": { - "tenant id equals 6 and username startswith a": { - "when_match": expected_value_first_check, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "6", - }, - { - "action": RuleAction.STARTSWITH.value, - "key": "username", - "value": "a", - }, - ], - }, - "tenant id equals 4446 and username startswith a and endswith z": { - "when_match": expected_value_second_check, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "4446", - }, - { - "action": RuleAction.STARTSWITH.value, - "key": "username", - "value": "a", - }, - { - "action": RuleAction.ENDSWITH.value, - "key": "username", - "value": "z", - }, - ], - }, + "my_feature": { + "default": expected_value_third_check, + "rules": { + "tenant id equals 6 and username startswith a": { + "when_match": expected_value_first_check, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "6", + }, + { + "action": RuleAction.STARTSWITH.value, + "key": "username", + "value": "a", + }, + ], + }, + "tenant id equals 4446 and username startswith a and endswith z": { + "when_match": expected_value_second_check, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "4446", + }, + { + "action": RuleAction.STARTSWITH.value, + "key": "username", + "value": "a", + }, + { + "action": RuleAction.ENDSWITH.value, + "key": "username", + "value": "z", + }, + ], }, - } + }, } } @@ -271,22 +261,20 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ def test_toggles_match_rule_with_contains_action(mocker, config): expected_value = True mocked_app_config_schema = { - "features": { - "my_feature": { - "default": False, - "rules": { - "tenant id is contained in [6, 2]": { - "when_match": expected_value, - "conditions": [ - { - "action": RuleAction.CONTAINS.value, - "key": "tenant_id", - "value": ["6", "2"], - } - ], - } - }, - } + "my_feature": { + "default": False, + "rules": { + "tenant id is contained in [6, 2]": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.CONTAINS.value, + "key": "tenant_id", + "value": ["6", "2"], + } + ], + } + }, } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) @@ -297,24 +285,22 @@ def test_toggles_match_rule_with_contains_action(mocker, config): def test_toggles_no_match_rule_with_contains_action(mocker, config): expected_value = False mocked_app_config_schema = { - "features": { - "my_feature": { - "default": expected_value, - "rules": [ - { - "rule_name": "tenant id is contained in [8, 2]", - "when_match": True, - "conditions": [ - { - "action": RuleAction.CONTAINS.value, - "key": "tenant_id", - "value": ["8", "2"], - } - ], - }, - ], - } - }, + "my_feature": { + "default": expected_value, + "rules": [ + { + "rule_name": "tenant id is contained in [8, 2]", + "when_match": True, + "conditions": [ + { + "action": RuleAction.CONTAINS.value, + "key": "tenant_id", + "value": ["8", "2"], + } + ], + }, + ], + } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) toggle = feature_flags.evaluate(name="my_feature", context={"tenant_id": "6", "username": "a"}, default=False) @@ -324,29 +310,27 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): def test_multiple_features_enabled(mocker, config): expected_value = ["my_feature", "my_feature2"] mocked_app_config_schema = { - "features": { - "my_feature": { - "default": False, - "rules": { - "tenant id is contained in [6, 2]": { - "when_match": True, - "conditions": [ - { - "action": RuleAction.CONTAINS.value, - "key": "tenant_id", - "value": ["6", "2"], - } - ], - } - }, - }, - "my_feature2": { - "default": True, + "my_feature": { + "default": False, + "rules": { + "tenant id is contained in [6, 2]": { + "when_match": True, + "conditions": [ + { + "action": RuleAction.CONTAINS.value, + "key": "tenant_id", + "value": ["6", "2"], + } + ], + } }, - "my_feature3": { - "default": False, - }, - } + }, + "my_feature2": { + "default": True, + }, + "my_feature3": { + "default": False, + }, } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) enabled_list: List[str] = feature_flags.get_enabled_features(context={"tenant_id": "6", "username": "a"}) @@ -356,45 +340,43 @@ def test_multiple_features_enabled(mocker, config): def test_multiple_features_only_some_enabled(mocker, config): expected_value = ["my_feature", "my_feature2", "my_feature4"] mocked_app_config_schema = { - "features": { - "my_feature": { # rule will match here, feature is enabled due to rule match - "default": False, - "rules": { - "tenant id is contained in [6, 2]": { - "when_match": True, - "conditions": [ - { - "action": RuleAction.CONTAINS.value, - "key": "tenant_id", - "value": ["6", "2"], - } - ], - } - }, + "my_feature": { # rule will match here, feature is enabled due to rule match + "default": False, + "rules": { + "tenant id is contained in [6, 2]": { + "when_match": True, + "conditions": [ + { + "action": RuleAction.CONTAINS.value, + "key": "tenant_id", + "value": ["6", "2"], + } + ], + } }, - "my_feature2": { - "default": True, - }, - "my_feature3": { - "default": False, - }, - # rule will not match here, feature is enabled by default - "my_feature4": { - "default": True, - "rules": { - "tenant id equals 7": { - "when_match": False, - "conditions": [ - { - "action": RuleAction.EQUALS.value, - "key": "tenant_id", - "value": "7", - } - ], - } - }, + }, + "my_feature2": { + "default": True, + }, + "my_feature3": { + "default": False, + }, + # rule will not match here, feature is enabled by default + "my_feature4": { + "default": True, + "rules": { + "tenant id equals 7": { + "when_match": False, + "conditions": [ + { + "action": RuleAction.EQUALS.value, + "key": "tenant_id", + "value": "7", + } + ], + } }, - } + }, } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) enabled_list: List[str] = feature_flags.get_enabled_features(context={"tenant_id": "6", "username": "a"}) diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index 948142a158b..a66cba5ad12 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -9,7 +9,6 @@ CONDITION_VALUE, CONDITIONS_KEY, FEATURE_DEFAULT_VAL_KEY, - FEATURES_KEY, RULE_DEFAULT_VALUE, RULES_KEY, ConditionsValidator, @@ -24,58 +23,27 @@ def test_invalid_features_dict(): - schema = {} - # empty dict - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - schema = [] - # invalid type - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - # invalid features key - schema = {FEATURES_KEY: []} - validator = SchemaValidator(schema) + validator = SchemaValidator(schema=[]) with pytest.raises(ConfigurationError): validator.validate() def test_empty_features_not_fail(): - schema = {FEATURES_KEY: {}} - validator = SchemaValidator(schema) + validator = SchemaValidator(schema={}) validator.validate() -def test_invalid_feature_dict(): - # invalid feature type, not dict - schema = {FEATURES_KEY: {"my_feature": []}} - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - # empty feature dict - schema = {FEATURES_KEY: {"my_feature": {}}} - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}} - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - # invalid FEATURE_DEFAULT_VAL_KEY type, not boolean #2 - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: 5}}} - validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): - validator.validate() - - # invalid rules type, not list - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}} +@pytest.mark.parametrize( + "schema", + [ + pytest.param({"my_feature": []}, id="feat_as_list"), + pytest.param({"my_feature": {}}, id="feat_empty_dict"), + pytest.param({"my_feature": {FEATURE_DEFAULT_VAL_KEY: "False"}}, id="feat_default_non_bool"), + pytest.param({"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: "4"}}, id="feat_rules_non_dict"), + pytest.param("%<>[]{}|^", id="unsafe-rfc3986"), + ], +) +def test_invalid_feature(schema): validator = SchemaValidator(schema) with pytest.raises(ConfigurationError): validator.validate() @@ -83,12 +51,12 @@ def test_invalid_feature_dict(): def test_valid_feature_dict(): # empty rules list - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}}} + schema = {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: []}} validator = SchemaValidator(schema) validator.validate() # no rules list at all - schema = {FEATURES_KEY: {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}}} + schema = {"my_feature": {FEATURE_DEFAULT_VAL_KEY: False}} validator = SchemaValidator(schema) validator.validate() @@ -96,14 +64,12 @@ def test_valid_feature_dict(): def test_invalid_rule(): # rules list is not a list of dict schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: [ - "a", - "b", - ], - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: [ + "a", + "b", + ], } } validator = SchemaValidator(schema) @@ -112,15 +78,13 @@ def test_invalid_rule(): # rules RULE_DEFAULT_VALUE is not bool schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: "False", - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": { + RULE_DEFAULT_VALUE: "False", + } + }, } } validator = SchemaValidator(schema) @@ -129,15 +93,13 @@ def test_invalid_rule(): # missing conditions list schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": { + RULE_DEFAULT_VALUE: False, + } + }, } } validator = SchemaValidator(schema) @@ -146,13 +108,11 @@ def test_invalid_rule(): # condition list is empty schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: []}, - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: []}, + }, } } validator = SchemaValidator(schema) @@ -161,13 +121,11 @@ def test_invalid_rule(): # condition is invalid type, not list schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {}}, - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {}}, + }, } } validator = SchemaValidator(schema) @@ -178,16 +136,14 @@ def test_invalid_rule(): def test_invalid_condition(): # invalid condition action schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, - CONDITIONS_KEY: {CONDITION_ACTION: "stuff", CONDITION_KEY: "a", CONDITION_VALUE: "a"}, - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": { + RULE_DEFAULT_VALUE: False, + CONDITIONS_KEY: {CONDITION_ACTION: "stuff", CONDITION_KEY: "a", CONDITION_VALUE: "a"}, + } + }, } } validator = SchemaValidator(schema) @@ -196,16 +152,14 @@ def test_invalid_condition(): # missing condition key and value schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, - CONDITIONS_KEY: {CONDITION_ACTION: RuleAction.EQUALS.value}, - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": { + RULE_DEFAULT_VALUE: False, + CONDITIONS_KEY: {CONDITION_ACTION: RuleAction.EQUALS.value}, + } + }, } } validator = SchemaValidator(schema) @@ -214,20 +168,18 @@ def test_invalid_condition(): # invalid condition key type, not string schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, - CONDITIONS_KEY: { - CONDITION_ACTION: RuleAction.EQUALS.value, - CONDITION_KEY: 5, - CONDITION_VALUE: "a", - }, - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 345345435": { + RULE_DEFAULT_VALUE: False, + CONDITIONS_KEY: { + CONDITION_ACTION: RuleAction.EQUALS.value, + CONDITION_KEY: 5, + CONDITION_VALUE: "a", + }, + } + }, } } validator = SchemaValidator(schema) @@ -237,37 +189,35 @@ def test_invalid_condition(): def test_valid_condition_all_actions(): schema = { - FEATURES_KEY: { - "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { - "tenant id equals 645654 and username is a": { - RULE_DEFAULT_VALUE: True, - CONDITIONS_KEY: [ - { - CONDITION_ACTION: RuleAction.EQUALS.value, - CONDITION_KEY: "tenant_id", - CONDITION_VALUE: "645654", - }, - { - CONDITION_ACTION: RuleAction.STARTSWITH.value, - CONDITION_KEY: "username", - CONDITION_VALUE: "a", - }, - { - CONDITION_ACTION: RuleAction.ENDSWITH.value, - CONDITION_KEY: "username", - CONDITION_VALUE: "a", - }, - { - CONDITION_ACTION: RuleAction.CONTAINS.value, - CONDITION_KEY: "username", - CONDITION_VALUE: ["a", "b"], - }, - ], - } - }, - } + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id equals 645654 and username is a": { + RULE_DEFAULT_VALUE: True, + CONDITIONS_KEY: [ + { + CONDITION_ACTION: RuleAction.EQUALS.value, + CONDITION_KEY: "tenant_id", + CONDITION_VALUE: "645654", + }, + { + CONDITION_ACTION: RuleAction.STARTSWITH.value, + CONDITION_KEY: "username", + CONDITION_VALUE: "a", + }, + { + CONDITION_ACTION: RuleAction.ENDSWITH.value, + CONDITION_KEY: "username", + CONDITION_VALUE: "a", + }, + { + CONDITION_ACTION: RuleAction.CONTAINS.value, + CONDITION_KEY: "username", + CONDITION_VALUE: ["a", "b"], + }, + ], + } + }, } } validator = SchemaValidator(schema) From f63a5dacc9ac828a9f37373816d7528172520c52 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 11:08:46 +0200 Subject: [PATCH 36/45] feat: support JMESPath envelope/options to extract features --- .../shared/jmespath_functions.py | 22 -------- .../shared/jmespath_utils.py | 55 +++++++++++++++++++ .../utilities/feature_flags/appconfig.py | 39 +++++++++---- .../utilities/feature_flags/base.py | 4 -- .../utilities/idempotency/persistence/base.py | 2 +- .../utilities/validation/base.py | 36 +----------- .../utilities/validation/validator.py | 13 +++-- .../feature_toggles/test_feature_toggles.py | 18 +++++- tests/functional/idempotency/conftest.py | 2 +- 9 files changed, 111 insertions(+), 80 deletions(-) delete mode 100644 aws_lambda_powertools/shared/jmespath_functions.py create mode 100644 aws_lambda_powertools/shared/jmespath_utils.py diff --git a/aws_lambda_powertools/shared/jmespath_functions.py b/aws_lambda_powertools/shared/jmespath_functions.py deleted file mode 100644 index b23ab477d6b..00000000000 --- a/aws_lambda_powertools/shared/jmespath_functions.py +++ /dev/null @@ -1,22 +0,0 @@ -import base64 -import gzip -import json - -import jmespath - - -class PowertoolsFunctions(jmespath.functions.Functions): - @jmespath.functions.signature({"types": ["string"]}) - def _func_powertools_json(self, value): - return json.loads(value) - - @jmespath.functions.signature({"types": ["string"]}) - def _func_powertools_base64(self, value): - return base64.b64decode(value).decode() - - @jmespath.functions.signature({"types": ["string"]}) - def _func_powertools_base64_gzip(self, value): - encoded = base64.b64decode(value) - uncompressed = gzip.decompress(encoded) - - return uncompressed.decode() diff --git a/aws_lambda_powertools/shared/jmespath_utils.py b/aws_lambda_powertools/shared/jmespath_utils.py new file mode 100644 index 00000000000..f2a865d4807 --- /dev/null +++ b/aws_lambda_powertools/shared/jmespath_utils.py @@ -0,0 +1,55 @@ +import base64 +import gzip +import json +from typing import Any, Dict, Optional, Union + +import jmespath +from jmespath.exceptions import LexerError + +from aws_lambda_powertools.utilities.validation import InvalidEnvelopeExpressionError +from aws_lambda_powertools.utilities.validation.base import logger + + +class PowertoolsFunctions(jmespath.functions.Functions): + @jmespath.functions.signature({"types": ["string"]}) + def _func_powertools_json(self, value): + return json.loads(value) + + @jmespath.functions.signature({"types": ["string"]}) + def _func_powertools_base64(self, value): + return base64.b64decode(value).decode() + + @jmespath.functions.signature({"types": ["string"]}) + def _func_powertools_base64_gzip(self, value): + encoded = base64.b64decode(value) + uncompressed = gzip.decompress(encoded) + + return uncompressed.decode() + + +def unwrap_event_from_envelope(data: Union[Dict, str], envelope: str, jmespath_options: Optional[Dict]) -> Any: + """Searches data using JMESPath expression + + Parameters + ---------- + data : Dict + Data set to be filtered + envelope : str + JMESPath expression to filter data against + jmespath_options : Dict + Alternative JMESPath options to be included when filtering expr + + Returns + ------- + Any + Data found using JMESPath expression given in envelope + """ + if not jmespath_options: + jmespath_options = {"custom_functions": PowertoolsFunctions()} + + try: + logger.debug(f"Envelope detected: {envelope}. JMESPath options: {jmespath_options}") + return jmespath.search(envelope, data, options=jmespath.Options(**jmespath_options)) + except (LexerError, TypeError, UnicodeError) as e: + message = f"Failed to unwrap event from envelope using expression. Error: {e} Exp: {envelope}, Data: {data}" # noqa: B306, E501 + raise InvalidEnvelopeExpressionError(message) diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index 343405e1675..777ca96bbee 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -5,6 +5,7 @@ from aws_lambda_powertools.utilities.parameters import AppConfigProvider, GetParameterError, TransformParameterError +from ...shared import jmespath_utils from .base import StoreProvider from .exceptions import ConfigurationError @@ -21,6 +22,8 @@ def __init__( name: str, cache_seconds: int, config: Optional[Config] = None, + envelope: str = "", + jmespath_options: Optional[Dict] = None, ): """This class fetches JSON schemas from AWS AppConfig @@ -36,18 +39,28 @@ def __init__( cache expiration time, how often to call AppConfig to fetch latest configuration config: Optional[Config] boto3 client configuration + envelope : str + JMESPath expression to pluck feature flags data from config + jmespath_options : Dict + Alternative JMESPath options to be included when filtering expr """ - super().__init__(name, cache_seconds) - self._logger = logger + super().__init__() + self.environment = environment + self.application = application + self.name = name + self.cache_seconds = cache_seconds + self.config = config + self.envelope = envelope + self.jmespath_options = jmespath_options self._conf_store = AppConfigProvider(environment=environment, application=application, config=config) def get_json_configuration(self) -> Dict[str, Any]: - """Get configuration string from AWs AppConfig and return the parsed JSON dictionary + """Get configuration string from AWS AppConfig and return the parsed JSON dictionary Raises ------ ConfigurationError - Any validation error or appconfig error that can occur + Any validation error or AppConfig error that can occur Returns ------- @@ -56,13 +69,17 @@ def get_json_configuration(self) -> Dict[str, Any]: """ try: # parse result conf as JSON, keep in cache for self.max_age seconds - return cast( - dict, - self._conf_store.get( - name=self.name, - transform=TRANSFORM_TYPE, - max_age=self._cache_seconds, - ), + config = self._conf_store.get( + name=self.name, + transform=TRANSFORM_TYPE, + max_age=self.cache_seconds, ) + + if self.envelope: + config = jmespath_utils.unwrap_event_from_envelope( + data=config, envelope=self.envelope, jmespath_options=self.jmespath_options + ) + + return cast(dict, config) except (GetParameterError, TransformParameterError) as exc: raise ConfigurationError("Unable to get AWS AppConfig configuration file") from exc diff --git a/aws_lambda_powertools/utilities/feature_flags/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py index f1f7c02217f..8304f5acadb 100644 --- a/aws_lambda_powertools/utilities/feature_flags/base.py +++ b/aws_lambda_powertools/utilities/feature_flags/base.py @@ -3,10 +3,6 @@ class StoreProvider(ABC): - def __init__(self, configuration_name: str, cache_seconds: int): - self.name = configuration_name - self._cache_seconds = cache_seconds - @abstractmethod def get_json_configuration(self) -> Dict[str, Any]: """Get configuration string from any configuration storing application and return the parsed JSON dictionary diff --git a/aws_lambda_powertools/utilities/idempotency/persistence/base.py b/aws_lambda_powertools/utilities/idempotency/persistence/base.py index eb43a8b30c5..0388adfbf55 100644 --- a/aws_lambda_powertools/utilities/idempotency/persistence/base.py +++ b/aws_lambda_powertools/utilities/idempotency/persistence/base.py @@ -14,7 +14,7 @@ import jmespath from aws_lambda_powertools.shared.cache_dict import LRUDict -from aws_lambda_powertools.shared.jmespath_functions import PowertoolsFunctions +from aws_lambda_powertools.shared.jmespath_utils import PowertoolsFunctions from aws_lambda_powertools.shared.json_encoder import Encoder from aws_lambda_powertools.utilities.idempotency.config import IdempotencyConfig from aws_lambda_powertools.utilities.idempotency.exceptions import ( diff --git a/aws_lambda_powertools/utilities/validation/base.py b/aws_lambda_powertools/utilities/validation/base.py index b818f11a40e..13deb4d24e2 100644 --- a/aws_lambda_powertools/utilities/validation/base.py +++ b/aws_lambda_powertools/utilities/validation/base.py @@ -1,13 +1,9 @@ import logging -from typing import Any, Dict, Optional, Union +from typing import Dict, Optional, Union import fastjsonschema # type: ignore -import jmespath -from jmespath.exceptions import LexerError # type: ignore -from aws_lambda_powertools.shared.jmespath_functions import PowertoolsFunctions - -from .exceptions import InvalidEnvelopeExpressionError, InvalidSchemaFormatError, SchemaValidationError +from .exceptions import InvalidSchemaFormatError, SchemaValidationError logger = logging.getLogger(__name__) @@ -39,31 +35,3 @@ def validate_data_against_schema(data: Union[Dict, str], schema: Dict, formats: except fastjsonschema.JsonSchemaException as e: message = f"Failed schema validation. Error: {e.message}, Path: {e.path}, Data: {e.value}" # noqa: B306, E501 raise SchemaValidationError(message) - - -def unwrap_event_from_envelope(data: Union[Dict, str], envelope: str, jmespath_options: Optional[Dict]) -> Any: - """Searches data using JMESPath expression - - Parameters - ---------- - data : Dict - Data set to be filtered - envelope : str - JMESPath expression to filter data against - jmespath_options : Dict - Alternative JMESPath options to be included when filtering expr - - Returns - ------- - Any - Data found using JMESPath expression given in envelope - """ - if not jmespath_options: - jmespath_options = {"custom_functions": PowertoolsFunctions()} - - try: - logger.debug(f"Envelope detected: {envelope}. JMESPath options: {jmespath_options}") - return jmespath.search(envelope, data, options=jmespath.Options(**jmespath_options)) - except (LexerError, TypeError, UnicodeError) as e: - message = f"Failed to unwrap event from envelope using expression. Error: {e} Exp: {envelope}, Data: {data}" # noqa: B306, E501 - raise InvalidEnvelopeExpressionError(message) diff --git a/aws_lambda_powertools/utilities/validation/validator.py b/aws_lambda_powertools/utilities/validation/validator.py index 0497a49a714..02a685a1565 100644 --- a/aws_lambda_powertools/utilities/validation/validator.py +++ b/aws_lambda_powertools/utilities/validation/validator.py @@ -2,7 +2,8 @@ from typing import Any, Callable, Dict, Optional, Union from ...middleware_factory import lambda_handler_decorator -from .base import unwrap_event_from_envelope, validate_data_against_schema +from ...shared import jmespath_utils +from .base import validate_data_against_schema logger = logging.getLogger(__name__) @@ -16,7 +17,7 @@ def validator( inbound_formats: Optional[Dict] = None, outbound_schema: Optional[Dict] = None, outbound_formats: Optional[Dict] = None, - envelope: Optional[str] = None, + envelope: str = "", jmespath_options: Optional[Dict] = None, ) -> Any: """Lambda handler decorator to validate incoming/outbound data using a JSON Schema @@ -116,7 +117,9 @@ def handler(event, context): When JMESPath expression to unwrap event is invalid """ if envelope: - event = unwrap_event_from_envelope(data=event, envelope=envelope, jmespath_options=jmespath_options) + event = jmespath_utils.unwrap_event_from_envelope( + data=event, envelope=envelope, jmespath_options=jmespath_options + ) if inbound_schema: logger.debug("Validating inbound event") @@ -216,6 +219,8 @@ def handler(event, context): When JMESPath expression to unwrap event is invalid """ if envelope: - event = unwrap_event_from_envelope(data=event, envelope=envelope, jmespath_options=jmespath_options) + event = jmespath_utils.unwrap_event_from_envelope( + data=event, envelope=envelope, jmespath_options=jmespath_options + ) validate_data_against_schema(data=event, schema=schema, formats=formats) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 1a0f02e5b1b..f512cb823a1 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -1,4 +1,4 @@ -from typing import Dict, List +from typing import Dict, List, Optional import pytest from botocore.config import Config @@ -15,7 +15,9 @@ def config(): return Config(region_name="us-east-1") -def init_feature_flags(mocker, mock_schema: Dict, config: Config) -> FeatureFlags: +def init_feature_flags( + mocker, mock_schema: Dict, config: Config, envelope: str = "", jmespath_options: Optional[Dict] = None +) -> FeatureFlags: mocked_get_conf = mocker.patch("aws_lambda_powertools.utilities.parameters.AppConfigProvider.get") mocked_get_conf.return_value = mock_schema @@ -25,6 +27,8 @@ def init_feature_flags(mocker, mock_schema: Dict, config: Config) -> FeatureFlag name="test_conf_name", cache_seconds=600, config=config, + envelope=envelope, + jmespath_options=jmespath_options, ) feature_flags: FeatureFlags = FeatureFlags(store=app_conf_fetcher) return feature_flags @@ -73,7 +77,7 @@ def test_toggles_rule_does_not_match(mocker, config): # you get the default value of False that was sent to the evaluate API def test_toggles_no_conditions_feature_does_not_exist(mocker, config): expected_value = False - mocked_app_config_schema = {"features": {"my_fake_feature": {"default": True}}} + mocked_app_config_schema = {"my_fake_feature": {"default": True}} feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) toggle = feature_flags.evaluate(name="my_feature", context={}, default=expected_value) @@ -448,3 +452,11 @@ def test_is_rule_matched_no_matches(mocker, config): # THEN return False assert result is False + + +def test_features_jmespath_envelope(mocker, config): + expected_value = True + mocked_app_config_schema = {"features": {"my_feature": {"default": expected_value}}} + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config, envelope="features") + toggle = feature_flags.evaluate(name="my_feature", context={}, default=False) + assert toggle == expected_value diff --git a/tests/functional/idempotency/conftest.py b/tests/functional/idempotency/conftest.py index e100957dee7..9f61d50d656 100644 --- a/tests/functional/idempotency/conftest.py +++ b/tests/functional/idempotency/conftest.py @@ -11,11 +11,11 @@ from botocore.config import Config from jmespath import functions +from aws_lambda_powertools.shared.jmespath_utils import unwrap_event_from_envelope from aws_lambda_powertools.shared.json_encoder import Encoder from aws_lambda_powertools.utilities.idempotency import DynamoDBPersistenceLayer from aws_lambda_powertools.utilities.idempotency.idempotency import IdempotencyConfig from aws_lambda_powertools.utilities.validation import envelopes -from aws_lambda_powertools.utilities.validation.base import unwrap_event_from_envelope from tests.functional.utils import load_event TABLE_NAME = "TEST_TABLE" From 9c714abbf507a6689d21c461e6585e6835bf67e6 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 11:24:53 +0200 Subject: [PATCH 37/45] refactor(store): use get_configuration over get_json_configuration --- aws_lambda_powertools/utilities/feature_flags/appconfig.py | 2 +- aws_lambda_powertools/utilities/feature_flags/base.py | 2 +- .../utilities/feature_flags/feature_flags.py | 2 +- tests/functional/feature_toggles/test_feature_toggles.py | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index 777ca96bbee..1243d2dca32 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -54,7 +54,7 @@ def __init__( self.jmespath_options = jmespath_options self._conf_store = AppConfigProvider(environment=environment, application=application, config=config) - def get_json_configuration(self) -> Dict[str, Any]: + def get_configuration(self) -> Dict[str, Any]: """Get configuration string from AWS AppConfig and return the parsed JSON dictionary Raises diff --git a/aws_lambda_powertools/utilities/feature_flags/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py index 8304f5acadb..c10106275c6 100644 --- a/aws_lambda_powertools/utilities/feature_flags/base.py +++ b/aws_lambda_powertools/utilities/feature_flags/base.py @@ -4,7 +4,7 @@ class StoreProvider(ABC): @abstractmethod - def get_json_configuration(self) -> Dict[str, Any]: + def get_configuration(self) -> Dict[str, Any]: """Get configuration string from any configuration storing application and return the parsed JSON dictionary Raises diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 038d684f420..f5ca57b8bf3 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -116,7 +116,7 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: } """ # parse result conf as JSON, keep in cache for self.max_age seconds - config = self._store.get_json_configuration() + config = self._store.get_configuration() validator = schema.SchemaValidator(schema=config) validator.validate() diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index f512cb823a1..4cee2b933aa 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -415,9 +415,9 @@ def test_app_config_get_parameter_err(mocker, config): # GIVEN an appconfig with a missing config app_conf_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) - # WHEN calling get_json_configuration + # WHEN calling get_configuration with pytest.raises(ConfigurationError) as err: - app_conf_fetcher.get_json_configuration() + app_conf_fetcher.get_configuration() # THEN raise ConfigurationError error assert "AWS AppConfig configuration" in str(err.value) From f89d5727acd0b890f817777ec4291010aa839110 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 18:43:35 +0200 Subject: [PATCH 38/45] refactor: add docstrings and logging to FeatureFlags, Store --- .../utilities/feature_flags/appconfig.py | 12 +- .../utilities/feature_flags/base.py | 26 ++- .../utilities/feature_flags/feature_flags.py | 204 +++++++++--------- .../utilities/feature_flags/schema.py | 27 ++- .../feature_toggles/test_feature_toggles.py | 10 +- .../feature_toggles/test_schema_validation.py | 20 +- 6 files changed, 171 insertions(+), 128 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index 1243d2dca32..411683d649b 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -21,7 +21,7 @@ def __init__( application: str, name: str, cache_seconds: int, - config: Optional[Config] = None, + sdk_config: Optional[Config] = None, envelope: str = "", jmespath_options: Optional[Dict] = None, ): @@ -37,8 +37,8 @@ def __init__( AppConfig configuration name e.g. `my_conf` cache_seconds: int cache expiration time, how often to call AppConfig to fetch latest configuration - config: Optional[Config] - boto3 client configuration + sdk_config: Optional[Config] + Botocore Config object to pass during client initialization envelope : str JMESPath expression to pluck feature flags data from config jmespath_options : Dict @@ -49,13 +49,13 @@ def __init__( self.application = application self.name = name self.cache_seconds = cache_seconds - self.config = config + self.config = sdk_config self.envelope = envelope self.jmespath_options = jmespath_options - self._conf_store = AppConfigProvider(environment=environment, application=application, config=config) + self._conf_store = AppConfigProvider(environment=environment, application=application, config=sdk_config) def get_configuration(self) -> Dict[str, Any]: - """Get configuration string from AWS AppConfig and return the parsed JSON dictionary + """Fetch feature schema configuration from AWS AppConfig Raises ------ diff --git a/aws_lambda_powertools/utilities/feature_flags/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py index c10106275c6..9997d7f3a4d 100644 --- a/aws_lambda_powertools/utilities/feature_flags/base.py +++ b/aws_lambda_powertools/utilities/feature_flags/base.py @@ -5,7 +5,7 @@ class StoreProvider(ABC): @abstractmethod def get_configuration(self) -> Dict[str, Any]: - """Get configuration string from any configuration storing application and return the parsed JSON dictionary + """Get configuration from any store and return the parsed JSON dictionary Raises ------ @@ -16,6 +16,30 @@ def get_configuration(self) -> Dict[str, Any]: ------- Dict[str, Any] parsed JSON dictionary + + **Example** + + ```python + { + "premium_features": { + "default": False, + "rules": { + "customer tier equals premium": { + "when_match": True, + "conditions": [ + { + "action": "EQUALS", + "key": "tier", + "value": "premium", + } + ], + } + }, + }, + "feature_two": { + "default": False + } + } """ return NotImplemented # pragma: no cover diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index f5ca57b8bf3..d55321ecd4e 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -10,17 +10,36 @@ class FeatureFlags: def __init__(self, store: StoreProvider): - """constructor + """Evaluates whether feature flags should be enabled based on a given context. + + It uses the provided store to fetch feature flag rules before evaluating them. + + Examples + -------- + + ```python + from aws_lambda_powertools.utilities.feature_flags import FeatureFlags, AppConfigStore + + app_config = AppConfigStore( + environment="test", + application="powertools", + name="test_conf_name", + cache_seconds=300, + envelope="features" + ) + + feature_flags: FeatureFlags = FeatureFlags(store=app_config) + ``` Parameters ---------- store: StoreProvider - A schema JSON fetcher, can be AWS AppConfig, Hashicorp Consul etc. + Store to use to fetch feature flag schema configuration. """ - self._logger = logger self._store = store - def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: + @staticmethod + def _match_by_action(action: str, condition_value: Any, context_value: Any) -> bool: if not context_value: return False mapping_by_action = { @@ -34,88 +53,91 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any func = mapping_by_action.get(action, lambda a, b: False) return func(context_value, condition_value) except Exception as exc: - self._logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") + logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") return False - def _is_rule_matched( + def _evaluate_conditions( self, rule_name: str, feature_name: str, rule: Dict[str, Any], context: Dict[str, Any] ) -> bool: - rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) + """Evaluates whether context matches conditions, return False otherwise""" + rule_match_value = rule.get(schema.RULE_MATCH_VALUE) conditions = cast(List[Dict], rule.get(schema.CONDITIONS_KEY)) for condition in conditions: context_value = context.get(str(condition.get(schema.CONDITION_KEY))) - if not self._match_by_action( - condition.get(schema.CONDITION_ACTION, ""), - condition.get(schema.CONDITION_VALUE), - context_value, - ): + cond_action = condition.get(schema.CONDITION_ACTION, "") + cond_value = condition.get(schema.CONDITION_VALUE) + + if not self._match_by_action(action=cond_action, condition_value=cond_value, context_value=context_value): logger.debug( - f"rule did not match action, rule_name={rule_name}, rule_default_value={rule_default_value}, " + f"rule did not match action, rule_name={rule_name}, rule_value={rule_match_value}, " f"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}, name={feature_name}" - ) + return False # context doesn't match condition + + logger.debug(f"rule matched, rule_name={rule_name}, rule_value={rule_match_value}, name={feature_name}") return True return False - def _handle_rules( - self, - *, - feature_name: str, - context: Dict[str, Any], - feature_default_value: bool, - rules: Dict[str, Any], + def _evaluate_rules( + self, *, feature_name: str, context: Dict[str, Any], feat_default: bool, rules: Dict[str, Any] ) -> bool: + """Evaluates whether context matches rules and conditions, otherwise return feature default""" for rule_name, rule in rules.items(): - rule_default_value = rule.get(schema.RULE_DEFAULT_VALUE) - if self._is_rule_matched(rule_name=rule_name, feature_name=feature_name, rule=rule, context=context): - return bool(rule_default_value) + rule_match_value = rule.get(schema.RULE_MATCH_VALUE) + + # Context might contain PII data; do not log its value + logger.debug(f"Evaluating rule matching, rule={rule_name}, feature={feature_name}, default={feat_default}") + if self._evaluate_conditions(rule_name=rule_name, feature_name=feature_name, rule=rule, context=context): + return bool(rule_match_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}, " - f"name={feature_name}" - ) - return feature_default_value + logger.debug(f"no rule matched, returning feature default, default={feat_default}, name={feature_name}") + return feat_default return False def get_configuration(self) -> Union[Dict[str, Dict], Dict]: - """Get configuration string from AWs AppConfig and returned the parsed JSON dictionary + """Get validated feature flag schema from configured store. + + Largely used to aid testing, since it's called by `evaluate` and `get_enabled_features` methods. Raises ------ ConfigurationError - Any validation error or appconfig error that can occur + Any validation error Returns ------ Dict[str, Dict] parsed JSON dictionary + **Example** + + ```python { - "my_feature": { - "feature_default_value": True, - "rules": [ - { - "rule_name": "tenant id equals 345345435", - "value_when_applies": False, + "premium_features": { + "default": False, + "rules": { + "customer tier equals premium": { + "when_match": True, "conditions": [ { "action": "EQUALS", - "key": "tenant_id", - "value": "345345435", + "key": "tier", + "value": "premium", } ], - }, - ], + } + }, + }, + "feature_two": { + "default": False } } + ``` """ - # parse result conf as JSON, keep in cache for self.max_age seconds + # parse result conf as JSON, keep in cache for max age defined in store + logger.debug(f"Fetching schema from registered store, store={self._store}") config = self._store.get_configuration() validator = schema.SchemaValidator(schema=config) @@ -124,30 +146,30 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: return config def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, default: bool) -> bool: - """Get a feature toggle boolean value. Value is calculated according to a set of rules and conditions. + """Evaluate whether a feature flag should be enabled according to stored schema and input context + + **Logic when evaluating a feature flag** - See below for explanation. + 1. Feature exists and a rule matches, returns when_match value + 2. Feature exists but has either no rules or no match, return feature default value + 3. Feature doesn't exist in stored schema, encountered an error when fetching -> return default value provided Parameters ---------- name: str - feature name that you wish to fetch + feature name to evaluate 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. + Attributes that should be evaluated against the stored schema. + + for example: `{"tenant_id": "X", "username": "Y", "region": "Z"}` default: bool default value if feature flag doesn't exist in the schema, - or there has been an error while fetching the configuration from appconfig + or there has been an error when fetching the configuration from the store 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 + whether feature should be enabled or not """ if context is None: context = {} @@ -155,37 +177,25 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau try: features = self.get_configuration() except ConfigurationError as err: - logger.debug(f"Unable to get feature toggles JSON, returning provided default value, reason={err}") + logger.debug(f"Failed to fetch feature flags from store, returning default provided, reason={err}") return default feature = features.get(name) if feature is None: - logger.debug( - f"feature does not appear in configuration, using provided default, name={name}, default={default}" - ) + logger.debug(f"Feature not found; returning default provided, name={name}, default={default}") return default rules = feature.get(schema.RULES_KEY) - feature_default_value = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) + feat_default = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) if not rules: - # no rules but value - logger.debug( - f"no rules found, returning feature default value, name={name}, " - f"default_value={feature_default_value}" - ) - return bool(feature_default_value) - - # look for first rule match - logger.debug(f"looking for rule match, name={name}, feature_default_value={feature_default_value}") - return self._handle_rules( - feature_name=name, - context=context, - feature_default_value=bool(feature_default_value), - rules=rules, - ) + logger.debug(f"no rules found, returning feature default, name={name}, default={feat_default}") + return bool(feat_default) + + logger.debug(f"looking for rule match, name={name}, default={feat_default}") + return self._evaluate_rules(feature_name=name, context=context, feat_default=bool(feat_default), rules=rules) def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> List[str]: - """Get all enabled feature toggles while also taking into account rule_context + """Get all enabled feature flags while also taking into account context (when a feature has defined rules) Parameters @@ -197,8 +207,13 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L 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) + list of all feature names that either matches context or have True as default + + **Example** + + ```python + ["premium_features", "my_feature_two", "always_true_feature"] + ``` """ if context is None: context = {} @@ -207,23 +222,20 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L try: features: Dict[str, Any] = self.get_configuration() - except ConfigurationError: - logger.debug("unable to get feature toggles JSON") + except ConfigurationError as err: + logger.debug(f"Failed to fetch feature flags from store, returning empty list, reason={err}") return features_enabled - for feature_name, feature_dict_def in features.items(): - rules = feature_dict_def.get(schema.RULES_KEY, {}) - feature_default_value = feature_dict_def.get(schema.FEATURE_DEFAULT_VAL_KEY) + for name, feature in features.items(): + rules = feature.get(schema.RULES_KEY, {}) + feature_default_value = feature.get(schema.FEATURE_DEFAULT_VAL_KEY) if feature_default_value and not rules: - self._logger.debug(f"feature is enabled by default and has no defined rules, name={feature_name}") - features_enabled.append(feature_name) - elif self._handle_rules( - feature_name=feature_name, - context=context, - feature_default_value=feature_default_value, - rules=rules, + logger.debug(f"feature is enabled by default and has no defined rules, name={name}") + features_enabled.append(name) + elif self._evaluate_rules( + feature_name=name, context=context, feat_default=feature_default_value, rules=rules ): - self._logger.debug(f"feature's calculated value is True, name={feature_name}") - features_enabled.append(feature_name) + logger.debug(f"feature's calculated value is True, name={name}") + features_enabled.append(name) return features_enabled diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index d38e4269625..7c082acd3ed 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -10,7 +10,7 @@ RULES_KEY = "rules" FEATURE_DEFAULT_VAL_KEY = "default" CONDITIONS_KEY = "conditions" -RULE_DEFAULT_VALUE = "when_match" +RULE_MATCH_VALUE = "when_match" CONDITION_KEY = "key" CONDITION_VALUE = "value" CONDITION_ACTION = "action" @@ -28,6 +28,7 @@ def __init__(self, schema: Dict[str, Any]): self.schema = schema def validate(self) -> None: + logger.debug("Validating schema") if not isinstance(self.schema, dict): raise ConfigurationError(f"Features must be a dictionary, schema={str(self.schema)}") @@ -41,6 +42,7 @@ def __init__(self, schema: Dict): def validate(self): for name, feature in self.schema.items(): + logger.debug(f"Attempting to validate feature '{name}'") self.validate_feature(name, feature) rules = RulesValidator(feature=feature) rules.validate() @@ -52,7 +54,7 @@ def validate_feature(name, feature): default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) if default_value is None or not isinstance(default_value, bool): - raise ConfigurationError(f"'feature_default_value' boolean key must be present, feature_name={name}") + raise ConfigurationError(f"feature 'default' boolean key must be present, feature={name}") class RulesValidator(BaseValidator): @@ -67,9 +69,10 @@ def validate(self): return if not isinstance(self.rules, dict): - raise ConfigurationError(f"Feature rules must be a dictionary, feature_name={self.feature_name}") + raise ConfigurationError(f"Feature rules must be a dictionary, feature={self.feature_name}") for rule_name, rule in self.rules.items(): + logger.debug(f"Attempting to validate rule '{rule_name}'") self.validate_rule(rule=rule, rule_name=rule_name, feature_name=self.feature_name) conditions = ConditionsValidator(rule=rule, rule_name=rule_name) conditions.validate() @@ -77,7 +80,7 @@ def validate(self): @staticmethod def validate_rule(rule, rule_name, feature_name): if not rule or not isinstance(rule, dict): - raise ConfigurationError(f"Feature rule must be a dictionary, feature_name={feature_name}") + raise ConfigurationError(f"Feature rule must be a dictionary, feature={feature_name}") RulesValidator.validate_rule_name(rule_name=rule_name, feature_name=feature_name) RulesValidator.validate_rule_default_value(rule=rule, rule_name=rule_name) @@ -85,13 +88,13 @@ def validate_rule(rule, rule_name, feature_name): @staticmethod def validate_rule_name(rule_name: str, feature_name: str): if not rule_name or not isinstance(rule_name, str): - raise ConfigurationError(f"Rule name key must have a non-empty string, feature_name={feature_name}") + raise ConfigurationError(f"Rule name key must have a non-empty string, feature={feature_name}") @staticmethod def validate_rule_default_value(rule: Dict, rule_name: str): - rule_default_value = rule.get(RULE_DEFAULT_VALUE) + rule_default_value = rule.get(RULE_MATCH_VALUE) if not isinstance(rule_default_value, bool): - raise ConfigurationError(f"'rule_default_value' key must have be bool, rule_name={rule_name}") + raise ConfigurationError(f"'rule_default_value' key must have be bool, rule={rule_name}") class ConditionsValidator(BaseValidator): @@ -101,7 +104,7 @@ def __init__(self, rule: Dict[str, Any], rule_name: str): def validate(self): if not self.conditions or not isinstance(self.conditions, list): - raise ConfigurationError(f"Invalid condition, rule_name={self.rule_name}") + raise ConfigurationError(f"Invalid condition, rule={self.rule_name}") for condition in self.conditions: self.validate_condition(rule_name=self.rule_name, condition=condition) @@ -109,8 +112,10 @@ def validate(self): @staticmethod def validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): - raise ConfigurationError(f"Feature rule condition must be a dictionary, rule_name={rule_name}") + raise ConfigurationError(f"Feature rule condition must be a dictionary, rule={rule_name}") + # Condition can contain PII data; do not log condition value + logger.debug(f"Attempting to validate condition for '{rule_name}'") ConditionsValidator.validate_condition_action(condition=condition, rule_name=rule_name) ConditionsValidator.validate_condition_key(condition=condition, rule_name=rule_name) ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @@ -128,10 +133,10 @@ def validate_condition_action(condition: Dict[str, Any], rule_name: str): def validate_condition_key(condition: Dict[str, Any], rule_name: str): key = condition.get(CONDITION_KEY, "") if not key or not isinstance(key, str): - raise ConfigurationError(f"'key' value must be a non empty string, rule_name={rule_name}") + raise ConfigurationError(f"'key' value must be a non empty string, rule={rule_name}") @staticmethod def validate_condition_value(condition: Dict[str, Any], rule_name: str): value = condition.get(CONDITION_VALUE, "") if not value: - raise ConfigurationError(f"'value' key must not be empty, rule_name={rule_name}") + raise ConfigurationError(f"'value' key must not be empty, rule={rule_name}") diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 4cee2b933aa..ae4215203ee 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -26,7 +26,7 @@ def init_feature_flags( application="test_app", name="test_conf_name", cache_seconds=600, - config=config, + sdk_config=config, envelope=envelope, jmespath_options=jmespath_options, ) @@ -42,7 +42,7 @@ def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigSt application="application", name="conf", cache_seconds=1, - config=config, + sdk_config=config, ) @@ -447,8 +447,10 @@ def test_is_rule_matched_no_matches(mocker, config): rules_context = {} feature_flags = init_feature_flags(mocker, {}, config) - # WHEN calling _is_rule_matched - result = feature_flags._is_rule_matched(rule_name="dummy", feature_name="dummy", rule=rule, context=rules_context) + # WHEN calling _evaluate_conditions + result = feature_flags._evaluate_conditions( + rule_name="dummy", feature_name="dummy", rule=rule, context=rules_context + ) # THEN return False assert result is False diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index a66cba5ad12..e68b7e51ec8 100644 --- a/tests/functional/feature_toggles/test_schema_validation.py +++ b/tests/functional/feature_toggles/test_schema_validation.py @@ -9,7 +9,7 @@ CONDITION_VALUE, CONDITIONS_KEY, FEATURE_DEFAULT_VAL_KEY, - RULE_DEFAULT_VALUE, + RULE_MATCH_VALUE, RULES_KEY, ConditionsValidator, RuleAction, @@ -76,13 +76,13 @@ def test_invalid_rule(): with pytest.raises(ConfigurationError): validator.validate() - # rules RULE_DEFAULT_VALUE is not bool + # rules RULE_MATCH_VALUE is not bool schema = { "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: "False", + RULE_MATCH_VALUE: "False", } }, } @@ -97,7 +97,7 @@ def test_invalid_rule(): FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, + RULE_MATCH_VALUE: False, } }, } @@ -111,7 +111,7 @@ def test_invalid_rule(): "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { - "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: []}, + "tenant id equals 345345435": {RULE_MATCH_VALUE: False, CONDITIONS_KEY: []}, }, } } @@ -124,7 +124,7 @@ def test_invalid_rule(): "my_feature": { FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { - "tenant id equals 345345435": {RULE_DEFAULT_VALUE: False, CONDITIONS_KEY: {}}, + "tenant id equals 345345435": {RULE_MATCH_VALUE: False, CONDITIONS_KEY: {}}, }, } } @@ -140,7 +140,7 @@ def test_invalid_condition(): FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, + RULE_MATCH_VALUE: False, CONDITIONS_KEY: {CONDITION_ACTION: "stuff", CONDITION_KEY: "a", CONDITION_VALUE: "a"}, } }, @@ -156,7 +156,7 @@ def test_invalid_condition(): FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, + RULE_MATCH_VALUE: False, CONDITIONS_KEY: {CONDITION_ACTION: RuleAction.EQUALS.value}, } }, @@ -172,7 +172,7 @@ def test_invalid_condition(): FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 345345435": { - RULE_DEFAULT_VALUE: False, + RULE_MATCH_VALUE: False, CONDITIONS_KEY: { CONDITION_ACTION: RuleAction.EQUALS.value, CONDITION_KEY: 5, @@ -193,7 +193,7 @@ def test_valid_condition_all_actions(): FEATURE_DEFAULT_VAL_KEY: False, RULES_KEY: { "tenant id equals 645654 and username is a": { - RULE_DEFAULT_VALUE: True, + RULE_MATCH_VALUE: True, CONDITIONS_KEY: [ { CONDITION_ACTION: RuleAction.EQUALS.value, From b61cafa638c253e54ad20b4730e5652adf0d0a15 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 18:44:12 +0200 Subject: [PATCH 39/45] docs: remove changes; add in PR description --- docs/utilities/feature_flags.md | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 93c63544085..b450d45806c 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -55,21 +55,3 @@ By default, this utility provides AWS AppConfig as a configuration store. As suc ## Testing your code > NOTE: Share example on how customers can unit test their feature flags - -## Changes - -Potential changes to be validated when docs are in a better shape - -- [x] ~~`rules_context` to `context`~~ -- [x] `ConfigurationStore` to `FeatureFlags` -- [x] `StoreProvider` to `StoreProvider` -- [x] ~~Use `base.py` for interfaces for consistency (e.g. Metrics, Tracer, etc.)~~ -- [x] ~~AppConfig construct parameter names for consistency (e.g. `configuration_name` -> `name`, `service` -> `application`)~~ -- [x] ~~Rename `value_if_missing` param to `default` to match Python consistency (e.g. `os.getenv("VAR", default=False)`)~~ -- [x] ~~Rename `get_feature` to `evaluate`~~ -- [x] ~~Rename `get_all_enabled_feature_toggles` to `get_enabled_features`~~ -- [x] ~~Review redundant logging~~ -- [ ] Some docstrings and logger refer to AWS AppConfig only (outdated given StoreProvider) -- [ ] Review why we're testing a private method(`is_rule_matched`) -- [ ] Review `get_configuration`, `get_json_configuration` -- [ ] Consider fine grained ConfigurationError instead of a single ConfigurationError From e9bb1902ee1845b617638cd9ad629ddb3c6c2ac7 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 19:14:55 +0200 Subject: [PATCH 40/45] refactor(schema): add docstrings and schema specification --- .../utilities/feature_flags/schema.py | 81 ++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 7c082acd3ed..00467d65faf 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -23,7 +23,82 @@ class RuleAction(str, Enum): CONTAINS = "CONTAINS" -class SchemaValidator: +class SchemaValidator(BaseValidator): + """Validates feature flag schema configuration + + Schema + ------ + + **Feature object** + + A dictionary containing default value and rules for matching. + The value MUST be an object and MIGHT contain the following members: + + * **default**: `bool`. Defines default feature value. This MUST be present + * **rules**: `Dict[str, Dict]`. Rules object. This MIGHT be present + + ```json + { + "my_feature": { + "default": True, + "rules": {} + } + } + ``` + + **Rules object** + + A dictionary with each rule and their conditions that a feature might have. + The value MIGHT be present, and when defined it MUST contain the following members: + + * **when_match**: `bool`. Defines value to return when context matches conditions + * **conditions**: `List[Dict]`. Conditions object. This MUST be present + + ```json + { + "my_feature": { + "default": True, + "rules": { + "tenant id equals 345345435": { + "when_match": False, + "conditions": [] + } + } + } + } + ``` + + **Conditions object** + + A list of dictionaries containing conditions for a given rule. + The value MUST contain the following members: + + * **action**: `str`. Operation to perform to match a key and value. + The value MUST be either EQUALS, STARTSWITH, ENDSWITH, CONTAINS + * **key**: `str`. Key in given context to perform operation + * **value**: `Any`. Value in given context that should match action operation. + + ```json + { + "my_feature": { + "default": True, + "rules": { + "tenant id equals 345345435": { + "when_match": False, + "conditions": [ + { + "action": "EQUALS", + "key": "tenant_id", + "value": "345345435", + } + ] + } + } + } + } + ``` + """ + def __init__(self, schema: Dict[str, Any]): self.schema = schema @@ -37,6 +112,8 @@ def validate(self) -> None: class FeaturesValidator(BaseValidator): + """Validates each feature and calls RulesValidator to validate its rules""" + def __init__(self, schema: Dict): self.schema = schema @@ -58,6 +135,8 @@ def validate_feature(name, feature): class RulesValidator(BaseValidator): + """Validates each rule and calls ConditionsValidator to validate each rule's conditions""" + def __init__(self, feature: Dict[str, Any]): self.feature = feature self.feature_name = next(iter(self.feature)) From a55131f3f54dc3cffc5941c0dbc3acbfadce20a2 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 20:03:36 +0200 Subject: [PATCH 41/45] refactor: add SchemaValidationError --- .../utilities/feature_flags/exceptions.py | 6 ++- .../utilities/feature_flags/feature_flags.py | 12 +++++ .../utilities/feature_flags/schema.py | 31 +++++++------ .../feature_toggles/test_feature_toggles.py | 9 ++-- .../feature_toggles/test_schema_validation.py | 44 +++++++++---------- 5 files changed, 61 insertions(+), 41 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/exceptions.py b/aws_lambda_powertools/utilities/feature_flags/exceptions.py index d87f9a39dec..9b43744562e 100644 --- a/aws_lambda_powertools/utilities/feature_flags/exceptions.py +++ b/aws_lambda_powertools/utilities/feature_flags/exceptions.py @@ -1,2 +1,6 @@ class ConfigurationError(Exception): - """When a a configuration store raises an exception on config retrieval or parsing""" + """When a configuration store raises an exception on config retrieval or parsing""" + + +class SchemaValidationError(Exception): + """When feature flag schema fails validation""" diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index d55321ecd4e..c2f698f4f84 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -105,6 +105,8 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: ------ ConfigurationError Any validation error + SchemaValidationError + When schema doesn't conform with feature flag schema Returns ------ @@ -170,6 +172,11 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau ------ bool whether feature should be enabled or not + + Raises + ------ + SchemaValidationError + When schema doesn't conform with feature flag schema """ if context is None: context = {} @@ -214,6 +221,11 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L ```python ["premium_features", "my_feature_two", "always_true_feature"] ``` + + Raises + ------ + SchemaValidationError + When schema doesn't conform with feature flag schema """ if context is None: context = {} diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 00467d65faf..3de7ac22363 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -3,7 +3,7 @@ from typing import Any, Dict, List, Optional from .base import BaseValidator -from .exceptions import ConfigurationError +from .exceptions import SchemaValidationError logger = logging.getLogger(__name__) @@ -26,6 +26,11 @@ class RuleAction(str, Enum): class SchemaValidator(BaseValidator): """Validates feature flag schema configuration + Raises + ------ + SchemaValidationError + When schema doesn't conform with feature flag schema + Schema ------ @@ -105,7 +110,7 @@ def __init__(self, schema: Dict[str, Any]): def validate(self) -> None: logger.debug("Validating schema") if not isinstance(self.schema, dict): - raise ConfigurationError(f"Features must be a dictionary, schema={str(self.schema)}") + raise SchemaValidationError(f"Features must be a dictionary, schema={str(self.schema)}") features = FeaturesValidator(schema=self.schema) features.validate() @@ -127,11 +132,11 @@ def validate(self): @staticmethod def validate_feature(name, feature): if not feature or not isinstance(feature, dict): - raise ConfigurationError(f"Feature must be a non-empty dictionary, feature={name}") + raise SchemaValidationError(f"Feature must be a non-empty dictionary, feature={name}") default_value = feature.get(FEATURE_DEFAULT_VAL_KEY) if default_value is None or not isinstance(default_value, bool): - raise ConfigurationError(f"feature 'default' boolean key must be present, feature={name}") + raise SchemaValidationError(f"feature 'default' boolean key must be present, feature={name}") class RulesValidator(BaseValidator): @@ -148,7 +153,7 @@ def validate(self): return if not isinstance(self.rules, dict): - raise ConfigurationError(f"Feature rules must be a dictionary, feature={self.feature_name}") + raise SchemaValidationError(f"Feature rules must be a dictionary, feature={self.feature_name}") for rule_name, rule in self.rules.items(): logger.debug(f"Attempting to validate rule '{rule_name}'") @@ -159,7 +164,7 @@ def validate(self): @staticmethod def validate_rule(rule, rule_name, feature_name): if not rule or not isinstance(rule, dict): - raise ConfigurationError(f"Feature rule must be a dictionary, feature={feature_name}") + raise SchemaValidationError(f"Feature rule must be a dictionary, feature={feature_name}") RulesValidator.validate_rule_name(rule_name=rule_name, feature_name=feature_name) RulesValidator.validate_rule_default_value(rule=rule, rule_name=rule_name) @@ -167,13 +172,13 @@ def validate_rule(rule, rule_name, feature_name): @staticmethod def validate_rule_name(rule_name: str, feature_name: str): if not rule_name or not isinstance(rule_name, str): - raise ConfigurationError(f"Rule name key must have a non-empty string, feature={feature_name}") + raise SchemaValidationError(f"Rule name key must have a non-empty string, feature={feature_name}") @staticmethod def validate_rule_default_value(rule: Dict, rule_name: str): rule_default_value = rule.get(RULE_MATCH_VALUE) if not isinstance(rule_default_value, bool): - raise ConfigurationError(f"'rule_default_value' key must have be bool, rule={rule_name}") + raise SchemaValidationError(f"'rule_default_value' key must have be bool, rule={rule_name}") class ConditionsValidator(BaseValidator): @@ -183,7 +188,7 @@ def __init__(self, rule: Dict[str, Any], rule_name: str): def validate(self): if not self.conditions or not isinstance(self.conditions, list): - raise ConfigurationError(f"Invalid condition, rule={self.rule_name}") + raise SchemaValidationError(f"Invalid condition, rule={self.rule_name}") for condition in self.conditions: self.validate_condition(rule_name=self.rule_name, condition=condition) @@ -191,7 +196,7 @@ def validate(self): @staticmethod def validate_condition(rule_name: str, condition: Dict[str, str]) -> None: if not condition or not isinstance(condition, dict): - raise ConfigurationError(f"Feature rule condition must be a dictionary, rule={rule_name}") + raise SchemaValidationError(f"Feature rule condition must be a dictionary, rule={rule_name}") # Condition can contain PII data; do not log condition value logger.debug(f"Attempting to validate condition for '{rule_name}'") @@ -204,7 +209,7 @@ def validate_condition_action(condition: Dict[str, Any], rule_name: str): action = condition.get(CONDITION_ACTION, "") if action not in RuleAction.__members__: allowed_values = [_action.value for _action in RuleAction] - raise ConfigurationError( + raise SchemaValidationError( f"'action' value must be either {allowed_values}, rule_name={rule_name}, action={action}" ) @@ -212,10 +217,10 @@ def validate_condition_action(condition: Dict[str, Any], rule_name: str): def validate_condition_key(condition: Dict[str, Any], rule_name: str): key = condition.get(CONDITION_KEY, "") if not key or not isinstance(key, str): - raise ConfigurationError(f"'key' value must be a non empty string, rule={rule_name}") + raise SchemaValidationError(f"'key' value must be a non empty string, rule={rule_name}") @staticmethod def validate_condition_value(condition: Dict[str, Any], rule_name: str): value = condition.get(CONDITION_VALUE, "") if not value: - raise ConfigurationError(f"'value' key must not be empty, rule={rule_name}") + raise SchemaValidationError(f"'value' key must not be empty, rule={rule_name}") diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index ae4215203ee..752ad5f5ede 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -291,9 +291,8 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): mocked_app_config_schema = { "my_feature": { "default": expected_value, - "rules": [ - { - "rule_name": "tenant id is contained in [8, 2]", + "rules": { + "tenant id is contained in [8, 2]": { "when_match": True, "conditions": [ { @@ -302,8 +301,8 @@ def test_toggles_no_match_rule_with_contains_action(mocker, config): "value": ["8", "2"], } ], - }, - ], + } + }, } } feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_toggles/test_schema_validation.py index e68b7e51ec8..2c33d3c61cc 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_flags.exceptions import ConfigurationError +from aws_lambda_powertools.utilities.feature_flags.exceptions import SchemaValidationError from aws_lambda_powertools.utilities.feature_flags.schema import ( CONDITION_ACTION, CONDITION_KEY, @@ -24,7 +24,7 @@ def test_invalid_features_dict(): validator = SchemaValidator(schema=[]) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() @@ -45,7 +45,7 @@ def test_empty_features_not_fail(): ) def test_invalid_feature(schema): validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() @@ -73,7 +73,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # rules RULE_MATCH_VALUE is not bool @@ -88,7 +88,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # missing conditions list @@ -103,7 +103,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # condition list is empty @@ -116,7 +116,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # condition is invalid type, not list @@ -129,7 +129,7 @@ def test_invalid_rule(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() @@ -147,7 +147,7 @@ def test_invalid_condition(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # missing condition key and value @@ -163,7 +163,7 @@ def test_invalid_condition(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() # invalid condition key type, not string @@ -183,7 +183,7 @@ def test_invalid_condition(): } } validator = SchemaValidator(schema) - with pytest.raises(ConfigurationError): + with pytest.raises(SchemaValidationError): validator.validate() @@ -229,8 +229,8 @@ def test_validate_condition_invalid_condition_type(): condition = {} # WHEN calling validate_condition - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Feature rule condition must be a dictionary"): + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="Feature rule condition must be a dictionary"): ConditionsValidator.validate_condition(condition=condition, rule_name="dummy") @@ -239,8 +239,8 @@ def test_validate_condition_invalid_condition_action(): condition = {"action": "INVALID", "key": "tenant_id", "value": "12345"} # WHEN calling validate_condition - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="'action' value must be either"): + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="'action' value must be either"): ConditionsValidator.validate_condition_action(condition=condition, rule_name="dummy") @@ -249,8 +249,8 @@ def test_validate_condition_invalid_condition_key(): condition = {"action": RuleAction.EQUALS.value, "value": "12345"} # WHEN calling validate_condition - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="'key' value must be a non empty string"): + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="'key' value must be a non empty string"): ConditionsValidator.validate_condition_key(condition=condition, rule_name="dummy") @@ -262,21 +262,21 @@ def test_validate_condition_missing_condition_value(): } # WHEN calling validate_condition - with pytest.raises(ConfigurationError, match="'value' key must not be empty"): + with pytest.raises(SchemaValidationError, match="'value' key must not be empty"): ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy") def test_validate_rule_invalid_rule_type(): # GIVEN an invalid rule type of empty list # WHEN calling validate_rule - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Feature rule must be a dictionary"): + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="Feature rule must be a dictionary"): RulesValidator.validate_rule(rule=[], rule_name="dummy", feature_name="dummy") def test_validate_rule_invalid_rule_name(): # GIVEN a rule name is empty # WHEN calling validate_rule_name - # THEN raise ConfigurationError - with pytest.raises(ConfigurationError, match="Rule name key must have a non-empty string"): + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="Rule name key must have a non-empty string"): RulesValidator.validate_rule_name(rule_name="", feature_name="dummy") From e0ab7a1918b44021b386679d886e848ffd6ecc76 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 3 Aug 2021 20:11:08 +0200 Subject: [PATCH 42/45] refactor: rename ConfigurationError to ConfigurationStoreError, impro catch --- .../utilities/feature_flags/__init__.py | 4 ++-- .../utilities/feature_flags/appconfig.py | 6 +++--- .../utilities/feature_flags/base.py | 2 +- .../utilities/feature_flags/exceptions.py | 2 +- .../utilities/feature_flags/feature_flags.py | 15 +++++++++------ .../feature_toggles/test_feature_toggles.py | 10 +++++----- 6 files changed, 21 insertions(+), 18 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/__init__.py b/aws_lambda_powertools/utilities/feature_flags/__init__.py index 96e3ccee0e1..1edbea18dee 100644 --- a/aws_lambda_powertools/utilities/feature_flags/__init__.py +++ b/aws_lambda_powertools/utilities/feature_flags/__init__.py @@ -2,12 +2,12 @@ """ from .appconfig import AppConfigStore from .base import StoreProvider -from .exceptions import ConfigurationError +from .exceptions import ConfigurationStoreError from .feature_flags import FeatureFlags from .schema import RuleAction, SchemaValidator __all__ = [ - "ConfigurationError", + "ConfigurationStoreError", "FeatureFlags", "RuleAction", "SchemaValidator", diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index 411683d649b..ea3f5ddcd8c 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -7,7 +7,7 @@ from ...shared import jmespath_utils from .base import StoreProvider -from .exceptions import ConfigurationError +from .exceptions import ConfigurationStoreError logger = logging.getLogger(__name__) @@ -59,7 +59,7 @@ def get_configuration(self) -> Dict[str, Any]: Raises ------ - ConfigurationError + ConfigurationStoreError Any validation error or AppConfig error that can occur Returns @@ -82,4 +82,4 @@ def get_configuration(self) -> Dict[str, Any]: return cast(dict, config) except (GetParameterError, TransformParameterError) as exc: - raise ConfigurationError("Unable to get AWS AppConfig configuration file") from exc + raise ConfigurationStoreError("Unable to get AWS AppConfig configuration file") from exc diff --git a/aws_lambda_powertools/utilities/feature_flags/base.py b/aws_lambda_powertools/utilities/feature_flags/base.py index 9997d7f3a4d..1df90f19ac8 100644 --- a/aws_lambda_powertools/utilities/feature_flags/base.py +++ b/aws_lambda_powertools/utilities/feature_flags/base.py @@ -9,7 +9,7 @@ def get_configuration(self) -> Dict[str, Any]: Raises ------ - ConfigurationError + ConfigurationStoreError Any error that can occur during schema fetch or JSON parse Returns diff --git a/aws_lambda_powertools/utilities/feature_flags/exceptions.py b/aws_lambda_powertools/utilities/feature_flags/exceptions.py index 9b43744562e..6fb0d6a69bf 100644 --- a/aws_lambda_powertools/utilities/feature_flags/exceptions.py +++ b/aws_lambda_powertools/utilities/feature_flags/exceptions.py @@ -1,4 +1,4 @@ -class ConfigurationError(Exception): +class ConfigurationStoreError(Exception): """When a configuration store raises an exception on config retrieval or parsing""" diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index c2f698f4f84..23fbcfcd9da 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -3,7 +3,7 @@ from . import schema from .base import StoreProvider -from .exceptions import ConfigurationError +from .exceptions import ConfigurationStoreError logger = logging.getLogger(__name__) @@ -103,8 +103,8 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: Raises ------ - ConfigurationError - Any validation error + ConfigurationStoreError + Any propagated error from store SchemaValidationError When schema doesn't conform with feature flag schema @@ -140,7 +140,10 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: """ # parse result conf as JSON, keep in cache for max age defined in store logger.debug(f"Fetching schema from registered store, store={self._store}") - config = self._store.get_configuration() + try: + config = self._store.get_configuration() + except Exception as err: + raise ConfigurationStoreError("Unable to fetch schema from registered store") from err validator = schema.SchemaValidator(schema=config) validator.validate() @@ -183,7 +186,7 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau try: features = self.get_configuration() - except ConfigurationError as err: + except ConfigurationStoreError as err: logger.debug(f"Failed to fetch feature flags from store, returning default provided, reason={err}") return default @@ -234,7 +237,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L try: features: Dict[str, Any] = self.get_configuration() - except ConfigurationError as err: + except ConfigurationStoreError as err: logger.debug(f"Failed to fetch feature flags from store, returning empty list, reason={err}") return features_enabled diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 752ad5f5ede..00b037cbdd4 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -3,7 +3,7 @@ import pytest from botocore.config import Config -from aws_lambda_powertools.utilities.feature_flags import ConfigurationError, schema +from aws_lambda_powertools.utilities.feature_flags import ConfigurationStoreError, schema from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags from aws_lambda_powertools.utilities.feature_flags.schema import RuleAction @@ -387,7 +387,7 @@ def test_multiple_features_only_some_enabled(mocker, config): def test_get_feature_toggle_handles_error(mocker, config): - # GIVEN a schema fetch that raises a ConfigurationError + # GIVEN a schema fetch that raises a ConfigurationStoreError schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) feature_flags = FeatureFlags(schema_fetcher) @@ -399,7 +399,7 @@ def test_get_feature_toggle_handles_error(mocker, config): def test_get_all_enabled_feature_toggles_handles_error(mocker, config): - # GIVEN a schema fetch that raises a ConfigurationError + # GIVEN a schema fetch that raises a ConfigurationStoreError schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) feature_flags = FeatureFlags(schema_fetcher) @@ -415,10 +415,10 @@ def test_app_config_get_parameter_err(mocker, config): app_conf_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) # WHEN calling get_configuration - with pytest.raises(ConfigurationError) as err: + with pytest.raises(ConfigurationStoreError) as err: app_conf_fetcher.get_configuration() - # THEN raise ConfigurationError error + # THEN raise ConfigurationStoreError error assert "AWS AppConfig configuration" in str(err.value) From d8125c732765eb4167d0ac4a8677791ffc1a42d7 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Wed, 4 Aug 2021 20:25:28 +0200 Subject: [PATCH 43/45] test: conditional dict values --- .../feature_toggles/test_feature_toggles.py | 37 ++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-) diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_toggles/test_feature_toggles.py index 00b037cbdd4..6ceefc7f500 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_toggles/test_feature_toggles.py @@ -6,7 +6,16 @@ from aws_lambda_powertools.utilities.feature_flags import ConfigurationStoreError, schema from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags -from aws_lambda_powertools.utilities.feature_flags.schema import RuleAction +from aws_lambda_powertools.utilities.feature_flags.schema import ( + CONDITION_ACTION, + CONDITION_KEY, + CONDITION_VALUE, + CONDITIONS_KEY, + FEATURE_DEFAULT_VAL_KEY, + RULE_MATCH_VALUE, + RULES_KEY, + RuleAction, +) from aws_lambda_powertools.utilities.parameters import GetParameterError @@ -461,3 +470,29 @@ def test_features_jmespath_envelope(mocker, config): feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config, envelope="features") toggle = feature_flags.evaluate(name="my_feature", context={}, default=False) assert toggle == expected_value + + +# test_match_rule_with_contains_action +def test_match_condition_with_dict_value(mocker, config): + expected_value = True + mocked_app_config_schema = { + "my_feature": { + FEATURE_DEFAULT_VAL_KEY: False, + RULES_KEY: { + "tenant id is 6 and username is lessa": { + RULE_MATCH_VALUE: expected_value, + CONDITIONS_KEY: [ + { + CONDITION_ACTION: RuleAction.EQUALS.value, + CONDITION_KEY: "tenant", + CONDITION_VALUE: {"tenant_id": "6", "username": "lessa"}, + } + ], + } + }, + } + } + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + ctx = {"tenant": {"tenant_id": "6", "username": "lessa"}} + toggle = feature_flags.evaluate(name="my_feature", context=ctx, default=False) + assert toggle == expected_value From 1b202be22e6a3462b7cd6d2273c4f7106ec4d09d Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Wed, 4 Aug 2021 20:38:18 +0200 Subject: [PATCH 44/45] refactor: rename toggles to flags --- .pre-commit-config.yaml | 2 +- .../utilities/feature_flags/__init__.py | 3 +- .../utilities/feature_flags/appconfig.py | 13 +++-- .../__init__.py | 0 .../test_feature_flags.py} | 49 ++++++++----------- .../test_schema_validation.py | 0 6 files changed, 30 insertions(+), 37 deletions(-) rename tests/functional/{feature_toggles => feature_flags}/__init__.py (100%) rename tests/functional/{feature_toggles/test_feature_toggles.py => feature_flags/test_feature_flags.py} (92%) rename tests/functional/{feature_toggles => feature_flags}/test_schema_validation.py (100%) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index eabc7a65a10..f42337d5c5b 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -24,7 +24,7 @@ repos: types: [python] - id: isort name: formatting::isort - entry: poetry run isort -rc + entry: poetry run isort language: system types: [python] - repo: local diff --git a/aws_lambda_powertools/utilities/feature_flags/__init__.py b/aws_lambda_powertools/utilities/feature_flags/__init__.py index 1edbea18dee..db7dfca5b57 100644 --- a/aws_lambda_powertools/utilities/feature_flags/__init__.py +++ b/aws_lambda_powertools/utilities/feature_flags/__init__.py @@ -1,5 +1,4 @@ -"""Advanced feature toggles utility -""" +"""Advanced feature flags utility""" from .appconfig import AppConfigStore from .base import StoreProvider from .exceptions import ConfigurationStoreError diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index ea3f5ddcd8c..6dd62926ee1 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -69,10 +69,13 @@ def get_configuration(self) -> Dict[str, Any]: """ try: # parse result conf as JSON, keep in cache for self.max_age seconds - config = self._conf_store.get( - name=self.name, - transform=TRANSFORM_TYPE, - max_age=self.cache_seconds, + config = cast( + dict, + self._conf_store.get( + name=self.name, + transform=TRANSFORM_TYPE, + max_age=self.cache_seconds, + ), ) if self.envelope: @@ -80,6 +83,6 @@ def get_configuration(self) -> Dict[str, Any]: data=config, envelope=self.envelope, jmespath_options=self.jmespath_options ) - return cast(dict, config) + return config except (GetParameterError, TransformParameterError) as exc: raise ConfigurationStoreError("Unable to get AWS AppConfig configuration file") from exc diff --git a/tests/functional/feature_toggles/__init__.py b/tests/functional/feature_flags/__init__.py similarity index 100% rename from tests/functional/feature_toggles/__init__.py rename to tests/functional/feature_flags/__init__.py diff --git a/tests/functional/feature_toggles/test_feature_toggles.py b/tests/functional/feature_flags/test_feature_flags.py similarity index 92% rename from tests/functional/feature_toggles/test_feature_toggles.py rename to tests/functional/feature_flags/test_feature_flags.py index 6ceefc7f500..5abd9e49fec 100644 --- a/tests/functional/feature_toggles/test_feature_toggles.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -6,16 +6,7 @@ from aws_lambda_powertools.utilities.feature_flags import ConfigurationStoreError, schema from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags -from aws_lambda_powertools.utilities.feature_flags.schema import ( - CONDITION_ACTION, - CONDITION_KEY, - CONDITION_VALUE, - CONDITIONS_KEY, - FEATURE_DEFAULT_VAL_KEY, - RULE_MATCH_VALUE, - RULES_KEY, - RuleAction, -) +from aws_lambda_powertools.utilities.feature_flags.schema import RuleAction from aws_lambda_powertools.utilities.parameters import GetParameterError @@ -57,7 +48,7 @@ def init_fetcher_side_effect(mocker, config: Config, side_effect) -> AppConfigSt # this test checks that we get correct value of feature that exists in the schema. # we also don't send an empty context dict in this case -def test_toggles_rule_does_not_match(mocker, config): +def test_flags_rule_does_not_match(mocker, config): expected_value = True mocked_app_config_schema = { "my_feature": { @@ -84,7 +75,7 @@ def test_toggles_rule_does_not_match(mocker, config): # this test checks that if you try to get a feature that doesn't exist in the schema, # you get the default value of False that was sent to the evaluate API -def test_toggles_no_conditions_feature_does_not_exist(mocker, config): +def test_flags_no_conditions_feature_does_not_exist(mocker, config): expected_value = False mocked_app_config_schema = {"my_fake_feature": {"default": True}} @@ -95,7 +86,7 @@ def test_toggles_no_conditions_feature_does_not_exist(mocker, config): # check that feature match works when they are no rules and we send context. # default value is False but the feature has a True default_value. -def test_toggles_no_rules(mocker, config): +def test_flags_no_rules(mocker, config): expected_value = True mocked_app_config_schema = {"my_feature": {"default": expected_value}} feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) @@ -104,7 +95,7 @@ def test_toggles_no_rules(mocker, config): # check a case where the feature exists but the rule doesn't match so we revert to the default value of the feature -def test_toggles_conditions_no_match(mocker, config): +def test_flags_conditions_no_match(mocker, config): expected_value = True mocked_app_config_schema = { "my_feature": { @@ -129,7 +120,7 @@ def test_toggles_conditions_no_match(mocker, config): # check that a rule can match when it has multiple conditions, see rule name for further explanation -def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config): +def test_flags_conditions_rule_match_equal_multiple_conditions(mocker, config): expected_value = False tenant_id_val = "6" username_val = "a" @@ -170,7 +161,7 @@ def test_toggles_conditions_rule_match_equal_multiple_conditions(mocker, config) # check a case when rule doesn't match and it has multiple conditions, # different tenant id causes the rule to not match. # default value of the feature in this case is True -def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, config): +def test_flags_conditions_no_rule_match_equal_multiple_conditions(mocker, config): expected_val = True mocked_app_config_schema = { "my_feature": { @@ -201,7 +192,7 @@ def test_toggles_conditions_no_rule_match_equal_multiple_conditions(mocker, conf # check rule match for multiple of action types -def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_conditions(mocker, config): +def test_flags_conditions_rule_match_multiple_actions_multiple_rules_multiple_conditions(mocker, config): expected_value_first_check = True expected_value_second_check = False expected_value_third_check = False @@ -271,7 +262,7 @@ def test_toggles_conditions_rule_match_multiple_actions_multiple_rules_multiple_ # check a case where the feature exists but the rule doesn't match so we revert to the default value of the feature -def test_toggles_match_rule_with_contains_action(mocker, config): +def test_flags_match_rule_with_contains_action(mocker, config): expected_value = True mocked_app_config_schema = { "my_feature": { @@ -295,7 +286,7 @@ def test_toggles_match_rule_with_contains_action(mocker, config): assert toggle == expected_value -def test_toggles_no_match_rule_with_contains_action(mocker, config): +def test_flags_no_match_rule_with_contains_action(mocker, config): expected_value = False mocked_app_config_schema = { "my_feature": { @@ -407,16 +398,16 @@ def test_get_feature_toggle_handles_error(mocker, config): assert toggle is False -def test_get_all_enabled_feature_toggles_handles_error(mocker, config): +def test_get_all_enabled_feature_flags_handles_error(mocker, config): # GIVEN a schema fetch that raises a ConfigurationStoreError schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError()) feature_flags = FeatureFlags(schema_fetcher) # WHEN calling get_enabled_features - toggles = feature_flags.get_enabled_features(context=None) + flags = feature_flags.get_enabled_features(context=None) # THEN handle the error and return an empty list - assert toggles == [] + assert flags == [] def test_app_config_get_parameter_err(mocker, config): @@ -477,15 +468,15 @@ def test_match_condition_with_dict_value(mocker, config): expected_value = True mocked_app_config_schema = { "my_feature": { - FEATURE_DEFAULT_VAL_KEY: False, - RULES_KEY: { + "default": False, + "rules": { "tenant id is 6 and username is lessa": { - RULE_MATCH_VALUE: expected_value, - CONDITIONS_KEY: [ + "when_match": expected_value, + "conditions": [ { - CONDITION_ACTION: RuleAction.EQUALS.value, - CONDITION_KEY: "tenant", - CONDITION_VALUE: {"tenant_id": "6", "username": "lessa"}, + "action": RuleAction.EQUALS.value, + "key": "tenant", + "value": {"tenant_id": "6", "username": "lessa"}, } ], } diff --git a/tests/functional/feature_toggles/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py similarity index 100% rename from tests/functional/feature_toggles/test_schema_validation.py rename to tests/functional/feature_flags/test_schema_validation.py From a0ae9da484ed5d24ebcefdddc5e05c3d5392841d Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Wed, 4 Aug 2021 22:14:27 +0200 Subject: [PATCH 45/45] fix: propagate access denied errors --- .../utilities/feature_flags/appconfig.py | 6 +++++- .../utilities/feature_flags/exceptions.py | 7 +++++++ .../utilities/feature_flags/feature_flags.py | 6 +----- .../functional/feature_flags/test_feature_flags.py | 14 ++++++++++++++ 4 files changed, 27 insertions(+), 6 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/appconfig.py b/aws_lambda_powertools/utilities/feature_flags/appconfig.py index 6dd62926ee1..6c075eac1a1 100644 --- a/aws_lambda_powertools/utilities/feature_flags/appconfig.py +++ b/aws_lambda_powertools/utilities/feature_flags/appconfig.py @@ -1,4 +1,5 @@ import logging +import traceback from typing import Any, Dict, Optional, cast from botocore.config import Config @@ -7,7 +8,7 @@ from ...shared import jmespath_utils from .base import StoreProvider -from .exceptions import ConfigurationStoreError +from .exceptions import ConfigurationStoreError, StoreClientError logger = logging.getLogger(__name__) @@ -85,4 +86,7 @@ def get_configuration(self) -> Dict[str, Any]: return config except (GetParameterError, TransformParameterError) as exc: + err_msg = traceback.format_exc() + if "AccessDenied" in err_msg: + raise StoreClientError(err_msg) from exc raise ConfigurationStoreError("Unable to get AWS AppConfig configuration file") from exc diff --git a/aws_lambda_powertools/utilities/feature_flags/exceptions.py b/aws_lambda_powertools/utilities/feature_flags/exceptions.py index 6fb0d6a69bf..eaea6c61cca 100644 --- a/aws_lambda_powertools/utilities/feature_flags/exceptions.py +++ b/aws_lambda_powertools/utilities/feature_flags/exceptions.py @@ -4,3 +4,10 @@ class ConfigurationStoreError(Exception): class SchemaValidationError(Exception): """When feature flag schema fails validation""" + + +class StoreClientError(Exception): + """When a store raises an exception that should be propagated to the client to fix + + For example, Access Denied errors when the client doesn't permissions to fetch config + """ diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 23fbcfcd9da..a862baf61c2 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -140,11 +140,7 @@ def get_configuration(self) -> Union[Dict[str, Dict], Dict]: """ # parse result conf as JSON, keep in cache for max age defined in store logger.debug(f"Fetching schema from registered store, store={self._store}") - try: - config = self._store.get_configuration() - except Exception as err: - raise ConfigurationStoreError("Unable to fetch schema from registered store") from err - + config = self._store.get_configuration() validator = schema.SchemaValidator(schema=config) validator.validate() diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 5abd9e49fec..d2150268062 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -5,6 +5,7 @@ from aws_lambda_powertools.utilities.feature_flags import ConfigurationStoreError, schema from aws_lambda_powertools.utilities.feature_flags.appconfig import AppConfigStore +from aws_lambda_powertools.utilities.feature_flags.exceptions import StoreClientError from aws_lambda_powertools.utilities.feature_flags.feature_flags import FeatureFlags from aws_lambda_powertools.utilities.feature_flags.schema import RuleAction from aws_lambda_powertools.utilities.parameters import GetParameterError @@ -487,3 +488,16 @@ def test_match_condition_with_dict_value(mocker, config): ctx = {"tenant": {"tenant_id": "6", "username": "lessa"}} toggle = feature_flags.evaluate(name="my_feature", context=ctx, default=False) assert toggle == expected_value + + +def test_get_feature_toggle_propagates_access_denied_error(mocker, config): + # GIVEN a schema fetch that raises a StoreClientError + # due to client invalid permissions to fetch from the store + err = "An error occurred (AccessDeniedException) when calling the GetConfiguration operation" + schema_fetcher = init_fetcher_side_effect(mocker, config, GetParameterError(err)) + feature_flags = FeatureFlags(schema_fetcher) + + # WHEN calling evaluate + # THEN raise StoreClientError error + with pytest.raises(StoreClientError, match="AccessDeniedException") as err: + feature_flags.evaluate(name="Foo", default=False)