updates per code review

Author: James Fox

packages/optimizely-sdk/lib/core/audience_evaluator/index.js

@@ -15,19 +15,29 @@
  */
 var conditionTreeEvaluator = require('../condition_tree_evaluator');
 var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
-var ERROR_MESSAGES = require('../../utils/enums').ERROR_MESSAGES;
 var enums = require('../../utils/enums');
+var fns = require('../../utils/fns');
 var sprintf = require('sprintf-js').sprintf;
 
+var ERROR_MESSAGES = enums.ERROR_MESSAGES;
 var LOG_LEVEL = enums.LOG_LEVEL;
 var LOG_MESSAGES = enums.LOG_MESSAGES;
 var MODULE_NAME = 'AUDIENCE_EVALUATOR';
 
 
-function AudienceEvaluator(conditionEvaluators) {
-  this.typeToEvaluatorMap = Object.assign({
+/**
+ * Construct an instance of AudienceEvaluator with a given logger and options
+ * @param {Logger}  logger                           The Logger instance
+ * @param {Object=} __exploratoryConditionEvaluators A map of condition evaluators provided by the consumer. This enables matching
+ *                                                   condition types which are not supported natively by the SDK. Note that built in
+ *                                                   Optimizely evaluators cannot be overridden.
+ * @constructor
+ */
+function AudienceEvaluator(logger, __exploratoryConditionEvaluators) {
+  this.logger = logger;
+  this.typeToEvaluatorMap = fns.assignIn({}, __exploratoryConditionEvaluators, {
     'custom_attribute': customAttributeConditionEvaluator
-  }, conditionEvaluators);
+  });
 }
 
 /**
@@ -40,11 +50,10 @@ function AudienceEvaluator(conditionEvaluators) {
  *                                                              should be full audience objects with conditions properties
  * @param  {Object}                       [userAttributes]      User attributes which will be used in determining if audience conditions
  *                                                              are met. If not provided, defaults to an empty object
- * @param  {Object}                       logger                Logger instance.
  * @return {Boolean}                                            true if the user attributes match the given audience conditions, false
  *                                                              otherwise
  */
-AudienceEvaluator.prototype.evaluate = function(audienceConditions, audiencesById, userAttributes, logger) {
+AudienceEvaluator.prototype.evaluate = function(audienceConditions, audiencesById, userAttributes) {
   // if there are no audiences, return true because that means ALL users are included in the experiment
   if (!audienceConditions || audienceConditions.length === 0) {
     return true;
@@ -54,29 +63,41 @@ AudienceEvaluator.prototype.evaluate = function(audienceConditions, audiencesByI
     userAttributes = {};
   }
 
-  var evaluateConditionWithUserAttributes = function(condition) {
-    var evaluator = this.typeToEvaluatorMap[condition.type];
-    if (!evaluator) {
-      logger.log(LOG_LEVEL.WARNING, sprintf(ERROR_MESSAGES.AUDIENCE_EVALUATOR_EXPECTED, MODULE_NAME, condition.type));
-      return false;
-    }
-    return evaluator.evaluate(condition, userAttributes, logger);
-  }.bind(this);
-
   var evaluateAudience = function(audienceId) {
     var audience = audiencesById[audienceId];
     if (audience) {
-      logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCE, MODULE_NAME, audienceId, JSON.stringify(audience.conditions)));
-      var result = conditionTreeEvaluator.evaluate(audience.conditions, evaluateConditionWithUserAttributes);
+      this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCE, MODULE_NAME, audienceId, JSON.stringify(audience.conditions)));
+      var result = conditionTreeEvaluator.evaluate(audience.conditions, this.evaluateConditionWithUserAttributes.bind(this, userAttributes));
       var resultText = result === null ? 'UNKNOWN' : result.toString().toUpperCase();
-      logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT, MODULE_NAME, audienceId, resultText));
+      this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT, MODULE_NAME, audienceId, resultText));
       return result;
     }
 
     return null;
-  };
+  }.bind(this);
 
   return conditionTreeEvaluator.evaluate(audienceConditions, evaluateAudience) || false;
 };
 
+/**
+ * Wrapper around evaluator.evaluate that is passed to the conditionTreeEvaluator.
+ * Evaluates the condition provided given the user attributes if an evaluator has been defined for the condition type.
+ * @param  {Object} userAttributes     A map of user attributes.
+ * @param  {Object} condition          A single condition object to evaluate.
+ * @return {Boolean|null}              true if the condition is satisfied, null if a matcher is not found.
+ */
+AudienceEvaluator.prototype.evaluateConditionWithUserAttributes = function(userAttributes, condition) {
+  var evaluator = this.typeToEvaluatorMap[condition.type];
+  if (!evaluator) {
+    this.logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));
+    return null;
+  }
+  try {
+    return evaluator.evaluate(condition, userAttributes, this.logger);
+  } catch (err) {
+    this.logger.log(LOG_LEVEL.ERROR, sprintf(ERROR_MESSAGES.CONDITION_EVALUATOR_ERROR, MODULE_NAME, condition.type, err.message));
+  }
+  return null;
+};
+
 module.exports = AudienceEvaluator;

