♻ Throw error for pending ops on hard rollback

docs/api/sharedb-error.md

@@ -35,6 +35,10 @@ Representation of an error, with a machine-parsable [code](#error-codes).
 
 > This error might be used as part of standard control flow. For example, consumers may define a middleware that validates document structure, and rejects operations that do not conform to this schema using this error code to reset the client to a valid state.
 
+### `ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED`
+
+> This may happen if server rejected op with ERR_OP_SUBMIT_REJECTED and the type is not invertible or there are some pending ops after the create op was rejected with ERR_OP_SUBMIT_REJECTED
+
 ### `ERR_OP_ALREADY_SUBMITTED`
 
 > The same op has been received by the server twice.

lib/client/doc.js

@@ -329,10 +329,6 @@ Doc.prototype._handleSubscribe = function(error, snapshot) {
 Doc.prototype._handleOp = function(err, message) {
   if (err) {
     if (this.inflightOp) {
-      // The server has rejected submission of the current operation. If we get
-      // an "Op submit rejected" error, this was done intentionally
-      // and we should roll back but not return an error to the user.
-      if (err.code === ERROR_CODE.ERR_OP_SUBMIT_REJECTED) err = null;
       return this._rollback(err);
     }
     return this.emit('error', err);
@@ -1014,6 +1010,13 @@ Doc.prototype._rollback = function(err) {
     return this._hardRollback(error);
   }
 
+  // The server has rejected submission of the current operation. If we get
+  // an "Op submit rejected" error, this was done intentionally
+  // and we should roll back but not return an error to the user.
+  if (err.code === ERROR_CODE.ERR_OP_SUBMIT_REJECTED) {
+    return this._clearInflightOp(null);
+  }
+
   this._clearInflightOp(err);
 };
 
@@ -1023,9 +1026,8 @@ Doc.prototype._hardRollback = function(err) {
   // to hit a condition where we have no inflight op, but we do have pending
   // ops. This can happen when an invalid op is submitted, which causes us
   // to hard rollback before the pending op was flushed.
-  var pendingOps = [];
-  if (this.inflightOp) pendingOps.push(this.inflightOp);
-  pendingOps = pendingOps.concat(this.pendingOps);
+  var pendingOps = this.pendingOps;
+  var inflightOp = this.inflightOp;
 
   // Cancel all pending ops and reset if we can't invert
   this._setType(null);
@@ -1035,17 +1037,46 @@ Doc.prototype._hardRollback = function(err) {
 
   // Fetch the latest version from the server to get us back into a working state
   var doc = this;
-  this._fetch({force: true}, function() {
+  this._fetch({force: true}, function(fetchError) {
     // We want to check that no errors are swallowed, so we check that:
     // - there are callbacks to call, and
     // - that every single pending op called a callback
     // If there are no ops queued, or one of them didn't handle the error,
     // then we emit the error.
+
+    if (fetchError) {
+      // This is critical error as it means that our doc is not in usable state
+      // anymore, we should throw doc error.
+      logger.error('Hard rollback doc fetch failed.', fetchError, inflightOp);
+      doc.emit('error', fetchError);
+    }
+
+    if (err.code === ERROR_CODE.ERR_OP_SUBMIT_REJECTED) {
+      /**
+       * Handle special case of ERR_OP_SUBMIT_REJECTED
+       * This ensures that we resolve the main op callback and reject
+       * all the pending ops. This is hard rollback so all the pending ops will be
+       * discarded. This will ensure that the user is at least informed about it.
+       * more info: https://github.com/share/sharedb/pull/626
+       */
+      if (inflightOp) {
+        util.callEach(inflightOp.callbacks);
+        inflightOp = null;
+      }
+
+      if (!pendingOps.length) return;
+      err = new ShareDBError(
+        ERROR_CODE.ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED,
+        'Pending op are present when doing rollback of invertible operation'
+      );
+    }
+
+    if (inflightOp) pendingOps.push(inflightOp);
     var allOpsHadCallbacks = !!pendingOps.length;
     for (var i = 0; i < pendingOps.length; i++) {
       allOpsHadCallbacks = util.callEach(pendingOps[i].callbacks, err) && allOpsHadCallbacks;
     }
-    if (err && !allOpsHadCallbacks) return doc.emit('error', err);
+    if (err && !allOpsHadCallbacks) doc.emit('error', err);
   });
 };
 
@@ -1061,3 +1092,5 @@ Doc.prototype._clearInflightOp = function(err) {
 
   if (err && !called) return this.emit('error', err);
 };
+
+

lib/error.js

@@ -38,6 +38,7 @@ ShareDBError.CODES = {
   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',
+  ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED: 'ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED',
   ERR_OP_VERSION_MISMATCH_AFTER_TRANSFORM: 'ERR_OP_VERSION_MISMATCH_AFTER_TRANSFORM',
   ERR_OP_VERSION_MISMATCH_DURING_TRANSFORM: 'ERR_OP_VERSION_MISMATCH_DURING_TRANSFORM',
   ERR_OP_VERSION_NEWER_THAN_CURRENT_SNAPSHOT: 'ERR_OP_VERSION_NEWER_THAN_CURRENT_SNAPSHOT',

test/client/doc.js

@@ -6,6 +6,7 @@ var richText = require('rich-text').type;
 var ShareDBError = require('../../lib/error');
 var errorHandler = require('../util').errorHandler;
 var sinon = require('sinon');
+var types = require('../../lib/types');
 
 describe('Doc', function() {
   beforeEach(function() {
@@ -514,6 +515,38 @@ describe('Doc', function() {
       ], done);
     });
 
+    it('throws an error when hard rollback fetch failed', function(done) {
+      var backend = this.backend;
+      doc = this.connection.get('dogs', 'scrappy');
+      types.register(richText);
+
+      async.series([
+        doc.create.bind(doc, {ops: [{insert: 'Scrappy'}]}, 'rich-text'),
+        function(next) {
+          backend.use('reply', function(replyContext, cb) {
+            if (replyContext.request.a !== 'f') return cb();
+            cb({code: 'FETCH_ERROR'});
+          });
+          backend.use('submit', function(_context, cb) {
+            cb(new ShareDBError('SUBMIT_ERROR'));
+          });
+          var nonInvertibleOp = [{insert: 'e'}];
+
+          var count = 0;
+          function expectError(code) {
+            count++;
+            return function(error) {
+              expect(error.code).to.equal(code);
+              count--;
+              if (!count) next();
+            };
+          }
+
+          doc.on('error', expectError('FETCH_ERROR'));
+          doc.submitOp(nonInvertibleOp, expectError('SUBMIT_ERROR'));
+        }
+      ], done);
+    });
 
     it('rescues an irreversible op collision', function(done) {
       // This test case attempts to reconstruct the following corner case, with

test/client/submit.js

@@ -5,9 +5,11 @@ var types = require('../../lib/types');
 var deserializedType = require('./deserialized-type');
 var numberType = require('./number-type');
 var errorHandler = require('../util').errorHandler;
+var richText = require('rich-text');
 types.register(deserializedType.type);
 types.register(deserializedType.type2);
 types.register(numberType.type);
+types.register(richText.type);
 
 module.exports = function() {
   describe('client submit', function() {
@@ -919,6 +921,27 @@ module.exports = function() {
       ], done);
     });
 
+
+    it('request.rejectedError() soft rejects main op and throws for pending ops on hard rollback', function(done) {
+      this.backend.use('submit', function(request, next) {
+        if (request.op.create) {
+          next(request.rejectedError());
+        }
+      });
+
+      var connection = this.backend.connect();
+      var doc = connection.get('dogs', 'fido');
+      doc.preventCompose = true;
+
+      doc.create({age: 3}, function(error) {
+        if (error) done(error);
+      });
+      doc.submitOp({p: ['age'], na: 1}, function(err) {
+        expect(err.code).to.be.equal('ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED');
+        done();
+      });
+    });
+
     it('request.rejectedError() soft rejects a create', function(done) {
       this.backend.use('submit', function(request, next) {
         next(request.rejectedError());
@@ -949,6 +972,29 @@ module.exports = function() {
       });
     });
 
+    it(
+      'request.rejectedError() soft rejects main op and throws for pending ops on hard rollback without callback',
+      function(done) {
+        this.backend.use('submit', function(request, next) {
+          if (request.op.create) {
+            next(request.rejectedError());
+          }
+        });
+
+        var connection = this.backend.connect();
+        var doc = connection.get('dogs', 'fido');
+        doc.preventCompose = true;
+
+        doc.create({age: 3});
+        doc.submitOp({p: ['age'], na: 1});
+
+        doc.on('error', function(err) {
+          expect(err.code).to.be.equal('ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED');
+          done();
+        });
+      }
+    );
+
     it('passing an error in submit middleware rejects an op and calls back with the erorr', function(done) {
       this.backend.use('submit', function(request, next) {
         if ('op' in request.op) return next({message: 'Custom error'});
@@ -1043,6 +1089,65 @@ module.exports = function() {
       });
     });
 
+    it('request.rejectedError() soft rejects main op and pending ops for invertible type', function(done) {
+      var rejectedOnce = false;
+      this.backend.use('submit', function(request, next) {
+        if ('op' in request.op && !rejectedOnce) {
+          rejectedOnce = true;
+          return next(request.rejectedError());
+        }
+        next();
+      });
+      var doc = this.backend.connect().get('dogs', 'fido');
+      doc.preventCompose = true;
+
+      doc.create({age: 3}, function(err) {
+        if (err) return done(err);
+        doc.submitOp({p: ['age'], na: 1}, function(err) {
+          if (err) return done(err);
+        });
+        doc.submitOp({p: ['age'], na: 3}, function(err) {
+          if (err) return done(err);
+          expect(doc.version).equal(2);
+          expect(doc.data).eql({age: 6});
+          done();
+        });
+        expect(doc.version).equal(1);
+        expect(doc.data).eql({age: 7});
+      });
+    });
+
+    it(
+      'request.rejectedError() soft rejects main op and throws for pending ops for non invertible type',
+      function(done) {
+        var rejectedOnce = false;
+        this.backend.use('submit', function(request, next) {
+          if ('op' in request.op && !rejectedOnce) {
+            rejectedOnce = true;
+            return next(request.rejectedError());
+          }
+          next();
+        });
+        var doc = this.backend.connect().get('dogs', 'fido');
+        doc.preventCompose = true;
+
+        doc.create({ops: [{insert: 'Scrappy'}]}, 'rich-text', function(err) {
+          if (err) return done(err);
+
+          var nonInvertibleOp = [{insert: 'a'}];
+          doc.submitOp(nonInvertibleOp, function(err) {
+            if (err) return done(err);
+          });
+          doc.submitOp([{insert: 'b'}], function(err) {
+            expect(err.code).to.be.equal('ERR_PENDING_OP_REMOVED_BY_OP_SUBMIT_REJECTED');
+            done();
+          });
+          expect(doc.version).equal(1);
+          expect(doc.data.ops).eql([{insert: 'baScrappy'}]);
+        });
+      }
+    );
+
     it('request.rejectedError() soft rejects an op without callback', function(done) {
       this.backend.use('submit', function(request, next) {
         if ('op' in request.op) return next(request.rejectedError());