fix: MutableContext and ImmutableContext merge are made recursive (#280)

MutableContext and ImmutableContext merge are made recursive Signed-off-by: Javier Collado <[email protected]> Co-authored-by: Thiyagu GK <[email protected]> Co-authored-by: Todd Baert <[email protected]> Co-authored-by: Michael Beemer <[email protected]>
open-feature · Feb 8, 2023 · bd4e12e · bd4e12e
1 parent fe8698f
commit bd4e12e
Show file tree

Hide file tree

Showing 4 changed files with 99 additions and 10 deletions.
diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
@@ -86,10 +86,11 @@ public EvaluationContext merge(EvaluationContext overridingContext) {
         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
@@ -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 EvaluationContext merge(EvaluationContext overridingContext) {
             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
@@ -2,8 +2,11 @@
 
 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 @@ default Object convertValue(Value value) {
         }
         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;
+    }
 }
diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
@@ -8,6 +8,7 @@
 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 @@ void mergeShouldReturnAllTheValuesFromTheContextWhenOverridingContextIsNull() {
         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());
+    }
 }