fix: issue #859 (#860)

* fix: issue #859 Added failing tests Signed-off-by: Adam Roberts <[email protected]> * fix: issue #859 Fixed the logic to allow the tests to pass by copying the passed in attributes Signed-off-by: Adam Roberts <[email protected]> --------- Signed-off-by: Adam Roberts <[email protected]>
open-feature · Mar 22, 2024 · d51cacb · d51cacb
1 parent 675de14
commit d51cacb
Show file tree

Hide file tree

Showing 5 changed files with 35 additions and 5 deletions.
diff --git a/src/main/java/dev/openfeature/sdk/AbstractStructure.java b/src/main/java/dev/openfeature/sdk/AbstractStructure.java
@@ -13,7 +13,7 @@ abstract class AbstractStructure implements Structure {
     }
 
     AbstractStructure(Map<String, Value> attributes) {
-        this.attributes = attributes;
+        this.attributes = new HashMap<>(attributes);
     }
 
     /**

diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java
@@ -52,9 +52,12 @@ public ImmutableContext(Map<String, Value> attributes) {
      */
     public ImmutableContext(String targetingKey, Map<String, Value> attributes) {
         if (targetingKey != null && !targetingKey.trim().isEmpty()) {
-            attributes.put(TARGETING_KEY, new Value(targetingKey));
+            final Map<String, Value> actualAttribs = new HashMap<>(attributes);
+            actualAttribs.put(TARGETING_KEY, new Value(targetingKey));
+            this.structure = new ImmutableStructure(actualAttribs);
+        } else {
+            this.structure = new ImmutableStructure(attributes);
         }
-        this.structure = new ImmutableStructure(attributes);
     }
 
     /**

diff --git a/src/main/java/dev/openfeature/sdk/MutableContext.java b/src/main/java/dev/openfeature/sdk/MutableContext.java
@@ -42,10 +42,10 @@ public MutableContext(Map<String, Value> attributes) {
      * @param attributes   evaluation context attributes
      */
     public MutableContext(String targetingKey, Map<String, Value> attributes) {
+        this.structure = new MutableStructure(attributes);
         if (targetingKey != null && !targetingKey.trim().isEmpty()) {
-            attributes.put(TARGETING_KEY, new Value(targetingKey));
+            this.structure.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

diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
@@ -1,5 +1,7 @@
 package dev.openfeature.sdk;
 
+import java.util.Collections;
+import java.util.Map;
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 
@@ -12,6 +14,17 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 class ImmutableContextTest {
+    @DisplayName("attributes unable to allow mutation should not affect the immutable context")
+    @Test
+    void shouldNotAttemptToModifyAttributesForImmutableContext() {
+        final Map<String, Value> attributes = new HashMap<>();
+        attributes.put("key1", new Value("val1"));
+        attributes.put("key2", new Value("val2"));
+        // should check the usage of Map.of() which is a more likely use case, but that API isn't available in Java 8
+        EvaluationContext ctx = new ImmutableContext("targeting key", Collections.unmodifiableMap(attributes));
+        attributes.put("key3", new Value("val3"));
+        assertArrayEquals(new Object[]{"key1", "key2", TARGETING_KEY}, ctx.keySet().toArray());
+    }
 
     @DisplayName("attributes mutation should not affect the immutable context")
     @Test

diff --git a/src/test/java/dev/openfeature/sdk/MutableContextTest.java b/src/test/java/dev/openfeature/sdk/MutableContextTest.java
@@ -3,7 +3,9 @@
 import org.junit.jupiter.api.DisplayName;
 import org.junit.jupiter.api.Test;
 
+import java.util.Collections;
 import java.util.HashMap;
+import java.util.Map;
 
 import static dev.openfeature.sdk.EvaluationContext.TARGETING_KEY;
 import static org.junit.jupiter.api.Assertions.assertArrayEquals;
@@ -12,6 +14,18 @@
 
 class MutableContextTest {
 
+    @DisplayName("attributes unable to allow mutation should not affect the Mutable context")
+    @Test
+    void shouldNotAttemptToModifyAttributesForMutableContext() {
+        final Map<String, Value> attributes = new HashMap<>();
+        attributes.put("key1", new Value("val1"));
+        attributes.put("key2", new Value("val2"));
+        // should check the usage of Map.of() which is a more likely use case, but that API isn't available in Java 8
+        EvaluationContext ctx = new MutableContext("targeting key", Collections.unmodifiableMap(attributes));
+        attributes.put("key3", new Value("val3"));
+        assertArrayEquals(new Object[]{"key1", "key2", TARGETING_KEY}, ctx.keySet().toArray());
+    }
+
     @DisplayName("targeting key should be changed from the overriding context")
     @Test
     void shouldChangeTargetingKeyFromOverridingContext() {
-Original file line number
+Diff line change
@@ Expand Up / @@ -13,7 +13,7 @@ abstract class AbstractStructure implements Structure { @@
         }
         AbstractStructure(Map<String, Value> attributes) {
-            this.attributes = attributes;
+            this.attributes = new HashMap<>(attributes);
         }
         /**
@@ Expand Down @@