Partition Bucketer.bucket() (#89)

* break up bucketing into separate parts
  1. `getForcedVariation`
  2. `getStoredVariation`
  3. `bucket`

Author: wangjoshuah

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

@@ -464,7 +464,26 @@ public void track(@Nonnull String eventName,
             return null;
         }
 
-        return bucketer.bucket(experiment, userId);
+        Variation variation;
+
+        // check for whitelisting
+        variation = bucketer.getForcedVariation(experiment, userId);
+        if (variation != null) {
+            return variation;
+        }
+
+        // check if user exists in user profile
+        variation = bucketer.getStoredVariation(experiment, userId);
+        if (variation != null) {
+            return variation;
+        }
+
+        if (ProjectValidationUtils.isUserInExperiment(projectConfig, experiment, attributes)) {
+            return bucketer.bucket(experiment, userId);
+        }
+        logger.info("User \"{}\" does not meet conditions to be in experiment \"{}\".", userId, experiment.getKey());
+
+        return null;
     }
 
     /**

core-api/src/main/java/com/optimizely/ab/bucketing/Bucketer.java

@@ -131,48 +131,15 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         return null;
     }
 
+    /**
+     * Assign a {@link Variation} of an {@link Experiment} to a user based on hashed value from murmurhash3.
+     * @param experiment The Experiment in which the user is to be bucketed.
+     * @param userId User Identifier
+     * @return Variation the user is bucketed into or null.
+     */
     public @Nullable Variation bucket(@Nonnull Experiment experiment,
                                       @Nonnull String userId) {
-
-        // ---------- Check whitelist ----------
-        // if a user has a forced variation mapping, return the respective variation
-        Map<String, String> userIdToVariationKeyMap = experiment.getUserIdToVariationKeyMap();
-        if (userIdToVariationKeyMap.containsKey(userId)) {
-            String forcedVariationKey = userIdToVariationKeyMap.get(userId);
-            Variation forcedVariation = experiment.getVariationKeyToVariationMap().get(forcedVariationKey);
-            if (forcedVariation != null) {
-                logger.info("User \"{}\" is forced in variation \"{}\".", userId, forcedVariationKey);
-            } else {
-                logger.error("Variation \"{}\" is not in the datafile. Not activating user \"{}\".", forcedVariationKey,
-                        userId);
-            }
-
-            return forcedVariation;
-        }
-
-        // ---------- Check User Profile for Sticky Bucketing ----------
-        // If a user profile instance is present then check it for a saved variation
         String experimentId = experiment.getId();
-        String experimentKey = experiment.getKey();
-        if (userProfile != null) {
-            String variationId = userProfile.lookup(userId, experimentId);
-            if (variationId != null) {
-                Variation savedVariation = projectConfig
-                        .getExperimentIdMapping()
-                        .get(experimentId)
-                        .getVariationIdToVariationMap()
-                        .get(variationId);
-                logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" "
-                                + "for user \"{}\" from user profile.",
-                        savedVariation.getKey(), experimentKey, userId);
-                // A variation is stored for this combined bucket id
-                return savedVariation;
-            } else {
-                logger.info("No previously activated variation of experiment \"{}\" "
-                                + "for user \"{}\" found in user profile.",
-                        experimentKey, userId);
-            }
-        }
 
         // ---------- Bucket User ----------
         String groupId = experiment.getGroupId();
@@ -217,6 +184,67 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         return bucketedVariation;
     }
 
