Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: updated context not passed to all hooks #1049

Merged
merged 2 commits into from
Aug 20, 2024
Merged

fix: updated context not passed to all hooks #1049

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9dde9bb
fix: updated context not passed to all hooks
toddbaert Aug 20, 2024
88ecaf4
fixup: checkstyle
toddbaert Aug 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/ImmutableContext.java
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed before the fix.

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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This failed before the fix.

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
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The instance of the evaluation context now changes here, but that's not something we guarantee.


}

Expand Down
Loading