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

Issue:
Evaluation context `merge` implementations should be recursive
If there is a property on this called `myKey`, and it has subkeys, and there's also a property called `myKey` on overridingContext, then the subkeys are lost, instead of merged.

This is un-intuitive and should be correct.

see: https://github.com/open-feature/java-sdk/pull/210/files#r1071367173


Pull request description:
fix: MutableContext and ImmutableContext merge are made recursive
Signed-off-by: Javier Collado <javicv@gmail.com>

<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

- Makes the evaluation context merge recursive for both MutableContext and ImmutableContext

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #270

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->
- A new test method _mergeShouldRetainItsSubkeysWhenOverridingContextHasTheSameKey_ in _dev.openfeature.sdk.ImmutableContextTest_ has been added


Patch:
diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
index 8f8692c..3fb25c2 100644
--- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java
+++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
@@ -86,10 +86,11 @@ public final class ImmutableContext implements EvaluationContext {
         if (overridingContext.getTargetingKey() != null && !overridingContext.getTargetingKey().trim().equals("")) {
             newTargetingKey = overridingContext.getTargetingKey();
         }
-        Map<String, Value> merged = new HashMap<>();
 
-        merged.putAll(this.asMap());
-        merged.putAll(overridingContext.asMap());
+        Map<String, Value> merged = this.merge(m -> new ImmutableStructure(m),
+                                               this.asMap(),
+                                               overridingContext.asMap());
+
         return new ImmutableContext(newTargetingKey, merged);
     }
 }
diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java
index b11503c..6abd74a 100644
--- a/src/main/java/dev/openfeature/sdk/MutableContext.java
+++ b/src/main/java/dev/openfeature/sdk/MutableContext.java
@@ -1,7 +1,6 @@
 package dev.openfeature.sdk;
 
 import java.time.Instant;
-import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 
@@ -92,18 +91,25 @@ public class MutableContext implements EvaluationContext {
             return new MutableContext(this.asMap());
         }
 
-        Map<String, Value> merged = new HashMap<String, Value>();
+        Map<String, Value> merged = this.merge(map -> new MutableStructure(map),
+                                               this.asMap(),
+                                               overridingContext.asMap());
 
-        merged.putAll(this.asMap());
-        merged.putAll(overridingContext.asMap());
-        EvaluationContext ec = new MutableContext(merged);
+        String newTargetingKey = "";
 
         if (this.getTargetingKey() != null && !this.getTargetingKey().trim().equals("")) {
-            ec.setTargetingKey(this.getTargetingKey());
+            newTargetingKey = this.getTargetingKey();
         }
 
         if (overridingContext.getTargetingKey() != null && !overridingContext.getTargetingKey().trim().equals("")) {
-            ec.setTargetingKey(overridingContext.getTargetingKey());
+            newTargetingKey = overridingContext.getTargetingKey();
+        }
+        
+        EvaluationContext ec = null;
+        if (newTargetingKey != null && !newTargetingKey.trim().equals("")) {
+            ec = new MutableContext(newTargetingKey, merged);
+        } else {
+            ec = new MutableContext(merged);
         }
 
         return ec;
diff --git a/src/main/java/dev/openfeature/sdk/Structure.java b/src/main/java/dev/openfeature/sdk/Structure.java
index ee75442..9d66c2b 100644
--- a/src/main/java/dev/openfeature/sdk/Structure.java
+++ b/src/main/java/dev/openfeature/sdk/Structure.java
@@ -2,8 +2,11 @@ package dev.openfeature.sdk;
 
 import dev.openfeature.sdk.exceptions.ValueNotConvertableError;
 
+import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
+import java.util.Map.Entry;
+import java.util.function.Function;
 import java.util.stream.Collectors;
 
 /**
@@ -90,4 +93,33 @@ public interface Structure {
         }
         throw new ValueNotConvertableError();
     }
+
+    /**
+     * Recursively merges the base map with the overriding map.
+     * 
+     * @param <T> Structure type
+     * @param newStructure function to create the right structure
+     * @param base base map to merge
+     * @param overriding overriding map to merge
+     * @return resulting merged map
+     */
+    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);
+        for (Entry<String, Value> overridingEntry : overriding.entrySet()) {
+            String key = overridingEntry.getKey();
+            if (overridingEntry.getValue().isStructure() && merged.containsKey(key) && merged.get(key).isStructure()) {
+                Structure mergedValue = merged.get(key).asStructure();
+                Structure overridingValue = overridingEntry.getValue().asStructure();
+                Map<String, Value> newMap = this.merge(newStructure, mergedValue.asMap(), overridingValue.asMap());
+                merged.put(key, new Value(newStructure.apply(newMap)));
+            } else {
+                merged.put(key, overridingEntry.getValue());
+            }
+        }
+        return merged;
+    }
 }


Test patch:
diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
index 4f3f6c8..437e492 100644
--- a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
+++ b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
@@ -8,6 +8,7 @@ import java.util.HashMap;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class ImmutableContextTest {
 
@@ -64,4 +65,53 @@ class ImmutableContextTest {
         assertEquals("targeting_key", merge.getTargetingKey());
         assertArrayEquals(new Object[]{"key1", "key2"}, merge.keySet().toArray());
     }
+
+    @DisplayName("Merge should retain subkeys from the existing context when the overriding context has the same targeting key")
+    @Test
+    void mergeShouldRetainItsSubkeysWhenOverridingContextHasTheSameKey() {
+        HashMap<String, Value> attributes = new HashMap<>();
+        HashMap<String, Value> overridingAttributes = new HashMap<>();
+        HashMap<String, Value> key1Attributes = new HashMap<>();
+        HashMap<String, Value> ovKey1Attributes = new HashMap<>();
+
+        key1Attributes.put("key1_1", new Value("val1_1"));
+        attributes.put("key1", new Value(new ImmutableStructure(key1Attributes)));
+        attributes.put("key2", new Value("val2"));
+        ovKey1Attributes.put("overriding_key1_1", new Value("overriding_val_1_1"));
+        overridingAttributes.put("key1", new Value(new ImmutableStructure(ovKey1Attributes)));
+        
+        EvaluationContext ctx = new ImmutableContext("targeting_key", attributes);
+        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());
+        
+        Value key1 = merge.getValue("key1");
+        assertTrue(key1.isStructure());
+        
+        Structure value = key1.asStructure();
+        assertArrayEquals(new Object[]{"key1_1","overriding_key1_1"}, value.keySet().toArray());
+    }
+    
+    @DisplayName("Merge should retain subkeys from the existing context when the overriding context doesn't have targeting key")
+    @Test
+    void mergeShouldRetainItsSubkeysWhenOverridingContextHasNoTargetingKey() {
+        HashMap<String, Value> attributes = new HashMap<>();
+        HashMap<String, Value> key1Attributes = new HashMap<>();
+
+        key1Attributes.put("key1_1", new Value("val1_1"));
+        attributes.put("key1", new Value(new ImmutableStructure(key1Attributes)));
+        attributes.put("key2", new Value("val2"));
+        
+        EvaluationContext ctx = new ImmutableContext(attributes);
+        EvaluationContext overriding = new ImmutableContext();
+        EvaluationContext merge = ctx.merge(overriding);
+        assertArrayEquals(new Object[]{"key1", "key2"}, merge.keySet().toArray());
+        
+        Value key1 = merge.getValue("key1");
+        assertTrue(key1.isStructure());
+        
+        Structure value = key1.asStructure();
+        assertArrayEquals(new Object[]{"key1_1"}, value.keySet().toArray());
+    }
 }