Skip to content

fix(nullfilter): filters out attributes with null values #214

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
merged 5 commits into from
Sep 19, 2018
Merged

fix(nullfilter): filters out attributes with null values #214

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
062b536
fix(nullfilter): filters out attributes with null values
mnoman09 Sep 11, 2018
1a8c5c5
Added checks if attribute value is String, Integer, Double, Boolean o…
mnoman09 Sep 12, 2018
a76fc66
* Added Unit test to verify string double integer and boolean are get…
mnoman09 Sep 13, 2018
b573492
* Updated Comment on eventFactory.BuildAttributeList Filter condition
mnoman09 Sep 17, 2018
49da93d
Changed schema link.
msohailhussain Sep 19, 2018
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
12 changes: 12 additions & 0 deletions core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,18 @@ private List<Attribute> buildAttributeList(ProjectConfig projectConfig, Map<Stri
List<Attribute> attributesList = new ArrayList<Attribute>();

for (Map.Entry<String, ?> entry : attributes.entrySet()) {
// Filter down to the types of values we're allowed to track.
// Don't allow Longs, BigIntegers, or BigDecimals - they /can/ theoretically be serialized as JSON numbers
// but may take on values that can't be faithfully parsed by the backend.
// https://developers.optimizely.com/x/events/api/#Attribute
if (entry.getValue() == null ||
!((entry.getValue() instanceof String) ||
(entry.getValue() instanceof Integer) ||
(entry.getValue() instanceof Double) ||
(entry.getValue() instanceof Boolean))) {
continue;
}

String attributeId = projectConfig.getAttributeId(projectConfig, entry.getKey());
if(attributeId == null) {
continue;
Expand Down
204 changes: 196 additions & 8 deletions core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,18 +40,15 @@

import javax.annotation.Nullable;
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.math.BigDecimal;
import java.math.BigInteger;
import java.util.*;

import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV2;
import static com.optimizely.ab.config.ProjectConfigTestUtils.validProjectConfigV4;
import static com.optimizely.ab.config.ValidProjectConfigV4.*;
import static junit.framework.Assert.assertNotNull;
import static junit.framework.Assert.assertNotSame;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.Matchers.closeTo;
import static org.hamcrest.Matchers.containsInAnyOrder;
Expand All @@ -60,6 +57,7 @@
import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assume.assumeTrue;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.never;
import static org.mockito.Mockito.spy;
Expand Down Expand Up @@ -249,6 +247,196 @@ public void createImpressionEventIgnoresUnknownAttributes() throws Exception {
}
}

/**
* Verify that passing through an list value attribute causes that attribute to be ignored, rather than
* causing an exception to be thrown and passing only the valid attributes.
*/
@Test
public void createConversionEventIgnoresInvalidAndAcceptsValidAttributes() {
assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString()));

Bucketer mockBucketAlgorithm = mock(Bucketer.class);
EventType eventType = validProjectConfig.getEventTypes().get(0);

List<Experiment> allExperiments = validProjectConfig.getExperiments();

// Bucket to the first variation for all experiments. However, only a subset of the experiments will actually
// call the bucket function.
for (Experiment experiment : allExperiments) {
when(mockBucketAlgorithm.bucket(experiment, userId))
.thenReturn(experiment.getVariations().get(0));
}
Attribute attribute1 = validProjectConfig.getAttributes().get(0);
Attribute attribute2 = validProjectConfig.getAttributes().get(1);
Attribute doubleAttribute = validProjectConfig.getAttributes().get(5);
Attribute integerAttribute = validProjectConfig.getAttributes().get(4);
Attribute boolAttribute = validProjectConfig.getAttributes().get(3);

BigInteger bigInteger = new BigInteger("12323");
BigDecimal bigDecimal = new BigDecimal("123");
double validDoubleAttribute = 13.1;
int validIntegerAttribute = 12;
boolean validBoolAttribute = true;

Map<String, Object> eventTagMap = new HashMap<>();
eventTagMap.put("boolean_param", false);
eventTagMap.put("string_param", "123");

HashMap<String, Object> attributes = new HashMap<>();
attributes.put(attribute1.getKey(), bigInteger);
attributes.put(attribute2.getKey(), bigDecimal);
attributes.put(doubleAttribute.getKey(), validDoubleAttribute);
attributes.put(integerAttribute.getKey(), validIntegerAttribute);
attributes.put(boolAttribute.getKey(), validBoolAttribute);

DecisionService decisionService = new DecisionService(
mockBucketAlgorithm,
mock(ErrorHandler.class),
validProjectConfig,
mock(UserProfileService.class)
);
Map<Experiment, Variation> experimentVariationMap = createExperimentVariationMap(
validProjectConfig,
decisionService,
eventType.getKey(),
userId,
attributes);
LogEvent conversionEvent = factory.createConversionEvent(
validProjectConfig,
experimentVariationMap,
userId,
eventType.getId(),
eventType.getKey(),
attributes,
eventTagMap);

EventBatch impression = gson.fromJson(conversionEvent.getBody(), EventBatch.class);

//Check valid attributes are getting passed.
assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getKey(), boolAttribute.getKey());
assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getValue(), validBoolAttribute);

assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getKey(), doubleAttribute.getKey());
assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getValue(), validDoubleAttribute);

assertEquals(impression.getVisitors().get(0).getAttributes().get(2).getKey(), integerAttribute.getKey());
assertEquals((int) ((double) impression.getVisitors().get(0).getAttributes().get(2).getValue()), validIntegerAttribute);

// verify that no Feature is created for attribute.getKey() -> invalidAttribute
for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) {
assertNotSame(feature.getKey(), attribute1.getKey());
assertNotSame(feature.getValue(), bigInteger);
assertNotSame(feature.getKey(), attribute2.getKey());
assertNotSame(feature.getValue(), bigDecimal);
}
}

/**
* Verify that passing through an list value attribute causes that attribute to be ignored, rather than
* causing an exception to be thrown.
*/
@Test
public void createImpressionEventIgnoresInvalidAttributes() {
assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString()));
// use the "valid" project config and its associated experiment, variation, and attributes
ProjectConfig projectConfig = validProjectConfig;
Experiment activatedExperiment = projectConfig.getExperiments().get(0);
Variation bucketedVariation = activatedExperiment.getVariations().get(0);
Attribute attribute1 = validProjectConfig.getAttributes().get(0);
Attribute attribute2 = validProjectConfig.getAttributes().get(1);
BigInteger bigInteger = new BigInteger("12323");
BigDecimal bigDecimal = new BigDecimal("123");

HashMap<String, Object> attributes = new HashMap<>();
attributes.put(attribute1.getKey(), bigInteger);
attributes.put(attribute2.getKey(), bigDecimal);

LogEvent impressionEvent =
factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId",
attributes);

EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class);

// verify that no Feature is created for attribute.getKey() -> invalidAttribute
for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) {
assertNotSame(feature.getKey(), attribute1.getKey());
assertNotSame(feature.getValue(), bigInteger);
assertNotSame(feature.getKey(), attribute2.getKey());
assertNotSame(feature.getValue(), bigDecimal);
}
}

/**
* Verify that Integer, Decimal, Bool and String variables are allowed to pass.
*/
@Test
public void createImpressionEventWithIntegerDecimalBoolAndStringAttributes() {
assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString()));
// use the "valid" project config and its associated experiment, variation, and attributes
ProjectConfig projectConfig = validProjectConfig;
Experiment activatedExperiment = projectConfig.getExperiments().get(0);
Variation bucketedVariation = activatedExperiment.getVariations().get(0);
Attribute doubleAttribute = validProjectConfig.getAttributes().get(5);
Attribute integerAttribute = validProjectConfig.getAttributes().get(4);
Attribute boolAttribute = validProjectConfig.getAttributes().get(3);
Attribute stringAttribute = validProjectConfig.getAttributes().get(0);
double validDoubleAttribute = 13.1;
int validIntegerAttribute = 12;
boolean validBoolAttribute = true;
String validStringAttribute = "grayfindor";