packages/optimizely-sdk/lib/core/audience_evaluator/index.tests.js

@@ -15,7 +15,6 @@
  */
 var AudienceEvaluator = require('./');
 var chai = require('chai');
-var sprintf = require('sprintf-js').sprintf;
 var conditionTreeEvaluator = require('../condition_tree_evaluator');
 var customAttributeConditionEvaluator = require('../custom_attribute_condition_evaluator');
 var sinon = require('sinon');
@@ -54,14 +53,13 @@ var audiencesById = {
 
 describe('lib/core/audience_evaluator', function() {
   var audienceEvaluator;
+  var mockLogger = logger.createLogger({logLevel: LOG_LEVEL.INFO});
   beforeEach(function() {
-    audienceEvaluator = new AudienceEvaluator();
+    audienceEvaluator = new AudienceEvaluator(mockLogger);
   });
 
   describe('APIs', function() {
     describe('evaluate', function() {
-      var mockLogger = logger.createLogger({logLevel: LOG_LEVEL.INFO});
-
       beforeEach(function () {
         sinon.stub(mockLogger, 'log');
       });
@@ -71,11 +69,11 @@ describe('lib/core/audience_evaluator', function() {
       });
 
       it('should return true if there are no audiences', function() {
-        assert.isTrue(audienceEvaluator.evaluate([], audiencesById, {}, mockLogger));
+        assert.isTrue(audienceEvaluator.evaluate([], audiencesById, {}));
       });
 
       it('should return false if there are audiences but no attributes', function() {
-        assert.isFalse(audienceEvaluator.evaluate(['0'], audiencesById, {}, mockLogger));
+        assert.isFalse(audienceEvaluator.evaluate(['0'], audiencesById, {}));
       });
 
       it('should return true if any of the audience conditions are met', function() {
@@ -92,9 +90,9 @@ describe('lib/core/audience_evaluator', function() {
           'device_model': 'iphone',
         };
 
-        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneUsers, mockLogger));
-        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, chromeUsers, mockLogger));
-        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneChromeUsers, mockLogger));
+        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneUsers));
+        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, chromeUsers));
+        assert.isTrue(audienceEvaluator.evaluate(['0', '1'], audiencesById, iphoneChromeUsers));
       });
 
       it('should return false if none of the audience conditions are met', function() {
@@ -111,13 +109,13 @@ describe('lib/core/audience_evaluator', function() {
           'device_model': 'nexus5',
         };
 
-        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusUsers, mockLogger));
-        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, safariUsers, mockLogger));
-        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusSafariUsers, mockLogger));
+        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusUsers));
+        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, safariUsers));
+        assert.isFalse(audienceEvaluator.evaluate(['0', '1'], audiencesById, nexusSafariUsers));
       });
 
       it('should return true if no attributes are passed and the audience conditions evaluate to true in the absence of attributes', function() {
-        assert.isTrue(audienceEvaluator.evaluate(['2'], audiencesById, null, mockLogger));
+        assert.isTrue(audienceEvaluator.evaluate(['2'], audiencesById, null));
       });
 
       describe('complex audience conditions', function() {
@@ -204,9 +202,9 @@ describe('lib/core/audience_evaluator', function() {
           });
           customAttributeConditionEvaluator.evaluate.returns(false);
           var userAttributes = { device_model: 'android' };
-          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
+          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
           sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
-          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
+          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
           assert.isFalse(result);
         });
       });
@@ -229,9 +227,9 @@ describe('lib/core/audience_evaluator', function() {
           });
           customAttributeConditionEvaluator.evaluate.returns(null);
           var userAttributes = { device_model: 5.5 };
-          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
+          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
           sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
-          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
+          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
           assert.isFalse(result);
           assert.strictEqual(2, mockLogger.log.callCount);
           assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');
@@ -244,9 +242,9 @@ describe('lib/core/audience_evaluator', function() {
           });
           customAttributeConditionEvaluator.evaluate.returns(true);
           var userAttributes = { device_model: 'iphone' };
-          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
+          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
           sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
-          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
+          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
           assert.isTrue(result);
           assert.strictEqual(2, mockLogger.log.callCount);
           assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');
@@ -259,9 +257,9 @@ describe('lib/core/audience_evaluator', function() {
           });
           customAttributeConditionEvaluator.evaluate.returns(false);
           var userAttributes = { device_model: 'android' };
