diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index 348cdbedb02..06b283de3e1 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -727,12 +727,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); @@ -767,7 +761,7 @@ export default class InternalModel { } if (this.isNew()) { - this.clearRelationships(); + this.removeFromInverseRelationships(true); } if (this.isValid()) { @@ -881,23 +875,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)) { @@ -981,6 +995,8 @@ export default class InternalModel { } /* + Used to notify the store to update FilteredRecordArray membership. + @method updateRecordArrays @private */ diff --git a/addon/-private/system/model/states.js b/addon/-private/system/model/states.js index 4507f5277df..f2ff22e0942 100644 --- a/addon/-private/system/model/states.js +++ b/addon/-private/system/model/states.js @@ -671,7 +671,7 @@ const RootState = { isDirty: false, setup(internalModel) { - internalModel.clearRelationships(); + internalModel.removeFromInverseRelationships(); }, invokeLifecycleCallbacks(internalModel) { diff --git a/addon/-private/system/record-array-manager.js b/addon/-private/system/record-array-manager.js index 6c2a227d892..8f57b6f9ab0 100644 --- a/addon/-private/system/record-array-manager.js +++ b/addon/-private/system/record-array-manager.js @@ -87,7 +87,7 @@ export default class RecordArrayManager { return; } - internalModel._pendingRecordArrayManagerFlush = true + internalModel._pendingRecordArrayManagerFlush = true; let pending = this._pending; let models = pending[modelName] = pending[modelName] || []; diff --git a/addon/-private/system/relationships/state/belongs-to.js b/addon/-private/system/relationships/state/belongs-to.js index cd9c2c01c9d..dad4b527101 100644 --- a/addon/-private/system/relationships/state/belongs-to.js +++ b/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 diff --git a/addon/-private/system/relationships/state/has-many.js b/addon/-private/system/relationships/state/has-many.js index e504c0dcb2c..e700112ff7f 100644 --- a/addon/-private/system/relationships/state/has-many.js +++ b/addon/-private/system/relationships/state/has-many.js @@ -110,6 +110,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(); diff --git a/addon/-private/system/relationships/state/relationship.js b/addon/-private/system/relationships/state/relationship.js index 84ebe6771ff..fff2b92be17 100644 --- a/addon/-private/system/relationships/state/relationship.js +++ b/addon/-private/system/relationships/state/relationship.js @@ -2,6 +2,9 @@ import { assert, warn } from 'ember-data/-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; @@ -326,7 +370,6 @@ export default class Relationship { } notifyRecordRelationshipAdded() { } - notifyRecordRelationshipRemoved() { } /* `hasData` for a relationship is a flag to indicate if we consider the diff --git a/tests/integration/relationships/has-many-test.js b/tests/integration/relationships/has-many-test.js index 250bcd5eb0e..5794fc3c364 100644 --- a/tests/integration/relationships/has-many-test.js +++ b/tests/integration/relationships/has-many-test.js @@ -2279,9 +2279,11 @@ test("adding and removing records from hasMany relationship #2666", function(ass // Delete comment #4 return comments.get('lastObject').destroyRecord(); - }).then(function() { - var comments = post.get('comments'); - assert.equal(comments.get('length'), 3, "Comments count after destroy"); + }).then(() => { + let comments = post.get('comments'); + let length = comments.get('length'); + + assert.equal(length, 3, "Comments count after destroy"); // Add another comment #4 var comment = env.store.createRecord('comment'); diff --git a/tests/unit/model/relationships/has-many-test.js b/tests/unit/model/relationships/has-many-test.js index 3fc39968926..c2007619266 100644 --- a/tests/unit/model/relationships/has-many-test.js +++ b/tests/unit/model/relationships/has-many-test.js @@ -895,6 +895,567 @@ test('it is possible to add a new item to a relationship', function(assert) { }); }); +test('new items added to a hasMany relationship are not cleared by a delete', function(assert) { + assert.expect(4); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + pets: DS.hasMany('pet', { async: false, inverse: null }) + }); + + const Pet = DS.Model.extend({ + name: DS.attr('string'), + person: DS.belongsTo('person', { async: false, inverse: null }) + }); + + let env = setupStore({ + person: Person, + pet: Pet + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + env.adapter.deleteRecord = () => { + return null; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + pets: { + data: [ + { type: 'pet', id: '1' } + ] + } + } + }, + included: [ + { + type: 'pet', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'pet', + id: '2', + attributes: { + name: 'Rambunctious' + } + }, + { + type: 'pet', + id: '3', + attributes: { + name: 'Rebel' + } + } + ] + }); + }); + + const person = store.peekRecord('person', '1'); + const pets = run(() => person.get('pets')); + + const shen = pets.objectAt(0); + const rambo = store.peekRecord('pet', '2'); + const rebel = store.peekRecord('pet', '3'); + + assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work'); + assert.deepEqual(pets.map(p => get(p, 'id')), ['1'], 'precond - relationship has the correct pets to start'); + + run(() => { + pets.pushObjects([rambo, rebel]); + }); + + assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2', '3'], 'precond2 - relationship now has the correct three pets'); + + run(() => { + return shen.destroyRecord({}) + .then(() => { + shen.unloadRecord(); + }); + }); + + assert.deepEqual(pets.map(p => get(p, 'id')), ['2', '3'], 'relationship now has the correct two pets'); +}); + +test('new items added to an async hasMany relationship are not cleared by a delete', function(assert) { + assert.expect(7); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + pets: DS.hasMany('pet', { async: true, inverse: null }) + }); + + const Pet = DS.Model.extend({ + name: DS.attr('string'), + person: DS.belongsTo('person', { async: false, inverse: null }) + }); + + let env = setupStore({ + person: Person, + pet: Pet + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + env.adapter.deleteRecord = () => { + return null; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + pets: { + data: [ + { type: 'pet', id: '1' } + ] + } + } + }, + included: [ + { + type: 'pet', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'pet', + id: '2', + attributes: { + name: 'Rambunctious' + } + }, + { + type: 'pet', + id: '3', + attributes: { + name: 'Rebel' + } + } + ] + }); + }); + + return run(() => { + const person = store.peekRecord('person', '1'); + const petsProxy = run(() => person.get('pets')); + + return petsProxy.then((pets) => { + const shen = pets.objectAt(0); + const rambo = store.peekRecord('pet', '2'); + const rebel = store.peekRecord('pet', '3'); + + assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work'); + assert.deepEqual(pets.map(p => get(p, 'id')), ['1'], 'precond - relationship has the correct pet to start'); + assert.equal(get(petsProxy, 'length'), 1, 'precond - proxy has only one pet to start'); + + pets.pushObjects([rambo, rebel]); + + assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2', '3'], 'precond2 - relationship now has the correct three pets'); + assert.equal(get(petsProxy, 'length'), 3, 'precond2 - proxy now reflects three pets'); + + return shen.destroyRecord({}) + .then(() => { + shen.unloadRecord(); + + assert.deepEqual(pets.map(p => get(p, 'id')), ['2', '3'], 'relationship now has the correct two pets'); + assert.equal(get(petsProxy, 'length'), 2, 'proxy now reflects two pets'); + }); + }); + }); +}); + +test('new items added to a belongsTo relationship are not cleared by a delete', function(assert) { + assert.expect(4); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + dog: DS.belongsTo('dog', { async: false, inverse: null }) + }); + + const Dog = DS.Model.extend({ + name: DS.attr('string') + }); + + let env = setupStore({ + person: Person, + dog: Dog + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + env.adapter.deleteRecord = () => { + return null; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + dog: { + data: { type: 'dog', id: '1' } + } + } + }, + included: [ + { + type: 'dog', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'dog', + id: '2', + attributes: { + name: 'Rambunctious' + } + } + ] + }); + }); + + const person = store.peekRecord('person', '1'); + let dog = run(() => person.get('dog')); + const shen = store.peekRecord('dog', '1'); + const rambo = store.peekRecord('dog', '2'); + + assert.ok(dog === shen, 'precond - the belongsTo points to the correct dog'); + assert.equal(get(dog, 'name'), 'Shenanigans', 'precond - relationships work'); + + run(() => { + person.set('dog', rambo); + }); + + dog = person.get('dog'); + assert.equal(dog, rambo, 'precond2 - relationship was updated'); + + return run(() => { + return shen.destroyRecord({}) + .then(() => { + shen.unloadRecord(); + + dog = person.get('dog'); + assert.equal(dog, rambo, 'The currentState of the belongsTo was preserved after the delete'); + }); + }); +}); + +test('new items added to an async belongsTo relationship are not cleared by a delete', function(assert) { + assert.expect(4); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + dog: DS.belongsTo('dog', { async: true, inverse: null }) + }); + + const Dog = DS.Model.extend({ + name: DS.attr('string') + }); + + let env = setupStore({ + person: Person, + dog: Dog + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + env.adapter.deleteRecord = () => { + return null; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + dog: { + data: { type: 'dog', id: '1' } + } + } + }, + included: [ + { + type: 'dog', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'dog', + id: '2', + attributes: { + name: 'Rambunctious' + } + } + ] + }); + }); + + return run(() => { + const person = store.peekRecord('person', '1'); + const shen = store.peekRecord('dog', '1'); + const rambo = store.peekRecord('dog', '2'); + + return person.get('dog').then((dog) => { + assert.ok(dog === shen, 'precond - the belongsTo points to the correct dog'); + assert.equal(get(dog, 'name'), 'Shenanigans', 'precond - relationships work'); + + person.set('dog', rambo); + + dog = person.get('dog.content'); + + assert.ok(dog === rambo, 'precond2 - relationship was updated'); + + return shen.destroyRecord({}) + .then(() => { + shen.unloadRecord(); + + dog = person.get('dog.content'); + assert.ok(dog === rambo, 'The currentState of the belongsTo was preserved after the delete'); + }); + }); + }); +}); + +test('deleting an item that is the current state of a belongsTo clears currentState', function(assert) { + assert.expect(4); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + dog: DS.belongsTo('dog', { async: false, inverse: null }) + }); + + const Dog = DS.Model.extend({ + name: DS.attr('string') + }); + + let env = setupStore({ + person: Person, + dog: Dog + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + env.adapter.deleteRecord = () => { + return null; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + dog: { + data: { type: 'dog', id: '1' } + } + } + }, + included: [ + { + type: 'dog', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'dog', + id: '2', + attributes: { + name: 'Rambunctious' + } + } + ] + }); + }); + + const person = store.peekRecord('person', '1'); + let dog = run(() => person.get('dog')); + const shen = store.peekRecord('dog', '1'); + const rambo = store.peekRecord('dog', '2'); + + assert.ok(dog === shen, 'precond - the belongsTo points to the correct dog'); + assert.equal(get(dog, 'name'), 'Shenanigans', 'precond - relationships work'); + + run(() => { + person.set('dog', rambo); + }); + + dog = person.get('dog'); + assert.equal(dog, rambo, 'precond2 - relationship was updated'); + + return run(() => { + return rambo.destroyRecord({}) + .then(() => { + rambo.unloadRecord(); + + dog = person.get('dog'); + assert.equal(dog, null, 'The current state of the belongsTo was clearer'); + }); + }); +}); + +/* + This test, when passing, affirms that a known limitation of ember-data still exists. + + When pushing new data into the store, ember-data is currently incapable of knowing whether + a relationship has been persisted. In order to update relationship state effectively, ember-data + blindly "flushes canonical" state, removing any `currentState` changes. A delete that sideloads + the parent record's hasMany is a situation in which this limitation will be encountered should other + local changes to the relationship still exist. + */ +test('[ASSERTS KNOWN LIMITATION STILL EXISTS] returning new hasMany relationship info from a delete clears local state', function(assert) { + assert.expect(4); + + const Person = DS.Model.extend({ + name: DS.attr('string'), + pets: DS.hasMany('pet', { async: false, inverse: null }) + }); + + const Pet = DS.Model.extend({ + name: DS.attr('string'), + person: DS.belongsTo('person', { async: false, inverse: null }) + }); + + let env = setupStore({ + person: Person, + pet: Pet + }); + env.adapter.shouldBackgroundReloadRecord = () => false; + let _super = env.serializer.normalize; + env.serializer.normalize = function(klass, payload) { + let normalized = _super.apply(this, ...arguments); + if (payload.data === null) { + normalized.data = null; + } + return normalized; + }; + + env.adapter.deleteRecord = () => { + return { + included: [ + { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + pets: { + data: [ + { type: 'pet', id: '2' } + ] + } + } + } + ] + }; + }; + + let { store } = env; + + run(() => { + store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Chris Thoburn' + }, + relationships: { + pets: { + data: [ + { type: 'pet', id: '1' }, + { type: 'pet', id: '2' } + ] + } + } + }, + included: [ + { + type: 'pet', + id: '1', + attributes: { + name: 'Shenanigans' + } + }, + { + type: 'pet', + id: '2', + attributes: { + name: 'Rambunctious' + } + }, + { + type: 'pet', + id: '3', + attributes: { + name: 'Rebel' + } + } + ] + }); + }); + + const person = store.peekRecord('person', '1'); + const pets = run(() => person.get('pets')); + + const shen = store.peekRecord('pet', '1'); + const rebel = store.peekRecord('pet', '3'); + + assert.equal(get(shen, 'name'), 'Shenanigans', 'precond - relationships work'); + assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2'], 'precond - relationship has the correct pets to start'); + + run(() => { + pets.pushObjects([rebel]); + }); + + assert.deepEqual(pets.map(p => get(p, 'id')), ['1', '2', '3'], 'precond2 - relationship now has the correct three pets'); + + return run(() => { + return shen.destroyRecord({}) + .then(() => { + shen.unloadRecord(); + + // were ember-data to now preserve local edits during a relationship push, this would be '2' + assert.deepEqual(pets.map(p => get(p, 'id')), ['2'], 'relationship now has only one pet, as the server overwrote the UI state'); + }); + }); +}); + test('possible to replace items in a relationship using setObjects w/ Ember Enumerable Array/Object as the argument (GH-2533)', function(assert) { assert.expect(2);