diff --git a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java index ba347e7af..0ea6000b0 100644 --- a/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java +++ b/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java @@ -168,6 +168,18 @@ private List buildAttributeList(ProjectConfig projectConfig, Map attributesList = new ArrayList(); for (Map.Entry 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; diff --git a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java index be170c839..f2b555526 100644 --- a/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java +++ b/core-api/src/test/java/com/optimizely/ab/event/internal/EventFactoryTest.java @@ -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; @@ -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; @@ -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 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 eventTagMap = new HashMap<>(); + eventTagMap.put("boolean_param", false); + eventTagMap.put("string_param", "123"); + + HashMap 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 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 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 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); + } + } + /** * Verify that supplying {@link EventFactory} with a custom client engine and client version results in impression * events being sent with the overriden values. @@ -946,7 +1134,7 @@ public static Map createExperimentVariationMap(ProjectCon DecisionService decisionService, String eventName, String userId, - @Nullable Map attributes) { + @Nullable Map attributes) { List eventExperiments = projectConfig.getExperimentsForEventKey(eventName); Map experimentVariationMap = new HashMap(eventExperiments.size());