Skip to content

Commit 9dde9bb

Browse files
toddbaerttoddbaert
committed
fix: updated context not passed to all hooks
Signed-off-by: Todd Baert <[email protected]>
1 parent 7cac198 commit 9dde9bb

File tree

4 files changed

+73
-27
lines changed

4 files changed

+73
-27
lines changed

Diff for: ‎src/main/java/dev/openfeature/sdk/ImmutableContext.java

+1-1
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ public String getTargetingKey() {
7373
* Merges this EvaluationContext object with the passed EvaluationContext, overriding in case of conflict.
7474
*
7575
* @param overridingContext overriding context
76-
* @return resulting merged context
76+
* @return new, resulting merged context
7777
*/
7878
@Override
7979
public EvaluationContext merge(EvaluationContext overridingContext) {

Diff for: ‎src/main/java/dev/openfeature/sdk/OpenFeatureClient.java

+12-15
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
105105

106106
FlagEvaluationDetails<T> details = null;
107107
List<Hook> mergedHooks = null;
108-
HookContext<T> hookCtx = null;
108+
HookContext<T> afterHookContext = null;
109109
FeatureProvider provider;
110110

111111
try {
@@ -115,12 +115,11 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
115115
mergedHooks = ObjectUtils.merge(provider.getProviderHooks(), flagOptions.getHooks(), clientHooks,
116116
openfeatureApi.getHooks());
117117

118-
hookCtx = HookContext.from(key, type, this.getMetadata(),
119-
provider.getMetadata(), ctx, defaultValue);
118+
EvaluationContext mergedCtx = hookSupport.beforeHooks(type, HookContext.from(key, type, this.getMetadata(),
119+
provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue), mergedHooks, hints);
120120

121-
EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints);
122-
123-
EvaluationContext mergedCtx = mergeEvaluationContext(ctxFromHook, ctx);
121+
afterHookContext = HookContext.from(key, type, this.getMetadata(),
122+
provider.getMetadata(), mergedCtx, defaultValue);
124123

125124
ProviderEvaluation<T> providerEval = (ProviderEvaluation<T>) createProviderEvaluation(type, key,
126125
defaultValue, provider, mergedCtx);
@@ -129,7 +128,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
129128
if (details.getErrorCode() != null) {
130129
throw ExceptionUtils.instantiateErrorByErrorCode(details.getErrorCode(), details.getErrorMessage());
131130
} else {
132-
hookSupport.afterHooks(type, hookCtx, details, mergedHooks, hints);
131+
hookSupport.afterHooks(type, afterHookContext, details, mergedHooks, hints);
133132
}
134133
} catch (Exception e) {
135134
log.error("Unable to correctly evaluate flag with key '{}'", key, e);
@@ -144,24 +143,22 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
144143
details.setErrorMessage(e.getMessage());
145144
details.setValue(defaultValue);
146145
details.setReason(Reason.ERROR.toString());
147-
hookSupport.errorHooks(type, hookCtx, e, mergedHooks, hints);
146+
hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints);
148147
} finally {
149-
hookSupport.afterAllHooks(type, hookCtx, mergedHooks, hints);
148+
hookSupport.afterAllHooks(type, afterHookContext, mergedHooks, hints);
150149
}
151150

152151
return details;
153152
}
154153

155154
/**
156-
* Merge hook and invocation contexts with API, transaction and client contexts.
155+
* Merge invocation contexts with API, transaction and client contexts.
156+
* Does not merge before context.
157157
*
158-
* @param hookContext hook context
159158
* @param invocationContext invocation context
160159
* @return merged evaluation context
161160
*/
162-
private EvaluationContext mergeEvaluationContext(
163-
EvaluationContext hookContext,
164-
EvaluationContext invocationContext) {
161+
private EvaluationContext mergeEvaluationContext(EvaluationContext invocationContext) {
165162
final EvaluationContext apiContext = openfeatureApi.getEvaluationContext() != null
166163
? openfeatureApi.getEvaluationContext()
167164
: new ImmutableContext();
@@ -172,7 +169,7 @@ private EvaluationContext mergeEvaluationContext(
172169
? openfeatureApi.getTransactionContext()
173170
: new ImmutableContext();
174171

175-
return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext.merge(hookContext))));
172+
return apiContext.merge(transactionContext.merge(clientContext.merge(invocationContext)));
176173
}
177174

