From 1c18a9e534688a3eec8e7165ce81eaf76c41b812 Mon Sep 17 00:00:00 2001 From: Kavindu Dodanduwa Date: Wed, 14 Feb 2024 10:20:45 -0800 Subject: [PATCH] improve targeting key handling Signed-off-by: Kavindu Dodanduwa --- .../openfeature/sdk/EvaluationContext.java | 3 + .../dev/openfeature/sdk/ImmutableContext.java | 40 ++++----- .../dev/openfeature/sdk/MutableContext.java | 84 ++++++++++--------- .../java/dev/openfeature/sdk/Structure.java | 3 +- .../dev/openfeature/sdk/EvalContextTest.java | 10 +-- .../openfeature/sdk/ImmutableContextTest.java | 7 +- 6 files changed, 74 insertions(+), 73 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/EvaluationContext.java b/src/main/java/dev/openfeature/sdk/EvaluationContext.java index 02fc24019..b95ea454d 100644 --- a/src/main/java/dev/openfeature/sdk/EvaluationContext.java +++ b/src/main/java/dev/openfeature/sdk/EvaluationContext.java @@ -6,6 +6,9 @@ */ @SuppressWarnings("PMD.BeanMembersShouldSerialize") public interface EvaluationContext extends Structure { + + String TARGETING_KEY = "targetingKey"; + String getTargetingKey(); /** diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java index 486e789e1..632448aae 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java +++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java @@ -1,12 +1,11 @@ package dev.openfeature.sdk; -import java.util.HashMap; -import java.util.Map; - -import lombok.Getter; import lombok.ToString; import lombok.experimental.Delegate; +import java.util.HashMap; +import java.util.Map; + /** * The EvaluationContext is a container for arbitrary contextual data * that can be used as a basis for dynamic evaluation. @@ -17,8 +16,6 @@ @SuppressWarnings("PMD.BeanMembersShouldSerialize") public final class ImmutableContext implements EvaluationContext { - @Getter - private final String targetingKey; @Delegate private final Structure structure; @@ -26,7 +23,7 @@ public final class ImmutableContext implements EvaluationContext { * Create an immutable context with an empty targeting_key and attributes provided. */ public ImmutableContext() { - this("", new HashMap<>()); + this(new HashMap<>()); } /** @@ -54,8 +51,18 @@ public ImmutableContext(Map attributes) { * @param attributes evaluation context attributes */ public ImmutableContext(String targetingKey, Map attributes) { + if (targetingKey != null && !targetingKey.trim().isEmpty()) { + attributes.put(TARGETING_KEY, new Value(targetingKey)); + } this.structure = new ImmutableStructure(attributes); - this.targetingKey = targetingKey; + } + + /** + * Retrieve targetingKey from the context. + */ + @Override + public String getTargetingKey() { + return this.getValue(TARGETING_KEY).asString(); } /** @@ -67,21 +74,10 @@ public ImmutableContext(String targetingKey, Map attributes) { @Override public EvaluationContext merge(EvaluationContext overridingContext) { if (overridingContext == null) { - return new ImmutableContext(this.targetingKey, this.asMap()); + return new ImmutableContext(this.asMap()); } - String newTargetingKey = ""; - if (this.getTargetingKey() != null && !this.getTargetingKey().trim().equals("")) { - newTargetingKey = this.getTargetingKey(); - } - - if (overridingContext.getTargetingKey() != null && !overridingContext.getTargetingKey().trim().equals("")) { - newTargetingKey = overridingContext.getTargetingKey(); - } - - Map merged = this.merge(m -> new ImmutableStructure(m), - this.asMap(), - overridingContext.asMap()); - return new ImmutableContext(newTargetingKey, merged); + return new ImmutableContext( + this.merge(ImmutableStructure::new, this.asMap(), overridingContext.asMap())); } } diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java index 42fc90f58..69c22b841 100644 --- a/src/main/java/dev/openfeature/sdk/MutableContext.java +++ b/src/main/java/dev/openfeature/sdk/MutableContext.java @@ -1,19 +1,18 @@ package dev.openfeature.sdk; -import java.time.Instant; -import java.util.List; -import java.util.Map; - import lombok.EqualsAndHashCode; -import lombok.Getter; -import lombok.Setter; import lombok.ToString; import lombok.experimental.Delegate; +import java.time.Instant; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + /** * The EvaluationContext is a container for arbitrary contextual data * that can be used as a basis for dynamic evaluation. - * The MutableContext is an EvaluationContext implementation which is not threadsafe, and whose attributes can + * The MutableContext is an EvaluationContext implementation which is not threadsafe, and whose attributes can * be modified after instantiation. */ @ToString @@ -21,27 +20,32 @@ @SuppressWarnings("PMD.BeanMembersShouldSerialize") public class MutableContext implements EvaluationContext { - @Setter() @Getter private String targetingKey; @Delegate(excludes = HideDelegateAddMethods.class) private final MutableStructure structure; public MutableContext() { - this.structure = new MutableStructure(); - this.targetingKey = ""; + this(new HashMap<>()); } public MutableContext(String targetingKey) { - this(); - this.targetingKey = targetingKey; + this(targetingKey, new HashMap<>()); } public MutableContext(Map attributes) { - this.structure = new MutableStructure(attributes); - this.targetingKey = ""; + this("", attributes); } + /** + * Create a mutable context with given targetingKey and attributes provided. TargetingKey should be non-null + * and non-empty to be accepted. + * + * @param targetingKey targeting key + * @param attributes evaluation context attributes + */ public MutableContext(String targetingKey, Map attributes) { - this(attributes); - this.targetingKey = targetingKey; + if (targetingKey != null && !targetingKey.trim().isEmpty()) { + attributes.put(TARGETING_KEY, new Value(targetingKey)); + } + this.structure = new MutableStructure(attributes); } // override @Delegate methods so that we can use "add" methods and still return MutableContext, not Structure @@ -81,8 +85,25 @@ public MutableContext add(String key, List value) { } /** - * Merges this EvaluationContext objects with the second overriding the this in - * case of conflict. + * Override or set targeting key for this mutable context. Value should be non-null and non-empty to be accepted. + */ + public void setTargetingKey(String targetingKey) { + if (targetingKey != null && !targetingKey.trim().isEmpty()) { + this.add(TARGETING_KEY, targetingKey); + } + } + + + /** + * Retrieve targetingKey from the context. + */ + @Override + public String getTargetingKey() { + return this.getValue(TARGETING_KEY).asString(); + } + + /** + * Merges this EvaluationContext objects with the second overriding the in case of conflict. * * @param overridingContext overriding context * @return resulting merged context @@ -90,31 +111,12 @@ public MutableContext add(String key, List value) { @Override public EvaluationContext merge(EvaluationContext overridingContext) { if (overridingContext == null) { - return new MutableContext(this.targetingKey, this.asMap()); - } - - Map merged = this.merge(map -> new MutableStructure(map), - this.asMap(), - overridingContext.asMap()); - - String newTargetingKey = ""; - - if (this.getTargetingKey() != null && !this.getTargetingKey().trim().equals("")) { - newTargetingKey = this.getTargetingKey(); - } - - if (overridingContext.getTargetingKey() != null && !overridingContext.getTargetingKey().trim().equals("")) { - newTargetingKey = overridingContext.getTargetingKey(); - } - - EvaluationContext ec = null; - if (newTargetingKey != null && !newTargetingKey.trim().equals("")) { - ec = new MutableContext(newTargetingKey, merged); - } else { - ec = new MutableContext(merged); + return new MutableContext(this.asMap()); } - return ec; + Map merged = this.merge( + MutableStructure::new, this.asMap(), overridingContext.asMap()); + return new MutableContext(merged); } /** diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java index 6ceaf5926..f3768e958 100644 --- a/src/main/java/dev/openfeature/sdk/Structure.java +++ b/src/main/java/dev/openfeature/sdk/Structure.java @@ -113,9 +113,8 @@ default Object convertValue(Value value) { default Map merge(Function, Structure> newStructure, Map base, Map overriding) { - Map merged = new HashMap<>(); - merged.putAll(base); + final Map merged = new HashMap<>(base); for (Entry overridingEntry : overriding.entrySet()) { String key = overridingEntry.getKey(); if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) { diff --git a/src/test/java/dev/openfeature/sdk/EvalContextTest.java b/src/test/java/dev/openfeature/sdk/EvalContextTest.java index f4cd804c8..c7f3aa44d 100644 --- a/src/test/java/dev/openfeature/sdk/EvalContextTest.java +++ b/src/test/java/dev/openfeature/sdk/EvalContextTest.java @@ -1,17 +1,16 @@ package dev.openfeature.sdk; -import static org.junit.jupiter.api.Assertions.assertEquals; +import org.junit.jupiter.api.Test; import java.time.Instant; import java.time.temporal.ChronoUnit; -import java.time.temporal.TemporalUnit; import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; -import io.cucumber.java.hu.Ha; -import org.junit.jupiter.api.Test; +import static dev.openfeature.sdk.EvaluationContext.TARGETING_KEY; +import static org.junit.jupiter.api.Assertions.assertEquals; public class EvalContextTest { @Specification(number="3.1.1", @@ -184,7 +183,7 @@ public class EvalContextTest { ctx2.setTargetingKey(" "); ctxMerged = ctx1.merge(ctx2); - assertEquals(key1, ctxMerged.getTargetingKey()); + assertEquals(key2, ctxMerged.getTargetingKey()); } @Test void asObjectMap() { @@ -214,6 +213,7 @@ public class EvalContextTest { Map want = new HashMap<>(); + want.put(TARGETING_KEY, key1); want.put("stringItem", "stringValue"); want.put("boolItem", false); want.put("integerItem", 1); diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java index df5784b64..7cfedec99 100644 --- a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java +++ b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java @@ -5,6 +5,7 @@ import java.util.HashMap; +import static dev.openfeature.sdk.EvaluationContext.TARGETING_KEY; import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; @@ -20,7 +21,7 @@ void shouldCreateCopyOfAttributesForImmutableContext() { attributes.put("key2", new Value("val2")); EvaluationContext ctx = new ImmutableContext("targeting key", attributes); attributes.put("key3", new Value("val3")); - assertArrayEquals(new Object[]{"key1", "key2"}, ctx.keySet().toArray()); + assertArrayEquals(new Object[]{"key1", "key2", TARGETING_KEY}, ctx.keySet().toArray()); } @DisplayName("targeting key should be changed from the overriding context") @@ -56,7 +57,7 @@ void mergeShouldReturnAllTheValuesFromTheContextWhenOverridingContextIsNull() { EvaluationContext ctx = new ImmutableContext("targeting_key", attributes); EvaluationContext merge = ctx.merge(null); assertEquals("targeting_key", merge.getTargetingKey()); - assertArrayEquals(new Object[]{"key1", "key2"}, merge.keySet().toArray()); + assertArrayEquals(new Object[]{"key1", "key2", TARGETING_KEY}, merge.keySet().toArray()); } @DisplayName("Merge should retain subkeys from the existing context when the overriding context has the same targeting key") @@ -77,7 +78,7 @@ void mergeShouldRetainItsSubkeysWhenOverridingContextHasTheSameKey() { EvaluationContext overriding = new ImmutableContext("targeting_key", overridingAttributes); EvaluationContext merge = ctx.merge(overriding); assertEquals("targeting_key", merge.getTargetingKey()); - assertArrayEquals(new Object[]{"key1", "key2"}, merge.keySet().toArray()); + assertArrayEquals(new Object[]{"key1", "key2", TARGETING_KEY}, merge.keySet().toArray()); Value key1 = merge.getValue("key1"); assertTrue(key1.isStructure());