Skip to content

Commit

Permalink
fix: targeting key sometimes missing in rule context (#676)
Browse files Browse the repository at this point in the history 
Signed-off-by: Kavindu Dodanduwa <kavindudodanduwa@gmail.com>
Co-authored-by: Todd Baert <todd.baert@dynatrace.com>
  • Loading branch information
@Kavindu-Dodan @toddbaert
Kavindu-Dodan and toddbaert authored Feb 13, 2024
1 parent eb6383d commit 7407b84
Show file tree
Hide file tree
Showing 6 changed files with 96 additions and 51 deletions.
9 changes: 7 additions & 2 deletions ...agd/src/main/java/dev/openfeature/contrib/providers/flagd/resolver/grpc/GrpcResolver.java
View file
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
77 changes: 42 additions & 35 deletions ...ain/java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/Operator.java
View file
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;
}
}
}
35 changes: 21 additions & 14 deletions providers/flagd/src/test/java/dev/openfeature/contrib/providers/flagd/FlagdProviderTest.java
View file
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
20 changes: 20 additions & 0 deletions .../java/dev/openfeature/contrib/providers/flagd/resolver/process/InProcessResolverTest.java
View file
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
3 changes: 3 additions & 0 deletions ...agd/src/test/java/dev/openfeature/contrib/providers/flagd/resolver/process/MockFlags.java
View file
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
3 changes: 3 additions & 0 deletions ...java/dev/openfeature/contrib/providers/flagd/resolver/process/targeting/OperatorTest.java
View file
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

0 comments on commit 7407b84

Please sign in to comment.