From 6b7f8dccf2c5f5d9bdf865d092022854d52f7e0c Mon Sep 17 00:00:00 2001 From: vishalup29 Date: Sat, 29 Nov 2025 03:08:41 -0600 Subject: [PATCH] Issue #1754 Implement equals/hashCode for EvaluationContext and add tests. Signed-off-by: vishalup29 --- .../dev/openfeature/sdk/ImmutableContext.java | 46 ++++++++++- .../sdk/LayeredEvaluationContext.java | 80 ++++++++++++------- .../openfeature/sdk/ImmutableContextTest.java | 44 +++++++--- .../sdk/LayeredEvaluationContextTest.java | 32 ++++++++ 4 files changed, 159 insertions(+), 43 deletions(-) diff --git a/src/main/java/dev/openfeature/sdk/ImmutableContext.java b/src/main/java/dev/openfeature/sdk/ImmutableContext.java index 35f28d4f4..c9d998fec 100644 --- a/src/main/java/dev/openfeature/sdk/ImmutableContext.java +++ b/src/main/java/dev/openfeature/sdk/ImmutableContext.java @@ -5,7 +5,6 @@ import java.util.HashMap; import java.util.Map; import java.util.function.Function; -import lombok.EqualsAndHashCode; import lombok.ToString; import lombok.experimental.Delegate; @@ -17,7 +16,6 @@ * not be modified after instantiation. */ @ToString -@EqualsAndHashCode @SuppressWarnings("PMD.BeanMembersShouldSerialize") public final class ImmutableContext implements EvaluationContext { @@ -26,6 +24,9 @@ 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. @@ -96,6 +97,47 @@ 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.asMap().equals(that.asMap()); + } + + /** + * 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 = asMap().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 bdd81f8c3..266091e7f 100644 --- a/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java +++ b/src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java @@ -21,6 +21,9 @@ 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. @@ -174,15 +177,20 @@ public Value getValue(String key) { return getFromContext(apiContext, key); } - @Override - public Map asMap() { + private Map getResolvedMap() { + if (cachedMap != null) { + return cachedMap; + } + if (keySet != null && keySet.isEmpty()) { - return new HashMap<>(0); + cachedMap = Collections.emptyMap(); + return cachedMap; } HashMap map; if (keySet != null) { - map = new HashMap<>(keySet.size()); + // use helper to size the map based on expected entries + map = HashMapUtils.forEntries(keySet.size()); } else { map = new HashMap<>(); } @@ -205,7 +213,15 @@ public Map asMap() { map.putAll(hookContext.asMap()); } } - return map; + + 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()); } @Override @@ -214,41 +230,48 @@ public Map asUnmodifiableMap() { return Collections.emptyMap(); } - return Collections.unmodifiableMap(asMap()); + return getResolvedMap(); } @Override public Map asObjectMap() { - if (keySet != null && keySet.isEmpty()) { + // 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()) { return new HashMap<>(0); } - HashMap map; - if (keySet != null) { - map = new HashMap<>(keySet.size()); - } else { - map = new HashMap<>(); + 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()); } + return map; + } - if (apiContext != null) { - map.putAll(apiContext.asObjectMap()); - } - if (transactionContext != null) { - map.putAll(transactionContext.asObjectMap()); - } - if (clientContext != null) { - map.putAll(clientContext.asObjectMap()); + @Override + public boolean equals(Object o) { + if (this == o) { + return true; } - if (invocationContext != null) { - map.putAll(invocationContext.asObjectMap()); + if (!(o instanceof EvaluationContext)) { + return false; } - if (hookContexts != null) { - for (int i = 0; i < hookContexts.size(); i++) { - EvaluationContext hookContext = hookContexts.get(i); - map.putAll(hookContext.asObjectMap()); - } + + EvaluationContext that = (EvaluationContext) o; + + if (that instanceof LayeredEvaluationContext) { + return this.getResolvedMap().equals(((LayeredEvaluationContext) that).getResolvedMap()); } - return map; + + return this.getResolvedMap().equals(that.asMap()); + } + + @Override + public int hashCode() { + return getResolvedMap().hashCode(); } void putHookContext(EvaluationContext context) { @@ -265,5 +288,6 @@ 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 0b8a44d0d..624f9b42a 100644 --- a/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java +++ b/src/test/java/dev/openfeature/sdk/ImmutableContextTest.java @@ -4,10 +4,13 @@ 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; @@ -50,7 +53,7 @@ void shouldChangeTargetingKeyFromOverridingContext() { assertEquals("overriding_key", merge.getTargetingKey()); } - @DisplayName("targeting key should not changed from the overriding context if missing") + @DisplayName("targeting key should not be changed from the overriding context if missing") @Test void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() { HashMap attributes = new HashMap<>(); @@ -66,7 +69,7 @@ void shouldRetainTargetingKeyWhenOverridingContextTargetingKeyValueIsEmpty() { @Test void missingTargetingKeyShould() { EvaluationContext ctx = new ImmutableContext(); - assertEquals(null, ctx.getTargetingKey()); + assertNull(ctx.getTargetingKey()); } @DisplayName("Merge should retain all the attributes from the existing context when overriding context is null") @@ -145,10 +148,26 @@ void mergeShouldObtainKeysFromOverridingContextWhenExistingContextIsEmpty() { EvaluationContext ctx = new ImmutableContext(); EvaluationContext overriding = new ImmutableContext(attributes); EvaluationContext merge = ctx.merge(overriding); - assertEquals(new java.util.HashSet<>(java.util.Arrays.asList("key1", "key2")), merge.keySet()); + assertEquals(new HashSet<>(Arrays.asList("key1", "key2")), merge.keySet()); } - @DisplayName("Two different MutableContext objects with the different contents are not considered equal") + @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") @Test void unequalImmutableContextsAreNotEqual() { final Map attributes = new HashMap<>(); @@ -161,17 +180,16 @@ void unequalImmutableContextsAreNotEqual() { assertNotEquals(ctx, ctx2); } - @DisplayName("Two different MutableContext objects with the same content are considered equal") + @DisplayName("ImmutableContext hashCode is stable across multiple invocations") @Test - void equalImmutableContextsAreEqual() { - final Map attributes = new HashMap<>(); - attributes.put("key1", new Value("val1")); - final ImmutableContext ctx = new ImmutableContext(attributes); + void immutableContextHashCodeIsStable() { + Map map = new HashMap<>(); + map.put("key", new Value("value")); - final Map attributes2 = new HashMap<>(); - attributes2.put("key1", new Value("val1")); - final ImmutableContext ctx2 = new ImmutableContext(attributes2); + ImmutableContext ctx = new ImmutableContext(null, map); - assertEquals(ctx, ctx2); + int first = ctx.hashCode(); + int second = ctx.hashCode(); + assertEquals(first, second); } } diff --git a/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java b/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java index 7eecd9abd..511241939 100644 --- a/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java +++ b/src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java @@ -397,5 +397,37 @@ 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()); + } } }