feat: Add evaluation details to finally hook stage #1246 (#1262)

Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com> Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
open-feature · Jan 9, 2025 · ae85278 · ae85278
1 parent 3c97b7b
commit ae85278
Show file tree

Hide file tree

Showing 8 changed files with 106 additions and 81 deletions.
diff --git a/README.md b/README.md
@@ -426,7 +426,7 @@ class MyHook implements Hook {
     }
 
     @Override
-    public void finallyAfter(HookContext ctx, Map hints) {
+    public void finallyAfter(HookContext ctx, FlagEvaluationDetails details, Map hints) {
         // code that runs regardless of success or error
     }
 };

diff --git a/src/main/java/dev/openfeature/sdk/Hook.java b/src/main/java/dev/openfeature/sdk/Hook.java
@@ -46,7 +46,7 @@ default void error(HookContext<T> ctx, Exception error, Map<String, Object> hint
      * @param ctx   Information about the particular flag evaluation
      * @param hints An immutable mapping of data for users to communicate to the hooks.
      */
-    default void finallyAfter(HookContext<T> ctx, Map<String, Object> hints) {}
+    default void finallyAfter(HookContext<T> ctx, FlagEvaluationDetails<T> details, Map<String, Object> hints) {}
 
     default boolean supportsFlagValueType(FlagValueType flagValueType) {
         return true;

diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java
@@ -29,8 +29,12 @@ public void afterHooks(
     }
 
     public void afterAllHooks(
-            FlagValueType flagValueType, HookContext hookCtx, List<Hook> hooks, Map<String, Object> hints) {
-        executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, hints));
+            FlagValueType flagValueType,
+            HookContext hookCtx,
+            FlagEvaluationDetails details,
+            List<Hook> hooks,
+            Map<String, Object> hints) {
+        executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, details, hints));
     }
 
     public void errorHooks(

diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
@@ -228,7 +228,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             enrichDetailsWithErrorDefaults(defaultValue, details);
             hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints);
         } finally {
-            hookSupport.afterAllHooks(type, afterHookContext, mergedHooks, hints);
+            hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints);
         }
 
         return details;

diff --git a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java
@@ -39,7 +39,7 @@ void clientHooks() {
         Client client = api.getClient();
         client.addHooks(exampleHook);
         Boolean retval = client.getBooleanValue(flagKey, false);
-        verify(exampleHook, times(1)).finallyAfter(any(), any());
+        verify(exampleHook, times(1)).finallyAfter(any(), any(), any());
         assertFalse(retval);
     }
 
@@ -57,8 +57,8 @@ void evalHooks() {
                 false,
                 null,
                 FlagEvaluationOptions.builder().hook(evalHook).build());
-        verify(clientHook, times(1)).finallyAfter(any(), any());
-        verify(evalHook, times(1)).finallyAfter(any(), any());
+        verify(clientHook, times(1)).finallyAfter(any(), any(), any());
+        verify(evalHook, times(1)).finallyAfter(any(), any(), any());
         assertFalse(retval);
     }
 

diff --git a/src/test/java/dev/openfeature/sdk/HookSpecTest.java b/src/test/java/dev/openfeature/sdk/HookSpecTest.java
@@ -1,10 +1,20 @@
 package dev.openfeature.sdk;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.fail;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.doThrow;
+import static org.mockito.Mockito.inOrder;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
 import dev.openfeature.sdk.exceptions.FlagNotFoundError;
 import dev.openfeature.sdk.fixtures.HookFixtures;
@@ -187,7 +197,7 @@ void feo_has_hook_list() {
     void error_hook_run_during_non_finally_stage() {
         final boolean[] error_called = {false};
         Hook h = mockBooleanHook();
-        doThrow(RuntimeException.class).when(h).finallyAfter(any(), any());
+        doThrow(RuntimeException.class).when(h).finallyAfter(any(), any(), any());
 
         verify(h, times(0)).error(any(), any(), any());
     }
@@ -219,7 +229,7 @@ void error_hook_must_run_if_resolution_details_returns_an_error_code() {
 
         verify(hook, times(1)).before(any(), any());
         verify(hook, times(1)).error(any(), captor.capture(), any());
-        verify(hook, times(1)).finallyAfter(any(), any());
+        verify(hook, times(1)).finallyAfter(any(), any(), any());
         verify(hook, never()).after(any(), any(), any());
 
         Exception exception = captor.getValue();
@@ -274,7 +284,10 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
                     }
 
                     @Override
-                    public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
+                    public void finallyAfter(
+                            HookContext<Boolean> ctx,
+                            FlagEvaluationDetails<Boolean> details,
+                            Map<String, Object> hints) {
                         evalOrder.add("provider finally");
                     }
                 });
