From bdbcd99f57937e16fc7c72ba550b13f3b751bde1 Mon Sep 17 00:00:00 2001 From: Todd Baert Date: Mon, 8 Dec 2025 17:39:42 -0500 Subject: [PATCH] fix: context serialization with revert This reverts commit 6f3a30a9aa10ce2a057ff52504e232ee52893425. Signed-off-by: Todd Baert --- .../dev/openfeature/sdk/ImmutableContext.java | 46 +---------- .../sdk/LayeredEvaluationContext.java | 80 +++++++------------ .../openfeature/sdk/ImmutableContextTest.java | 44 +++------- .../sdk/LayeredEvaluationContextTest.java | 32 -------- 4 files changed, 43 insertions(+), 159 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java index e4b53e2d9..35f28d4f4 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java +++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java @@ -5,6 +5,7 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Function; +import lombok.EqualsAndHashCode; import lombok.ToString; import lombok.experimental.Delegate; @@ -16,6 +17,7 @@ * not be modified after instantiation. */ @ToString +@EqualsAndHashCode @SuppressWarnings("PMD.BeanMembersShouldSerialize") public final class ImmutableContext implements EvaluationContext { @@ -24,9 +26,6 @@ public final class ImmutableContext implements EvaluationContext { @Delegate(excludes = DelegateExclusions.class) private final ImmutableStructure structure; - // Lazily computed hash code, safe because this class is immutable. - private volatile Integer cachedHashCode; - /** * Create an immutable context with an empty targeting_key and attributes * provided. @@ -97,47 +96,6 @@ public EvaluationContext merge(EvaluationContext overridingContext) { return new ImmutableContext(attributes); } - /** - * Equality for EvaluationContext implementations is defined in terms of their resolved - * attribute maps. Two contexts are considered equal if their {@link #asMap()} representations - * contain the same key/value pairs, regardless of how the context was constructed or layered. - * - * @param o the object to compare with this context - * @return true if the other object is an EvaluationContext whose resolved attributes match - */ - @Override - public boolean equals(Object o) { - if (this == o) { - return true; - } - if (!(o instanceof EvaluationContext)) { - return false; - } - EvaluationContext that = (EvaluationContext) o; - return this.asUnmodifiableMap().equals(that.asUnmodifiableMap()); - } - - /** - * Computes a hash code consistent with {@link #equals(Object)}. Since this context is immutable, - * the hash code is lazily computed once from its resolved attribute map and then cached. - * - * @return the cached hash code derived from this context's attribute map - */ - @Override - public int hashCode() { - Integer result = cachedHashCode; - if (result == null) { - synchronized (this) { - result = cachedHashCode; - if (result == null) { - result = asUnmodifiableMap().hashCode(); - cachedHashCode = result; - } - } - } - return result; - } - @SuppressWarnings("all") private static class DelegateExclusions { @ExcludeFromGeneratedCoverageReport diff --git a/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java b/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java index 1179ab4ad..bdd81f8c3 100644 --- a/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java +++ b/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java @@ -21,9 +21,6 @@ public class LayeredEvaluationContext implements EvaluationContext { private ArrayList hookContexts; private String targetingKey; private Set keySet = null; - // Lazily computed resolved attribute map for this layered context. - // This must be invalidated whenever the underlying layers change. - private Map cachedMap; /** * Constructor for LayeredEvaluationContext. @@ -177,20 +174,15 @@ public Value getValue(String key) { return getFromContext(apiContext, key); } - private Map getResolvedMap() { - if (cachedMap != null) { - return cachedMap; - } - + @Override + public Map asMap() { if (keySet != null && keySet.isEmpty()) { - cachedMap = Collections.emptyMap(); - return cachedMap; + return new HashMap<>(0); } HashMap map; if (keySet != null) { - // use helper to size the map based on expected entries - map = HashMapUtils.forEntries(keySet.size()); + map = new HashMap<>(keySet.size()); } else { map = new HashMap<>(); } @@ -213,15 +205,7 @@ private Map getResolvedMap() { map.putAll(hookContext.asMap()); } } - - cachedMap = Collections.unmodifiableMap(map); - return cachedMap; - } - - @Override - public Map asMap() { - // Return a defensive copy so callers can't mutate our cached map. - return new HashMap<>(getResolvedMap()); + return map; } @Override @@ -230,48 +214,41 @@ public Map asUnmodifiableMap() { return Collections.emptyMap(); } - return getResolvedMap(); + return Collections.unmodifiableMap(asMap()); } @Override public Map asObjectMap() { - // Build the object map directly from the resolved attribute map, - // so this stays consistent with equals/hashCode and asMap(). - Map resolved = getResolvedMap(); - if (resolved.isEmpty()) { + if (keySet != null && keySet.isEmpty()) { return new HashMap<>(0); } - HashMap map = HashMapUtils.forEntries(resolved.size()); - for (Map.Entry entry : resolved.entrySet()) { - Value value = entry.getValue(); - // Value is responsible for exposing the underlying Java representation. - map.put(entry.getKey(), value == null ? null : value.asObject()); + HashMap map; + if (keySet != null) { + map = new HashMap<>(keySet.size()); + } else { + map = new HashMap<>(); } - return map; - } - @Override - public boolean equals(Object o) { - if (this == o) { - return true; + if (apiContext != null) { + map.putAll(apiContext.asObjectMap()); } - if (!(o instanceof EvaluationContext)) { - return false; + if (transactionContext != null) { + map.putAll(transactionContext.asObjectMap()); } - - EvaluationContext that = (EvaluationContext) o; - - if (that instanceof LayeredEvaluationContext) { - return this.getResolvedMap().equals(((LayeredEvaluationContext) that).getResolvedMap()); + if (clientContext != null) { + map.putAll(clientContext.asObjectMap()); } - - return this.getResolvedMap().equals(that.asUnmodifiableMap()); - } - - @Override - public int hashCode() { - return getResolvedMap().hashCode(); + if (invocationContext != null) { + map.putAll(invocationContext.asObjectMap()); + } + if (hookContexts != null) { + for (int i = 0; i < hookContexts.size(); i++) { + EvaluationContext hookContext = hookContexts.get(i); + map.putAll(hookContext.asObjectMap()); + } + } + return map; } void putHookContext(EvaluationContext context) { @@ -288,6 +265,5 @@ void putHookContext(EvaluationContext context) { } this.hookContexts.add(context); this.keySet = null; - this.cachedMap = null; } } diff --git a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java index 624f9b42a..0b8a44d0d 100644 --- a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java +++ b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java @@ -4,13 +4,10 @@ import static org.junit.jupiter.api.Assertions.assertArrayEquals; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; -import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; -import java.util.HashSet; import java.util.Map; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; @@ -53,7 +50,7 @@ void shouldChangeTargetingKeyFromOverridingContext() { assertEquals("overriding_key", merge.getTargetingKey()); } - @DisplayName("targeting key should not be changed from the overriding context if missing") + @DisplayName("targeting key should not changed from the overriding context if missing") @Test void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() { HashMap attributes = new HashMap<>(); @@ -69,7 +66,7 @@ void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() { @Test void missingTargetingKeyShould() { EvaluationContext ctx = new ImmutableContext(); - assertNull(ctx.getTargetingKey()); + assertEquals(null, ctx.getTargetingKey()); } @DisplayName("Merge should retain all the attributes from the existing context when overriding context is null") @@ -148,26 +145,10 @@ void mergeShouldObtainKeysFromOverridingContextWhenExistingContextIsEmpty() { EvaluationContext ctx = new ImmutableContext(); EvaluationContext overriding = new ImmutableContext(attributes); EvaluationContext merge = ctx.merge(overriding); - assertEquals(new HashSet<>(Arrays.asList("key1", "key2")), merge.keySet()); + assertEquals(new java.util.HashSet<>(java.util.Arrays.asList("key1", "key2")), merge.keySet()); } - @DisplayName("Two ImmutableContext objects with identical attributes are considered equal") - @Test - void testImmutableContextEquality() { - Map map1 = new HashMap<>(); - map1.put("key", new Value("value")); - - Map map2 = new HashMap<>(); - map2.put("key", new Value("value")); - - ImmutableContext a = new ImmutableContext(null, map1); - ImmutableContext b = new ImmutableContext(null, map2); - - assertEquals(a, b); - assertEquals(a.hashCode(), b.hashCode()); - } - - @DisplayName("Two different ImmutableContext objects with different contents are not considered equal") + @DisplayName("Two different MutableContext objects with the different contents are not considered equal") @Test void unequalImmutableContextsAreNotEqual() { final Map attributes = new HashMap<>(); @@ -180,16 +161,17 @@ void unequalImmutableContextsAreNotEqual() { assertNotEquals(ctx, ctx2); } - @DisplayName("ImmutableContext hashCode is stable across multiple invocations") + @DisplayName("Two different MutableContext objects with the same content are considered equal") @Test - void immutableContextHashCodeIsStable() { - Map map = new HashMap<>(); - map.put("key", new Value("value")); + void equalImmutableContextsAreEqual() { + final Map attributes = new HashMap<>(); + attributes.put("key1", new Value("val1")); + final ImmutableContext ctx = new ImmutableContext(attributes); - ImmutableContext ctx = new ImmutableContext(null, map); + final Map attributes2 = new HashMap<>(); + attributes2.put("key1", new Value("val1")); + final ImmutableContext ctx2 = new ImmutableContext(attributes2); - int first = ctx.hashCode(); - int second = ctx.hashCode(); - assertEquals(first, second); + assertEquals(ctx, ctx2); } } diff --git a/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java b/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java index 511241939..7eecd9abd 100644 --- a/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java +++ b/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java @@ -397,37 +397,5 @@ void mergesCorrectlyWhenOtherHasNoTargetingKey() { merged.asMap()); assertEquals(invocationContext.getTargetingKey(), merged.getTargetingKey()); } - - @Test - void testLayeredContextEquality() { - Map baseMap = Map.of("k", new Value("v")); - Map layerMap = Map.of("x", new Value("y")); - - EvaluationContext base = new MutableContext(null, baseMap); - EvaluationContext layer = new MutableContext(null, layerMap); - - LayeredEvaluationContext l1 = new LayeredEvaluationContext(base, layer, null, null); - LayeredEvaluationContext l2 = new LayeredEvaluationContext(base, layer, null, null); - - assertEquals(l1, l2); - assertEquals(l1.hashCode(), l2.hashCode()); - } - - @Test - void testMixedContextEquality() { - Map map = Map.of("foo", new Value("bar")); - - EvaluationContext base = new MutableContext(null, map); - LayeredEvaluationContext layered = new LayeredEvaluationContext(null, null, null, base); - - // Equality from the layered context's perspective (map-based equality) - assertEquals(layered, base); - - // Resolved maps should be identical - assertEquals(base.asMap(), layered.asMap()); - - // Layered's hashCode must be consistent with its resolved attribute map - assertEquals(base.asMap().hashCode(), layered.hashCode()); - } } }