From 020c5e1ef04baba2030ef1c30954e451dfc0b34c Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 18 Apr 2018 00:22:08 -0700 Subject: [PATCH 1/3] [BUGFIX] ensure destroy-sync cleanup is correct --- tests/integration/records/unload-test.js | 87 +++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-) diff --git a/tests/integration/records/unload-test.js b/tests/integration/records/unload-test.js index 1d4b628c809..13bbf8aab1f 100644 --- a/tests/integration/records/unload-test.js +++ b/tests/integration/records/unload-test.js @@ -518,6 +518,92 @@ test('unloadAll(type) does not leave stranded internalModels in relationships (r assert.ok(reloadedBoatInternalModel === initialBoatInternalModel, 'after an unloadAll, subsequent fetch results in the same InternalModel'); }); +test('(regression) unloadRecord followed by push in the same run-loop', function(assert) { + let { store } = env; + + let person = run(() => store.push({ + data: { + type: 'person', + id: '1', + attributes: { + name: 'Could be Anybody' + }, + relationships: { + boats: { + data: [ + { type: 'boat', id: '1' } + ] + } + } + }, + included: [ + makeBoatOneForPersonOne() + ] + })); + + let boat = store.peekRecord('boat', '1'); + let initialBoatInternalModel = boat._internalModel; + let relationshipState = person.hasMany('boats').hasManyRelationship; + let knownPeople = env.store._internalModelsFor('person'); + let knownBoats = store._internalModelsFor('boat'); + + // ensure we loaded the people and boats + assert.equal(knownPeople.models.length, 1, 'one person record is loaded'); + assert.equal(knownBoats.models.length, 1, 'one boat record is loaded'); + assert.equal(env.store.hasRecordForId('person', '1'), true); + assert.equal(env.store.hasRecordForId('boat', '1'), true); + + // ensure the relationship was established (we reach through the async proxy here) + let peopleBoats = run(() => person.get('boats.content')); + let boatPerson = run(() => boat.get('person.content')); + + assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); + assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.ok(get(peopleBoats, 'length') === 1, 'Our person has a boat'); + assert.ok(peopleBoats.objectAt(0) === boat, 'Our person has the right boat'); + assert.ok(boatPerson === person, 'Our boat has the right person'); + + run(() => boat.unloadRecord()); + + // ensure that our new state is correct + assert.equal(knownPeople.models.length, 1, 'one person record is loaded'); + assert.equal(knownBoats.models.length, 1, 'one boat record is known'); + assert.ok(knownBoats.models[0] === initialBoatInternalModel, 'We still have our boat'); + assert.equal(initialBoatInternalModel.isEmpty(), true, 'Model is in the empty state'); + assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should still be 1'); + assert.equal(relationshipState.members.size, 1, 'members size should still be 1'); + assert.ok(get(peopleBoats, 'length') === 0, 'Our person thinks they have no boats'); + + run(() => store.push({ + data: makeBoatOneForPersonOne() + })); + + let reloadedBoat = store.peekRecord('boat', '1'); + let reloadedBoatInternalModel = reloadedBoat._internalModel; + + assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); + assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.ok(reloadedBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent fetch results in the same InternalModel'); + + // and now the kicker, run-loop fun! + // here, we will dematerialize the record, but push it back into the store + // all in the same run-loop! + // effectively this tests that our destroySync is not stupid + run(() => { + reloadedBoat.unloadRecord(); + store.push({ + data: makeBoatOneForPersonOne() + }); + }); + + let yaBoat = store.peekRecord('boat', '1'); + let yaBoatInternalModel = yaBoat._internalModel; + + assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); + assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.ok(yaBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent same-loop push results in the same InternalModel'); +}); + test('unloading a disconnected subgraph clears the relevant internal models', function(assert) { env.adapter.shouldBackgroundReloadRecord = () => false; @@ -632,7 +718,6 @@ test('unloading a disconnected subgraph clears the relevant internal models', fu }); }); - test('Unloading a record twice only schedules destroy once', function(assert) { const store = env.store; let record; From da4a36417b9d6881ea6e3119e617b51d9f698476 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 18 Apr 2018 14:30:52 -0700 Subject: [PATCH 2/3] implement fix --- addon/-private/system/model/internal-model.js | 12 ++++++---- .../system/relationships/state/has-many.js | 2 +- addon/-private/system/store.js | 23 +++++++++++++------ tests/integration/records/unload-test.js | 19 +++++++++------ 4 files changed, 36 insertions(+), 20 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index 936a6385402..5244df8f305 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -141,7 +141,7 @@ export default class InternalModel { this._record = null; this._isDestroyed = false; this.isError = false; - this._isUpdatingRecordArrays = false; // used by the recordArrayManager + this._pendingRecordArrayManagerFlush = false; // used by the recordArrayManager // During dematerialization we don't want to rematerialize the record. The // reason this might happen is that dematerialization removes records from @@ -415,13 +415,15 @@ export default class InternalModel { } dematerializeRecord() { + this._isDematerializing = true; + if (this._record) { - this._isDematerializing = true; this._record.destroy(); - this.destroyRelationships(); - this.resetRecord(); } + // move to an empty never-loaded state + this.destroyRelationships(); + this.resetRecord(); this.updateRecordArrays(); } @@ -607,7 +609,7 @@ export default class InternalModel { destroy() { assert("Cannot destroy an internalModel while its record is materialized", !this._record || this._record.get('isDestroyed') || this._record.get('isDestroying')); - + this.isDestroying = true; this.store._internalModelDestroyed(this); this._relationships.forEach((name, rel) => rel.destroy()); diff --git a/addon/-private/system/relationships/state/has-many.js b/addon/-private/system/relationships/state/has-many.js index 6a73365e369..dd0c1a29680 100755 --- a/addon/-private/system/relationships/state/has-many.js +++ b/addon/-private/system/relationships/state/has-many.js @@ -167,9 +167,9 @@ export default class ManyRelationship extends Relationship { } removeAllCanonicalInternalModelsFromOwn() { - super.removeAllCanonicalInternalModelsFromOwn(); this.canonicalMembers.clear(); this.canonicalState.splice(0, this.canonicalState.length); + super.removeAllCanonicalInternalModelsFromOwn(); } removeCompletelyFromOwn(internalModel) { diff --git a/addon/-private/system/store.js b/addon/-private/system/store.js index f57a811e28c..887b7c69244 100644 --- a/addon/-private/system/store.js +++ b/addon/-private/system/store.js @@ -1103,15 +1103,20 @@ Store = Service.extend({ let internalModel = this._internalModelsFor(modelName).get(trueId); if (internalModel) { + // unloadRecord is async, if one attempts to unload + then sync push, + // we must ensure the unload is canceled before continuing + // The createRecord path will take _existingInternalModelForId() + // which will call `destroySync` instead for this unload + then + // sync createRecord scenario. Once we have true client-side + // delete signaling, we should never call destroySync if (internalModel.hasScheduledDestroy()) { - internalModel.destroySync(); - return this._buildInternalModel(modelName, trueId); - } else { - return internalModel; + internalModel.cancelDestroy(); } - } else { - return this._buildInternalModel(modelName, trueId); + + return internalModel; } + + return this._buildInternalModel(modelName, trueId); }, _internalModelDidReceiveRelationshipData(modelName, id, relationshipData) { @@ -2555,7 +2560,11 @@ Store = Service.extend({ if (internalModel && internalModel.hasScheduledDestroy()) { // unloadRecord is async, if one attempts to unload + then sync create, - // we must ensure the unload is complete before starting the create + // we must ensure the unload is complete before starting the create + // The push path will take _internalModelForId() + // which will call `cancelDestroy` instead for this unload + then + // sync push scenario. Once we have true client-side + // delete signaling, we should never call destroySync internalModel.destroySync(); internalModel = null; } diff --git a/tests/integration/records/unload-test.js b/tests/integration/records/unload-test.js index 13bbf8aab1f..b3531d3cf7f 100644 --- a/tests/integration/records/unload-test.js +++ b/tests/integration/records/unload-test.js @@ -838,7 +838,7 @@ test('after unloading a record, the record can be fetched again immediately', fu assert.equal(record.isDestroying, true, 'the record is destroying'); assert.equal(internalModel.currentState.stateName, 'root.empty', 'We are unloaded after unloadRecord'); return store.findRecord('person', '1').then(newRecord => { - assert.equal(internalModel.currentState.stateName, 'root.empty', 'the old internalModel is discarded'); + assert.ok(internalModel === newRecord._internalModel, 'the old internalModel is reused'); assert.equal(newRecord._internalModel.currentState.stateName, 'root.loaded.saved', 'We are loaded after findRecord'); }); }); @@ -857,7 +857,9 @@ test('after unloading a record, the record can be fetched again immediately (pur name: 'Adam Sunderland' }, relationships: { - cars: { data: null } + cars: { + data: [] + } } } }; @@ -876,7 +878,7 @@ test('after unloading a record, the record can be fetched again immediately (pur cars: { data: [ { - id: 1, + id: '1', type: 'car' } ] @@ -886,7 +888,7 @@ test('after unloading a record, the record can be fetched again immediately (pur included: [ { type: 'car', - id: 1, + id: '1', attributes: { make: 'jeep', model: 'wrangler' @@ -907,8 +909,8 @@ test('after unloading a record, the record can be fetched again immediately (pur assert.equal(internalModel.currentState.stateName, 'root.empty', 'Expected the previous internal model tobe unloaded'); return store.findRecord('person', '1').then(record => { - assert.equal(record.get('cars.length'), 0); - assert.equal(internalModel.currentState.stateName, 'root.empty', 'Expected the previous internal model to STILL be unloaded'); + assert.equal(record.get('cars.length'), 0, 'Expected relationship to be cleared by the new push'); + assert.ok(internalModel === record._internalModel, 'the old internalModel is reused'); assert.equal(record._internalModel.currentState.stateName, 'root.loaded.saved', 'Expected the NEW internal model to be loaded'); }); }); @@ -955,6 +957,7 @@ test('after unloading a record, the record can be fetched again immediately (wit }); const internalModel = record._internalModel; + const bike = store.peekRecord('bike', '1'); assert.equal(internalModel.currentState.stateName, 'root.loaded.saved', 'We are loaded initially'); assert.equal(record.get('bike.name'), 'mr bike'); @@ -965,10 +968,12 @@ test('after unloading a record, the record can be fetched again immediately (wit assert.equal(record.isDestroying, true, 'the record is destroying'); assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed'); assert.equal(internalModel.currentState.stateName, 'root.empty', 'We are unloaded after unloadRecord'); + let wait = store.findRecord('person', '1').then(newRecord => { assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed'); - assert.ok(newRecord.get('bike') === null, 'the newRecord should NOT have had a bike'); + assert.ok(newRecord.get('bike') === bike, 'the newRecord should retain knowledge of the bike'); }); + assert.equal(record.isDestroyed, false, 'the record is NOT YET destroyed'); return wait; }); From 88069026a7698db9f0d5f57469048f2919b7ccdf Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Wed, 18 Apr 2018 19:52:16 -0700 Subject: [PATCH 3/3] assert against IDs to catch more error cases --- tests/integration/records/unload-test.js | 28 ++++++++++++++---------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/tests/integration/records/unload-test.js b/tests/integration/records/unload-test.js index b3531d3cf7f..28589232515 100644 --- a/tests/integration/records/unload-test.js +++ b/tests/integration/records/unload-test.js @@ -7,6 +7,10 @@ import { module, test } from 'qunit'; import DS from 'ember-data'; import setupStore from 'dummy/tests/helpers/store'; +function idsFromOrderedSet(set) { + return set.list.map(i => i.id); +} + const { attr, belongsTo, @@ -548,8 +552,8 @@ test('(regression) unloadRecord followed by push in the same run-loop', function let knownBoats = store._internalModelsFor('boat'); // ensure we loaded the people and boats - assert.equal(knownPeople.models.length, 1, 'one person record is loaded'); - assert.equal(knownBoats.models.length, 1, 'one boat record is loaded'); + assert.deepEqual(knownPeople.models.map(m => m.id), ['1'], 'one person record is loaded'); + assert.deepEqual(knownBoats.models.map(m => m.id), ['1'], 'one boat record is loaded'); assert.equal(env.store.hasRecordForId('person', '1'), true); assert.equal(env.store.hasRecordForId('boat', '1'), true); @@ -557,8 +561,8 @@ test('(regression) unloadRecord followed by push in the same run-loop', function let peopleBoats = run(() => person.get('boats.content')); let boatPerson = run(() => boat.get('person.content')); - assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); - assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1'); assert.ok(get(peopleBoats, 'length') === 1, 'Our person has a boat'); assert.ok(peopleBoats.objectAt(0) === boat, 'Our person has the right boat'); assert.ok(boatPerson === person, 'Our boat has the right person'); @@ -566,12 +570,12 @@ test('(regression) unloadRecord followed by push in the same run-loop', function run(() => boat.unloadRecord()); // ensure that our new state is correct - assert.equal(knownPeople.models.length, 1, 'one person record is loaded'); - assert.equal(knownBoats.models.length, 1, 'one boat record is known'); + assert.deepEqual(knownPeople.models.map(m => m.id), ['1'], 'one person record is loaded'); + assert.deepEqual(knownBoats.models.map(m => m.id), ['1'], 'one boat record is known'); assert.ok(knownBoats.models[0] === initialBoatInternalModel, 'We still have our boat'); assert.equal(initialBoatInternalModel.isEmpty(), true, 'Model is in the empty state'); - assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should still be 1'); - assert.equal(relationshipState.members.size, 1, 'members size should still be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should still be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should still be 1'); assert.ok(get(peopleBoats, 'length') === 0, 'Our person thinks they have no boats'); run(() => store.push({ @@ -581,8 +585,8 @@ test('(regression) unloadRecord followed by push in the same run-loop', function let reloadedBoat = store.peekRecord('boat', '1'); let reloadedBoatInternalModel = reloadedBoat._internalModel; - assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); - assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1'); assert.ok(reloadedBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent fetch results in the same InternalModel'); // and now the kicker, run-loop fun! @@ -599,8 +603,8 @@ test('(regression) unloadRecord followed by push in the same run-loop', function let yaBoat = store.peekRecord('boat', '1'); let yaBoatInternalModel = yaBoat._internalModel; - assert.equal(relationshipState.canonicalMembers.size, 1, 'canonical member size should be 1'); - assert.equal(relationshipState.members.size, 1, 'members size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.canonicalMembers), ['1'], 'canonical member size should be 1'); + assert.deepEqual(idsFromOrderedSet(relationshipState.members), ['1'], 'members size should be 1'); assert.ok(yaBoatInternalModel === initialBoatInternalModel, 'after an unloadRecord, subsequent same-loop push results in the same InternalModel'); });