-          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes, mockLogger);
+          var result = audienceEvaluator.evaluate(['or', '1'], audiencesById, userAttributes);
           sinon.assert.calledOnce(customAttributeConditionEvaluator.evaluate);
-          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes, mockLogger);
+          sinon.assert.calledWithExactly(customAttributeConditionEvaluator.evaluate, iphoneUserAudience.conditions[1], userAttributes);
           assert.isFalse(result);
           assert.strictEqual(2, mockLogger.log.callCount);
           assert.strictEqual(mockLogger.log.args[0][1], 'AUDIENCE_EVALUATOR: Starting to evaluate audience "1" with conditions: ["and",{"name":"device_model","value":"iphone","type":"custom_attribute"}].');

packages/optimizely-sdk/lib/core/custom_attribute_condition_evaluator/index.js

@@ -56,7 +56,6 @@ EVALUATORS_BY_MATCH_TYPE[SUBSTRING_MATCH_TYPE] = substringEvaluator;
  */
 function evaluate(condition, userAttributes, logger) {
   if (condition.type !== CUSTOM_ATTRIBUTE_CONDITION_TYPE) {
-    logger.log(LOG_LEVEL.WARNING, sprintf(LOG_MESSAGES.UNKNOWN_CONDITION_TYPE, MODULE_NAME, JSON.stringify(condition)));
     return null;
   }
 

packages/optimizely-sdk/lib/core/decision_service/index.js

@@ -52,7 +52,7 @@ function DecisionService(options) {
   this.configObj = options.configObj;
   this.userProfileService = options.userProfileService || null;
   this.logger = options.logger;
-  this.audienceEvaluator = new AudienceEvaluator(options.conditionEvaluators);
+  this.audienceEvaluator = new AudienceEvaluator(options.logger, options.__exploratoryConditionEvaluators);
 }
 
 /**
@@ -169,7 +169,7 @@ DecisionService.prototype.__checkIfUserIsInAudience = function(experimentKey, us
   var experimentAudienceConditions = projectConfig.getExperimentAudienceConditions(this.configObj, experimentKey);
   var audiencesById = projectConfig.getAudiencesById(this.configObj);
   this.logger.log(LOG_LEVEL.DEBUG, sprintf(LOG_MESSAGES.EVALUATING_AUDIENCES_COMBINED, MODULE_NAME, experimentKey, JSON.stringify(experimentAudienceConditions)));
-  var result = this.audienceEvaluator.evaluate(experimentAudienceConditions, audiencesById, attributes, this.logger);
+  var result = this.audienceEvaluator.evaluate(experimentAudienceConditions, audiencesById, attributes);
   this.logger.log(LOG_LEVEL.INFO, sprintf(LOG_MESSAGES.AUDIENCE_EVALUATION_RESULT_COMBINED, MODULE_NAME, experimentKey, result.toString().toUpperCase()));
 
   if (!result) {

packages/optimizely-sdk/lib/optimizely/index.js

@@ -98,7 +98,7 @@ function Optimizely(config) {
     configObj: this.configObj,
     userProfileService: userProfileService,
     logger: this.logger,
-    conditionEvaluators: config.conditionEvaluators
+    __exploratoryConditionEvaluators: config.__exploratoryConditionEvaluators
   });
 
   this.notificationCenter = notificationCenter.createNotificationCenter({

packages/optimizely-sdk/lib/optimizely/index.tests.js

@@ -238,7 +238,7 @@ describe('lib/optimizely', function() {
             configObj: optlyInstance.configObj,
             userProfileService: userProfileServiceInstance,
             logger: createdLogger,
-            conditionEvaluators: undefined
+            __exploratoryConditionEvaluators: undefined
           });
 
           // Checking the second log message as the first one just says "Datafile is valid"
@@ -263,7 +263,7 @@ describe('lib/optimizely', function() {
             configObj: optlyInstance.configObj,
             userProfileService: null,
             logger: createdLogger,
-            conditionEvaluators: undefined
+            __exploratoryConditionEvaluators: undefined
           });
 
           // Checking the second log message as the first one just says "Datafile is valid"

packages/optimizely-sdk/lib/utils/enums/index.js

@@ -26,7 +26,7 @@ exports.LOG_LEVEL = {
 };
 
 exports.ERROR_MESSAGES = {
-  AUDIENCE_EVALUATOR_EXPECTED: '%s: No audience evaluator present for condition type: %s.',
+  CONDITION_EVALUATOR_ERROR: '%s: Error evaluating condition type %s: %s',
   EXPERIMENT_KEY_NOT_IN_DATAFILE: '%s: Experiment key %s is not in datafile.',
   FEATURE_NOT_IN_DATAFILE: '%s: Feature key %s is not in datafile.',
   IMPROPERLY_FORMATTED_EXPERIMENT: '%s: Experiment key %s is improperly formatted.',