From db29f12b977e9e3a8faa94eb16f48f6ab0683956 Mon Sep 17 00:00:00 2001 From: Stefan Penner Date: Fri, 21 Oct 2016 22:51:31 +0300 Subject: [PATCH] [BUGFIX] ensure removals from AdapterPopulatedRecordArrays also remove the corresponding internalModel -> recordArray reference. When working with AdapterPopulatedRecordArrays: * make the very private method more private \w underscore * rename loadRecords to _setInternalModels --- .../adapter-populated-record-array.js | 29 ++++++-- addon/-private/system/store/finders.js | 18 ++--- .../adapter-populated-record-array-test.js | 73 +++++++++++++++++-- .../adapter-populated-record-array-test.js | 63 +++++++++------- 4 files changed, 135 insertions(+), 48 deletions(-) diff --git a/addon/-private/system/record-arrays/adapter-populated-record-array.js b/addon/-private/system/record-arrays/adapter-populated-record-array.js index cd18d9cc1aa..14f4d7f59cd 100644 --- a/addon/-private/system/record-arrays/adapter-populated-record-array.js +++ b/addon/-private/system/record-arrays/adapter-populated-record-array.js @@ -69,13 +69,26 @@ export default RecordArray.extend({ }, /** - @method loadRecords + @method _setInternalModels @param {Array} internalModels @param {Object} payload normalized payload @private */ - loadRecords(internalModels, payload) { - let token = heimdall.start('AdapterPopulatedRecordArray.loadRecords'); + _setInternalModels(internalModels, payload) { + let token = heimdall.start('AdapterPopulatedRecordArray._setInternalModels'); + let operations = {}; + + this.get('content').forEach(internalModel => { + operations[internalModel.id] = { type: 'remove', internalModel }; + }); + + internalModels.forEach(internalModel => { + if (operations[internalModel.id]) { + operations[internalModel.id].type = 'remain'; + } else { + operations[internalModel.id] = { type: 'add', internalModel }; + } + }); // TODO: initial load should not cause change events at all, only // subsequent. This requires changing the public api of adapter.query, but @@ -89,8 +102,14 @@ export default RecordArray.extend({ links: cloneNull(payload.links) }); - internalModels.forEach(record => { - this.manager.recordArraysForRecord(record).add(this); + Object.keys(operations).forEach(id => { + let operation = operations[id]; + let recordArraysSet = this.manager.recordArraysForRecord(operation.internalModel); + + switch (operation.type) { + case 'remove': { recordArraysSet.delete(this); break; } + case 'add': { recordArraysSet.add(this); break; } + } }); // TODO: should triggering didLoad event be the last action of the runLoop? diff --git a/addon/-private/system/store/finders.js b/addon/-private/system/store/finders.js index f863de7eb54..ecca90fd1cf 100644 --- a/addon/-private/system/store/finders.js +++ b/addon/-private/system/store/finders.js @@ -153,27 +153,27 @@ export function _findAll(adapter, store, typeClass, sinceToken, options) { } export function _query(adapter, store, typeClass, query, recordArray) { - var modelName = typeClass.modelName; - var promise = adapter.query(store, typeClass, query, recordArray); + let modelName = typeClass.modelName; + let promise = adapter.query(store, typeClass, query, recordArray); - var serializer = serializerForAdapter(store, adapter, modelName); - var label = "DS: Handle Adapter#query of " + typeClass; + let serializer = serializerForAdapter(store, adapter, modelName); + let label = 'DS: Handle Adapter#query of ' + typeClass; promise = Promise.resolve(promise, label); promise = _guard(promise, _bind(_objectIsAlive, store)); - return promise.then(function(adapterPayload) { - var internalModels, payload; - store._adapterRun(function() { + return promise.then(adapterPayload => { + let internalModels, payload; + store._adapterRun(() => { payload = normalizeResponseHelper(serializer, store, typeClass, adapterPayload, null, 'query'); internalModels = store._push(payload); }); assert('The response to store.query is expected to be an array but it was a single record. Please wrap your response in an array or use `store.queryRecord` to query for a single record.', Array.isArray(internalModels)); - recordArray.loadRecords(internalModels, payload); + recordArray._setInternalModels(internalModels, payload); return recordArray; - }, null, "DS: Extract payload of query " + typeClass); + }, null, 'DS: Extract payload of query ' + typeClass); } export function _queryRecord(adapter, store, typeClass, query) { diff --git a/tests/integration/record-arrays/adapter-populated-record-array-test.js b/tests/integration/record-arrays/adapter-populated-record-array-test.js index 7fc4ec5c406..4c2e13e06ef 100644 --- a/tests/integration/record-arrays/adapter-populated-record-array-test.js +++ b/tests/integration/record-arrays/adapter-populated-record-array-test.js @@ -9,9 +9,13 @@ let store; const { run, RSVP: { Promise } } = Ember; const Person = DS.Model.extend({ - name: DS.attr('string') + name: DS.attr('string'), + toString() { + return ``; + } }); + const adapter = DS.Adapter.extend({ deleteRecord() { return Promise.resolve(); @@ -58,8 +62,7 @@ test('when a record is deleted in an adapter populated record array, it should b }; run(() => { - let records = store.push(payload); - recordArray.loadRecords(records, payload); + recordArray._setInternalModels(store._push(payload), payload); }); assert.equal(recordArray.get('length'), 3, "expected recordArray to contain exactly 3 records"); @@ -103,8 +106,7 @@ test('stores the metadata off the payload', function(assert) { }; run(() => { - let records = store.push(payload); - recordArray.loadRecords(records, payload); + recordArray._setInternalModels(store._push(payload), payload); }); assert.equal(recordArray.get('meta.foo'), 'bar', 'expected meta.foo to be bar from payload'); @@ -144,8 +146,7 @@ test('stores the links off the payload', function(assert) { }; run(() => { - let records = store.push(payload); - recordArray.loadRecords(records, payload); + recordArray._setInternalModels(store._push(payload), payload); }); assert.equal(recordArray.get('links.first'), '/foo?page=1', 'expected links.first to be "/foo?page=1" from payload'); @@ -160,6 +161,64 @@ test('recordArray.replace() throws error', function(assert) { }, Error('The result of a server query (on (subclass of DS.Model)) is immutable.'), 'throws error'); }); +function assertRecordHasRecordArray(assert, record, recordArray) { + if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); } + if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); } + if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); } + + assert.ok(recordArray.manager.recordArraysForRecord(record._internalModel).has(recordArray), `record '${record}' should have reference to its record array`); +} + +function assertRecordDoesNOTRecordArray(assert, record, recordArray) { + if (!assert) { throw TypeError('assertRecordHasRecordArray first argument must be QUnit\'s assert'); } + if (!record) { throw TypeError('assertRecordHasRecordArray second argument must be a record'); } + if (!assert) { throw TypeError('assertRecordHasRecordArray third argument must be a recordArray'); } + + assert.ok(!recordArray.manager.recordArraysForRecord(record._internalModel).has(recordArray), `record '${record}' should NOT have reference to their record array`); +} + +test('loadRecord re-syncs internalModels recordArrays', function(assert) { + let env = setupStore({ person: Person }); + let store = env.store; + + let payload = [ + { id: '1', name: 'Scumbag Dale' }, + { id: '2', name: 'Scumbag Katz' } + ]; + + env.adapter.query = function(store, type, query, recordArray) { + return payload; + }; + + let katz, dale, penner; + return store.query('person', { }).then(recordArray => { + return recordArray.update().then(recordArray => { + assert.deepEqual(recordArray.getEach('name'), ['Scumbag Dale', 'Scumbag Katz'], 'expected query to contain specific records'); + + dale = recordArray.findBy('id', '1'); + katz = recordArray.findBy('id', '2'); + + assertRecordHasRecordArray(assert, dale, recordArray); + assertRecordHasRecordArray(assert, katz, recordArray); + + payload = [ + { id: '1', name: 'Scumbag Dale' }, + { id: '3', name: 'Scumbag Penner' } + ]; + + return recordArray.update(); + }).then(recordArray => { + penner = recordArray.findBy('id', '3'); + + assertRecordHasRecordArray(assert, dale, recordArray); + assertRecordDoesNOTRecordArray(assert, katz, recordArray); + assertRecordHasRecordArray(assert, penner, recordArray); + + assert.deepEqual(recordArray.getEach('name'), ['Scumbag Dale', 'Scumbag Penner']); + }); + }); +}); + test('when an adapter populated record gets updated the array contents are also updated', function(assert) { assert.expect(8); diff --git a/tests/unit/record-arrays/adapter-populated-record-array-test.js b/tests/unit/record-arrays/adapter-populated-record-array-test.js index d5d742377c7..a1fd86bcefc 100644 --- a/tests/unit/record-arrays/adapter-populated-record-array-test.js +++ b/tests/unit/record-arrays/adapter-populated-record-array-test.js @@ -8,18 +8,18 @@ const { AdapterPopulatedRecordArray } = DS; module('unit/record-arrays/adapter-populated-record-array - DS.AdapterPopulatedRecordArray'); -function recordFor(record) { +function internalModelFor(record) { let _internalModel = { + get id() { + return record.id; + }, getRecord() { - record._internalModel = _internalModel return record; } - } - - return { - id: record.id, - _internalModel }; + + record._internalModel = _internalModel + return _internalModel; } test('default initial state', function(assert) { @@ -102,7 +102,7 @@ test('#update uses _update enabling query specific behavior', function(assert) { }); // TODO: is this method required, i suspect store._query should be refactor so this is not needed -test('#loadRecords', function(assert) { +test('#_setInternalModels', function(assert) { let didAddRecord = 0; const manager = { recordArraysForRecord(record) { @@ -120,8 +120,8 @@ test('#loadRecords', function(assert) { manager }); - let model1 = recordFor({ id: 1 }); - let model2 = recordFor({ id: 2 }); + let model1 = internalModelFor({ id: 1 }); + let model2 = internalModelFor({ id: 2 }); assert.equal(didAddRecord, 0, 'no records should have been added yet'); @@ -130,21 +130,21 @@ test('#loadRecords', function(assert) { didLoad++; }); - let links = { foo:1 }; - let meta = { bar:2 }; + let links = { foo: 1 }; + let meta = { bar: 2 }; run(() => { - assert.equal(recordArray.loadRecords([model1._internalModel, model2._internalModel], { + assert.equal(recordArray._setInternalModels([model1, model2], { links, meta - }), undefined, 'loadRecords should have no return value'); + }), undefined, '_setInternalModels should have no return value'); - assert.equal(didAddRecord, 2, 'two records should have been adde'); + assert.equal(didAddRecord, 2, 'two records should have been added'); assert.deepEqual(recordArray.toArray(), [ model1, model2 - ], 'should now contain the loaded records'); + ].map(x => x.getRecord()), 'should now contain the loaded records'); assert.equal(didLoad, 0, 'didLoad event should not have fired'); assert.equal(recordArray.get('links').foo, 1); @@ -154,11 +154,12 @@ test('#loadRecords', function(assert) { }); test('change events when receiving a new query payload', function(assert) { - assert.expect(37); + assert.expect(41); let arrayDidChange = 0; let contentDidChange = 0; let didAddRecord = 0; + let didRemoveRecord = 0; const manager = { recordArraysForRecord(record) { @@ -166,8 +167,12 @@ test('change events when receiving a new query payload', function(assert) { add(array) { didAddRecord++; assert.equal(array, recordArray); + }, + delete(array) { + didRemoveRecord++; + assert.equal(array, recordArray); } - } + }; } }; @@ -177,9 +182,9 @@ test('change events when receiving a new query payload', function(assert) { }); run(() => { - recordArray.loadRecords([ - recordFor({ id: '1', name: 'Scumbag Dale' }), - recordFor({ id: '2', name: 'Scumbag Katz' }) + recordArray._setInternalModels([ + internalModelFor({ id: '1', name: 'Scumbag Dale' }), + internalModelFor({ id: '2', name: 'Scumbag Katz' }) ], {}); }); @@ -212,15 +217,17 @@ test('change events when receiving a new query payload', function(assert) { arrayDidChange = 0; contentDidChange = 0; didAddRecord = 0; + didRemoveRecord = 0; run(() => { // re-query - recordArray.loadRecords([ - recordFor({ id: '3', name: 'Scumbag Penner' }), - recordFor({ id: '4', name: 'Scumbag Hamilton' }) + recordArray._setInternalModels([ + internalModelFor({ id: '3', name: 'Scumbag Penner' }), + internalModelFor({ id: '4', name: 'Scumbag Hamilton' }) ], {}); }); + assert.equal(didRemoveRecord, 2, 'expected 2 didAddRecords'); assert.equal(didAddRecord, 2, 'expected 2 didAddRecords'); assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); assert.equal(recordArray.get('isUpdating'), false, 'should no longer be updating'); @@ -233,6 +240,7 @@ test('change events when receiving a new query payload', function(assert) { arrayDidChange = 0; // reset change event counter contentDidChange = 0; // reset change event counter didAddRecord = 0; + didRemoveRecord= 0; recordArray.one('@array:change', function(array, startIdx, removeAmt, addAmt) { arrayDidChange++; @@ -252,12 +260,13 @@ test('change events when receiving a new query payload', function(assert) { assert.equal(contentDidChange, 0, 'recordArray.content should not have changed'); run(() => { - recordArray.loadRecords([ - recordFor({ id: '3', name: 'Scumbag Penner' }) + recordArray._setInternalModels([ + internalModelFor({ id: '3', name: 'Scumbag Penner' }) ], {}); }); - assert.equal(didAddRecord, 1, 'expected 1 didAddRecord'); + assert.equal(didAddRecord, 0, 'expected 0 didAddRecord'); + assert.equal(didRemoveRecord, 1, 'expected 1 didRemoveRecord'); assert.equal(recordArray.get('isLoaded'), true, 'should be considered loaded'); assert.equal(recordArray.get('isUpdating'), false, 'should not longer be updating');