178175
private <T> ProviderEvaluation<?> createProviderEvaluation(

Diff for: ‎src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java

+53-5
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import static dev.openfeature.sdk.DoSomethingProvider.DEFAULT_METADATA;
44
import static org.assertj.core.api.Assertions.assertThat;
5-
import static org.assertj.core.api.Assertions.assertThatCode;
65
import static org.junit.jupiter.api.Assertions.assertEquals;
76
import static org.junit.jupiter.api.Assertions.assertFalse;
87
import static org.junit.jupiter.api.Assertions.assertNotNull;
@@ -20,6 +19,7 @@
2019
import java.util.HashMap;
2120
import java.util.List;
2221
import java.util.Map;
22+
import java.util.Optional;
2323

2424
import org.awaitility.Awaitility;
2525
import org.junit.jupiter.api.AfterEach;
@@ -336,11 +336,25 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
336336
FeatureProviderTestUtils.setFeatureProvider(provider);
337337
TransactionContextPropagator transactionContextPropagator = new ThreadLocalTransactionContextPropagator();
338338
api.setTransactionContextPropagator(transactionContextPropagator);
339+
Hook<Boolean> hook = spy(new Hook<Boolean>() {
340+
@Override
341+
public Optional<EvaluationContext> before(HookContext<Boolean> ctx, Map<String, Object> hints) {
342+
Map<String, Value> attrs = ctx.getCtx().asMap();
343+
attrs.put("before", new Value("5"));
344+
attrs.put("common7", new Value("5"));
345+
return Optional.ofNullable(new ImmutableContext(attrs));
346+
}
347+
@Override
348+
public void after(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
349+
Hook.super.after(ctx, details, hints);
350+
}
351+
});
339352

340353
Map<String, Value> apiAttributes = new HashMap<>();
341354
apiAttributes.put("common1", new Value("1"));
342355
apiAttributes.put("common2", new Value("1"));
343356
apiAttributes.put("common3", new Value("1"));
357+
apiAttributes.put("common7", new Value("1"));
344358
apiAttributes.put("api", new Value("1"));
345359
EvaluationContext apiCtx = new ImmutableContext(apiAttributes);
346360

@@ -377,21 +391,55 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
377391
invocationAttributes.put("invocation", new Value("4"));
378392
EvaluationContext invocationCtx = new ImmutableContext(invocationAttributes);
379393

380-
c.getBooleanValue("key", false, invocationCtx);
381-
382-
// assert the connect overrides
394+
c.getBooleanValue("key", false, invocationCtx, FlagEvaluationOptions.builder().hook(hook).build());
395+
396+
// assert the correct overrides in before hook
397+
verify(hook).before(argThat((arg) -> {
398+
EvaluationContext evaluationContext = arg.getCtx();
399+
return evaluationContext.getValue("api").asString().equals("1") &&
400+
evaluationContext.getValue("transaction").asString().equals("2") &&
401+
evaluationContext.getValue("client").asString().equals("3") &&
402+
evaluationContext.getValue("invocation").asString().equals("4") &&
403+
evaluationContext.getValue("common1").asString().equals("2") &&
404+
evaluationContext.getValue("common2").asString().equals("3") &&
405+
evaluationContext.getValue("common3").asString().equals("4") &&
406+
evaluationContext.getValue("common4").asString().equals("3") &&
407+
evaluationContext.getValue("common5").asString().equals("4") &&
408+
evaluationContext.getValue("common6").asString().equals("4");
409+
}), any());
410+
411+
// assert the correct overrides in evaluation
383412
verify(provider).getBooleanEvaluation(any(), any(), argThat((arg) -> {
384413
return arg.getValue("api").asString().equals("1") &&
385414
arg.getValue("transaction").asString().equals("2") &&
386415
arg.getValue("client").asString().equals("3") &&
387416
arg.getValue("invocation").asString().equals("4") &&
417+
arg.getValue("before").asString().equals("5") &&
388418
arg.getValue("common1").asString().equals("2") &&
389419
arg.getValue("common2").asString().equals("3") &&
390420
arg.getValue("common3").asString().equals("4") &&
391421
arg.getValue("common4").asString().equals("3") &&
392422
arg.getValue("common5").asString().equals("4") &&
393-
arg.getValue("common6").asString().equals("4");
423+
arg.getValue("common6").asString().equals("4") &&
424+
arg.getValue("common7").asString().equals("5");
394425
}));
426+
427+
// assert the correct overrides in after hook
428+
verify(hook).after(argThat((arg) -> {
429+
EvaluationContext evaluationContext = arg.getCtx();
430+
return evaluationContext.getValue("api").asString().equals("1") &&
431+
evaluationContext.getValue("transaction").asString().equals("2") &&
432+
evaluationContext.getValue("client").asString().equals("3") &&
433+
evaluationContext.getValue("invocation").asString().equals("4") &&
434+
evaluationContext.getValue("before").asString().equals("5") &&
435+
evaluationContext.getValue("common1").asString().equals("2") &&
436+
evaluationContext.getValue("common2").asString().equals("3") &&
437+
evaluationContext.getValue("common3").asString().equals("4") &&
438+
evaluationContext.getValue("common4").asString().equals("3") &&
439+
evaluationContext.getValue("common5").asString().equals("4") &&
440+
evaluationContext.getValue("common6").asString().equals("4") &&
441+
evaluationContext.getValue("common7").asString().equals("5");
442+
}), any(), any());
395443
}
396444

