Update session storing to mitigate client screwyness

bench/head-to-head.js

@@ -14,21 +14,21 @@ var CRStore = require('connect-redis')({ Store: function () {} });
 var crstore = new CRStore({ client: client, ttl: 60 });
 var smartstore = new SmartStore({ client: client, ttl: 60 });
 
-var session = new (require('../lib/session'))({ a: 1, b: 3,c: [1,2,3] });
+var session = require('../lib/session').attach({ a: 1, b: 3,c: [1,2,3] });
 
 function noop () {}
         crstore.get('foo', noop);
         crstore.set('foo', { cookie: {}, a: 1, b: 3,c: [1,2,3] }, noop);
 
 new Benchmark.Suite()
-    .add('connect-redis', function () {
-        crstore.get('foo', noop);
-        crstore.set('foo', { cookie: {}, a: 1, b: 3,c: [1,2,3] }, noop);
-    })
     .add('connect-smart-redis', function () {
         smartstore.get('foo', noop);
         smartstore.set('foo', session, noop);
     })
+    .add('connect-redis', function () {
+        crstore.get('foo', noop);
+        crstore.set('foo', { cookie: {}, a: 1, b: 3,c: [1,2,3] }, noop);
+    })
     .on('cycle', function(event) {
       console.log(String(event.target));
     })

lib/index.js