HashMap<String, Object> attributes = new HashMap<>();
attributes.put(doubleAttribute.getKey(), validDoubleAttribute);
attributes.put(integerAttribute.getKey(), validIntegerAttribute);
attributes.put(boolAttribute.getKey(), validBoolAttribute);
attributes.put(stringAttribute.getKey(), validStringAttribute);

LogEvent impressionEvent =
factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId",
attributes);

EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class);


assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getKey(), boolAttribute.getKey());
assertEquals(impression.getVisitors().get(0).getAttributes().get(0).getValue(), validBoolAttribute);

assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getKey(), doubleAttribute.getKey());
assertEquals(impression.getVisitors().get(0).getAttributes().get(1).getValue(), validDoubleAttribute);

assertEquals(impression.getVisitors().get(0).getAttributes().get(2).getKey(), integerAttribute.getKey());
assertEquals((int) ((double) impression.getVisitors().get(0).getAttributes().get(2).getValue()), validIntegerAttribute);

assertEquals(impression.getVisitors().get(0).getAttributes().get(3).getKey(), stringAttribute.getKey());
assertEquals(impression.getVisitors().get(0).getAttributes().get(3).getValue(), validStringAttribute);

}

/**
* Verify that passing through an null value attribute causes that attribute to be ignored, rather than
* causing an exception to be thrown.
*/
@Test
public void createImpressionEventIgnoresNullAttributes() {
// use the "valid" project config and its associated experiment, variation, and attributes
ProjectConfig projectConfig = validProjectConfig;
Experiment activatedExperiment = projectConfig.getExperiments().get(0);
Variation bucketedVariation = activatedExperiment.getVariations().get(0);
Attribute attribute = validProjectConfig.getAttributes().get(0);

LogEvent impressionEvent =
factory.createImpressionEvent(projectConfig, activatedExperiment, bucketedVariation, "userId",
Collections.singletonMap(attribute.getKey(), null));

EventBatch impression = gson.fromJson(impressionEvent.getBody(), EventBatch.class);

// verify that no Feature is created for attribute.getKey() -> null
for (com.optimizely.ab.event.internal.payload.Attribute feature : impression.getVisitors().get(0).getAttributes()) {
assertNotSame(feature.getKey(), attribute.getKey());
assertNotSame(feature.getValue(), null);
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we test attribute selection for conversion events too?

BTW I think we could combine createImpressionEventIgnoresInvalidAttributes, createImpressionEventWithIntegerDecimalBoolAndStringAttributes , createImpressionEventIgnoresNullAttributes - that'd reduce complexity while also confirming that we merely ignore invalid attributes rather than dropping all attributes when one or two are invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets leave these Unit Tests. I added one more unit test which is making ConversionEvent and in that unit test we are passing valid and invalid attributes in same attributes map, we are verifying that only valid attributes are getting passed and others are getting ignored. I think that will cover the condition you are asking to verify. So is that ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, that works too. Thanks!

/**
* Verify that supplying {@link EventFactory} with a custom client engine and client version results in impression
* events being sent with the overriden values.
Expand Down Expand Up @@ -946,7 +1134,7 @@ public static Map<Experiment, Variation> createExperimentVariationMap(ProjectCon
DecisionService decisionService,
String eventName,
String userId,
@Nullable Map<String, String> attributes) {
@Nullable Map<String, ?> attributes) {

List<Experiment> eventExperiments = projectConfig.getExperimentsForEventKey(eventName);
Map<Experiment, Variation> experimentVariationMap = new HashMap<Experiment, Variation>(eventExperiments.size());
Expand Down