From f820fd67085705c96a179002bd3afc8f30004e05 Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Tue, 24 May 2022 15:29:30 -0700 Subject: [PATCH 1/4] 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); From 23e2cd0a6bad86c2fd8a708f70544e20b9db3934 Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Thu, 19 May 2022 13:47:35 -0700 Subject: [PATCH 2/4] Blocked waiting on EvalContext spec to be finalized. --- .../javasdk/EvaluationContext.java | 165 ++++++++++++++++++ .../dev/openfeature/javasdk/HookContext.java | 3 +- .../javasdk/OpenFeatureClient.java | 16 +- .../javasdk/DeveloperExperienceTest.java | 2 +- .../openfeature/javasdk/EvalContextTests.java | 100 +++++++++++ .../openfeature/javasdk/HookSpecTests.java | 4 +- 6 files changed, 277 insertions(+), 13 deletions(-) create mode 100644 lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java diff --git a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java index 96301656..34e4fd07 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java @@ -1,6 +1,171 @@ package dev.openfeature.javasdk; +import lombok.Getter; + +import java.time.ZonedDateTime; +import java.time.format.DateTimeFormatter; +import java.util.HashMap; +import java.util.Map; + public class EvaluationContext { + @Getter private final String targetingKey; + private final Map integerAttributes; + private final Map stringAttributes; + + + private enum KNOWN_KEYS { + EMAIL, + FIRST_NAME, + LAST_NAME, + NAME, + IP, + TZ, + LOCALE, + COUNTRY_CODE, + ENVIRONMENT, + APPLICATION, + VERSION, + TIMESTAMP, + } + + EvaluationContext() { + this.targetingKey = ""; + this.integerAttributes = new HashMap<>(); + this.stringAttributes = new HashMap<>(); + } + + public void addStringAttribute(String key, String value) { + stringAttributes.put(key, value); + } + + public String getStringAttribute(String key) { + return stringAttributes.get(key); + } + + public void addIntegerAttribute(String key, Integer value) { + integerAttributes.put(key, value); + } + + public Integer getIntegerAttribute(String key) { + return integerAttributes.get(key); + } + + public Boolean getBooleanAttribute(String key) { + return Boolean.valueOf(stringAttributes.get(key)); + } + + public void addBooleanAttribute(String key, Boolean b) { + stringAttributes.put(key, b.toString()); + } + + public void addDatetimeAttribute(String key, ZonedDateTime value) { + this.stringAttributes.put(key, value.format(DateTimeFormatter.ISO_ZONED_DATE_TIME)); + } + + public ZonedDateTime getDatetimeAttribute(String key) { + String attr = this.stringAttributes.get(key); + if (attr == null) { + return null; + } + return ZonedDateTime.parse(attr, DateTimeFormatter.ISO_ZONED_DATE_TIME); + } + + public String getEmail() { + return this.stringAttributes.get(KNOWN_KEYS.EMAIL.toString()); + } + + public String getFirstName() { + return this.stringAttributes.get(KNOWN_KEYS.FIRST_NAME.toString()); + } + + public String getLastName() { + return this.stringAttributes.get(KNOWN_KEYS.LAST_NAME.toString()); + } + + public String getName() { + return this.stringAttributes.get(KNOWN_KEYS.NAME.toString()); + } + + public String getIp() { + return this.stringAttributes.get(KNOWN_KEYS.IP.toString()); + } + + public String getTz() { + return this.stringAttributes.get(KNOWN_KEYS.TZ.toString()); + } + + public String getLocale() { + return this.stringAttributes.get(KNOWN_KEYS.LOCALE.toString()); + } + + public String getCountryCode() { + return this.stringAttributes.get(KNOWN_KEYS.COUNTRY_CODE.toString()); + } + + public String getEnvironment() { + return this.stringAttributes.get(KNOWN_KEYS.ENVIRONMENT.toString()); + } + + public String getApplication() { + return this.stringAttributes.get(KNOWN_KEYS.APPLICATION.toString()); + } + + public String getVersion() { + return this.stringAttributes.get(KNOWN_KEYS.VERSION.toString()); + } + + public ZonedDateTime getTimestamp() { + return getDatetimeAttribute(KNOWN_KEYS.TIMESTAMP.toString()); + } + + public void setEmail(String email) { + this.stringAttributes.put(KNOWN_KEYS.EMAIL.toString(), email); + } + + public void setFirstName(String firstname) { + this.stringAttributes.put(KNOWN_KEYS.FIRST_NAME.toString(), firstname); + } + + public void setLastName(String lastname) { + this.stringAttributes.put(KNOWN_KEYS.LAST_NAME.toString(), lastname); + } + + public void setName(String name) { + this.stringAttributes.put(KNOWN_KEYS.NAME.toString(), name); + } + + public void setIp(String ip) { + this.stringAttributes.put(KNOWN_KEYS.IP.toString(), ip); + } + + public void setTz(String tz) { + this.stringAttributes.put(KNOWN_KEYS.TZ.toString(), tz); + } + + public void setLocale(String locale) { + this.stringAttributes.put(KNOWN_KEYS.LOCALE.toString(), locale); + } + + public void setCountryCode(String countryCode) { + this.stringAttributes.put(KNOWN_KEYS.COUNTRY_CODE.toString(), countryCode); + } + + public void setEnvironment(String environment) { + this.stringAttributes.put(KNOWN_KEYS.ENVIRONMENT.toString(), environment); + } + + public void setApplication(String application) { + this.stringAttributes.put(KNOWN_KEYS.APPLICATION.toString(), application); + } + + public void setVersion(String version) { + this.stringAttributes.put(KNOWN_KEYS.VERSION.toString(), version); + } + + public void setTimestamp(ZonedDateTime timestamp) { + addDatetimeAttribute(KNOWN_KEYS.TIMESTAMP.toString(), timestamp); + } + /** * Merges two EvaluationContext objects with the second overriding the first in case of conflict. */ diff --git a/lib/src/main/java/dev/openfeature/javasdk/HookContext.java b/lib/src/main/java/dev/openfeature/javasdk/HookContext.java index fe8a0eb2..3773a26c 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/HookContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/HookContext.java @@ -4,13 +4,14 @@ import lombok.Builder; import lombok.NonNull; import lombok.Value; import lombok.With; +import javax.annotation.Nullable; @Value @Builder @With public class HookContext { @NonNull String flagKey; @NonNull FlagValueType type; @NonNull T defaultValue; - @NonNull EvaluationContext ctx; + @Nullable EvaluationContext ctx; Client client; FeatureProvider provider; diff --git a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java index 9fbd52f3..4e3d8e29 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java +++ b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java @@ -35,14 +35,12 @@ public class OpenFeatureClient implements Client { FlagEvaluationDetails evaluateFlag(FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) { FeatureProvider provider = this.openfeatureApi.getProvider(); - if (ctx == null) { - ctx = new EvaluationContext(); - } - ImmutableMap hints = options.getHookHints(); + // merge of: API.context, client.context, invocation.context + // TODO: Context transformation? - HookContext hookCtx = HookContext.from(key, type, this, ctx, defaultValue); + HookContext hookCtx = HookContext.from(key, type, this, null, defaultValue); List mergedHooks; if (options != null && options.getHooks() != null) { @@ -139,7 +137,7 @@ public class OpenFeatureClient implements Client { @Override public FlagEvaluationDetails getBooleanDetails(String key, Boolean defaultValue) { - return getBooleanDetails(key, defaultValue, new EvaluationContext()); + return getBooleanDetails(key, defaultValue, null); } @Override @@ -169,12 +167,12 @@ public class OpenFeatureClient implements Client { @Override public FlagEvaluationDetails getStringDetails(String key, String defaultValue) { - return getStringDetails(key, defaultValue, new EvaluationContext()); + return getStringDetails(key, defaultValue, null); } @Override public FlagEvaluationDetails getStringDetails(String key, String defaultValue, EvaluationContext ctx) { - return getStringDetails(key, defaultValue, new EvaluationContext(), FlagEvaluationOptions.builder().build()); + return getStringDetails(key, defaultValue, ctx, FlagEvaluationOptions.builder().build()); } @Override @@ -199,7 +197,7 @@ public class OpenFeatureClient implements Client { @Override public FlagEvaluationDetails getIntegerDetails(String key, Integer defaultValue) { - return getIntegerDetails(key, defaultValue, new EvaluationContext()); + return getIntegerDetails(key, defaultValue, null); } @Override diff --git a/lib/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java b/lib/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java index e9ada0e4..f103843b 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java +++ b/lib/src/test/java/dev/openfeature/javasdk/DeveloperExperienceTest.java @@ -41,7 +41,7 @@ class DeveloperExperienceTest { api.setProvider(new NoOpProvider()); Client client = api.getClient(); client.registerHooks(clientHook); - Boolean retval = client.getBooleanValue(flagKey, false, new EvaluationContext(), + 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()); diff --git a/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java b/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java new file mode 100644 index 00000000..d777a74c --- /dev/null +++ b/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java @@ -0,0 +1,100 @@ +package dev.openfeature.javasdk; + +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; + +import java.time.ZonedDateTime; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNull; + +public class EvalContextTests { + @Specification(spec="flag evaluation", number="3.1", + text="The `evaluation context` structure MUST define a required `targeting key` " + + "field of type string, identifying the subject of the flag evaluation.") + @Disabled("https://github.com/open-feature/spec/pull/60/files#r872827439") + @Test void requires_targeting_key() { + EvaluationContext ec = new EvaluationContext(); + assertEquals("targeting-key", ec.getTargetingKey()); + } + + @Specification(spec="flag evaluation", number="3.3", text="The evaluation context MUST support the inclusion " + + "of custom fields, having keys of type `string`, and values of " + + "type `boolean | string | number | datetime`.") + @Test void eval_context() { + EvaluationContext ec = new EvaluationContext(); + + ec.addStringAttribute("str", "test"); + assertEquals("test", ec.getStringAttribute("str")); + + ec.addBooleanAttribute("bool", true); + assertEquals(true, ec.getBooleanAttribute("bool")); + + ec.addIntegerAttribute("int", 4); + assertEquals(4, ec.getIntegerAttribute("int")); + + ZonedDateTime dt = ZonedDateTime.now(); + ec.addDatetimeAttribute("dt", dt); + assertEquals(dt, ec.getDatetimeAttribute("dt")); + } + + + @Specification(spec="flag evaluation", number="3.2", text="The evaluation context MUST define the " + + "following optional fields: `email` (string), `first name` (string), `last name`(string), " + + "`name`(string), `ip`(string), `tz`(string), `locale`(string), `country code` (string), " + + "`timestamp`(date), `environment`(string), `application`(string), and `version`(string).") + @Test void mandated_fields() { + EvaluationContext ec = new EvaluationContext(); + + assertNull(ec.getEmail()); + ec.setEmail("Test"); + assertEquals("Test", ec.getEmail()); + + assertNull(ec.getFirstName()); + ec.setFirstName("Test"); + assertEquals("Test", ec.getFirstName()); + + assertNull(ec.getLastName()); + ec.setLastName("Test"); + assertEquals("Test", ec.getLastName()); + + assertNull(ec.getName()); + ec.setName("Test"); + assertEquals("Test", ec.getName()); + + assertNull(ec.getIp()); + ec.setIp("Test"); + assertEquals("Test", ec.getIp()); + + assertNull(ec.getTz()); + ec.setTz("Test"); + assertEquals("Test", ec.getTz()); + + assertNull(ec.getLocale()); + ec.setLocale("Test"); + assertEquals("Test", ec.getLocale()); + + assertNull(ec.getCountryCode()); + ec.setCountryCode("Test"); + assertEquals("Test", ec.getCountryCode()); + + assertNull(ec.getTimestamp()); + ZonedDateTime dt = ZonedDateTime.now(); + ec.setTimestamp(dt); + assertEquals(dt, ec.getTimestamp()); + + assertNull(ec.getEnvironment()); + ec.setEnvironment("Test"); + assertEquals("Test", ec.getEnvironment()); + + + assertNull(ec.getApplication()); + ec.setApplication("Test"); + assertEquals("Test", ec.getApplication()); + + + assertNull(ec.getVersion()); + ec.setVersion("Test"); + assertEquals("Test", ec.getVersion()); + } +} diff --git a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java index 265064ff..8190b9a6 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java +++ b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java @@ -65,7 +65,7 @@ public class HookSpecTests { try { HookContext.builder() .flagKey("key") - .ctx(new EvaluationContext()) + .ctx(null) .defaultValue(1) .build(); fail("Missing type shouldn't be valid"); @@ -77,7 +77,7 @@ public class HookSpecTests { try { HookContext.builder() .type(FlagValueType.INTEGER) - .ctx(new EvaluationContext()) + .ctx(null) .defaultValue(1) .build(); fail("Missing key shouldn't be valid"); From aaa924f94c13123479a4ab88b4579059a970113c Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Wed, 25 May 2022 17:57:32 -0700 Subject: [PATCH 3/4] Support for eval context & merging relevant thereof --- .../javasdk/EvaluationContext.java | 147 ++++-------------- .../java/dev/openfeature/javasdk/Hook.java | 2 +- .../dev/openfeature/javasdk/HookContext.java | 3 +- .../javasdk/OpenFeatureClient.java | 11 +- .../openfeature/javasdk/EvalContextTests.java | 79 ++-------- .../javasdk/FlagEvaluationSpecTests.java | 2 +- .../openfeature/javasdk/HookSpecTests.java | 60 +++++++ 7 files changed, 114 insertions(+), 190 deletions(-) diff --git a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java index 34e4fd07..7b4792ff 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java @@ -1,33 +1,21 @@ package dev.openfeature.javasdk; +import lombok.EqualsAndHashCode; import lombok.Getter; +import lombok.Setter; +import lombok.ToString; import java.time.ZonedDateTime; import java.time.format.DateTimeFormatter; import java.util.HashMap; import java.util.Map; +@ToString @EqualsAndHashCode public class EvaluationContext { - @Getter private final String targetingKey; + @Setter @Getter private String targetingKey; private final Map integerAttributes; private final Map stringAttributes; - - private enum KNOWN_KEYS { - EMAIL, - FIRST_NAME, - LAST_NAME, - NAME, - IP, - TZ, - LOCALE, - COUNTRY_CODE, - ENVIRONMENT, - APPLICATION, - VERSION, - TIMESTAMP, - } - EvaluationContext() { this.targetingKey = ""; this.integerAttributes = new HashMap<>(); @@ -62,6 +50,8 @@ public class EvaluationContext { this.stringAttributes.put(key, value.format(DateTimeFormatter.ISO_ZONED_DATE_TIME)); } + // TODO: addStructure or similar. + public ZonedDateTime getDatetimeAttribute(String key) { String attr = this.stringAttributes.get(key); if (attr == null) { @@ -70,107 +60,34 @@ public class EvaluationContext { return ZonedDateTime.parse(attr, DateTimeFormatter.ISO_ZONED_DATE_TIME); } - public String getEmail() { - return this.stringAttributes.get(KNOWN_KEYS.EMAIL.toString()); - } - - public String getFirstName() { - return this.stringAttributes.get(KNOWN_KEYS.FIRST_NAME.toString()); - } - - public String getLastName() { - return this.stringAttributes.get(KNOWN_KEYS.LAST_NAME.toString()); - } - - public String getName() { - return this.stringAttributes.get(KNOWN_KEYS.NAME.toString()); - } - - public String getIp() { - return this.stringAttributes.get(KNOWN_KEYS.IP.toString()); - } - - public String getTz() { - return this.stringAttributes.get(KNOWN_KEYS.TZ.toString()); - } - - public String getLocale() { - return this.stringAttributes.get(KNOWN_KEYS.LOCALE.toString()); - } - - public String getCountryCode() { - return this.stringAttributes.get(KNOWN_KEYS.COUNTRY_CODE.toString()); - } - - public String getEnvironment() { - return this.stringAttributes.get(KNOWN_KEYS.ENVIRONMENT.toString()); - } - - public String getApplication() { - return this.stringAttributes.get(KNOWN_KEYS.APPLICATION.toString()); - } - - public String getVersion() { - return this.stringAttributes.get(KNOWN_KEYS.VERSION.toString()); - } - - public ZonedDateTime getTimestamp() { - return getDatetimeAttribute(KNOWN_KEYS.TIMESTAMP.toString()); - } - - public void setEmail(String email) { - this.stringAttributes.put(KNOWN_KEYS.EMAIL.toString(), email); - } - - public void setFirstName(String firstname) { - this.stringAttributes.put(KNOWN_KEYS.FIRST_NAME.toString(), firstname); - } - - public void setLastName(String lastname) { - this.stringAttributes.put(KNOWN_KEYS.LAST_NAME.toString(), lastname); - } - - public void setName(String name) { - this.stringAttributes.put(KNOWN_KEYS.NAME.toString(), name); - } - - public void setIp(String ip) { - this.stringAttributes.put(KNOWN_KEYS.IP.toString(), ip); - } - - public void setTz(String tz) { - this.stringAttributes.put(KNOWN_KEYS.TZ.toString(), tz); - } - - public void setLocale(String locale) { - this.stringAttributes.put(KNOWN_KEYS.LOCALE.toString(), locale); - } - - public void setCountryCode(String countryCode) { - this.stringAttributes.put(KNOWN_KEYS.COUNTRY_CODE.toString(), countryCode); - } - - public void setEnvironment(String environment) { - this.stringAttributes.put(KNOWN_KEYS.ENVIRONMENT.toString(), environment); - } - - public void setApplication(String application) { - this.stringAttributes.put(KNOWN_KEYS.APPLICATION.toString(), application); - } - - public void setVersion(String version) { - this.stringAttributes.put(KNOWN_KEYS.VERSION.toString(), version); - } - - public void setTimestamp(ZonedDateTime timestamp) { - addDatetimeAttribute(KNOWN_KEYS.TIMESTAMP.toString(), timestamp); - } - /** * 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; + EvaluationContext ec = new EvaluationContext(); + for (Map.Entry e : ctx1.integerAttributes.entrySet()) { + ec.addIntegerAttribute(e.getKey(), e.getValue()); + } + + for (Map.Entry e : ctx2.integerAttributes.entrySet()) { + ec.addIntegerAttribute(e.getKey(), e.getValue()); + } + + for (Map.Entry e : ctx1.stringAttributes.entrySet()) { + ec.addStringAttribute(e.getKey(), e.getValue()); + } + + for (Map.Entry e : ctx2.stringAttributes.entrySet()) { + ec.addStringAttribute(e.getKey(), e.getValue()); + } + if (ctx1.getTargetingKey() != null) { + ec.setTargetingKey(ctx1.getTargetingKey()); + } + + if (ctx2.getTargetingKey() != null) { + ec.setTargetingKey(ctx2.getTargetingKey()); + } + + return ec; } } diff --git a/lib/src/main/java/dev/openfeature/javasdk/Hook.java b/lib/src/main/java/dev/openfeature/javasdk/Hook.java index 932c9591..4976c5e3 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/Hook.java +++ b/lib/src/main/java/dev/openfeature/javasdk/Hook.java @@ -7,7 +7,7 @@ import java.util.Optional; // TODO: interface? or abstract class? public abstract class Hook { public Optional before(HookContext ctx, ImmutableMap hints) { - return null; + return Optional.empty(); } public void after(HookContext ctx, FlagEvaluationDetails details, ImmutableMap hints) {} public void error(HookContext ctx, Exception error, 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 3773a26c..fe8a0eb2 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/HookContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/HookContext.java @@ -4,14 +4,13 @@ import lombok.Builder; import lombok.NonNull; import lombok.Value; import lombok.With; -import javax.annotation.Nullable; @Value @Builder @With public class HookContext { @NonNull String flagKey; @NonNull FlagValueType type; @NonNull T defaultValue; - @Nullable EvaluationContext ctx; + @NonNull EvaluationContext ctx; Client client; FeatureProvider provider; diff --git a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java index 4e3d8e29..238b714f 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java +++ b/lib/src/main/java/dev/openfeature/javasdk/OpenFeatureClient.java @@ -36,11 +36,14 @@ public class OpenFeatureClient implements Client { FlagEvaluationDetails evaluateFlag(FlagValueType type, String key, T defaultValue, EvaluationContext ctx, FlagEvaluationOptions options) { FeatureProvider provider = this.openfeatureApi.getProvider(); ImmutableMap hints = options.getHookHints(); + if (ctx == null) { + ctx = new EvaluationContext(); + } // merge of: API.context, client.context, invocation.context // TODO: Context transformation? - HookContext hookCtx = HookContext.from(key, type, this, null, defaultValue); + HookContext hookCtx = HookContext.from(key, type, this, ctx, defaultValue); List mergedHooks; if (options != null && options.getHooks() != null) { @@ -112,7 +115,7 @@ public class OpenFeatureClient implements Client { EvaluationContext ctx = hookCtx.getCtx(); for (Hook hook : Lists.reverse(hooks)) { Optional newCtx = hook.before(hookCtx, hints); - if (newCtx.isPresent()) { + if (newCtx != null && newCtx.isPresent()) { ctx = EvaluationContext.merge(ctx, newCtx.get()); hookCtx = hookCtx.withCtx(ctx); } @@ -137,7 +140,7 @@ public class OpenFeatureClient implements Client { @Override public FlagEvaluationDetails getBooleanDetails(String key, Boolean defaultValue) { - return getBooleanDetails(key, defaultValue, null); + return getBooleanDetails(key, defaultValue, new EvaluationContext()); } @Override @@ -197,7 +200,7 @@ public class OpenFeatureClient implements Client { @Override public FlagEvaluationDetails getIntegerDetails(String key, Integer defaultValue) { - return getIntegerDetails(key, defaultValue, null); + return getIntegerDetails(key, defaultValue, new EvaluationContext()); } @Override diff --git a/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java b/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java index d777a74c..92d9df7a 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java +++ b/lib/src/test/java/dev/openfeature/javasdk/EvalContextTests.java @@ -6,21 +6,20 @@ import org.junit.jupiter.api.Test; import java.time.ZonedDateTime; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNull; public class EvalContextTests { - @Specification(spec="flag evaluation", number="3.1", - text="The `evaluation context` structure MUST define a required `targeting key` " + - "field of type string, identifying the subject of the flag evaluation.") - @Disabled("https://github.com/open-feature/spec/pull/60/files#r872827439") + @Specification(spec="Evaluation Context", number="3.1", + text="The `evaluation context` structure **MUST** define an optional `targeting key` field of " + + "type string, identifying the subject of the flag evaluation.") @Test void requires_targeting_key() { EvaluationContext ec = new EvaluationContext(); + ec.setTargetingKey("targeting-key"); assertEquals("targeting-key", ec.getTargetingKey()); } - @Specification(spec="flag evaluation", number="3.3", text="The evaluation context MUST support the inclusion " + - "of custom fields, having keys of type `string`, and values of " + - "type `boolean | string | number | datetime`.") + @Specification(spec="Evaluation Context", number="3.2", text="The evaluation context MUST support the inclusion of " + + "custom fields, having keys of type `string`, and " + + "values of type `boolean | string | number | datetime | structure`.") @Test void eval_context() { EvaluationContext ec = new EvaluationContext(); @@ -38,63 +37,9 @@ public class EvalContextTests { assertEquals(dt, ec.getDatetimeAttribute("dt")); } - - @Specification(spec="flag evaluation", number="3.2", text="The evaluation context MUST define the " + - "following optional fields: `email` (string), `first name` (string), `last name`(string), " + - "`name`(string), `ip`(string), `tz`(string), `locale`(string), `country code` (string), " + - "`timestamp`(date), `environment`(string), `application`(string), and `version`(string).") - @Test void mandated_fields() { - EvaluationContext ec = new EvaluationContext(); - - assertNull(ec.getEmail()); - ec.setEmail("Test"); - assertEquals("Test", ec.getEmail()); - - assertNull(ec.getFirstName()); - ec.setFirstName("Test"); - assertEquals("Test", ec.getFirstName()); - - assertNull(ec.getLastName()); - ec.setLastName("Test"); - assertEquals("Test", ec.getLastName()); - - assertNull(ec.getName()); - ec.setName("Test"); - assertEquals("Test", ec.getName()); - - assertNull(ec.getIp()); - ec.setIp("Test"); - assertEquals("Test", ec.getIp()); - - assertNull(ec.getTz()); - ec.setTz("Test"); - assertEquals("Test", ec.getTz()); - - assertNull(ec.getLocale()); - ec.setLocale("Test"); - assertEquals("Test", ec.getLocale()); - - assertNull(ec.getCountryCode()); - ec.setCountryCode("Test"); - assertEquals("Test", ec.getCountryCode()); - - assertNull(ec.getTimestamp()); - ZonedDateTime dt = ZonedDateTime.now(); - ec.setTimestamp(dt); - assertEquals(dt, ec.getTimestamp()); - - assertNull(ec.getEnvironment()); - ec.setEnvironment("Test"); - assertEquals("Test", ec.getEnvironment()); - - - assertNull(ec.getApplication()); - ec.setApplication("Test"); - assertEquals("Test", ec.getApplication()); - - - assertNull(ec.getVersion()); - ec.setVersion("Test"); - assertEquals("Test", ec.getVersion()); - } + @Specification(spec="Evaluation Context", number="3.2", text="The evaluation context MUST support the inclusion of " + + "custom fields, having keys of type `string`, and " + + "values of type `boolean | string | number | datetime | structure`.") + @Disabled("Structure support") + @Test void eval_context__structure() {} } diff --git a/lib/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTests.java b/lib/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTests.java index cc0b541f..641af25c 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTests.java +++ b/lib/src/test/java/dev/openfeature/javasdk/FlagEvaluationSpecTests.java @@ -1,6 +1,5 @@ package dev.openfeature.javasdk; -import dev.openfeature.javasdk.*; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -132,6 +131,7 @@ public class FlagEvaluationSpecTests { "unhandled error, etc) the reason field in the evaluation details SHOULD indicate an error.") @Disabled @Test void detail_flags() { + // TODO: Add tests re: detail functions. throw new NotImplementedException(); } diff --git a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java index 8190b9a6..65e1293a 100644 --- a/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java +++ b/lib/src/test/java/dev/openfeature/javasdk/HookSpecTests.java @@ -5,6 +5,7 @@ import lombok.SneakyThrows; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.InOrder; import java.util.ArrayList; @@ -396,7 +397,66 @@ public class HookSpecTests { } @Specification(spec="hooks", number="3.3", text="Any `EvaluationContext` returned from a `before` hook **MUST** be passed to subsequent `before` hooks (via `HookContext`).") + @Test void beforeContextUpdated() { + EvaluationContext ctx = new EvaluationContext(); + Hook hook = mock(Hook.class); + when(hook.before(any(), any())).thenReturn(Optional.of(ctx)); + Hook hook2 = mock(Hook.class); + when(hook.before(any(), any())).thenReturn(Optional.empty()); + InOrder order = inOrder(hook, hook2); + + + + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + api.setProvider(new NoOpProvider()); + Client client = api.getClient(); + client.getBooleanValue("key", false, new EvaluationContext(), + FlagEvaluationOptions.builder() + .hook(hook2) + .hook(hook) + .build()); + + order.verify(hook).before(any(), any()); + ArgumentCaptor captor = ArgumentCaptor.forClass(HookContext.class); + order.verify(hook2).before(captor.capture(), any()); + + HookContext hc = captor.getValue(); + assertEquals(hc.getCtx(), ctx); + + } @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.") + @Test void mergeHappensCorrectly() { + EvaluationContext hookCtx = new EvaluationContext(); + hookCtx.addStringAttribute("test", "broken"); + hookCtx.addStringAttribute("another", "exists"); + + EvaluationContext invocationCtx = new EvaluationContext(); + invocationCtx.addStringAttribute("test", "works"); + + Hook hook = mock(Hook.class); + when(hook.before(any(), any())).thenReturn(Optional.of(hookCtx)); + + FeatureProvider provider = mock(FeatureProvider.class); + when(provider.getBooleanEvaluation(any(),any(),any(),any())).thenReturn(ProviderEvaluation.builder() + .value(true) + .build()); + + OpenFeatureAPI api = OpenFeatureAPI.getInstance(); + api.setProvider(provider); + Client client = api.getClient(); + client.getBooleanValue("key", false, invocationCtx, + FlagEvaluationOptions.builder() + .hook(hook) + .build()); + + ArgumentCaptor captor = ArgumentCaptor.forClass(EvaluationContext.class); + verify(provider).getBooleanEvaluation(any(), any(), captor.capture(), any()); + EvaluationContext ec = captor.getValue(); + assertEquals("works", ec.getStringAttribute("test")); + assertEquals("exists", ec.getStringAttribute("another")); + } + + @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() {} From 49e37496e5568bc7f55fd4dd18b2235da14de9a2 Mon Sep 17 00:00:00 2001 From: Justin Abrahms Date: Wed, 25 May 2022 21:16:04 -0700 Subject: [PATCH 4/4] Have PMD quieten down about unserializable things which aren't marked as serializable. --- lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java index 7b4792ff..74d28a0e 100644 --- a/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java +++ b/lib/src/main/java/dev/openfeature/javasdk/EvaluationContext.java @@ -11,6 +11,7 @@ import java.util.HashMap; import java.util.Map; @ToString @EqualsAndHashCode +@SuppressWarnings("PMD.BeanMembersShouldSerialize") public class EvaluationContext { @Setter @Getter private String targetingKey; private final Map integerAttributes;