Rework change conflict detection

lib/models/change.js

@@ -7,7 +7,8 @@ var DataModel = require('./data-model')
   , crypto = require('crypto')
   , CJSON = {stringify: require('canonical-json')}
   , async = require('async')
-  , assert = require('assert');
+  , assert = require('assert')
+  , debug = require('debug')('loopback:change');
 
 /**
  * Properties
@@ -141,6 +142,7 @@ Change.findOrCreateChange = function(modelName, modelId, callback) {
         modelName: modelName,
         modelId: modelId
       });
+      ch.debug('creating change');
       ch.save(callback);
     }
   });
@@ -160,8 +162,13 @@ Change.prototype.rectify = function(cb) {
     updateRevision,
     updateCheckpoint
   ];
+  var currentRev = this.rev;
 
-  if(this.rev) this.prev = this.rev;
+  change.debug('rectify change');
+
+  cb = cb || function(err) {
+    if(err) throw new Error(err);
+  }
 
   async.parallel(tasks, function(err) {
     if(err) return cb(err);
@@ -172,7 +179,27 @@ Change.prototype.rectify = function(cb) {
     // get the current revision
     change.currentRevision(function(err, rev) {
       if(err) return Change.handleError(err, cb);
-      change.rev = rev;
+      change.debug('updating revision ('+ rev +')');
+      // deleted
+      if(rev) {
+        // avoid setting rev and prev to the same value
+        if(currentRev !== rev) {
+          change.rev = rev;
+          change.prev = currentRev;
+        } else {
+          change.debug('rev and prev are equal (not updating rev)');
+        }
+      } else {
+        change.rev = null;
+        if(currentRev) {
+          change.prev = currentRev;
+        } else if(!change.prev) {
+          change.debug('ERROR - could not determing prev');
+          change.prev = Change.UNKNOWN;
+          return cb(new Error('could not determine the previous rev for '
+            + change.modelId));
+        }
+      }
       cb();
     });
   }
@@ -268,6 +295,32 @@ Change.prototype.equals = function(change) {
   return thisRev === thatRev;
 }
 
+/**
+ * Does this change conflict with the given change.
+ * @param  {Change} change
+ * @return {Boolean}
+ */
+
+Change.prototype.conflictsWith = function(change) {
+  if(!change) return false;
+  if(this.equals(change)) return false;
+  if(Change.bothDeleted(this, change)) return false;
+  if(this.isBasedOn(change)) return false;
+  return true;
+}
+
+/**
+ * Are both changes deletes?
+ * @param  {Change} a
+ * @param  {Change} b
+ * @return {Boolean}
+ */
+
+Change.bothDeleted = function(a, b) {
+  return a.type() === Change.DELETE
+      && b.type() === Change.DELETE;
+}
+
 /**
  * Determine if the change is based on the given change.
  * @param  {Change} change
@@ -335,10 +388,13 @@ Change.diff = function(modelName, since, remoteChanges, callback) {
       localModelIds.push(localChange.modelId);
       var remoteChange = remoteChangeIndex[localChange.modelId];
       if(remoteChange && !localChange.equals(remoteChange)) {
-        if(remoteChange.isBasedOn(localChange)) {
-          deltas.push(remoteChange);
-        } else {
+        if(remoteChange.conflictsWith(localChange)) {
+          remoteChange.debug('remote conflict');
+          localChange.debug('local conflict');
           conflicts.push(localChange);
+        } else {
+          remoteChange.debug('remote delta');
+          deltas.push(remoteChange);
         }
       }
     });
@@ -362,6 +418,7 @@ Change.diff = function(modelName, since, remoteChanges, callback) {
  */
 
 Change.rectifyAll = function(cb) {
+  debug('rectify all');
   var Change = this;
   // this should be optimized
   this.find(function(err, changes) {
@@ -394,6 +451,19 @@ Change.handleError = function(err) {
   }
 }
 
+Change.prototype.debug = function() {
+  if(debug.enabled) {
+    var args = Array.prototype.slice.call(arguments);
+    debug.apply(this, args);
+    debug('\tid', this.id);
+    debug('\trev', this.rev);
+    debug('\tprev', this.prev);
+    debug('\tmodelName', this.modelName);
+    debug('\tmodelId', this.modelId);
+    debug('\ttype', this.type());
+  }
+}
+
 /**
  * Get the `Model` class for `change.modelName`.
  * @return {Model}
@@ -510,7 +580,6 @@ Conflict.prototype.changes = function(cb) {
   }
 
   function getTargetChange(cb) {
-    debugger;
     conflict.TargetChange.findOne({
       modelId: conflict.targetModelId
     }, function(err, change) {