PR suggestion: call hooks as early as possible

Signed-off-by: Alexandra Oberaigner <alexandra.oberaigner@dynatrace.com>

Author: alexandraoberaigner

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

@@ -6,46 +6,63 @@
 import java.util.Optional;
 import lombok.extern.slf4j.Slf4j;
 
+/**
+ * Helper class to run hooks. Initialize {@link HookSupportData} by calling setHooks, setHookContexts
+ * & updateEvaluationContext in this exact order.
+ */
 @Slf4j
 class HookSupport {
 
     /**
-     * Updates the evaluation context in the given data object's eval context and each hooks eval context.
+     * Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object with {@link HookContext}
+     * set to null. Filters hooks by supported {@link FlagValueType}.
      *
      * @param hookSupportData   the data object to modify
-     * @param evaluationContext the new context to set
+     * @param hooks             the hooks to set
+     * @param type              the flag value type to filter unsupported hooks
      */
-    public void updateEvaluationContext(HookSupportData hookSupportData, EvaluationContext evaluationContext) {
-        hookSupportData.evaluationContext = evaluationContext;
-        if (hookSupportData.hooks != null) {
-            for (Pair<Hook, HookContext> hookContextPair : hookSupportData.hooks) {
-                hookContextPair.getValue().setCtx(evaluationContext);
+    public void setHooks(HookSupportData hookSupportData, List<Hook> hooks, FlagValueType type) {
+        List<Pair<Hook, HookContext>> hookContextPairs = new ArrayList<>();
+        for (Hook hook : hooks) {
+            if (hook.supportsFlagValueType(type)) {
+                hookContextPairs.add(Pair.of(hook, null));
             }
         }
+        hookSupportData.hooks = hookContextPairs;
     }
 
     /**
-     * Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object.
+     * Creates & sets a {@link HookContext} for every {@link Hook}-{@link HookContext}-{@link Pair}
+     * in the given data object with a new {@link HookData} instance.
      *
      * @param hookSupportData   the data object to modify
-     * @param hooks             the new hooks to set
-     * @param sharedContext     the shared context across all hooks from which each hook's {@link HookContext} is
-     *                          created
-     * @param evaluationContext the evaluation context which is
+     * @param sharedContext     the shared context from which the new {@link HookContext} is created
      */
-    public void setHookSupportDataHooks(
-            HookSupportData hookSupportData,
-            List<Hook> hooks,
-            SharedHookContext sharedContext,
-            EvaluationContext evaluationContext) {
-        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));
+    public void setHookContexts(HookSupportData hookSupportData, SharedHookContext sharedContext) {
+        for (int i = 0; i < hookSupportData.hooks.size(); i++) {
+            Pair<Hook, HookContext> hookContextPair = hookSupportData.hooks.get(i);
+            HookContext curHookContext = sharedContext.hookContextFor(null, new DefaultHookData());
+            Pair<Hook, HookContext> updatedPair = Pair.of(hookContextPair.getKey(), curHookContext);
+            hookSupportData.hooks.set(i, updatedPair);
+        }
+    }
+
+    /**
+     * Updates the evaluation context in the given data object's eval context and each hooks eval context.
+     *
+     * @param hookSupportData   the data object to modify
+     * @param evaluationContext the new context to set
+     */
+    public void updateEvaluationContext(HookSupportData hookSupportData, EvaluationContext evaluationContext) {
+        hookSupportData.evaluationContext = evaluationContext;
+        if (hookSupportData.hooks != null) {
+            for (Pair<Hook, HookContext> hookContextPair : hookSupportData.hooks) {
+                var curHookContext = hookContextPair.getValue();
+                if (curHookContext != null) {
+                    curHookContext.setCtx(evaluationContext);
+                }
             }
         }
-        hookSupportData.hooks = hookContextPairs;
     }
 
     public void executeBeforeHooks(HookSupportData data) {
@@ -57,9 +74,10 @@ public void executeBeforeHooks(HookSupportData data) {
             var hook = hookContextPair.getKey();
             var hookContext = hookContextPair.getValue();
 
-            Optional<EvaluationContext> returnedEvalContext = Optional.ofNullable(
-                            hook.before(hookContext, data.getHints()))
-                    .orElse(Optional.empty());
+            Optional<EvaluationContext> returnedEvalContext = hook.before(hookContext, data.getHints());
+            if (returnedEvalContext == null) {
+                returnedEvalContext = Optional.empty();
+            }
             if (returnedEvalContext.isPresent()) {
                 // update shared evaluation context for all hooks
                 updateEvaluationContext(data, data.getEvaluationContext().merge(returnedEvalContext.get()));

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

@@ -173,13 +173,14 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             final var provider = stateManager.getProvider();
             final var state = stateManager.getState();
 
+            // Hooks are initialized as early as possible to enable the execution of error stages
             var mergedHooks = ObjectUtils.merge(
                     provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getMutableHooks());
+            hookSupport.setHooks(hookSupportData, mergedHooks, type);
 
             var sharedHookContext =
                     new SharedHookContext(key, type, this.getMetadata(), provider.getMetadata(), defaultValue);
-
-            hookSupport.setHookSupportDataHooks(hookSupportData, mergedHooks, sharedHookContext, ctx);
+            hookSupport.setHookContexts(hookSupportData, sharedHookContext);
 
             var evalContext = mergeEvaluationContext(ctx);
             hookSupport.updateEvaluationContext(hookSupportData, evalContext);

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

@@ -35,8 +35,8 @@ void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
 
         var sharedContext = getBaseHookContextForType(FlagValueType.STRING);
         var hookSupportData = new HookSupportData();
-        hookSupport.setHookSupportDataHooks(
-                hookSupportData, Arrays.asList(hook1, hook2), sharedContext, baseEvalContext);
+        hookSupport.setHooks(hookSupportData, Arrays.asList(hook1, hook2), FlagValueType.STRING);
+        hookSupport.setHookContexts(hookSupportData, sharedContext);
         hookSupport.updateEvaluationContext(hookSupportData, baseEvalContext);
 
         hookSupport.executeBeforeHooks(hookSupportData);
@@ -55,11 +55,7 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
         Hook<?> genericHook = mockGenericHook();
 
         var hookSupportData = new HookSupportData();
-        hookSupport.setHookSupportDataHooks(
-                hookSupportData,
-                List.of(genericHook),
-                getBaseHookContextForType(flagValueType),
-                ImmutableContext.EMPTY);
+        hookSupport.setHooks(hookSupportData, List.of(genericHook), flagValueType);
 
         callAllHooks(hookSupportData);
 
@@ -75,8 +71,8 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
     void shouldPassDataAcrossStages(FlagValueType flagValueType) {
         var testHook = new TestHookWithData();
         var hookSupportData = new HookSupportData();
-        hookSupport.setHookSupportDataHooks(
-                hookSupportData, List.of(testHook), getBaseHookContextForType(flagValueType), ImmutableContext.EMPTY);
+        hookSupport.setHooks(hookSupportData, List.of(testHook), flagValueType);
+        hookSupport.setHookContexts(hookSupportData, getBaseHookContextForType(flagValueType));
 
         hookSupport.executeBeforeHooks(hookSupportData);
         assertHookData(testHook, "before");
@@ -101,11 +97,8 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
         var testHook2 = new TestHookWithData(2);
 
         var hookSupportData = new HookSupportData();
-        hookSupport.setHookSupportDataHooks(
-                hookSupportData,
-                List.of(testHook1, testHook2),
-                getBaseHookContextForType(flagValueType),
-                ImmutableContext.EMPTY);
+        hookSupport.setHooks(hookSupportData, List.of(testHook1, testHook2), flagValueType);
+        hookSupport.setHookContexts(hookSupportData, getBaseHookContextForType(flagValueType));
 
         callAllHooks(hookSupportData);