Merge pull request #5438 from runspired/fix-destroy-sync

[BUGFIX] ensure destroy-sync cleanup is correct

addon/-private/system/model/internal-model.js

@@ -141,7 +141,7 @@ export default class InternalModel {
     this._record = null;
     this._isDestroyed = false;
     this.isError = false;
-    this._isUpdatingRecordArrays = false; // used by the recordArrayManager
+    this._pendingRecordArrayManagerFlush = false; // used by the recordArrayManager
 
     // During dematerialization we don't want to rematerialize the record.  The
     // reason this might happen is that dematerialization removes records from
@@ -415,13 +415,15 @@ export default class InternalModel {
   }
 
   dematerializeRecord() {
+    this._isDematerializing = true;
+
     if (this._record) {
-      this._isDematerializing = true;
       this._record.destroy();
-      this.destroyRelationships();
-      this.resetRecord();
     }
 
+    // move to an empty never-loaded state
+    this.destroyRelationships();
+    this.resetRecord();
     this.updateRecordArrays();
   }
 
@@ -607,7 +609,7 @@ export default class InternalModel {
 
   destroy() {
     assert("Cannot destroy an internalModel while its record is materialized", !this._record || this._record.get('isDestroyed') || this._record.get('isDestroying'));
-
+    this.isDestroying = true;
     this.store._internalModelDestroyed(this);
 
     this._relationships.forEach((name, rel) => rel.destroy());

addon/-private/system/relationships/state/has-many.js

@@ -167,9 +167,9 @@ export default class ManyRelationship extends Relationship {
   }
 
   removeAllCanonicalInternalModelsFromOwn() {
-    super.removeAllCanonicalInternalModelsFromOwn();
     this.canonicalMembers.clear();
     this.canonicalState.splice(0, this.canonicalState.length);
+    super.removeAllCanonicalInternalModelsFromOwn();
   }
 
   removeCompletelyFromOwn(internalModel) {

addon/-private/system/store.js

@@ -1103,15 +1103,20 @@ Store = Service.extend({
     let internalModel = this._internalModelsFor(modelName).get(trueId);
 
     if (internalModel) {
+      // unloadRecord is async, if one attempts to unload + then sync push,
+      //   we must ensure the unload is canceled before continuing
+      //   The createRecord path will take _existingInternalModelForId()
+      //   which will call `destroySync` instead for this unload + then
+      //   sync createRecord scenario. Once we have true client-side
+      //   delete signaling, we should never call destroySync
       if (internalModel.hasScheduledDestroy()) {
-        internalModel.destroySync();
-        return this._buildInternalModel(modelName, trueId);
-      } else {
-        return internalModel;
+        internalModel.cancelDestroy();
       }
-    } else {
-      return this._buildInternalModel(modelName, trueId);
+
+      return internalModel;
     }
+
+    return this._buildInternalModel(modelName, trueId);
   },
 
   _internalModelDidReceiveRelationshipData(modelName, id, relationshipData) {
@@ -2555,7 +2560,11 @@ Store = Service.extend({
 
     if (internalModel && internalModel.hasScheduledDestroy()) {
       // unloadRecord is async, if one attempts to unload + then sync create,
-      // we must ensure the unload is complete before starting the create
+      //   we must ensure the unload is complete before starting the create
+      //   The push path will take _internalModelForId()
+      //   which will call `cancelDestroy` instead for this unload + then
+      //   sync push scenario. Once we have true client-side
+      //   delete signaling, we should never call destroySync
       internalModel.destroySync();
       internalModel = null;
     }

tests/integration/records/unload-test.js

@@ -7,6 +7,10 @@ import { module, test } from 'qunit';
 import DS from 'ember-data';
 import setupStore from 'dummy/tests/helpers/store';
 
+function idsFromOrderedSet(set) {
+  return set.list.map(i => i.id);
+}
+
 const {
   attr,
   belongsTo,
@@ -518,6 +522,92 @@ test('unloadAll(type) does not leave stranded internalModels in relationships (r
   assert.ok(reloadedBoatInternalModel === initialBoatInternalModel, 'after an unloadAll, subsequent fetch results in the same InternalModel');
 });
 
+test('(regression) unloadRecord followed by push in the same run-loop', function(assert) {
+  let { store } = env;
+
+  let person = run(() => store.push({
+    data: {
+      type: 'person',
+      id: '1',
+      attributes: {
+        name: 'Could be Anybody'
+      },
+      relationships: {
+        boats: {
+          data: [
+            { type: 'boat', id: '1' }
+          ]
+        }
+      }
+    },
+    included: [
+      makeBoatOneForPersonOne()
+    ]
+  }));
+
+  let boat = store.peekRecord('boat', '1');
+  let initialBoatInternalModel = boat._internalModel;
+  let relationshipState = person.hasMany('boats').hasManyRelationship;
+  let knownPeople = env.store._internalModelsFor('person');
+  let knownBoats = store._internalModelsFor('boat');
+
+  // ensure we loaded the people and boats
+  assert.deepEqual(knownPeople.models.map(m => m.id), ['1'], 'one person record is loaded');
+  assert.deepEqual(knownBoats.models.map(m => m.id), ['1'], 'one boat record is loaded');
+  assert.equal(env.store.hasRecordForId('person', '1'), true);
+  assert.equal(env.store.hasRecordForId('boat', '1'), true);
+
+  // ensure the relationship was established (we reach through the async proxy here)
+  let peopleBoats = run(() => person.get('boats.content'));
+  let boatPerson = run(() => boat.get('person.content'));
+
+  assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1');
+  assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1');
+  assert.ok(get(peopleBoats, 'length') === 1, 'Our person has a boat');
+  assert.ok(peopleBoats.objectAt(0) === boat, 'Our person has the right boat');
+  assert.ok(boatPerson === person, 'Our boat has the right person');
+
+  run(() => boat.unloadRecord());
+
+  // ensure that our new state is correct
+  assert.deepEqual(knownPeople.models.map(m => m.id), ['1'], 'one person record is loaded');
+  assert.deepEqual(knownBoats.models.map(m => m.id), ['1'], 'one boat record is known');
+  assert.ok(knownBoats.models[0] === initialBoatInternalModel, 'We still have our boat');
+  assert.equal(initialBoatInternalModel.isEmpty(), true, 'Model is in the empty state');
+  assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should still be 1');
+  assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should still be 1');
+  assert.ok(get(peopleBoats, 'length') === 0, 'Our person thinks they have no boats');
+
+  run(() => store.push({
+    data: makeBoatOneForPersonOne()
+  }));
+
+  let reloadedBoat = store.peekRecord('boat', '1');
+  let reloadedBoatInternalModel = reloadedBoat._internalModel;
+
+  assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1');
+  assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1');
+  assert.ok(reloadedBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent fetch results in the same InternalModel');
+
+  // and now the kicker, run-loop fun!
+  //   here, we will dematerialize the record, but push it back into the store
+  //   all in the same run-loop!
+  //   effectively this tests that our destroySync is not stupid
+  run(() => {
+    reloadedBoat.unloadRecord();
+    store.push({
+      data: makeBoatOneForPersonOne()
+    });
+  });
+
+  let yaBoat = store.peekRecord('boat', '1');
+  let yaBoatInternalModel = yaBoat._internalModel;
+
+  assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1');
+  assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1');
+  assert.ok(yaBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent same-loop push results in the same InternalModel');
+});
+
 test('unloading a disconnected subgraph clears the relevant internal models', function(assert) {
   env.adapter.shouldBackgroundReloadRecord = () => false;
 
@@ -632,7 +722,6 @@ test('unloading a disconnected subgraph clears the relevant internal models', fu
   });
 });
 
-
 test('Unloading a record twice only schedules destroy once', function(assert) {
   const store = env.store;
   let record;
@@ -753,7 +842,7 @@ test('after unloading a record, the record can be fetched again immediately', fu
     assert.equal(record.isDestroying, true, 'the record is destroying');
     assert.equal(internalModel.currentState.stateName, 'root.empty', 'We are unloaded after unloadRecord');
     return store.findRecord('person', '1').then(newRecord => {
-      assert.equal(internalModel.currentState.stateName, 'root.empty', 'the old internalModel is discarded');
+      assert.ok(internalModel === newRecord._internalModel, 'the old internalModel is reused');
       assert.equal(newRecord._internalModel.currentState.stateName, 'root.loaded.saved', 'We are loaded after findRecord');
     });
   });
@@ -772,7 +861,9 @@ test('after unloading a record, the record can be fetched again immediately (pur
           name: 'Adam Sunderland'
         },
         relationships: {
-          cars: { data: null }
+          cars: {
+            data: []
+          }
         }
       }
     };
@@ -791,7 +882,7 @@ test('after unloading a record, the record can be fetched again immediately (pur
           cars: {
             data: [
               {
-                id: 1,
+                id: '1',
                 type: 'car'
               }
             ]
@@ -801,7 +892,7 @@ test('after unloading a record, the record can be fetched again immediately (pur
       included: [
         {
           type: 'car',
-          id: 1,
+          id: '1',
           attributes: {
             make: 'jeep',
             model: 'wrangler'
@@ -822,8 +913,8 @@ test('after unloading a record, the record can be fetched again immediately (pur
     assert.equal(internalModel.currentState.stateName, 'root.empty', 'Expected the previous internal model tobe unloaded');
 
     return store.findRecord('person', '1').then(record => {
-      assert.equal(record.get('cars.length'), 0);
-      assert.equal(internalModel.currentState.stateName, 'root.empty', 'Expected the previous internal model to STILL be unloaded');
+      assert.equal(record.get('cars.length'), 0,  'Expected relationship to be cleared by the new push');
+      assert.ok(internalModel === record._internalModel, 'the old internalModel is reused');
       assert.equal(record._internalModel.currentState.stateName, 'root.loaded.saved', 'Expected the NEW internal model to be loaded');
     });
   });
@@ -870,6 +961,7 @@ test('after unloading a record, the record can be fetched again immediately (wit
   });
 
   const internalModel = record._internalModel;
+  const bike = store.peekRecord('bike', '1');
   assert.equal(internalModel.currentState.stateName, 'root.loaded.saved', 'We are loaded initially');
 
   assert.equal(record.get('bike.name'), 'mr bike');
@@ -880,10 +972,12 @@ test('after unloading a record, the record can be fetched again immediately (wit
     assert.equal(record.isDestroying, true, 'the record is destroying');
     assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed');
     assert.equal(internalModel.currentState.stateName, 'root.empty', 'We are unloaded after unloadRecord');
+
     let wait = store.findRecord('person', '1').then(newRecord => {
       assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed');
-      assert.ok(newRecord.get('bike') === null, 'the newRecord should NOT have had a bike');
+      assert.ok(newRecord.get('bike') === bike, 'the newRecord should retain knowledge of the bike');
     });
+
     assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed');
     return wait;
   });