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 a4b4b2a83..032f4aff2 100644 --- a/core-api/src/main/java/com/optimizely/ab/Optimizely.java +++ b/core-api/src/main/java/com/optimizely/ab/Optimizely.java @@ -122,6 +122,11 @@ Variation activate(@Nonnull String experimentKey, @Nonnull String userId, @Nonnull Map attributes) throws UnknownExperimentException { + if (experimentKey == null) { + logger.error("The experimentKey parameter must be nonnull."); + return null; + } + if (!validateUserId(userId)) { logger.info("Not activating user for experiment \"{}\".", experimentKey); return null; @@ -161,6 +166,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig, @Nonnull String userId, @Nonnull Map attributes) { + if (!validateUserId(userId)){ + logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey()); + return null; + } // determine whether all the given attributes are present in the project config. If not, filter out the unknown // attributes. Map filteredAttributes = filterAttributes(projectConfig, attributes); @@ -224,6 +233,17 @@ public void track(@Nonnull String eventName, @Nonnull Map attributes, @Nonnull Map eventTags) throws UnknownEventTypeException { + if (!validateUserId(userId)) { + logger.info("Not tracking event \"{}\".", eventName); + return; + } + + if (eventName == null || eventName.trim().isEmpty()){ + logger.error("Event Key is null or empty when non-null and non-empty String was expected."); + logger.info("Not tracking event for user \"{}\".", userId); + return; + } + ProjectConfig currentConfig = getProjectConfig(); EventType eventType = currentConfig.getEventTypeForName(eventName, errorHandler); @@ -584,7 +604,7 @@ else if (userId == null) { * @return List of the feature keys that are enabled for the user if the userId is empty it will * return Empty List. */ - public List getEnabledFeatures(@Nonnull String userId,@Nonnull Map attributes) { + public List getEnabledFeatures(@Nonnull String userId, @Nonnull Map attributes) throws IllegalArgumentException { List enabledFeaturesList = new ArrayList(); if (!validateUserId(userId)){ @@ -634,6 +654,11 @@ Variation getVariation(@Nonnull String experimentKey, return null; } + if (experimentKey == null || experimentKey.trim().isEmpty()){ + logger.error("The experimentKey parameter must be nonnull."); + return null; + } + ProjectConfig currentConfig = getProjectConfig(); Experiment experiment = currentConfig.getExperimentForKey(experimentKey, errorHandler); @@ -767,6 +792,10 @@ private Map filterAttributes(@Nonnull ProjectConfig projectConfi * @return whether the user ID is valid */ private boolean validateUserId(String userId) { + if (userId == null) { + logger.error("The user ID parameter must be nonnull."); + return false; + } if (userId.trim().isEmpty()) { logger.error("Non-empty user ID required"); return false; 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 b45fd4889..f08e8561f 100644 --- a/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java +++ b/core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java @@ -851,6 +851,80 @@ public void activateUserInAudience() throws Exception { assertNotNull(actualVariation); } + /** + * Verify that if user ID sent is null will return null variation. + */ + @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") + @Test + public void activateUserIDIsNull() throws Exception { + Experiment experimentToCheck = validProjectConfig.getExperiments().get(0); + String nullUserID = null; + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testUserAttributes = new HashMap(); + testUserAttributes.put("browser_type", "chrome"); + + Variation nullVariation = optimizely.activate(experimentToCheck.getKey(), nullUserID, testUserAttributes); + assertNull(nullVariation); + + logbackVerifier.expectMessage( + Level.ERROR, + "The user ID parameter must be nonnull." + ); + } + + /** + * Verify that if user ID sent is null will return null variation. + * In activate override function where experiment object is passed + */ + @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") + @Test + public void activateWithExperimentUserIDIsNull() throws Exception { + Experiment experimentToCheck = validProjectConfig.getExperiments().get(0); + String nullUserID = null; + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testUserAttributes = new HashMap(); + testUserAttributes.put("browser_type", "chrome"); + + Variation nullVariation = optimizely.activate(experimentToCheck, nullUserID, testUserAttributes); + assertNull(nullVariation); + + logbackVerifier.expectMessage( + Level.ERROR, + "The user ID parameter must be nonnull." + ); + } + + /** + * Verify that if Experiment key sent is null will return null variation. + */ + @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") + @Test + public void activateExperimentKeyIsNull() throws Exception { + String nullExperimentKey = null; + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testUserAttributes = new HashMap(); + testUserAttributes.put("browser_type", "chrome"); + + Variation nullVariation = optimizely.activate(nullExperimentKey, testUserId, testUserAttributes); + assertNull(nullVariation); + + logbackVerifier.expectMessage( + Level.ERROR, + "The experimentKey parameter must be nonnull." + ); + } /** * Verify that a user not in any of an experiment's audiences isn't assigned to a variation. */ @@ -1722,11 +1796,112 @@ public void trackEventWithNullEventTags() throws Exception { verify(mockEventHandler).dispatchEvent(logEventToDispatch); } + /** - * Verify that {@link Optimizely#track(String, String, Map)} doesn't dispatch an event when no valid experiments - * correspond to an event. + * Verify that {@link Optimizely#track(String, String, Map, Map)} called with null User ID will return and will not track */ @Test + @SuppressFBWarnings( + value="NP_NONNULL_PARAM_VIOLATION", + justification="testing nullness contract violation") + public void trackEventWithNullOrEmptyUserID() throws Exception { + EventType eventType; + if (datafileVersion >= 4) { + eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); + } else { + eventType = validProjectConfig.getEventTypes().get(0); + } + // setup a mock event builder to return expected conversion params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + Map experimentVariationMap = createExperimentVariationMap( + validProjectConfig, + mockDecisionService, + eventType.getKey(), + genericUserId, + Collections.emptyMap()); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createConversionEvent( + eq(validProjectConfig), + eq(experimentVariationMap), + eq(genericUserId), + eq(eventType.getId()), + eq(eventType.getKey()), + eq(Collections.emptyMap()), + eq(Collections.emptyMap()))) + .thenReturn(logEventToDispatch); + + String userID = null; + // call track with null event key + optimizely.track(eventType.getKey(), userID, Collections.emptyMap(), Collections.emptyMap()); + logbackVerifier.expectMessage(Level.ERROR, "The user ID parameter must be nonnull."); + logbackVerifier.expectMessage(Level.INFO, "Not tracking event \""+eventType.getKey()+"\"."); + } + + /** + * Verify that {@link Optimizely#track(String, String, Map, Map)} called with null event name will return and will not track + */ + @Test + @SuppressFBWarnings( + value="NP_NONNULL_PARAM_VIOLATION", + justification="testing nullness contract violation") + public void trackEventWithNullOrEmptyEventKey() throws Exception { + EventType eventType; + if (datafileVersion >= 4) { + eventType = validProjectConfig.getEventNameMapping().get(EVENT_BASIC_EVENT_KEY); + } else { + eventType = validProjectConfig.getEventTypes().get(0); + } + String nullEventKey = null; + // setup a mock event builder to return expected conversion params + EventBuilder mockEventBuilder = mock(EventBuilder.class); + + Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withEventBuilder(mockEventBuilder) + .withConfig(validProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + Map testParams = new HashMap(); + testParams.put("test", "params"); + Map experimentVariationMap = createExperimentVariationMap( + validProjectConfig, + mockDecisionService, + eventType.getKey(), + genericUserId, + Collections.emptyMap()); + LogEvent logEventToDispatch = new LogEvent(RequestMethod.GET, "test_url", testParams, ""); + when(mockEventBuilder.createConversionEvent( + eq(validProjectConfig), + eq(experimentVariationMap), + eq(genericUserId), + eq(eventType.getId()), + eq(eventType.getKey()), + eq(Collections.emptyMap()), + eq(Collections.emptyMap()))) + .thenReturn(logEventToDispatch); + + // call track with null event key + optimizely.track(nullEventKey, genericUserId, Collections.emptyMap(), Collections.emptyMap()); + logbackVerifier.expectMessage(Level.ERROR, "Event Key is null or empty when non-null and non-empty String was expected."); + logbackVerifier.expectMessage(Level.INFO, "Not tracking event for user \""+genericUserId+"\"."); + + } + /** + * Verify that {@link Optimizely#track(String, String, Map)} doesn't dispatch an event when no valid experiments + * correspond to an event. + */ + @Test public void trackEventWithNoValidExperiments() throws Exception { EventType eventType; if (datafileVersion >= 4) { @@ -1982,6 +2157,28 @@ public void getVariationWithExperimentKey() throws Exception { verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class)); } + /** + * Verify that {@link Optimizely#getVariation(String, String)} returns null variation when null or empty + * experimentKey is sent + */ + @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") + @Test + public void getVariationWithNullExperimentKey() throws Exception { + Optimizely optimizely = Optimizely.builder(noAudienceDatafile, mockEventHandler) + .withBucketing(mockBucketer) + .withConfig(noAudienceProjectConfig) + .withErrorHandler(mockErrorHandler) + .build(); + + String nullExperimentKey = null; + // activate the experiment + Variation nullVariation = optimizely.getVariation(nullExperimentKey, testUserId); + + assertNull(nullVariation); + logbackVerifier.expectMessage(Level.ERROR, "The experimentKey parameter must be nonnull."); + + } + /** * Verify that {@link Optimizely#getVariation(String, String)} handles the case where an unknown experiment * (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{ } + /** + * Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into + * {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending + * userId as null + * Exception of IllegalArgumentException will be thrown + * return + */ + @SuppressFBWarnings("NP_NONNULL_PARAM_VIOLATION") + @Test + public void getEnabledFeatureWithNullUserID() throws ConfigParseException{ + assumeTrue(datafileVersion >= Integer.parseInt(ProjectConfig.Version.V4.toString())); + String userID = null; + Optimizely spyOptimizely = spy(Optimizely.builder(validDatafile, mockEventHandler) + .withConfig(validProjectConfig) + .build()); + ArrayList featureFlags = (ArrayList) spyOptimizely.getEnabledFeatures(userID, + new HashMap()); + assertTrue(featureFlags.isEmpty()); + + logbackVerifier.expectMessage( + Level.ERROR, + "The user ID parameter must be nonnull." + ); + } + /** * Verify {@link Optimizely#getEnabledFeatures(String, Map)} calls into * {@link Optimizely#isFeatureEnabled(String, String, Map)} for each featureFlag sending