Skip to content

removed Nonnull from getEnabledFeature UserID #3

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

removed Nonnull from getEnabledFeature UserID #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3298b75
removed Nonnull from getEnabledFeature UserID and added check in Vali…
mnoman09 Apr 18, 2018
c3127a8
Added @Nonnull
mnoman09 Apr 18, 2018
03a74cb
Added test Case of Non null user id in get enabled Feature
mnoman09 Apr 19, 2018
f7eced7
Added Null check of experiment in user
mnoman09 Apr 19, 2018
ea08778
added null check in user id and in event key of track
mnoman09 Apr 19, 2018
a700d4c
Added null check on Experiment key of getVariation and added its unit…
mnoman09 Apr 19, 2018
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
31 changes: 30 additions & 1 deletion 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 @@ -122,6 +122,11 @@ Variation activate(@Nonnull String experimentKey,
@Nonnull String userId,
@Nonnull Map<String, String> 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;
Expand Down Expand Up @@ -161,6 +166,10 @@ Variation activate(@Nonnull ProjectConfig projectConfig,
@Nonnull String userId,
@Nonnull Map<String, String> 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<String, String> filteredAttributes = filterAttributes(projectConfig, attributes);
Expand Down Expand Up @@ -224,6 +233,17 @@ public void track(@Nonnull String eventName,
@Nonnull Map<String, String> attributes,
@Nonnull Map<String, ?> 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);
Expand Down Expand Up @@ -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<String> getEnabledFeatures(@Nonnull String userId,@Nonnull Map<String, String> attributes) {
public List<String> getEnabledFeatures(@Nonnull String userId, @Nonnull Map<String, String> attributes) throws IllegalArgumentException {
List<String> enabledFeaturesList = new ArrayList<String>();

if (!validateUserId(userId)){
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -767,6 +792,10 @@ private Map<String, String> 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;
Expand Down
226 changes: 224 additions & 2 deletions core-api/src/test/java/com/optimizely/ab/OptimizelyTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, String> testUserAttributes = new HashMap<String, String>();
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<String, String> testUserAttributes = new HashMap<String, String>();
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<String, String> testUserAttributes = new HashMap<String, String>();
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.
*/
Expand Down Expand Up @@ -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<String, String> testParams = new HashMap<String, String>();
testParams.put("test", "params");
Map<Experiment, Variation> experimentVariationMap = createExperimentVariationMap(
validProjectConfig,
mockDecisionService,
eventType.getKey(),
genericUserId,
Collections.<String, String>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.<String, String>emptyMap()),
eq(Collections.<String, String>emptyMap())))
.thenReturn(logEventToDispatch);

String userID = null;
// call track with null event key
optimizely.track(eventType.getKey(), userID, Collections.<String, String>emptyMap(), Collections.<String, Object>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<String, String> testParams = new HashMap<String, String>();
testParams.put("test", "params");
Map<Experiment, Variation> experimentVariationMap = createExperimentVariationMap(
validProjectConfig,
mockDecisionService,
eventType.getKey(),
genericUserId,
Collections.<String, String>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.<String, String>emptyMap()),
eq(Collections.<String, String>emptyMap())))
.thenReturn(logEventToDispatch);

// call track with null event key
optimizely.track(nullEventKey, genericUserId, Collections.<String, String>emptyMap(), Collections.<String, Object>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) {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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<String> featureFlags = (ArrayList<String>) spyOptimizely.getEnabledFeatures(userID,
new HashMap<String, String>());
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
Expand Down