Skip to content
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.

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

fix: targeting key sometimes missing in rule context #676

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -161,7 +161,7 @@ private <ValT, ReqT extends Message, ResT extends Message> ProviderEvaluation<Va
// build the gRPC request
Message req = request.newBuilderForType()
.setField(getFieldDescriptor(request, FLAG_KEY_FIELD), key)
.setField(getFieldDescriptor(request, CONTEXT_FIELD), this.convertContext(ctx))
.setField(getFieldDescriptor(request, CONTEXT_FIELD), convertContext(ctx))
.build();

final Message response;
Expand Down Expand Up @@ -216,7 +216,12 @@ private static Value convertObjectResponse(Struct protobuf) {
* Recursively convert the Evaluation context to a protobuf structure.
*/
private static Struct convertContext(EvaluationContext ctx) {
return convertMap(ctx.asMap()).getStructValue();
Map<String, Value> ctxMap = ctx.asMap();
// asMap() does not provide explicitly set targeting key (ex:- new ImmutableContext("TargetingKey") ).
// Hence, we add this explicitly here for targeting rule processing.
ctxMap.put("targetingKey", new Value(ctx.getTargetingKey()));

return convertMap(ctxMap).getStructValue();
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
package dev.openfeature.contrib.providers.flagd.resolver.process.targeting;

import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
import java.time.Instant;

import dev.openfeature.sdk.EvaluationContext;
import io.github.jamsesso.jsonlogic.JsonLogic;
import io.github.jamsesso.jsonlogic.JsonLogicException;
import lombok.Getter;

import java.time.Instant;
import java.util.HashMap;
import java.util.Map;

/**
* Targeting operator wraps JsonLogic handlers and expose a simple API for
* external layers.
Expand Down Expand Up @@ -46,49 +45,57 @@ public Object apply(final String flagKey, final String targetingRule, final Eval
long unixTimestamp = Instant.now().getEpochSecond();
flagdProperties.put(TIME_STAMP, unixTimestamp);

final Map<String, Object> valueMap = ctx.asObjectMap();
valueMap.put(FLAGD_PROPS_KEY, flagdProperties);
final Map<String, Object> targetingCtxData = ctx.asObjectMap();

// asObjectMap() does not provide explicitly set targeting key (ex:- new ImmutableContext("TargetingKey") ).
// Hence, we add this explicitly here for targeting rule processing.
targetingCtxData.put(TARGET_KEY, ctx.getTargetingKey());
targetingCtxData.put(FLAGD_PROPS_KEY, flagdProperties);

try {
return jsonLogicHandler.apply(targetingRule, valueMap);
return jsonLogicHandler.apply(targetingRule, targetingCtxData);
} catch (JsonLogicException e) {
throw new TargetingRuleException("Error evaluating json logic", e);
}
}

/**
* A utility class to extract well-known properties such as flag key, targeting key and timestamp from json logic
* evaluation context data for further processing at evaluators.
*/
@Getter
static class FlagProperties {
private final Object flagKey;
private final Object timestamp;
private final String targetingKey;
private Object flagKey = null;
private Object timestamp = null;
private String targetingKey = null;

FlagProperties(Object from) {
if (from instanceof Map) {
Map<?, ?> dataMap = (Map<?, ?>) from;

this.flagKey = extractSubPropertyFromFlagd(dataMap, FLAG_KEY);
this.timestamp = extractSubPropertyFromFlagd(dataMap, TIME_STAMP);

final Object targetKey = dataMap.get(TARGET_KEY);

if (targetKey instanceof String) {
targetingKey = (String) targetKey;
} else {
targetingKey = null;
}

} else {
flagKey = null;
timestamp = null;
targetingKey = null;
if (!(from instanceof Map)) {
return;
}

final Map<?, ?> dataMap = (Map<?, ?>) from;
final Object targetKey = dataMap.get(TARGET_KEY);
if (targetKey instanceof String) {
targetingKey = (String) targetKey;
}

final Map<?, ?> flagdPropertyMap = flagdPropertyMap(dataMap);
if (flagdPropertyMap == null) {
return;
}

this.flagKey = flagdPropertyMap.get(FLAG_KEY);
this.timestamp = flagdPropertyMap.get(TIME_STAMP);
}

private static Object extractSubPropertyFromFlagd(Map<?, ?> dataMap, String propertyName) {
return Optional.ofNullable(dataMap.get(FLAGD_PROPS_KEY))
.filter(flagdProps -> flagdProps instanceof Map)
.map(flagdProps -> ((Map<?, ?>) flagdProps).get(propertyName))
.orElse(null);
}
private static Map<?, ?> flagdPropertyMap(Map<?, ?> dataMap) {
Object o = dataMap.get(FLAGD_PROPS_KEY);
if (o instanceof Map) {
return (Map<?, ?>) o;
}

return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import io.grpc.Deadline;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Test;
import org.mockito.ArgumentMatcher;
import org.mockito.MockedStatic;

import java.lang.reflect.Field;
Expand Down Expand Up @@ -303,7 +304,6 @@ void resolvers_should_not_cache_responses_if_event_stream_not_alive() {

@Test
void context_is_parsed_and_passed_to_grpc_service() {

final String BOOLEAN_ATTR_KEY = "bool-attr";
final String INT_ATTR_KEY = "int-attr";
final String STRING_ATTR_KEY = "string-attr";
Expand All @@ -313,9 +313,9 @@ void context_is_parsed_and_passed_to_grpc_service() {
final String STRUCT_ATTR_INNER_KEY = "struct-inner-key";

final Boolean BOOLEAN_ATTR_VALUE = true;
final Integer INT_ATTR_VALUE = 1;
final int INT_ATTR_VALUE = 1;
final String STRING_ATTR_VALUE = "str";
final Double DOUBLE_ATTR_VALUE = 0.5d;
final double DOUBLE_ATTR_VALUE = 0.5d;
final List<Value> LIST_ATTR_VALUE = new ArrayList<Value>() {
{
add(new Value(1));
Expand All @@ -335,23 +335,30 @@ void context_is_parsed_and_passed_to_grpc_service() {
when(serviceBlockingStubMock.withDeadlineAfter(anyLong(), any(TimeUnit.class)))
.thenReturn(serviceBlockingStubMock);
when(serviceBlockingStubMock.resolveBoolean(argThat(
x -> STRING_ATTR_VALUE.equals(x.getContext().getFieldsMap().get(STRING_ATTR_KEY).getStringValue())
&& INT_ATTR_VALUE == x.getContext().getFieldsMap().get(INT_ATTR_KEY).getNumberValue()
&& DOUBLE_ATTR_VALUE == x.getContext().getFieldsMap().get(DOUBLE_ATTR_KEY).getNumberValue()
&& LIST_ATTR_VALUE.get(0).asInteger() == x.getContext().getFieldsMap()
.get(LIST_ATTR_KEY).getListValue().getValuesList().get(0).getNumberValue()
&& x.getContext().getFieldsMap().get(BOOLEAN_ATTR_KEY).getBoolValue()
&& STRUCT_ATTR_INNER_VALUE.equals(x.getContext().getFieldsMap()
.get(STRUCT_ATTR_KEY).getStructValue().getFieldsMap().get(STRUCT_ATTR_INNER_KEY)
.getStringValue()))))
.thenReturn(booleanResponse);
x -> {
final Struct struct = x.getContext();
final Map<String, com.google.protobuf.Value> valueMap = struct.getFieldsMap();

return STRING_ATTR_VALUE.equals(valueMap.get(STRING_ATTR_KEY).getStringValue())
&& INT_ATTR_VALUE == valueMap.get(INT_ATTR_KEY).getNumberValue()
&& DOUBLE_ATTR_VALUE == valueMap.get(DOUBLE_ATTR_KEY).getNumberValue()
&& valueMap.get(BOOLEAN_ATTR_KEY).getBoolValue()
&& "MY_TARGETING_KEY".equals(valueMap.get("targetingKey").getStringValue())
&& LIST_ATTR_VALUE.get(0).asInteger() ==
valueMap.get(LIST_ATTR_KEY).getListValue().getValuesList().get(0).getNumberValue()
&& STRUCT_ATTR_INNER_VALUE.equals(
valueMap.get(STRUCT_ATTR_KEY).getStructValue().getFieldsMap()
.get(STRUCT_ATTR_INNER_KEY).getStringValue());
}
))
).thenReturn(booleanResponse);

GrpcConnector grpc = mock(GrpcConnector.class);
when(grpc.getResolver()).thenReturn(serviceBlockingStubMock);

OpenFeatureAPI.getInstance().setProvider(createProvider(grpc));

MutableContext context = new MutableContext();
final MutableContext context = new MutableContext("MY_TARGETING_KEY");
context.add(BOOLEAN_ATTR_KEY, BOOLEAN_ATTR_VALUE);
context.add(INT_ATTR_KEY, INT_ATTR_VALUE);
context.add(DOUBLE_ATTR_KEY, DOUBLE_ATTR_VALUE);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.FLAG_WIH_IF_IN_TARGET;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.FLAG_WIH_INVALID_TARGET;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.FLAG_WIH_SHORTHAND_TARGETING;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.FLAG_WITH_TARGETING_KEY;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.INT_FLAG;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.OBJECT_FLAG;
import static dev.openfeature.contrib.providers.flagd.resolver.process.MockFlags.VARIANT_MISMATCH_FLAG;
Expand Down Expand Up @@ -333,6 +334,25 @@ public void targetingUnmatchedEvaluationFlag() throws Exception {
assertEquals(Reason.DEFAULT.toString(), providerEvaluation.getReason());
}

@Test
public void explicitTargetingKeyHandling() throws NoSuchFieldException, IllegalAccessException {
// given
final Map<String, FeatureFlag> flagMap = new HashMap<>();
flagMap.put("stringFlag", FLAG_WITH_TARGETING_KEY);

InProcessResolver inProcessResolver = getInProcessResolverWth(new MockStorage(flagMap), providerState -> {
});

// when
ProviderEvaluation<String> providerEvaluation =
inProcessResolver.stringEvaluation("stringFlag", "loop", new MutableContext("xyz"));

// then
assertEquals("binetAlg", providerEvaluation.getValue());
assertEquals("binet", providerEvaluation.getVariant());
assertEquals(Reason.TARGETING_MATCH.toString(), providerEvaluation.getReason());
}

@Test
public void targetingErrorEvaluationFlag() throws Exception {
// given
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ public class MockFlags {
static final FeatureFlag FLAG_WIH_IF_IN_TARGET = new FeatureFlag("ENABLED", "loop", stringVariants,
"{\"if\":[{\"in\":[\"@faas.com\",{\"var\":[\"email\"]}]},\"binet\",null]}");

static final FeatureFlag FLAG_WITH_TARGETING_KEY = new FeatureFlag("ENABLED", "loop", stringVariants,
"{\"if\":[{\"==\":[{\"var\":\"targetingKey\"},\"xyz\"]},\"binet\",null]}");

// flag with incorrect targeting rule
static final FeatureFlag FLAG_WIH_INVALID_TARGET = new FeatureFlag("ENABLED", "loop", stringVariants,
"{if this, then that}");
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package dev.openfeature.contrib.providers.flagd.resolver.process.targeting;

import static dev.openfeature.contrib.providers.flagd.resolver.process.targeting.Operator.TARGET_KEY;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;

Expand Down Expand Up @@ -64,13 +65,15 @@ void testFlagPropertiesConstructor() {
flagdProperties.put(Operator.TIME_STAMP, 1634000000L);

Map<String, Object> dataMap = new HashMap<>();
dataMap.put(TARGET_KEY, "myTargetingKey");
dataMap.put(Operator.FLAGD_PROPS_KEY, flagdProperties);

// When
Operator.FlagProperties flagProperties = new Operator.FlagProperties(dataMap);

// Then
assertEquals("some-key", flagProperties.getFlagKey());
assertEquals("myTargetingKey", flagProperties.getTargetingKey());
assertEquals(1634000000L, flagProperties.getTimestamp());
}

Expand Down
Loading