Merge pull request #682 from share/avoid-committing-no-ops

⚡️ Avoid committing no-ops to the database

Author: alecgibson

docs/api/backend.md

@@ -113,6 +113,19 @@ Optional
 
 {: .d-inline-block }
 
+`doNotCommitNoOps` -- boolean
+
+Optional
+{: .label .label-grey }
+
+> Default: `false`
+
+> If set to `true`, will avoid committing no-ops to the database. Clients that submit no-ops
+> (or ops that are transformed to no-ops) will have their ops acknowledged as if they were
+> committed, but the doc version will not increment.
+
+{: .d-inline-block }
+
 `errorHandler` -- Function
 
 Optional

docs/api/sharedb-error.md

@@ -100,3 +100,9 @@ Representation of an error, with a machine-parsable [code](#error-codes).
 ### `ERR_TYPE_CANNOT_BE_PROJECTED`
 
 > The document's type cannot be projected. [`json0`]({{ site.baseurl }}{% link types/json0.md %}) is currently the only type that supports projections.
+
+### `ERR_NO_OP`
+
+> The submitted op resulted in a no-op, possibly after transformation by a remote op.
+
+> This is normal behavior and the client should swallow this error without bumping doc version.

lib/backend.js

@@ -43,6 +43,7 @@ function Backend(options) {
       'new Backend({doNotForwardSendPresenceErrorsToClient: true})\n\n'
     );
   }
+  this.doNotCommitNoOps = !!options.doNotCommitNoOps;
 
   // Map from event name to a list of middleware
   this.middleware = Object.create(null);

lib/client/doc.js

