fix: context serialization missing props (with revert) (#1768)

fix: context serialization with revert

This reverts commit 6f3a30a.

Signed-off-by: Todd Baert <todd.baert@dynatrace.com>

Author: toddbaert

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

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

@@ -21,9 +21,6 @@ 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.
@@ -177,20 +174,15 @@ public Value getValue(String key) {
         return getFromContext(apiContext, key);
     }
 
-    private Map<String, Value> getResolvedMap() {
-        if (cachedMap != null) {
-            return cachedMap;
-        }
-
+    @Override
+    public Map<String, Value> asMap() {
         if (keySet != null && keySet.isEmpty()) {
-            cachedMap = Collections.emptyMap();
-            return cachedMap;
+            return new HashMap<>(0);
         }
 
         HashMap<String, Value> 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<String, Value> getResolvedMap() {
                 map.putAll(hookContext.asMap());
             }
         }
-
-        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());
+        return map;
     }
 
     @Override
@@ -230,48 +214,41 @@ public Map<String, Value> asUnmodifiableMap() {
             return Collections.emptyMap();
         }
 
-        return getResolvedMap();
+        return Collections.unmodifiableMap(asMap());
     }
 
     @Override
     public Map<String, Object> asObjectMap() {
-        // Build the object map directly from the resolved attribute map,
-        // so this stays consistent with equals/hashCode and asMap().
-        Map<String, Value> resolved = getResolvedMap();
-        if (resolved.isEmpty()) {
+        if (keySet != null && keySet.isEmpty()) {
             return new HashMap<>(0);
         }
 
-        HashMap<String, Object> map = HashMapUtils.forEntries(resolved.size());
-        for (Map.Entry<String, Value> 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<String, Object> 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;
     }
 }

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<String, Value> 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<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")
+    @DisplayName("Two different MutableContext objects with the different contents are not considered equal")
     @Test
     void unequalImmutableContextsAreNotEqual() {
         final Map<String, Value> 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<String, Value> map = new HashMap<>();
-        map.put("key", new Value("value"));
+    void equalImmutableContextsAreEqual() {
+        final Map<String, Value> attributes = new HashMap<>();
+        attributes.put("key1", new Value("val1"));
+        final ImmutableContext ctx = new ImmutableContext(attributes);
 
-        ImmutableContext ctx = new ImmutableContext(null, map);
+        final Map<String, Value> 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);
     }
 }

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<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());
-        }
     }
 }