Skip to content

Commit

Permalink
feat: Add evaluation details to finally hook stage open-feature#1246
Browse files Browse the repository at this point in the history 
Signed-off-by: christian.lutnik <christian.lutnik@dynatrace.com>
  • Loading branch information
@chrfwow
chrfwow committed Dec 23, 2024
1 parent fc6f35e commit 0e1db4b
Show file tree
Hide file tree
Showing 7 changed files with 90 additions and 93 deletions.
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/Hook.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ default void error(HookContext<T> ctx, Exception error, Map<String, Object> hint
* @param ctx Information about the particular flag evaluation
* @param hints An immutable mapping of data for users to communicate to the hooks.
*/
default void finallyAfter(HookContext<T> ctx, Map<String, Object> hints) {
default void finallyAfter(HookContext<T> ctx, FlagEvaluationDetails<T> details, Map<String, Object> hints) {
}

default boolean supportsFlagValueType(FlagValueType flagValueType) {
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/dev/openfeature/sdk/HookSupport.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,9 @@ public void afterHooks(FlagValueType flagValueType, HookContext hookContext, Fla
executeHooksUnchecked(flagValueType, hooks, hook -> hook.after(hookContext, details, hints));
}

public void afterAllHooks(FlagValueType flagValueType, HookContext hookCtx, List<Hook> hooks,
Map<String, Object> hints) {
executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, hints));
public void afterAllHooks(FlagValueType flagValueType, HookContext hookCtx, FlagEvaluationDetails details,
List<Hook> hooks, Map<String, Object> hints) {
executeHooks(flagValueType, hooks, "finally", hook -> hook.finallyAfter(hookCtx, details, hints));
}

public void errorHooks(FlagValueType flagValueType, HookContext hookCtx, Exception e, List<Hook> hooks,
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/dev/openfeature/sdk/OpenFeatureClient.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,7 @@ private <T> FlagEvaluationDetails<T> evaluateFlag(FlagValueType type, String key
enrichDetailsWithErrorDefaults(defaultValue, details);
hookSupport.errorHooks(type, afterHookContext, e, mergedHooks, hints);
} finally {
hookSupport.afterAllHooks(type, afterHookContext, mergedHooks, hints);
hookSupport.afterAllHooks(type, afterHookContext, details, mergedHooks, hints);
}

return details;
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ void clientHooks() {
Client client = api.getClient();
client.addHooks(exampleHook);
Boolean retval = client.getBooleanValue(flagKey, false);
verify(exampleHook, times(1)).finallyAfter(any(), any());
verify(exampleHook, times(1)).finallyAfter(any(), any(), any());
assertFalse(retval);
}

Expand All @@ -51,8 +51,8 @@ void evalHooks() {
client.addHooks(clientHook);
Boolean retval = client.getBooleanValue(flagKey, false, null,
FlagEvaluationOptions.builder().hook(evalHook).build());
verify(clientHook, times(1)).finallyAfter(any(), any());
verify(evalHook, times(1)).finallyAfter(any(), any());
verify(clientHook, times(1)).finallyAfter(any(), any(), any());
verify(evalHook, times(1)).finallyAfter(any(), any(), any());
assertFalse(retval);
}

Expand Down
85 changes: 68 additions & 17 deletions src/test/java/dev/openfeature/sdk/HookSpecTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@

import java.util.*;

import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.fail;
import static org.assertj.core.api.Assertions.*;
import static org.junit.jupiter.api.Assertions.*;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;
Expand Down Expand Up @@ -115,7 +115,7 @@ void nullish_properties_on_hookcontext() {

}

@Specification(number = "4.1.2", text = "The hook context SHOULD provide: access to the client metadata and the provider metadata fields.")
@Specification(number = "4.1.2", text = "The `hook context` SHOULD provide access to the `client metadata` and the `provider metadata` fields.")
@Test
void optional_properties() {
// don't specify
Expand Down Expand Up @@ -170,7 +170,7 @@ void feo_has_hook_list() {
void error_hook_run_during_non_finally_stage() {
final boolean[] error_called = {false};
Hook h = mockBooleanHook();
doThrow(RuntimeException.class).when(h).finallyAfter(any(), any());
doThrow(RuntimeException.class).when(h).finallyAfter(any(), any(), any());

verify(h, times(0)).error(any(), any(), any());
}
Expand Down Expand Up @@ -201,7 +201,7 @@ void error_hook_must_run_if_resolution_details_returns_an_error_code() {

verify(hook, times(1)).before(any(), any());
verify(hook, times(1)).error(any(), captor.capture(), any());
verify(hook, times(1)).finallyAfter(any(), any());
verify(hook, times(1)).finallyAfter(any(), any(), any());
verify(hook, never()).after(any(), any(), any());

Exception exception = captor.getValue();
Expand Down Expand Up @@ -241,7 +241,7 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
}

@Override
public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
evalOrder.add("provider finally");
}
});
Expand All @@ -266,7 +266,7 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
}

@Override
public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
evalOrder.add("api finally");
}
});
Expand All @@ -290,7 +290,7 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
}

