From a222159f8dbc9de512a1162dcad0b44425872d3a Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 25 Jul 2023 14:39:50 +0200 Subject: [PATCH 01/27] fix(parameters): make cache aware of single vs multiple calls Signed-off-by: heitorlessa --- aws_lambda_powertools/utilities/parameters/base.py | 2 +- aws_lambda_powertools/utilities/parameters/types.py | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/parameters/base.py b/aws_lambda_powertools/utilities/parameters/base.py index 710634636d0..411a2520ae9 100644 --- a/aws_lambda_powertools/utilities/parameters/base.py +++ b/aws_lambda_powertools/utilities/parameters/base.py @@ -27,7 +27,7 @@ from aws_lambda_powertools.shared import constants, user_agent from aws_lambda_powertools.shared.functions import resolve_max_age -from aws_lambda_powertools.utilities.parameters.types import TransformOptions +from aws_lambda_powertools.utilities.parameters.types import RecursiveOptions, TransformOptions from .exceptions import GetParameterError, TransformParameterError diff --git a/aws_lambda_powertools/utilities/parameters/types.py b/aws_lambda_powertools/utilities/parameters/types.py index faa06cee89e..a916f1a344d 100644 --- a/aws_lambda_powertools/utilities/parameters/types.py +++ b/aws_lambda_powertools/utilities/parameters/types.py @@ -1,3 +1,4 @@ from aws_lambda_powertools.shared.types import Literal TransformOptions = Literal["json", "binary", "auto", None] +RecursiveOptions = Literal[True, False] From 971e70f1de42bc0a2143a187c039035f0e1786dc Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Tue, 25 Jul 2023 15:16:51 +0200 Subject: [PATCH 02/27] chore: cleanup, add test for single and nested Signed-off-by: heitorlessa --- aws_lambda_powertools/utilities/parameters/base.py | 2 +- aws_lambda_powertools/utilities/parameters/types.py | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/parameters/base.py b/aws_lambda_powertools/utilities/parameters/base.py index 411a2520ae9..710634636d0 100644 --- a/aws_lambda_powertools/utilities/parameters/base.py +++ b/aws_lambda_powertools/utilities/parameters/base.py @@ -27,7 +27,7 @@ from aws_lambda_powertools.shared import constants, user_agent from aws_lambda_powertools.shared.functions import resolve_max_age -from aws_lambda_powertools.utilities.parameters.types import RecursiveOptions, TransformOptions +from aws_lambda_powertools.utilities.parameters.types import TransformOptions from .exceptions import GetParameterError, TransformParameterError diff --git a/aws_lambda_powertools/utilities/parameters/types.py b/aws_lambda_powertools/utilities/parameters/types.py index a916f1a344d..faa06cee89e 100644 --- a/aws_lambda_powertools/utilities/parameters/types.py +++ b/aws_lambda_powertools/utilities/parameters/types.py @@ -1,4 +1,3 @@ from aws_lambda_powertools.shared.types import Literal TransformOptions = Literal["json", "binary", "auto", None] -RecursiveOptions = Literal[True, False] From a0ca2c45910cc9afb374efcd0b4544642af3881f Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 13:02:26 +0100 Subject: [PATCH 03/27] chore: update deprecated ruff config --- ruff.toml | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/ruff.toml b/ruff.toml index 553a8c47b3d..374a183541b 100644 --- a/ruff.toml +++ b/ruff.toml @@ -1,5 +1,5 @@ # Enable rules. -select = [ +lint.select = [ "A", # flake8-builtins - https://beta.ruff.rs/docs/rules/#flake8-builtins-a "B", # flake8-bugbear-b - https://beta.ruff.rs/docs/rules/#flake8-bugbear-b "C4", # flake8-comprehensions - https://beta.ruff.rs/docs/rules/#flake8-comprehensions-c4 @@ -28,7 +28,7 @@ select = [ ] # Ignore specific rules -ignore = [ +lint.ignore = [ "W291", # https://beta.ruff.rs/docs/rules/trailing-whitespace/ "PLR0913", # https://beta.ruff.rs/docs/rules/too-many-arguments/ "PLR2004", #https://beta.ruff.rs/docs/rules/magic-value-comparison/ @@ -60,28 +60,28 @@ line-length = 120 fix = true -fixable = ["I", "COM812", "W"] +lint.fixable = ["I", "COM812", "W"] # See: https://github.com/astral-sh/ruff/issues/128 -typing-modules = [ +lint.typing-modules = [ "aws_lambda_powertools.utilities.parser.types", "aws_lambda_powertools.shared.types", ] -[mccabe] +[lint.mccabe] # Maximum cyclomatic complexity max-complexity = 15 -[pylint] +[lint.pylint] # Maximum number of nested blocks max-branches = 15 # Maximum number of if statements in a function max-statements = 70 -[isort] +[lint.isort] split-on-trailing-comma = true -[per-file-ignores] +[lint.per-file-ignores] # Ignore specific rules for specific files "tests/e2e/utils/data_builder/__init__.py" = ["F401"] "tests/e2e/utils/data_fetcher/__init__.py" = ["F401"] From 59dc7767985313b43c4845527bbc1adf98c958fd Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 13:02:38 +0100 Subject: [PATCH 04/27] refactor: define rule action mapping once --- .../utilities/feature_flags/feature_flags.py | 56 +++++++++---------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index ac4dfef0162..32486e5ea45 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -6,16 +6,40 @@ from . import schema from .base import StoreProvider from .comparators import ( + compare_all_in_list, + compare_any_in_list, compare_datetime_range, compare_days_of_week, compare_modulo_range, + compare_none_in_list, compare_time_range, - compare_all_in_list, - compare_any_in_list, - compare_none_in_list ) from .exceptions import ConfigurationStoreError +RULE_ACTION_MAPPING = { + schema.RuleAction.EQUALS.value: lambda a, b: a == b, + schema.RuleAction.NOT_EQUALS.value: lambda a, b: a != b, + schema.RuleAction.KEY_GREATER_THAN_VALUE.value: lambda a, b: a > b, + schema.RuleAction.KEY_GREATER_THAN_OR_EQUAL_VALUE.value: lambda a, b: a >= b, + schema.RuleAction.KEY_LESS_THAN_VALUE.value: lambda a, b: a < b, + schema.RuleAction.KEY_LESS_THAN_OR_EQUAL_VALUE.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.IN.value: lambda a, b: a in b, + schema.RuleAction.NOT_IN.value: lambda a, b: a not in b, + schema.RuleAction.KEY_IN_VALUE.value: lambda a, b: a in b, + schema.RuleAction.KEY_NOT_IN_VALUE.value: lambda a, b: a not in b, + schema.RuleAction.VALUE_IN_KEY.value: lambda a, b: b in a, + schema.RuleAction.VALUE_NOT_IN_KEY.value: lambda a, b: b not in a, + schema.RuleAction.ALL_IN_VALUE.value: lambda a, b: compare_all_in_list(a, b), + schema.RuleAction.ANY_IN_VALUE.value: lambda a, b: compare_any_in_list(a, b), + schema.RuleAction.NONE_IN_VALUE.value: lambda a, b: compare_none_in_list(a, b), + schema.RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value: lambda a, b: compare_time_range(a, b), + schema.RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value: lambda a, b: compare_datetime_range(a, b), + schema.RuleAction.SCHEDULE_BETWEEN_DAYS_OF_WEEK.value: lambda a, b: compare_days_of_week(a, b), + schema.RuleAction.MODULO_RANGE.value: lambda a, b: compare_modulo_range(a, b), +} + class FeatureFlags: def __init__(self, store: StoreProvider, logger: Optional[Union[logging.Logger, Logger]] = None): @@ -51,32 +75,8 @@ def __init__(self, store: StoreProvider, logger: Optional[Union[logging.Logger, self.logger = logger or logging.getLogger(__name__) def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: - mapping_by_action = { - schema.RuleAction.EQUALS.value: lambda a, b: a == b, - schema.RuleAction.NOT_EQUALS.value: lambda a, b: a != b, - schema.RuleAction.KEY_GREATER_THAN_VALUE.value: lambda a, b: a > b, - schema.RuleAction.KEY_GREATER_THAN_OR_EQUAL_VALUE.value: lambda a, b: a >= b, - schema.RuleAction.KEY_LESS_THAN_VALUE.value: lambda a, b: a < b, - schema.RuleAction.KEY_LESS_THAN_OR_EQUAL_VALUE.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.IN.value: lambda a, b: a in b, - schema.RuleAction.NOT_IN.value: lambda a, b: a not in b, - schema.RuleAction.KEY_IN_VALUE.value: lambda a, b: a in b, - schema.RuleAction.KEY_NOT_IN_VALUE.value: lambda a, b: a not in b, - schema.RuleAction.VALUE_IN_KEY.value: lambda a, b: b in a, - schema.RuleAction.VALUE_NOT_IN_KEY.value: lambda a, b: b not in a, - schema.RuleAction.ALL_IN_VALUE.value: lambda a, b: compare_all_in_list(a, b), - schema.RuleAction.ANY_IN_VALUE.value: lambda a, b: compare_any_in_list(a, b), - schema.RuleAction.NONE_IN_VALUE.value: lambda a, b: compare_none_in_list(a, b), - schema.RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value: lambda a, b: compare_time_range(a, b), - schema.RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value: lambda a, b: compare_datetime_range(a, b), - schema.RuleAction.SCHEDULE_BETWEEN_DAYS_OF_WEEK.value: lambda a, b: compare_days_of_week(a, b), - schema.RuleAction.MODULO_RANGE.value: lambda a, b: compare_modulo_range(a, b), - } - try: - func = mapping_by_action.get(action, lambda a, b: False) + func = RULE_ACTION_MAPPING.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)}") From 04f67b79ceebdde866d2766a5cfb4dfbd56f6aa9 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 13:54:00 +0100 Subject: [PATCH 05/27] chore: add tests for ANY_IN_VALUE Signed-off-by: heitorlessa --- .../feature_flags/test_feature_flags.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 12adaa4525b..cdce0e2b876 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1410,3 +1410,32 @@ def test_get_all_enabled_features_non_boolean_truthy_defaults(mocker, config): 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_flags_any_in_value_match(mocker, config): + expected_value = True + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.ANY_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Gerald"]}, + default=False, + ) + assert toggle == expected_value From 52e9dd6e908932d799b2169142bcfcb8e381144c Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:00:18 +0100 Subject: [PATCH 06/27] chore: add tests for ALL_IN_VALUE --- .../feature_flags/test_feature_flags.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index cdce0e2b876..32b634eb690 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1439,3 +1439,63 @@ def test_flags_any_in_value_match(mocker, config): default=False, ) assert toggle == expected_value + + +def test_flags_all_in_value_match(mocker, config): + expected_value = True + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.ALL_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Gerald"]}, + default=False, + ) + + assert toggle == expected_value + + +def test_flags_all_in_value_no_match(mocker, config): + expected_value = False + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.ALL_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Gerald", "Simon"]}, + default=False, + ) + + assert toggle == expected_value From 19bec81c019387c0343f359bbc7d4cdf418a917f Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:04:32 +0100 Subject: [PATCH 07/27] chore: add no match test for ANY_IN_VALUE --- .../feature_flags/test_feature_flags.py | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 32b634eb690..02704925228 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1441,6 +1441,35 @@ def test_flags_any_in_value_match(mocker, config): assert toggle == expected_value +def test_flags_any_in_value_no_match(mocker, config): + expected_value = False + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.ANY_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Simon"]}, + default=False, + ) + assert toggle == expected_value + + def test_flags_all_in_value_match(mocker, config): expected_value = True mocked_app_config_schema = { From 1681ceb890a3228e9718e4f48108dca490f39adb Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:18:00 +0100 Subject: [PATCH 08/27] chore: add tests for NONE_IN_VALUE --- .../feature_flags/test_feature_flags.py | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 02704925228..5b4089a2591 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1528,3 +1528,63 @@ def test_flags_all_in_value_no_match(mocker, config): ) assert toggle == expected_value + + +def test_flags_none_in_value_match(mocker, config): + expected_value = True + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.NONE_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Rubao"]}, + default=False, + ) + + assert toggle == expected_value + + +def test_flags_none_in_value_no_match(mocker, config): + expected_value = False + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.NONE_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": ["Heitor"]}, + default=False, + ) + + assert toggle == expected_value From 4d6a36c6cb583f517c10436e0b8a715b90946b54 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:47:35 +0100 Subject: [PATCH 09/27] refactor: typing --- .../utilities/feature_flags/comparators.py | 33 ++++++++++--------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/comparators.py b/aws_lambda_powertools/utilities/feature_flags/comparators.py index 1419dd6dd83..efa5d358bf5 100644 --- a/aws_lambda_powertools/utilities/feature_flags/comparators.py +++ b/aws_lambda_powertools/utilities/feature_flags/comparators.py @@ -1,10 +1,12 @@ +from __future__ import annotations + from datetime import datetime, tzinfo from typing import Any, Dict, Optional from dateutil.tz import gettz -from .schema import HOUR_MIN_SEPARATOR, ModuloRangeValues, TimeValues from .exceptions import SchemaValidationError +from .schema import HOUR_MIN_SEPARATOR, ModuloRangeValues, TimeValues def _get_now_from_timezone(timezone: Optional[tzinfo]) -> datetime: @@ -85,41 +87,40 @@ def compare_modulo_range(context_value: int, condition_value: Dict) -> bool: return start <= context_value % base <= end -def compare_any_in_list(key_list, value_list): - if not (isinstance(key_list, list) and isinstance(value_list, list)): +def compare_any_in_list(context_value: list, condition_value: list) -> bool: + if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() - + results = False - for key in key_list: - if key in value_list: + for key in context_value: + if key in condition_value: results = True break - + return results -def compare_all_in_list(key_list, value_list): - if not (isinstance(key_list, list) and isinstance(value_list, list)): +def compare_all_in_list(context_value: list, condition_value: list) -> bool: + if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() results = True - for key in key_list: - if key not in value_list: + for key in context_value: + if key not in condition_value: results = False break return results -def compare_none_in_list(key_list, value_list): - if not (isinstance(key_list, list) and isinstance(value_list, list)): +def compare_none_in_list(context_value: list, condition_value: list) -> bool: + if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() results = True - for key in key_list: - if key in value_list: + for key in context_value: + if key in condition_value: results = False break return results - From 4d24cb37a39c2110c5c52d0ce281c8639ffae6d1 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:51:13 +0100 Subject: [PATCH 10/27] refactor: use any and all built-in instead --- .../utilities/feature_flags/comparators.py | 24 +++---------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/comparators.py b/aws_lambda_powertools/utilities/feature_flags/comparators.py index efa5d358bf5..ec202652406 100644 --- a/aws_lambda_powertools/utilities/feature_flags/comparators.py +++ b/aws_lambda_powertools/utilities/feature_flags/comparators.py @@ -91,36 +91,18 @@ def compare_any_in_list(context_value: list, condition_value: list) -> bool: if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() - results = False - for key in context_value: - if key in condition_value: - results = True - break - - return results + return any(key in condition_value for key in context_value) def compare_all_in_list(context_value: list, condition_value: list) -> bool: if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() - results = True - for key in context_value: - if key not in condition_value: - results = False - break - - return results + return all(key in condition_value for key in context_value) def compare_none_in_list(context_value: list, condition_value: list) -> bool: if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() - results = True - for key in context_value: - if key in condition_value: - results = False - break - - return results + return all(key not in condition_value for key in context_value) From 608774131455513238e05d1d98aaa4252b3b5956 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Thu, 15 Feb 2024 14:56:35 +0100 Subject: [PATCH 11/27] refactor: add docstrings --- .../utilities/feature_flags/comparators.py | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/aws_lambda_powertools/utilities/feature_flags/comparators.py b/aws_lambda_powertools/utilities/feature_flags/comparators.py index ec202652406..6e8f34e9d8a 100644 --- a/aws_lambda_powertools/utilities/feature_flags/comparators.py +++ b/aws_lambda_powertools/utilities/feature_flags/comparators.py @@ -88,6 +88,20 @@ def compare_modulo_range(context_value: int, condition_value: Dict) -> bool: def compare_any_in_list(context_value: list, condition_value: list) -> bool: + """Comparator for ANY_IN_VALUE action + + Parameters + ---------- + context_value : list + user-defined context for flag evaluation + condition_value : list + schema value available for condition being evaluated + + Returns + ------- + bool + Whether any list item in context_value is available in condition_value + """ if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() @@ -95,6 +109,20 @@ def compare_any_in_list(context_value: list, condition_value: list) -> bool: def compare_all_in_list(context_value: list, condition_value: list) -> bool: + """Comparator for ALL_IN_VALUE action + + Parameters + ---------- + context_value : list + user-defined context for flag evaluation + condition_value : list + schema value available for condition being evaluated + + Returns + ------- + bool + Whether all list items in context_value are available in condition_value + """ if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() @@ -102,6 +130,20 @@ def compare_all_in_list(context_value: list, condition_value: list) -> bool: def compare_none_in_list(context_value: list, condition_value: list) -> bool: + """Comparator for NONE_IN_VALUE action + + Parameters + ---------- + context_value : list + user-defined context for flag evaluation + condition_value : list + schema value available for condition being evaluated + + Returns + ------- + bool + Whether list items in context_value are **not** available in condition_value + """ if not (isinstance(context_value, list) and isinstance(condition_value, list)): raise SchemaValidationError() From cc6079dad2485546adaeeaeeb748e64aa2ddde71 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 11:14:06 +0100 Subject: [PATCH 12/27] refactor: use getattr to allow for specialized validators Signed-off-by: heitorlessa --- .../utilities/feature_flags/schema.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 2ef4b9e29a4..1bc9f078ce9 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -350,6 +350,16 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): raise SchemaValidationError(f"'value' key must not be null, rule={rule_name}") action = condition.get(CONDITION_ACTION, "") + # maintenance: we may need a new design if we end up with more exceptions like datetime/time range + # e.g., visitor pattern, registry etc. + validator = getattr( + ConditionsValidator, + f"_validate_{action.lower()}_value", + ConditionsValidator._validate_noop_value, + ) + + validator(value, rule_name) + # time actions need to be parsed to make sure date and time format is valid and timezone is recognized if action == RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value: ConditionsValidator._validate_schedule_between_time_and_datetime_ranges( @@ -365,12 +375,13 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): action, ConditionsValidator._validate_datetime_value, ) - elif action == RuleAction.SCHEDULE_BETWEEN_DAYS_OF_WEEK.value: - ConditionsValidator._validate_schedule_between_days_of_week(value, rule_name) - # modulo range condition needs validation on base, start, and end attributes elif action == RuleAction.MODULO_RANGE.value: ConditionsValidator._validate_modulo_range(value, rule_name) + @staticmethod + def _validate_noop_value(*args, **kwargs): + return True + @staticmethod def _validate_datetime_value(datetime_str: str, rule_name: str): date = None @@ -408,7 +419,7 @@ def _validate_time_value(time: str, rule_name: str): ) @staticmethod - def _validate_schedule_between_days_of_week(value: Any, rule_name: str): + def _validate_schedule_between_days_of_week_value(value: Any, rule_name: str): error_str = f"condition with a CURRENT_DAY_OF_WEEK action must have a condition value dictionary with 'DAYS' and 'TIMEZONE' (optional) keys, rule={rule_name}" # noqa: E501 if not isinstance(value, dict): raise SchemaValidationError(error_str) From c42d477c918cfa5193fa912183801a2ef757423e Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 11:23:01 +0100 Subject: [PATCH 13/27] refactor: use getattr for modulo range --- aws_lambda_powertools/utilities/feature_flags/schema.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 1bc9f078ce9..d846eda66e1 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -1,3 +1,5 @@ +from __future__ import annotations + import logging import re from datetime import datetime @@ -375,8 +377,6 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): action, ConditionsValidator._validate_datetime_value, ) - elif action == RuleAction.MODULO_RANGE.value: - ConditionsValidator._validate_modulo_range(value, rule_name) @staticmethod def _validate_noop_value(*args, **kwargs): @@ -419,7 +419,7 @@ def _validate_time_value(time: str, rule_name: str): ) @staticmethod - def _validate_schedule_between_days_of_week_value(value: Any, rule_name: str): + def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): error_str = f"condition with a CURRENT_DAY_OF_WEEK action must have a condition value dictionary with 'DAYS' and 'TIMEZONE' (optional) keys, rule={rule_name}" # noqa: E501 if not isinstance(value, dict): raise SchemaValidationError(error_str) @@ -479,7 +479,7 @@ def _validate_schedule_between_time_and_datetime_ranges( raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") @staticmethod - def _validate_modulo_range(value: Any, rule_name: str): + def _validate_modulo_range_value(value: dict, rule_name: str): error_str = f"condition with a 'MODULO_RANGE' action must have a condition value type dictionary with 'BASE', 'START' and 'END' keys, rule={rule_name}" # noqa: E501 if not isinstance(value, dict): raise SchemaValidationError(error_str) From 8729f859b471abd41137dff2e146a6b2ecfefaf8 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 14:12:17 +0100 Subject: [PATCH 14/27] refactor: use getattr for time range --- .../utilities/feature_flags/__init__.py | 1 + .../utilities/feature_flags/schema.py | 49 ++++++++++++------- .../feature_flags/test_schema_validation.py | 9 ++-- 3 files changed, 34 insertions(+), 25 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/__init__.py b/aws_lambda_powertools/utilities/feature_flags/__init__.py index db7dfca5b57..e8d8229c9dc 100644 --- a/aws_lambda_powertools/utilities/feature_flags/__init__.py +++ b/aws_lambda_powertools/utilities/feature_flags/__init__.py @@ -1,4 +1,5 @@ """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/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index d846eda66e1..1da027a50dd 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -362,15 +362,7 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): validator(value, rule_name) - # time actions need to be parsed to make sure date and time format is valid and timezone is recognized - if action == RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value: - ConditionsValidator._validate_schedule_between_time_and_datetime_ranges( - value, - rule_name, - action, - ConditionsValidator._validate_time_value, - ) - elif action == RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value: + if action == RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value: ConditionsValidator._validate_schedule_between_time_and_datetime_ranges( value, rule_name, @@ -408,16 +400,6 @@ def _validate_datetime_value(datetime_str: str, rule_name: str): f"field, rule={rule_name} ", ) - @staticmethod - def _validate_time_value(time: str, rule_name: str): - # Using a regex instead of strptime because it's several orders of magnitude faster - match = TIME_RANGE_RE_PATTERN.match(time) - - if not match: - raise SchemaValidationError( - f"'START' and 'END' must be a valid time format, time_format={TIME_RANGE_FORMAT}, rule={rule_name}", - ) - @staticmethod def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): error_str = f"condition with a CURRENT_DAY_OF_WEEK action must have a condition value dictionary with 'DAYS' and 'TIMEZONE' (optional) keys, rule={rule_name}" # noqa: E501 @@ -449,6 +431,35 @@ def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): if not tz.gettz(timezone): raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") + @staticmethod + def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> bool: + if not isinstance(value, dict): + raise SchemaValidationError( + f"{RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value} action must have a dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + ) + + start_time = value.get(TimeValues.START.value, "") + end_time = value.get(TimeValues.END.value, "") + + if not isinstance(start_time, str) or not isinstance(end_time, str): + raise SchemaValidationError(f"'START' and 'END' must be a non empty string, rule={rule_name}") + + # Using a regex instead of strptime because it's several orders of magnitude faster + if not TIME_RANGE_RE_PATTERN.match(start_time) or not TIME_RANGE_RE_PATTERN.match(end_time): + raise SchemaValidationError( + f"'START' and 'END' must be a valid time format, time_format={TIME_RANGE_FORMAT}, rule={rule_name}", + ) + + timezone = value.get(TimeValues.TIMEZONE.value, "UTC") + if not isinstance(timezone, str): + raise SchemaValidationError(f"'TIMEZONE' must be a string, rule={rule_name}") + + # try to see if the timezone string corresponds to any known timezone + if not tz.gettz(timezone): + raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") + + return True + @staticmethod def _validate_schedule_between_time_and_datetime_ranges( value: Any, diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py index 654cfbcab40..6a935320d06 100644 --- a/tests/functional/feature_flags/test_schema_validation.py +++ b/tests/functional/feature_flags/test_schema_validation.py @@ -441,7 +441,7 @@ def test_validate_time_condition_between_time_range_invalid_condition_value(): # THEN raise SchemaValidationError with pytest.raises( SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_TIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + match=f"SCHEDULE_BETWEEN_TIME_RANGE action must have a dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 ): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @@ -460,7 +460,7 @@ def test_validate_time_condition_between_time_range_invalid_condition_value_no_s # THEN raise SchemaValidationError with pytest.raises( SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_TIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + match="'START' and 'END' must be a valid time format", ): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @@ -477,10 +477,7 @@ def test_validate_time_condition_between_time_range_invalid_condition_value_no_e # WHEN calling validate_condition # THEN raise SchemaValidationError - with pytest.raises( - SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_TIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 - ): + with pytest.raises(SchemaValidationError, match="'START' and 'END' must be a valid time format"): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) From b3d0f89f38970199bb16ea694b30e38b6fca763b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 15:10:10 +0100 Subject: [PATCH 15/27] refactor: use getattr for datetime range --- .../utilities/feature_flags/schema.py | 125 ++++++++---------- .../feature_flags/test_schema_validation.py | 6 +- 2 files changed, 55 insertions(+), 76 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 1da027a50dd..80c0a2da03c 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -4,7 +4,7 @@ import re from datetime import datetime from enum import Enum -from typing import Any, Callable, Dict, List, Optional, Union +from typing import Any, Dict, List, Optional, Union from dateutil import tz @@ -21,7 +21,7 @@ CONDITION_ACTION = "action" FEATURE_DEFAULT_VAL_TYPE_KEY = "boolean_type" TIME_RANGE_FORMAT = "%H:%M" # hour:min 24 hours clock -TIME_RANGE_RE_PATTERN = re.compile(r"2[0-3]:[0-5]\d|[0-1]\d:[0-5]\d") # 24 hour clock +TIME_RANGE_PATTERN = re.compile(r"2[0-3]:[0-5]\d|[0-1]\d:[0-5]\d") # 24 hour clock HOUR_MIN_SEPARATOR = ":" @@ -362,44 +362,10 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): validator(value, rule_name) - if action == RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value: - ConditionsValidator._validate_schedule_between_time_and_datetime_ranges( - value, - rule_name, - action, - ConditionsValidator._validate_datetime_value, - ) - @staticmethod def _validate_noop_value(*args, **kwargs): return True - @staticmethod - def _validate_datetime_value(datetime_str: str, rule_name: str): - date = None - - # We try to parse first with timezone information in order to return the correct error messages - # when a timestamp with timezone is used. Otherwise, the user would get the first error "must be a valid - # ISO8601 time format" which is misleading - - try: - # python < 3.11 don't support the Z timezone on datetime.fromisoformat, - # so we replace any Z with the equivalent "+00:00" - # datetime.fromisoformat is orders of magnitude faster than datetime.strptime - date = datetime.fromisoformat(datetime_str.replace("Z", "+00:00")) - except Exception: - raise SchemaValidationError(f"'START' and 'END' must be a valid ISO8601 time format, rule={rule_name}") - - # we only allow timezone information to be set via the TIMEZONE field - # this way we can encode DST into the calculation. For instance, Copenhagen is - # UTC+2 during winter, and UTC+1 during summer, which would be impossible to define - # using a single ISO datetime string - if date.tzinfo is not None: - raise SchemaValidationError( - "'START' and 'END' must not include timezone information. Set the timezone using the 'TIMEZONE' " - f"field, rule={rule_name} ", - ) - @staticmethod def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): error_str = f"condition with a CURRENT_DAY_OF_WEEK action must have a condition value dictionary with 'DAYS' and 'TIMEZONE' (optional) keys, rule={rule_name}" # noqa: E501 @@ -423,13 +389,7 @@ def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): f"condition value DAYS must represent a day of the week in 'TimeValues' enum, rule={rule_name}", ) - timezone = value.get(TimeValues.TIMEZONE.value, "UTC") - if not isinstance(timezone, str): - raise SchemaValidationError(error_str) - - # try to see if the timezone string corresponds to any known timezone - if not tz.gettz(timezone): - raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") + ConditionsValidator._validate_timezone(timezone=value.get(TimeValues.TIMEZONE.value), rule=rule_name) @staticmethod def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> bool: @@ -445,49 +405,31 @@ def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> raise SchemaValidationError(f"'START' and 'END' must be a non empty string, rule={rule_name}") # Using a regex instead of strptime because it's several orders of magnitude faster - if not TIME_RANGE_RE_PATTERN.match(start_time) or not TIME_RANGE_RE_PATTERN.match(end_time): + if not TIME_RANGE_PATTERN.match(start_time) or not TIME_RANGE_PATTERN.match(end_time): raise SchemaValidationError( f"'START' and 'END' must be a valid time format, time_format={TIME_RANGE_FORMAT}, rule={rule_name}", ) - timezone = value.get(TimeValues.TIMEZONE.value, "UTC") - if not isinstance(timezone, str): - raise SchemaValidationError(f"'TIMEZONE' must be a string, rule={rule_name}") - - # try to see if the timezone string corresponds to any known timezone - if not tz.gettz(timezone): - raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") + ConditionsValidator._validate_timezone(timezone=value.get(TimeValues.TIMEZONE.value), rule=rule_name) return True @staticmethod - def _validate_schedule_between_time_and_datetime_ranges( - value: Any, - rule_name: str, - action_name: str, - validator: Callable[[str, str], None], - ): - error_str = f"condition with a '{action_name}' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}" # noqa: E501 + def _validate_schedule_between_datetime_range_value(value: dict, rule_name: str): if not isinstance(value, dict): - raise SchemaValidationError(error_str) + raise SchemaValidationError( + f"{RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value} action must have a dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + ) + + start_time = value.get(TimeValues.START.value, "") + end_time = value.get(TimeValues.END.value, "") - start_time = value.get(TimeValues.START.value) - end_time = value.get(TimeValues.END.value) - if not start_time or not end_time: - raise SchemaValidationError(error_str) if not isinstance(start_time, str) or not isinstance(end_time, str): raise SchemaValidationError(f"'START' and 'END' must be a non empty string, rule={rule_name}") - validator(start_time, rule_name) - validator(end_time, rule_name) - - timezone = value.get(TimeValues.TIMEZONE.value, "UTC") - if not isinstance(timezone, str): - raise SchemaValidationError(f"'TIMEZONE' must be a string, rule={rule_name}") - - # try to see if the timezone string corresponds to any known timezone - if not tz.gettz(timezone): - raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule_name}") + ConditionsValidator._validate_datetime(start_time, rule_name) + ConditionsValidator._validate_datetime(end_time, rule_name) + ConditionsValidator._validate_timezone(timezone=value.get(TimeValues.TIMEZONE.value), rule=rule_name) @staticmethod def _validate_modulo_range_value(value: dict, rule_name: str): @@ -507,3 +449,40 @@ def _validate_modulo_range_value(value: dict, rule_name: str): raise SchemaValidationError( f"condition with 'MODULO_RANGE' action must satisfy 0 <= START <= END <= BASE-1, rule={rule_name}", ) + + @staticmethod + def _validate_datetime(datetime_str: str, rule_name: str): + date = None + + # We try to parse first with timezone information in order to return the correct error messages + # when a timestamp with timezone is used. Otherwise, the user would get the first error "must be a valid + # ISO8601 time format" which is misleading + + try: + # python < 3.11 don't support the Z timezone on datetime.fromisoformat, + # so we replace any Z with the equivalent "+00:00" + # datetime.fromisoformat is orders of magnitude faster than datetime.strptime + date = datetime.fromisoformat(datetime_str.replace("Z", "+00:00")) + except Exception: + raise SchemaValidationError(f"'START' and 'END' must be a valid ISO8601 time format, rule={rule_name}") + + # we only allow timezone information to be set via the TIMEZONE field + # this way we can encode DST into the calculation. For instance, Copenhagen is + # UTC+2 during winter, and UTC+1 during summer, which would be impossible to define + # using a single ISO datetime string + if date.tzinfo is not None: + raise SchemaValidationError( + "'START' and 'END' must not include timezone information. Set the timezone using the 'TIMEZONE' " + f"field, rule={rule_name} ", + ) + + @staticmethod + def _validate_timezone(rule: str, timezone: str | None = None): + timezone = timezone or "UTC" + + if not isinstance(timezone, str): + raise SchemaValidationError(f"'TIMEZONE' must be a string, rule={str}") + + # try to see if the timezone string corresponds to any known timezone + if not tz.gettz(timezone): + raise SchemaValidationError(f"'TIMEZONE' value must represent a valid IANA timezone, rule={rule}") diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py index 6a935320d06..75f86cf5b25 100644 --- a/tests/functional/feature_flags/test_schema_validation.py +++ b/tests/functional/feature_flags/test_schema_validation.py @@ -646,7 +646,7 @@ def test_a_validate_time_condition_between_datetime_range_invalid_condition_valu # THEN raise SchemaValidationError with pytest.raises( SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_DATETIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + match=f"SCHEDULE_BETWEEN_DATETIME_RANGE action must have a dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 ): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @@ -665,7 +665,7 @@ def test_validate_time_condition_between_datetime_range_invalid_condition_value_ # THEN raise SchemaValidationError with pytest.raises( SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_DATETIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + match=f"'START' and 'END' must be a valid ISO8601 time format, rule={rule_name}", ): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) @@ -684,7 +684,7 @@ def test_validate_time_condition_between_datetime_range_invalid_condition_value_ # THEN raise SchemaValidationError with pytest.raises( SchemaValidationError, - match=f"condition with a 'SCHEDULE_BETWEEN_DATETIME_RANGE' action must have a condition value type dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 + match="'START' and 'END' must not include timezone information.*", ): ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) From 46bd225797fe1362573eca4def6cafee952e4f73 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 15:22:16 +0100 Subject: [PATCH 16/27] refactor: use getattr for condition keys --- .../utilities/feature_flags/schema.py | 46 ++++++++++++------- 1 file changed, 30 insertions(+), 16 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 80c0a2da03c..a026b15b709 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -327,23 +327,16 @@ def validate_condition_key(condition: Dict[str, Any], rule_name: str): if not key or not isinstance(key, str): raise SchemaValidationError(f"'key' value must be a non empty string, rule={rule_name}") - # time actions need to have very specific keys - # SCHEDULE_BETWEEN_TIME_RANGE => CURRENT_TIME - # SCHEDULE_BETWEEN_DATETIME_RANGE => CURRENT_DATETIME - # SCHEDULE_BETWEEN_DAYS_OF_WEEK => CURRENT_DAY_OF_WEEK action = condition.get(CONDITION_ACTION, "") - if action == RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value and key != TimeKeys.CURRENT_TIME.value: - raise SchemaValidationError( - f"'condition with a 'SCHEDULE_BETWEEN_TIME_RANGE' action must have a 'CURRENT_TIME' condition key, rule={rule_name}", # noqa: E501 - ) - if action == RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value and key != TimeKeys.CURRENT_DATETIME.value: - raise SchemaValidationError( - f"'condition with a 'SCHEDULE_BETWEEN_DATETIME_RANGE' action must have a 'CURRENT_DATETIME' condition key, rule={rule_name}", # noqa: E501 - ) - if action == RuleAction.SCHEDULE_BETWEEN_DAYS_OF_WEEK.value and key != TimeKeys.CURRENT_DAY_OF_WEEK.value: - raise SchemaValidationError( - f"'condition with a 'SCHEDULE_BETWEEN_DAYS_OF_WEEK' action must have a 'CURRENT_DAY_OF_WEEK' condition key, rule={rule_name}", # noqa: E501 - ) + # maintenance: we may need a new design if we end up with more exceptions like datetime/time range + # e.g., visitor pattern, registry etc. + validator = getattr( + ConditionsValidator, + f"_validate_{action.lower()}_key", + ConditionsValidator._validate_noop_value, + ) + + validator(key, rule_name) @staticmethod def validate_condition_value(condition: Dict[str, Any], rule_name: str): @@ -366,6 +359,13 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): def _validate_noop_value(*args, **kwargs): return True + @staticmethod + def _validate_schedule_between_days_of_week_key(key: str, rule_name: str): + if key != TimeKeys.CURRENT_DAY_OF_WEEK.value: + raise SchemaValidationError( + f"'condition with a 'SCHEDULE_BETWEEN_DAYS_OF_WEEK' action must have a 'CURRENT_DAY_OF_WEEK' condition key, rule={rule_name}", # noqa: E501 + ) + @staticmethod def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): error_str = f"condition with a CURRENT_DAY_OF_WEEK action must have a condition value dictionary with 'DAYS' and 'TIMEZONE' (optional) keys, rule={rule_name}" # noqa: E501 @@ -391,6 +391,13 @@ def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): ConditionsValidator._validate_timezone(timezone=value.get(TimeValues.TIMEZONE.value), rule=rule_name) + @staticmethod + def _validate_schedule_between_time_range_key(key: str, rule_name: str): + if key != TimeKeys.CURRENT_TIME.value: + raise SchemaValidationError( + f"'condition with a 'SCHEDULE_BETWEEN_TIME_RANGE' action must have a 'CURRENT_TIME' condition key, rule={rule_name}", # noqa: E501 + ) + @staticmethod def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> bool: if not isinstance(value, dict): @@ -414,6 +421,13 @@ def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> return True + @staticmethod + def _validate_schedule_between_datetime_range_key(key: str, rule_name: str): + if key != TimeKeys.CURRENT_DATETIME.value: + raise SchemaValidationError( + f"'condition with a 'SCHEDULE_BETWEEN_DATETIME_RANGE' action must have a 'CURRENT_DATETIME' condition key, rule={rule_name}", # noqa: E501 + ) + @staticmethod def _validate_schedule_between_datetime_range_value(value: dict, rule_name: str): if not isinstance(value, dict): From 3f436288583ef1cf2d999bb33433631aabfbfb89 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 16:13:01 +0100 Subject: [PATCH 17/27] refactor: split condition from context validation --- .../utilities/feature_flags/comparators.py | 13 +++--- .../utilities/feature_flags/schema.py | 15 +++++++ .../feature_flags/test_schema_validation.py | 45 +++++++++++++++++++ 3 files changed, 66 insertions(+), 7 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/comparators.py b/aws_lambda_powertools/utilities/feature_flags/comparators.py index 6e8f34e9d8a..03cb91e649a 100644 --- a/aws_lambda_powertools/utilities/feature_flags/comparators.py +++ b/aws_lambda_powertools/utilities/feature_flags/comparators.py @@ -5,7 +5,6 @@ from dateutil.tz import gettz -from .exceptions import SchemaValidationError from .schema import HOUR_MIN_SEPARATOR, ModuloRangeValues, TimeValues @@ -102,8 +101,8 @@ def compare_any_in_list(context_value: list, condition_value: list) -> bool: bool Whether any list item in context_value is available in condition_value """ - if not (isinstance(context_value, list) and isinstance(condition_value, list)): - raise SchemaValidationError() + if not isinstance(context_value, list): + raise ValueError("Context provided must be a list. Unable to compare ANY_IN_VALUE action.") return any(key in condition_value for key in context_value) @@ -123,8 +122,8 @@ def compare_all_in_list(context_value: list, condition_value: list) -> bool: bool Whether all list items in context_value are available in condition_value """ - if not (isinstance(context_value, list) and isinstance(condition_value, list)): - raise SchemaValidationError() + if not isinstance(context_value, list): + raise ValueError("Context provided must be a list. Unable to compare ALL_IN_VALUE action.") return all(key in condition_value for key in context_value) @@ -144,7 +143,7 @@ def compare_none_in_list(context_value: list, condition_value: list) -> bool: bool Whether list items in context_value are **not** available in condition_value """ - if not (isinstance(context_value, list) and isinstance(condition_value, list)): - raise SchemaValidationError() + if not isinstance(context_value, list): + raise ValueError("Context provided must be a list. Unable to compare NONE_IN_VALUE action.") return all(key not in condition_value for key in context_value) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index a026b15b709..06803815a55 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -464,6 +464,21 @@ def _validate_modulo_range_value(value: dict, rule_name: str): f"condition with 'MODULO_RANGE' action must satisfy 0 <= START <= END <= BASE-1, rule={rule_name}", ) + @staticmethod + def _validate_all_in_value_value(value: list, rule_name: str): + if not (isinstance(value, list)): + raise SchemaValidationError(f"ALL_IN_VALUE action must have a list value, rule={rule_name}") + + @staticmethod + def _validate_any_in_value_value(value: list, rule_name: str): + if not (isinstance(value, list)): + raise SchemaValidationError(f"ANY_IN_VALUE action must have a list value, rule={rule_name}") + + @staticmethod + def _validate_none_in_value_value(value: list, rule_name: str): + if not (isinstance(value, list)): + raise SchemaValidationError(f"NONE_IN_VALUE action must have a list value, rule={rule_name}") + @staticmethod def _validate_datetime(datetime_str: str, rule_name: str): date = None diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py index 75f86cf5b25..670c405fe53 100644 --- a/tests/functional/feature_flags/test_schema_validation.py +++ b/tests/functional/feature_flags/test_schema_validation.py @@ -1029,3 +1029,48 @@ def test_validate_modulo_range_condition_valid(): # WHEN calling validate_condition # THEN nothing is raised ConditionsValidator.validate_condition_value(condition=condition, rule_name="dummy") + + +def test_validate_any_in_value_condition_invalid_value(): + # GIVEN a schema with a ANY_IN_VALUE action with non-list value + condition = { + CONDITION_ACTION: RuleAction.ANY_IN_VALUE.value, + CONDITION_VALUE: "Gerald", + } + + rule_name = "non-list value for ANY_IN_VALUE" + + # WHEN calling validate_condition + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="ANY_IN_VALUE action must have a list"): + ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) + + +def test_validate_all_in_value_condition_invalid_value(): + # GIVEN a schema with a ANY_IN_VALUE action with non-list value + condition = { + CONDITION_ACTION: RuleAction.ALL_IN_VALUE.value, + CONDITION_VALUE: "Leandro", + } + + rule_name = "non-list value for ALL_IN_VALUE" + + # WHEN calling validate_condition + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="ALL_IN_VALUE action must have a list"): + ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) + + +def test_validate_none_in_value_condition_invalid_value(): + # GIVEN a schema with a ANY_IN_VALUE action with non-list value + condition = { + CONDITION_ACTION: RuleAction.NONE_IN_VALUE.value, + CONDITION_VALUE: "Heitor", + } + + rule_name = "non-list value for NONE_IN_VALUE" + + # WHEN calling validate_condition + # THEN raise SchemaValidationError + with pytest.raises(SchemaValidationError, match="NONE_IN_VALUE action must have a list"): + ConditionsValidator.validate_condition_value(condition=condition, rule_name=rule_name) From ba89f4e0b00d56e46e91f996034a55c591937b51 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 16:44:04 +0100 Subject: [PATCH 18/27] chore: test non-list intersection contexts --- .../feature_flags/test_feature_flags.py | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 5b4089a2591..da5de7f984c 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1588,3 +1588,45 @@ def test_flags_none_in_value_no_match(mocker, config): ) assert toggle == expected_value + + +@pytest.mark.parametrize( + "intersection_action", + [ + RuleAction.ALL_IN_VALUE.value, + RuleAction.ANY_IN_VALUE.value, + RuleAction.NONE_IN_VALUE.value, + ], +) +def test_intersection_non_list_value(mocker, config, intersection_action): + # GIVEN a schema with list intersection action + expected_value = False + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": intersection_action, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + + # WHEN a context value isn't a list + toggle = feature_flags.evaluate( + name="my_feature", + context={"tenant_id": "not a list value"}, + default=False, + ) + + # THEN TypeError should be swallowed and use default value + assert toggle == expected_value From 62c3f18ae31c529fefc97c85ef5a0fe968097881 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 16:58:22 +0100 Subject: [PATCH 19/27] refactor: reduce quadractic loop --- .../utilities/feature_flags/schema.py | 22 +++++++++---------- 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 06803815a55..2f9984551dc 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -76,6 +76,10 @@ class TimeValues(Enum): FRIDAY = "FRIDAY" SATURDAY = "SATURDAY" + @classmethod + def days(cls) -> list[str]: + return [day.value for day in cls if day.value not in ["START", "END", "TIMEZONE"]] + class ModuloRangeValues(Enum): """ @@ -375,16 +379,10 @@ def _validate_schedule_between_days_of_week_value(value: dict, rule_name: str): days = value.get(TimeValues.DAYS.value) if not isinstance(days, list) or not value: raise SchemaValidationError(error_str) + + valid_days = TimeValues.days() for day in days: - if not isinstance(day, str) or day not in [ - TimeValues.MONDAY.value, - TimeValues.TUESDAY.value, - TimeValues.WEDNESDAY.value, - TimeValues.THURSDAY.value, - TimeValues.FRIDAY.value, - TimeValues.SATURDAY.value, - TimeValues.SUNDAY.value, - ]: + if not isinstance(day, str) or day not in valid_days: raise SchemaValidationError( f"condition value DAYS must represent a day of the week in 'TimeValues' enum, rule={rule_name}", ) @@ -399,7 +397,7 @@ def _validate_schedule_between_time_range_key(key: str, rule_name: str): ) @staticmethod - def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> bool: + def _validate_schedule_between_time_range_value(value: Dict, rule_name: str): if not isinstance(value, dict): raise SchemaValidationError( f"{RuleAction.SCHEDULE_BETWEEN_TIME_RANGE.value} action must have a dictionary with 'START' and 'END' keys, rule={rule_name}", # noqa: E501 @@ -419,8 +417,6 @@ def _validate_schedule_between_time_range_value(value: Dict, rule_name: str) -> ConditionsValidator._validate_timezone(timezone=value.get(TimeValues.TIMEZONE.value), rule=rule_name) - return True - @staticmethod def _validate_schedule_between_datetime_range_key(key: str, rule_name: str): if key != TimeKeys.CURRENT_DATETIME.value: @@ -454,8 +450,10 @@ def _validate_modulo_range_value(value: dict, rule_name: str): base = value.get(ModuloRangeValues.BASE.value) start = value.get(ModuloRangeValues.START.value) end = value.get(ModuloRangeValues.END.value) + if base is None or start is None or end is None: raise SchemaValidationError(error_str) + if not isinstance(base, int) or not isinstance(start, int) or not isinstance(end, int): raise SchemaValidationError(f"'BASE', 'START' and 'END' must be integers, rule={rule_name}") From b34a3d86c294e1a8e6333ec45f3418173ee5d85b Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Fri, 16 Feb 2024 18:37:30 +0100 Subject: [PATCH 20/27] feat: add exception handler mechanism --- .../utilities/feature_flags/feature_flags.py | 36 +++++++++++++++++- .../feature_flags/test_feature_flags.py | 37 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 32486e5ea45..ef9fb46cae3 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -1,5 +1,9 @@ +from __future__ import annotations + import logging -from typing import Any, Dict, List, Optional, Union, cast +from typing import Any, Callable, Dict, List, Optional, TypeVar, Union, cast + +from typing_extensions import ParamSpec from ... import Logger from ...shared.types import JSONType @@ -16,6 +20,9 @@ ) from .exceptions import ConfigurationStoreError +T = TypeVar("T") +P = ParamSpec("P") + RULE_ACTION_MAPPING = { schema.RuleAction.EQUALS.value: lambda a, b: a == b, schema.RuleAction.NOT_EQUALS.value: lambda a, b: a != b, @@ -73,6 +80,7 @@ def __init__(self, store: StoreProvider, logger: Optional[Union[logging.Logger, """ self.store = store self.logger = logger or logging.getLogger(__name__) + self._exception_handlers: dict[Exception, Callable] = {} def _match_by_action(self, action: str, condition_value: Any, context_value: Any) -> bool: try: @@ -80,6 +88,12 @@ def _match_by_action(self, action: str, condition_value: Any, context_value: Any return func(context_value, condition_value) except Exception as exc: self.logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") + + handler = self._lookup_exception_handler(exc) + if handler: + self.logger.debug("Exception handler found! Delegating response.") + return handler(exc) + return False def _evaluate_conditions( @@ -335,3 +349,23 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L features_enabled.append(name) return features_enabled + + def exception_handler(self, exc_class: Exception | list[Exception]): + def register_exception_handler(func: Callable[P, T]) -> Callable[P, T]: + if isinstance(exc_class, list): + for exp in exc_class: + self._exception_handlers[exp] = func + else: + self._exception_handlers[exc_class] = func + + return func + + return register_exception_handler + + def _lookup_exception_handler(self, exc: BaseException) -> Callable | None: + # Use "Method Resolution Order" to allow for matching against a base class + # of an exception + for cls in type(exc).__mro__: + if cls in self._exception_handlers: + return self._exception_handlers[cls] # type: ignore[index] # index is correct + return None diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index da5de7f984c..13c7c1574dc 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1630,3 +1630,40 @@ def test_intersection_non_list_value(mocker, config, intersection_action): # THEN TypeError should be swallowed and use default value assert toggle == expected_value + + +def test_exception_handler(mocker, config): + # GIVEN a schema with list intersection action + expected_value = False + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": expected_value, + "conditions": [ + { + "action": RuleAction.ANY_IN_VALUE.value, + "key": "tenant_id", + "value": ["Łukasz", "Gerald", "Leandro", "Heitor"], + }, + ], + }, + }, + }, + } + + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + + @feature_flags.exception_handler(ValueError) + def catch_exception(exc): + raise TypeError("re-raised") + + # WHEN a context value isn't a list + # THEN exception handler should be able to intercept and raise, instead of returning `False` + with pytest.raises(TypeError): + feature_flags.evaluate( + name="my_feature", + context={"tenant_id": "not a list value"}, + default=False, + ) From 4936897dfa8fe0cfe9b0c2148c4cd82fea1ef8a3 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 09:40:10 +0100 Subject: [PATCH 21/27] chore: improve docstring Signed-off-by: heitorlessa --- .../utilities/feature_flags/feature_flags.py | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index ef9fb46cae3..857b4d7126f 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -223,6 +223,22 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau 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 + ┌────────────────────────┐ ┌────────────────────────┐ ┌────────────────────────┐ + │ Feature flags │──────▶ Get Configuration ├───────▶ Evaluate rules │ + └────────────────────────┘ │ │ │ │ + │┌──────────────────────┐│ │┌──────────────────────┐│ + ││ Fetch schema ││ ││ Match rule ││ + │└───────────┬──────────┘│ │└───────────┬──────────┘│ + │ │ │ │ │ │ + │┌───────────▼──────────┐│ │┌───────────▼──────────┐│ + ││ Cache schema ││ ││ Match condition ││ + │└───────────┬──────────┘│ │└───────────┬──────────┘│ + │ │ │ │ │ │ + │┌───────────▼──────────┐│ │┌───────────▼──────────┐│ + ││ Validate schema ││ ││ Match action ││ + │└──────────────────────┘│ │└──────────────────────┘│ + └────────────────────────┘ └────────────────────────┘ + Parameters ---------- name: str @@ -236,6 +252,31 @@ def evaluate(self, *, name: str, context: Optional[Dict[str, Any]] = None, defau or there has been an error when fetching the configuration from the store Can be boolean or any JSON values for non-boolean features. + + Examples + -------- + + ```python + from aws_lambda_powertools.utilities.feature_flags import AppConfigStore, FeatureFlags + from aws_lambda_powertools.utilities.typing import LambdaContext + + app_config = AppConfigStore(environment="dev", application="product-catalogue", name="features") + + feature_flags = FeatureFlags(store=app_config) + + + def lambda_handler(event: dict, context: LambdaContext): + # Get customer's tier from incoming request + ctx = {"tier": event.get("tier", "standard")} + + # Evaluate whether customer's tier has access to premium features + # based on `has_premium_features` rules + has_premium_features: bool = feature_flags.evaluate(name="premium_features", context=ctx, default=False) + if has_premium_features: + # enable premium features + ... + ``` + Returns ------ JSONType @@ -351,6 +392,28 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L return features_enabled def exception_handler(self, exc_class: Exception | list[Exception]): + """Registers function to handle unexpected exceptions when evaluating flags. + + It does not override the function of a default flag value in case of network and IAM permissions. + For example, you won't be able to catch ConfigurationStoreError exception. + + Parameters + ---------- + exc_class : Exception | list[Exception] + One or more exceptions to catch + + Examples + -------- + + ```python + feature_flags = FeatureFlags(store=app_config) + + @feature_flags.exception_handler(Exception) # any exception + def catch_exception(exc): + raise TypeError("re-raised") from exc + ``` + """ + def register_exception_handler(func: Callable[P, T]) -> Callable[P, T]: if isinstance(exc_class, list): for exp in exc_class: From f8e7c443caa31e486fc3eba688c61a5835a0ba92 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 09:56:18 +0100 Subject: [PATCH 22/27] chore: explain getattr dispatcher over if/elif/elif Signed-off-by: heitorlessa --- .../utilities/feature_flags/schema.py | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 2f9984551dc..e3cb3e2162f 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -332,15 +332,22 @@ def validate_condition_key(condition: Dict[str, Any], rule_name: str): raise SchemaValidationError(f"'key' value must be a non empty string, rule={rule_name}") action = condition.get(CONDITION_ACTION, "") - # maintenance: we may need a new design if we end up with more exceptions like datetime/time range - # e.g., visitor pattern, registry etc. - validator = getattr( + + # To allow for growth and prevent if/elif chains, we align extra validators based on the action name. + # for example: + # + # SCHEDULE_BETWEEN_DAYS_OF_WEEK_KEY + # - extra validation: `_validate_schedule_between_days_of_week_key` + # + # maintenance: we can split to separate file and classes for better organization later, e.g., visitor pattern. + + custom_validator = getattr( ConditionsValidator, f"_validate_{action.lower()}_key", ConditionsValidator._validate_noop_value, ) - validator(key, rule_name) + custom_validator(key, rule_name) @staticmethod def validate_condition_value(condition: Dict[str, Any], rule_name: str): @@ -349,15 +356,21 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): raise SchemaValidationError(f"'value' key must not be null, rule={rule_name}") action = condition.get(CONDITION_ACTION, "") - # maintenance: we may need a new design if we end up with more exceptions like datetime/time range - # e.g., visitor pattern, registry etc. - validator = getattr( + # To allow for growth and prevent if/elif chains, we align extra validators based on the action name. + # for example: + # + # SCHEDULE_BETWEEN_DAYS_OF_WEEK_KEY + # - extra validation: `_validate_schedule_between_days_of_week_value` + # + # maintenance: we can split to separate file and classes for better organization later, e.g., visitor pattern. + + custom_validator = getattr( ConditionsValidator, f"_validate_{action.lower()}_value", ConditionsValidator._validate_noop_value, ) - validator(value, rule_name) + custom_validator(value, rule_name) @staticmethod def _validate_noop_value(*args, **kwargs): From 9473c2837c09a418d710281fd973b13e5bdcbcad Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 10:16:17 +0100 Subject: [PATCH 23/27] chore: rename to validation_exception_handler --- .../utilities/feature_flags/feature_flags.py | 4 ++-- tests/functional/feature_flags/test_feature_flags.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 857b4d7126f..013542f1636 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -391,7 +391,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L return features_enabled - def exception_handler(self, exc_class: Exception | list[Exception]): + def validation_exception_handler(self, exc_class: Exception | list[Exception]): """Registers function to handle unexpected exceptions when evaluating flags. It does not override the function of a default flag value in case of network and IAM permissions. @@ -408,7 +408,7 @@ def exception_handler(self, exc_class: Exception | list[Exception]): ```python feature_flags = FeatureFlags(store=app_config) - @feature_flags.exception_handler(Exception) # any exception + @feature_flags.validation_exception_handler(Exception) # any exception def catch_exception(exc): raise TypeError("re-raised") from exc ``` diff --git a/tests/functional/feature_flags/test_feature_flags.py b/tests/functional/feature_flags/test_feature_flags.py index 13c7c1574dc..cc6aa60aaac 100644 --- a/tests/functional/feature_flags/test_feature_flags.py +++ b/tests/functional/feature_flags/test_feature_flags.py @@ -1655,7 +1655,7 @@ def test_exception_handler(mocker, config): feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) - @feature_flags.exception_handler(ValueError) + @feature_flags.validation_exception_handler(ValueError) def catch_exception(exc): raise TypeError("re-raised") From 2828fe6fe83dbc84ca4b58059a5183426068f9f8 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 10:53:11 +0100 Subject: [PATCH 24/27] refactor: use LRU cache for days classmethod --- aws_lambda_powertools/utilities/feature_flags/schema.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index e3cb3e2162f..a89d01d695d 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -4,6 +4,7 @@ import re from datetime import datetime from enum import Enum +from functools import lru_cache from typing import Any, Dict, List, Optional, Union from dateutil import tz @@ -77,6 +78,7 @@ class TimeValues(Enum): SATURDAY = "SATURDAY" @classmethod + @lru_cache(maxsize=1) def days(cls) -> list[str]: return [day.value for day in cls if day.value not in ["START", "END", "TIMEZONE"]] From 8e115bb554db118285f4fb717590fce7bdba184c Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 11:38:57 +0100 Subject: [PATCH 25/27] chore: log debug custom validators Signed-off-by: heitorlessa --- .../utilities/feature_flags/schema.py | 55 ++++++++++++------- .../feature_flags/test_schema_validation.py | 6 +- 2 files changed, 37 insertions(+), 24 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index a89d01d695d..2cabd714509 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -25,6 +25,8 @@ TIME_RANGE_PATTERN = re.compile(r"2[0-3]:[0-5]\d|[0-1]\d:[0-5]\d") # 24 hour clock HOUR_MIN_SEPARATOR = ":" +LOGGER: logging.Logger | Logger = logging.getLogger(__name__) + class RuleAction(str, Enum): EQUALS = "EQUALS" @@ -196,7 +198,12 @@ class SchemaValidator(BaseValidator): def __init__(self, schema: Dict[str, Any], logger: Optional[Union[logging.Logger, Logger]] = None): self.schema = schema - self.logger = logger or logging.getLogger(__name__) + self.logger = logger or LOGGER + + # Validators are designed for modular testing + # therefore we link the custom logger with global LOGGER + # so custom validators can use them when necessary + SchemaValidator._link_global_logger(self.logger) def validate(self) -> None: self.logger.debug("Validating schema") @@ -206,13 +213,18 @@ def validate(self) -> None: features = FeaturesValidator(schema=self.schema, logger=self.logger) features.validate() + @staticmethod + def _link_global_logger(logger: logging.Logger | Logger): + global LOGGER + LOGGER = logger + class FeaturesValidator(BaseValidator): """Validates each feature and calls RulesValidator to validate its rules""" def __init__(self, schema: Dict, logger: Optional[Union[logging.Logger, Logger]] = None): self.schema = schema - self.logger = logger or logging.getLogger(__name__) + self.logger = logger or LOGGER def validate(self): for name, feature in self.schema.items(): @@ -250,7 +262,7 @@ def __init__( self.feature = feature self.feature_name = next(iter(self.feature)) self.rules: Optional[Dict] = self.feature.get(RULES_KEY) - self.logger = logger or logging.getLogger(__name__) + self.logger = logger or LOGGER self.boolean_feature = boolean_feature def validate(self): @@ -297,7 +309,7 @@ class ConditionsValidator(BaseValidator): def __init__(self, rule: Dict[str, Any], rule_name: str, logger: Optional[Union[logging.Logger, Logger]] = None): self.conditions: List[Dict[str, Any]] = rule.get(CONDITIONS_KEY, {}) self.rule_name = rule_name - self.logger = logger or logging.getLogger(__name__) + self.logger = logger or LOGGER def validate(self): if not self.conditions or not isinstance(self.conditions, list): @@ -341,14 +353,17 @@ def validate_condition_key(condition: Dict[str, Any], rule_name: str): # SCHEDULE_BETWEEN_DAYS_OF_WEEK_KEY # - extra validation: `_validate_schedule_between_days_of_week_key` # - # maintenance: we can split to separate file and classes for better organization later, e.g., visitor pattern. + # maintenance: we should split to separate file/classes for better organization, e.g., visitor pattern. + + custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_key") - custom_validator = getattr( - ConditionsValidator, - f"_validate_{action.lower()}_key", - ConditionsValidator._validate_noop_value, - ) + # ~90% of actions available don't require a custom validator + # logging a debug statement for no-match will increase CPU cycles for most customers + # for that reason only, we invert and log only when extra validation is found. + if custom_validator is None: + return + LOGGER.debug(f"{action} requires key validation. Running '{custom_validator}' validator.") custom_validator(key, rule_name) @staticmethod @@ -364,19 +379,19 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): # SCHEDULE_BETWEEN_DAYS_OF_WEEK_KEY # - extra validation: `_validate_schedule_between_days_of_week_value` # - # maintenance: we can split to separate file and classes for better organization later, e.g., visitor pattern. + # maintenance: we should split to separate file/classes for better organization, e.g., visitor pattern. - custom_validator = getattr( - ConditionsValidator, - f"_validate_{action.lower()}_value", - ConditionsValidator._validate_noop_value, - ) + custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_value") - custom_validator(value, rule_name) + # ~90% of actions available don't require a custom validator + # logging a debug statement for no-match will increase CPU cycles for most customers + # for that reason only, we invert and log only when extra validation is found. + if custom_validator is None: + return - @staticmethod - def _validate_noop_value(*args, **kwargs): - return True + LOGGER.debug(f"{action} requires value validation. Running '{custom_validator}' validator.") + + custom_validator(value, rule_name) @staticmethod def _validate_schedule_between_days_of_week_key(key: str, rule_name: str): diff --git a/tests/functional/feature_flags/test_schema_validation.py b/tests/functional/feature_flags/test_schema_validation.py index 670c405fe53..45b4c7dbeda 100644 --- a/tests/functional/feature_flags/test_schema_validation.py +++ b/tests/functional/feature_flags/test_schema_validation.py @@ -1,8 +1,8 @@ -import logging import re -import pytest # noqa: F401 +import pytest +from aws_lambda_powertools.logging.logger import Logger # noqa: F401 from aws_lambda_powertools.utilities.feature_flags.exceptions import ( SchemaValidationError, ) @@ -24,8 +24,6 @@ TimeValues, ) -logger = logging.getLogger(__name__) - EMPTY_SCHEMA = {"": ""} From 4ec3c7ee230aa4ea4fd0e6627fdfd3ff8bf455d0 Mon Sep 17 00:00:00 2001 From: heitorlessa Date: Mon, 19 Feb 2024 11:47:59 +0100 Subject: [PATCH 26/27] chore: fix missing sentinel getattr --- aws_lambda_powertools/utilities/feature_flags/schema.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/schema.py b/aws_lambda_powertools/utilities/feature_flags/schema.py index 2cabd714509..1df16677bd8 100644 --- a/aws_lambda_powertools/utilities/feature_flags/schema.py +++ b/aws_lambda_powertools/utilities/feature_flags/schema.py @@ -355,7 +355,7 @@ def validate_condition_key(condition: Dict[str, Any], rule_name: str): # # maintenance: we should split to separate file/classes for better organization, e.g., visitor pattern. - custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_key") + custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_key", None) # ~90% of actions available don't require a custom validator # logging a debug statement for no-match will increase CPU cycles for most customers @@ -381,7 +381,7 @@ def validate_condition_value(condition: Dict[str, Any], rule_name: str): # # maintenance: we should split to separate file/classes for better organization, e.g., visitor pattern. - custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_value") + custom_validator = getattr(ConditionsValidator, f"_validate_{action.lower()}_value", None) # ~90% of actions available don't require a custom validator # logging a debug statement for no-match will increase CPU cycles for most customers From 9d149f11149cb94e8066a4494f653bdaf20f18f7 Mon Sep 17 00:00:00 2001 From: Heitor Lessa Date: Mon, 19 Feb 2024 12:00:00 +0100 Subject: [PATCH 27/27] chore: typo Co-authored-by: Ruben Fonseca Signed-off-by: Heitor Lessa --- aws_lambda_powertools/utilities/feature_flags/feature_flags.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 013542f1636..bd7e19d0efe 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -392,7 +392,7 @@ def get_enabled_features(self, *, context: Optional[Dict[str, Any]] = None) -> L return features_enabled def validation_exception_handler(self, exc_class: Exception | list[Exception]): - """Registers function to handle unexpected exceptions when evaluating flags. + """Registers function to handle unexpected validation exceptions when evaluating flags. It does not override the function of a default flag value in case of network and IAM permissions. For example, you won't be able to catch ConfigurationStoreError exception.