PR suggestion: make HookSupportData a real POJO

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

Author: alexandraoberaigner

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

@@ -4,7 +4,6 @@
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.util.Objects;
 import lombok.EqualsAndHashCode;
-import lombok.Generated;
 import lombok.NonNull;
 import lombok.ToString;
 

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

@@ -9,6 +9,42 @@
 @Slf4j
 class HookSupport {
 
+    /**
+     * 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) {
+                hookContextPair.getValue().setCtx(evaluationContext);
+            }
+        }
+    }
+
+    /**
+     * Sets the {@link Hook}-{@link HookContext}-{@link Pair} list in the given data object.
+     *
+     * @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
+     */
+    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));
+            }
+        }
+        hookSupportData.hooks = hookContextPairs;
+    }
+
     public void executeBeforeHooks(HookSupportData data) {
         // These traverse backwards from normal.
         List<Pair<Hook, HookContext>> reversedHooks = new ArrayList<>(data.getHooks());
@@ -23,7 +59,7 @@ public void executeBeforeHooks(HookSupportData data) {
                     .orElse(Optional.empty());
             if (returnedEvalContext.isPresent()) {
                 // update shared evaluation context for all hooks
-                data.setEvaluationContext(data.getEvaluationContext().merge(returnedEvalContext.get()));
+                updateEvaluationContext(data, data.getEvaluationContext().merge(returnedEvalContext.get()));
             }
         }
     }

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

@@ -1,43 +1,19 @@
 package dev.openfeature.sdk;
 
-import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
 import lombok.Getter;
-import lombok.Setter;
 
 /**
  * Encapsulates data for hook execution per flag evaluation.
  */
 @Getter
 class HookSupportData {
 
-    private List<Pair<Hook, HookContext>> hooks;
-    private EvaluationContext evaluationContext;
-
-    @Setter
-    private Map<String, Object> hints;
+    List<Pair<Hook, HookContext>> hooks;
+    EvaluationContext evaluationContext;
+    Map<String, Object> hints;
 
     HookSupportData() {}
 
-    public void setEvaluationContext(EvaluationContext evaluationContext) {
-        this.evaluationContext = evaluationContext;
-        if (hooks != null) {
-            for (Pair<Hook, HookContext> hookContextPair : hooks) {
-                hookContextPair.getValue().setCtx(evaluationContext);
-            }
-        }
-    }
-
-    public void setHooks(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));
-            }
-        }
-        this.hooks = hookContextPairs;
-        this.evaluationContext = evaluationContext;
-    }
 }

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

@@ -165,8 +165,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
 
         var flagOptions = ObjectUtils.defaultIfNull(
                 options, () -> FlagEvaluationOptions.builder().build());
-        var hints = Collections.unmodifiableMap(flagOptions.getHookHints());
-        hookSupportData.setHints(hints);
+        hookSupportData.hints = Collections.unmodifiableMap(flagOptions.getHookHints());
 
         try {
             final var stateManager = openfeatureApi.getFeatureProviderStateManager(this.domain);
@@ -180,10 +179,10 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(
             var sharedHookContext =
                     new SharedHookContext(key, type, this.getMetadata(), provider.getMetadata(), defaultValue);
 
-            hookSupportData.setHooks(mergedHooks, sharedHookContext, ctx);
+            hookSupport.setHookSupportDataHooks(hookSupportData, mergedHooks, sharedHookContext, ctx);
 
             var evalContext = mergeEvaluationContext(ctx);
-            hookSupportData.setEvaluationContext(evalContext);
+            hookSupport.updateEvaluationContext(hookSupportData, evalContext);
 
             hookSupport.executeBeforeHooks(hookSupportData);
 

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

@@ -1,7 +1,5 @@
 package dev.openfeature.sdk;
 
-import dev.openfeature.sdk.internal.ExcludeFromGeneratedCoverageReport;
-import java.util.Objects;
 import lombok.EqualsAndHashCode;
 import lombok.Getter;
 

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

@@ -26,16 +26,18 @@ class HookSupportTest implements HookFixtures {
     void shouldMergeEvaluationContextsOnBeforeHooksCorrectly() {
         Map<String, Value> attributes = new HashMap<>();
         attributes.put("baseKey", new Value("baseValue"));
-        EvaluationContext baseContext = new ImmutableContext(attributes);
+        EvaluationContext baseEvalContext = new ImmutableContext(attributes);
 
         Hook<String> hook1 = mockStringHook();
         Hook<String> 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 sharedContext = getBaseHookContextForType(FlagValueType.STRING);
         var hookSupportData = new HookSupportData();
-        hookSupportData.setHooks(
-                Arrays.asList(hook1, hook2), getBaseHookContextForType(FlagValueType.STRING), baseContext);
+        hookSupport.setHookSupportDataHooks(hookSupportData,
+                Arrays.asList(hook1, hook2), sharedContext, baseEvalContext);
+        hookSupport.updateEvaluationContext(hookSupportData, baseEvalContext);
 
         hookSupport.executeBeforeHooks(hookSupportData);
 
@@ -53,7 +55,7 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
         Hook<?> genericHook = mockGenericHook();
 
         var hookSupportData = new HookSupportData();
-        hookSupportData.setHooks(
+        hookSupport.setHookSupportDataHooks(hookSupportData,
                 List.of(genericHook), getBaseHookContextForType(flagValueType), ImmutableContext.EMPTY);
 
         callAllHooks(hookSupportData);
@@ -70,7 +72,8 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {
     void shouldPassDataAcrossStages(FlagValueType flagValueType) {
         var testHook = new TestHookWithData();
         var hookSupportData = new HookSupportData();
-        hookSupportData.setHooks(List.of(testHook), getBaseHookContextForType(flagValueType), ImmutableContext.EMPTY);
+        hookSupport.setHookSupportDataHooks(hookSupportData,
+                List.of(testHook), getBaseHookContextForType(flagValueType), ImmutableContext.EMPTY);
 
         hookSupport.executeBeforeHooks(hookSupportData);
         assertHookData(testHook, "before");
@@ -95,7 +98,7 @@ void shouldIsolateDataBetweenHooks(FlagValueType flagValueType) {
         var testHook2 = new TestHookWithData(2);
 
         var hookSupportData = new HookSupportData();
-        hookSupportData.setHooks(
+        hookSupport.setHookSupportDataHooks(hookSupportData,
                 List.of(testHook1, testHook2), getBaseHookContextForType(flagValueType), ImmutableContext.EMPTY);
 
         callAllHooks(hookSupportData);