fix: .equals of equivalent context false (#1768)

This reverts commit 4cb39a4.

Co-authored-by: Vishalup29 <67001661+Vishalup29@users.noreply.github.com>
Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Author: 2 people

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.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

src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java

@@ -21,6 +21,9 @@ public class LayeredEvaluationContext implements EvaluationContext {
     private ArrayList<EvaluationContext> hookContexts;
     private String targetingKey;
     private Set<String> keySet = null;
+    // Lazily computed resolved attribute map for this layered context.
+    // This must be invalidated whenever the underlying layers change.
+    private Map<String, Value> cachedMap;
 
     /**
      * Constructor for LayeredEvaluationContext.
@@ -174,15 +177,20 @@ public Value getValue(String key) {
         return getFromContext(apiContext, key);
     }
 
-    @Override
-    public Map<String, Value> asMap() {
+    private Map<String, Value> getResolvedMap() {
+        if (cachedMap != null) {
+            return cachedMap;
+        }
+
         if (keySet != null && keySet.isEmpty()) {
-            return new HashMap<>(0);
+            cachedMap = Collections.emptyMap();
+            return cachedMap;
         }
 
         HashMap<String, Value> 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<String, Value> asMap() {
                 map.putAll(hookContext.asMap());
             }
         }
-        return map;
+
+        cachedMap = Collections.unmodifiableMap(map);
+        return cachedMap;
+    }
+
+    @Override
+    public Map<String, Value> asMap() {
+        // Return a defensive copy so callers can't mutate our cached map.
+        return new HashMap<>(getResolvedMap());
     }
 
     @Override
@@ -214,7 +230,7 @@ public Map<String, Value> asUnmodifiableMap() {
             return Collections.emptyMap();
         }
 
-        return Collections.unmodifiableMap(asMap());
+        return getResolvedMap();
     }
 
     @Override
@@ -225,7 +241,8 @@ public Map<String, Object> asObjectMap() {
 
         HashMap<String, Object> 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<>();
         }
@@ -248,9 +265,33 @@ public Map<String, Object> asObjectMap() {
                 map.putAll(hookContext.asObjectMap());
             }
         }
+
         return map;
     }
 
+    @Override
+    public boolean equals(Object o) {
+        if (this == o) {
+            return true;
+        }
+        if (!(o instanceof EvaluationContext)) {
+            return false;
+        }
+
+        EvaluationContext that = (EvaluationContext) o;
+
+        if (that instanceof LayeredEvaluationContext) {
+            return this.getResolvedMap().equals(((LayeredEvaluationContext) that).getResolvedMap());
+        }
+
+        return this.getResolvedMap().equals(that.asUnmodifiableMap());
+    }
+
+    @Override
+    public int hashCode() {
+        return getResolvedMap().hashCode();
+    }
+
     void putHookContext(EvaluationContext context) {
         if (context == null || context.isEmpty()) {
             return;
@@ -265,5 +306,6 @@ void putHookContext(EvaluationContext context) {
         }
         this.hookContexts.add(context);
         this.keySet = null;
+        this.cachedMap = null;
     }
 }

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<String, Value> 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<String, Value> map1 = new HashMap<>();
+        map1.put("key", new Value("value"));
+
+        Map<String, Value> 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<String, Value> 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<String, Value> attributes = new HashMap<>();
-        attributes.put("key1", new Value("val1"));
-        final ImmutableContext ctx = new ImmutableContext(attributes);
+    void immutableContextHashCodeIsStable() {
+        Map<String, Value> map = new HashMap<>();
+        map.put("key", new Value("value"));
 
-        final Map<String, Value> 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);
     }
 }

src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java

@@ -1,6 +1,11 @@
 package dev.openfeature.sdk;
 
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import java.util.Map;
 import java.util.Set;
@@ -257,6 +262,75 @@ void creatingMapWithCachedNonEmptyKeySetWorks() {
             assertNotNull(layeredContext.keySet());
             assertEquals(apiContext.asObjectMap(), layeredContext.asObjectMap());
         }
+
+        @Test
+        void nestedContextsAreUnwrappedCorrectly() {
+            var innerApiContext = new ImmutableContext(Map.of("inner", new Value("api")));
+            var outerApiContext = new ImmutableContext(Map.of("outer", new Value(innerApiContext)));
+
+            var innerClientContext = new ImmutableContext(Map.of("inner", new Value("client")));
+            var outerClientContext = new ImmutableContext(Map.of("outer", new Value(innerClientContext)));
+            var layeredContext = new LayeredEvaluationContext(outerApiContext, null, outerClientContext, null);
+
+            var objectMap = layeredContext.asObjectMap();
+
+            assertEquals(Map.of("outer", Map.of("inner", "client")), objectMap);
+        }
+
+        @Test
+        void nestedStructuresInContextsAreUnwrappedCorrectly() {
+            var innerApiStructure = new ImmutableStructure(Map.of("inner", new Value("api")));
+            var outerApiContext = new ImmutableContext(Map.of("outer", new Value(innerApiStructure)));
+
+            var innerClientStructure = new ImmutableStructure(Map.of("inner", new Value("client")));
+            var outerClientContext = new ImmutableContext(Map.of("outer", new Value(innerClientStructure)));
+            var layeredContext = new LayeredEvaluationContext(outerApiContext, null, outerClientContext, null);
+
+            var objectMap = layeredContext.asObjectMap();
+
+            assertEquals(Map.of("outer", Map.of("inner", "client")), objectMap);
+        }
+
+        @Test
+        void nestedHookContextsAreUnwrappedCorrectly() {
+            var innerApiStructure = new ImmutableStructure(Map.of("inner", new Value("api")));
+            var outerApiContext = new ImmutableContext(Map.of("outer", new Value(innerApiStructure)));
+
+            var innerClientStructure = new ImmutableStructure(Map.of("inner", new Value("client")));
+            var outerClientContext = new ImmutableContext(Map.of("outer", new Value(innerClientStructure)));
+            var layeredContext = new LayeredEvaluationContext(outerApiContext, null, outerClientContext, null);
+
+            var innerHookStructure = new ImmutableStructure(Map.of("inner", new Value("hook")));
+            var outerHookContext = new ImmutableContext(Map.of("outer", new Value(innerHookStructure)));
+
+            layeredContext.putHookContext(outerHookContext);
+
+            var objectMap = layeredContext.asObjectMap();
+
+            assertEquals(Map.of("outer", Map.of("inner", "hook")), objectMap);
+        }
+
+        @Test
+        void objectMapIsMutable() {
+            LayeredEvaluationContext layeredContext =
+                    new LayeredEvaluationContext(apiContext, transactionContext, clientContext, invocationContext);
+
+            var objectMap = layeredContext.asObjectMap();
+            assertDoesNotThrow(() -> objectMap.put("a", "b"));
+            assertEquals("b", objectMap.get("a"));
+        }
+
+        @Test
+        void mutatingObjectMapHasNoSideEffects() {
+            LayeredEvaluationContext layeredContext =
+                    new LayeredEvaluationContext(apiContext, transactionContext, clientContext, invocationContext);
+
+            var objectMap1 = layeredContext.asObjectMap();
+            objectMap1.put("a", "b");
+
+            var objectMap2 = layeredContext.asObjectMap();
+            assertNull(objectMap2.get("a"));
+        }
     }
 
     @Nested
@@ -397,5 +471,37 @@ void mergesCorrectlyWhenOtherHasNoTargetingKey() {
                     merged.asMap());
             assertEquals(invocationContext.getTargetingKey(), merged.getTargetingKey());
         }
+
+        @Test
+        void testLayeredContextEquality() {
+            Map<String, Value> baseMap = Map.of("k", new Value("v"));
+            Map<String, Value> 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<String, Value> 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());
+        }
     }
 }