@Override
public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
evalOrder.add("client finally");
}
});
Expand All @@ -315,7 +315,7 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
}

@Override
public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
evalOrder.add("invocation finally");
}
})
Expand Down Expand Up @@ -344,8 +344,8 @@ void error_stops_before() {
.hook(h2)
.hook(h)
.build());
verify(h, times(1)).before(any(), any());
verify(h2, times(0)).before(any(), any());
verify(h, times(1)).before(any(), any());
verify(h2, times(0)).before(any(), any());
}

@Specification(number = "4.4.6", text = "If an error occurs during the evaluation of before or after hooks, any remaining hooks in the before or after stages MUST NOT be invoked.")
Expand Down Expand Up @@ -393,7 +393,7 @@ public void error(HookContext<Boolean> ctx, Exception error, Map<String, Object>
}

@Override
public void finallyAfter(HookContext<Boolean> ctx, Map<String, Object> hints) {
public void finallyAfter(HookContext<Boolean> ctx, FlagEvaluationDetails<Boolean> details, Map<String, Object> hints) {
assertThatCode(() -> hints.put(hintKey, "changed value")).isInstanceOf(UnsupportedOperationException.class);
}
};
Expand Down Expand Up @@ -435,7 +435,7 @@ void flag_eval_hook_order() {
order.verify(hook).before(any(), any());
order.verify(provider).getBooleanEvaluation(any(), any(), any());
order.verify(hook).after(any(), any(), any());
order.verify(hook).finallyAfter(any(), any());
order.verify(hook).finallyAfter(any(), any(), any());
}

@Specification(number = "4.4.5", text = "If an error occurs in the before or after hooks, the error hooks MUST be invoked.")
Expand Down Expand Up @@ -464,6 +464,58 @@ void error_hooks__after() {
verify(hook, times(1)).error(any(), any(), any());
}

@Test
void erroneous_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
Hook hook = mockBooleanHook();
doThrow(RuntimeException.class).when(hook).after(any(), any(), any());
String flagKey = "test-flag-key";
Client client = getClient(TestEventsProvider.newInitializedTestEventsProvider());
client.getBooleanValue(
flagKey,
true,
new ImmutableContext(),
FlagEvaluationOptions.builder().hook(hook).build()
);

ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
verify(hook).finallyAfter(any(), captor.capture(), any());

FlagEvaluationDetails<Boolean> evaluationDetails = captor.getValue();
assertThat(evaluationDetails).isNotNull();

assertThat(evaluationDetails.getErrorCode()).isEqualTo(ErrorCode.GENERAL);
assertThat(evaluationDetails.getReason()).isEqualTo("ERROR");
assertThat(evaluationDetails.getVariant()).isEqualTo("Passed in default");
assertThat(evaluationDetails.getFlagKey()).isEqualTo(flagKey);
assertThat(evaluationDetails.getFlagMetadata()).isEqualTo(ImmutableMetadata.builder().build());
assertThat(evaluationDetails.getValue()).isEqualTo(true);
}

@Test
void successful_flagResolution_setsAppropriateFieldsInFlagEvaluationDetails() {
Hook hook = mockBooleanHook();
String flagKey = "test-flag-key";
Client client = getClient(TestEventsProvider.newInitializedTestEventsProvider());
client.getBooleanValue(
flagKey,
true,
new ImmutableContext(),
FlagEvaluationOptions.builder().hook(hook).build()
);

ArgumentCaptor<FlagEvaluationDetails<Boolean>> captor = ArgumentCaptor.forClass(FlagEvaluationDetails.class);
verify(hook).finallyAfter(any(), captor.capture(), any());

FlagEvaluationDetails<Boolean> evaluationDetails = captor.getValue();
assertThat(evaluationDetails).isNotNull();
assertThat(evaluationDetails.getErrorCode()).isNull();
assertThat(evaluationDetails.getReason()).isEqualTo("DEFAULT");
assertThat(evaluationDetails.getVariant()).isEqualTo("Passed in default");
assertThat(evaluationDetails.getFlagKey()).isEqualTo(flagKey);
assertThat(evaluationDetails.getFlagMetadata()).isEqualTo(ImmutableMetadata.builder().build());
assertThat(evaluationDetails.getValue()).isEqualTo(true);
}