+    /**
+     * Get the variation the user has been whitelisted into.
+     * @param experiment {@link Experiment} in which user is to be bucketed.
+     * @param userId User Identifier
+     * @return null if the user is not whitelisted into any variation
+     *      {@link Variation} the user is bucketed into if the user has a specified whitelisted variation.
+     */
+    public @Nullable Variation getForcedVariation(@Nonnull Experiment experiment, @Nonnull String userId) {
+        // if a user has a forced variation mapping, return the respective variation
+        Map<String, String> userIdToVariationKeyMap = experiment.getUserIdToVariationKeyMap();
+        if (userIdToVariationKeyMap.containsKey(userId)) {
+            String forcedVariationKey = userIdToVariationKeyMap.get(userId);
+            Variation forcedVariation = experiment.getVariationKeyToVariationMap().get(forcedVariationKey);
+            if (forcedVariation != null) {
+                logger.info("User \"{}\" is forced in variation \"{}\".", userId, forcedVariationKey);
+            } else {
+                logger.error("Variation \"{}\" is not in the datafile. Not activating user \"{}\".", forcedVariationKey,
+                        userId);
+            }
+
+            return forcedVariation;
+        }
+
+        return null;
+    }
+
+    /**
+     * Get the {@link Variation} that has been stored for the user in the {@link UserProfile} implementation.
+     * @param experiment {@link Experiment} in which the user was bucketed.
+     * @param userId User Identifier
+     * @return null if the {@link UserProfile} implementation is null or the user was not previously bucketed.
+     *      else return the {@link Variation} the user was previously bucketed into.
+     */
+    public @Nullable Variation getStoredVariation(@Nonnull Experiment experiment, @Nonnull String userId) {
+        // ---------- Check User Profile for Sticky Bucketing ----------
+        // If a user profile instance is present then check it for a saved variation
+        String experimentId = experiment.getId();
+        String experimentKey = experiment.getKey();
+        if (userProfile != null) {
+            String variationId = userProfile.lookup(userId, experimentId);
+            if (variationId != null) {
+                Variation savedVariation = projectConfig
+                        .getExperimentIdMapping()
+                        .get(experimentId)
+                        .getVariationIdToVariationMap()
+                        .get(variationId);
+                logger.info("Returning previously activated variation \"{}\" of experiment \"{}\" "
+                                + "for user \"{}\" from user profile.",
+                        savedVariation.getKey(), experimentKey, userId);
+                // A variation is stored for this combined bucket id
+                return savedVariation;
+            } else {
+                logger.info("No previously activated variation of experiment \"{}\" "
+                                + "for user \"{}\" found in user profile.",
+                        experimentKey, userId);
+            }
+        }
+
+        return null;
+    }
+
     //======== Helper methods ========//
 
     /**

core-api/src/main/java/com/optimizely/ab/internal/ProjectValidationUtils.java

@@ -51,19 +51,6 @@ public static boolean validatePreconditions(ProjectConfig projectConfig, UserPro
             return false;
         }
 
-        if (experiment.getUserIdToVariationKeyMap().containsKey(userId)) {
-            return true;
-        }
-
-        if (userProfile != null && userProfile.lookup(userId, experiment.getId()) != null) {
-            return true;
-        }
-
-        if (!isUserInExperiment(projectConfig, experiment, attributes)) {
-            logger.info("User \"{}\" does not meet conditions to be in experiment \"{}\".", userId, experiment.getKey());
-            return false;
-        }
-
         return true;
     }
 
@@ -75,7 +62,7 @@ public static boolean validatePreconditions(ProjectConfig projectConfig, UserPro
      * @param attributes the attributes of the user
      * @return whether the user meets the criteria for the experiment
      */
