From c71284df5a5de905a09304bacd0bd2cab0c59c48 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Mon, 24 Jul 2017 20:34:57 -0700 Subject: [PATCH] [BUGFIX release] relationship [x, y] should not break on x.unloadRecord() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For an async relationship [x, y] with x.unloadRecord(), now adjusts only the relationship’s currentState, leaving that relationship’s canonical state alone, ensuring the existing client-side delete semantics are preserved. But when that relationship is reloaded, the canonicalState consulted. For sync relationship [x, y] with x.unloadRecord(), both currentState and canonical state are updated. This is to mirror the client-side delete semantics. But since we cannot reload a sync relationship we must assume this to be the new canonical state and rely on subsequent `push` or `adapterPayloads` or manual `store.push` to update. This aims to: * [FIX] hasMany arrays never contain dematerialized records (so they no longer become broken) * [FIX] using unloadRecord as a type of client side delete is restored * [PRESERVE] the garbage collector pass to cleanup orphaned models * [PRESERVE] second access to a relationship which did contain an unloadRecord to cause a reload note: if both sides of a relationships are unloaded, the above doesn’t apply. This is largely just when members of a loaded relationship are themselves unloaded. [fixes #4986 #5052 #4987 #4996] --- addon/-private/system/model/internal-model.js | 48 ++++++---- .../system/relationships/state/create.js | 7 ++ .../system/relationships/state/has-many.js | 14 +++ .../relationships/state/relationship.js | 14 ++- .../integration/records/rematerialize-test.js | 91 +++++++++---------- .../relationships/has-many-test.js | 4 +- tests/integration/store-test.js | 2 +- .../unit/model/relationships/has-many-test.js | 80 ++++++++++++++-- 8 files changed, 182 insertions(+), 78 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index ccc2ee168ef..3f8f8ad3c94 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -431,14 +431,10 @@ export default class InternalModel { */ _directlyRelatedInternalModels() { let array = []; - this.type.eachRelationship((key, relationship) => { - if (this._relationships.has(key)) { - let relationship = this._relationships.get(key); - let localRelationships = relationship.members.toArray(); - let serverRelationships = relationship.canonicalMembers.toArray(); - - array = array.concat(localRelationships, serverRelationships); - } + this._relationships.forEach((name, rel) => { + let local = rel.members.toArray(); + let server = rel.canonicalMembers.toArray(); + array = array.concat(local, server); }); return array; } @@ -492,6 +488,7 @@ export default class InternalModel { unloadRecord() { this.send('unloadRecord'); this.dematerializeRecord(); + if (this._scheduledDestroy === null) { this._scheduledDestroy = run.schedule('destroy', this, '_checkForOrphanedInternalModels'); } @@ -511,6 +508,7 @@ export default class InternalModel { if (this.isDestroyed) { return; } this._cleanupOrphanedInternalModels(); + } _cleanupOrphanedInternalModels() { @@ -533,6 +531,9 @@ export default class InternalModel { assert("Cannot destroy an internalModel while its record is materialized", !this._record || this._record.get('isDestroyed') || this._record.get('isDestroying')); this.store._internalModelDestroyed(this); + + this._relationships.forEach((name, rel) => rel.destroy()); + this._isDestroyed = true; } @@ -889,14 +890,10 @@ export default class InternalModel { @private */ removeFromInverseRelationships(isNew = false) { - this.eachRelationship((name) => { - if (this._relationships.has(name)) { - let rel = this._relationships.get(name); - - rel.removeCompletelyFromInverse(); - if (isNew === true) { - rel.clear(); - } + this._relationships.forEach((name, rel) => { + rel.removeCompletelyFromInverse(); + if (isNew === true) { + rel.clear(); } }); @@ -918,17 +915,28 @@ export default class InternalModel { and destroys any ManyArrays. */ destroyRelationships() { - this.eachRelationship((name, relationship) => { - if (this._relationships.has(name)) { - let rel = this._relationships.get(name); + this._relationships.forEach((name, rel) => { + if (rel._inverseIsAsync()) { + rel.removeInternalModelFromInverse(this); rel.removeInverseRelationships(); + } else { + rel.removeCompletelyFromInverse(); } }); let implicitRelationships = this._implicitRelationships; this.__implicitRelationships = null; Object.keys(implicitRelationships).forEach((key) => { - implicitRelationships[key].removeInverseRelationships(); + let rel = implicitRelationships[key]; + + if (rel._inverseIsAsync()) { + rel.removeInternalModelFromInverse(this); + rel.removeInverseRelationships(); + } else { + rel.removeCompletelyFromInverse(); + } + + rel.destroy(); }); } diff --git a/addon/-private/system/relationships/state/create.js b/addon/-private/system/relationships/state/create.js index f0fe88c7cbc..0acc2b86adf 100644 --- a/addon/-private/system/relationships/state/create.js +++ b/addon/-private/system/relationships/state/create.js @@ -46,6 +46,13 @@ export default class Relationships { return !!this.initializedRelationships[key]; } + forEach(cb) { + let rels = this.initializedRelationships; + Object.keys(rels).forEach(name => { + cb(name, rels[name]); + }); + } + get(key) { let relationships = this.initializedRelationships; let relationship = relationships[key]; diff --git a/addon/-private/system/relationships/state/has-many.js b/addon/-private/system/relationships/state/has-many.js index 10cb1e8826a..135c660f3f5 100755 --- a/addon/-private/system/relationships/state/has-many.js +++ b/addon/-private/system/relationships/state/has-many.js @@ -281,6 +281,20 @@ export default class ManyRelationship extends Relationship { this.updateInternalModelsFromAdapter(internalModels); } } + + destroy() { + super.destroy(); + let manyArray = this._manyArray; + if (manyArray) { + manyArray.destroy(); + } + + let proxy = this.__loadingPromise; + + if (proxy) { + proxy.destroy(); + } + } } function setForArray(array) { diff --git a/addon/-private/system/relationships/state/relationship.js b/addon/-private/system/relationships/state/relationship.js index 4bf194c2567..8a6893c4f79 100644 --- a/addon/-private/system/relationships/state/relationship.js +++ b/addon/-private/system/relationships/state/relationship.js @@ -83,6 +83,13 @@ export default class Relationship { return this.internalModel.modelName; } + _inverseIsAsync() { + if (!this.inverseKey || !this.inverseInternalModel) { + return false; + } + return this.inverseInternalModel._relationships.get(this.inverseKey).isAsync; + } + removeInverseRelationships() { if (!this.inverseKey) { return; } @@ -174,7 +181,7 @@ export default class Relationship { let relationship = relationships[this.inverseKeyForImplicit]; if (!relationship) { relationship = relationships[this.inverseKeyForImplicit] = - new Relationship(this.store, internalModel, this.key, { options: {} }); + new Relationship(this.store, internalModel, this.key, { options: { async: this.isAsync } }); } relationship.addCanonicalInternalModel(this.internalModel); } @@ -215,7 +222,7 @@ 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: {} }); + internalModel._implicitRelationships[this.inverseKeyForImplicit] = new Relationship(this.store, internalModel, this.key, { options: { async: this.isAsync } }); } internalModel._implicitRelationships[this.inverseKeyForImplicit].addInternalModel(this.internalModel); } @@ -454,4 +461,7 @@ export default class Relationship { } updateData() {} + + destroy() { + } } diff --git a/tests/integration/records/rematerialize-test.js b/tests/integration/records/rematerialize-test.js index 743db76a36b..b7a71c0be8f 100644 --- a/tests/integration/records/rematerialize-test.js +++ b/tests/integration/records/rematerialize-test.js @@ -146,22 +146,47 @@ test("an async has many relationship to an unloaded record can restore that reco // disable background reloading so we do not re-create the relationship. env.adapter.shouldBackgroundReloadRecord = () => false; - env.adapter.findRecord = function() { - assert.ok('adapter called'); - return Ember.RSVP.Promise.resolve({ - data: { - type: 'boat', - id: '1', - attributes: { - name: "Boaty McBoatface" - }, - relationships: { - person: { - data: { type: 'person', id: '1' } - } - } + const BOAT_ONE = { + type: 'boat', + id: '1', + attributes: { + name: "Boaty McBoatface" + }, + relationships: { + person: { + data: { type: 'person', id: '1' } } - }); + } + }; + + const BOAT_TWO = { + type: 'boat', + id: '2', + attributes: { + name: 'Some other boat' + }, + relationships: { + person: { + data: { type: 'person', id: '1' } + } + } + }; + + env.adapter.findRecord = function(store, model, param) { + assert.ok('adapter called'); + + let data; + if (param === '1') { + data = BOAT_ONE; + } else if (param === '1') { + data = BOAT_TWO; + } else { + throw new Error(`404: no such boat with id=${param}`); + } + + return { + data + }; } run(function() { @@ -186,29 +211,7 @@ test("an async has many relationship to an unloaded record can restore that reco run(function() { env.store.push({ - data: [{ - type: 'boat', - id: '2', - attributes: { - name: 'Some other boat' - }, - relationships: { - person: { - data: { type: 'person', id: '1' } - } - } - }, { - type: 'boat', - id: '1', - attributes: { - name: 'Boaty McBoatface' - }, - relationships: { - person: { - data: { type: 'person', id: '1' } - } - } - }] + data: [BOAT_ONE, BOAT_TWO] }); }); @@ -220,22 +223,16 @@ test("an async has many relationship to an unloaded record can restore that reco assert.equal(env.store.hasRecordForId('boat', 1), true, 'The boat is in the store'); assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is loaded'); - let boats = run(() => { - return adam.get('boats'); - }); + let boats = run(() => adam.get('boats')); assert.equal(boats.get('length'), 2, 'Before unloading boats.length is correct'); - run(function() { - boaty.unloadRecord(); - }); + run(() => boaty.unloadRecord()); assert.equal(env.store.hasRecordForId('boat', 1), false, 'The boat is unloaded'); assert.equal(env.store._internalModelsFor('boat').has(1), true, 'The boat internalModel is retained'); - let rematerializedBoaty = run(() => { - return rematerializedBoaty = adam.get('boats').objectAt(1); - }); + let rematerializedBoaty = run(() => adam.get('boats')).objectAt(0); assert.equal(adam.get('boats.length'), 2, 'boats.length correct after rematerialization'); assert.equal(rematerializedBoaty.get('id'), '1'); diff --git a/tests/integration/relationships/has-many-test.js b/tests/integration/relationships/has-many-test.js index c75749abe5a..5b6b8a49624 100644 --- a/tests/integration/relationships/has-many-test.js +++ b/tests/integration/relationships/has-many-test.js @@ -287,14 +287,14 @@ test("hasMany + canonical vs currentState + unloadRecord", function(assert) { env.store.peekRecord('user', 6).unloadRecord(); }); - assert.deepEqual(contacts.map(c => c.get('id')), ['2','3','4','5','6','7'], `user's contacts should have expected contacts`); + assert.deepEqual(contacts.map(c => c.get('id')), ['3','4','5','7'], `user's contacts should have expected contacts`); assert.equal(contacts, user.get('contacts')); run(() => { contacts.addObject(env.store.createRecord('user', { id: 8 })); }); - assert.deepEqual(contacts.map(c => c.get('id')), ['2','3','4','5','6','7','8'], `user's contacts should have expected contacts`); + assert.deepEqual(contacts.map(c => c.get('id')), ['3','4','5','7','8'], `user's contacts should have expected contacts`); assert.equal(contacts, user.get('contacts')); }); diff --git a/tests/integration/store-test.js b/tests/integration/store-test.js index f3db892465d..03abfe1b4b6 100644 --- a/tests/integration/store-test.js +++ b/tests/integration/store-test.js @@ -201,7 +201,7 @@ test("destroying the store correctly cleans everything up", function(assert) { assert.equal(personWillDestroy.called.length, 1, 'expected person to have recieved willDestroy once'); assert.equal(carWillDestroy.called.length, 1, 'expected car to recieve willDestroy once'); - assert.equal(carsWillDestroy.called.length, 1, 'expected cars to recieve willDestroy once'); + assert.equal(carsWillDestroy.called.length, 1, 'expected person.cars to recieve willDestroy once'); assert.equal(adapterPopulatedPeopleWillDestroy.called.length, 1, 'expected adapterPopulatedPeople to recieve willDestroy once'); assert.equal(filterdPeopleWillDestroy.called.length, 1, 'expected filterdPeople.willDestroy to have been called once'); }); diff --git a/tests/unit/model/relationships/has-many-test.js b/tests/unit/model/relationships/has-many-test.js index 4098a067102..cc22d6798b6 100644 --- a/tests/unit/model/relationships/has-many-test.js +++ b/tests/unit/model/relationships/has-many-test.js @@ -1339,6 +1339,71 @@ test('deleting an item that is the current state of a belongsTo clears currentSt }); }); +test('hasMany.firstObject.unloadRecord should not break that hasMany', function(assert) { + const Person = DS.Model.extend({ + cars: DS.hasMany('car', { async: false }), + name: DS.attr() + }); + + Person.reopenClass({ + toString() { + return 'person'; + } + }); + + const Car = DS.Model.extend({ + name: DS.attr() + }); + + Car.reopenClass({ + toString() { + return 'car'; + } + }); + + let env = setupStore({ + person: Person, + car: Car + }); + + run(() => { + env.store.push({ + data: [ + { + type: 'person', + id: 1, + attributes: { + name: 'marvin' + }, + relationships: { + cars: { + data: [ + { type: 'car', id: 1 }, + { type: 'car', id: 2 } + ] + } + } + }, + { type: 'car', id: 1, attributes: { name: 'a' } }, + { type: 'car', id: 2, attributes: { name: 'b' } } + ] + }) + }); + + let person = env.store.peekRecord('person', 1); + let cars = person.get('cars'); + + assert.equal(cars.get('length'), 2); + + run(() => { + cars.get('firstObject').unloadRecord(); + // assert.equal(cars.get('length'), 1); // unload now.. + // assert.equal(person.get('cars.length'), 1); // unload now.. + }); + + // assert.equal(cars.get('length'), 1); // unload now.. + assert.equal(person.get('cars.length'), 1); // unload now.. +}); /* This test, when passing, affirms that a known limitation of ember-data still exists. @@ -1692,16 +1757,19 @@ test('DS.hasMany proxy is destroyed', function(assert) { let { store } = setupStore({ tag: Tag, person: Person }); let tag = run(() => store.createRecord('tag')); - let people = tag.get('people'); + let peopleProxy = tag.get('people'); - return people.then(() => { + return peopleProxy.then(people => { Ember.run(() => { tag.unloadRecord(); - assert.equal(people.get('isDestroying'), true); - assert.equal(people.get('isDestroyed'), false); + assert.equal(people.isDestroying, false, 'people is destroying sync after unloadRecord'); + assert.equal(people.isDestroyed, false, 'people is NOT YET destroyed sync after unloadRecord'); + assert.equal(peopleProxy.isDestroying, false, 'peopleProxy is destroying sync after unloadRecord'); + assert.equal(peopleProxy.isDestroyed, false, 'peopleProxy is NOT YET destroyed sync after unloadRecord'); }); - assert.equal(people.get('isDestroying'), true); - assert.equal(people.get('isDestroyed'), true); + + assert.equal(peopleProxy.isDestroying, true, 'peopleProxy is destroying after the run post unloadRecord'); + assert.equal(peopleProxy.isDestroyed, true, 'peopleProxy is destroying after the run post unloadRecord'); }) });