From 47e6ae7dc7d193a668b538877fe4f36943753cda Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Fri, 14 Jul 2017 12:49:01 -0700 Subject: [PATCH] [BUGFIX] Preserve local relationship changes after persisting a deletion when possible. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This reverts commit 16b8a045779a4fd47e8251ab2c6cd4a76ab2824e. — The `deleteRecord() { return null; }` changes are to allow the back ported tests to work with the older JSONSerializer rather then the JSONAPISerializer which the test suite (in beta and master) use. For reference the JSONSerializer -> JSONAPISerializer landed -> https://github.com/emberjs/data/pull/5003 --- addon/-private/system/model/internal-model.js | 46 +- addon/-private/system/model/states.js | 2 +- addon/-private/system/record-array-manager.js | 2 +- .../system/relationships/state/belongs-to.js | 13 + .../system/relationships/state/has-many.js | 20 + .../relationships/state/relationship.js | 47 +- .../relationships/has-many-test.js | 8 +- .../unit/model/relationships/has-many-test.js | 564 ++++++++++++++++++ 8 files changed, 680 insertions(+), 22 deletions(-) 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..56792ac8fd9 100644 --- a/tests/unit/model/relationships/has-many-test.js +++ b/tests/unit/model/relationships/has-many-test.js @@ -895,6 +895,570 @@ 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) { + // this is required, so that the backported test actually has meaning on the release branch + let normalized = _super.call(this, klass, payload); + if (payload.data === null) { + normalized.data = null; + } + normalized.included = payload.included; + return normalized; + }; + + env.adapter.deleteRecord = () => { + return { + data: null, + 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);