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 1d4b628c809..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, @@ -518,6 +522,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.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); + + // 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.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'); + + run(() => boat.unloadRecord()); + + // ensure that our new state is correct + 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.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({ + data: makeBoatOneForPersonOne() + })); + + let reloadedBoat = store.peekRecord('boat', '1'); + let reloadedBoatInternalModel = reloadedBoat._internalModel; + + 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! + // 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.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'); +}); + test('unloading a disconnected subgraph clears the relevant internal models', function(assert) { env.adapter.shouldBackgroundReloadRecord = () => false; @@ -632,7 +722,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; @@ -753,7 +842,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'); }); }); @@ -772,7 +861,9 @@ test('after unloading a record, the record can be fetched again immediately (pur name: 'Adam Sunderland' }, relationships: { - cars: { data: null } + cars: { + data: [] + } } } }; @@ -791,7 +882,7 @@ test('after unloading a record, the record can be fetched again immediately (pur cars: { data: [ { - id: 1, + id: '1', type: 'car' } ] @@ -801,7 +892,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' @@ -822,8 +913,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'); }); }); @@ -870,6 +961,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'); @@ -880,10 +972,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; });