[BUGFIX release] relationship [x, y] should not break on x.unloadReco…

…rd()

 
For an async relationship [x, y] with x.unloadRecord(), now adjusts only the relationship’s currentState, leaving that relationship’s canonical state alone, ensuring the existing client-side delete semantics are preserved. But when that relationship is reloaded, the canonicalState consulted.

For sync relationship [x, y] with x.unloadRecord(), both currentState and canonical state are updated. This is to mirror the client-side delete semantics. But since we cannot reload a sync relationship we must assume this to be the new canonical state and rely on subsequent `push` or `adapterPayloads` or manual `store.push` to update.

This aims to:

* [FIX] hasMany arrays never contain dematerialized records (so they no longer become broken)
* [FIX] using unloadRecord as a type of client side delete is restored
* [PRESERVE] the garbage collector pass to cleanup orphaned models
* [PRESERVE] second access to a relationship which did contain an unloadRecord to cause a reload


note: if both sides of a relationships are unloaded, the above doesn’t apply. This is largely just when members of a loaded relationship are themselves unloaded.

[fixes #4986 #5052 #4987 #4996]

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

@@ -430,14 +430,10 @@ export default class InternalModel {
   */
   _directlyRelatedInternalModels() {
     let array = [];
-    this.type.eachRelationship((key, relationship) => {
-      if (this._relationships.has(key)) {
-        let relationship = this._relationships.get(key);
-        let localRelationships = relationship.members.toArray();
-        let serverRelationships = relationship.canonicalMembers.toArray();
-
-        array = array.concat(localRelationships, serverRelationships);
-      }
+    this._relationships.forEach((name, rel) => {
+      let local = rel.members.toArray();
+      let server = rel.canonicalMembers.toArray();
+      array = array.concat(local, server);
     });
     return array;
   }
@@ -491,6 +487,7 @@ export default class InternalModel {
   unloadRecord() {
     this.send('unloadRecord');
     this.dematerializeRecord();
+
     if (this._scheduledDestroy === null) {
       this._scheduledDestroy = run.schedule('destroy', this, '_checkForOrphanedInternalModels');
     }
@@ -510,6 +507,7 @@ export default class InternalModel {
     if (this.isDestroyed) { return; }
 
     this._cleanupOrphanedInternalModels();
+
   }
 
   _cleanupOrphanedInternalModels() {
@@ -532,6 +530,9 @@ export default class InternalModel {
     assert("Cannot destroy an internalModel while its record is materialized", !this._record || this._record.get('isDestroyed') || this._record.get('isDestroying'));
 
     this.store._internalModelDestroyed(this);
+
+    this._relationships.forEach((name, rel) => rel.destroy());
+
     this._isDestroyed = true;
   }
 
@@ -888,14 +889,10 @@ export default class InternalModel {
     @private
    */
   removeFromInverseRelationships(isNew = false) {
-    this.eachRelationship((name) => {
-      if (this._relationships.has(name)) {
-        let rel = this._relationships.get(name);
-
-        rel.removeCompletelyFromInverse();
-        if (isNew === true) {
-          rel.clear();
-        }
+    this._relationships.forEach((name, rel) => {
+      rel.removeCompletelyFromInverse();
+      if (isNew === true) {
+        rel.clear();
       }
     });
 
@@ -917,17 +914,12 @@ export default class InternalModel {
     and destroys any ManyArrays.
    */
   destroyRelationships() {
-    // this.removeFromInverseRelationships();
-    // return;
-    this.eachRelationship((name, relationship) => {
-      if (this._relationships.has(name)) {
-        let rel = this._relationships.get(name);
-
-        if (rel.inverseKey && rel.inverseInternalModel && rel.inverseInternalModel._relationships.get(rel.inverseKey).isAsync) {
-          rel.removeInverseRelationships();
-        }  else {
-          rel.removeCompletelyFromInverse();
-        }
+    this._relationships.forEach((name, rel) => {
+      if (rel._inverseIsAsync()) {
+        rel.removeInternalModelFromInverse(this);
+        rel.removeInverseRelationships();
+      } else {
+        rel.removeCompletelyFromInverse();
       }
     });
 
@@ -936,11 +928,14 @@ export default class InternalModel {
     Object.keys(implicitRelationships).forEach((key) => {
       let rel = implicitRelationships[key];
 
-      if (rel.inverseKey && rel.inverseInternalModel && rel.inverseInternalModel._relationships.get(rel.inverseKey).isAsync) {
+      if (rel._inverseIsAsync()) {
+        rel.removeInternalModelFromInverse(this);
         rel.removeInverseRelationships();
-      }  else {
+      } else {
         rel.removeCompletelyFromInverse();
       }
+
+      rel.destroy();
     });
   }
 

addon/-private/system/relationships/state/create.js

@@ -48,6 +48,13 @@ export default class Relationships {
     return !!this.initializedRelationships[key];
   }
 
+  forEach(cb) {
+    let rels = this.initializedRelationships;
+    Object.keys(rels).forEach(name => {
+      cb(name, rels[name]);
+    });
+  }
+
   get(key) {
     let relationships = this.initializedRelationships;
     let relationship = relationships[key];

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

@@ -287,6 +287,20 @@ export default class ManyRelationship extends Relationship {
       this.updateInternalModelsFromAdapter(internalModels);
     }
   }
+
+  destroy() {
+    super.destroy();
+    let manyArray = this._manyArray;
+    if (manyArray) {
+      manyArray.destroy();
+    }
+
+    let proxy = this.__loadingPromise;
+
+    if (proxy) {
+      proxy.destroy();
+    }
+  }
 }
 
 function setForArray(array) {

addon/-private/system/relationships/state/relationship.js

@@ -83,6 +83,13 @@ export default class Relationship {
     return this.internalModel.modelName;
   }
 
+  _inverseIsAsync() {
+    if (!this.inverseKey || !this.inverseInternalModel) {
+      return false;
+    }
+    return this.inverseInternalModel._relationships.get(this.inverseKey).isAsync;
+  }
+
   removeInverseRelationships() {
     if (!this.inverseKey) { return; }
 
@@ -453,4 +460,7 @@ export default class Relationship {
   }
 
   updateData() {}
+
+  destroy() {
+  }
 }

tests/integration/records/rematerialize-test.js

@@ -146,22 +146,47 @@ test("an async has many relationship to an unloaded record can restore that reco
   // disable background reloading so we do not re-create the relationship.
   env.adapter.shouldBackgroundReloadRecord = () => false;
 
-  env.adapter.findRecord = function() {
-    assert.ok('adapter called');
-    return Ember.RSVP.Promise.resolve({
-      data: {
-        type: 'boat',
-        id: '1',
-        attributes: {
-          name: "Boaty McBoatface"
-        },
-        relationships: {
-          person: {
-            data: { type: 'person', id: '1' }
-          }
-        }
+  const BOAT_ONE = {
+    type: 'boat',
+    id: '1',
+    attributes: {
+      name: "Boaty McBoatface"
+    },
+    relationships: {
+      person: {
+        data: { type: 'person', id: '1' }
       }
-    });
+    }
+  };
+
+  const BOAT_TWO = {
+    type: 'boat',
+    id: '2',
+    attributes: {
+      name: 'Some other boat'
+    },
+    relationships: {
+      person: {
+        data: { type: 'person', id: '1' }
+      }
+    }
+  };
+
+  env.adapter.findRecord = function(store, model, param) {
+    assert.ok('adapter called');
+
+    let data;
+    if (param === '1') {
+      data = BOAT_ONE;
+    } else if (param === '1') {
+      data = BOAT_TWO;
+    } else {
+      throw new Error(`404: no such boat with id=${param}`);
+    }
+
+    return {
+      data
+    };
   }
 
   run(function() {
@@ -186,29 +211,7 @@ test("an async has many relationship to an unloaded record can restore that reco
 
   run(function() {
     env.store.push({
-      data: [{
-        type: 'boat',
-        id: '2',
-        attributes: {
-          name: 'Some other boat'
-        },
-        relationships: {
-          person: {
-            data: { type: 'person', id: '1' }
-          }
-        }
-      }, {
-        type: 'boat',
-        id: '1',
-        attributes: {
-          name: 'Boaty McBoatface'
-        },
-        relationships: {
-          person: {
-            data: { type: 'person', id: '1' }
-          }
-        }
-      }]
+      data: [BOAT_ONE, BOAT_TWO]
     });
   });
 
@@ -220,22 +223,16 @@ test("an async has many relationship to an unloaded record can restore that reco
   assert.equal(env.store.hasRecordForId('boat', 1), true, 'The boat is in the store');
   assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is loaded');
 
-  let boats = run(() => {
-    return adam.get('boats');
-  });
+  let boats = run(() => adam.get('boats'));
 
   assert.equal(boats.get('length'), 2, 'Before unloading boats.length is correct');
 
-  run(function() {
-    boaty.unloadRecord();
-  });
+  run(() => boaty.unloadRecord());
 
   assert.equal(env.store.hasRecordForId('boat', 1), false, 'The boat is unloaded');
   assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is retained');
 
-  let rematerializedBoaty = run(() => {
-    return rematerializedBoaty = adam.get('boats').objectAt(1);
-  });
+  let rematerializedBoaty = run(() => adam.get('boats')).objectAt(0);
 
   assert.equal(adam.get('boats.length'), 2, 'boats.length correct after rematerialization');
   assert.equal(rematerializedBoaty.get('id'), '1');

tests/integration/store-test.js

@@ -201,7 +201,7 @@ test("destroying the store correctly cleans everything up", function(assert) {
 
   assert.equal(personWillDestroy.called.length, 1, 'expected person to have recieved willDestroy once');
   assert.equal(carWillDestroy.called.length, 1, 'expected car to recieve willDestroy once');
-  assert.equal(carsWillDestroy.called.length, 1, 'expected cars to recieve willDestroy once');
+  assert.equal(carsWillDestroy.called.length, 1, 'expected person.cars to recieve willDestroy once');
   assert.equal(adapterPopulatedPeopleWillDestroy.called.length, 1, 'expected adapterPopulatedPeople to recieve willDestroy once');
   assert.equal(filterdPeopleWillDestroy.called.length, 1, 'expected filterdPeople.willDestroy to have been called once');
 });

tests/unit/model/relationships/has-many-test.js

@@ -1812,16 +1812,19 @@ test('DS.hasMany proxy is destroyed', function(assert) {
   let { store } = setupStore({ tag: Tag, person: Person });
 
   let tag = run(() => store.createRecord('tag'));
-  let people = tag.get('people');
+  let peopleProxy = tag.get('people');
 
-  return people.then(() => {
+  return peopleProxy.then(people => {
     Ember.run(() => {
       tag.unloadRecord();
-      assert.equal(people.get('isDestroying'), true);
-      assert.equal(people.get('isDestroyed'),  false);
+      assert.equal(people.isDestroying, false, 'people is destroying sync after unloadRecord');
+      assert.equal(people.isDestroyed, false, 'people is NOT YET destroyed sync after unloadRecord');
+      assert.equal(peopleProxy.isDestroying, false, 'peopleProxy is destroying sync after unloadRecord');
+      assert.equal(peopleProxy.isDestroyed, false, 'peopleProxy is NOT YET destroyed sync after unloadRecord');
     });
-    assert.equal(people.get('isDestroying'), true);
-    assert.equal(people.get('isDestroyed'), true);
+
+    assert.equal(peopleProxy.isDestroying, true, 'peopleProxy is destroying after the run post unloadRecord');
+    assert.equal(peopleProxy.isDestroyed, true, 'peopleProxy is destroying after the run post unloadRecord');
   })
 });