🐛 Restore the op before fixups when retrying a submit

docs/middleware/op-submission.md

@@ -65,6 +65,9 @@ backend.use('apply', (context, next) => {
 })
 ```
 
+{: .warn :}
+The `'apply'` hook may be triggered more than once for a single submission. If another client wins the race to commit, the op is transformed over the winning op and applied again to the newer snapshot, re-triggering the hook. Be careful, therefore, with any side effects that assume the hook only runs once per op.
+
 ### Commit
 
 The [`commit`]({{ site.baseurl }}{% link middleware/actions.md %}#commit) hook is triggered after the op has been applied to the snapshot in memory, and both the op and snapshot are about to be written to the database.
@@ -158,6 +161,9 @@ backend.use('apply', (request, next) => {
 {: .warn :}
 The `request.$fixup()` method may throw an error, which should be handled appropriately, usually by passing directly to the `next()` callback.
 
+{: .info :}
+Since the [`'apply'`](#apply) hook can be triggered more than once for a single submission, `request.$fixup()` may also be called more than once. Fixups from an abandoned attempt are discarded along with it, and the hook is expected to fix the op up again against the newer snapshot. Only the fixups from the attempt that is actually committed are sent to the client.
+
 ## Comparing old snapshot version with new version
 
 Frequently, it becomes necessary to verify the changes made. This can be accomplished by leveraging two hooks, `apply` and `commit`, and creating a snapshot clone within the `apply` hook.

lib/submit-request.js

@@ -3,6 +3,7 @@ var projections = require('./projections');
 var ShareDBError = require('./error');
 var types = require('./types');
 var protocol = require('./protocol');
+var util = require('./util');
 
 var ERROR_CODE = ShareDBError.CODES;
 
@@ -43,6 +44,7 @@ function SubmitRequest(backend, agent, index, id, op, options) {
   this.ops = [];
   this.channels = null;
   this._fixupOps = [];
+  this._opBeforeFixups = null;
 }
 module.exports = SubmitRequest;
 
@@ -70,6 +72,12 @@ SubmitRequest.prototype.$fixup = function(op) {
     );
   }
 
+  if (!this._opBeforeFixups) {
+    this._opBeforeFixups = this.op.create ?
+      {data: util.clone(this.op.create.data)} :
+      {op: util.clone(this.op.op)};
+  }
+
   if (this.op.create) this.op.create.data = type.apply(this.op.create.data, op);
   else this.op.op = type.compose(this.op.op, op);
 
@@ -186,8 +194,6 @@ SubmitRequest.prototype.apply = function(callback) {
   // Always set the channels before each attempt to apply. If the channels are
   // modified in a middleware and we retry, we want to reset to a new array
   this.channels = this.backend.getChannels(this.collection, this.id);
-  this._fixupOps = [];
-  delete this.op.m.fixup;
 
   var request = this;
   this.backend.trigger(this.backend.MIDDLEWARE_ACTIONS.apply, this.agent, this, function(err) {
@@ -254,10 +260,21 @@ SubmitRequest.prototype.retry = function(callback) {
   if (this.maxRetries != null && this.retries > this.maxRetries) {
     return callback(this.maxRetriesError());
   }
+  this._resetFixups();
   this.backend.emit('timing', 'submit.retry', Date.now() - this.start, this);
   this.submit(callback);
 };
 
+// The fixups must be undone before the op is transformed forward again
+SubmitRequest.prototype._resetFixups = function() {
+  this._fixupOps = [];
+  delete this.op.m.fixup;
+  if (!this._opBeforeFixups) return;
+  if (this.op.create) this.op.create.data = this._opBeforeFixups.data;
+  else this.op.op = this._opBeforeFixups.op;
+  this._opBeforeFixups = null;
+};
+
 SubmitRequest.prototype._transformOp = function(ops) {
   var type = this.snapshot.type;
   for (var i = 0; i < ops.length; i++) {

test/middleware.js

@@ -702,6 +702,79 @@ describe('middleware', function() {
       });
     });
 
+    it('applies the fixup once when the submit is retried', function(done) {
+      var remoteDoc = backend.connect().get('dogs', 'fido');
+
+      doc.submitOp([{p: ['fixups'], oi: 0}], function(error) {
+        if (error) return done(error);
+        remoteDoc.fetch(function(error) {
+          if (error) return done(error);
+
+          backend.use('apply', function(request, next) {
+            request.$fixup([{p: ['fixups'], na: 1}]);
+            next();
+          });
+
+          var commitDoc;
+          var commitRemoteDoc;
+          backend.use('commit', function(request, next) {
+            // Once we've forced the race, let the retry through untouched
+            if (commitDoc && commitRemoteDoc) return next();
+            if (request.op.src === doc.connection.id) commitDoc = next;
+            else commitRemoteDoc = next;
+            // Both ops have now been applied to the same snapshot, so
+            // committing one of them will force the other to retry
+            if (commitDoc && commitRemoteDoc) commitDoc();
+          });
+
+          doc.submitOp([{p: ['name', 0], si: 'a'}], function(error) {
+            if (error) return done(error);
+            commitRemoteDoc();
+          });
+
+          remoteDoc.submitOp([{p: ['name', 0], si: 'b'}], function(error) {
+            if (error) return done(error);
+            backend.db.getSnapshot('dogs', 'fido', null, null, function(error, snapshot) {
+              if (error) return done(error);
+              expect(snapshot.data.fixups).to.equal(2);
+              expect(snapshot.data).to.eql(remoteDoc.data);
+              expect(snapshot.v).to.equal(remoteDoc.version);
+              done();
+            });
+          });
+        });
+      });
+    });
+
+    it('applies the fixup once when a create is retried', function(done) {
+      backend.use('apply', function(request, next) {
+        request.$fixup([{p: ['fixups'], na: 1}]);
+        next();
+      });
+
+      // A create can only be retried if the database reports a failed commit
+      // without another op having landed, since a create that loses a genuine
+      // race is rejected during transform
+      var commit = backend.db.commit;
+      var lostRace = false;
+      sinon.stub(backend.db, 'commit').callsFake(function(collection, id, op, snapshot, options, callback) {
+        if (lostRace) return commit.call(this, collection, id, op, snapshot, options, callback);
+        lostRace = true;
+        process.nextTick(callback, null, false);
+      });
+
+      doc = connection.get('dogs', 'rover');
+      doc.create({name: 'rover', fixups: 0}, function(error) {
+        if (error) return done(error);
+        backend.db.getSnapshot('dogs', 'rover', null, null, function(error, snapshot) {
+          if (error) return done(error);
+          expect(snapshot.data.fixups).to.equal(1);
+          expect(snapshot.data).to.eql(doc.data);
+          done();
+        });
+      });
+    });
+
     it('applies two fixups', function(done) {
       backend.use('apply', function(request, next) {
         request.$fixup([{p: ['tricks', 0], li: 'sit'}]);