fixup! feat: Add evaluation details to finally hook stage #1246

Signed-off-by: christian.lutnik <[email protected]>
open-feature · Jan 8, 2025 · 56d9fd2 · 56d9fd2
1 parent 49d7b95
commit 56d9fd2
Show file tree

Hide file tree

Showing 5 changed files with 96 additions and 68 deletions.
diff --git a/src/main/java/dev/openfeature/sdk/Hook.java b/src/main/java/dev/openfeature/sdk/Hook.java
@@ -46,8 +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, FlagEvaluationDetails<T> details, 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
@@ -28,8 +28,12 @@ public void afterHooks(
         executeHooksUnchecked(flagValueType, hooks, hook -> hook.after(hookContext, details, hints));
     }
 
-    public void afterAllHooks(FlagValueType flagValueType, HookContext hookCtx, FlagEvaluationDetails details,
-                              List<Hook> hooks, Map<String, Object> 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));
     }
 

diff --git a/src/test/java/dev/openfeature/sdk/HookSpecTest.java b/src/test/java/dev/openfeature/sdk/HookSpecTest.java
@@ -1,23 +1,5 @@
 package dev.openfeature.sdk;
 
-import dev.openfeature.sdk.exceptions.FlagNotFoundError;
-import dev.openfeature.sdk.fixtures.HookFixtures;
-import dev.openfeature.sdk.testutils.FeatureProviderTestUtils;
-import dev.openfeature.sdk.testutils.TestEventsProvider;
-import lombok.SneakyThrows;
-import org.junit.jupiter.api.AfterEach;
-import org.junit.jupiter.api.Test;
-import org.mockito.ArgumentCaptor;
-import org.mockito.InOrder;
-
-import java.util.ArrayList;
-import java.util.Arrays;
-import java.util.Collections;
-import java.util.HashMap;
-import java.util.List;
-import java.util.Map;
-import java.util.Optional;
-
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.fail;
@@ -34,6 +16,23 @@
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
+import dev.openfeature.sdk.exceptions.FlagNotFoundError;
+import dev.openfeature.sdk.fixtures.HookFixtures;
+import dev.openfeature.sdk.testutils.FeatureProviderTestUtils;
+import dev.openfeature.sdk.testutils.TestEventsProvider;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import lombok.SneakyThrows;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.Test;
+import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
+
 class HookSpecTest implements HookFixtures {
     @AfterEach
     void emptyApiHooks() {
@@ -285,7 +284,10 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
                     }
 
                     @Override
-                    public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
+                    public void finallyAfter(
+                            HookContext<Boolean> ctx,
+                            FlagEvaluationDetails<Boolean> details,
+                            Map<String, Object> hints) {
                         evalOrder.add("provider finally");
                     }
                 });
@@ -311,7 +313,8 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
                 evalOrder.add("api finally");
             }
         });
@@ -336,7 +339,8 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
                 evalOrder.add("client finally");
             }
         });
@@ -367,12 +371,15 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
                                 evalOrder.add("invocation error");
                             }
 
-                    @Override
-                    public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
-                        evalOrder.add("invocation finally");
-                    }
-                })
-                .build());
+                            @Override
+                            public void finallyAfter(
+                                    HookContext<Boolean> ctx,
+                                    FlagEvaluationDetails<Boolean> details,
+                                    Map<String, Object> hints) {
+                                evalOrder.add("invocation finally");
+                            }
+                        })
+                        .build());
 
         List<String> expectedOrder = Arrays.asList(
                 "api before",
@@ -408,10 +415,11 @@ void error_stops_before() {
         api.setProviderAndWait(new AlwaysBrokenProvider());
         Client c = api.getClient();
 
-        c.getBooleanDetails("key", false, null, FlagEvaluationOptions.builder()
-                .hook(h2)
-                .hook(h)
-                .build());
+        c.getBooleanDetails(
+                "key",
+                false,
+                null,
+                FlagEvaluationOptions.builder().hook(h2).hook(h).build());
         verify(h, times(1)).before(any(), any());
         verify(h2, times(0)).before(any(), any());
     }
@@ -472,8 +480,10 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
             }
 
             @Override
-            public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
-                assertThatCode(() -> hints.put(hintKey, "changed value")).isInstanceOf(UnsupportedOperationException.class);
+            public void finallyAfter(
+                    HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
+                assertThatCode(() -> hints.put(hintKey, "changed value"))
+                        .isInstanceOf(UnsupportedOperationException.class);
             }
         };
 
@@ -569,8 +579,7 @@ void erroneous_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
                 flagKey,
                 true,
                 new ImmutableContext(),
-                FlagEvaluationOptions.builder().hook(hook).build()
-        );
+                FlagEvaluationOptions.builder().hook(hook).build());
 
         ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
         verify(hook).finallyAfter(any(), captor.capture(), any());
@@ -582,7 +591,8 @@ void erroneous_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
         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.getFlagMetadata())
+                .isEqualTo(ImmutableMetadata.builder().build());
         assertThat(evaluationDetails.getValue()).isTrue();
     }
 
@@ -595,8 +605,7 @@ void successful_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
                 flagKey,
                 true,
                 new ImmutableContext(),
-                FlagEvaluationOptions.builder().hook(hook).build()
-        );
+                FlagEvaluationOptions.builder().hook(hook).build());
 
         ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
         verify(hook).finallyAfter(any(), captor.capture(), any());
@@ -607,7 +616,8 @@ void successful_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
         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.getFlagMetadata())
+                .isEqualTo(ImmutableMetadata.builder().build());
         assertThat(evaluationDetails.getValue()).isTrue();
     }
 
@@ -772,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, FlagEvaluationDetails.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
@@ -1,21 +1,20 @@
 package dev.openfeature.sdk;
 
-import dev.openfeature.sdk.fixtures.HookFixtures;
-import org.junit.jupiter.api.DisplayName;
-import org.junit.jupiter.api.Test;
-import org.junit.jupiter.params.ParameterizedTest;
-import org.junit.jupiter.params.provider.EnumSource;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
 
+import dev.openfeature.sdk.fixtures.HookFixtures;
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
-
-import static org.assertj.core.api.Assertions.assertThat;
-import static org.mockito.ArgumentMatchers.any;
-import static org.mockito.Mockito.verify;
-import static org.mockito.Mockito.when;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
 
 class HookSupportTest implements HookFixtures {
     @Test
@@ -56,10 +55,26 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
                 () -> "client",
                 () -> "provider");
 
-        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, FlagEvaluationDetails.builder().build(), Collections.singletonList(genericHook), Collections.emptyMap());
-        hookSupport.errorHooks(flagValueType, hookContext, expectedException, Collections.singletonList(genericHook), Collections.emptyMap());
+        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,
+                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());

diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java
@@ -1,8 +1,17 @@
 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.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 org.junit.jupiter.api.AfterEach;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.DisplayName;
@@ -11,16 +20,6 @@
 import org.simplify4u.slf4jmock.LoggerMock;
 import org.slf4j.Logger;
 
-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 {
 
     private Logger logger;