Repository: open-feature/java-sdk (java)

Issue:
[Feature] Consider exposing TargetingKey with all value exporters 
Consider `ImmutableStructure.asMap()` [^1] or `ImmutableStructure. asObjectMap()` [^2] methods. These expose the attributes but they do not include the targeting key [^3]. 

This could lead to unexpected behaviors where evaluations miss targeting key if provider authors overlook the implementation details (see [^4]). In contrast, go-sdk perform a context flattening to include targeting key [^5] and js-sdk context is a record hence this issue does not exist in them.

**AC**

If agreed, we should include the targeting key in bulk exporters (ex:- ctx.asObject) as well as improve overall context handling by documentation improvements 

[^1]: https://github.com/open-feature/java-sdk/blob/v1.7.3/src/main/java/dev/openfeature/sdk/MutableStructure.java#L90
[^2]: https://github.com/open-feature/java-sdk/blob/v1.7.3/src/main/java/dev/openfeature/sdk/AbstractStructure.java#L25 
[^3]: https://github.com/open-feature/java-sdk/blob/v1.7.3/src/main/java/dev/openfeature/sdk/ImmutableContext.java#L21 
[^4]: https://github.com/open-feature/java-sdk-contrib/pull/676
[^5]: https://github.com/open-feature/go-sdk/blob/main/openfeature/client.go#L687-L695

Pull request description:
fix: improve targetingKey handling in the context
## This PR

Fixes #801. 

**What's changed?** 

I have changed how we store the targetingKey internally (in both mutable and immutable contexts). Instead of storing in a field, we now store targetingKey as a property of the delegated Structure implemementation. 

**Why** 

Both mutable and immutable contexts use a Structure implementation instance and delegate operations onto this instance. For example, `asObjectMap` and `asMap()` are exposed through this delegation. With this change, we keep the same delegation and expose targetingKey from the above-mentioned attribute exporters. 

**Impact**

There are no behaviour change other than for avoiding storing of empty string (`""`) for targetingKey.   

Patch:
diff --git a/src/main/java/dev/openfeature/sdk/EvaluationContext.java b/src/main/java/dev/openfeature/sdk/EvaluationContext.java
index 02fc240..b95ea45 100644
--- a/src/main/java/dev/openfeature/sdk/EvaluationContext.java
+++ b/src/main/java/dev/openfeature/sdk/EvaluationContext.java
@@ -6,6 +6,9 @@ package dev.openfeature.sdk;
  */
 @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 486e789..632448a 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 @@ import lombok.experimental.Delegate;
 @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 final class ImmutableContext implements EvaluationContext {
      * @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();
     }
 
     /**
@@ -67,21 +74,10 @@ public final class ImmutableContext implements EvaluationContext {
     @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()));
     }
 }
diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java
index 42fc90f..69c22b8 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 @@ import lombok.experimental.Delegate;
 @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
@@ -81,8 +85,25 @@ public class MutableContext implements EvaluationContext {
     }
 
     /**
-     * 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 class MutableContext implements EvaluationContext {
     @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);
     }
 
     /**
diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java
index 6ceaf59..f3768e9 100644
--- a/src/main/java/dev/openfeature/sdk/Structure.java
+++ b/src/main/java/dev/openfeature/sdk/Structure.java
@@ -113,9 +113,8 @@ public interface Structure {
     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()) {


Test patch:
diff --git a/src/test/java/dev/openfeature/sdk/EvalContextTest.java b/src/test/java/dev/openfeature/sdk/EvalContextTest.java
index f4cd804..c7f3aa4 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<String, Object> 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 df5784b..7cfedec 100644
--- a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
+++ b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
@@ -5,6 +5,7 @@ import org.junit.jupiter.api.Test;
 
 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 @@ class ImmutableContextTest {
         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 @@ class ImmutableContextTest {
         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 @@ class ImmutableContextTest {
         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());