@@ -4,6 +4,12 @@ module.exports = function (connect) {
 
     var ConnectStore = connect.Store || connect.session.Store;
 
+    /**
+     * Key where we attach the Session object on the plain session literal.
+     * @type {String}
+     */
+    var attachKey = '_';
+
     /**
      * Store instance for managing Redis sessions.
      *
@@ -77,7 +83,9 @@ module.exports = function (connect) {
                 } catch (e) {}
             }
 
-            callback(undefined, new Session(results, data === Store.DESTROYED));
+            // Add the session object into the results.
+            Session.attach(results, data === Store.DESTROYED);
+            callback(undefined, results);
         });
     };
 
@@ -135,7 +143,7 @@ module.exports = function (connect) {
 
     /**
      * Re-retrieves the session from Redis, applies the changes that
-     * were made locally, then saves the session back into Redis.
+     * were made locally, then sends it to be saved in Redis.
      * @param  {String}   id
      * @param  {Session}  session
      * @param  {Function} callback
@@ -145,40 +153,59 @@ module.exports = function (connect) {
 
         this.get(id, function (err, result) {
             if (err) return callback(err);
+            var other = result[attachKey];
 
             // If what we got most recently was destroyed, but the original
             // session was not from a destroyed session, then the session
             // was just destroyed and we should not persist data.
-            if (result.isFromDestroyed() && !session.isFromDestroyed()) {
+            if (other.isFromDestroyed() && !session.isFromDestroyed()) {
                 return callback();
             }
 
-            var toSave = JSON.stringify(result.applyChanges(session));
-            self.client.SETEX([ self.getKey(id), self.ttl, toSave ], callback);
+            self.setex(id, other.applyChanges(session), callback);
         });
     };
 
+    /**
+     * Saves the session straight into Redis.
+     * @param  {String}   id
+     * @param  {Object}   data
+     * @param  {Function} callback
+     */
+    Store.prototype.setex = function (id, data, callback) {
+        this.client.SETEX([ this.getKey(id), this.ttl, JSON.stringify(data) ], callback);
+    };
+
     /**
      * Persists the session into Redis.
      * @param {String}   id
      * @param {Session}   session
      * @param {Function} callback
      */
-    Store.prototype.set = function (id, session, callback) {
-        // A new session won't be pulled from the store, it'll
-        // just be a plain object.
-        if (!(session instanceof Session)) {
-            session = new Session(session, true);
+    Store.prototype.set = function (id, data, callback) {
+        // If we don't find an attached session, just save it if there's
+        // anything worth saving.
+        var session = data._;
+        if (!session) {
+            data = Session.prototype.trim(data);
+            if (Object.keys(data).length > 0) {
+                return this.setex(id, data, callback);
+            } else {
+                return callback();
+            }
         }
 
+        // Set the original data to compare to.
+        session.setAgainst(data);
+
         // Send of "destroyed" sessions to the destroy method.
         if (session.isDestroyed()) {
             return this.destroy(id, callback);
         }
 
         // Don't do anything for sessions that aren't persisted,
         // or have not changed.
-        if (session.isForgotten() || !session.hasChanged()) {
+        if (session.isForgotten() || !session.hasChanged(data)) {
             return callback();
         }
 

lib/session.js

@@ -2,138 +2,187 @@ var patcher = require('fast-json-patch');
 var util = require('./util');
 
 /**
- * Object used for storing session data. Provides several useful methods,
- * like .destroy().
+ * List of method names that will be "hoisted" at attached to the main
+ * session object when it's created.
+ * @type {String[]}
+ */
+var hoist = ['destroy', 'forget'];
+
+/**
+ * List of keys to exclude from session savings.
+ * @type {String[]}
+ */
+var exclude = hoist.concat(['_', 'cookie']);
+
+/**
+ * Object used for storing session data. We copy it as an original
+ * object as well as provide several useful methods, like "destroy".
  * @param {Object} data
  * @param {Boolean} fromDestroyed Whether the session was created when the
  *                                key was set to DESTROYED
  */
 function Session (data, fromDestroyed) {
-    // Define a `_` property to store some metadata on.
-    this._ = {
-        fromDestroyed: !!fromDestroyed,
-        destroyed: false,
-        persisting: true,
-        original: fromDestroyed ? {} : util.clone(data),
-        patch: null
-    };
-
-    this.cookie = {};
-
-    for (var key in data) {
-        this[key] = data[key];
+    this.fromDestroyed = !!fromDestroyed;
+    this.destroyed = false;
+    this.persisting = true;
+    this.original = fromDestroyed ? {} : util.clone(data);
+    this.patch = null;
+    this.against = null;
+
+    for (var i = 0; i < hoist.length; i++) {
+        data[hoist[i]] = util.bind(this, this[hoist[i]]);
     }
+
+    data.cookie = {};
+    data._ = this;
 }
 
+var methods = {};
+
 /**
  * Sets the session to be destroyed when we go to save it.
  */
-Session.prototype.destroy = function () {
-    this._.destroyed = true;
+methods.destroy = function () {
+    this.destroyed = true;
 };
 
 /**
  * Does not persist changes made to the session.
  */
-Session.prototype.forget = function () {
-    this._.persisting = false;
+methods.forget = function () {
+    this.persisting = false;
 };
 
 /**
  * Returns if the session is "forgotten"
  * @return {Boolean}
  */
-Session.prototype.isForgotten = function () {
-    return !this._.persisting;
+methods.isForgotten = function () {
+    return !this.persisting;
 };
 
 /**
  * Return whether we want to destroy this session.
  * @return {Boolean}
  */
-Session.prototype.isDestroyed = function () {
-    return this._.destroyed;
+methods.isDestroyed = function () {
+    return this.destroyed;
 };
 
 /**
  * Return whether the session was DESTROYED when this one was created.
  * @return {Boolean}
  */
-Session.prototype.isFromDestroyed = function () {
-    return this._.fromDestroyed;
+methods.isFromDestroyed = function () {
+    return this.fromDestroyed;
 };
 
 /**
  * Returns the original data the session was loaded with.
  * @return {Object}
  */
-Session.prototype.getOriginal = function () {
-    return this._.original;
+methods.getOriginal = function () {
+    return this.original;
 };
 
 /**
- * Returns whether this session's data has been
- * modified since it was created.
+ * Gets the changes made in the data against the original session
+ * information stored on this object.
  *
- * Note: calling this will cause further changes to not be recorded;
- * it should only be called when we're done and ready to save the session.
+ * Note: this function is stateful and should only be called when
+ * we're done and ready to save the session.
  *
+ * @param {Object} against The data to compare against
  * @return {Boolean}
  */
-Session.prototype.getChanges = function () {
-    var patch = this._.patch;
+methods.getChanges = function () {
+    var patch = this.patch;
 
     if (patch === null) {
-        patch = this._.patch = patcher.compare(this.getOriginal(), this.toObject());
+        patch = this.patch = patcher.compare(this.original, this.against);
     }
 
     return patch;
 };
 
+/**
+ * Sets the updated session to compare against at the end of the
+ * lifecycle.
+ *
+ * Note: this function is stateful and should only be called when
+ * we're done and ready to save the session.
+ *
+ * @param {Object} against
+ */
+methods.setAgainst = function (against) {
+    this.against = this.trim(against);
+};
+
 /**
  * Returns whether any changes were made to the session.
  *
- * Note: calling this will cause further changes to not be recorded;
- * it should only be called when we're done and ready to save the session.
+ * Note: this function is stateful and should only be called when
+ * we're done and ready to save the session.
  *
  * @return {Boolean}
  */
-Session.prototype.hasChanged = function () {
+methods.hasChanged = function () {
     return this.isFromDestroyed() || this.getChanges().length > 0;
 };
 
 /**
- * Returns a plain object without methods or helpers, for the current
- * session data.
+ * Returns a plain object without methods or helpers, for the session data.
+ * @param {Object} data
  * @return {Object}
  */
-Session.prototype.toObject = function () {
-    var exclude = Object.keys(Session.prototype).concat(['_', 'cookie']);
+methods.trim = function (data) {
     var output = {};
 
-    for (var key in this) {
+    for (var key in data) {
         // Exclude things from the prototype and additional helper functions
         // that various middleware attach.
-        if (exclude.indexOf(key) === -1 && typeof this[key] !== 'function') {
-            output[key] = this[key];
+        if (exclude.indexOf(key) === -1 && typeof data[key] !== 'function') {
+            output[key] = data[key];
         }
     }
 
     return output;
 };
 
 /**
- * Applies the changes made in another session onto this one.
+ * Applies the changes made in this session to another one. It's expected that
+ * prior to calling this method, hasChanged was called.
  * @param  {Session} session
  * @return {Session}
  */
-Session.prototype.applyChanges = function (session) {
+methods.applyChanges = function (session) {
     var changes = session.getChanges();
-    var output = this.toObject();
+    var output = this.trim(this.original);
 
     patcher.apply(output, changes);
 
     return output;
 };
 
+// Define the methods to be non-enumerable, so that if the session gets
+// cloned these are kept clean and away.
+for (var key in methods) {
+    Object.defineProperty(Session.prototype, key, {
+        value: methods[key]
+    });
+}
+
+/**
+ * Wrapper method that attaches a new Session to the results plain object.
+ * @param  {Object} data
+ * @param  {Bool} fromDestroyed
+ * @return {Object}
+ */
+Object.defineProperty(Session, 'attach', {
+    value: function (data, fromDestroyed) {
+        new Session(data, fromDestroyed);
+        return data;
+    }
+});
+
 module.exports = Session;

lib/util.js

@@ -19,3 +19,15 @@ util.clone = function (data) {
         return output;
     }
 };
+
+/**
+ * Binds the context of a function and returns it.
+ * @param  {*}   self
+ * @param  {Function} fn
+ * @return {Function}
+ */
+util.bind = function (self, fn) {
+    return function () {
+        fn.apply(self, arguments);
+    };
+};