From f820fd67085705c96a179002bd3afc8f30004e05 Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Tue, 24 May 2022 15:29:30 -0700 Subject: [PATCH] Wiring for mergeable evaluation contexts. --- .../javasdk/EvaluationContext.java | 7 +++++ .../java/dev/openfeature/javasdk/Hook.java | 6 ++++- .../dev/openfeature/javasdk/HookContext.java | 3 ++- .../javasdk/OpenFeatureClient.java | 19 +++++++++----- .../openfeature/javasdk/HookSpecTests.java | 26 ++++++++++++------- 5 files changed, 43 insertions(+), 18 deletions(-) diff --git a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java index 58e62a27..96301656 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java @@ -1,4 +1,11 @@ package dev.openfeature.javasdk; public class EvaluationContext { + /** + * Merges two EvaluationContext objects with the second overriding the first in case of conflict. + */ + public static EvaluationContext merge(EvaluationContext ctx1, EvaluationContext ctx2) { + // TODO(abrahms): Actually implement this when we know what the fields of EC are. + return ctx1; + } } diff --git a/lib/src/main/java/dev/openfeature/javasdk/Hook.java b/lib/src/main/java/dev/openfeature/javasdk/Hook.java index c4e9bb40..932c9591 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/Hook.java +++ b/lib/src/main/java/dev/openfeature/javasdk/Hook.java @@ -2,9 +2,13 @@ package dev.openfeature.javasdk; import com.google.common.collect.ImmutableMap; +import java.util.Optional; + // TODO: interface? or abstract class? public abstract class Hook { - public void before(HookContext ctx, ImmutableMap hints) {} + public Optional before(HookContext ctx, ImmutableMap hints) { + return null; + } public void after(HookContext ctx, FlagEvaluationDetails details, ImmutableMap hints) {} public void error(HookContext ctx, Exception error, ImmutableMap hints) {} public void finallyAfter(HookContext ctx, ImmutableMap hints) {} diff --git a/lib/src/main/java/dev/openfeature/javasdk/HookContext.java b/lib/src/main/java/dev/openfeature/javasdk/HookContext.java index b3752a82..fe8a0eb2 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/HookContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/HookContext.java @@ -3,8 +3,9 @@ package dev.openfeature.javasdk; import lombok.Builder; import lombok.NonNull; import lombok.Value; +import lombok.With; -@Value @Builder +@Value @Builder @With public class HookContext { @NonNull String flagKey; @NonNull FlagValueType type; diff --git a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java index dc198c7a..9fbd52f3 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java +++ b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java @@ -11,6 +11,7 @@ import org.slf4j.LoggerFactory; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Optional; @SuppressWarnings("PMD.DataflowAnomalyAnalysis") public class OpenFeatureClient implements Client { @@ -56,13 +57,15 @@ public class OpenFeatureClient implements Client { FlagEvaluationDetails details = null; try { - this.beforeHooks(hookCtx, mergedHooks, hints); + EvaluationContext ctxFromHook = this.beforeHooks(hookCtx, mergedHooks, hints); + EvaluationContext invocationContext = EvaluationContext.merge(ctxFromHook, ctx); ProviderEvaluation providerEval; if (type == FlagValueType.BOOLEAN) { // TODO: Can we guarantee that defaultValue is a boolean? If not, how to we handle that? - providerEval = (ProviderEvaluation) provider.getBooleanEvaluation(key, (Boolean) defaultValue, ctx, options); + providerEval = (ProviderEvaluation) provider.getBooleanEvaluation(key, (Boolean) defaultValue, invocationContext, options); } else { + // TODO: Support other flag types. throw new GeneralError("Unknown flag type"); } @@ -106,13 +109,17 @@ public class OpenFeatureClient implements Client { } } - private HookContext beforeHooks(HookContext hookCtx, List hooks, ImmutableMap hints) { + private EvaluationContext beforeHooks(HookContext hookCtx, List hooks, ImmutableMap hints) { // These traverse backwards from normal. + EvaluationContext ctx = hookCtx.getCtx(); for (Hook hook : Lists.reverse(hooks)) { - hook.before(hookCtx, hints); - // TODO: Merge returned context w/ hook context object + Optional newCtx = hook.before(hookCtx, hints); + if (newCtx.isPresent()) { + ctx = EvaluationContext.merge(ctx, newCtx.get()); + hookCtx = hookCtx.withCtx(ctx); + } } - return hookCtx; + return ctx; } @Override diff --git a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java index da7175ab..265064ff 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java +++ b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java @@ -1,7 +1,6 @@ package dev.openfeature.javasdk; import com.google.common.collect.ImmutableMap; -import dev.openfeature.javasdk.*; import lombok.SneakyThrows; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Disabled; @@ -11,6 +10,7 @@ import org.mockito.InOrder; import java.util.ArrayList; import java.util.Arrays; import java.util.List; +import java.util.Optional; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.ArgumentMatchers.any; @@ -140,7 +140,7 @@ public class HookSpecTests { .build(); } - @Specification(spec="hooks", number="3.2", text="The before stage MUST run before flag evaluation occurs. It accepts a hook context (required) and HookHints (optional) as parameters and returns either a HookContext or nothing.") + @Specification(spec="hooks", number="3.2", text="The before stage MUST run before flag evaluation occurs. It accepts a hook context (required) and HookHints (optional) as parameters and returns either a EvaluationContext or nothing.") @Test void before_runs_ahead_of_evaluation() { OpenFeatureAPI api = OpenFeatureAPI.getInstance(); api.setProvider(new AlwaysBrokenProvider()); @@ -175,7 +175,7 @@ public class HookSpecTests { verify(h, times(0)).error(any(), any(), any()); } - @Specification(spec="hooks", number="3.4", text="The error hook MUST run when errors are encountered in the before stage, the after stage or during flag evaluation. It accepts hook context (required), exception for what went wrong (required), and HookHints (optional). It has no return value.") + @Specification(spec="hooks", number="3.6", text="The error hook MUST run when errors are encountered in the before stage, the after stage or during flag evaluation. It accepts hook context (required), exception for what went wrong (required), and HookHints (optional). It has no return value.") @Test void error_hook_run_during_non_finally_stage() { final boolean[] error_called = {false}; Hook h = mock(Hook.class); @@ -196,8 +196,9 @@ public class HookSpecTests { api.setProvider(new NoOpProvider()); api.registerHooks(new Hook() { @Override - public void before(HookContext ctx, ImmutableMap hints) { + public Optional before(HookContext ctx, ImmutableMap hints) { evalOrder.add("api before"); + return null; } @Override @@ -220,8 +221,9 @@ public class HookSpecTests { Client c = api.getClient(); c.registerHooks(new Hook() { @Override - public void before(HookContext ctx, ImmutableMap hints) { + public Optional before(HookContext ctx, ImmutableMap hints) { evalOrder.add("client before"); + return null; } @Override @@ -243,8 +245,9 @@ public class HookSpecTests { c.getBooleanValue("key", false, null, FlagEvaluationOptions.builder() .hook(new Hook() { @Override - public void before(HookContext ctx, ImmutableMap hints) { + public Optional before(HookContext ctx, ImmutableMap hints) { evalOrder.add("invocation before"); + return null; } @Override @@ -297,8 +300,9 @@ public class HookSpecTests { Client client = api.getClient(); Hook mutatingHook = new Hook() { @Override - public void before(HookContext ctx, ImmutableMap hints) { + public Optional before(HookContext ctx, ImmutableMap hints) { assertTrue(hints instanceof ImmutableMap); + return null; } @Override @@ -332,8 +336,8 @@ public class HookSpecTests { assertTrue(feo.getHookHints().isEmpty()); } - @Specification(spec="hooks", number="3.3", text="The after stage MUST run after flag evaluation occurs. It accepts a hook context (required), flag evaluation details (required) and HookHints (optional). It has no return value.") - @Specification(spec="hooks", number="3.5", text="The finally hook MUST run after the before, after, and error stages. It accepts a hook context (required) and HookHints (optional). There is no return value.") + @Specification(spec="hooks", number="3.5", text="The after stage MUST run after flag evaluation occurs. It accepts a hook context (required), flag evaluation details (required) and HookHints (optional). It has no return value.") + @Specification(spec="hooks", number="3.7", text="The finally hook MUST run after the before, after, and error stages. It accepts a hook context (required) and HookHints (optional). There is no return value.") @Test void flag_eval_hook_order() { Hook hook = mock(Hook.class); FeatureProvider provider = mock(FeatureProvider.class); @@ -391,12 +395,14 @@ public class HookSpecTests { verify(hook2, times(1)).error(any(), any(), any()); } + @Specification(spec="hooks", number="3.3", text="Any `EvaluationContext` returned from a `before` hook **MUST** be passed to subsequent `before` hooks (via `HookContext`).") + @Specification(spec="hooks", number="3.4", text="When `before` hooks have finished executing, any resulting `EvaluationContext` **MUST** be merged with the invocation `EvaluationContext` wherein the invocation `EvaluationContext` win any conflicts.") @Specification(spec="hooks", number="1.4", text="The evaluation context MUST be mutable only within the before hook.") @Specification(spec="hooks", number="3.1", text="Hooks MUST specify at least one stage.") @Test @Disabled void todo() {} @SneakyThrows - @Specification(spec="hooks", number="3.6", text="Condition: If finally is a reserved word in the language, finallyAfter SHOULD be used.") + @Specification(spec="hooks", number="3.8", text="Condition: If finally is a reserved word in the language, finallyAfter SHOULD be used.") @Test void doesnt_use_finally() { try { Hook.class.getMethod("finally", HookContext.class, ImmutableMap.class);