From 5f5d5d17ed5adab3f2d56a2f8c50a1161a870d17 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Fri, 25 Aug 2017 05:43:36 -0700 Subject: [PATCH] [BUGFIX release] ensure inverse async HasMany is correctly maintained In order to ensure symmetry between async and sync relationships, on destroy/unload of a record we must remove it from existing hasManys Prior to this it appears in the inverse async case accidentally removed the inverse of the inverse which is wrong, and may also not exist at that name. How this manifests --- addon/-private/system/model/internal-model.js | 31 ++++++++----------- .../relationships/state/relationship.js | 7 +++-- .../integration/records/rematerialize-test.js | 23 +++++++------- yarn.lock | 16 +++++----- 4 files changed, 36 insertions(+), 41 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index f8f1f498c9f..62f27deec0b 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -68,6 +68,14 @@ function areAllModelsUnloaded(internalModels) { return true; } +function destroyRelationship(rel) { + if (rel._inverseIsAsync()) { + rel.removeInternalModelFromInverse(rel.inverseInternalModel); + rel.removeInverseRelationships(); + } else { + rel.removeCompletelyFromInverse(); + } +} // this (and all heimdall instrumentation) will be stripped by a babel transform // https://github.com/heimdalljs/babel5-plugin-strip-heimdall const { @@ -432,9 +440,7 @@ export default class InternalModel { _directlyRelatedInternalModels() { let array = []; this._relationships.forEach((name, rel) => { - let local = rel.members.toArray(); - let server = rel.canonicalMembers.toArray(); - array = array.concat(local, server); + array = array.concat(rel.members.list, rel.canonicalMembers.list); }); return array; } @@ -486,6 +492,7 @@ export default class InternalModel { once all models that refer to it via some relationship are also unloaded. */ unloadRecord() { + if (this.isDestroyed) { return; } this.send('unloadRecord'); this.dematerializeRecord(); @@ -539,7 +546,6 @@ export default class InternalModel { if (this.isDestroyed) { return; } this._cleanupOrphanedInternalModels(); - } _cleanupOrphanedInternalModels() { @@ -946,26 +952,15 @@ export default class InternalModel { and destroys any ManyArrays. */ destroyRelationships() { - this._relationships.forEach((name, rel) => { - if (rel._inverseIsAsync()) { - rel.removeInternalModelFromInverse(this); - rel.removeInverseRelationships(); - } else { - rel.removeCompletelyFromInverse(); - } - }); + let relationships = this._relationships; + relationships.forEach((name, rel) => destroyRelationship(rel)); let implicitRelationships = this._implicitRelationships; this.__implicitRelationships = null; Object.keys(implicitRelationships).forEach((key) => { let rel = implicitRelationships[key]; - if (rel._inverseIsAsync()) { - rel.removeInternalModelFromInverse(this); - rel.removeInverseRelationships(); - } else { - rel.removeCompletelyFromInverse(); - } + destroyRelationship(rel); rel.destroy(); }); diff --git a/addon/-private/system/relationships/state/relationship.js b/addon/-private/system/relationships/state/relationship.js index 2d1792d9877..140d997d3ca 100644 --- a/addon/-private/system/relationships/state/relationship.js +++ b/addon/-private/system/relationships/state/relationship.js @@ -96,12 +96,13 @@ export default class Relationship { let allMembers = // we actually want a union of members and canonicalMembers // they should be disjoint but currently are not due to a bug - this.members.toArray().concat(this.canonicalMembers.toArray()); + this.members.list.concat(this.canonicalMembers.list); - allMembers.forEach(inverseInternalModel => { + for (let i = 0; i < allMembers.length; i++) { + let inverseInternalModel = allMembers[i]; let relationship = inverseInternalModel._relationships.get(this.inverseKey); relationship.inverseDidDematerialize(); - }); + } } inverseDidDematerialize() {} diff --git a/tests/integration/records/rematerialize-test.js b/tests/integration/records/rematerialize-test.js index b7a71c0be8f..d3aa5a738a4 100644 --- a/tests/integration/records/rematerialize-test.js +++ b/tests/integration/records/rematerialize-test.js @@ -57,12 +57,10 @@ module("integration/unload - Rematerializing Unloaded Records", { }); test("a sync belongs to relationship to an unloaded record can restore that record", function(assert) { - let adam, bob; - // disable background reloading so we do not re-create the relationship. env.adapter.shouldBackgroundReloadRecord = () => false; - run(function() { + let adam = run(() => { env.store.push({ data: { type: 'person', @@ -79,10 +77,11 @@ test("a sync belongs to relationship to an unloaded record can restore that reco } } }); - adam = env.store.peekRecord('person', 1); + + return env.store.peekRecord('person', 1); }); - run(function() { + let bob = run(() => { env.store.push({ data: { type: 'car', @@ -98,7 +97,8 @@ test("a sync belongs to relationship to an unloaded record can restore that reco } } }); - bob = env.store.peekRecord('car', 1); + + return env.store.peekRecord('car', 1); }); let person = env.store.peekRecord('person', 1); @@ -107,9 +107,7 @@ test("a sync belongs to relationship to an unloaded record can restore that reco assert.equal(env.store.hasRecordForId('person', 1), true, 'The person is in the store'); assert.equal(env.store._internalModelsFor('person').has(1), true, 'The person internalModel is loaded'); - run(function() { - person.unloadRecord(); - }); + run(() => person.unloadRecord()); assert.equal(env.store.hasRecordForId('person', 1), false, 'The person is unloaded'); assert.equal(env.store._internalModelsFor('person').has(1), true, 'The person internalModel is retained'); @@ -141,7 +139,7 @@ test("a sync belongs to relationship to an unloaded record can restore that reco }); test("an async has many relationship to an unloaded record can restore that record", function(assert) { - assert.expect(14); + assert.expect(15); // disable background reloading so we do not re-create the relationship. env.adapter.shouldBackgroundReloadRecord = () => false; @@ -189,7 +187,7 @@ test("an async has many relationship to an unloaded record can restore that reco }; } - run(function() { + run(() => { env.store.push({ data: { type: 'person', @@ -209,7 +207,7 @@ test("an async has many relationship to an unloaded record can restore that reco }); }); - run(function() { + run(() => { env.store.push({ data: [BOAT_ONE, BOAT_TWO] }); @@ -228,6 +226,7 @@ test("an async has many relationship to an unloaded record can restore that reco assert.equal(boats.get('length'), 2, 'Before unloading boats.length is correct'); run(() => boaty.unloadRecord()); + assert.equal(boats.get('length'), 1, 'after unloading boats.length is correct'); 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'); diff --git a/yarn.lock b/yarn.lock index 7a7783e870c..9b557f2fb2b 100644 --- a/yarn.lock +++ b/yarn.lock @@ -107,6 +107,12 @@ amd-name-resolver@0.0.6: dependencies: ensure-posix-path "^1.0.1" +amd-name-resolver@0.0.7: + version "0.0.7" + resolved "https://registry.yarnpkg.com/amd-name-resolver/-/amd-name-resolver-0.0.7.tgz#814301adfe8a2f109f6e84d5e935196efb669615" + dependencies: + ensure-posix-path "^1.0.1" + amdefine@>=0.0.4: version "1.0.1" resolved "https://registry.npmjs.org/amdefine/-/amdefine-1.0.1.tgz#4a5282ac164729e93619bcfd3ad151f817ce91f5" @@ -1018,7 +1024,7 @@ bower-endpoint-parser@0.2.2: version "0.2.2" resolved "https://registry.npmjs.org/bower-endpoint-parser/-/bower-endpoint-parser-0.2.2.tgz#00b565adbfab6f2d35addde977e97962acbcb3f6" -bower@^1.6.5, bower@^1.8.0: +bower@^1.8.0: version "1.8.0" resolved "https://registry.npmjs.org/bower/-/bower-1.8.0.tgz#55dbebef0ad9155382d9e9d3e497c1372345b44a" @@ -1484,13 +1490,7 @@ bytes@2.4.0: version "2.4.0" resolved "https://registry.npmjs.org/bytes/-/bytes-2.4.0.tgz#7d97196f9d5baf7f6935e25985549edd2a6c2339" -calculate-cache-key-for-tree@^1.0.0: - version "1.1.0" - resolved "https://registry.npmjs.org/calculate-cache-key-for-tree/-/calculate-cache-key-for-tree-1.1.0.tgz#0c3e42c9c134f3c9de5358c0f16793627ea976d6" - dependencies: - json-stable-stringify "^1.0.1" - -calculate-cache-key-for-tree@^1.1.0: +calculate-cache-key-for-tree@^1.0.0, calculate-cache-key-for-tree@^1.1.0: version "1.1.0" resolved "https://registry.yarnpkg.com/calculate-cache-key-for-tree/-/calculate-cache-key-for-tree-1.1.0.tgz#0c3e42c9c134f3c9de5358c0f16793627ea976d6" dependencies: