From 106f965b897bcd289e5fe95e149536402b372396 Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Fri, 21 Oct 2016 18:00:39 -0700 Subject: [PATCH] chore(store): cleans up and optimizes code paths around fetching relationships and single records --- addon/-private/system/model/internal-model.js | 4 +- .../system/relationships/state/has-many.js | 4 +- addon/-private/system/snapshot.js | 1 - addon/-private/system/store.js | 212 ++++++++++-------- tests/integration/snapshot-test.js | 12 +- tests/unit/store/adapter-interop-test.js | 47 ++-- 6 files changed, 146 insertions(+), 134 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index aa15574a875..22a4bff9581 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -707,11 +707,11 @@ export default class InternalModel { Preloaded data can be attributes and relationships passed in either as IDs or as actual models. - @method _preloadData + @method preloadData @private @param {Object} preload */ - _preloadData(preload) { + preloadData(preload) { //TODO(Igor) consider the polymorphic case Object.keys(preload).forEach((key) => { var preloadValue = get(preload, key); diff --git a/addon/-private/system/relationships/state/has-many.js b/addon/-private/system/relationships/state/has-many.js index 9aa3286abf0..3314a350a43 100644 --- a/addon/-private/system/relationships/state/has-many.js +++ b/addon/-private/system/relationships/state/has-many.js @@ -130,9 +130,7 @@ ManyRelationship.prototype.reload = function() { this._loadingPromise = promiseManyArray(this.fetchLink(), 'Reload with link'); return this._loadingPromise; } else { - this._loadingPromise = promiseManyArray(this.store.scheduleFetchMany(manyArray.toArray()).then(() => { - return manyArray; - }), 'Reload with ids'); + this._loadingPromise = promiseManyArray(this.store._scheduleFetchMany(manyArray.currentState).then(() => manyArray), 'Reload with ids'); return this._loadingPromise; } }; diff --git a/addon/-private/system/snapshot.js b/addon/-private/system/snapshot.js index 4800ae80458..0b723f83cbb 100644 --- a/addon/-private/system/snapshot.js +++ b/addon/-private/system/snapshot.js @@ -36,7 +36,6 @@ export default function Snapshot(internalModel, options = {}) { @type {Object} */ this.adapterOptions = options.adapterOptions; - this.include = options.include; this._changedAttributes = record.changedAttributes(); diff --git a/addon/-private/system/store.js b/addon/-private/system/store.js index e30e25bc857..4db4a7c8754 100644 --- a/addon/-private/system/store.js +++ b/addon/-private/system/store.js @@ -49,7 +49,7 @@ const { get, isNone, isPresent, - Map, + MapWithDefault, run: emberRun, set, Service @@ -211,7 +211,7 @@ Store = Service.extend({ this._instanceCache = new ContainerInstanceCache(getOwner(this), this); //Used to keep track of all the find requests that need to be coalesced - this._pendingFetch = Map.create(); + this._pendingFetch = MapWithDefault.create({ defaultValue: [] }); }, /** @@ -660,7 +660,7 @@ Store = Service.extend({ _findRecord(internalModel, options) { // Refetch if the reload option is passed if (options.reload) { - return this.scheduleFetch(internalModel, options); + return this._scheduleFetch(internalModel, options); } var snapshot = internalModel.createSnapshot(options); @@ -669,7 +669,7 @@ Store = Service.extend({ // Refetch the record if the adapter thinks the record is stale if (adapter.shouldReloadRecord(this, snapshot)) { - return this.scheduleFetch(internalModel, options); + return this._scheduleFetch(internalModel, options); } if (options.backgroundReload === false) { @@ -678,7 +678,7 @@ Store = Service.extend({ // Trigger the background refetch if backgroundReload option is passed if (options.backgroundReload || adapter.shouldBackgroundReloadRecord(this, snapshot)) { - this.scheduleFetch(internalModel, options); + this._scheduleFetch(internalModel, options); } // Return the cached record @@ -689,7 +689,7 @@ Store = Service.extend({ options = options || {}; if (options.preload) { - internalModel._preloadData(options.preload); + internalModel.preloadData(options.preload); } var fetchedInternalModel = this._findEmptyInternalModel(internalModel, options); @@ -699,7 +699,7 @@ Store = Service.extend({ _findEmptyInternalModel(internalModel, options) { if (internalModel.isEmpty()) { - return this.scheduleFetch(internalModel, options); + return this._scheduleFetch(internalModel, options); } //TODO double check about reloading @@ -737,57 +737,51 @@ Store = Service.extend({ type/id pair hasn't been loaded yet to kick off a request to the adapter. - @method fetchRecord + @method _fetchRecord @private @param {InternalModel} internalModel model @return {Promise} promise */ - // TODO rename this to have an underscore - fetchRecord(internalModel, options) { - var typeClass = internalModel.type; - var id = internalModel.id; - var adapter = this.adapterFor(typeClass.modelName); + _fetchRecord(internalModel, options) { + let modelClass = internalModel.type; + let id = internalModel.id; + let adapter = this.adapterFor(modelClass.modelName); - assert("You tried to find a record but you have no adapter (for " + typeClass + ")", adapter); - assert("You tried to find a record but your adapter (for " + typeClass + ") does not implement 'findRecord'", typeof adapter.findRecord === 'function' || typeof adapter.find === 'function'); + assert("You tried to find a record but you have no adapter (for " + modelClass.modelName + ")", adapter); + assert("You tried to find a record but your adapter (for " + modelClass.modelName + ") does not implement 'findRecord'", typeof adapter.findRecord === 'function' || typeof adapter.find === 'function'); - var promise = _find(adapter, this, typeClass, id, internalModel, options); - return promise; + return _find(adapter, this, modelClass, id, internalModel, options); }, - scheduleFetchMany(records) { - let internalModels = new Array(records.length); - let fetches = new Array(records.length); - for (let i = 0; i < records.length; i++) { - internalModels[i] = records[i]._internalModel; - } + _scheduleFetchMany(internalModels) { + let fetches = new Array(internalModels.length); for (let i = 0; i < internalModels.length; i++) { - fetches[i] = this.scheduleFetch(internalModels[i]); + fetches[i] = this._scheduleFetch(internalModels[i]); } - return Ember.RSVP.Promise.all(fetches); + return Promise.all(fetches); }, - scheduleFetch(internalModel, options) { - var typeClass = internalModel.type; - + _scheduleFetch(internalModel, options) { if (internalModel._loadingPromise) { return internalModel._loadingPromise; } - var resolver = Ember.RSVP.defer('Fetching ' + typeClass + 'with id: ' + internalModel.id); - var pendingFetchItem = { - record: internalModel, - resolver: resolver, - options: options + let modelClass = internalModel.type; + let resolver = Ember.RSVP.defer('Fetching ' + modelClass.modelName + 'with id: ' + internalModel.id); + let pendingFetchItem = { + internalModel, + resolver, + options }; - var promise = resolver.promise; + let promise = resolver.promise; internalModel.loadingData(promise); + let cache = this._pendingFetch.getWithDefault(); - if (!this._pendingFetch.get(typeClass)) { - this._pendingFetch.set(typeClass, [pendingFetchItem]); + if (!this._pendingFetch.get(modelClass)) { + this._pendingFetch.set(modelClass, [pendingFetchItem]); } else { - this._pendingFetch.get(typeClass).push(pendingFetchItem); + this._pendingFetch.get(modelClass).push(pendingFetchItem); } emberRun.scheduleOnce('afterRender', this, this.flushAllPendingFetches); @@ -800,63 +794,67 @@ Store = Service.extend({ } this._pendingFetch.forEach(this._flushPendingFetchForType, this); - this._pendingFetch = Map.create(); + this._pendingFetch.clear(); }, - _flushPendingFetchForType(pendingFetchItems, typeClass) { - var store = this; - var adapter = store.adapterFor(typeClass.modelName); - var shouldCoalesce = !!adapter.findMany && adapter.coalesceFindRequests; - var records = Ember.A(pendingFetchItems).mapBy('record'); + _flushPendingFetchForType(pendingFetchItems, modelClass) { + let store = this; + let adapter = store.adapterFor(modelClass.modelName); + let shouldCoalesce = !!adapter.findMany && adapter.coalesceFindRequests; + let totalItems = pendingFetchItems.length; + let internalModels = new Array(totalItems); + let seeking = new EmptyObject(); + + for (let i = 0; i < totalItems; i++) { + internalModels[i] = pendingFetchItems[i].internalModel; + seeking[internalModels[i].id] = pendingFetchItems[i]; + } function _fetchRecord(recordResolverPair) { - recordResolverPair.resolver.resolve(store.fetchRecord(recordResolverPair.record, recordResolverPair.options)); // TODO adapter options + recordResolverPair.resolver.resolve(store._fetchRecord(recordResolverPair.internalModel, recordResolverPair.options)); // TODO adapter options } - function resolveFoundRecords(records) { - records.forEach((record) => { - var pair = Ember.A(pendingFetchItems).findBy('record', record); + function handleFoundRecords(foundInternalModels, expectedInternalModels) { + // resolve found records + let found = new EmptyObject(); + for (let i = 0, l = foundInternalModels.length; i < l; i++) { + let internalModel = foundInternalModels[i]; + let pair = seeking[internalModel.id]; + found[internalModel.id] = internalModel; + if (pair) { - var resolver = pair.resolver; - resolver.resolve(record); + let resolver = pair.resolver; + resolver.resolve(internalModel); } - }); - return records; - } + } - function makeMissingRecordsRejector(requestedRecords) { - return function rejectMissingRecords(resolvedRecords) { - resolvedRecords = Ember.A(resolvedRecords); - var missingRecords = requestedRecords.reject((record) => resolvedRecords.includes(record)); - if (missingRecords.length) { - warn('Ember Data expected to find records with the following ids in the adapter response but they were missing: ' + Ember.inspect(Ember.A(missingRecords).mapBy('id')), false, { - id: 'ds.store.missing-records-from-adapter' - }); + // reject missing records + let missingRecords = []; + for (let i = 0, l = expectedInternalModels.length; i < l; i++) { + if (!found[expectedInternalModels[i].id]) { + missingRecords.push(expectedInternalModels[i]); } + } + + if (missingRecords.length) { + warn('Ember Data expected to find records with the following ids in the adapter response but they were missing: ' + Ember.inspect(missingRecords.map(r => r.id)), false, { + id: 'ds.store.missing-records-from-adapter' + }); rejectRecords(missingRecords); - }; + } } - function makeRecordsRejector(records) { - return function (error) { - rejectRecords(records, error); - }; - } + function rejectRecords(internalModels, error) { + for (let i = 0, l = internalModels.length; i < l; i++) { + let pair = seeking[internalModels[i].id]; - function rejectRecords(records, error) { - records.forEach((record) => { - var pair = Ember.A(pendingFetchItems).findBy('record', record); if (pair) { - var resolver = pair.resolver; - resolver.reject(error); + pair.resolver.reject(error); } - }); + } } - if (pendingFetchItems.length === 1) { - _fetchRecord(pendingFetchItems[0]); - } else if (shouldCoalesce) { - + if (shouldCoalesce) { // TODO: Improve records => snapshots => records => snapshots // // We want to provide records to all store methods and snapshots to all @@ -867,27 +865,43 @@ Store = Service.extend({ // But since the _findMany() finder is a store method we need to get the // records from the grouped snapshots even though the _findMany() finder // will once again convert the records to snapshots for adapter.findMany() + let snapshots = new Array(totalItems); + for (let i = 0; i < totalItems; i++) { + snapshots[i] = internalModels[i].createSnapshot(); + } + + let groups = adapter.groupRecordsForFindMany(this, snapshots); + + for (let i = 0, l = groups.length; i < l; i++) { + let group = groups[i]; + let totalInGroup = groups[i].length; + let ids = new Array(totalInGroup); + let groupedInternalModels = new Array(totalInGroup); - var snapshots = Ember.A(records).invoke('createSnapshot'); - var groups = adapter.groupRecordsForFindMany(this, snapshots); - groups.forEach((groupOfSnapshots) => { - var groupOfRecords = Ember.A(groupOfSnapshots).mapBy('_internalModel'); - var requestedRecords = Ember.A(groupOfRecords); - var ids = requestedRecords.mapBy('id'); - if (ids.length > 1) { - _findMany(adapter, store, typeClass, ids, requestedRecords). - then(resolveFoundRecords). - then(makeMissingRecordsRejector(requestedRecords)). - then(null, makeRecordsRejector(requestedRecords)); + for (let j = 0; j < totalInGroup; j++) { + groupedInternalModels[j] = group[j]._internalModel; + ids[j] = groupedInternalModels[j].id; + } + + if (totalInGroup > 1) { + _findMany(adapter, store, modelClass, ids, groupedInternalModels) + .then(function(foundInternalModels) { + handleFoundRecords(foundInternalModels, groupedInternalModels); + }) + .catch(function(error) { + rejectRecords(groupedInternalModels, error); + }); } else if (ids.length === 1) { - var pair = Ember.A(pendingFetchItems).findBy('record', groupOfRecords[0]); + let pair = seeking[groupedInternalModels[0].id]; _fetchRecord(pair); } else { assert("You cannot return an empty array from adapter's method groupRecordsForFindMany", false); } - }); + } } else { - pendingFetchItems.forEach(_fetchRecord); + for (let i = 0; i < totalItems; i++) { + _fetchRecord(pendingFetchItems[i]); + } } }, @@ -985,7 +999,7 @@ Store = Service.extend({ assert("You tried to reload a record but you have no adapter (for " + modelName + ")", adapter); assert("You tried to reload a record but your adapter does not implement `findRecord`", typeof adapter.findRecord === 'function' || typeof adapter.find === 'function'); - return this.scheduleFetch(internalModel); + return this._scheduleFetch(internalModel); }, /** @@ -1021,18 +1035,18 @@ Store = Service.extend({ return this._internalModelForId(modelName, id).getRecord(); }, - _internalModelForId(typeName, inputId) { + _internalModelForId(modelName, inputId) { heimdall.increment(_internalModelForId); - var typeClass = this.modelFor(typeName); - var id = coerceId(inputId); - var idToRecord = this.typeMapFor(typeClass).idToRecord; - var record = idToRecord[id]; + let modelClass = this.modelFor(modelName); + let id = coerceId(inputId); + let idToRecord = this.typeMapFor(modelClass).idToRecord; + let internalModel = idToRecord[id]; - if (!record || !idToRecord[id]) { - record = this.buildInternalModel(typeClass, id); + if (!internalModel || !idToRecord[id]) { + internalModel = this.buildInternalModel(modelClass, id); } - return record; + return internalModel; }, diff --git a/tests/integration/snapshot-test.js b/tests/integration/snapshot-test.js index b63a5941892..00fc28db881 100644 --- a/tests/integration/snapshot-test.js +++ b/tests/integration/snapshot-test.js @@ -945,11 +945,11 @@ test("snapshot.eachRelationship() proxies to record", function(assert) { }); }); -test("snapshot.belongsTo() does not trigger a call to store.scheduleFetch", function(assert) { +test("snapshot.belongsTo() does not trigger a call to store._scheduleFetch", function(assert) { assert.expect(0); - env.store.scheduleFetch = function() { - assert.ok(false, 'store.scheduleFetch should not be called'); + env.store._scheduleFetch = function() { + assert.ok(false, 'store._scheduleFetch should not be called'); }; run(function() { @@ -974,11 +974,11 @@ test("snapshot.belongsTo() does not trigger a call to store.scheduleFetch", func }); }); -test("snapshot.hasMany() does not trigger a call to store.scheduleFetch", function(assert) { +test("snapshot.hasMany() does not trigger a call to store._scheduleFetch", function(assert) { assert.expect(0); - env.store.scheduleFetch = function() { - assert.ok(false, 'store.scheduleFetch should not be called'); + env.store._scheduleFetch = function() { + assert.ok(false, 'store._scheduleFetch should not be called'); }; run(function() { diff --git a/tests/unit/store/adapter-interop-test.js b/tests/unit/store/adapter-interop-test.js index 100d7f069f4..f2e04bbe782 100644 --- a/tests/unit/store/adapter-interop-test.js +++ b/tests/unit/store/adapter-interop-test.js @@ -614,21 +614,21 @@ test("store.fetchMany should always return a promise", function(assert) { run(function() { store.createRecord('person'); }); - var records = Ember.A([]); + var records = []; var results; run(function() { - results = store.scheduleFetchMany(records); + results = store._scheduleFetchMany(records); }); - assert.ok(results, "A call to store.scheduleFetchMany() should return a result"); - assert.ok(results.then, "A call to store.scheduleFetchMany() should return a promise"); + assert.ok(results, "A call to store._scheduleFetchMany() should return a result"); + assert.ok(results.then, "A call to store._scheduleFetchMany() should return a promise"); results.then(assert.wait(function(returnedRecords) { assert.deepEqual(returnedRecords, [], "The correct records are returned"); })); }); -test("store.scheduleFetchMany should not resolve until all the records are resolved", function(assert) { +test("store._scheduleFetchMany should not resolve until all the records are resolved", function(assert) { assert.expect(1); var Person = DS.Model.extend(); @@ -672,15 +672,16 @@ test("store.scheduleFetchMany should not resolve until all the records are resol store.createRecord('test'); }); - var records = Ember.A([ - store.recordForId('test', 10), - store.recordForId('phone', 20), - store.recordForId('phone', 21) - ]); + let internalModels = [ + store._internalModelForId('test', 10), + store._internalModelForId('phone', 20), + store._internalModelForId('phone', 21) + ]; run(function() { - store.scheduleFetchMany(records).then(assert.wait(function() { - var unloadedRecords = records.filterBy('isEmpty'); + store._scheduleFetchMany(internalModels).then(assert.wait(function() { + var unloadedRecords = Ember.A(internalModels.map(r => r.record)).filterBy('isEmpty'); + assert.equal(get(unloadedRecords, 'length'), 0, 'All unloaded records should be loaded'); })); }); @@ -722,21 +723,21 @@ test("the store calls adapter.findMany according to groupings returned by adapte test: Person }); - var records = Ember.A([ - store.recordForId('test', 10), - store.recordForId('test', 20), - store.recordForId('test', 21) - ]); + var internalModels = [ + store._internalModelForId('test', 10), + store._internalModelForId('test', 20), + store._internalModelForId('test', 21) + ]; run(function() { - store.scheduleFetchMany(records).then(assert.wait(function() { - var ids = records.mapBy('id'); + store._scheduleFetchMany(internalModels).then(assert.wait(function() { + var ids = Ember.A(internalModels).mapBy('id'); assert.deepEqual(ids, ["10", "20", "21"], "The promise fulfills with the records"); })); }); }); -test("the promise returned by `scheduleFetch`, when it resolves, does not depend on the promises returned to other calls to `scheduleFetch` that are in the same run loop, but different groups", function(assert) { +test("the promise returned by `_scheduleFetch`, when it resolves, does not depend on the promises returned to other calls to `_scheduleFetch` that are in the same run loop, but different groups", function(assert) { assert.expect(2); var Person = DS.Model.extend(); @@ -785,7 +786,7 @@ test("the promise returned by `scheduleFetch`, when it resolves, does not depend }); }); -test("the promise returned by `scheduleFetch`, when it rejects, does not depend on the promises returned to other calls to `scheduleFetch` that are in the same run loop, but different groups", function(assert) { +test("the promise returned by `_scheduleFetch`, when it rejects, does not depend on the promises returned to other calls to `_scheduleFetch` that are in the same run loop, but different groups", function(assert) { assert.expect(2); var Person = DS.Model.extend(); @@ -834,7 +835,7 @@ test("the promise returned by `scheduleFetch`, when it rejects, does not depend }); }); -test("store.fetchRecord reject records that were not found, even when those requests were coalesced with records that were found", function(assert) { +test("store._fetchRecord reject records that were not found, even when those requests were coalesced with records that were found", function(assert) { assert.expect(2); var Person = DS.Model.extend(); @@ -872,7 +873,7 @@ test("store.fetchRecord reject records that were not found, even when those requ }); }); -testInDebug("store.fetchRecord warns when records are missing", function(assert) { +testInDebug("store._fetchRecord warns when records are missing", function(assert) { var Person = DS.Model.extend(); var adapter = TestAdapter.extend({