@@ -300,7 +313,8 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
                 evalOrder.add("api finally");
             }
         });
@@ -325,7 +339,8 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
                 evalOrder.add("client finally");
             }
         });
@@ -357,7 +372,10 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
                             }
 
                             @Override
-                            public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
+                            public void finallyAfter(
+                                    HookContext<Boolean> ctx,
+                                    FlagEvaluationDetails<Boolean> details,
+                                    Map<String, Object> hints) {
                                 evalOrder.add("invocation finally");
                             }
                         })
@@ -462,7 +480,8 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
                 assertThatCode(() -> hints.put(hintKey, "changed value"))
                         .isInstanceOf(UnsupportedOperationException.class);
             }
@@ -509,7 +528,7 @@ void flag_eval_hook_order() {
         order.verify(hook).before(any(), any());
         order.verify(provider).getBooleanEvaluation(any(), any(), any());
         order.verify(hook).after(any(), any(), any());
-        order.verify(hook).finallyAfter(any(), any());
+        order.verify(hook).finallyAfter(any(), any(), any());
     }
 
     @Specification(
@@ -550,6 +569,58 @@ void error_hooks__after() {
         verify(hook, times(1)).error(any(), any(), any());
     }
 
+    @Test
+    void erroneous_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
+        Hook hook = mockBooleanHook();
+        doThrow(RuntimeException.class).when(hook).after(any(), any(), any());
+        String flagKey = "test-flag-key";
+        Client client = getClient(TestEventsProvider.newInitializedTestEventsProvider());
+        client.getBooleanValue(
+                flagKey,
+                true,
+                new ImmutableContext(),
+                FlagEvaluationOptions.builder().hook(hook).build());
+
+        ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
+        verify(hook).finallyAfter(any(), captor.capture(), any());
+
+        FlagEvaluationDetails<Boolean> evaluationDetails = captor.getValue();
+        assertThat(evaluationDetails).isNotNull();
+
+        assertThat(evaluationDetails.getErrorCode()).isEqualTo(ErrorCode.GENERAL);
+        assertThat(evaluationDetails.getReason()).isEqualTo("ERROR");
+        assertThat(evaluationDetails.getVariant()).isEqualTo("Passed in default");
+        assertThat(evaluationDetails.getFlagKey()).isEqualTo(flagKey);
+        assertThat(evaluationDetails.getFlagMetadata())
+                .isEqualTo(ImmutableMetadata.builder().build());
+        assertThat(evaluationDetails.getValue()).isTrue();
+    }
+
+    @Test
+    void successful_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
+        Hook hook = mockBooleanHook();
+        String flagKey = "test-flag-key";
+        Client client = getClient(TestEventsProvider.newInitializedTestEventsProvider());
+        client.getBooleanValue(
+                flagKey,
+                true,
+                new ImmutableContext(),
+                FlagEvaluationOptions.builder().hook(hook).build());
+
+        ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
+        verify(hook).finallyAfter(any(), captor.capture(), any());
+
+        FlagEvaluationDetails<Boolean> evaluationDetails = captor.getValue();
+        assertThat(evaluationDetails).isNotNull();
+        assertThat(evaluationDetails.getErrorCode()).isNull();
+        assertThat(evaluationDetails.getReason()).isEqualTo("DEFAULT");
+        assertThat(evaluationDetails.getVariant()).isEqualTo("Passed in default");
+        assertThat(evaluationDetails.getFlagKey()).isEqualTo(flagKey);
+        assertThat(evaluationDetails.getFlagMetadata())
+                .isEqualTo(ImmutableMetadata.builder().build());
+        assertThat(evaluationDetails.getValue()).isTrue();
+    }
+
     @Test
     void multi_hooks_early_out__before() {
         Hook<Boolean> hook = mockBooleanHook();
@@ -649,7 +720,7 @@ void mergeHappensCorrectly() {
     void first_finally_broken() {
         Hook hook = mockBooleanHook();
         doThrow(RuntimeException.class).when(hook).before(any(), any());
-        doThrow(RuntimeException.class).when(hook).finallyAfter(any(), any());
+        doThrow(RuntimeException.class).when(hook).finallyAfter(any(), any(), any());
         Hook hook2 = mockBooleanHook();
         InOrder order = inOrder(hook, hook2);
 
@@ -661,8 +732,8 @@ void first_finally_broken() {
                 FlagEvaluationOptions.builder().hook(hook2).hook(hook).build());
 
         order.verify(hook).before(any(), any());
-        order.verify(hook2).finallyAfter(any(), any());
-        order.verify(hook).finallyAfter(any(), any());
+        order.verify(hook2).finallyAfter(any(), any(), any());
+        order.verify(hook).finallyAfter(any(), any(), any());
     }
 
     @Specification(
@@ -711,7 +782,8 @@ void doesnt_use_finally() {
                 .as("Not possible. Finally is a reserved word.")
                 .isInstanceOf(NoSuchMethodException.class);
 
-        assertThatCode(() -> Hook.class.getMethod("finallyAfter", HookContext.class, Map.class))
+        assertThatCode(() ->
+                        Hook.class.getMethod("finallyAfter", HookContext.class, FlagEvaluationDetails.class, Map.class))
                 .doesNotThrowAnyException();
     }
 }
diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java
@@ -64,7 +64,11 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
                 Collections.singletonList(genericHook),
                 Collections.emptyMap());
         hookSupport.afterAllHooks(
-                flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
+                flagValueType,
+                hookContext,
+                FlagEvaluationDetails.builder().build(),
+                Collections.singletonList(genericHook),
+                Collections.emptyMap());
         hookSupport.errorHooks(
                 flagValueType,
                 hookContext,
@@ -74,7 +78,7 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
 
         verify(genericHook).before(any(), any());
         verify(genericHook).after(any(), any(), any());
-        verify(genericHook).finallyAfter(any(), any());
+        verify(genericHook).finallyAfter(any(), any(), any());
         verify(genericHook).error(any(), any(), any());
     }
 

diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java
@@ -5,13 +5,13 @@
 import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyString;
-import static org.mockito.Mockito.*;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
 
 import dev.openfeature.sdk.exceptions.FatalError;
 import dev.openfeature.sdk.fixtures.HookFixtures;
 import dev.openfeature.sdk.testutils.TestEventsProvider;
 import java.util.HashMap;
-import java.util.concurrent.atomic.AtomicBoolean;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
@@ -104,59 +104,4 @@ void shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState() {
 
         assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY);
     }
