Skip to content

Commit 6b906b1

Browse files
toddbaerttoddbaert
committed
fix: updated context not passed to all hooks
Signed-off-by: Todd Baert <[email protected]>
1 parent 5e77f8a commit 6b906b1

File tree

5 files changed

+75
-20
lines changed

5 files changed

+75
-20
lines changed

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

+11-1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import lombok.NonNull;
55
import lombok.Value;
66
import lombok.With;
7+
import lombok.experimental.NonFinal;
78

89
/**
910
* A data class to hold immutable context that {@link Hook} instances use.
@@ -15,10 +16,19 @@ public class HookContext<T> {
1516
@NonNull String flagKey;
1617
@NonNull FlagValueType type;
1718
@NonNull T defaultValue;
18-
@NonNull EvaluationContext ctx;
19+
@NonNull @NonFinal EvaluationContext ctx;
1920
ClientMetadata clientMetadata;
2021
Metadata providerMetadata;
2122

23+
/**
24+
* Internal means of updating the evaluation context in this hook context.
25+
* Not for public use.
26+
* @param ctx context to set
27+
*/
28+
void setCtxInternal(EvaluationContext ctx) {
29+
this.ctx = ctx;
30+
}
31+
2232
/**
2333
* Builds a {@link HookContext} instances from request data.
2434
* @param key feature flag key

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
public final class ImmutableContext implements EvaluationContext {
1818

1919
@Delegate
20-
private final Structure structure;
20+
private Structure structure;
2121

2222
/**
2323
* Create an immutable context with an empty targeting_key and attributes provided.
@@ -78,7 +78,7 @@ public String getTargetingKey() {
7878
@Override
7979
public EvaluationContext merge(EvaluationContext overridingContext) {
8080
if (overridingContext == null) {
81-
return new ImmutableContext(this.asMap());
81+
return this;
8282
}
8383

8484
return new ImmutableContext(

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

+7-10
Original file line numberDiff line numberDiff line change
@@ -116,11 +116,10 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
116116
openfeatureApi.getHooks());
117117

118118
hookCtx = HookContext.from(key, type, this.getMetadata(),
119-
provider.getMetadata(), ctx, defaultValue);
119+
provider.getMetadata(), mergeEvaluationContext(ctx), defaultValue);
120120

121-
EvaluationContext ctxFromHook = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints);
122-
123-
EvaluationContext mergedCtx = mergeEvaluationContext(ctxFromHook, ctx);
121+
EvaluationContext mergedCtx = hookSupport.beforeHooks(type, hookCtx, mergedHooks, hints);
122+
hookCtx.setCtxInternal(mergedCtx);
124123

125124
ProviderEvaluation<T> providerEval = (ProviderEvaluation<T>) createProviderEvaluation(type, key,
126125
defaultValue, provider, mergedCtx);
@@ -153,15 +152,13 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
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

+52-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,54 @@ 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 connect 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 connect 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 connect 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+
}), any(), any());
395442
}
396443

397444
@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

+3-2
Original file line numberDiff line numberDiff line change
@@ -484,7 +484,8 @@ 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();
487+
String targetingKey = "test-key";
488+
EvaluationContext ctx = new ImmutableContext(targetingKey);
488489
Hook hook = mockBooleanHook();
489490
when(hook.before(any(), any())).thenReturn(Optional.of(ctx));
490491
Hook hook2 = mockBooleanHook();
@@ -503,7 +504,7 @@ public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
503504
order.verify(hook2).before(captor.capture(), any());
504505

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

508509
}
509510

0 commit comments

Comments
 (0)