Merge pull request #5067 from runspired/fix-delete-with-local-state

[BUGFIX] Preserve local relationship changes after persisting a delet…

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

@@ -728,12 +728,6 @@ export default class InternalModel {
     }
   }
 
-  notifyHasManyRemoved(key, record, idx) {
-    if (this.hasRecord) {
-      this._record.notifyHasManyRemoved(key, record, idx);
-    }
-  }
-
   notifyBelongsToChanged(key, record) {
     if (this.hasRecord) {
       this._record.notifyBelongsToChanged(key, record);
@@ -768,7 +762,7 @@ export default class InternalModel {
     }
 
     if (this.isNew()) {
-      this.clearRelationships();
+      this.removeFromInverseRelationships(true);
     }
 
     if (this.isValid()) {
@@ -882,23 +876,43 @@ export default class InternalModel {
   }
 
   /*
-    @method clearRelationships
+   This method should only be called by records in the `isNew()` state OR once the record
+   has been deleted and that deletion has been persisted.
+
+   It will remove this record from any associated relationships.
+
+   If `isNew` is true (default false), it will also completely reset all
+    relationships to an empty state as well.
+
+    @method removeFromInverseRelationships
+    @param {Boolean} isNew whether to unload from the `isNew` perspective
     @private
-  */
-  clearRelationships() {
-    this.eachRelationship((name, relationship) => {
+   */
+  removeFromInverseRelationships(isNew = false) {
+    this.eachRelationship((name) => {
       if (this._relationships.has(name)) {
         let rel = this._relationships.get(name);
-        rel.clear();
-        rel.removeInverseRelationships();
+
+        rel.removeCompletelyFromInverse();
+        if (isNew === true) {
+          rel.clear();
+        }
       }
     });
     Object.keys(this._implicitRelationships).forEach((key) => {
-      this._implicitRelationships[key].clear();
-      this._implicitRelationships[key].removeInverseRelationships();
+      let rel = this._implicitRelationships[key];
+
+      rel.removeCompletelyFromInverse();
+      if (isNew === true) {
+        rel.clear();
+      }
     });
   }
 
+  /*
+    Notify all inverses that this internalModel has been dematerialized
+    and destroys any ManyArrays.
+   */
   destroyRelationships() {
     this.eachRelationship((name, relationship) => {
       if (this._relationships.has(name)) {
@@ -982,6 +996,8 @@ export default class InternalModel {
   }
 
   /*
+    Used to notify the store to update FilteredRecordArray membership.
+
     @method updateRecordArrays
     @private
   */

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

@@ -671,7 +671,7 @@ const RootState = {
       isDirty: false,
 
       setup(internalModel) {
-        internalModel.clearRelationships();
+        internalModel.removeFromInverseRelationships();
       },
 
       invokeLifecycleCallbacks(internalModel) {

addon/-private/system/record-array-manager.js

@@ -88,7 +88,7 @@ export default class RecordArrayManager {
       return;
     }
 
-    internalModel._pendingRecordArrayManagerFlush = true
+    internalModel._pendingRecordArrayManagerFlush = true;
 
     let pending = this._pending;
     let models = pending[modelName] = pending[modelName] || [];

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

@@ -63,6 +63,19 @@ export default class BelongsToRelationship extends Relationship {
     this.notifyBelongsToChanged();
   }
 
+  removeCompletelyFromOwn(internalModel) {
+    super.removeCompletelyFromOwn(internalModel);
+
+    if (this.canonicalState === internalModel) {
+      this.canonicalState = null;
+    }
+
+    if (this.inverseInternalModel === internalModel) {
+      this.inverseInternalModel = null;
+      this.notifyBelongsToChanged();
+    }
+  }
+
   flushCanonical() {
     //temporary fix to not remove newly created records if server returned null.
     //TODO remove once we have proper diffing

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

@@ -111,6 +111,26 @@ export default class ManyRelationship extends Relationship {
     super.removeCanonicalInternalModelFromOwn(internalModel, idx);
   }
 
+  removeCompletelyFromOwn(internalModel) {
+    super.removeCompletelyFromOwn(internalModel);
+
+    const canonicalIndex = this.canonicalState.indexOf(internalModel);
+
+    if (canonicalIndex !== -1) {
+      this.canonicalState.splice(canonicalIndex, 1);
+    }
+
+    const manyArray = this._manyArray;
+
+    if (manyArray) {
+      const idx = manyArray.currentState.indexOf(internalModel);
+
+      if (idx !== -1) {
+        manyArray.internalReplace(idx, 1);
+      }
+    }
+  }
+
   flushCanonical() {
     if (this._manyArray) {
       this._manyArray.flushCanonical();

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

@@ -2,6 +2,9 @@
 import { assert, warn } from '@ember/debug';
 import OrderedSet from '../../ordered-set';
 import _normalizeLink from '../../normalize-link';
+import Ember from 'ember';
+
+const { guidFor } = Ember;
 
 const {
   addCanonicalInternalModel,
@@ -247,7 +250,6 @@ export default class Relationship {
   removeInternalModelFromOwn(internalModel) {
     heimdall.increment(removeInternalModelFromOwn);
     this.members.delete(internalModel);
-    this.notifyRecordRelationshipRemoved(internalModel);
     this.internalModel.updateRecordArrays();
   }
 
@@ -266,6 +268,48 @@ export default class Relationship {
     this.flushCanonicalLater();
   }
 
+  /*
+    Call this method once a record deletion has been persisted
+    to purge it from BOTH current and canonical state of all
+    relationships.
+
+    @method removeCompletelyFromInverse
+    @private
+   */
+  removeCompletelyFromInverse() {
+    if (!this.inverseKey) { return; }
+
+    // we actually want a union of members and canonicalMembers
+    // they should be disjoint but currently are not due to a bug
+    let seen = Object.create(null);
+    const internalModel = this.internalModel;
+
+    const unload = inverseInternalModel => {
+      const id = guidFor(inverseInternalModel);
+
+      if (seen[id] === undefined) {
+        const relationship = inverseInternalModel._relationships.get(this.inverseKey);
+        relationship.removeCompletelyFromOwn(internalModel);
+        seen[id] = true;
+      }
+    };
+
+    this.members.forEach(unload);
+    this.canonicalMembers.forEach(unload);
+  }
+
+  /*
+    Removes the given internalModel from BOTH canonical AND current state.
+
+    This method is useful when either a deletion or a rollback on a new record
+    needs to entirely purge itself from an inverse relationship.
+   */
+  removeCompletelyFromOwn(internalModel) {
+    this.canonicalMembers.delete(internalModel);
+    this.members.delete(internalModel);
+    this.internalModel.updateRecordArrays();
+  }
+
   flushCanonical() {
     heimdall.increment(flushCanonical);
     let list = this.members.list;
@@ -327,7 +371,6 @@ export default class Relationship {
   }
 
   notifyRecordRelationshipAdded() { }
-  notifyRecordRelationshipRemoved() { }
 
   /*
    `hasData` for a relationship is a flag to indicate if we consider the

tests/integration/relationships/has-many-test.js

@@ -2520,7 +2520,9 @@ test("adding and removing records from hasMany relationship #2666", function(ass
         return comments.get('lastObject').destroyRecord();
       }).then(() => {
         let comments = post.get('comments');
-        assert.equal(comments.get('length'), 3, "Comments count after destroy");
+        let length = comments.get('length');
+
+        assert.equal(length, 3, "Comments count after destroy");
 
         // Add another comment #4
         let comment = env.store.createRecord('comment');