From 5c76410dce2b2e8aed3e67ff4aceef5ab7dd28bd Mon Sep 17 00:00:00 2001 From: Adam Roberts Date: Thu, 21 Mar 2024 13:03:07 -0400 Subject: [PATCH 1/2] fix: issue #859 Added failing tests Signed-off-by: Adam Roberts --- .../dev/openfeature/sdk/ImmutableContextTest.java | 13 +++++++++++++ .../dev/openfeature/sdk/MutableContextTest.java | 14 ++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java index c3847b933..44a6f4790 100644 --- 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 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 index 655cc85d5..1d462b12c 100644 --- 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 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() { From 368319773f29b61b359ce947e4313e88551ac770 Mon Sep 17 00:00:00 2001 From: Adam Roberts Date: Thu, 21 Mar 2024 15:57:01 -0400 Subject: [PATCH 2/2] fix: issue #859 Fixed the logic to allow the tests to pass by copying the passed in attributes Signed-off-by: Adam Roberts --- src/main/java/dev/openfeature/sdk/AbstractStructure.java | 2 +- src/main/java/dev/openfeature/sdk/ImmutableContext.java | 7 +++++-- src/main/java/dev/openfeature/sdk/MutableContext.java | 4 ++-- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/AbstractStructure.java b/src/main/java/dev/openfeature/sdk/AbstractStructure.java index a7d7c2eae..e50fbe920 100644 --- 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 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 index 67e356290..0d02ba31a 100644 --- 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 attributes) { */ public ImmutableContext(String targetingKey, Map attributes) { if (targetingKey != null && !targetingKey.trim().isEmpty()) { - attributes.put(TARGETING_KEY, new Value(targetingKey)); + final Map 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 index 42b9ccb7a..b63f9b314 100644 --- 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 attributes) { * @param attributes evaluation context attributes */ public MutableContext(String targetingKey, Map 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