feat: separate hook support data from logic, PR suggestions

Signed-off-by: Alexandra Oberaigner <[email protected]>

Author: alexandraoberaigner

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

@@ -1,5 +1,6 @@
 package dev.openfeature.sdk;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.util.Objects;
 import lombok.NonNull;
 
@@ -114,6 +115,7 @@ public Metadata getProviderMetadata() {
         return sharedContext.getProviderMetadata();
     }
 
+    @SuppressFBWarnings(value = "EI_EXPOSE_REP", justification = "Intentional exposure of hookData")
     public HookData getHookData() {
         return this.hookData;
     }
@@ -300,10 +302,10 @@ public HookContext<T> withHookData(HookData hookData) {
      */
     @Deprecated
     public static class HookContextBuilder<T> {
-        private @NonNull String flagKey;
-        private @NonNull FlagValueType type;
-        private @NonNull T defaultValue;
-        private @NonNull EvaluationContext ctx;
+        private String flagKey;
+        private FlagValueType type;
+        private T defaultValue;
+        private EvaluationContext ctx;
         private ClientMetadata clientMetadata;
         private Metadata providerMetadata;
         private HookData hookData;
@@ -340,6 +342,7 @@ public HookContextBuilder<T> providerMetadata(Metadata providerMetadata) {
             return this;
         }
 
+        @SuppressFBWarnings(value = "EI_EXPOSE_REP2", justification = "Intentional exposure of hookData")
         public HookContextBuilder<T> hookData(HookData hookData) {
             this.hookData = hookData;
             return this;

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

@@ -3,68 +3,37 @@
 import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
-import java.util.Map;
 import java.util.Optional;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
 class HookSupport {
-    private List<Pair<Hook, HookContext>> hooks;
-    private EvaluationContext evaluationContext;
-    private final Map<String, Object> hints;
 
-    HookSupport(
-            List<Hook> hooks,
-            SharedHookContext sharedContext,
-            EvaluationContext evaluationContext,
-            Map<String, Object> hints) {
-        List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>();
-        for (Hook hook : hooks) {
-            if (hook.supportsFlagValueType(sharedContext.getType())) {
-                HookContext curContext = sharedContext.hookContextFor(evaluationContext, new DefaultHookData());
-                hookContextPairs.add(Pair.of(hook, curContext));
-            }
-        }
-        this.hooks = hookContextPairs;
-        this.evaluationContext = evaluationContext;
-        this.hints = hints;
-    }
-
-    public EvaluationContext getEvaluationContext() {
-        return evaluationContext;
-    }
-
-    private void setEvaluationContext(EvaluationContext evaluationContext) {
-        this.evaluationContext = evaluationContext;
-        for (Pair<Hook, HookContext> hookContextPair : hooks) {
-            hookContextPair.getValue().setCtx(evaluationContext);
-        }
-    }
-
-    public void executeBeforeHooks() {
+    public void executeBeforeHooks(HookSupportData data) {
         // These traverse backwards from normal.
-        List<Pair<Hook, HookContext>> reversedHooks = new ArrayList<>(hooks);
+        List<Pair<Hook, HookContext>> reversedHooks = new ArrayList<>(data.getHooks());
         Collections.reverse(reversedHooks);
 
         for (Pair<Hook, HookContext> hookContextPair : reversedHooks) {
             var hook = hookContextPair.getKey();
             var hookContext = hookContextPair.getValue();
 
-            Optional<EvaluationContext> returnedEvalContext =
-                    Optional.ofNullable(hook.before(hookContext, hints)).orElse(Optional.empty());
+            Optional<EvaluationContext> returnedEvalContext = Optional.ofNullable(
+                            hook.before(hookContext, data.getHints()))
+                    .orElse(Optional.empty());
             if (returnedEvalContext.isPresent()) {
                 // update shared evaluation context for all hooks
-                setEvaluationContext(evaluationContext.merge(returnedEvalContext.get()));
+                data.setEvaluationContext(data.getEvaluationContext().merge(returnedEvalContext.get()));
             }
         }
     }
 
-    public void executeErrorHooks(Exception error) {
-        for (Pair<Hook, HookContext> hookContextPair : hooks) {
+    public void executeErrorHooks(HookSupportData data, Exception error) {
+        for (Pair<Hook, HookContext> hookContextPair : data.getHooks()) {
             var hook = hookContextPair.getKey();
             var hookContext = hookContextPair.getValue();
             try {
-                hook.error(hookContext, error, hints);
+                hook.error(hookContext, error, data.getHints());
             } catch (Exception e) {
                 log.error(
                         "Unhandled exception when running {} hook {} (only 'after' hooks should throw)",
@@ -76,20 +45,20 @@ public void executeErrorHooks(Exception error) {
     }
 
     // after hooks can throw in order to do validation
-    public <T> void executeAfterHooks(FlagEvaluationDetails<T> details) {
-        for (Pair<Hook, HookContext> hookContextPair : hooks) {
+    public <T> void executeAfterHooks(HookSupportData data, FlagEvaluationDetails<T> details) {
+        for (Pair<Hook, HookContext> hookContextPair : data.getHooks()) {
             var hook = hookContextPair.getKey();
             var hookContext = hookContextPair.getValue();
-            hook.after(hookContext, details, hints);
+            hook.after(hookContext, details, data.getHints());
         }
     }
 
-    public <T> void executeAfterAllHooks(FlagEvaluationDetails<T> details) {
-        for (Pair<Hook, HookContext> hookContextPair : hooks) {
+    public <T> void executeAfterAllHooks(HookSupportData data, FlagEvaluationDetails<T> details) {
+        for (Pair<Hook, HookContext> hookContextPair : data.getHooks()) {
             var hook = hookContextPair.getKey();
             var hookContext = hookContextPair.getValue();
             try {
-                hook.finallyAfter(hookContext, details, hints);
+                hook.finallyAfter(hookContext, details, data.getHints());
             } catch (Exception e) {
                 log.error(
                         "Unhandled exception when running {} hook {} (only 'after' hooks should throw)",

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

@@ -0,0 +1,45 @@
+package dev.openfeature.sdk;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
+import lombok.Getter;
+
+/**
+ * Encapsulates data for hook execution per flag evaluation.
+ */
+@Getter
+class HookSupportData {
+
+    private List<Pair<Hook, HookContext>> hooks;
+    private EvaluationContext evaluationContext;
+    private Map<String, Object> hints;
+    private boolean isInitialized = false;
+
+    HookSupportData() {}
+
+    void initialize(
+            List<Hook> hooks,
+            SharedHookContext sharedContext,
+            EvaluationContext evaluationContext,
+            Map<String, Object> hints) {
+        List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>();
+        for (Hook hook : hooks) {
+            if (hook.supportsFlagValueType(sharedContext.getType())) {
+                HookContext curContext = sharedContext.hookContextFor(evaluationContext, new DefaultHookData());
+                hookContextPairs.add(Pair.of(hook, curContext));
+            }
+        }
+        this.hooks = hookContextPairs;
+        this.evaluationContext = evaluationContext;
+        this.hints = hints;
+        isInitialized = true;
+    }
+
+    public void setEvaluationContext(EvaluationContext evaluationContext) {
+        this.evaluationContext = evaluationContext;
+        for (Pair<Hook, HookContext> hookContextPair : hooks) {
+            hookContextPair.getValue().setCtx(evaluationContext);
+        }
+    }
+}

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

@@ -50,6 +50,8 @@ public class OpenFeatureClient implements Client {
     private final ConcurrentLinkedQueue<Hook> clientHooks;
     private final AtomicReference<EvaluationContext> evaluationContext = new AtomicReference<>();
 
+    private final HookSupport hookSupport;
+
     /**
      * Deprecated public constructor. Use OpenFeature.API.getClient() instead.
      *
@@ -66,6 +68,7 @@ public OpenFeatureClient(OpenFeatureAPI openFeatureAPI, String domain, String ve
         this.openfeatureApi = openFeatureAPI;
         this.domain = domain;
         this.version = version;
+        this.hookSupport = new HookSupport();
         this.clientHooks = new ConcurrentLinkedQueue<>();
     }
 
@@ -162,7 +165,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
         var hints = Collections.unmodifiableMap(flagOptions.getHookHints());
 
         FlagEvaluationDetails<T> details = null;
-        HookSupport hookSupport = null;
+        HookSupportData hookSupportData = new HookSupportData();
 
         try {
             final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
@@ -176,10 +179,12 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             var sharedHookContext =
                     new SharedHookContext(key, type, this.getMetadata(), provider.getMetadata(), defaultValue);
 
+            hookSupportData.initialize(mergedHooks, sharedHookContext, ctx, hints);
+
             var evalContext = mergeEvaluationContext(ctx);
-            hookSupport = new HookSupport(mergedHooks, sharedHookContext, evalContext, hints);
+            hookSupportData.setEvaluationContext(evalContext);
 
-            hookSupport.executeBeforeHooks();
+            hookSupport.executeBeforeHooks(hookSupportData);
 
             // "short circuit" if the provider is in NOT_READY or FATAL state
             if (ProviderState.NOT_READY.equals(state)) {
@@ -190,16 +195,16 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             }
 
             var providerEval = (ProviderEvaluation<T>)
-                    createProviderEvaluation(type, key, defaultValue, provider, hookSupport.getEvaluationContext());
+                    createProviderEvaluation(type, key, defaultValue, provider, hookSupportData.getEvaluationContext());
 
             details = FlagEvaluationDetails.from(providerEval, key);
             if (details.getErrorCode() != null) {
                 var error =
                         ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage());
                 enrichDetailsWithErrorDefaults(defaultValue, details);
-                hookSupport.executeErrorHooks(error);
+                hookSupport.executeErrorHooks(hookSupportData, error);
             } else {
-                hookSupport.executeAfterHooks(details);
+                hookSupport.executeAfterHooks(hookSupportData, details);
             }
         } catch (Exception e) {
             if (details == null) {
@@ -212,10 +217,12 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             }
             details.setErrorMessage(e.getMessage());
             enrichDetailsWithErrorDefaults(defaultValue, details);
-            hookSupport.executeErrorHooks(e);
+            if (hookSupportData.isInitialized()) {
+                hookSupport.executeErrorHooks(hookSupportData, e);
+            }
         } finally {
-            if (hookSupport != null) {
-                hookSupport.executeAfterAllHooks(details);
+            if (hookSupportData.isInitialized()) {
+                hookSupport.executeAfterAllHooks(hookSupportData, details);
             }
         }
 

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

@@ -19,6 +19,9 @@
 import org.junit.jupiter.params.provider.EnumSource;
 
 class HookSupportTest implements HookFixtures {
+
+    private static final HookSupport hookSupport = new HookSupport();
+
     @Test
     @DisplayName("should merge EvaluationContexts on before hooks correctly")
     void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
@@ -31,15 +34,16 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
         when(hook1.before(any(), any())).thenReturn(Optional.of(evaluationContextWithValue("bla", "blubber")));
         when(hook2.before(any(), any())).thenReturn(Optional.of(evaluationContextWithValue("foo", "bar")));
 
-        HookSupport executor = new HookSupport(
+        var hookSupportData = new HookSupportData();
+        hookSupportData.initialize(
                 Arrays.asList(hook1, hook2),
                 getBaseHookContextForType(FlagValueType.STRING),
                 baseContext,
                 Collections.emptyMap());
 
-        executor.executeBeforeHooks();
+        hookSupport.executeBeforeHooks(hookSupportData);
 
-        EvaluationContext result = executor.getEvaluationContext();
+        EvaluationContext result = hookSupportData.getEvaluationContext();
 
         assertThat(result.getValue("bla").asString()).isEqualTo("blubber");
         assertThat(result.getValue("foo").asString()).isEqualTo("bar");
@@ -52,13 +56,14 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
     void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
         Hook<?> genericHook = mockGenericHook();
 
-        HookSupport hookSupport = new HookSupport(
+        var hookSupportData = new HookSupportData();
+        hookSupportData.initialize(
                 List.of(genericHook),
                 getBaseHookContextForType(flagValueType),
                 ImmutableContext.EMPTY,
                 Collections.emptyMap());
 
-        callAllHooks(hookSupport);
+        callAllHooks(hookSupportData);
 
         verify(genericHook).before(any(), any());
         verify(genericHook).after(any(), any(), any());
@@ -71,22 +76,25 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
     @DisplayName("should allow hooks to store and retrieve data across stages")
     void shouldPassDataAcrossStages(FlagValueType flagValueType) {
         var testHook = new TestHookWithData();
-        HookSupport hookSupport = new HookSupport(
+        var hookSupportData = new HookSupportData();
+        hookSupportData.initialize(
                 List.of(testHook),
                 getBaseHookContextForType(flagValueType),
                 ImmutableContext.EMPTY,
                 Collections.emptyMap());
 
-        hookSupport.executeBeforeHooks();
+        hookSupport.executeBeforeHooks(hookSupportData);
         assertHookData(testHook, "before");
 
-        hookSupport.executeAfterHooks(FlagEvaluationDetails.builder().build());
+        hookSupport.executeAfterHooks(
+                hookSupportData, FlagEvaluationDetails.builder().build());
         assertHookData(testHook, "before", "after");
 
-        hookSupport.executeAfterAllHooks(FlagEvaluationDetails.builder().build());
+        hookSupport.executeAfterAllHooks(
+                hookSupportData, FlagEvaluationDetails.builder().build());
         assertHookData(testHook, "before", "after", "finallyAfter");
 
-        hookSupport.executeErrorHooks(mock(Exception.class));
+        hookSupport.executeErrorHooks(hookSupportData, mock(Exception.class));
         assertHookData(testHook, "before", "after", "finallyAfter", "error");
     }
 
@@ -97,23 +105,26 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
         var testHook1 = new TestHookWithData(1);
         var testHook2 = new TestHookWithData(2);
 
-        HookSupport hookSupport = new HookSupport(
+        var hookSupportData = new HookSupportData();
+        hookSupportData.initialize(
                 List.of(testHook1, testHook2),
                 getBaseHookContextForType(flagValueType),
                 ImmutableContext.EMPTY,
                 Collections.emptyMap());
 
-        callAllHooks(hookSupport);
+        callAllHooks(hookSupportData);
 
         assertHookData(testHook1, 1, "before", "after", "finallyAfter", "error");
         assertHookData(testHook2, 2, "before", "after", "finallyAfter", "error");
     }
 
-    private static void callAllHooks(HookSupport hookSupport) {
-        hookSupport.executeBeforeHooks();
-        hookSupport.executeAfterHooks(FlagEvaluationDetails.builder().build());
-        hookSupport.executeAfterAllHooks(FlagEvaluationDetails.builder().build());
-        hookSupport.executeErrorHooks(mock(Exception.class));
+    private static void callAllHooks(HookSupportData hookSupportData) {
+        hookSupport.executeBeforeHooks(hookSupportData);
+        hookSupport.executeAfterHooks(
+                hookSupportData, FlagEvaluationDetails.builder().build());
+        hookSupport.executeAfterAllHooks(
+                hookSupportData, FlagEvaluationDetails.builder().build());
+        hookSupport.executeErrorHooks(hookSupportData, mock(Exception.class));
     }
 
     private static void assertHookData(TestHookWithData testHook, String... expectedKeys) {