397445
@Specification(number="3.3.1.1", text="The API SHOULD have a method for setting a transaction context propagator.")

Diff for: ‎src/test/java/dev/openfeature/sdk/HookSpecTest.java

+7-6
Original file line numberDiff line numberDiff line change
@@ -484,10 +484,11 @@ public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
484484
@Specification(number = "4.1.4", text = "The evaluation context MUST be mutable only within the before hook.")
485485
@Specification(number = "4.3.4", text = "Any `evaluation context` returned from a `before` hook MUST be passed to subsequent `before` hooks (via `HookContext`).")
486486
@Test void beforeContextUpdated() {
487-
EvaluationContext ctx = new ImmutableContext();
488-
Hook hook = mockBooleanHook();
487+
String targetingKey = "test-key";
488+
EvaluationContext ctx = new ImmutableContext(targetingKey);
489+
Hook<Boolean> hook = mockBooleanHook();
489490
when(hook.before(any(), any())).thenReturn(Optional.of(ctx));
490-
Hook hook2 = mockBooleanHook();
491+
Hook<Boolean> hook2 = mockBooleanHook();
491492
when(hook.before(any(), any())).thenReturn(Optional.empty());
492493
InOrder order = inOrder(hook, hook2);
493494

@@ -499,11 +500,11 @@ public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
499500
.build());
500501

501502
order.verify(hook).before(any(), any());
502-
ArgumentCaptor<HookContext> captor = ArgumentCaptor.forClass(HookContext.class);
503+
ArgumentCaptor<HookContext<Boolean>> captor = ArgumentCaptor.forClass(HookContext.class);
503504
order.verify(hook2).before(captor.capture(), any());
504505

505-
HookContext hc = captor.getValue();
506-
assertEquals(hc.getCtx(), ctx);
506+
HookContext<Boolean> hc = captor.getValue();
507+
assertEquals(hc.getCtx().getTargetingKey(), targetingKey);
507508

508509
}
509510

0 commit comments

Comments
 (0)