Skip to content

Commit

Permalink
fix: updated context not passed to all hooks (#1049)
Browse files Browse the repository at this point in the history 
* fix: updated context not passed to all hooks

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

* fixup: checkstyle

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

---------

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>
  • Loading branch information
@toddbaert
toddbaert authored Aug 20, 2024
1 parent 7cac198 commit dbf967a
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 27 deletions.
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/ImmutableContext.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
27 changes: 12 additions & 15 deletions src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key

FlagEvaluationDetails<T> details = null;
List<Hook> mergedHooks = null;
HookContext<T> hookCtx = null;
HookContext<T> afterHookContext = null;
FeatureProvider provider;

try {
Expand All @@ -115,12 +115,11 @@ private <T> FlagEvaluationDetails<T> 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<T> providerEval = (ProviderEvaluation<T>) createProviderEvaluation(type, key,
defaultValue, provider, mergedCtx);
Expand All @@ -129,7 +128,7 @@ private <T> FlagEvaluationDetails<T> 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);
Expand All @@ -144,24 +143,22 @@ private <T> FlagEvaluationDetails<T> 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();
Expand All @@ -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 <T> ProviderEvaluation<?> createProviderEvaluation(
Expand Down
58 changes: 53 additions & 5 deletions src/test/java/dev/openfeature/sdk/FlagEvaluationSpecTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -336,11 +336,25 @@ public void initialize(EvaluationContext evaluationContext) throws Exception {
FeatureProviderTestUtils.setFeatureProvider(provider);
TransactionContextPropagator transactionContextPropagator = new ThreadLocalTransactionContextPropagator();
api.setTransactionContextPropagator(transactionContextPropagator);
Hook<Boolean> hook = spy(new Hook<Boolean>() {
@Override
public Optional<EvaluationContext> before(HookContext<Boolean> ctx, Map<String, Object> hints) {
Map<String, Value> 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<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
Hook.super.after(ctx, details, hints);
}
});

Map<String, Value> 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);

Expand Down Expand Up @@ -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.")
Expand Down
13 changes: 7 additions & 6 deletions src/test/java/dev/openfeature/sdk/HookSpecTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,10 +484,11 @@ public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> 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<Boolean> hook = mockBooleanHook();
when(hook.before(any(), any())).thenReturn(Optional.of(ctx));
Hook hook2 = mockBooleanHook();
Hook<Boolean> hook2 = mockBooleanHook();
when(hook.before(any(), any())).thenReturn(Optional.empty());
InOrder order = inOrder(hook, hook2);

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

order.verify(hook).before(any(), any());
ArgumentCaptor<HookContext> captor = ArgumentCaptor.forClass(HookContext.class);
ArgumentCaptor<HookContext<Boolean>> captor = ArgumentCaptor.forClass(HookContext.class);
order.verify(hook2).before(captor.capture(), any());

HookContext hc = captor.getValue();
assertEquals(hc.getCtx(), ctx);
HookContext<Boolean> hc = captor.getValue();
assertEquals(hc.getCtx().getTargetingKey(), targetingKey);

}

Expand Down

0 comments on commit dbf967a

Please sign in to comment.