@@ -328,6 +328,15 @@ Doc.prototype._handleSubscribe = function(error, snapshot) {
 
 Doc.prototype._handleOp = function(err, message) {
   if (err) {
+    if (err.code === ERROR_CODE.ERR_NO_OP && message.seq === this.inflightOp.seq) {
+      // Our op was a no-op, either because we submitted a no-op, or - more
+      // likely - because our op was transformed into a no-op by the server
+      // because of a similar remote op. In this case, the server has avoided
+      // committing the op to the database, and we should just clear the in-flight
+      // op and call the callbacks. However, let's first catch ourselves up to
+      // the remote, so that we're in a nice consistent state
+      return this.fetch(this._clearInflightOp.bind(this));
+    }
     if (this.inflightOp) {
       return this._rollback(err);
     }

lib/error.js

@@ -35,6 +35,7 @@ ShareDBError.CODES = {
   ERR_MAX_SUBMIT_RETRIES_EXCEEDED: 'ERR_MAX_SUBMIT_RETRIES_EXCEEDED',
   ERR_MESSAGE_BADLY_FORMED: 'ERR_MESSAGE_BADLY_FORMED',
   ERR_MILESTONE_ARGUMENT_INVALID: 'ERR_MILESTONE_ARGUMENT_INVALID',
+  ERR_NO_OP: 'ERR_NO_OP',
   ERR_OP_ALREADY_SUBMITTED: 'ERR_OP_ALREADY_SUBMITTED',
   ERR_OP_NOT_ALLOWED_IN_PROJECTION: 'ERR_OP_NOT_ALLOWED_IN_PROJECTION',
   ERR_OP_SUBMIT_REJECTED: 'ERR_OP_SUBMIT_REJECTED',

lib/protocol.js

@@ -1,6 +1,6 @@
 module.exports = {
   major: 1,
-  minor: 1,
+  minor: 2,
   checkAtLeast: checkAtLeast
 };
 

lib/submit-request.js

@@ -2,6 +2,7 @@ var ot = require('./ot');
 var projections = require('./projections');
 var ShareDBError = require('./error');
 var types = require('./types');
+var protocol = require('./protocol');
 
 var ERROR_CODE = ShareDBError.CODES;
 
@@ -152,6 +153,18 @@ SubmitRequest.prototype.submit = function(callback) {
       err = request._transformOp(ops);
       if (err) return callback(err);
 
+      var skipNoOp = backend.doNotCommitNoOps &&
+        protocol.checkAtLeast(request.agent.protocol, '1.2') &&
+        request.op.op &&
+        request.op.op.length === 0;
+
+      if (skipNoOp) {
+        // The op is a no-op, either because it was submitted as such, or - more
+        // likely - because it was transformed into one. Let's avoid committing it
+        // and tell the client.
+        return callback(request.noOpError());
+      }
+
       if (op.v !== snapshot.v) {
         // This shouldn't happen, but is just a final sanity check to make
         // sure we have transformed the op to the current snapshot version
@@ -361,3 +374,9 @@ SubmitRequest.prototype.maxRetriesError = function() {
     'Op submit failed. Exceeded max submit retries of ' + this.maxRetries
   );
 };
+SubmitRequest.prototype.noOpError = function() {
+  return new ShareDBError(
+    ERROR_CODE.ERR_NO_OP,
+    'Op is a no-op. Skipping apply.'
+  );
+};

test/client/submit.js

@@ -1290,6 +1290,119 @@ module.exports = function() {
       });
     });
 
+    it('commits both of two identical ops submitted from different clients by default', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = false;
+      var doc1 = backend.connect().get('dogs', id);
+      var doc2 = backend.connect().get('dogs', id);
+      var op = [{p: ['tricks', 0], ld: 'fetch'}];
+
+      async.series([
+        doc1.create.bind(doc1, {tricks: ['fetch', 'sit']}),
+        doc2.fetch.bind(doc2),
+        async.parallel.bind(async.parallel, [
+          doc1.submitOp.bind(doc1, op),
+          doc2.submitOp.bind(doc2, op)
+        ]),
+        function(next) {
+          expect(doc1.data).to.eql({tricks: ['sit']});
+          expect(doc2.data).to.eql({tricks: ['sit']});
+          next();
+        },
+        function(next) {
+          backend.db.getOps('dogs', id, 0, null, {}, function(error, ops) {
+            if (error) return next(error);
+            // Expect:
+            // v0: create
+            // v1: update
+            // v2: duplicate update
+            expect(ops).to.have.length(3);
+            next();
+          });
+        }
+      ], done);
+    });
+
+    it('only commits one of two identical ops submitted from different clients', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = true;
+      var doc1 = backend.connect().get('dogs', id);
+      var doc2 = backend.connect().get('dogs', id);
+      var op = [{p: ['tricks', 0], ld: 'fetch'}];
+
+      async.series([
+        doc1.create.bind(doc1, {tricks: ['fetch', 'sit']}),
+        doc2.fetch.bind(doc2),
+        async.parallel.bind(async.parallel, [
+          doc1.submitOp.bind(doc1, op),
+          doc2.submitOp.bind(doc2, op)
+        ]),
+        function(next) {
+          expect(doc1.data).to.eql({tricks: ['sit']});
+          expect(doc2.data).to.eql({tricks: ['sit']});
+          next();
+        },
+        function(next) {
+          backend.db.getOps('dogs', id, 0, null, {}, function(error, ops) {
+            if (error) return next(error);
+            // Expect:
+            // v0: create
+            // v1: update
+            // no duplicate update
+            expect(ops).to.have.length(2);
+            next();
+          });
+        }
+      ], done);
+    });
+
+    it('can submit a new op after getting a no-op', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = true;
+      var doc1 = backend.connect().get('dogs', id);
+      var doc2 = backend.connect().get('dogs', id);
+      var op = [{p: ['tricks', 0], ld: 'fetch'}];
+
+      async.series([
+        doc1.create.bind(doc1, {tricks: ['fetch', 'sit']}),
+        doc2.fetch.bind(doc2),
+        async.parallel.bind(async.parallel, [
+          doc1.submitOp.bind(doc1, op),
+          doc2.submitOp.bind(doc2, op)
+        ]),
+        function(next) {
+          expect(doc1.data).to.eql({tricks: ['sit']});
+          expect(doc2.data).to.eql({tricks: ['sit']});
+          next();
+        },
+        doc1.submitOp.bind(doc1, [{p: ['tricks', 0], li: 'play dead'}]),
+        function(next) {
+          expect(doc1.data.tricks).to.eql(['play dead', 'sit']);
+          next();
+        }
+      ], done);
+    });
+
+    it('fixes up even if an op is fixed up to become a no-op', function(done) {
+      var backend = this.backend;
+      backend.doNotCommitNoOps = true;
+      var doc = backend.connect().get('dogs', id);
+
+      backend.use('apply', function(req, next) {
+        req.$fixup([{p: ['fixme'], od: true}]);
+        next();
+      });
+
+      async.series([
+        doc.create.bind(doc, {}),
+        doc.submitOp.bind(doc, [{p: ['fixme'], oi: true}]),
+        function(next) {
+          expect(doc.data).to.eql({});
+          next();
+        }
+      ], done);
+    });
+
     describe('type.deserialize', function() {
       it('can create a new doc', function(done) {
         var doc = this.backend.connect().get('dogs', id);