diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java index 0d02ba31a..570099871 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java +++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java @@ -73,7 +73,7 @@ public String getTargetingKey() { * Merges this EvaluationContext object with the passed EvaluationContext, overriding in case of conflict. * * @param overridingContext overriding context - * @return resulting merged context + * @return new, resulting merged context */ @Override public EvaluationContext merge(EvaluationContext overridingContext) { diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index 4a1c1179f..962f7d954 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -105,7 +105,7 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key FlagEvaluationDetails details = null; List mergedHooks = null; - HookContext hookCtx = null; + HookContext afterHookContext = null; FeatureProvider provider; try { @@ -115,12 +115,11 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key mergedHooks = ObjectUtils.merge(provider.getProviderHooks(), flagOptions.getHooks(), clientHooks, openfeatureApi.getHooks()); - hookCtx = HookContext.from(key, type, this.getMetadata(), - provider.getMetadata(), ctx, defaultValue); + EvaluationContext mergedCtx = hookSupport.beforeHooks(type, HookContext.from(key, type, this.getMetadata(), + provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue), mergedHooks, hints); - EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints); - - EvaluationContext mergedCtx = mergeEvaluationContext(ctxFromHook, ctx); + afterHookContext = HookContext.from(key, type, this.getMetadata(), + provider.getMetadata(), mergedCtx, defaultValue); ProviderEvaluation providerEval = (ProviderEvaluation) createProviderEvaluation(type, key, defaultValue, provider, mergedCtx); @@ -129,7 +128,7 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key if (details.getErrorCode() != null) { throw ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage()); } else { - hookSupport.afterHooks(type, hookCtx, details, mergedHooks, hints); + hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints); } } catch (Exception e) { log.error("Unable to correctly evaluate flag with key '{}'", key, e); @@ -144,24 +143,22 @@ private FlagEvaluationDetails evaluateFlag(FlagValueType type, String key details.setErrorMessage(e.getMessage()); details.setValue(defaultValue); details.setReason(Reason.ERROR.toString()); - hookSupport.errorHooks(type, hookCtx, e, mergedHooks, hints); + hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints); } finally { - hookSupport.afterAllHooks(type, hookCtx, mergedHooks, hints); + hookSupport.afterAllHooks(type, afterHookContext, mergedHooks, hints); } return details; } /** - * Merge hook and invocation contexts with API, transaction and client contexts. + * Merge invocation contexts with API, transaction and client contexts. + * Does not merge before context. * - * @param hookContext hook context * @param invocationContext invocation context * @return merged evaluation context */ - private EvaluationContext mergeEvaluationContext( - EvaluationContext hookContext, - EvaluationContext invocationContext) { + private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) { final EvaluationContext apiContext = openfeatureApi.getEvaluationContext() != null ? openfeatureApi.getEvaluationContext() : new ImmutableContext(); @@ -172,7 +169,7 @@ private EvaluationContext mergeEvaluationContext( ? openfeatureApi.getTransactionContext() : new ImmutableContext(); - return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext.merge(hookContext)))); + return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext))); } private ProviderEvaluation createProviderEvaluation( diff --git a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java index b2a7510e6..6d2bca3a5 100644 --- a/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java @@ -2,7 +2,6 @@ import static dev.openfeature.sdk.DoSomethingProvider.DEFAULT_METADATA; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatCode; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -20,6 +19,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import org.awaitility.Awaitility; import org.junit.jupiter.api.AfterEach; @@ -336,11 +336,25 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { FeatureProviderTestUtils.setFeatureProvider(provider); TransactionContextPropagator transactionContextPropagator = new ThreadLocalTransactionContextPropagator(); api.setTransactionContextPropagator(transactionContextPropagator); + Hook hook = spy(new Hook() { + @Override + public Optional before(HookContext ctx, Map hints) { + Map attrs = ctx.getCtx().asMap(); + attrs.put("before", new Value("5")); + attrs.put("common7", new Value("5")); + return Optional.ofNullable(new ImmutableContext(attrs)); + } + @Override + public void after(HookContext ctx, FlagEvaluationDetails details, Map hints) { + Hook.super.after(ctx, details, hints); + } + }); Map apiAttributes = new HashMap<>(); apiAttributes.put("common1", new Value("1")); apiAttributes.put("common2", new Value("1")); apiAttributes.put("common3", new Value("1")); + apiAttributes.put("common7", new Value("1")); apiAttributes.put("api", new Value("1")); EvaluationContext apiCtx = new ImmutableContext(apiAttributes); @@ -377,21 +391,55 @@ public void initialize(EvaluationContext evaluationContext) throws Exception { invocationAttributes.put("invocation", new Value("4")); EvaluationContext invocationCtx = new ImmutableContext(invocationAttributes); - c.getBooleanValue("key", false, invocationCtx); - - // assert the connect overrides + c.getBooleanValue("key", false, invocationCtx, FlagEvaluationOptions.builder().hook(hook).build()); + + // assert the correct overrides in before hook + verify(hook).before(argThat((arg) -> { + EvaluationContext evaluationContext = arg.getCtx(); + return evaluationContext.getValue("api").asString().equals("1") && + evaluationContext.getValue("transaction").asString().equals("2") && + evaluationContext.getValue("client").asString().equals("3") && + evaluationContext.getValue("invocation").asString().equals("4") && + evaluationContext.getValue("common1").asString().equals("2") && + evaluationContext.getValue("common2").asString().equals("3") && + evaluationContext.getValue("common3").asString().equals("4") && + evaluationContext.getValue("common4").asString().equals("3") && + evaluationContext.getValue("common5").asString().equals("4") && + evaluationContext.getValue("common6").asString().equals("4"); + }), any()); + + // assert the correct overrides in evaluation verify(provider).getBooleanEvaluation(any(), any(), argThat((arg) -> { return arg.getValue("api").asString().equals("1") && arg.getValue("transaction").asString().equals("2") && arg.getValue("client").asString().equals("3") && arg.getValue("invocation").asString().equals("4") && + arg.getValue("before").asString().equals("5") && arg.getValue("common1").asString().equals("2") && arg.getValue("common2").asString().equals("3") && arg.getValue("common3").asString().equals("4") && arg.getValue("common4").asString().equals("3") && arg.getValue("common5").asString().equals("4") && - arg.getValue("common6").asString().equals("4"); + arg.getValue("common6").asString().equals("4") && + arg.getValue("common7").asString().equals("5"); })); + + // assert the correct overrides in after hook + verify(hook).after(argThat((arg) -> { + EvaluationContext evaluationContext = arg.getCtx(); + return evaluationContext.getValue("api").asString().equals("1") && + evaluationContext.getValue("transaction").asString().equals("2") && + evaluationContext.getValue("client").asString().equals("3") && + evaluationContext.getValue("invocation").asString().equals("4") && + evaluationContext.getValue("before").asString().equals("5") && + evaluationContext.getValue("common1").asString().equals("2") && + evaluationContext.getValue("common2").asString().equals("3") && + evaluationContext.getValue("common3").asString().equals("4") && + evaluationContext.getValue("common4").asString().equals("3") && + evaluationContext.getValue("common5").asString().equals("4") && + evaluationContext.getValue("common6").asString().equals("4") && + evaluationContext.getValue("common7").asString().equals("5"); + }), any(), any()); } @Specification(number="3.3.1.1", text="The API SHOULD have a method for setting a transaction context propagator.") diff --git a/src/test/java/dev/openfeature/sdk/HookSpecTest.java b/src/test/java/dev/openfeature/sdk/HookSpecTest.java index 098d80d7f..2554457ab 100644 --- a/src/test/java/dev/openfeature/sdk/HookSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSpecTest.java @@ -484,10 +484,11 @@ public void finallyAfter(HookContext ctx, Map hints) { @Specification(number = "4.1.4", text = "The evaluation context MUST be mutable only within the before hook.") @Specification(number = "4.3.4", text = "Any `evaluation context` returned from a `before` hook MUST be passed to subsequent `before` hooks (via `HookContext`).") @Test void beforeContextUpdated() { - EvaluationContext ctx = new ImmutableContext(); - Hook hook = mockBooleanHook(); + String targetingKey = "test-key"; + EvaluationContext ctx = new ImmutableContext(targetingKey); + Hook hook = mockBooleanHook(); when(hook.before(any(), any())).thenReturn(Optional.of(ctx)); - Hook hook2 = mockBooleanHook(); + Hook hook2 = mockBooleanHook(); when(hook.before(any(), any())).thenReturn(Optional.empty()); InOrder order = inOrder(hook, hook2); @@ -499,11 +500,11 @@ public void finallyAfter(HookContext ctx, Map hints) { .build()); order.verify(hook).before(any(), any()); - ArgumentCaptor captor = ArgumentCaptor.forClass(HookContext.class); + ArgumentCaptor> captor = ArgumentCaptor.forClass(HookContext.class); order.verify(hook2).before(captor.capture(), any()); - HookContext hc = captor.getValue(); - assertEquals(hc.getCtx(), ctx); + HookContext hc = captor.getValue(); + assertEquals(hc.getCtx().getTargetingKey(), targetingKey); }