-addresses review comments

-executes generic hooks for all flag evaluations, regardless of the type

Author: lopitz

lib/src/main/java/dev/openfeature/javasdk/Hook.java

@@ -49,5 +49,7 @@ default void error(HookContext<T> ctx, Exception error, Map<String, Object> hint
     default void finallyAfter(HookContext<T> ctx, Map<String, Object> hints) {
     }
 
-    FlagValueType supportsFlagValueType();
+    default FlagValueType supportsFlagValueType() {
+        return FlagValueType.OBJECT;
+    }
 }

lib/src/main/java/dev/openfeature/javasdk/HookSupport.java

@@ -6,14 +6,13 @@
 
 import com.google.common.collect.Lists;
 import lombok.RequiredArgsConstructor;
-import org.slf4j.Logger;
+import lombok.extern.slf4j.Slf4j;
 
-@SuppressWarnings({"unchecked", "rawtypes", "PMD.LoggerIsNotStaticFinal"})
+@Slf4j
 @RequiredArgsConstructor
+@SuppressWarnings({"unchecked", "rawtypes"})
 class HookSupport {
 
-    private final Logger log;
-
     public void errorHooks(FlagValueType flagValueType, HookContext hookCtx, Exception e, List<Hook> hooks, Map<String, Object> hints) {
         executeHooks(flagValueType, hooks, "error", hook -> hook.error(hookCtx, e, hints));
     }
@@ -33,17 +32,21 @@ private <T> void executeHooks(
     ) {
         hooks
             .stream()
-            .filter(hook -> hook.supportsFlagValueType() == flagValueType)
+            .filter(hook -> isHookCompatible(flagValueType, hook))
             .forEach(hook -> executeChecked(hook, hookCode, hookMethod));
     }
 
+    private boolean isHookCompatible(FlagValueType flagValueType, Hook hook) {
+        return hook.supportsFlagValueType() == flagValueType || hook.supportsFlagValueType() == FlagValueType.OBJECT;
+    }
+
     private <T> void executeHooksUnchecked(
         FlagValueType flagValueType, List<Hook> hooks,
         Consumer<Hook<T>> hookCode
     ) {
         hooks
             .stream()
-            .filter(hook -> hook.supportsFlagValueType() == flagValueType)
+            .filter(hook -> isHookCompatible(flagValueType, hook))
             .forEach(hookCode::accept);
     }
 
@@ -65,7 +68,7 @@ private Stream<EvaluationContext> callBeforeHooks(FlagValueType flagValueType, H
         return Lists
             .reverse(hooks)
             .stream()
-            .filter(hook -> hook.supportsFlagValueType() == flagValueType)
+            .filter(hook -> isHookCompatible(flagValueType, hook))
             .map(hook -> hook.before(hookCtx, hints))
             .filter(Objects::nonNull)
             .flatMap(Optional::stream);

lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java

@@ -5,27 +5,24 @@
 import dev.openfeature.javasdk.exceptions.GeneralError;
 import dev.openfeature.javasdk.internal.ObjectUtils;
 import lombok.Getter;
-import org.slf4j.*;
+import lombok.extern.slf4j.Slf4j;
 
+@Slf4j
 @SuppressWarnings({"PMD.DataflowAnomalyAnalysis", "PMD.BeanMembersShouldSerialize", "unchecked", "rawtypes"})
 public class OpenFeatureClient implements Client {
-    private static final Logger log = LoggerFactory.getLogger(OpenFeatureClient.class);
 
     private final OpenFeatureAPI openfeatureApi;
-    @Getter
-    private final String name;
-    @Getter
-    private final String version;
-    @Getter
-    private final List<Hook> clientHooks;
+    @Getter private final String name;
+    @Getter private final String version;
+    @Getter private final List<Hook> clientHooks;
     private final HookSupport hookSupport;
 
     public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String name, String version) {
         this.openfeatureApi = openFeatureAPI;
         this.name = name;
         this.version = version;
         this.clientHooks = new ArrayList<>();
-        this.hookSupport = new HookSupport(log);
+        this.hookSupport = new HookSupport();
     }
 
     @Override

lib/src/test/java/dev/openfeature/javasdk/HookSupportTest.java

@@ -2,26 +2,27 @@
 
 import java.util.*;
 
+import dev.openfeature.javasdk.fixtures.HookFixtures;
 import org.junit.jupiter.api.*;
-import org.slf4j.LoggerFactory;
+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.Mockito.*;
 
-class HookSupportTest {
+class HookSupportTest implements HookFixtures {
 
     @Test
     @DisplayName("should merge EvaluationContexts on before hooks correctly")
     void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
         var baseContext = new EvaluationContext();
         baseContext.addStringAttribute("baseKey", "baseValue");
         var hookContext = new HookContext<>("flagKey", FlagValueType.STRING, "defaultValue", baseContext, () -> "client", () -> "provider");
-        var hook1 = stringHookMock();
-        var hook2 = stringHookMock();
+        var hook1 = mockStringHook();
+        var hook2 = mockStringHook();
         when(hook1.before(any(), any())).thenReturn(Optional.of(evaluationContextWithValue("bla", "blubber")));
         when(hook2.before(any(), any())).thenReturn(Optional.of(evaluationContextWithValue("foo", "bar")));
-
-        var hookSupport = new HookSupport(LoggerFactory.getLogger("test"));
+        var hookSupport = new HookSupport();
 
         var result = hookSupport.beforeHooks(FlagValueType.STRING, hookContext, List.of(hook1, hook2), Map.of());
 
@@ -30,14 +31,45 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
         assertThat(result.getStringAttribute("baseKey")).isEqualTo("baseValue");
     }
 
+    @ParameterizedTest
+    @EnumSource(value = FlagValueType.class)
+    @DisplayName("should always call generic hook")
+    void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
+        var genericHook = mockGenericHook();
+        var hookSupport = new HookSupport();
+        var baseContext = new EvaluationContext();
+        var hookContext = new HookContext<>("flagKey", flagValueType, createDefaultValue(flagValueType), baseContext, () -> "client", () -> "provider");
+
+        hookSupport.beforeHooks(flagValueType, hookContext, List.of(genericHook), Map.of());
+        hookSupport.afterHooks(flagValueType, hookContext, FlagEvaluationDetails.builder().build(), List.of(genericHook), Map.of());
+        hookSupport.afterAllHooks(flagValueType, hookContext, List.of(genericHook), Map.of());
+        hookSupport.errorHooks(flagValueType, hookContext, new IllegalStateException("All fine, just a test"), List.of(genericHook), Map.of());
+
+        verify(genericHook).before(any(), any());
+        verify(genericHook).after(any(), any(), any());
+        verify(genericHook).finallyAfter(any(), any());
+        verify(genericHook).error(any(), any(), any());
+    }
+
+    private Object createDefaultValue(FlagValueType flagValueType) {
+        switch (flagValueType) {
+            case INTEGER:
+                return 1;
+            case BOOLEAN:
+                return true;
+            case STRING:
+                return "defaultValue";
+            case OBJECT:
+                return "object";
+            default:
+                throw new IllegalArgumentException();
+        }
+    }
+
     private EvaluationContext evaluationContextWithValue(String key, String value) {
         var result = new EvaluationContext();
         result.addStringAttribute(key, value);
         return result;
     }
 
-    private StringHook stringHookMock() {
-        return spy(StringHook.class);
-    }
-
 }