Merge pull request #129 from gnarf/fix-remove-has-one

Fix call stack overflow when removing circular hasOne inverse
orbitjs · Apr 8, 2015 · cb85917 · cb85917
2 parents 616b3c7 + a9e4675
commit cb85917
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 4 deletions.
diff --git a/lib/orbit-common/cache.js b/lib/orbit-common/cache.js
@@ -275,16 +275,22 @@ var Cache = Class.extend({
   },
 
   _markForRemoval: function(path) {
-    this._pathsToRemove.push(path.join('/'));
+    path = path.join('/');
+    // console.log('_markForRemoval', path);
+    this._pathsToRemove.push(path);
   },
 
   _unmarkForRemoval: function(path) {
-    var i = this._pathsToRemove.indexOf(path.join('/'));
+    path = path.join('/');
+    var i = this._pathsToRemove.indexOf(path);
+    // console.log('_unmarkForRemoval', path, i);
     if (i > -1) this._pathsToRemove.splice(i, 1);
   },
 
   _isMarkedForRemoval: function(path) {
-    return (this._pathsToRemove.indexOf(path.join('/')) > -1);
+    path = path.join('/');
+    // console.log('_isMarkedForRemoval', path);
+    return (this._pathsToRemove.indexOf(path) > -1);
   },
 
   _isOperationRequired: function(operation) {
@@ -683,7 +689,7 @@ var Cache = Class.extend({
     var op = this._removeLinkOp(type, id, key, value);
 
     // Apply operation only if necessary
-    if (this.retrieve(op.path)) {
+    if (this.retrieve(op.path) && !this._isMarkedForRemoval(op.path)) {
       this.transform(parentOperation.spawn(op));
     }
   },

diff --git a/test/tests/orbit-common/unit/cache-test.js b/test/tests/orbit-common/unit/cache-test.js
@@ -235,6 +235,43 @@ test("does not remove hasOne if link doesn't exist", function(){
   ok(!appliedTransform, "didn't apply transform");
 });
 
+test("#transform removing model with a bi-directional hasOne", function() {
+  expect(5);
+
+  var hasOneSchema = new Schema({
+    models: {
+      one: {
+        links: {
+          two: { type: 'hasOne', model: 'two', inverse: 'one' }
+        }
+      },
+      two: {
+        links: {
+          one: { type: 'hasOne', model: 'one', inverse: 'two' }
+        }
+      },
+    }
+  });
+  cache = new Cache(hasOneSchema);
+  cache.transform({ op: 'add', path: 'one/one', value: {
+    id: 'one',
+    __rel: { two: null },
+  }});
+  cache.transform({ op: 'add', path: 'two/two', value: {
+    id: 'two',
+    __rel: { one: 'one' },
+  }});
+  var one = cache.retrieve(['one', 'one']);
+  var two = cache.retrieve(['two', 'two']);
+  ok(one, 'one exists');
+  ok(two, 'two exists');
+  equal(one.__rel.two, 'two', 'one links to two');
+  equal(two.__rel.one, 'one', 'two links to one');
+
+  cache.transform({ op: 'remove', path: 'two/two' });
+  strictEqual(one.__rel.two, null, 'ones link to two got removed');
+});
+
 test("#transform maintainDependencies:true removes dependent records", function() {
   // By making this schema recursively dependent remove we check that recursive
   // works as well.