Merge pull request #130 from launchdarkly/eb/ch35206/stringify-attrs

stringify built-in user attributes in events, and secondary key for evals

evaluate_flag.js

@@ -3,8 +3,12 @@ var dataKind = require('./versioned_data_kind');
 var util = require('util');
 var sha1 = require('node-sha1');
 var async = require('async');
+var stringifyAttrs = require('./utils/stringifyAttrs');
 
 var builtins = ['key', 'ip', 'country', 'email', 'firstName', 'lastName', 'avatar', 'name', 'anonymous'];
+var userAttrsToStringifyForEvaluation = [ "key", "secondary" ];
+// Currently we are not stringifying the rest of the built-in attributes prior to evaluation, only for events.
+// This is because it could affect evaluation results for existing users (ch35206).
 
 var noop = function(){};
 
@@ -22,8 +26,9 @@ function evaluate(flag, user, featureStore, cb) {
     return;
   }
 
+  var sanitizedUser = stringifyAttrs(user, userAttrsToStringifyForEvaluation);
   var events = [];
-  evalInternal(flag, user, featureStore, events, function(err, detail) {
+  evalInternal(flag, sanitizedUser, featureStore, events, function(err, detail) {
     cb(err, detail, events);
   });
 }

event_processor.js

@@ -4,8 +4,11 @@ var EventSummarizer = require('./event_summarizer');
 var UserFilter = require('./user_filter');
 var errors = require('./errors');
 var messages = require('./messages');
+var stringifyAttrs = require('./utils/stringifyAttrs');
 var wrapPromiseCallback = require('./utils/wrapPromiseCallback');
 
+var userAttrsToStringifyForEvents = [ "key", "secondary", "ip", "country", "email", "firstName", "lastName", "avatar", "name" ];
+
 function EventProcessor(sdkKey, config, errorReporter) {
   var ep = {};
 
@@ -63,17 +66,17 @@ function EventProcessor(sdkKey, config, errorReporter) {
           out.reason = event.reason;
         }
         if (config.inlineUsersInEvents || debug) {
-          out.user = userFilter.filterUser(event.user);
+          out.user = processUser(event);
         } else {
-          out.userKey = event.user && event.user.key;
+          out.userKey = getUserKey(event);
         }
         return out;
       case 'identify':
         return {
           kind: 'identify',
           creationDate: event.creationDate,
-          key: event.user && event.user.key,
-          user: userFilter.filterUser(event.user)
+          key: getUserKey(event),
+          user: processUser(event)
         };
       case 'custom':
         var out = {
@@ -83,16 +86,25 @@ function EventProcessor(sdkKey, config, errorReporter) {
           data: event.data
         };
         if (config.inlineUsersInEvents) {
-          out.user = userFilter.filterUser(event.user);
+          out.user = processUser(event);
         } else {
-          out.userKey = event.user && event.user.key;
+          out.userKey = getUserKey(event);
         }
         return out;
       default:
         return event;
     }
   }
 
+  function processUser(event) {
+    var filtered = userFilter.filterUser(event.user);
+    return stringifyAttrs(filtered, userAttrsToStringifyForEvents);
+  }
+
+  function getUserKey(event) {
+    return event.user && String(event.user.key);
+  }
+
   ep.sendEvent = function(event) {
     var addIndexEvent = false,
         addFullEvent = false,
@@ -129,7 +141,7 @@ function EventProcessor(sdkKey, config, errorReporter) {
       enqueue({
         kind: 'index',
         creationDate: event.creationDate,
-        user: userFilter.filterUser(event.user)
+        user: processUser(event)
       });
     }
     if (addFullEvent) {

index.js

@@ -213,7 +213,6 @@ var newClient = function(sdkKey, config) {
       return resolve(errorResult('FLAG_NOT_FOUND', defaultVal));
     }
 
-    sanitizeUser(user);
     if (user && user.key === "") {
       config.logger.warn("User key is blank. Flag evaluation will proceed, but the user will not be stored in LaunchDarkly");
     }
@@ -286,8 +285,6 @@ var newClient = function(sdkKey, config) {
       options = options || {};
     }
     return wrapPromiseCallback(new Promise(function(resolve, reject) {
-      sanitizeUser(user);
-
       if (this.isOffline()) {
         config.logger.info("allFlagsState() called in offline mode. Returning empty state.");
         return resolve(FlagsStateBuilder(false).build());
@@ -347,7 +344,6 @@ var newClient = function(sdkKey, config) {
       return;
     }
 
-    sanitizeUser(user);
     var event = {"key": eventName,
                 "user": user,
                 "kind": "custom",
@@ -366,8 +362,7 @@ var newClient = function(sdkKey, config) {
       return;
     }
 
-    sanitizeUser(user);
-    var event = {"key": user.key,
+    var event = {"key": String(user.key),
                  "kind": "identify",
                  "user": user,
                  "creationDate": new Date().getTime()};
@@ -434,13 +429,3 @@ function createProxyAgent(config) {
     return tunnel.httpOverHttp(options);
   }
 }
-
-
-function sanitizeUser(u) {
-  if (!u) {
-    return;
-  }
-  if (u['key']) {
-    u['key'] = u['key'].toString();
-  }
-}

test/evaluate_flag-test.js

@@ -22,12 +22,6 @@ function defineSegment(segment, cb) {
   setTimeout(cb, 0);
 }
 
-function evalBooleanFlag(flag, user, cb) {
-  evaluate.evaluate(flag, user, featureStore, function(err, result) {
-    cb(result);
-  });
-}
-
 function makeFlagWithRules(rules, fallthrough) {
   if (!fallthrough) {
     fallthrough = { variation: 0 };
@@ -43,19 +37,23 @@ function makeFlagWithRules(rules, fallthrough) {
   };
 }
 
-function makeBooleanFlagWithOneClause(clause) {
+function makeBooleanFlagWithRules(rules) {
   return {
-      key: 'feature',
-      on: true,
-      prerequisites: [],
-      rules: [ { clauses: [ clause ], variation: 1 } ],
-      targets: [],
-      salt: "",
-      fallthrough: { variation: 0 },
-      offVariation: 0,
-      variations: [ false, true ],
-      version: 1
-    };
+    key: 'feature',
+    on: true,
+    prerequisites: [],
+    rules: rules,
+    targets: [],
+    salt: "",
+    fallthrough: { variation: 0 },
+    offVariation: 0,
+    variations: [ false, true ],
+    version: 1
+  };
+}
+
+function makeBooleanFlagWithOneClause(clause) {
+  return makeBooleanFlagWithRules([ { clauses: [ clause ], variation: 1 } ]);
 }
 
 function makeFlagWithSegmentMatch(segment) {
@@ -372,6 +370,37 @@ describe('evaluate', function() {
     });
   });
 
+  it('coerces user key to string', function(done) {
+    var clause = { 'attribute': 'key', 'op': 'in', 'values': [ '999' ] };
+    var flag = makeBooleanFlagWithOneClause(clause);
+    var user = { 'key': 999 };
+    evaluate.evaluate(flag, user, featureStore, function(err, detail, events) {
+      expect(detail.value).toBe(true);
+      done();
+    });
+  });
+
+  it('coerces secondary key to string', function(done) {
+    // We can't really verify that the rollout calculation works correctly, but we can at least
+    // make sure it doesn't error out if there's a non-string secondary value (ch35189)
+    var rule = {
+      id: 'ruleid',
+      clauses: [
+          { attribute: 'key', op: 'in', values: [ 'userkey' ] }
+      ],
+      rollout: {
+          salt:  '',
+          variations: [ { weight: 100000, variation: 1 } ]
+      }
+    };
+    var flag = makeBooleanFlagWithRules([ rule ]);
+    var user = { key: 'userkey', secondary: 999 };
+    evaluate.evaluate(flag, user, featureStore, function(err, detail, events) {
+      expect(detail.value).toBe(true);
+      done();
+    });
+  });
+
   it('matches user from targets', function(done) {
     var flag = {
       key: 'feature0',

test/event_processor-test.js

@@ -1,6 +1,5 @@
 var nock = require('nock');
 var EventProcessor = require('../event_processor');
-var EventEmitter = require('events').EventEmitter;
 
 describe('EventProcessor', function() {
 
@@ -20,6 +19,10 @@ describe('EventProcessor', function() {
   };
   var user = { key: 'userKey', name: 'Red' };
   var filteredUser = { key: 'userKey', privateAttrs: [ 'name' ] };
+  var numericUser = { key: 1, secondary: 2, ip: 3, country: 4, email: 5, firstName: 6, lastName: 7,
+    avatar: 8, name: 9, anonymous: false, custom: { age: 99 } };
+  var stringifiedNumericUser = { key: '1', secondary: '2', ip: '3', country: '4', email: '5', firstName: '6',
+    lastName: '7', avatar: '8', name: '9', anonymous: false, custom: { age: 99 } };
 
   afterEach(function() {
     if (ep) {
@@ -70,7 +73,7 @@ describe('EventProcessor', function() {
     if (inlineUser) {
       expect(e.user).toEqual(inlineUser);
     } else {
-      expect(e.userKey).toEqual(source.user.key);
+      expect(e.userKey).toEqual(String(source.user.key));
     }
   }
 
@@ -123,6 +126,22 @@ describe('EventProcessor', function() {
     });
   });
 
+  it('stringifies user attributes in identify event', function(done) {
+    ep = EventProcessor(sdkKey, defaultConfig);
+    var e = { kind: 'identify', creationDate: 1000, user: numericUser };
+    ep.sendEvent(e);
+
+    flushAndGetRequest(function(output) {
+      expect(output).toEqual([{
+        kind: 'identify',
+        creationDate: 1000,
+        key: stringifiedNumericUser.key,
+        user: stringifiedNumericUser
+      }]);
+      done();
+    });
+  });
+
   it('queues individual feature event with index event', function(done) {
     ep = EventProcessor(sdkKey, defaultConfig);
     var e = { kind: 'feature', creationDate: 1000, user: user, key: 'flagkey',
@@ -154,6 +173,21 @@ describe('EventProcessor', function() {
     });
   });
 
+  it('stringifies user attributes in index event', function(done) {
+    ep = EventProcessor(sdkKey, defaultConfig);
+    var e = { kind: 'feature', creationDate: 1000, user: numericUser, key: 'flagkey',
+      version: 11, variation: 1, value: 'value', trackEvents: true };
+    ep.sendEvent(e);
+
+    flushAndGetRequest(function(output) {
+      expect(output.length).toEqual(3);
+      checkIndexEvent(output[0], e, stringifiedNumericUser);
+      checkFeatureEvent(output[1], e, false);
+      checkSummaryEvent(output[2]);
+      done();
+    });
+  });
+
   it('can include inline user in feature event', function(done) {
     var config = Object.assign({}, defaultConfig, { inlineUsersInEvents: true });
     ep = EventProcessor(sdkKey, config);
@@ -185,6 +219,21 @@ describe('EventProcessor', function() {
     });
   });
 
+  it('stringifies user attributes in feature event', function(done) {
+    var config = Object.assign({}, defaultConfig, { inlineUsersInEvents: true });
+    ep = EventProcessor(sdkKey, config);
+    var e = { kind: 'feature', creationDate: 1000, user: numericUser, key: 'flagkey',
+      version: 11, variation: 1, value: 'value', trackEvents: true };
+    ep.sendEvent(e);
+
+    flushAndGetRequest(function(output) {
+      expect(output.length).toEqual(2);
+      checkFeatureEvent(output[0], e, false, stringifiedNumericUser);
+      checkSummaryEvent(output[1]);
+      done();
+    });
+  });
+
   it('can include reason in feature event', function(done) {
     var config = Object.assign({}, defaultConfig, { inlineUsersInEvents: true });
     ep = EventProcessor(sdkKey, config);
@@ -378,6 +427,20 @@ describe('EventProcessor', function() {
     });
   });
 
+  it('stringifies user attributes in custom event', function(done) {
+    var config = Object.assign({}, defaultConfig, { inlineUsersInEvents: true });
+    ep = EventProcessor(sdkKey, config);
+    var e = { kind: 'custom', creationDate: 1000, user: numericUser, key: 'eventkey',
+      data: { thing: 'stuff' } };
+    ep.sendEvent(e);
+
+    flushAndGetRequest(function(output) {
+      expect(output.length).toEqual(1);
+      checkCustomEvent(output[0], e, stringifiedNumericUser);
+      done();
+    });
+  });
+
   it('filters user in custom event', function(done) {
     var config = Object.assign({}, defaultConfig, { allAttributesPrivate: true,
       inlineUsersInEvents: true });

utils/stringifyAttrs.js

@@ -0,0 +1,16 @@
+
+module.exports = function stringifyAttrs(object, attrs) {
+  if (!object) {
+    return object;
+  }
+  var newObject;
+  for (var i in attrs) {
+    var attr = attrs[i];
+    var value = object[attr];
+    if (value !== undefined && typeof value !== 'string') {
+      newObject = newObject || Object.assign({}, object);
+      newObject[attr] = String(value);
+    }
+  }
+  return newObject || object;
+}