@Test
void multi_hooks_early_out__before() {
Hook<Boolean> hook = mockBooleanHook();
Expand Down Expand Up @@ -556,7 +608,7 @@ void mergeHappensCorrectly() {
void first_finally_broken() {
Hook hook = mockBooleanHook();
doThrow(RuntimeException.class).when(hook).before(any(), any());
doThrow(RuntimeException.class).when(hook).finallyAfter(any(), any());
doThrow(RuntimeException.class).when(hook).finallyAfter(any(), any(), any());
Hook hook2 = mockBooleanHook();
InOrder order = inOrder(hook, hook2);

Expand All @@ -568,8 +620,8 @@ void first_finally_broken() {
.build());

order.verify(hook).before(any(), any());
order.verify(hook2).finallyAfter(any(), any());
order.verify(hook).finallyAfter(any(), any());
order.verify(hook2).finallyAfter(any(), any(), any());
order.verify(hook).finallyAfter(any(), any(), any());
}

@Specification(number = "4.4.4", text = "If an error hook abnormally terminates, evaluation MUST proceed, including the execution of any remaining error hooks.")
Expand Down Expand Up @@ -616,8 +668,7 @@ void doesnt_use_finally() {
.as("Not possible. Finally is a reserved word.")
.isInstanceOf(NoSuchMethodException.class);

assertThatCode(() -> Hook.class.getMethod("finallyAfter", HookContext.class, Map.class))
assertThatCode(() -> Hook.class.getMethod("finallyAfter", HookContext.class, FlagEvaluationDetails.class, Map.class))
.doesNotThrowAnyException();
}

}
4 changes: 2 additions & 2 deletions src/test/java/dev/openfeature/sdk/HookSupportTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,12 +51,12 @@ void shouldAlwaysCallGenericHook(FlagValueType flagValueType) {

hookSupport.beforeHooks(flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.afterHooks(flagValueType, hookContext, FlagEvaluationDetails.builder().build(), Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.afterAllHooks(flagValueType, hookContext, Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.afterAllHooks(flagValueType, hookContext, FlagEvaluationDetails.builder().build(), Collections.singletonList(genericHook), Collections.emptyMap());
hookSupport.errorHooks(flagValueType, hookContext, expectedException, Collections.singletonList(genericHook), Collections.emptyMap());

verify(genericHook).before(any(), any());
verify(genericHook).after(any(), any(), any());
verify(genericHook).finallyAfter(any(), any());
verify(genericHook).finallyAfter(any(), any(), any());
verify(genericHook).error(any(), any(), any());
}

Expand Down
78 changes: 12 additions & 66 deletions src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,15 +1,8 @@
package dev.openfeature.sdk;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.*;

import java.util.HashMap;
import java.util.concurrent.atomic.AtomicBoolean;

import dev.openfeature.sdk.exceptions.FatalError;
import dev.openfeature.sdk.fixtures.HookFixtures;
import dev.openfeature.sdk.testutils.TestEventsProvider;
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.DisplayName;
Expand All @@ -18,9 +11,15 @@
import org.simplify4u.slf4jmock.LoggerMock;
import org.slf4j.Logger;

import dev.openfeature.sdk.exceptions.FatalError;
import dev.openfeature.sdk.fixtures.HookFixtures;
import dev.openfeature.sdk.testutils.TestEventsProvider;
import java.util.HashMap;

import static org.assertj.core.api.Assertions.assertThat;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;

class OpenFeatureClientTest implements HookFixtures {

Expand Down Expand Up @@ -102,57 +101,4 @@ void shouldNotCallEvaluationMethodsWhenProviderIsInNotReadyState() {

assertThat(details.getErrorCode()).isEqualTo(ErrorCode.PROVIDER_NOT_READY);
}

private static class MockProvider implements FeatureProvider {
private final AtomicBoolean evaluationCalled = new AtomicBoolean();
private final ProviderState providerState;

public MockProvider(ProviderState providerState) {
this.providerState = providerState;
}

public boolean isEvaluationCalled() {
return evaluationCalled.get();
}

@Override
public ProviderState getState() {
return providerState;
}

@Override
public Metadata getMetadata() {
return null;
}

@Override
public ProviderEvaluation<Boolean> getBooleanEvaluation(String key, Boolean defaultValue, EvaluationContext ctx) {
evaluationCalled.set(true);
return null;
}

@Override
public ProviderEvaluation<String> getStringEvaluation(String key, String defaultValue, EvaluationContext ctx) {
evaluationCalled.set(true);
return null;
}

@Override
public ProviderEvaluation<Integer> getIntegerEvaluation(String key, Integer defaultValue, EvaluationContext ctx) {
evaluationCalled.set(true);
return null;
}

@Override
public ProviderEvaluation<Double> getDoubleEvaluation(String key, Double defaultValue, EvaluationContext ctx) {
evaluationCalled.set(true);
return null;
}

@Override
public ProviderEvaluation<Value> getObjectEvaluation(String key, Value defaultValue, EvaluationContext ctx) {
evaluationCalled.set(true);
return null;
}
}
}

0 comments on commit 0e1db4b

Please sign in to comment.