From f1676e750b893a6ca012c1986b37d04bec69d970 Mon Sep 17 00:00:00 2001 From: Christian Lutnik Date: Mon, 13 Jan 2025 13:24:48 +0100 Subject: [PATCH 1/9] Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 9 ++++++--- openfeature/hook/__init__.py | 2 +- openfeature/hook/_hook_support.py | 3 ++- tests/hook/test_hook_support.py | 7 +++++-- 4 files changed, 14 insertions(+), 7 deletions(-) diff --git a/openfeature/client.py b/openfeature/client.py index 1edfca63..602e3bca 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -325,6 +325,7 @@ def evaluate_flag_details( # noqa: PLR0915 error_code=ErrorCode.PROVIDER_FATAL, ) + flag_evaluation = None try: # https://github.com/open-feature/spec/blob/main/specification/sections/03-evaluation-context.md # Any resulting evaluation context from a before hook will overwrite @@ -364,13 +365,14 @@ def evaluate_flag_details( # noqa: PLR0915 except OpenFeatureError as err: error_hooks(flag_type, hook_context, err, reversed_merged_hooks, hook_hints) - return FlagEvaluationDetails( + flag_evaluation = FlagEvaluationDetails( flag_key=flag_key, value=default_value, reason=Reason.ERROR, error_code=err.error_code, error_message=err.error_message, ) + return flag_evaluation # Catch any type of exception here since the user can provide any exception # in the error hooks except Exception as err: # pragma: no cover @@ -381,16 +383,17 @@ def evaluate_flag_details( # noqa: PLR0915 error_hooks(flag_type, hook_context, err, reversed_merged_hooks, hook_hints) error_message = getattr(err, "error_message", str(err)) - return FlagEvaluationDetails( + flag_evaluation = FlagEvaluationDetails( flag_key=flag_key, value=default_value, reason=Reason.ERROR, error_code=ErrorCode.GENERAL, error_message=error_message, ) + return flag_evaluation finally: - after_all_hooks(flag_type, hook_context, reversed_merged_hooks, hook_hints) + after_all_hooks(flag_type, hook_context, flag_evaluation, reversed_merged_hooks, hook_hints) def _create_provider_evaluation( self, diff --git a/openfeature/hook/__init__.py b/openfeature/hook/__init__.py index 77190dc0..8e4189e3 100644 --- a/openfeature/hook/__init__.py +++ b/openfeature/hook/__init__.py @@ -109,7 +109,7 @@ def error( """ pass - def finally_after(self, hook_context: HookContext, hints: HookHints) -> None: + def finally_after(self, hook_context: HookContext, details: FlagEvaluationDetails[typing.Any], hints: HookHints) -> None: """ Run after flag evaluation, including any error processing. This will always run. Errors will be swallowed. diff --git a/openfeature/hook/_hook_support.py b/openfeature/hook/_hook_support.py index 349b25f3..2c151ae1 100644 --- a/openfeature/hook/_hook_support.py +++ b/openfeature/hook/_hook_support.py @@ -25,10 +25,11 @@ def error_hooks( def after_all_hooks( flag_type: FlagType, hook_context: HookContext, + details: FlagEvaluationDetails[typing.Any], hooks: typing.List[Hook], hints: typing.Optional[HookHints] = None, ) -> None: - kwargs = {"hook_context": hook_context, "hints": hints} + kwargs = {"hook_context": hook_context, "details": details, "hints": hints} _execute_hooks( flag_type=flag_type, hooks=hooks, hook_method=HookType.FINALLY_AFTER, **kwargs ) diff --git a/tests/hook/test_hook_support.py b/tests/hook/test_hook_support.py index 64bb8f6f..37ba0637 100644 --- a/tests/hook/test_hook_support.py +++ b/tests/hook/test_hook_support.py @@ -137,12 +137,15 @@ def test_after_hooks_run_after_method(mock_hook): def test_finally_after_hooks_run_finally_after_method(mock_hook): # Given hook_context = HookContext("flag_key", FlagType.BOOLEAN, True, "") + flag_evaluation_details = FlagEvaluationDetails( + hook_context.flag_key, "val", "unknown" + ) hook_hints = MappingProxyType({}) # When - after_all_hooks(FlagType.BOOLEAN, hook_context, [mock_hook], hook_hints) + after_all_hooks(FlagType.BOOLEAN, hook_context, flag_evaluation_details, [mock_hook], hook_hints) # Then mock_hook.supports_flag_value_type.assert_called_once() mock_hook.finally_after.assert_called_once() mock_hook.finally_after.assert_called_with( - hook_context=hook_context, hints=hook_hints + hook_context=hook_context, details=flag_evaluation_details, hints=hook_hints ) From 71bedbde05817839bc2d9939f60e017bd5559a79 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Mon, 13 Jan 2025 13:36:21 +0100 Subject: [PATCH 2/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- tests/hook/test_hook_support.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/hook/test_hook_support.py b/tests/hook/test_hook_support.py index 37ba0637..fb6f8fa1 100644 --- a/tests/hook/test_hook_support.py +++ b/tests/hook/test_hook_support.py @@ -147,5 +147,7 @@ def test_finally_after_hooks_run_finally_after_method(mock_hook): mock_hook.supports_flag_value_type.assert_called_once() mock_hook.finally_after.assert_called_once() mock_hook.finally_after.assert_called_with( - hook_context=hook_context, details=flag_evaluation_details, hints=hook_hints + hook_context=hook_context, + details=flag_evaluation_details, + hints=hook_hints ) From fc97b1bdcf2d6d5015e3276d69458c1a373c9463 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 14 Jan 2025 15:00:06 +0100 Subject: [PATCH 3/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- tests/features/steps/hooks_steps.py | 71 +++++++++++++++++++++++++++++ tests/features/steps/steps.py | 3 +- 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 tests/features/steps/hooks_steps.py diff --git a/tests/features/steps/hooks_steps.py b/tests/features/steps/hooks_steps.py new file mode 100644 index 00000000..f86e5324 --- /dev/null +++ b/tests/features/steps/hooks_steps.py @@ -0,0 +1,71 @@ +from unittest.mock import MagicMock + +from behave import then, when + +from openfeature.exception import ErrorCode +from openfeature.hook import Hook + + +@when("a hook is added to the client") +def step_impl(context): + hook = MagicMock(spec=Hook) + hook.before = MagicMock() + hook.after = MagicMock() + hook.error = MagicMock() + hook.finally_after = MagicMock() + context.hook = hook + context.client.add_hooks([hook]) + + +@then('error hooks should be called') +def step_impl(context): + assert context.hook.before.called + assert context.hook.error.called + assert context.hook.finally_after.called + + +@then('non-error hooks should be called') +def step_impl(context): + assert context.hook.before.called + assert context.hook.after.called + assert context.hook.finally_after.called + + +def get_hook_from_name(context, hook_name): + if hook_name.lower() == "before": + return context.hook.before + elif hook_name.lower() == "after": + return context.hook.after + elif hook_name.lower() == "error": + return context.hook.error + elif hook_name.lower() == "finally_after" or hook_name.lower() == "finally after": + return context.hook.finally_after + else: + raise ValueError(str(hook_name) + " is not a valid hook name") + + +def convert_value_from_flag_type(value, flag_type): + if value == "None": + return None + if flag_type.lower() == 'boolean': + return bool(value) + elif flag_type.lower() == 'integer': + return int(value) + elif flag_type.lower() == 'float': + return float(value) + return value + +@then('"{hook_names}" hooks should have evaluation details') +def step_impl(context, hook_names): + for hook_name in hook_names.split(', '): + hook = get_hook_from_name(context, hook_name) + for row in context.table: + flag_type, key, value = row + + value = convert_value_from_flag_type(value, flag_type) + + actual = hook.call_args[1]['details'].__dict__[key] + if isinstance(actual, ErrorCode): + actual = str(actual) + + assert actual == value diff --git a/tests/features/steps/steps.py b/tests/features/steps/steps.py index ff517dfa..5cb8cae3 100644 --- a/tests/features/steps/steps.py +++ b/tests/features/steps/steps.py @@ -35,7 +35,8 @@ def step_impl(context): '"{default_value}"' ) def step_impl(context, flag_type, key, default_value): - context.client = get_client() + if context.client is None: + context.client = get_client() if flag_type == "boolean": context.boolean_flag_details = context.client.get_boolean_details( key, default_value From d0d74cfb332d5402e07c5d48e01b3ce556b36630 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 28 Jan 2025 10:22:25 +0100 Subject: [PATCH 4/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 2e076acd..50abed87 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,7 +49,7 @@ cov = [ ] e2e = [ "git submodule add --force https://github.com/open-feature/spec.git spec", - "cp -r spec/specification/assets/gherkin/evaluation.feature tests/features/", + "cp spec/specification/assets/gherkin/* tests/features/", "behave tests/features/", "rm tests/features/*.feature", ] From e803ae6cfa88979c6025c4ac4f107455d0d75f80 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 28 Jan 2025 10:35:01 +0100 Subject: [PATCH 5/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 8 +++++++- openfeature/hook/__init__.py | 7 ++++++- tests/features/steps/hooks_steps.py | 22 +++++++++++----------- tests/hook/test_hook_support.py | 10 +++++----- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/openfeature/client.py b/openfeature/client.py index 602e3bca..1d1bc547 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -393,7 +393,13 @@ def evaluate_flag_details( # noqa: PLR0915 return flag_evaluation finally: - after_all_hooks(flag_type, hook_context, flag_evaluation, reversed_merged_hooks, hook_hints) + after_all_hooks( + flag_type, + hook_context, + flag_evaluation, + reversed_merged_hooks, + hook_hints + ) def _create_provider_evaluation( self, diff --git a/openfeature/hook/__init__.py b/openfeature/hook/__init__.py index 8e4189e3..b765e64d 100644 --- a/openfeature/hook/__init__.py +++ b/openfeature/hook/__init__.py @@ -109,7 +109,12 @@ def error( """ pass - def finally_after(self, hook_context: HookContext, details: FlagEvaluationDetails[typing.Any], hints: HookHints) -> None: + def finally_after( + self, + hook_context: HookContext, + details: FlagEvaluationDetails[typing.Any], + hints: HookHints + ) -> None: """ Run after flag evaluation, including any error processing. This will always run. Errors will be swallowed. diff --git a/tests/features/steps/hooks_steps.py b/tests/features/steps/hooks_steps.py index f86e5324..5cf34d5a 100644 --- a/tests/features/steps/hooks_steps.py +++ b/tests/features/steps/hooks_steps.py @@ -6,8 +6,8 @@ from openfeature.hook import Hook -@when("a hook is added to the client") -def step_impl(context): +@when('a hook is added to the client') +def step_impl_add_hook(context): hook = MagicMock(spec=Hook) hook.before = MagicMock() hook.after = MagicMock() @@ -18,34 +18,34 @@ def step_impl(context): @then('error hooks should be called') -def step_impl(context): +def step_impl_call_error(context): assert context.hook.before.called assert context.hook.error.called assert context.hook.finally_after.called @then('non-error hooks should be called') -def step_impl(context): +def step_impl_call_non_error(context): assert context.hook.before.called assert context.hook.after.called assert context.hook.finally_after.called def get_hook_from_name(context, hook_name): - if hook_name.lower() == "before": + if hook_name.lower() == 'before': return context.hook.before - elif hook_name.lower() == "after": + elif hook_name.lower() == 'after': return context.hook.after - elif hook_name.lower() == "error": + elif hook_name.lower() == 'error': return context.hook.error - elif hook_name.lower() == "finally_after" or hook_name.lower() == "finally after": + elif hook_name.lower() == 'finally': return context.hook.finally_after else: - raise ValueError(str(hook_name) + " is not a valid hook name") + raise ValueError(str(hook_name) + ' is not a valid hook name') def convert_value_from_flag_type(value, flag_type): - if value == "None": + if value == 'None': return None if flag_type.lower() == 'boolean': return bool(value) @@ -56,7 +56,7 @@ def convert_value_from_flag_type(value, flag_type): return value @then('"{hook_names}" hooks should have evaluation details') -def step_impl(context, hook_names): +def step_impl_should_have_eval_details(context, hook_names): for hook_name in hook_names.split(', '): hook = get_hook_from_name(context, hook_name) for row in context.table: diff --git a/tests/hook/test_hook_support.py b/tests/hook/test_hook_support.py index fb6f8fa1..5d0febff 100644 --- a/tests/hook/test_hook_support.py +++ b/tests/hook/test_hook_support.py @@ -136,18 +136,18 @@ def test_after_hooks_run_after_method(mock_hook): def test_finally_after_hooks_run_finally_after_method(mock_hook): # Given - hook_context = HookContext("flag_key", FlagType.BOOLEAN, True, "") + hook_context = HookContext("flag_key", FlagType.BOOLEAN, True, '') flag_evaluation_details = FlagEvaluationDetails( hook_context.flag_key, "val", "unknown" ) hook_hints = MappingProxyType({}) # When - after_all_hooks(FlagType.BOOLEAN, hook_context, flag_evaluation_details, [mock_hook], hook_hints) + after_all_hooks( + FlagType.BOOLEAN, hook_context, flag_evaluation_details, [mock_hook], hook_hints + ) # Then mock_hook.supports_flag_value_type.assert_called_once() mock_hook.finally_after.assert_called_once() mock_hook.finally_after.assert_called_with( - hook_context=hook_context, - details=flag_evaluation_details, - hints=hook_hints + hook_context=hook_context, details=flag_evaluation_details, hints=hook_hints ) From 6f718426fba2c6defa0c724bf5125976e52c11f3 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 28 Jan 2025 10:59:47 +0100 Subject: [PATCH 6/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 2 +- openfeature/hook/__init__.py | 2 +- tests/features/steps/hooks_steps.py | 28 ++++++++++++++-------------- tests/hook/test_hook_support.py | 2 +- 4 files changed, 17 insertions(+), 17 deletions(-) diff --git a/openfeature/client.py b/openfeature/client.py index 1d1bc547..9430bddd 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -398,7 +398,7 @@ def evaluate_flag_details( # noqa: PLR0915 hook_context, flag_evaluation, reversed_merged_hooks, - hook_hints + hook_hints, ) def _create_provider_evaluation( diff --git a/openfeature/hook/__init__.py b/openfeature/hook/__init__.py index b765e64d..03d8c865 100644 --- a/openfeature/hook/__init__.py +++ b/openfeature/hook/__init__.py @@ -113,7 +113,7 @@ def finally_after( self, hook_context: HookContext, details: FlagEvaluationDetails[typing.Any], - hints: HookHints + hints: HookHints, ) -> None: """ Run after flag evaluation, including any error processing. diff --git a/tests/features/steps/hooks_steps.py b/tests/features/steps/hooks_steps.py index 5cf34d5a..e527a5a2 100644 --- a/tests/features/steps/hooks_steps.py +++ b/tests/features/steps/hooks_steps.py @@ -6,7 +6,7 @@ from openfeature.hook import Hook -@when('a hook is added to the client') +@when("a hook is added to the client") def step_impl_add_hook(context): hook = MagicMock(spec=Hook) hook.before = MagicMock() @@ -17,14 +17,14 @@ def step_impl_add_hook(context): context.client.add_hooks([hook]) -@then('error hooks should be called') +@then("error hooks should be called") def step_impl_call_error(context): assert context.hook.before.called assert context.hook.error.called assert context.hook.finally_after.called -@then('non-error hooks should be called') +@then("non-error hooks should be called") def step_impl_call_non_error(context): assert context.hook.before.called assert context.hook.after.called @@ -32,39 +32,39 @@ def step_impl_call_non_error(context): def get_hook_from_name(context, hook_name): - if hook_name.lower() == 'before': + if hook_name.lower() == "before": return context.hook.before - elif hook_name.lower() == 'after': + elif hook_name.lower() == "after": return context.hook.after - elif hook_name.lower() == 'error': + elif hook_name.lower() == "error": return context.hook.error - elif hook_name.lower() == 'finally': + elif hook_name.lower() == "finally": return context.hook.finally_after else: - raise ValueError(str(hook_name) + ' is not a valid hook name') + raise ValueError(str(hook_name) + " is not a valid hook name") def convert_value_from_flag_type(value, flag_type): - if value == 'None': + if value == "None": return None - if flag_type.lower() == 'boolean': + if flag_type.lower() == "boolean": return bool(value) - elif flag_type.lower() == 'integer': + elif flag_type.lower() == "integer": return int(value) - elif flag_type.lower() == 'float': + elif flag_type.lower() == "float": return float(value) return value @then('"{hook_names}" hooks should have evaluation details') def step_impl_should_have_eval_details(context, hook_names): - for hook_name in hook_names.split(', '): + for hook_name in hook_names.split(", "): hook = get_hook_from_name(context, hook_name) for row in context.table: flag_type, key, value = row value = convert_value_from_flag_type(value, flag_type) - actual = hook.call_args[1]['details'].__dict__[key] + actual = hook.call_args[1]["details"].__dict__[key] if isinstance(actual, ErrorCode): actual = str(actual) diff --git a/tests/hook/test_hook_support.py b/tests/hook/test_hook_support.py index 5d0febff..19a86ec0 100644 --- a/tests/hook/test_hook_support.py +++ b/tests/hook/test_hook_support.py @@ -136,7 +136,7 @@ def test_after_hooks_run_after_method(mock_hook): def test_finally_after_hooks_run_finally_after_method(mock_hook): # Given - hook_context = HookContext("flag_key", FlagType.BOOLEAN, True, '') + hook_context = HookContext("flag_key", FlagType.BOOLEAN, True, "") flag_evaluation_details = FlagEvaluationDetails( hook_context.flag_key, "val", "unknown" ) From 07b512e3a3a418c62b99cc8335a06b559b6122d3 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 28 Jan 2025 11:04:22 +0100 Subject: [PATCH 7/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 9 +++++++++ tests/features/steps/hooks_steps.py | 1 + 2 files changed, 10 insertions(+) diff --git a/openfeature/client.py b/openfeature/client.py index 9430bddd..fd05e8a0 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -393,6 +393,15 @@ def evaluate_flag_details( # noqa: PLR0915 return flag_evaluation finally: + if flag_evaluation is None: # should never happen, but keeps the linter happy + flag_evaluation = FlagEvaluationDetails( + flag_key=flag_key, + value=default_value, + reason=Reason.ERROR, + error_code=ErrorCode.GENERAL, + error_message="Unknown error", + ) + after_all_hooks( flag_type, hook_context, diff --git a/tests/features/steps/hooks_steps.py b/tests/features/steps/hooks_steps.py index e527a5a2..bc7e156b 100644 --- a/tests/features/steps/hooks_steps.py +++ b/tests/features/steps/hooks_steps.py @@ -55,6 +55,7 @@ def convert_value_from_flag_type(value, flag_type): return float(value) return value + @then('"{hook_names}" hooks should have evaluation details') def step_impl_should_have_eval_details(context, hook_names): for hook_name in hook_names.split(", "): From 81f15df034b467f8a83a8921d4be35cccbee4860 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Tue, 28 Jan 2025 11:05:37 +0100 Subject: [PATCH 8/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/openfeature/client.py b/openfeature/client.py index fd05e8a0..bff383cb 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -393,7 +393,8 @@ def evaluate_flag_details( # noqa: PLR0915 return flag_evaluation finally: - if flag_evaluation is None: # should never happen, but keeps the linter happy + if flag_evaluation is None: + # should never happen, but keeps the linter happy flag_evaluation = FlagEvaluationDetails( flag_key=flag_key, value=default_value, From 66c4a8f69ebe501143c8bd5d1be5dc599f201895 Mon Sep 17 00:00:00 2001 From: "christian.lutnik" Date: Wed, 29 Jan 2025 11:22:28 +0100 Subject: [PATCH 9/9] fixup! Add evaluation details to finally hook stage #403 Signed-off-by: christian.lutnik --- openfeature/client.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/openfeature/client.py b/openfeature/client.py index bff383cb..c7f05353 100644 --- a/openfeature/client.py +++ b/openfeature/client.py @@ -325,7 +325,6 @@ def evaluate_flag_details( # noqa: PLR0915 error_code=ErrorCode.PROVIDER_FATAL, ) - flag_evaluation = None try: # https://github.com/open-feature/spec/blob/main/specification/sections/03-evaluation-context.md # Any resulting evaluation context from a before hook will overwrite @@ -393,16 +392,6 @@ def evaluate_flag_details( # noqa: PLR0915 return flag_evaluation finally: - if flag_evaluation is None: - # should never happen, but keeps the linter happy - flag_evaluation = FlagEvaluationDetails( - flag_key=flag_key, - value=default_value, - reason=Reason.ERROR, - error_code=ErrorCode.GENERAL, - error_message="Unknown error", - ) - after_all_hooks( flag_type, hook_context,