From 16b8a045779a4fd47e8251ab2c6cd4a76ab2824e Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Fri, 14 Jul 2017 12:41:58 -0700 Subject: [PATCH] Revert "[BUGFIX] keep local state after a deletion" --- 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 | 4 +- .../unit/model/relationships/has-many-test.js | 553 ------------------ 8 files changed, 20 insertions(+), 667 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index 44624909f94..e1cdbd5a9b4 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -728,6 +728,12 @@ 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); @@ -762,7 +768,7 @@ export default class InternalModel { } if (this.isNew()) { - this.removeFromInverseRelationships(true); + this.clearRelationships(); } if (this.isValid()) { @@ -876,43 +882,23 @@ export default class InternalModel { } /* - 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 + @method clearRelationships @private - */ - removeFromInverseRelationships(isNew = false) { - this.eachRelationship((name) => { + */ + clearRelationships() { + this.eachRelationship((name, relationship) => { if (this._relationships.has(name)) { let rel = this._relationships.get(name); - - rel.removeCompletelyFromInverse(); - if (isNew === true) { - rel.clear(); - } + rel.clear(); + rel.removeInverseRelationships(); } }); Object.keys(this._implicitRelationships).forEach((key) => { - let rel = this._implicitRelationships[key]; - - rel.removeCompletelyFromInverse(); - if (isNew === true) { - rel.clear(); - } + this._implicitRelationships[key].clear(); + this._implicitRelationships[key].removeInverseRelationships(); }); } - /* - Notify all inverses that this internalModel has been dematerialized - and destroys any ManyArrays. - */ destroyRelationships() { this.eachRelationship((name, relationship) => { if (this._relationships.has(name)) { @@ -996,8 +982,6 @@ 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 4977133fefc..b14e9843b81 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.removeFromInverseRelationships(); + internalModel.clearRelationships(); }, invokeLifecycleCallbacks(internalModel) { diff --git a/addon/-private/system/record-array-manager.js b/addon/-private/system/record-array-manager.js index 483e7766502..bf8e4fbd3d4 100644 --- a/addon/-private/system/record-array-manager.js +++ b/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] || []; diff --git a/addon/-private/system/relationships/state/belongs-to.js b/addon/-private/system/relationships/state/belongs-to.js index 79ab00e7700..2959d13c25c 100644 --- a/addon/-private/system/relationships/state/belongs-to.js +++ b/addon/-private/system/relationships/state/belongs-to.js @@ -63,19 +63,6 @@ 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 10cb1e8826a..89f7e511e68 100755 --- a/addon/-private/system/relationships/state/has-many.js +++ b/addon/-private/system/relationships/state/has-many.js @@ -111,26 +111,6 @@ 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 4bf194c2567..43cc279a196 100644 --- a/addon/-private/system/relationships/state/relationship.js +++ b/addon/-private/system/relationships/state/relationship.js @@ -2,9 +2,6 @@ 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, @@ -250,6 +247,7 @@ export default class Relationship { removeInternalModelFromOwn(internalModel) { heimdall.increment(removeInternalModelFromOwn); this.members.delete(internalModel); + this.notifyRecordRelationshipRemoved(internalModel); this.internalModel.updateRecordArrays(); } @@ -268,48 +266,6 @@ 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; @@ -371,6 +327,7 @@ 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 4fe9e40c68e..9431753f66d 100644 --- a/tests/integration/relationships/has-many-test.js +++ b/tests/integration/relationships/has-many-test.js @@ -2520,9 +2520,7 @@ test("adding and removing records from hasMany relationship #2666", function(ass return comments.get('lastObject').destroyRecord(); }).then(() => { let comments = post.get('comments'); - let length = comments.get('length'); - - assert.equal(length, 3, "Comments count after destroy"); + assert.equal(comments.get('length'), 3, "Comments count after destroy"); // Add another comment #4 let 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 4098a067102..b9f3d391d5a 100644 --- a/tests/unit/model/relationships/has-many-test.js +++ b/tests/unit/model/relationships/has-many-test.js @@ -906,559 +906,6 @@ 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 Ember.RSVP.Promise.resolve({ data: 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 Ember.RSVP.Promise.resolve({ data: 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 Ember.RSVP.Promise.resolve({ data: 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 Ember.RSVP.Promise.resolve({ data: 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 Ember.RSVP.Promise.resolve({ data: 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; - env.adapter.deleteRecord = () => { - return Ember.RSVP.Promise.resolve({ - 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, we lost the local change'); - }); - }); -}); - 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);