feat: Add evaluation details to finally hook stage open-feature#1246

Signed-off-by: christian.lutnik <[email protected]>

Author: chrfwow

src/main/java/dev/openfeature/sdk/Hook.java

@@ -48,7 +48,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) {

src/main/java/dev/openfeature/sdk/HookSupport.java

@@ -25,9 +25,9 @@ public void afterHooks(FlagValueType flagValueType, HookContext hookContext, Fla
         executeHooksUnchecked(flagValueType, hooks, hook -> hook.after(hookContext, details, hints));
     }
 
-    public void afterAllHooks(FlagValueType flagValueType, HookContext hookCtx, List<Hook> hooks,
-            Map<String, Object> hints) {
-        executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, hints));
+    public void afterAllHooks(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(FlagValueType flagValueType, HookContext hookCtx, Exception e, List<Hook> hooks,

src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

@@ -218,7 +218,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
             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;

src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java

@@ -36,7 +36,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);
     }
 
@@ -51,8 +51,8 @@ void evalHooks() {
         client.addHooks(clientHook);
         Boolean retval = client.getBooleanValue(flagKey, 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);
     }
 

src/test/java/dev/openfeature/sdk/HookSpecTest.java

@@ -12,8 +12,8 @@
 
 import java.util.*;
 
-import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.fail;
+import static org.assertj.core.api.Assertions.*;
 import static org.junit.jupiter.api.Assertions.*;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.*;
@@ -115,7 +115,7 @@ void nullish_properties_on_hookcontext() {
 
     }
 
-    @Specification(number = "4.1.2", text = "The hook context SHOULD provide: access to the client metadata and the provider metadata fields.")
+    @Specification(number = "4.1.2", text = "The `hook context` SHOULD provide access to the `client metadata` and the `provider metadata` fields.")
     @Test
     void optional_properties() {
         // don't specify
@@ -170,7 +170,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());
     }
@@ -201,7 +201,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();
@@ -241,7 +241,7 @@ 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");
                     }
                 });
@@ -266,7 +266,7 @@ 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");
             }
         });
@@ -290,7 +290,7 @@ 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");
             }
         });
@@ -315,7 +315,7 @@ 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");
                     }
                 })
@@ -344,8 +344,8 @@ void error_stops_before() {
                 .hook(h2)
                 .hook(h)
                 .build());
-            verify(h, times(1)).before(any(), any());
-            verify(h2, times(0)).before(any(), any());
+        verify(h, times(1)).before(any(), any());
+        verify(h2, times(0)).before(any(), any());
     }
 
     @Specification(number = "4.4.6", text = "If an error occurs during the evaluation of before or after hooks, any remaining hooks in the before or after stages MUST NOT be invoked.")
@@ -393,7 +393,7 @@ 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);
             }
         };
@@ -435,7 +435,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(number = "4.4.5", text = "If an error occurs in the before or after hooks, the error hooks MUST be invoked.")
@@ -464,6 +464,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()).isEqualTo(true);
+    }
+
+    @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()).isEqualTo(true);
+    }
+
     @Test
     void multi_hooks_early_out__before() {
         Hook<Boolean> hook = mockBooleanHook();
@@ -556,7 +608,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);
 
@@ -568,8 +620,8 @@ void first_finally_broken() {
                         .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(number = "4.4.4", text = "If an error hook abnormally terminates, evaluation MUST proceed, including the execution of any remaining error hooks.")
@@ -616,8 +668,7 @@ 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();
     }
-
 }

src/test/java/dev/openfeature/sdk/HookSupportTest.java

@@ -51,12 +51,12 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
 
         hookSupport.beforeHooks(flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
         hookSupport.afterHooks(flagValueType, hookContext, FlagEvaluationDetails.builder().build(), Collections.singletonList(genericHook), Collections.emptyMap());
-        hookSupport.afterAllHooks(flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
+        hookSupport.afterAllHooks(flagValueType, hookContext, FlagEvaluationDetails.builder().build(), Collections.singletonList(genericHook), Collections.emptyMap());
         hookSupport.errorHooks(flagValueType, hookContext, expectedException, Collections.singletonList(genericHook), Collections.emptyMap());
 
         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());
     }
 

src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java

@@ -1,15 +1,8 @@
 package dev.openfeature.sdk;
 
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertEquals;
-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 java.util.HashMap;
-import java.util.concurrent.atomic.AtomicBoolean;
-
+import dev.openfeature.sdk.exceptions.FatalError;
+import dev.openfeature.sdk.fixtures.HookFixtures;
+import dev.openfeature.sdk.testutils.TestEventsProvider;
 import org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
@@ -18,9 +11,15 @@
 import org.simplify4u.slf4jmock.LoggerMock;
 import org.slf4j.Logger;
 
-import dev.openfeature.sdk.exceptions.FatalError;
-import dev.openfeature.sdk.fixtures.HookFixtures;
-import dev.openfeature.sdk.testutils.TestEventsProvider;
+import java.util.HashMap;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+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.mock;
+import static org.mockito.Mockito.never;
 
 class OpenFeatureClientTest implements HookFixtures {
 
@@ -102,57 +101,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;
-        }
-    }
 }