refactor(schema): use new rules dict over list

heitorlessa · heitorlessa · commit 4aba5070d97e · 2021-08-03T08:46:22.000+02:00
diff --git 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
@@ -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
@@ -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,16 +335,16 @@ def test_multiple_features_enabled(mocker, config):
                                 "value": ["6", "2"],
                             }
                         ],
-                    },
-                ],
+                    }
+                },
             },
             "my_feature2": {
                 "default": True,
             },
             "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,20 +367,20 @@ def test_multiple_features_only_some_enabled(mocker, config):
                                 "value": ["6", "2"],
                             }
                         ],
-                    },
-                ],
+                    }
+                },
             },
             "my_feature2": {
                 "default": True,
             },
             "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
