[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

@@ -431,14 +431,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;
   }
@@ -492,6 +488,7 @@ export default class InternalModel {
   unloadRecord() {
     this.send('unloadRecord');
     this.dematerializeRecord();
+
     if (this._scheduledDestroy === null) {
       this._scheduledDestroy = run.schedule('destroy', this, '_checkForOrphanedInternalModels');
     }
@@ -511,6 +508,7 @@ export default class InternalModel {
     if (this.isDestroyed) { return; }
 
     this._cleanupOrphanedInternalModels();
+
   }
 
   _cleanupOrphanedInternalModels() {
@@ -533,6 +531,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;
   }
 
@@ -889,14 +890,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();
       }
     });
 
@@ -918,17 +915,28 @@ export default class InternalModel {
     and destroys any ManyArrays.
    */
   destroyRelationships() {
-    this.eachRelationship((name, relationship) => {
-      if (this._relationships.has(name)) {
-        let rel = this._relationships.get(name);
+    this._relationships.forEach((name, rel) => {
+      if (rel._inverseIsAsync()) {
+        rel.removeInternalModelFromInverse(this);
         rel.removeInverseRelationships();
+      } else {
+        rel.removeCompletelyFromInverse();
       }
     });
 
     let implicitRelationships = this._implicitRelationships;
     this.__implicitRelationships = null;
     Object.keys(implicitRelationships).forEach((key) => {
-      implicitRelationships[key].removeInverseRelationships();
+      let rel = implicitRelationships[key];
+
+      if (rel._inverseIsAsync()) {
+        rel.removeInternalModelFromInverse(this);
+        rel.removeInverseRelationships();
+      } else {
+        rel.removeCompletelyFromInverse();
+      }
+
+      rel.destroy();
     });
   }
 

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

@@ -46,6 +46,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

@@ -281,6 +281,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; }
 
@@ -174,7 +181,7 @@ export default class Relationship {
       let relationship = relationships[this.inverseKeyForImplicit];
       if (!relationship) {
         relationship = relationships[this.inverseKeyForImplicit] =
-          new Relationship(this.store, internalModel, this.key,  { options: {} });
+          new Relationship(this.store, internalModel, this.key,  { options: { async: this.isAsync } });
       }
       relationship.addCanonicalInternalModel(this.internalModel);
     }
@@ -215,7 +222,7 @@ export default class Relationship {
         internalModel._relationships.get(this.inverseKey).addInternalModel(this.internalModel);
       } else {
         if (!internalModel._implicitRelationships[this.inverseKeyForImplicit]) {
-          internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, internalModel, this.key,  { options: {} });
+          internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, internalModel, this.key,  { options: { async: this.isAsync } });
         }
         internalModel._implicitRelationships[this.inverseKeyForImplicit].addInternalModel(this.internalModel);
       }
@@ -454,4 +461,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/relationships/has-many-test.js

@@ -287,14 +287,14 @@ test("hasMany + canonical vs currentState + unloadRecord", function(assert) {
     env.store.peekRecord('user', 6).unloadRecord();
   });
 
-  assert.deepEqual(contacts.map(c => c.get('id')), ['2','3','4','5','6','7'], `user's contacts should have expected contacts`);
+  assert.deepEqual(contacts.map(c => c.get('id')), ['3','4','5','7'], `user's contacts should have expected contacts`);
   assert.equal(contacts, user.get('contacts'));
 
   run(() => {
     contacts.addObject(env.store.createRecord('user', { id: 8 }));
   });
 
-  assert.deepEqual(contacts.map(c => c.get('id')), ['2','3','4','5','6','7','8'], `user's contacts should have expected contacts`);
+  assert.deepEqual(contacts.map(c => c.get('id')), ['3','4','5','7','8'], `user's contacts should have expected contacts`);
   assert.equal(contacts, user.get('contacts'));
 });
 
@@ -1949,7 +1949,7 @@ testInDebug('A sync hasMany errors out if there are unlaoded records in it', fun
   }, /You looked up the 'comments' relationship on a 'post' with id 1 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async \('DS.hasMany\({ async: true }\)'\)/);
 });
 
-testInDebug('After removing and unloading a record, a hasMany relationship should still be valid', function(assert) {
+test('After removing and unloading a record, a hasMany relationship should still be valid', function(assert) {
   const post = run(() => {
     env.store.push({
       data: {

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');
 });