Restore 2.12 semantics for sync unloadRecord

A number of use cases rely on unloadRecord's 2.12 behaviour of treating
unloadRecord as a client-side delete on the inverse side of a sync
relationship.

This pr restores that functionality, while retaining the
invalidate+refetch functionality for async relationships.

This behaviour is codified in a number of tests within
tests/integration/records/unload-test.js

Fix #5136, #5137

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

@@ -62,12 +62,24 @@ function areAllModelsUnloaded(internalModels) {
   return true;
 }
 
+// Handle dematerialization for relationship `rel`.  In all cases, notify the
+// relatinoship of the dematerialization: this is done so the relationship can
+// notify its inverse which needs to update state
+//
+// If the inverse is sync, unloading this record is treated as a client-side
+// delete, so we remove the inverse records from this relationship to
+// disconnect the graph.  Because it's not async, we don't need to keep around
+// the internalModel as an id-wrapper for references and because the graph is
+// disconnected we can actually destroy the internalModel when checking for
+// orphaned models.
 function destroyRelationship(rel) {
-  if (rel._inverseIsAsync()) {
-    rel.removeInternalModelFromInverse(rel.inverseInternalModel);
-    rel.removeInverseRelationships();
-  } else {
-    rel.removeCompletelyFromInverse();
+  rel.internalModelDidDematerialize();
+
+  if (rel._inverseIsSync()) {
+    // disconnect the graph so that the sync inverse relationship does not
+    // prevent us from cleaning up during `_cleanupOrphanedInternalModels`
+    rel.removeAllInternalModelsFromOwn();
+    rel.removeAllCanonicalInternalModelsFromOwn();
   }
 }
 // this (and all heimdall instrumentation) will be stripped by a babel transform
@@ -432,6 +444,7 @@ export default class InternalModel {
   */
   _directlyRelatedInternalModels() {
     let array = [];
+
     this._relationships.forEach((name, rel) => {
       array = array.concat(rel.members.list, rel.canonicalMembers.list);
     });
@@ -923,10 +936,7 @@ export default class InternalModel {
     this.__implicitRelationships = null;
     Object.keys(implicitRelationships).forEach((key) => {
       let rel = implicitRelationships[key];
-
       destroyRelationship(rel);
-
-      rel.destroy();
     });
   }
 

addon/-private/system/relationships/state/belongs-to.js

@@ -58,6 +58,7 @@ export default class BelongsToRelationship extends Relationship {
   }
 
   inverseDidDematerialize() {
+    super.inverseDidDematerialize(this.inverseInternalModel);
     this.notifyBelongsToChanged();
   }
 
@@ -74,6 +75,13 @@ export default class BelongsToRelationship extends Relationship {
     }
   }
 
+
+  removeCompletelyFromInverse() {
+    super.removeCompletelyFromInverse();
+
+    this.inverseInternalModel = null;
+  }
+
   flushCanonical() {
     //temporary fix to not remove newly created records if server returned null.
     //TODO remove once we have proper diffing
@@ -115,6 +123,12 @@ export default class BelongsToRelationship extends Relationship {
     this.notifyBelongsToChanged();
   }
 
+  removeAllInternalModelsFromOwn() {
+    super.removeAllInternalModelsFromOwn();
+    this.inverseInternalModel = null;
+    this.notifyBelongsToChanged();
+  }
+
   notifyBelongsToChanged() {
     this.internalModel.notifyBelongsToChanged(this.key);
   }
@@ -125,6 +139,11 @@ export default class BelongsToRelationship extends Relationship {
     super.removeCanonicalInternalModelFromOwn(internalModel);
   }
 
+  removeAllCanonicalInternalModelsFromOwn() {
+    super.removeAllCanonicalInternalModelsFromOwn();
+    this.canonicalState = null;
+  }
+
   findRecord() {
     if (this.inverseInternalModel) {
       return this.store._findByInternalModel(this.inverseInternalModel);

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

@@ -11,7 +11,12 @@ export default class ManyRelationship extends Relationship {
     this.belongsToType = relationshipMeta.type;
     this.canonicalState = [];
     this.isPolymorphic = relationshipMeta.options.polymorphic;
+    // The ManyArray for this relationship
     this._manyArray = null;
+    // The previous ManyArray for this relationship.  It will be destroyed when
+    // we create a new many array, but in the interim it will be updated if
+    // inverse internal models are unloaded.
+    this._retainedManyArray = null;
     this.__loadingPromise = null;
   }
 
@@ -33,6 +38,8 @@ export default class ManyRelationship extends Relationship {
   }
 
   get manyArray() {
+    assert(`Error: relationship ${this.parentType}:${this.key} has both many array and retained many array`, this._manyArray === null || this._retainedManyArray === null);
+
     if (!this._manyArray) {
       this._manyArray = ManyArray.create({
         canonicalState: this.canonicalState,
@@ -43,7 +50,13 @@ export default class ManyRelationship extends Relationship {
         meta: this.meta,
         isPolymorphic: this.isPolymorphic
       });
+
+      if (this._retainedManyArray !== null) {
+        this._retainedManyArray.destroy();
+        this._retainedManyArray = null;
+      }
     }
+
     return this._manyArray;
   }
 
@@ -78,10 +91,14 @@ export default class ManyRelationship extends Relationship {
     super.addCanonicalInternalModel(internalModel, idx);
   }
 
-  inverseDidDematerialize() {
-    if (this._manyArray) {
-      this._manyArray.destroy();
-      this._manyArray = null;
+  inverseDidDematerialize(inverseInternalModel) {
+    super.inverseDidDematerialize(inverseInternalModel);
+    if (this.isAsync) {
+      if (this._manyArray) {
+        this._retainedManyArray = this._manyArray;
+        this._manyArray = null;
+      }
+      this._removeInternalModelFromManyArray(this._retainedManyArray, inverseInternalModel);
     }
     this.notifyHasManyChanged();
   }
@@ -111,6 +128,12 @@ export default class ManyRelationship extends Relationship {
     super.removeCanonicalInternalModelFromOwn(internalModel, idx);
   }
 
+  removeAllCanonicalInternalModelsFromOwn() {
+    super.removeAllCanonicalInternalModelsFromOwn();
+    this.canonicalMembers.clear();
+    this.canonicalState.splice(0, this.canonicalState.length);
+  }
+
   removeCompletelyFromOwn(internalModel) {
     super.removeCompletelyFromOwn(internalModel);
 
@@ -143,7 +166,33 @@ export default class ManyRelationship extends Relationship {
       return;
     }
     super.removeInternalModelFromOwn(internalModel, idx);
-    let manyArray = this.manyArray;
+    // note that ensuring the many array is created, via `this.manyArray`
+    // (instead of `this._manyArray`) is intentional.
+    //
+    // Because we're removing from local, and not canonical, state, it is
+    // important that the many array is initialized now with those changes,
+    // otherwise it will be initialized with canonical state and we'll have
+    // lost the fact that this internalModel was removed.
+    this._removeInternalModelFromManyArray(this.manyArray, internalModel, idx);
+    this._removeInternalModelFromManyArray(this._retainedManyArray, internalModel, idx);
+  }
+
+  removeAllInternalModelsFromOwn() {
+    super.removeAllInternalModelsFromOwn();
+    // as with removeInternalModelFromOwn, we make sure the many array is
+    // instantiated, or we'll lose local removals, as we're not updating
+    // canonical state here.
+    this.manyArray.clear();
+    if (this._retainedManyArray) {
+      this._retainedManyArray.clear();
+    }
+  }
+
+  _removeInternalModelFromManyArray(manyArray, internalModel, idx) {
+    if (manyArray === null) {
+      return;
+    }
+
     if (idx !== undefined) {
       //TODO(Igor) not used currently, fix
       manyArray.currentState.removeAt(idx);
@@ -292,12 +341,14 @@ export default class ManyRelationship extends Relationship {
     let manyArray = this._manyArray;
     if (manyArray) {
       manyArray.destroy();
+      this._manyArray = null;
     }
 
     let proxy = this.__loadingPromise;
 
     if (proxy) {
       proxy.destroy();
+      this.__loadingPromise = null;
     }
   }
 }

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

@@ -1,5 +1,6 @@
 /* global heimdall */
 import { guidFor } from '@ember/object/internals';
+import { get } from '@ember/object';
 
 import { assert, warn } from '@ember/debug';
 import OrderedSet from '../../ordered-set';
@@ -76,35 +77,67 @@ export default class Relationship {
     this.meta = null;
     this.hasData = false;
     this.hasLoaded = false;
+    this.__inverseMeta = undefined;
   }
 
-  get parentType() {
-    return this.internalModel.modelName;
+  _inverseIsAsync() {
+    let inverseMeta = this._inverseMeta;
+    if (!inverseMeta) {
+      return false;
+    }
+
+    let inverseAsync = inverseMeta.options.async;
+    return typeof inverseAsync === 'undefined' ? true : inverseAsync;
   }
 
-  _inverseIsAsync() {
-    if (!this.inverseKey || !this.inverseInternalModel) {
+  _inverseIsSync() {
+    let inverseMeta = this._inverseMeta;
+    if (!inverseMeta) {
       return false;
     }
-    return this.inverseInternalModel._relationships.get(this.inverseKey).isAsync;
+
+    let inverseAsync = inverseMeta.options.async;
+    return typeof inverseAsync === 'undefined' ? false : !inverseAsync;
   }
 
-  removeInverseRelationships() {
-    if (!this.inverseKey) { return; }
+  get _inverseMeta() {
+    if (this.__inverseMeta === undefined) {
+      let inverseMeta = null;
 
-    let allMembers =
-      // we actually want a union of members and canonicalMembers
-      // they should be disjoint but currently are not due to a bug
-      this.members.list.concat(this.canonicalMembers.list);
+      if (this.inverseKey) {
+        let inverseModelClass = this.store.modelFor(this.relationshipMeta.type);
+        let inverseRelationships = get(inverseModelClass, 'relationshipsByName');
+        inverseMeta = inverseRelationships.get(this.inverseKey);
+      }
 
-    for (let i = 0; i < allMembers.length; i++) {
-      let inverseInternalModel = allMembers[i];
-      let relationship = inverseInternalModel._relationships.get(this.inverseKey);
-      relationship.inverseDidDematerialize();
+      this.__inverseMeta = inverseMeta;
     }
+
+    return this.__inverseMeta;
+  }
+
+  get parentType() {
+    return this.internalModel.modelName;
+  }
+
+  internalModelDidDematerialize() {
+    if (!this.inverseKey) { return; }
+
+    this.forAllMembers((inverseInternalModel) => {
+      let relationship = inverseInternalModel._relationships.get(this.inverseKey);
+      relationship.inverseDidDematerialize(this.internalModel);
+    });
   }
 
-  inverseDidDematerialize() {}
+  inverseDidDematerialize(inverseInternalModel) {
+    if (!this.isAsync) {
+      // unloading inverse of a sync relationship is treated as a client-side
+      // delete, so actually remove the models don't merely invalidate the cp
+      // cache.
+      this.removeInternalModelFromOwn(inverseInternalModel);
+      this.removeCanonicalInternalModelFromOwn(inverseInternalModel);
+    }
+  }
 
   updateMeta(meta) {
     heimdall.increment(updateMeta);
@@ -127,6 +160,16 @@ export default class Relationship {
     }
   }
 
+  removeAllInternalModelsFromOwn() {
+    this.members.clear();
+    this.internalModel.updateRecordArrays();
+  }
+
+  removeAllCanonicalInternalModelsFromOwn() {
+    this.canonicalMembers.clear();
+    this.flushCanonicalLater();
+  }
+
   removeInternalModels(internalModels) {
     heimdall.increment(removeInternalModels);
     internalModels.forEach((internalModel) => this.removeInternalModel(internalModel));
@@ -181,7 +224,7 @@ export default class Relationship {
       let relationship = relationships[this.inverseKeyForImplicit];
       if (!relationship) {
         relationship = relationships[this.inverseKeyForImplicit] =
-          new Relationship(this.store, internalModel, this.key,  { options: { async: this.isAsync } });
+        new Relationship(this.store, internalModel, this.key, { options: { async: this.isAsync }, type: this.parentType });
       }
       relationship.addCanonicalInternalModel(this.internalModel);
     }
@@ -222,7 +265,12 @@ 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: { async: this.isAsync } });
+          internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(
+            this.store,
+            internalModel,
+            this.key,
+            { options: { async: this.isAsync }, type: this.parentType }
+          );
         }
         internalModel._implicitRelationships[this.inverseKeyForImplicit].addInternalModel(this.internalModel);
       }
@@ -303,6 +351,32 @@ export default class Relationship {
 
     this.members.forEach(unload);
     this.canonicalMembers.forEach(unload);
+
+    if (!this.isAsync) {
+      this.clear();
+    }
+  }
+
+  forAllMembers(callback) {
+    let seen = Object.create(null);
+
+    for (let i = 0; i < this.members.list.length; i++) {
+      const inverseInternalModel = this.members.list[i];
+      const id = guidFor(inverseInternalModel);
+      if (!seen[id]) {
+        seen[id] = true;
+        callback(inverseInternalModel);
+      }
+    }
+
+    for (let i = 0; i < this.canonicalMembers.list.length; i++) {
+      const inverseInternalModel = this.canonicalMembers.list[i];
+      const id = guidFor(inverseInternalModel);
+      if (!seen[id]) {
+        seen[id] = true;
+        callback(inverseInternalModel);
+      }
+    }
   }
 
   /*

tests/integration/records/rematerialize-test.js

@@ -110,7 +110,7 @@ test("a sync belongs to relationship to an unloaded record can restore that reco
   run(() => person.unloadRecord());
 
   assert.equal(env.store.hasRecordForId('person', 1), false, 'The person is unloaded');
-  assert.equal(env.store._internalModelsFor('person').has(1), true, 'The person internalModel is retained');
+  assert.equal(env.store._internalModelsFor('person').has(1), false, 'The person internalModel is freed');
 
   run(() => {
     env.store.push({