Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve targetingKey handling in the context #805

Merged
merged 1 commit into from
Feb 14, 2024
+74 −73
Merged

fix: improve targetingKey handling in the context #805

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/main/java/dev/openfeature/sdk/EvaluationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
*/
@SuppressWarnings("PMD.BeanMembersShouldSerialize")
public interface EvaluationContext extends Structure {

String TARGETING_KEY = "targetingKey";

String getTargetingKey();

/**
Expand Down
40 changes: 18 additions & 22 deletions src/main/java/dev/openfeature/sdk/ImmutableContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -17,16 +16,14 @@
@SuppressWarnings("PMD.BeanMembersShouldSerialize")
public final class ImmutableContext implements EvaluationContext {

@Getter
private final String targetingKey;
@Delegate
private final Structure structure;

/**
* Create an immutable context with an empty targeting_key and attributes provided.
*/
public ImmutableContext() {
this("", new HashMap<>());
this(new HashMap<>());
}

/**
Expand Down Expand Up @@ -54,8 +51,18 @@ public ImmutableContext(Map<String, Value> attributes) {
* @param attributes evaluation context attributes
*/
public ImmutableContext(String targetingKey, Map<String, Value> 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();
}

/**
Expand All @@ -67,21 +74,10 @@ public ImmutableContext(String targetingKey, Map<String, Value> 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<String, Value> 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()));
}
}
84 changes: 43 additions & 41 deletions src/main/java/dev/openfeature/sdk/MutableContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,47 +1,51 @@
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
@EqualsAndHashCode
@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<String, Value> 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<String, Value> 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
Expand Down Expand Up @@ -81,40 +85,38 @@ public MutableContext add(String key, List<Value> 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
*/
@Override
public EvaluationContext merge(EvaluationContext overridingContext) {
if (overridingContext == null) {
return new MutableContext(this.targetingKey, this.asMap());
}

Map<String, Value> 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<String, Value> merged = this.merge(
MutableStructure::new, this.asMap(), overridingContext.asMap());
return new MutableContext(merged);
}

/**
Expand Down
3 changes: 1 addition & 2 deletions src/main/java/dev/openfeature/sdk/Structure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,9 +113,8 @@ default Object convertValue(Value value) {
default <T extends Structure> Map<String, Value> merge(Function<Map<String, Value>, Structure> newStructure,
Map<String, Value> base,
Map<String, Value> overriding) {
Map<String, Value> merged = new HashMap<>();

merged.putAll(base);
final Map<String, Value> merged = new HashMap<>(base);
for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
String key = overridingEntry.getKey();
if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) {
Expand Down
10 changes: 5 additions & 5 deletions src/test/java/dev/openfeature/sdk/EvalContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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",
Expand Down Expand Up @@ -184,7 +183,7 @@ public class EvalContextTest {

ctx2.setTargetingKey(" ");
ctxMerged = ctx1.merge(ctx2);
assertEquals(key1, ctxMerged.getTargetingKey());
Copy link
Contributor Author

@Kavindu-Dodan Kavindu-Dodan Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only major change - we do not accept empty value or spaces for targeting key. Eaerlier this was handled in the merge stage where we filtered for null or empty values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

assertEquals(key2, ctxMerged.getTargetingKey());
}

@Test void asObjectMap() {
Expand Down Expand Up @@ -214,6 +213,7 @@ public class EvalContextTest {


Map<String, Object> want = new HashMap<>();
want.put(TARGETING_KEY, key1);
want.put("stringItem", "stringValue");
want.put("boolItem", false);
want.put("integerItem", 1);
Expand Down
7 changes: 4 additions & 3 deletions src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Expand All @@ -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());
Expand Down
Loading