-    private static boolean isUserInExperiment(ProjectConfig projectConfig, Experiment experiment,
+    public static boolean isUserInExperiment(ProjectConfig projectConfig, Experiment experiment,
                                               Map<String, String> attributes) {
         List<String> experimentAudienceIds = experiment.getAudienceIds();
 

core-api/src/test/java/com/optimizely/ab/OptimizelyTestV2.java

@@ -19,12 +19,14 @@
 import ch.qos.logback.classic.Level;
 import com.google.common.collect.ImmutableMap;
 import com.optimizely.ab.bucketing.Bucketer;
+import com.optimizely.ab.bucketing.UserProfile;
 import com.optimizely.ab.config.Attribute;
 import com.optimizely.ab.config.EventType;
 import com.optimizely.ab.config.Experiment;
 import com.optimizely.ab.config.ProjectConfig;
 import com.optimizely.ab.config.TrafficAllocation;
 import com.optimizely.ab.config.Variation;
+import com.optimizely.ab.config.parser.ConfigParseException;
 import com.optimizely.ab.error.ErrorHandler;
 import com.optimizely.ab.error.NoOpErrorHandler;
 import com.optimizely.ab.error.RaiseExceptionErrorHandler;
@@ -62,6 +64,7 @@
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasEntry;
 import static org.hamcrest.Matchers.hasKey;
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.mockito.Matchers.any;
@@ -71,6 +74,7 @@
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.verify;
 import static org.mockito.Mockito.when;
 
@@ -1450,6 +1454,30 @@ public void trackDispatchesWhenEventHasLaunchedAndRunningExperiments() throws Ex
         verify(client.eventHandler).dispatchEvent(eq(conversionEvent));
     }
 
+
+
+    /**
+     * Verify that an event is not dispatched if a user doesn't satisfy audience conditions for an experiment.
+     */
+    @Test
+    public void trackDoesNotSendEventWhenUserDoesNotSatisfyAudiences() throws Exception {
+        Attribute attribute = validProjectConfig.getAttributes().get(0);
+        EventType eventType = validProjectConfig.getEventTypes().get(2);
+
+        // the audience for the experiments is "NOT firefox" so this user shouldn't satisfy audience conditions
+        Map<String, String> attributeMap = Collections.singletonMap(attribute.getKey(), "firefox");
+
+        Optimizely client = Optimizely.builder(validDatafile, mockEventHandler)
+                .withConfig(validProjectConfig)
+                .build();
+
+        logbackVerifier.expectMessage(Level.INFO, "There are no valid experiments for event \"" + eventType.getKey()
+                + "\" to track.");
+
+        client.track(eventType.getKey(), genericUserId, attributeMap);
+        verify(mockEventHandler, never()).dispatchEvent(any(LogEvent.class));
+    }
+
     //======== getVariation tests ========//
 
     /**
@@ -1604,6 +1632,37 @@ public void getVariationNoAudiences() throws Exception {
         assertThat(actualVariation, is(bucketedVariation));
     }
 
+    /**
+     * Verify that {@link Optimizely#getVariation(Experiment, String, Map)}
+     * gives precedence to user profile over audience evaluation.
+     */
+    @Test
+    public void getVariationEvaluatesUserProfileBeforeAudienceTargeting() throws ConfigParseException {
+        Experiment experiment = validProjectConfig.getExperiments().get(0);
+        Variation storedVariation = experiment.getVariations().get(0);
+        String userProfileUserId = "userProfileId";
+
+        UserProfile mockedUserProfile = mock(UserProfile.class);
+        when(mockedUserProfile.lookup(userProfileUserId, experiment.getId())).thenReturn(storedVariation.getId());
+
+        Optimizely client = Optimizely.builder(validDatafile, mockEventHandler)
+                .withConfig(validProjectConfig)
+                .withUserProfile(mockedUserProfile)
+                .build();
+        assertNotNull(client);
+
+        // ensure that normal users still get excluded from the experiment when they fail audience evaluation
+        assertNull(client.getVariation(experiment, genericUserId, Collections.<String, String>emptyMap()));
+
+        logbackVerifier.expectMessage(Level.INFO,
+                "User \"" + genericUserId + "\" does not meet conditions to be in experiment \""
+                        + experiment.getKey() + "\".");
+
+        // ensure that a user with a saved user profile, sees the same variation regardless of audience evaluation
+        assertEquals(storedVariation,
+                client.getVariation(experiment, userProfileUserId, Collections.<String, String>emptyMap()));
+    }
+
     /**
      * 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 RaiseExceptionErrorHandler} is provided.
@@ -1685,24 +1744,31 @@ public void getVariationForGroupExperimentWithNonMatchingAttributes() throws Exc
 
     /**
      * Verify that {@link Optimizely#getVariation(String, String, Map)} gives precedence to forced variation bucketing
-     * over audience evaluation.
+     * over user profile and audience evaluation for murmurhash3 bucketing.
      */
     @Test
-    public void getVariationForcedVariationPrecedesAudienceEval() throws Exception {
+    public void getVariationForcedVariationPrecedesUserProfileAndAudienceEval() throws Exception {
+        Bucketer bucketer = spy(new Bucketer(validProjectConfig));
         Experiment experiment = validProjectConfig.getExperiments().get(0);
         Variation expectedVariation = experiment.getVariations().get(0);
+        String whitelistedUserId = "testUser1";
 
         Optimizely optimizely = Optimizely.builder(validDatafile, mockEventHandler)
-            .withConfig(validProjectConfig)
-            .build();
+                .withBucketing(bucketer)
+                .withConfig(validProjectConfig)
+                .build();
 
         // user excluded without audiences and whitelisting
         assertNull(optimizely.getVariation(experiment.getKey(), genericUserId));
 
-        logbackVerifier.expectMessage(Level.INFO, "User \"testUser1\" is forced in variation \"vtag1\".");
+        logbackVerifier.expectMessage(Level.INFO, "User \"" + whitelistedUserId + "\" is forced in variation \"vtag1\".");
 
         // no attributes provided for a experiment that has an audience
-        assertThat(optimizely.getVariation(experiment.getKey(), "testUser1"), is(expectedVariation));
+        assertThat(optimizely.getVariation(experiment.getKey(), whitelistedUserId), is(expectedVariation));
+
+        verify(bucketer).getForcedVariation(experiment, whitelistedUserId);
+        verify(bucketer, never()).getStoredVariation(experiment, whitelistedUserId);
+        verify(bucketer, never()).bucket(experiment, whitelistedUserId);
     }
 
     /**