Josh.wang/revise user profile (#80)

* Check for user profile in ProjectValidationUtils before audience evaluation
* Return saved variation if it was stored before bucketing. 
The order of bucketing should be
1) Check Experiment is Active
2) If the user is whitelisted, return the whitelisted variation.
3) If the user profile is saved, return it
4) Check if the user passes audience targeting.
5) Bucket the user with murmur hash

Author: wangjoshuah

build.gradle

@@ -19,6 +19,7 @@ plugins {
 
 allprojects {
     group = 'com.optimizely.ab'
+    apply plugin: 'idea'
 }
 
 apply from: 'gradle/license.gradle'

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

@@ -19,6 +19,7 @@
 import com.optimizely.ab.annotations.VisibleForTesting;
 import com.optimizely.ab.bucketing.Bucketer;
 import com.optimizely.ab.bucketing.UserProfile;
+import com.optimizely.ab.internal.UserProfileUtils;
 import com.optimizely.ab.config.Attribute;
 import com.optimizely.ab.config.EventType;
 import com.optimizely.ab.config.Experiment;
@@ -94,22 +95,25 @@ public class Optimizely {
     @VisibleForTesting final EventHandler eventHandler;
     @VisibleForTesting final ErrorHandler errorHandler;
     @VisibleForTesting final NotificationBroadcaster notificationBroadcaster = new NotificationBroadcaster();
+    @Nullable private final UserProfile userProfile;
 
     private Optimizely(@Nonnull ProjectConfig projectConfig,
                        @Nonnull Bucketer bucketer,
                        @Nonnull EventHandler eventHandler,
                        @Nonnull EventBuilder eventBuilder,
-                       @Nonnull ErrorHandler errorHandler) {
+                       @Nonnull ErrorHandler errorHandler,
+                       @Nullable UserProfile userProfile) {
         this.projectConfig = projectConfig;
         this.bucketer = bucketer;
         this.eventHandler = eventHandler;
         this.eventBuilder = eventBuilder;
         this.errorHandler = errorHandler;
+        this.userProfile = userProfile;
     }
 
     // Do work here that should be done once per Optimizely lifecycle
     @VisibleForTesting void initialize() {
-        bucketer.cleanUserProfiles();
+        UserProfileUtils.cleanUserProfiles(userProfile, projectConfig);
     }
 
     //======== activate calls ========//
@@ -162,13 +166,8 @@ private Optimizely(@Nonnull ProjectConfig projectConfig,
         // attributes.
         attributes = filterAttributes(projectConfig, attributes);
 
-        if (!ProjectValidationUtils.validatePreconditions(projectConfig, experiment, userId, attributes)) {
-            logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
-            return null;
-        }
-
         // bucket the user to the given experiment and dispatch an impression event
-        Variation variation = bucketer.bucket(experiment, userId);
+        Variation variation = getVariation(projectConfig, experiment, attributes, userId);
         if (variation == null) {
             logger.info("Not activating user \"{}\" for experiment \"{}\".", userId, experiment.getKey());
             return null;
@@ -255,7 +254,7 @@ public void track(@Nonnull String eventName,
         }
 
         // create the conversion event request parameters, then dispatch
-        LogEvent conversionEvent = eventBuilder.createConversionEvent(currentConfig, bucketer, userId,
+        LogEvent conversionEvent = eventBuilder.createConversionEvent(currentConfig, bucketer, userProfile, userId,
                                                                       eventType.getId(), eventType.getKey(), attributes,
                                                                       eventTags);
 
@@ -440,7 +439,7 @@ public void track(@Nonnull String eventName,
                                             @Nonnull Map<String, String> attributes,
                                             @Nonnull String userId) {
 
-        if (!ProjectValidationUtils.validatePreconditions(projectConfig, experiment, userId, attributes)) {
+        if (!ProjectValidationUtils.validatePreconditions(projectConfig, userProfile, experiment, userId, attributes)) {
             return null;
         }
 
@@ -468,6 +467,12 @@ private static ProjectConfig getProjectConfig(String datafile) throws ConfigPars
         return DefaultConfigParser.getInstance().parseProjectConfig(datafile);
     }
 
+    @Nullable
+    public UserProfile getUserProfile() {
+        return userProfile;
+    }
+
+
     //======== Notification listeners ========//
 
     /**
@@ -739,7 +744,7 @@ public Optimizely build() throws ConfigParseException {
                 errorHandler = new NoOpErrorHandler();
             }
 
-            Optimizely optimizely = new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler);
+            Optimizely optimizely = new Optimizely(projectConfig, bucketer, eventHandler, eventBuilder, errorHandler, userProfile);
             optimizely.initialize();
             return optimizely;
         }

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

@@ -26,12 +26,11 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.util.List;
-import java.util.Map;
-
 import javax.annotation.Nonnull;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
+import java.util.List;
+import java.util.Map;
 
 /**
  * Default Optimizely bucketing algorithm that evenly distributes users using the Murmur3 hash of some provided
@@ -46,8 +45,7 @@
 public class Bucketer {
 
     private final ProjectConfig projectConfig;
-
-    @Nullable private final UserProfile userProfile;
+    private final UserProfile userProfile;
 
     private static final Logger logger = LoggerFactory.getLogger(Bucketer.class);
 
@@ -112,27 +110,6 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
         String experimentKey = experiment.getKey();
         String combinedBucketId = userId + experimentId;
 
-        // If a user profile instance is present then check it for a saved variation
-        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);
-            }
-        }
-
         List<TrafficAllocation> trafficAllocations = experiment.getTrafficAllocation();
 
         int hashCode = MurmurHash3.murmurhash3_x86_32(combinedBucketId, 0, combinedBucketId.length(), MURMUR_HASH_SEED);
@@ -146,18 +123,6 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
             logger.info("User \"{}\" is in variation \"{}\" of experiment \"{}\".", userId, variationKey,
                         experimentKey);
 
-            // If a user profile is present give it a variation to store
-            if (userProfile != null) {
-                boolean saved = userProfile.save(userId, experimentId, bucketedVariationId);
-                if (saved) {
-                    logger.info("Saved variation \"{}\" of experiment \"{}\" for user \"{}\".",
-                                bucketedVariationId, experimentId, userId);
-                } else {
-                    logger.warn("Failed to save variation \"{}\" of experiment \"{}\" for user \"{}\".",
-                                bucketedVariationId, experimentId, userId);
-                }
-            }
-
             return bucketedVariation;
         }
 
@@ -169,6 +134,7 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
     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)) {
@@ -178,12 +144,37 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
                 logger.info("User \"{}\" is forced in variation \"{}\".", userId, forcedVariationKey);
             } else {
                 logger.error("Variation \"{}\" is not in the datafile. Not activating user \"{}\".", forcedVariationKey,
-                             userId);
+                        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();
         // check whether the experiment belongs to a group
         if (!groupId.isEmpty()) {
@@ -198,16 +189,32 @@ private Variation bucketToVariation(@Nonnull Experiment experiment,
                 // don't perform further bucketing within the experiment
                 if (!bucketedExperiment.getId().equals(experiment.getId())) {
                     logger.info("User \"{}\" is not in experiment \"{}\" of group {}.", userId, experiment.getKey(),
-                                experimentGroup.getId());
+                            experimentGroup.getId());
                     return null;
                 }
 
                 logger.info("User \"{}\" is in experiment \"{}\" of group {}.", userId, experiment.getKey(),
-                            experimentGroup.getId());
+                        experimentGroup.getId());
             }
         }
 
-        return bucketToVariation(experiment, userId);
+        Variation bucketedVariation = bucketToVariation(experiment, userId);
+
+        // ---------- Save Variation to User Profile ----------
+        // If a user profile is present give it a variation to store
+        if (userProfile != null && bucketedVariation != null) {
+            String bucketedVariationId = bucketedVariation.getId();
+            boolean saved = userProfile.save(userId, experimentId, bucketedVariationId);
+            if (saved) {
+                logger.info("Saved variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                        bucketedVariationId, experimentId, userId);
+            } else {
+                logger.warn("Failed to save variation \"{}\" of experiment \"{}\" for user \"{}\".",
+                        bucketedVariationId, experimentId, userId);
+            }
+        }
+
+        return bucketedVariation;
     }
 
     //======== Helper methods ========//
@@ -224,28 +231,5 @@ int generateBucketValue(int hashCode) {
         return (int)Math.floor(MAX_TRAFFIC_VALUE * ratio);
     }
 
-    @Nullable
-    public UserProfile getUserProfile() {
-        return userProfile;
-    }
 
-    /**
-     * Gives implementations of {@link UserProfile} a chance to remove records
-     * of experiments that are deleted or not running.
-     */
-    public void cleanUserProfiles() {
-        if (userProfile != null) {
-            Map<String, Map<String,String>> records = userProfile.getAllRecords();
-            if (records != null) {
-                for (Map.Entry<String,Map<String,String>> record : records.entrySet()) {
-                    for (String experimentId : record.getValue().keySet()) {
-                        Experiment experiment = projectConfig.getExperimentIdMapping().get(experimentId);
-                        if (experiment == null || !experiment.isActive()) {
-                            userProfile.remove(record.getKey(), experimentId);
-                        }
-                    }
-                }
-            }
-        }
-    }
 }

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

@@ -17,13 +17,15 @@
 package com.optimizely.ab.event.internal;
 
 import com.optimizely.ab.bucketing.Bucketer;
+import com.optimizely.ab.bucketing.UserProfile;
 import com.optimizely.ab.config.Experiment;
 import com.optimizely.ab.config.ProjectConfig;
 import com.optimizely.ab.config.Variation;
 import com.optimizely.ab.event.LogEvent;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 import java.util.Collections;
 import java.util.Map;
@@ -38,15 +40,17 @@ public abstract LogEvent createImpressionEvent(@Nonnull ProjectConfig projectCon
 
     public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
                                           @Nonnull Bucketer bucketer,
+                                          @Nullable UserProfile userProfile,
                                           @Nonnull String userId,
                                           @Nonnull String eventId,
                                           @Nonnull String eventName,
                                           @Nonnull Map<String, String> attributes) {
-        return createConversionEvent(projectConfig, bucketer, userId, eventId, eventName, attributes, Collections.<String, String>emptyMap());
+        return createConversionEvent(projectConfig, bucketer, userProfile, userId, eventId, eventName, attributes, Collections.<String, String>emptyMap());
     }
 
     public abstract LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
                                                    @Nonnull Bucketer bucketer,
+                                                   @Nullable UserProfile userProfile,
                                                    @Nonnull String userId,
                                                    @Nonnull String eventId,
                                                    @Nonnull String eventName,

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

@@ -16,6 +16,7 @@
  */
 package com.optimizely.ab.event.internal;
 
+import com.optimizely.ab.bucketing.UserProfile;
 import com.optimizely.ab.event.LogEvent;
 import com.optimizely.ab.internal.EventTagUtils;
 import com.optimizely.ab.internal.ProjectValidationUtils;
@@ -36,6 +37,7 @@
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
 
 import static com.optimizely.ab.event.LogEvent.RequestMethod;
 
@@ -81,6 +83,7 @@ public LogEvent createImpressionEvent(@Nonnull ProjectConfig projectConfig,
 
     public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
                                           @Nonnull Bucketer bucketer,
+                                          @Nullable UserProfile userProfile,
                                           @Nonnull String userId,
                                           @Nonnull String eventId,
                                           @Nonnull String eventName,
@@ -89,7 +92,7 @@ public LogEvent createConversionEvent(@Nonnull ProjectConfig projectConfig,
 
         Map<String, String> requestParams = new HashMap<String, String>();
         List<Experiment> addedExperiments =
-                addExperimentBucketMap(requestParams, projectConfig, bucketer, userId, eventName, attributes);
+                addExperimentBucketMap(requestParams, projectConfig, bucketer, userProfile, userId, eventName, attributes);
 
         if (addedExperiments.isEmpty()) {
             return null;
@@ -162,15 +165,15 @@ private void addProjectId(Map<String, String> requestParams, String projectId) {
      * @param attributes the user's attributes
      */
     private List<Experiment> addExperimentBucketMap(Map<String, String> requestParams, ProjectConfig projectConfig,
-                                                    Bucketer bucketer, String userId, String goalKey,
+                                                    Bucketer bucketer, UserProfile userProfile, String userId, String goalKey,
                                                     Map<String, String> attributes) {
         List<Experiment> allExperiments = projectConfig.getExperiments();
         List<String> experimentIds = projectConfig.getExperimentIdsForGoal(goalKey);
         List<Experiment> validExperiments = new ArrayList<Experiment>();
 
         for (Experiment experiment : allExperiments) {
             if (experimentIds.contains(experiment.getId()) &&
-                    ProjectValidationUtils.validatePreconditions(projectConfig, experiment, userId, attributes)) {
+                    ProjectValidationUtils.validatePreconditions(projectConfig, userProfile, experiment, userId, attributes)) {
                 Variation bucketedVariation = bucketer.bucket(experiment, userId);
                 if (bucketedVariation != null) {
                     requestParams.put(EXPERIMENT_PARAM_PREFIX + experiment.getId(), bucketedVariation.getId());