Review markups

This change:

  - bypasses `_sanitizeOps` and instead uses `_sanitizeSnapshot`
  - adds timing to `Backend.fetchSnapshot`
  - throws for negative versions

Author: Alec Gibson

lib/backend.js

@@ -7,6 +7,7 @@ var MemoryPubSub = require('./pubsub/memory');
 var ot = require('./ot');
 var projections = require('./projections');
 var QueryEmitter = require('./query-emitter');
+var Snapshot = require('./snapshot');
 var StreamSocket = require('./stream-socket');
 var SubmitRequest = require('./submit-request');
 var types = require('./types');
@@ -583,35 +584,39 @@ Backend.prototype.getChannels = function(collection, id) {
 };
 
 Backend.prototype.fetchSnapshot = function(agent, index, id, version, callback) {
+  var start = Date.now();
   var backend = this;
-  this._fetchSnapshot(agent, index, id, version, function (error, snapshot) {
-    if (error) return callback(error);
-
-    var request = {
-      collection: index,
-      id: id,
-      v: snapshot.v,
-      snapshots: snapshot.data ? [snapshot.data] : [],
-      type: snapshot.type
-    };
+  var projection = this.projections[index];
+  var collection = projection ? projection.target : index;
+  var request = {
+    agent: agent,
+    index: index,
+    collection: collection,
+    id: id,
+    version: version
+  };
 
-    backend.trigger(backend.MIDDLEWARE_ACTIONS.readSnapshots, agent, request, function (error) {
+  this._fetchSnapshot(collection, id, version, function (error, snapshot) {
+    if (error) return callback(error);
+    var snapshotProjection = backend._getSnapshotProjection(backend.db, projection);
+    var snapshots = [snapshot];
+    backend._sanitizeSnapshots(agent, snapshotProjection, collection, snapshots, function (error) {
       if (error) return callback(error);
-      callback(null, {
-        data: request.snapshots[0],
-        v: request.v,
-        type: request.type
-      });
+      backend.emit('timing', 'fetchSnapshot', Date.now() - start, request);
+      callback(null, snapshot);
     });
   });
 };
 
-Backend.prototype._fetchSnapshot = function (agent, index, id, version, callback) {
-  this.getOps(agent, index, id, 0, version, function (error, ops) {
+Backend.prototype._fetchSnapshot = function (collection, id, version, callback) {
+  // Bypass backend.getOps so that we don't call _sanitizeOps. We want to avoid this, because:
+  // - we want to avoid the 'op' middleware, because we later use the 'readSnapshots' middleware in _sanitizeSnapshots
+  // - we handle the projection in _sanitizeSnapshots
+  this.db.getOps(collection, id, 0, version, null, function (error, ops) {
     if (error) return callback(error);
 
     var type = null;
-    var snapshot;
+    var data;
     var fetchedVersion = 0;
 
     for (var index = 0; index < ops.length; index++) {
@@ -621,12 +626,12 @@ Backend.prototype._fetchSnapshot = function (agent, index, id, version, callback
       if (op.create) {
         type = types.map[op.create.type];
         if (!type) return callback({ code: 4008, message: 'Unknown type' });
-        snapshot = type.create(op.create.data);
+        data = type.create(op.create.data);
       } else if (op.del) {
-        snapshot = undefined;
+        data = undefined;
         type = null;
       } else {
-        snapshot = type.apply(snapshot, op.op);
+        data = type.apply(data, op.op);
       }
     }
 
@@ -636,11 +641,8 @@ Backend.prototype._fetchSnapshot = function (agent, index, id, version, callback
       return callback({ code: 4024, message: 'Requested version exceeds latest snapshot version' });
     }
 
-    callback(null, {
-      data: snapshot,
-      v: fetchedVersion,
-      type: type
-    });
+    var snapshot = new Snapshot(id, fetchedVersion, type, data, null);
+    callback(null, snapshot);
   });
 };
 

lib/client/connection.js

@@ -232,7 +232,7 @@ Connection.prototype.handleMessage = function(message) {
       return this._handleBulkMessage(message, '_handleUnsubscribe');
 
     case 'nf':
-      return this._handleSnapshot(err, message);
+      return this._handleSnapshotFetch(err, message);
 
     case 'f':
       var doc = this.getExisting(message.c, message.d);
@@ -537,15 +537,12 @@ Connection.prototype.hasPending = function() {
   return !!(
     this._firstDoc(hasPending) ||
     this._firstQuery(hasPending) ||
-    this._firstSnapshotRequest(exists)
+    this._firstSnapshotRequest()
   );
 };
 function hasPending(object) {
   return object.hasPending();
 }
-function exists(object) {
-  return !!object;
-}
 
 Connection.prototype.hasWritePending = function() {
   return !!this._firstDoc(hasWritePending);
@@ -569,7 +566,7 @@ Connection.prototype.whenNothingPending = function(callback) {
     query.once('ready', this._nothingPendingRetry(callback));
     return;
   }
-  var snapshotRequest = this._firstSnapshotRequest(exists);
+  var snapshotRequest = this._firstSnapshotRequest();
   if (snapshotRequest) {
     snapshotRequest.once('ready', this._nothingPendingRetry(callback));
     return;
@@ -607,12 +604,9 @@ Connection.prototype._firstQuery = function(fn) {
   }
 };
 
-Connection.prototype._firstSnapshotRequest = function (fn) {
+Connection.prototype._firstSnapshotRequest = function () {
   for (var id in this._snapshotRequests) {
-    var snapshotRequest = this._snapshotRequests[id];
-    if (fn(snapshotRequest)) {
-      return snapshotRequest;
-    }
+    return this._snapshotRequests[id];
   }
 };
 
@@ -644,7 +638,7 @@ Connection.prototype.fetchSnapshot = function(collection, id, version, callback)
   snapshotRequest.send();
 };
 
-Connection.prototype._handleSnapshot = function (error, message) {
+Connection.prototype._handleSnapshotFetch = function (error, message) {
   var snapshotRequest = this._snapshotRequests[message.id];
   if (!snapshotRequest) return;
   delete this._snapshotRequests[message.id];

lib/client/snapshot-request.js

@@ -11,21 +11,33 @@ function SnapshotRequest(connection, requestId, collection, id, version, callbac
     throw new Error('Callback is required for SnapshotRequest');
   }
 
-  if (version != null && !util.isInteger(version)) {
-    throw new Error('Snapshot version must be an integer');
+  if (!this.isValidVersion(version)) {
+    throw new Error('Snapshot version must be a positive integer or null');
   }
 
   this.requestId = requestId;
   this.connection = connection;
   this.id = id;
   this.collection = collection;
-  this.version = util.isInteger(version) ? Math.max(0, version) : null;
+  this.version = version;
   this.callback = callback;
 
   this.sent = false;
 }
 emitter.mixin(SnapshotRequest);
 
+SnapshotRequest.prototype.isValidVersion = function (version) {
+  if (version === null) {
+    return true;
+  }
+
+  if (!util.isInteger(version)) {
+    return false;
+  }
+
+  return version >= 0;
+}
+
 SnapshotRequest.prototype.send = function () {
   if (!this.connection.canSend) {
     return;
@@ -52,11 +64,12 @@ SnapshotRequest.prototype._onConnectionStateChanged = function () {
 };
 
 SnapshotRequest.prototype._handleResponse = function (error, message) {
+  this.emit('ready');
+
   if (error) {
     return this.callback(error);
   }
 
   var snapshot = new Snapshot(this.id, message.v, message.type, message.data, null);
   this.callback(null, snapshot);
-  this.emit('ready');
 };

test/client/snapshot-request.js

@@ -96,13 +96,13 @@ describe('SnapshotRequest', function () {
       });
     });
 
-    it('fetches the latest version if the version is undefined', function (done) {
-      backend.connect().fetchSnapshot('books', 'don-quixote', undefined, function (error, snapshot) {
-        if (error) return done(error);
-        expect(snapshot).to.eql(v3);
-        done();
+    it('throws if the version is undefined', function () {
+      var fetch = function () {
+        backend.connect().fetchSnapshot('books', 'don-quixote', undefined, function () {});
+      };
+
+      expect(fetch).to.throwError();
       });
-    });
 
     it('fetches the latest version when the optional version is not provided', function (done) {
       backend.connect().fetchSnapshot('books', 'don-quixote', function (error, snapshot) {
@@ -117,23 +117,23 @@ describe('SnapshotRequest', function () {
         backend.connect().fetchSnapshot('books', 'don-quixote');
       };
 
-      expect(fetch).to.throwError("Callback is required");
+      expect(fetch).to.throwError();
     });
 
-    it('returns an empty snapshot if the version is -1', function (done) {
-      backend.connect().fetchSnapshot('books', 'don-quixote', -1, function (error, snapshot) {
-        if (error) return done(error);
-        expect(snapshot).to.eql(v0);
-        done();
-      });
+    it('throws if the version is -1', function () {
+      var fetch = function () {
+        backend.connect().fetchSnapshot('books', 'don-quixote', -1, function () {});
+      };
+
+      expect(fetch).to.throwError();
     });
 
     it('errors if the version is a string', function () {
       var fetch = function () {
         backend.connect().fetchSnapshot('books', 'don-quixote', 'foo', function () { });
       }
 
-      expect(fetch).to.throwError("version must be an integer");
+      expect(fetch).to.throwError();
     });
 
     it('errors if asking for a version that does not exist', function (done) {
@@ -223,12 +223,7 @@ describe('SnapshotRequest', function () {
       it('triggers the middleware', function (done) {
         backend.use(backend.MIDDLEWARE_ACTIONS.readSnapshots,
           function (request) {
-            expect(request.collection).to.be('books');
-            expect(request.id).to.be('don-quixote');
-            expect(request.v).to.be(3);
-            expect(request.snapshots).to.eql([v3.data]);
-            expect(request.type).to.be('http://sharejs.org/types/JSONv0');
-
+            expect(request.snapshots[0]).to.eql(v3);
             done();
           }
         );
@@ -239,7 +234,7 @@ describe('SnapshotRequest', function () {
       it('can have its snapshot manipulated in the middleware', function (done) {
         backend.middleware[backend.MIDDLEWARE_ACTIONS.readSnapshots] = [
           function (request, callback) {
-            request.snapshots[0].title = 'Alice in Wonderland';
+            request.snapshots[0].data.title = 'Alice in Wonderland';
             callback();
           },
         ];

test/db.js

@@ -789,10 +789,10 @@ module.exports = function(options) {
         // test that getQuery({query: {}, sort: [['foo', 1], ['bar', -1]]})
         // sorts by foo first, then bar
         var snapshots = [
-          { type: 'json0', id: '0', v: 1, data: {foo: 1, bar: 1}, m: null },
-          { type: 'json0', id: '1', v: 1, data: { foo: 2, bar: 1 }, m: null },
-          { type: 'json0', id: '2', v: 1, data: { foo: 1, bar: 2 }, m: null },
-          { type: 'json0', id: '3', v: 1, data: { foo: 2, bar: 2 }, m: null }
+          {type: 'json0', id: '0', v: 1, data: {foo: 1, bar: 1}, m: null},
+          {type: 'json0', id: '1', v: 1, data: { foo: 2, bar: 1 }, m: null},
+          {type: 'json0', id: '2', v: 1, data: { foo: 1, bar: 2 }, m: null},
+          {type: 'json0', id: '3', v: 1, data: { foo: 2, bar: 2 }, m: null}
         ];
         var db = this.db;
         var dbQuery = getQuery({query: {}, sort: [['foo', 1], ['bar', -1]]});