diff --git a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py index 19e96a8641d..30e1b77872b 100644 --- a/aws_lambda_powertools/utilities/feature_flags/feature_flags.py +++ b/aws_lambda_powertools/utilities/feature_flags/feature_flags.py @@ -82,10 +82,26 @@ def __init__(self, store: StoreProvider, logger: logging.Logger | Logger | None 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: + def _match_by_action( + self, + action: str, + condition_value: Any, + context_value: Any, + context_key_present: bool = True, + ) -> bool: try: func = RULE_ACTION_MAPPING.get(action, lambda a, b: False) - return func(context_value, condition_value) + matched = func(context_value, condition_value) + + if not context_key_present: + # A key absent from the context never satisfies a condition. Without this, `None` would be + # compared as a regular value and negative actions (NOT_EQUALS, NOT_IN, ...) would match. + # We still run the comparator first so that any exception it raises (e.g. ANY_IN_VALUE on a + # non-list) reaches registered validation exception handlers exactly as it did before. + self.logger.debug(f"context key not present, condition does not match: action={action}") + return False + + return matched except Exception as exc: self.logger.debug(f"caught exception while matching action: action={action}, exception={str(exc)}") @@ -115,9 +131,10 @@ def _evaluate_conditions( return False for condition in conditions: - context_value = context.get(condition.get(schema.CONDITION_KEY, "")) + cond_key = condition.get(schema.CONDITION_KEY, "") cond_action = condition.get(schema.CONDITION_ACTION, "") cond_value = condition.get(schema.CONDITION_VALUE) + context_key_present = True # time based rule actions have no user context. the context is the condition key if cond_action in ( @@ -125,9 +142,17 @@ def _evaluate_conditions( schema.RuleAction.SCHEDULE_BETWEEN_DATETIME_RANGE.value, schema.RuleAction.SCHEDULE_BETWEEN_DAYS_OF_WEEK.value, ): - context_value = condition.get(schema.CONDITION_KEY) # e.g., CURRENT_TIME - - if not self._match_by_action(action=cond_action, condition_value=cond_value, context_value=context_value): + context_value = cond_key # e.g., CURRENT_TIME + else: + context_key_present = cond_key in context + context_value = context.get(cond_key) + + if not self._match_by_action( + action=cond_action, + condition_value=cond_value, + context_value=context_value, + context_key_present=context_key_present, + ): self.logger.debug( f"rule did not match action, rule_name={rule_name}, rule_value={rule_match_value}, " f"name={feature_name}, context_value={str(context_value)} ", diff --git a/docs/utilities/feature_flags.md b/docs/utilities/feature_flags.md index 2d95e025b06..22e7d623bda 100644 --- a/docs/utilities/feature_flags.md +++ b/docs/utilities/feature_flags.md @@ -444,6 +444,8 @@ The `action` configuration can have the following values, where the expressions ???+ info The `key` and `value` will be compared to the input from the `context` parameter. + If a condition's `key` is not present in `context`, the condition never matches, regardless of the action. For example, a `NOT_EQUALS` rule on `tier` will not match a request that carries no `tier` at all. + ???+ "Time based keys" For time based keys, we provide a list of predefined keys. These will automatically get converted to the corresponding timestamp on each invocation of your Lambda function. diff --git a/tests/functional/feature_flags/_boto3/test_feature_flags.py b/tests/functional/feature_flags/_boto3/test_feature_flags.py index a4d271aba57..db221c08739 100644 --- a/tests/functional/feature_flags/_boto3/test_feature_flags.py +++ b/tests/functional/feature_flags/_boto3/test_feature_flags.py @@ -883,6 +883,71 @@ def test_flags_not_equal_match(mocker, config): assert toggle == expected_value +@pytest.mark.parametrize( + "action, value", + [ + (RuleAction.NOT_EQUALS.value, "premium"), + (RuleAction.NOT_IN.value, ["premium", "enterprise"]), + (RuleAction.KEY_NOT_IN_VALUE.value, ["premium", "enterprise"]), + (RuleAction.VALUE_NOT_IN_KEY.value, "premium"), + ], +) +def test_flags_negative_action_no_match_when_context_key_missing(mocker, config, action, value): + # GIVEN a rule with a negative action on key "tier" + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "non premium users": { + "when_match": True, + "conditions": [ + { + "action": action, + "key": "tier", + "value": value, + }, + ], + }, + }, + }, + } + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + + # WHEN evaluating with a context that doesn't carry "tier" at all + toggle = feature_flags.evaluate(name="my_feature", context={"username": "a"}, default=False) + + # THEN the rule must not match; a missing key never satisfies a condition + assert toggle is False + + +def test_flags_not_equal_match_when_context_key_is_none(mocker, config): + # GIVEN a NOT_EQUALS rule on key "tier" + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "non premium users": { + "when_match": True, + "conditions": [ + { + "action": RuleAction.NOT_EQUALS.value, + "key": "tier", + "value": "premium", + }, + ], + }, + }, + }, + } + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + + # WHEN the key is present but explicitly set to None + toggle = feature_flags.evaluate(name="my_feature", context={"tier": None}, default=False) + + # THEN the value is still compared as usual (None != "premium"), so the rule matches + assert toggle is True + + # Test less than def test_flags_less_than_no_match_1(mocker, config): expected_value = False @@ -1702,3 +1767,67 @@ def catch_exception(exc): context={"tenant_id": "not a list value"}, default=False, ) + + +def test_flags_missing_context_key_still_invokes_validation_exception_handler(mocker, config): + # GIVEN an ANY_IN_VALUE rule and a handler registered for the ValueError the comparator raises + # when the context value is not a list + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": True, + "conditions": [ + { + "action": RuleAction.ANY_IN_VALUE.value, + "key": "tenant_id", + "value": ["Akua", "John", "Maria", "Pat"], + }, + ], + }, + }, + }, + } + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + handled = [] + + @feature_flags.validation_exception_handler(ValueError) + def handle_invalid_context(exc): + handled.append(exc) + return True + + # WHEN the context does not carry the key at all + toggle = feature_flags.evaluate(name="my_feature", context={}, default=False) + + # THEN the handler is still called and its result is honoured, as it was before missing keys were guarded + assert len(handled) == 1 + assert toggle is True + + +def test_flags_missing_context_key_no_match_without_handler_for_raising_action(mocker, config): + # GIVEN an ANY_IN_VALUE rule and no exception handler registered + mocked_app_config_schema = { + "my_feature": { + "default": False, + "rules": { + "tenant_id is in allowed list": { + "when_match": True, + "conditions": [ + { + "action": RuleAction.ANY_IN_VALUE.value, + "key": "tenant_id", + "value": ["Akua", "John", "Maria", "Pat"], + }, + ], + }, + }, + }, + } + feature_flags = init_feature_flags(mocker, mocked_app_config_schema, config) + + # WHEN the context does not carry the key at all + toggle = feature_flags.evaluate(name="my_feature", context={}, default=False) + + # THEN the rule does not match + assert toggle is False