Skip to content

filterAttributes returns an empty if attributes parameter is null #79

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 9 commits into from
Mar 3, 2017
Merged

filterAttributes returns an empty if attributes parameter is null #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
cb97d64
`filterAttributes` returns an empty if `attributes` parameter is null
Feb 28, 2017
f6488f9
Changed doc and logging message
Feb 28, 2017
a4a0983
Add tests for `activate` and `track` with null attributes
Feb 28, 2017
8c3da76
Add test cases for attributes with null values
Feb 28, 2017
cb495e1
Update tests
Mar 1, 2017
9f9ab7e
Update tests
Mar 2, 2017
eafc650
Suppress FindBug warnings
Mar 2, 2017
9660cf5
Change copyright date and fix test
Mar 3, 2017
25f29b6
Revert copyright
Mar 3, 2017
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: 9 additions & 3 deletions core-api/src/main/java/com/optimizely/ab/Optimizely.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> filterAttributes(ProjectConfig projectConfig, Map<String, String> attributes) {
private Map<String, String> filterAttributes(@Nonnull ProjectConfig projectConfig,
@Nonnull Map<String, String> attributes) {
if (attributes == null) {
logger.warn("Attributes is null when non-null was expected. Defaulting to an empty attributes map.");
return Collections.<String, String>emptyMap();
}

List<String> unknownAttributes = null;

Map<String, Attribute> attributeKeyMapping = projectConfig.getAttributeKeyMapping();
Expand Down
220 changes: 216 additions & 4 deletions core-api/src/test/java/com/optimizely/ab/OptimizelyTestV3.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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<String, String> testParams = new HashMap<String, String>();
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.<String, String>emptyMap()),
isNull(String.class)))
.thenReturn(logEventToDispatch);

when(mockBucketer.bucket(activatedExperiment, "userId"))
.thenReturn(bucketedVariation);

// activate the experiment
Map<String, String> 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<Map> attributeCaptor = ArgumentCaptor.forClass(Map.class);
verify(mockEventBuilder).createImpressionEvent(eq(projectConfig), eq(activatedExperiment),
eq(bucketedVariation), eq("userId"), attributeCaptor.capture(),
isNull(String.class));

Map<String, String> actualValue = attributeCaptor.getValue();
assertThat(actualValue, is(Collections.<String, String>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<String, String> testParams = new HashMap<String, String>();
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<String, String> attributes = new HashMap<String, String>();
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<Map> attributeCaptor = ArgumentCaptor.forClass(Map.class);
verify(mockEventBuilder).createImpressionEvent(eq(projectConfig), eq(activatedExperiment),
eq(bucketedVariation), eq("userId"), attributeCaptor.capture(),
isNull(String.class));

Map<String, String> 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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -821,14 +929,118 @@ 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<String, String> testParams = new HashMap<String, String>();
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.<String, String>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<String, String> 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<Map> 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<String, String> actualValue = attributeCaptor.getValue();
assertThat(actualValue, is(Collections.<String, String>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<String, String> testParams = new HashMap<String, String>();
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<String, String> attributes = new HashMap<String, String>();
attributes.put("test", null);
optimizely.track(eventType.getKey(), "userId", attributes);

// setup the attribute map captor (so we can verify its content)
ArgumentCaptor<Map> 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.
*
* 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();
Expand Down