Skip to content

Commit 4abffbb

Browse files
mnoman09thomaszurkan-optimizely
mnoman09
authored and
thomaszurkan-optimizely
committed
getEnabledFeature, Activate, getVariation and track Null check fixes (#180)
* getEnabledFeature, Activate, getVariation and track Null check fixes * removed Nonnull from getEnabledFeature UserID and added check in ValidateUserID of non null user ID * Added test Case of Non null user id in get enabled Feature * Added Null check of experiment in user activate * added null check in user id and in event key of track * Added null check on Experiment key of getVariation and added its unitTest * Removed throws IllegalArgumentException
1 parent 74b1cc6 commit 4abffbb

File tree

2 files changed

+254
-3
lines changed

2 files changed

+254
-3
lines changed

‎core-api/src/main/java/com/optimizely/ab/Optimizely.java

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,11 @@ Variation activate(@Nonnull String experimentKey,
122122
@Nonnull String userId,
123123
@Nonnull Map<String, String> attributes) throws UnknownExperimentException {
124124

125+
if (experimentKey == null) {
126+
logger.error("The experimentKey parameter must be nonnull.");
127+
return null;
128+
}
129+
125130
if (!validateUserId(userId)) {
126131
logger.info("Not activating user for experiment \"{}\".", experimentKey);
127132
return null;
@@ -161,6 +166,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig,
161166
@Nonnull String userId,
162167
@Nonnull Map<String, String> attributes) {
163168

169+
if (!validateUserId(userId)){
170+
logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
171+
return null;
172+
}
164173
// determine whether all the given attributes are present in the project config. If not, filter out the unknown
165174
// attributes.
166175
Map<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
@@ -224,6 +233,17 @@ public void track(@Nonnull String eventName,
224233
@Nonnull Map<String, String> attributes,
225234
@Nonnull Map<String, ?> eventTags) throws UnknownEventTypeException {
226235

236+
if (!validateUserId(userId)) {
237+
logger.info("Not tracking event \"{}\".", eventName);
238+
return;
239+
}
240+
241+
if (eventName == null || eventName.trim().isEmpty()){
242+
logger.error("Event Key is null or empty when non-null and non-empty String was expected.");
243+
logger.info("Not tracking event for user \"{}\".", userId);
244+
return;
245+
}
246+
227247
ProjectConfig currentConfig = getProjectConfig();
228248

229249
EventType eventType = currentConfig.getEventTypeForName(eventName, errorHandler);
@@ -584,7 +604,7 @@ else if (userId == null) {
584604
* @return List of the feature keys that are enabled for the user if the userId is empty it will
585605
* return Empty List.
586606
*/
587-
public List<String> getEnabledFeatures(@Nonnull String userId,@Nonnull Map<String, String> attributes) {
607+
public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<String, String> attributes) {
588608
List<String> enabledFeaturesList = new ArrayList<String>();
589609

590610
if (!validateUserId(userId)){
@@ -634,6 +654,11 @@ Variation getVariation(@Nonnull String experimentKey,
634654
return null;
635655
}
636656

657+
if (experimentKey == null || experimentKey.trim().isEmpty()){
658+
logger.error("The experimentKey parameter must be nonnull.");
659+
return null;
660+
}
661+
637662
ProjectConfig currentConfig = getProjectConfig();
638663

639664
Experiment experiment = currentConfig.getExperimentForKey(experimentKey, errorHandler);
@@ -767,6 +792,10 @@ private Map<String, String> filterAttributes(@Nonnull ProjectConfig projectConfi
767792
* @return whether the user ID is valid
768793
*/
769794
private boolean validateUserId(String userId) {
795+
if (userId == null) {
796+
logger.error("The user ID parameter must be nonnull.");
797+
return false;
798+
}
770799
if (userId.trim().isEmpty()) {
771800
logger.error("Non-empty user ID required");
772801
return false;

‎core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java

Lines changed: 224 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -851,6 +851,80 @@ public void activateUserInAudience() throws Exception {
851851
assertNotNull(actualVariation);
852852
}
853853

854+
/**
855+
* Verify that if user ID sent is null will return null variation.
856+
*/
857+
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
858+
@Test
859+
public void activateUserIDIsNull() throws Exception {
860+
Experiment experimentToCheck = validProjectConfig.getExperiments().get(0);
861+
String nullUserID = null;
862+
Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
863+
.withConfig(validProjectConfig)
864+
.withErrorHandler(mockErrorHandler)
865+
.build();
866+
867+
Map<String, String> testUserAttributes = new HashMap<String, String>();
868+
testUserAttributes.put("browser_type", "chrome");
869+
870+
Variation nullVariation = optimizely.activate(experimentToCheck.getKey(), nullUserID, testUserAttributes);
871+
assertNull(nullVariation);
872+
873+
logbackVerifier.expectMessage(
874+
Level.ERROR,
875+
"The user ID parameter must be nonnull."
876+
);
877+
}
878+
879+
/**
880+
* Verify that if user ID sent is null will return null variation.
881+
* In activate override function where experiment object is passed
882+
*/
883+
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
884+
@Test
885+
public void activateWithExperimentUserIDIsNull() throws Exception {
886+
Experiment experimentToCheck = validProjectConfig.getExperiments().get(0);
887+
String nullUserID = null;
888+
Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
889+
.withConfig(validProjectConfig)
890+
.withErrorHandler(mockErrorHandler)
891+
.build();
892+
893+
Map<String, String> testUserAttributes = new HashMap<String, String>();
894+
testUserAttributes.put("browser_type", "chrome");
895+
896+
Variation nullVariation = optimizely.activate(experimentToCheck, nullUserID, testUserAttributes);
897+
assertNull(nullVariation);
898+
899+
logbackVerifier.expectMessage(
900+
Level.ERROR,
901+
"The user ID parameter must be nonnull."
902+
);
903+
}
904+
905+
/**
906+
* Verify that if Experiment key sent is null will return null variation.
907+
*/
908+
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
909+
@Test
910+
public void activateExperimentKeyIsNull() throws Exception {
911+
String nullExperimentKey = null;
912+
Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
913+
.withConfig(validProjectConfig)
914+
.withErrorHandler(mockErrorHandler)
915+
.build();
916+
917+
Map<String, String> testUserAttributes = new HashMap<String, String>();
918+
testUserAttributes.put("browser_type", "chrome");
919+
920+
Variation nullVariation = optimizely.activate(nullExperimentKey, testUserId, testUserAttributes);
921+
assertNull(nullVariation);
922+
923+
logbackVerifier.expectMessage(
924+
Level.ERROR,
925+
"The experimentKey parameter must be nonnull."
926+
);
927+
}
854928
/**
855929
* Verify that a user not in any of an experiment's audiences isn't assigned to a variation.
856930
*/
@@ -1722,11 +1796,112 @@ public void trackEventWithNullEventTags() throws Exception {
17221796
verify(mockEventHandler).dispatchEvent(logEventToDispatch);
17231797
}
17241798

1799+
17251800
/**
1726-
* Verify that {@link Optimizely#track(String, String, Map)} doesn't dispatch an event when no valid experiments
1727-
* correspond to an event.
1801+
* Verify that {@link Optimizely#track(String, String, Map, Map)} called with null User ID will return and will not track
17281802
*/
17291803
@Test
1804+
@SuppressFBWarnings(
1805+
value="NP_NONNULL_PARAM_VIOLATION",
1806+
justification="testing nullness contract violation")
1807+
public void trackEventWithNullOrEmptyUserID() throws Exception {
1808+
EventType eventType;
1809+
if (datafileVersion >= 4) {
1810+
eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY);
1811+
} else {
1812+
eventType = validProjectConfig.getEventTypes().get(0);
1813+
}
1814+
// setup a mock event builder to return expected conversion params
1815+
EventBuilder mockEventBuilder = mock(EventBuilder.class);
1816+
1817+
Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
1818+
.withBucketing(mockBucketer)
1819+
.withEventBuilder(mockEventBuilder)
1820+
.withConfig(validProjectConfig)
1821+
.withErrorHandler(mockErrorHandler)
1822+
.build();
1823+
1824+
Map<String, String> testParams = new HashMap<String, String>();
1825+
testParams.put("test", "params");
1826+
Map<Experiment, Variation> experimentVariationMap = createExperimentVariationMap(
1827+
validProjectConfig,
1828+
mockDecisionService,
1829+
eventType.getKey(),
1830+
genericUserId,
1831+
Collections.<String, String>emptyMap());
1832+
LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, "");
1833+
when(mockEventBuilder.createConversionEvent(
1834+
eq(validProjectConfig),
1835+
eq(experimentVariationMap),
1836+
eq(genericUserId),
1837+
eq(eventType.getId()),
1838+
eq(eventType.getKey()),
1839+
eq(Collections.<String, String>emptyMap()),
1840+
eq(Collections.<String, String>emptyMap())))
1841+
.thenReturn(logEventToDispatch);
1842+
1843+
String userID = null;
1844+
// call track with null event key
1845+
optimizely.track(eventType.getKey(), userID, Collections.<String, String>emptyMap(), Collections.<String, Object>emptyMap());
1846+
logbackVerifier.expectMessage(Level.ERROR, "The user ID parameter must be nonnull.");
1847+
logbackVerifier.expectMessage(Level.INFO, "Not tracking event \""+eventType.getKey()+"\".");
1848+
}
1849+
1850+
/**
1851+
* Verify that {@link Optimizely#track(String, String, Map, Map)} called with null event name will return and will not track
1852+
*/
1853+
@Test
1854+
@SuppressFBWarnings(
1855+
value="NP_NONNULL_PARAM_VIOLATION",
1856+
justification="testing nullness contract violation")
1857+
public void trackEventWithNullOrEmptyEventKey() throws Exception {
1858+
EventType eventType;
1859+
if (datafileVersion >= 4) {
1860+
eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY);
1861+
} else {
1862+
eventType = validProjectConfig.getEventTypes().get(0);
1863+
}
1864+
String nullEventKey = null;
1865+
// setup a mock event builder to return expected conversion params
1866+
EventBuilder mockEventBuilder = mock(EventBuilder.class);
1867+
1868+
Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
1869+
.withBucketing(mockBucketer)
1870+
.withEventBuilder(mockEventBuilder)
1871+
.withConfig(validProjectConfig)
1872+
.withErrorHandler(mockErrorHandler)
1873+
.build();
1874+
1875+
Map<String, String> testParams = new HashMap<String, String>();
1876+
testParams.put("test", "params");
1877+
Map<Experiment, Variation> experimentVariationMap = createExperimentVariationMap(
1878+
validProjectConfig,
1879+
mockDecisionService,
1880+
eventType.getKey(),
1881+
genericUserId,
1882+
Collections.<String, String>emptyMap());
1883+
LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, "");
1884+
when(mockEventBuilder.createConversionEvent(
1885+
eq(validProjectConfig),
1886+
eq(experimentVariationMap),
1887+
eq(genericUserId),
1888+
eq(eventType.getId()),
1889+
eq(eventType.getKey()),
1890+
eq(Collections.<String, String>emptyMap()),
1891+
eq(Collections.<String, String>emptyMap())))
1892+
.thenReturn(logEventToDispatch);
1893+
1894+
// call track with null event key
1895+
optimizely.track(nullEventKey, genericUserId, Collections.<String, String>emptyMap(), Collections.<String, Object>emptyMap());
1896+
logbackVerifier.expectMessage(Level.ERROR, "Event Key is null or empty when non-null and non-empty String was expected.");
1897+
logbackVerifier.expectMessage(Level.INFO, "Not tracking event for user \""+genericUserId+"\".");
1898+
1899+
}
1900+
/**
1901+
* Verify that {@link Optimizely#track(String, String, Map)} doesn't dispatch an event when no valid experiments
1902+
* correspond to an event.
1903+
*/
1904+
@Test
17301905
public void trackEventWithNoValidExperiments() throws Exception {
17311906
EventType eventType;
17321907
if (datafileVersion >= 4) {
@@ -1982,6 +2157,28 @@ public void getVariationWithExperimentKey() throws Exception {
19822157
verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class));
19832158
}
19842159

2160+
/**
2161+
* Verify that {@link Optimizely#getVariation(String, String)} returns null variation when null or empty
2162+
* experimentKey is sent
2163+
*/
2164+
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
2165+
@Test
2166+
public void getVariationWithNullExperimentKey() throws Exception {
2167+
Optimizely optimizely = Optimizely.builder(noAudienceDatafile, mockEventHandler)
2168+
.withBucketing(mockBucketer)
2169+
.withConfig(noAudienceProjectConfig)
2170+
.withErrorHandler(mockErrorHandler)
2171+
.build();
2172+
2173+
String nullExperimentKey = null;
2174+
// activate the experiment
2175+
Variation nullVariation = optimizely.getVariation(nullExperimentKey, testUserId);
2176+
2177+
assertNull(nullVariation);
2178+
logbackVerifier.expectMessage(Level.ERROR, "The experimentKey parameter must be nonnull.");
2179+
2180+
}
2181+
19852182
/**
19862183
* Verify that {@link Optimizely#getVariation(String, String)} handles the case where an unknown experiment
19872184
* (i.e., not in the config) is passed through and a {@link NoOpErrorHandler} is used by default.
@@ -3412,6 +3609,31 @@ public void getEnabledFeatureWithEmptyUserId() throws ConfigParseException{
34123609

34133610
}
34143611

3612+
/**
3613+
* Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into
3614+
* {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending
3615+
* userId as null
3616+
* Exception of IllegalArgumentException will be thrown
3617+
* return
3618+
*/
3619+
@SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION")
3620+
@Test
3621+
public void getEnabledFeatureWithNullUserID() throws ConfigParseException{
3622+
assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString()));
3623+
String userID = null;
3624+
Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler)
3625+
.withConfig(validProjectConfig)
3626+
.build());
3627+
ArrayList<String> featureFlags = (ArrayList<String>) spyOptimizely.getEnabledFeatures(userID,
3628+
new HashMap<String, String>());
3629+
assertTrue(featureFlags.isEmpty());
3630+
3631+
logbackVerifier.expectMessage(
3632+
Level.ERROR,
3633+
"The user ID parameter must be nonnull."
3634+
);
3635+
}
3636+
34153637
/**
34163638
* Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into
34173639
* {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending

0 commit comments

Comments
 (0)