Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
toddbaert merged 1 commit into from
Dec 8, 2025
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 2 additions & 44 deletions src/main/java/dev/openfeature/sdk/ImmutableContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -16,6 +17,7 @@
* not be modified after instantiation.
*/
@ToString
@EqualsAndHashCode
@SuppressWarnings("PMD.BeanMembersShouldSerialize")
public final class ImmutableContext implements EvaluationContext {

Expand All @@ -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.
Expand Down Expand Up @@ -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
Expand Down
80 changes: 28 additions & 52 deletions src/main/java/dev/openfeature/sdk/LayeredEvaluationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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<>();
}
Expand All @@ -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
Expand All @@ -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) {
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The equals() and hashCode() methods have been removed from this class. This reverts to Object's identity-based equality, which is inconsistent with other EvaluationContext implementations like ImmutableContext and MutableContext that provide value-based equality. This is a significant behavioral change that could break consumers relying on value comparison (e.g., in collections).

If this was an unintended side-effect of removing the caching logic, please consider re-implementing equals() and hashCode() to restore value semantics, for example by delegating to asMap(). Without this, LayeredEvaluationContext will not behave as expected in HashMaps or other collections that rely on equals.

Expand All @@ -288,6 +265,5 @@ void putHookContext(EvaluationContext context) {
}
this.hookContexts.add(context);
this.keySet = null;
this.cachedMap = null;
}
}
44 changes: 13 additions & 31 deletions src/test/java/dev/openfeature/sdk/ImmutableContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<>();
Expand All @@ -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")
Expand Down Expand Up @@ -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")
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The display name for this test seems to have a copy-paste error, as it refers to MutableContext in a test for ImmutableContext. This should be corrected for clarity in test reports.

Suggested change
@DisplayName("Two different MutableContext objects with the different contents are not considered equal")
@DisplayName("Two different ImmutableContext objects with different contents are not considered equal")

@Test
void unequalImmutableContextsAreNotEqual() {
final Map<String, Value> attributes = new HashMap<>();
Expand All @@ -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")
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Dec 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The display name for this test incorrectly refers to MutableContext instead of ImmutableContext. This should be corrected for clarity.

Suggested change
@DisplayName("Two different MutableContext objects with the same content are considered equal")
@DisplayName("Two ImmutableContext 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);
}
}
32 changes: 0 additions & 32 deletions src/test/java/dev/openfeature/sdk/LayeredEvaluationContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}
Loading