diff --git a/core-api/src/main/java/com/optimizely/ab/Optimizely.java b/core-api/src/main/java/com/optimizely/ab/Optimizely.java index 989cb3fe5..274ec2525 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -631,10 +631,16 @@ private LiveVariable getLiveVariableOrThrow(ProjectConfig projectConfig, String * * @param projectConfig the current project config * @param attributes the attributes map to validate and potentially filter - * @return the filtered attributes map (containing only attributes that are present in the project config) - * + * @return the filtered attributes map (containing only attributes that are present in the project config) or an + * empty map if a null attributes object is passed in */ - private Map filterAttributes(ProjectConfig projectConfig, Map attributes) { + private Map filterAttributes(@Nonnull ProjectConfig projectConfig, + @Nonnull Map attributes) { + if (attributes == null) { + logger.warn("Attributes is null when non-null was expected. Defaulting to an empty attributes map."); + return Collections.emptyMap(); + } + List unknownAttributes = null; Map attributeKeyMapping = projectConfig.getAttributeKeyMapping(); diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTestV3.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTestV3.java index de8212376..1ebc82641 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTestV3.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTestV3.java @@ -322,7 +322,6 @@ public void activateWithUnknownExperimentKeyAndRaiseExceptionErrorHandler() thro * Verify that {@link Optimizely#activate(String, String, Map)} passes through attributes. */ @Test - @SuppressWarnings("unchecked") public void activateWithAttributes() throws Exception { String datafile = validConfigJsonV3(); ProjectConfig projectConfig = validProjectConfigV3(); @@ -379,7 +378,6 @@ public void activateWithAttributes() throws Exception { * In this case, the activate call should remove the unknown attribute from the given map. */ @Test - @SuppressWarnings("unchecked") public void activateWithUnknownAttribute() throws Exception { String datafile = validConfigJsonV3(); ProjectConfig projectConfig = validProjectConfigV3(); @@ -437,6 +435,117 @@ public void activateWithUnknownAttribute() throws Exception { verify(mockEventHandler).dispatchEvent(logEventToDispatch); } + /** + * Verify that {@link Optimizely#activate(String, String, Map)} ignores null attributes. + */ + @Test + @SuppressFBWarnings( + value="NP_NONNULL_PARAM_VIOLATION", + justification="testing nullness contract violation") + public void activateWithNullAttributes() throws Exception { + String datafile = noAudienceProjectConfigJsonV3(); + ProjectConfig projectConfig = noAudienceProjectConfigV3(); + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + + // setup a mock event builder to return expected impression params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(projectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createImpressionEvent(eq(projectConfig), eq(activatedExperiment), eq(bucketedVariation), + eq("userId"), eq(Collections.emptyMap()), + isNull(String.class))) + .thenReturn(logEventToDispatch); + + when(mockBucketer.bucket(activatedExperiment, "userId")) + .thenReturn(bucketedVariation); + + // activate the experiment + Map attributes = null; + Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), "userId", attributes); + + logbackVerifier.expectMessage(Level.WARN, "Attributes is null when non-null was expected. Defaulting to an empty attributes map."); + + // verify that the bucketing algorithm was called correctly + verify(mockBucketer).bucket(activatedExperiment, "userId"); + assertThat(actualVariation, is(bucketedVariation)); + + // setup the attribute map captor (so we can verify its content) + ArgumentCaptor attributeCaptor = ArgumentCaptor.forClass(Map.class); + verify(mockEventBuilder).createImpressionEvent(eq(projectConfig), eq(activatedExperiment), + eq(bucketedVariation), eq("userId"), attributeCaptor.capture(), + isNull(String.class)); + + Map actualValue = attributeCaptor.getValue(); + assertThat(actualValue, is(Collections.emptyMap())); + + // verify that dispatchEvent was called with the correct LogEvent object + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + } + + /** + * Verify that {@link Optimizely#activate(String, String, Map)} gracefully handles null attribute values. + */ + @Test + public void activateWithNullAttributeValues() throws Exception { + String datafile = validConfigJsonV3(); + ProjectConfig projectConfig = validProjectConfigV3(); + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Variation bucketedVariation = activatedExperiment.getVariations().get(0); + Attribute attribute = projectConfig.getAttributes().get(0); + + // setup a mock event builder to return expected impression params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(projectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createImpressionEvent(eq(projectConfig), eq(activatedExperiment), eq(bucketedVariation), + eq("userId"), anyMapOf(String.class, String.class), + isNull(String.class))) + .thenReturn(logEventToDispatch); + + when(mockBucketer.bucket(activatedExperiment, "userId")) + .thenReturn(bucketedVariation); + + // activate the experiment + Map attributes = new HashMap(); + attributes.put(attribute.getKey(), null); + Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), "userId", attributes); + + // verify that the bucketing algorithm was called correctly + verify(mockBucketer).bucket(activatedExperiment, "userId"); + assertThat(actualVariation, is(bucketedVariation)); + + // setup the attribute map captor (so we can verify its content) + ArgumentCaptor attributeCaptor = ArgumentCaptor.forClass(Map.class); + verify(mockEventBuilder).createImpressionEvent(eq(projectConfig), eq(activatedExperiment), + eq(bucketedVariation), eq("userId"), attributeCaptor.capture(), + isNull(String.class)); + + Map actualValue = attributeCaptor.getValue(); + assertThat(actualValue, hasEntry(attribute.getKey(), null)); + + // verify that dispatchEvent was called with the correct LogEvent object + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + } + /** * Verify that {@link Optimizely#activate(String, String)} returns null when the experiment id corresponds to a * non-running experiment. @@ -773,7 +882,6 @@ public void trackEventWithUnknownEventKeyAndRaiseExceptionErrorHandler() throws * Verify that {@link Optimizely#track(String, String)} passes through attributes. */ @Test - @SuppressWarnings("unchecked") public void trackEventWithAttributes() throws Exception { String datafile = validConfigJsonV3(); ProjectConfig projectConfig = validProjectConfigV3(); @@ -821,6 +929,111 @@ public void trackEventWithAttributes() throws Exception { verify(mockEventHandler).dispatchEvent(logEventToDispatch); } + /** + * Verify that {@link Optimizely#track(String, String)} ignores null attributes. + */ + @Test + @SuppressFBWarnings( + value="NP_NONNULL_PARAM_VIOLATION", + justification="testing nullness contract violation") + public void trackEventWithNullAttributes() throws Exception { + String datafile = noAudienceProjectConfigJsonV3(); + ProjectConfig projectConfig = noAudienceProjectConfigV3(); + EventType eventType = projectConfig.getEventTypes().get(0); + + // setup a mock event builder to return expected conversion params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(projectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createConversionEvent(eq(projectConfig), eq(mockBucketer), eq("userId"), + eq(eventType.getId()), eq(eventType.getKey()), + eq(Collections.emptyMap()), isNull(Long.class), + isNull(String.class))) + .thenReturn(logEventToDispatch); + + logbackVerifier.expectMessage(Level.INFO, "Tracking event \"clicked_cart\" for user \"userId\"."); + logbackVerifier.expectMessage(Level.DEBUG, "Dispatching conversion event to URL test_url with params " + + testParams + " and payload \"\""); + + // call track + Map attributes = null; + optimizely.track(eventType.getKey(), "userId", attributes); + + logbackVerifier.expectMessage(Level.WARN, "Attributes is null when non-null was expected. Defaulting to an empty attributes map."); + + // setup the attribute map captor (so we can verify its content) + ArgumentCaptor attributeCaptor = ArgumentCaptor.forClass(Map.class); + + // verify that the event builder was called with the expected attributes + verify(mockEventBuilder).createConversionEvent(eq(projectConfig), eq(mockBucketer), eq("userId"), + eq(eventType.getId()), eq(eventType.getKey()), + attributeCaptor.capture(), isNull(Long.class), + isNull(String.class)); + + Map actualValue = attributeCaptor.getValue(); + assertThat(actualValue, is(Collections.emptyMap())); + + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + } + + /** + * Verify that {@link Optimizely#track(String, String)} gracefully handles null attribute values. + */ + @Test + public void trackEventWithNullAttributeValues() throws Exception { + String datafile = validConfigJsonV3(); + ProjectConfig projectConfig = validProjectConfigV3(); + EventType eventType = projectConfig.getEventTypes().get(0); + + // setup a mock event builder to return expected conversion params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(projectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createConversionEvent(eq(projectConfig), eq(mockBucketer), eq("userId"), + eq(eventType.getId()), eq(eventType.getKey()), + anyMapOf(String.class, String.class), isNull(Long.class), + isNull(String.class))) + .thenReturn(logEventToDispatch); + + logbackVerifier.expectMessage(Level.INFO, "Tracking event \"clicked_cart\" for user \"userId\"."); + logbackVerifier.expectMessage(Level.DEBUG, "Dispatching conversion event to URL test_url with params " + + testParams + " and payload \"\""); + + // call track + Map attributes = new HashMap(); + attributes.put("test", null); + optimizely.track(eventType.getKey(), "userId", attributes); + + // setup the attribute map captor (so we can verify its content) + ArgumentCaptor attributeCaptor = ArgumentCaptor.forClass(Map.class); + + // verify that the event builder was called with the expected attributes + verify(mockEventBuilder).createConversionEvent(eq(projectConfig), eq(mockBucketer), eq("userId"), + eq(eventType.getId()), eq(eventType.getKey()), + attributeCaptor.capture(), isNull(Long.class), + isNull(String.class)); + + verify(mockEventHandler).dispatchEvent(logEventToDispatch); + } + /** * Verify that {@link Optimizely#track(String, String)} handles the case where an unknown attribute * (i.e., not in the config) is passed through. @@ -828,7 +1041,6 @@ public void trackEventWithAttributes() throws Exception { * In this case, the track event call should remove the unknown attribute from the given map. */ @Test - @SuppressWarnings("unchecked") public void trackEventWithUnknownAttribute() throws Exception { String datafile = validConfigJsonV3(); ProjectConfig projectConfig = validProjectConfigV3();