-
-    private static class MockProvider implements FeatureProvider {
-        private final AtomicBoolean evaluationCalled = new AtomicBoolean();
-        private final ProviderState providerState;
-
-        public MockProvider(ProviderState providerState) {
-            this.providerState = providerState;
-        }
-
-        public boolean isEvaluationCalled() {
-            return evaluationCalled.get();
-        }
-
-        @Override
-        public ProviderState getState() {
-            return providerState;
-        }
-
-        @Override
-        public Metadata getMetadata() {
-            return null;
-        }
-
-        @Override
-        public ProviderEvaluation<Boolean> getBooleanEvaluation(
-                String key, Boolean defaultValue, EvaluationContext ctx) {
-            evaluationCalled.set(true);
-            return null;
-        }
-
-        @Override
-        public ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) {
-            evaluationCalled.set(true);
-            return null;
-        }
-
-        @Override
-        public ProviderEvaluation<Integer> getIntegerEvaluation(
-                String key, Integer defaultValue, EvaluationContext ctx) {
-            evaluationCalled.set(true);
-            return null;
-        }
-
-        @Override
-        public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) {
-            evaluationCalled.set(true);
-            return null;
-        }
-
-        @Override
-        public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) {
-            evaluationCalled.set(true);
-            return null;
-        }
-    }
 }