From cf5dd7c0f93e8caee56cd14499ae2b685921dbbb Mon Sep 17 00:00:00 2001 From: "FOLIO3PK\\muhammadnoman" Date: Fri, 7 Sep 2018 21:10:39 +0500 Subject: [PATCH 1/2] removes null value attributes while filtering --- .../java/com/optimizely/ab/Optimizely.java | 24 +++++++------- .../com/optimizely/ab/OptimizelyTest.java | 31 +++++++++++++------ 2 files changed, 33 insertions(+), 22 deletions(-) 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 e4070e681..c3c01478e 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -823,7 +823,7 @@ public UserProfileService getUserProfileService() { * {@link ProjectConfig}. * * @param projectConfig the current project config - * @param attributes the attributes map to validate and potentially filter. Attributes which starts with reserved key + * @param attributes the attributes map to validate and potentially filter. Attributes which starts with reserved key or has null values * {@link ProjectConfig#RESERVED_ATTRIBUTE_PREFIX} are kept. * @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 @@ -836,25 +836,25 @@ public UserProfileService getUserProfileService() { } // List of attribute keys - List unknownAttributes = null; + List unknownAndNullValueAttributes = null; Map attributeKeyMapping = projectConfig.getAttributeKeyMapping(); for (Map.Entry attribute : attributes.entrySet()) { - if (!attributeKeyMapping.containsKey(attribute.getKey()) && - !attribute.getKey().startsWith(ProjectConfig.RESERVED_ATTRIBUTE_PREFIX)) { - if (unknownAttributes == null) { - unknownAttributes = new ArrayList(); + if ((!attributeKeyMapping.containsKey(attribute.getKey()) && + !attribute.getKey().startsWith(ProjectConfig.RESERVED_ATTRIBUTE_PREFIX)) || attribute.getValue() == null) { + if (unknownAndNullValueAttributes == null) { + unknownAndNullValueAttributes = new ArrayList(); } - unknownAttributes.add(attribute.getKey()); + unknownAndNullValueAttributes.add(attribute.getKey()); } } - if (unknownAttributes != null) { - logger.warn("Attribute(s) {} not in the datafile.", unknownAttributes); - // make a copy of the passed through attributes, then remove the unknown list + if (unknownAndNullValueAttributes != null) { + logger.warn("Attribute(s) {} not in the datafile or has NULL value.", unknownAndNullValueAttributes); + // make a copy of the passed through attributes, then remove the unknown and null values attributes list attributes = new HashMap<>(attributes); - for (String unknownAttribute : unknownAttributes) { - attributes.remove(unknownAttribute); + for (String unknownOrNullValueAttribute : unknownAndNullValueAttributes) { + attributes.remove(unknownOrNullValueAttribute); } } diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 1add5241e..26bf4b7f1 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -70,6 +70,7 @@ import static com.optimizely.ab.event.LogEvent.RequestMethod; import static com.optimizely.ab.event.internal.EventFactoryTest.createExperimentVariationMap; import static java.util.Arrays.asList; +import static junit.framework.Assert.assertNotSame; import static junit.framework.TestCase.assertTrue; import static org.hamcrest.CoreMatchers.is; import static org.hamcrest.CoreMatchers.not; @@ -676,7 +677,7 @@ public void activateWithUnknownAttribute() throws Exception { logbackVerifier.expectMessage(Level.INFO, "Activating user \"userId\" in experiment \"" + activatedExperiment.getKey() + "\"."); - logbackVerifier.expectMessage(Level.WARN, "Attribute(s) [unknownAttribute] not in the datafile."); + logbackVerifier.expectMessage(Level.WARN, "Attribute(s) [unknownAttribute] not in the datafile or has NULL value."); logbackVerifier.expectMessage(Level.DEBUG, "Dispatching impression event to URL test_url with params " + testParams + " and payload \"\""); @@ -755,21 +756,30 @@ public void activateWithNullAttributes() throws Exception { */ @Test public void activateWithNullAttributeValues() throws Exception { - Experiment activatedExperiment = validProjectConfig.getExperiments().get(0); + ProjectConfig projectConfig; + String datafile; + if (datafileVersion == 4) { + projectConfig = validProjectConfig; + datafile = validDatafile; + } else { + projectConfig = noAudienceProjectConfig; + datafile = noAudienceDatafile; + } + Experiment activatedExperiment = projectConfig.getExperiments().get(0); + Attribute attribute = projectConfig.getAttributes().get(0); Variation bucketedVariation = activatedExperiment.getVariations().get(0); - Attribute attribute = validProjectConfig.getAttributes().get(0); // setup a mock event builder to return expected impression params EventFactory mockEventFactory = mock(EventFactory.class); - Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + Optimizely optimizely = Optimizely.builder(datafile, mockEventHandler) .withBucketing(mockBucketer) .withEventBuilder(mockEventFactory) - .withConfig(validProjectConfig) + .withConfig(projectConfig) .withErrorHandler(mockErrorHandler) .build(); - when(mockEventFactory.createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment), eq(bucketedVariation), + when(mockEventFactory.createImpressionEvent(eq(projectConfig), eq(activatedExperiment), eq(bucketedVariation), eq(testUserId), anyMapOf(String.class, String.class))) .thenReturn(logEventToDispatch); @@ -777,7 +787,7 @@ public void activateWithNullAttributeValues() throws Exception { .thenReturn(bucketedVariation); // activate the experiment - Map attributes = new HashMap(); + Map attributes = new HashMap<>(); attributes.put(attribute.getKey(), null); Variation actualVariation = optimizely.activate(activatedExperiment.getKey(), testUserId, attributes); @@ -787,11 +797,12 @@ public void activateWithNullAttributeValues() throws Exception { // setup the attribute map captor (so we can verify its content) ArgumentCaptor attributeCaptor = ArgumentCaptor.forClass(Map.class); - verify(mockEventFactory).createImpressionEvent(eq(validProjectConfig), eq(activatedExperiment), + verify(mockEventFactory).createImpressionEvent(eq(projectConfig), eq(activatedExperiment), eq(bucketedVariation), eq(testUserId), attributeCaptor.capture()); Map actualValue = attributeCaptor.getValue(); - assertThat(actualValue, hasEntry(attribute.getKey(), null)); + assertNotSame(actualValue, hasEntry(attribute.getKey(), null)); + logbackVerifier.expectMessage(Level.WARN, "Attribute(s) ["+attribute.getKey()+"] not in the datafile or has NULL value."); // verify that dispatchEvent was called with the correct LogEvent object verify(mockEventHandler).dispatchEvent(logEventToDispatch); @@ -1624,7 +1635,7 @@ public void trackEventWithUnknownAttribute() throws Exception { logbackVerifier.expectMessage(Level.INFO, "Tracking event \"" + eventType.getKey() + "\" for user \"" + genericUserId + "\"."); - logbackVerifier.expectMessage(Level.WARN, "Attribute(s) [unknownAttribute] not in the datafile."); + logbackVerifier.expectMessage(Level.WARN, "Attribute(s) [unknownAttribute] not in the datafile or has NULL value."); logbackVerifier.expectMessage(Level.DEBUG, "Dispatching conversion event to URL test_url with params " + testParams + " and payload \"\""); From 7978c252c246c70ee09b45c48e47ceb3b47933e2 Mon Sep 17 00:00:00 2001 From: mnoman09 Date: Sat, 8 Sep 2018 04:04:07 +0500 Subject: [PATCH 2/2] nit: spaces before and after the + --- core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java index 26bf4b7f1..50c62b316 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -802,7 +802,7 @@ public void activateWithNullAttributeValues() throws Exception { Map actualValue = attributeCaptor.getValue(); assertNotSame(actualValue, hasEntry(attribute.getKey(), null)); - logbackVerifier.expectMessage(Level.WARN, "Attribute(s) ["+attribute.getKey()+"] not in the datafile or has NULL value."); + logbackVerifier.expectMessage(Level.WARN, "Attribute(s) [" + attribute.getKey() + "] not in the datafile or has NULL value."); // verify that dispatchEvent was called with the correct LogEvent object verify(mockEventHandler).dispatchEvent(logEventToDispatch);