diff --git a/src/main/java/dev/openfeature/sdk/Hook.java b/src/main/java/dev/openfeature/sdk/Hook.java index 3856af266..b4dbbd2cb 100644 --- a/src/main/java/dev/openfeature/sdk/Hook.java +++ b/src/main/java/dev/openfeature/sdk/Hook.java @@ -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) { diff --git a/src/main/java/dev/openfeature/sdk/HookSupport.java b/src/main/java/dev/openfeature/sdk/HookSupport.java index f0216b255..45ace1c58 100644 --- a/src/main/java/dev/openfeature/sdk/HookSupport.java +++ b/src/main/java/dev/openfeature/sdk/HookSupport.java @@ -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, diff --git a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java index ea566e652..54c138396 100644 --- a/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java +++ b/src/main/java/dev/openfeature/sdk/OpenFeatureClient.java @@ -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; diff --git a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java index 4502699b1..a20138f6b 100644 --- a/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java +++ b/src/test/java/dev/openfeature/sdk/DeveloperExperienceTest.java @@ -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); } @@ -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); } diff --git a/src/test/java/dev/openfeature/sdk/HookSpecTest.java b/src/test/java/dev/openfeature/sdk/HookSpecTest.java index 4609c8d51..7ab01c0a6 100644 --- a/src/test/java/dev/openfeature/sdk/HookSpecTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSpecTest.java @@ -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.*; @@ -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 @@ -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()); } @@ -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(); @@ -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"); } }); @@ -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"); } }); @@ -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"); } }); @@ -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"); } }) @@ -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.") @@ -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); } }; @@ -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.") @@ -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(); @@ -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); @@ -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.") @@ -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(); } - } diff --git a/src/test/java/dev/openfeature/sdk/HookSupportTest.java b/src/test/java/dev/openfeature/sdk/HookSupportTest.java index bf6501dd5..9b569354d 100644 --- a/src/test/java/dev/openfeature/sdk/HookSupportTest.java +++ b/src/test/java/dev/openfeature/sdk/HookSupportTest.java @@ -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()); } diff --git a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java index 3c82fd656..c15200b4a 100644 --- a/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java +++ b/src/test/java/dev/openfeature/sdk/OpenFeatureClientTest.java @@ -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; @@ -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 { @@ -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; - } - } }