From 4270054d73baf53baabe540a1b25d4c69feb13ed Mon Sep 17 00:00:00 2001 From: Chris Thoburn Date: Sat, 21 Apr 2018 19:43:43 -0700 Subject: [PATCH] [CLEANUP BUGFIX] cleanup modelFor and modelFactoryFor codepaths --- addon/-private/system/model/internal-model.js | 4 +- .../system/record-arrays/record-array.js | 2 +- .../relationship-payloads-manager.js | 4 +- addon/-private/system/store.js | 261 +++++++++++------- addon/serializers/json-api.js | 2 +- addon/serializers/rest.js | 4 +- tests/integration/injection-test.js | 2 +- tests/unit/model-test.js | 22 +- tests/unit/store/asserts-test.js | 2 +- .../relationship-payload-manager-test.js | 8 +- 10 files changed, 179 insertions(+), 132 deletions(-) diff --git a/addon/-private/system/model/internal-model.js b/addon/-private/system/model/internal-model.js index 5244df8f305..61fda841a20 100644 --- a/addon/-private/system/model/internal-model.js +++ b/addon/-private/system/model/internal-model.js @@ -172,7 +172,7 @@ export default class InternalModel { } get modelClass() { - return this._modelClass || (this._modelClass = this.store._modelFor(this.modelName)); + return this._modelClass || (this._modelClass = this.store.modelFor(this.modelName)); } get type() { @@ -391,7 +391,7 @@ export default class InternalModel { createOptions.container = this.store.container; } - this._record = this.store.modelFactoryFor(this.modelName).create(createOptions); + this._record = this.store._modelFactoryFor(this.modelName).create(createOptions); this._triggerDeferredTriggers(); heimdall.stop(token); diff --git a/addon/-private/system/record-arrays/record-array.js b/addon/-private/system/record-arrays/record-array.js index 220e17f01d2..71d2ffdef1c 100644 --- a/addon/-private/system/record-arrays/record-array.js +++ b/addon/-private/system/record-arrays/record-array.js @@ -95,7 +95,7 @@ export default ArrayProxy.extend(Evented, { if (!this.modelName) { return null; } - return this.store._modelFor(this.modelName); + return this.store.modelFor(this.modelName); }).readOnly(), /** diff --git a/addon/-private/system/relationships/relationship-payloads-manager.js b/addon/-private/system/relationships/relationship-payloads-manager.js index f64e976fe0a..8bf9ce4d967 100644 --- a/addon/-private/system/relationships/relationship-payloads-manager.js +++ b/addon/-private/system/relationships/relationship-payloads-manager.js @@ -128,7 +128,7 @@ export default class RelationshipPayloadsManager { @method */ unload(modelName, id) { - let modelClass = this._store._modelFor(modelName); + let modelClass = this._store.modelFor(modelName); let relationshipsByName = get(modelClass, 'relationshipsByName'); relationshipsByName.forEach((_, relationshipName) => { let relationshipPayloads = this._getRelationshipPayloads(modelName, relationshipName, false); @@ -189,7 +189,7 @@ export default class RelationshipPayloadsManager { return cached; } - let modelClass = store._modelFor(modelName); + let modelClass = store.modelFor(modelName); let relationshipsByName = get(modelClass, 'relationshipsByName'); // CASE: We don't have a relationship at all diff --git a/addon/-private/system/store.js b/addon/-private/system/store.js index 7f1eff9f1dd..d1dc9f34337 100644 --- a/addon/-private/system/store.js +++ b/addon/-private/system/store.js @@ -15,7 +15,7 @@ import { typeOf, isPresent, isNone } from '@ember/utils'; import Ember from 'ember'; import { InvalidError } from '../adapters/errors'; import { instrument } from 'ember-data/-debug'; -import { assert, warn, inspect } from '@ember/debug'; +import { assert, deprecate, warn, inspect } from '@ember/debug'; import { DEBUG } from '@glimmer/env'; import Model from './model/model'; import normalizeModelName from "./normalize-model-name"; @@ -88,14 +88,11 @@ const { _generateId, _internalModelForId, _load, - _modelForMixin, _pushInternalModel, _setupRelationships, adapterFor, _buildInternalModel, _didUpdateAll, - modelFactoryFor, - modelFor, normalize, peekAll, peekRecord, @@ -105,14 +102,11 @@ const { '_generateId', '_internalModelForId', '_load', - '_modelForMixin', '_pushInternalModel', '_setupRelationships', 'adapterFor', '_buildInternalModel', '_didUpdateAll', - 'modelFactoryFor', - 'modelFor', 'normalize', 'peekAll', 'peekRecord', @@ -1924,49 +1918,23 @@ Store = Service.extend({ }, /* - In case someone defined a relationship to a mixin, for example: - ``` - let Comment = DS.Model.extend({ - owner: belongsTo('commentable'. { polymorphic: true }) - }); - let Commentable = Ember.Mixin.create({ - comments: hasMany('comment') - }); - ``` - we want to look up a Commentable class which has all the necessary - relationship metadata. Thus, we look up the mixin and create a mock - DS.Model, so we can access the relationship CPs of the mixin (`comments`) - in this case - + @deprecated @private - */ - _modelForMixin(normalizedModelName) { - heimdall.increment(_modelForMixin); - // container.registry = 2.1 - // container._registry = 1.11 - 2.0 - // container = < 1.11 - let owner = getOwner(this); - let mixin; - - if (owner.factoryFor) { - let MaybeMixin = owner.factoryFor(`mixin:${normalizedModelName}`); - mixin = MaybeMixin && MaybeMixin.class; - } else { - mixin = owner._lookupFactory(`mixin:${normalizedModelName}`); - } - - if (mixin) { - let ModelForMixin = Model.extend(mixin); - ModelForMixin.reopenClass({ - __isMixin: true, - __mixin: mixin - }); - - //Cache the class as a model - owner.register('model:' + normalizedModelName, ModelForMixin); - } + */ + _modelForMixin(modelName) { + deprecate( + '_modelForMixin is private and deprecated and should never be used directly, use modelFor instead', + false, + { + id: 'ember-data:_modelForMixin', + until: '3.5' + } + ); + assert(`You need to pass a model name to the store's _modelForMixin method`, isPresent(modelName)); + assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); + let normalizedModelName = normalizeModelName(modelName); - return this.modelFactoryFor(normalizedModelName); + return _modelForMixin(this, normalizedModelName); }, /** @@ -1985,68 +1953,71 @@ Store = Service.extend({ assert(`You need to pass a model name to the store's modelFor method`, isPresent(modelName)); assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); - let normalizedModelName = normalizeModelName(modelName); + let maybeFactory = this._modelFactoryFor(modelName); - return this._modelFor(normalizedModelName); + // for factorFor factory/class split + return maybeFactory.class ? maybeFactory.class : maybeFactory; }, /* + @deprecated @private - */ + */ _modelFor(modelName) { - let maybeFactory = this._modelFactoryFor(modelName); - // for factorFor factory/class split - return maybeFactory.class ? maybeFactory.class : maybeFactory; + deprecate( + '_modelFor is private and deprecated, you should use modelFor instead', + false, + { + id: 'ember-data:_modelFor', + until: '3.5' + } + ); + return this.modelFor(modelName); }, _modelFactoryFor(modelName) { - heimdall.increment(modelFor); - let factory = this._modelFactoryCache[modelName]; - - if (!factory) { - factory = this.modelFactoryFor(modelName); - - if (!factory) { - //Support looking up mixins as base types for polymorphic relationships - factory = this._modelForMixin(modelName); - } - if (!factory) { - throw new EmberError(`No model was found for '${modelName}'`); - } - - // interopt with the future - let klass = getOwner(this).factoryFor ? factory.class : factory; - - assert(`'${inspect(klass)}' does not appear to be an ember-data model`, klass.isModel); - - // TODO: deprecate this - let hasOwnModelNameSet = klass.modelName && klass.hasOwnProperty('modelName'); - if (!hasOwnModelNameSet) { - klass.modelName = modelName; - } + assert(`You need to pass a model name to the store's _modelFactoryFor method`, isPresent(modelName)); + assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); + let normalizedModelName = normalizeModelName(modelName); + let factory = getModelFactory(this, this._modelFactoryCache, normalizedModelName); - this._modelFactoryCache[modelName] = factory; + if (factory === null) { + throw new EmberError(`No model was found for '${normalizedModelName}'`); } return factory; }, /* - @private - */ + @deprecated + @private + */ modelFactoryFor(modelName) { - heimdall.increment(modelFactoryFor); - assert(`You need to pass a model name to the store's modelFactoryFor method`, isPresent(modelName)); - assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); + deprecate('modelFactoryFor is private and deprecated', false, { + id: 'ember-data:modelFactoryFor', + until: '3.5' + }); + return this._modelFactoryFor(modelName); + }, + + /* + Returns whether a ModelClass exists for a given modelName + This exists for legacy support for the RESTSerializer, + which due to how it must guess whether a key is a model + must query for whether a match exists. + + We should investigate an RFC to make this public or removing + this requirement. + @private + */ + _hasModelFor(modelName) { + assert(`You need to pass a model name to the store's hasModelFor method`, isPresent(modelName)); + assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string'); let normalizedModelName = normalizeModelName(modelName); - let owner = getOwner(this); + let factory = getModelFactory(this, this._modelFactoryCache, normalizedModelName); - if (owner.factoryFor) { - return owner.factoryFor(`model:${normalizedModelName}`); - } else { - return owner._lookupFactory(`model:${normalizedModelName}`); - } + return factory !== null; }, /** @@ -2262,17 +2233,6 @@ Store = Service.extend({ return internalModelOrModels; }, - _hasModelFor(modelName) { - let owner = getOwner(this); - modelName = normalizeModelName(modelName); - - if (owner.factoryFor) { - return !!owner.factoryFor(`model:${modelName}`); - } else { - return !!owner._lookupFactory(`model:${modelName}`); - } - }, - _pushInternalModel(data) { heimdall.increment(_pushInternalModel); let modelName = data.type; @@ -2284,7 +2244,7 @@ Store = Service.extend({ // contains unknown attributes or relationships, log a warning. if (ENV.DS_WARN_ON_UNKNOWN_KEYS) { - let modelClass = this._modelFor(modelName); + let modelClass = this.modelFor(modelName); // Check unknown attributes let unknownAttributes = Object.keys(data.attributes || {}).filter((key) => { @@ -2443,7 +2403,7 @@ Store = Service.extend({ assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${inspect(modelName)}`, typeof modelName === 'string'); let normalizedModelName = normalizeModelName(modelName); let serializer = this.serializerFor(normalizedModelName); - let model = this._modelFor(normalizedModelName); + let model = this.modelFor(normalizedModelName); return serializer.normalize(model, payload); }, @@ -2746,7 +2706,7 @@ function defaultSerializer(store) { function _commit(adapter, store, operation, snapshot) { let internalModel = snapshot._internalModel; let modelName = snapshot.modelName; - let modelClass = store._modelFor(modelName); + let modelClass = store.modelFor(modelName); assert(`You tried to update a record but you have no adapter (for ${modelName})`, adapter); assert(`You tried to update a record but your adapter (for ${modelName}) does not implement '${operation}'`, typeof adapter[operation] === 'function'); @@ -2835,6 +2795,99 @@ function isInverseRelationshipInitialized(store, internalModel, data, key, model } } +/** + * + * @param store + * @param cache modelFactoryCache + * @param normalizedModelName already normalized modelName + * @returns {*} + */ +function getModelFactory(store, cache, normalizedModelName) { + let factory = cache[normalizedModelName]; + + if (!factory) { + factory = _lookupModelFactory(store, normalizedModelName); + + if (!factory) { + //Support looking up mixins as base types for polymorphic relationships + factory = _modelForMixin(store, normalizedModelName); + } + + if (!factory) { + // we don't cache misses in case someone wants to register a missing model + return null; + } + + // interopt with the future + let klass = getOwner(store).factoryFor ? factory.class : factory; + assert(`'${inspect(klass)}' does not appear to be an ember-data model`, klass.isModel); + + // TODO: deprecate this + let hasOwnModelNameSet = klass.modelName && klass.hasOwnProperty('modelName'); + if (!hasOwnModelNameSet) { + klass.modelName = normalizedModelName; + } + + cache[normalizedModelName] = factory; + } + + return factory; +} + +function _lookupModelFactory(store, normalizedModelName) { + let owner = getOwner(store); + + if (owner.factoryFor) { + return owner.factoryFor(`model:${normalizedModelName}`); + } else { + return owner._lookupFactory(`model:${normalizedModelName}`); + } +} + + +/* + In case someone defined a relationship to a mixin, for example: + ``` + let Comment = DS.Model.extend({ + owner: belongsTo('commentable'. { polymorphic: true }) + }); + let Commentable = Ember.Mixin.create({ + comments: hasMany('comment') + }); + ``` + we want to look up a Commentable class which has all the necessary + relationship metadata. Thus, we look up the mixin and create a mock + DS.Model, so we can access the relationship CPs of the mixin (`comments`) + in this case +*/ +function _modelForMixin(store, normalizedModelName) { + // container.registry = 2.1 + // container._registry = 1.11 - 2.0 + // container = < 1.11 + let owner = getOwner(store); + let mixin; + + if (owner.factoryFor) { + let MaybeMixin = owner.factoryFor(`mixin:${normalizedModelName}`); + mixin = MaybeMixin && MaybeMixin.class; + } else { + mixin = owner._lookupFactory(`mixin:${normalizedModelName}`); + } + + if (mixin) { + let ModelForMixin = Model.extend(mixin); + ModelForMixin.reopenClass({ + __isMixin: true, + __mixin: mixin + }); + + //Cache the class as a model + owner.register('model:' + normalizedModelName, ModelForMixin); + } + + return _lookupModelFactory(store, normalizedModelName); +} + function setupRelationships(store, internalModel, data, modelNameToInverseMap) { Object.keys(data.relationships).forEach(relationshipName => { let relationships = internalModel._relationships; diff --git a/addon/serializers/json-api.js b/addon/serializers/json-api.js index 77c93fb7c1d..24f16827522 100644 --- a/addon/serializers/json-api.js +++ b/addon/serializers/json-api.js @@ -202,7 +202,7 @@ const JSONAPISerializer = JSONSerializer.extend({ return null; } - let modelClass = this.store._modelFor(modelName); + let modelClass = this.store.modelFor(modelName); let serializer = this.store.serializerFor(modelName); let { data } = serializer.normalize(modelClass, resourceHash); return data; diff --git a/addon/serializers/rest.js b/addon/serializers/rest.js index 97e1085808e..84acff2610b 100644 --- a/addon/serializers/rest.js +++ b/addon/serializers/rest.js @@ -267,7 +267,7 @@ const RESTSerializer = JSONSerializer.extend({ } var typeName = this.modelNameFromPayloadKey(modelName); - if (!store.modelFactoryFor(typeName)) { + if (!store._hasModelFor(typeName)) { warn(this.warnMessageNoModelForKey(modelName, typeName), false, { id: 'ds.serializer.model-for-key-missing' }); @@ -395,7 +395,7 @@ const RESTSerializer = JSONSerializer.extend({ for (var prop in payload) { var modelName = this.modelNameFromPayloadKey(prop); - if (!store.modelFactoryFor(modelName)) { + if (!store._hasModelFor(modelName)) { warn(this.warnMessageNoModelForKey(prop, modelName), false, { id: 'ds.serializer.model-for-key-missing' }); diff --git a/tests/integration/injection-test.js b/tests/integration/injection-test.js index 81fda524348..9b7e72315c9 100644 --- a/tests/integration/injection-test.js +++ b/tests/integration/injection-test.js @@ -67,7 +67,7 @@ module('integration/injection factoryFor enabled', { }); test('modelFactoryFor', function(assert) { - const modelFactory = env.store.modelFactoryFor('super-villain'); + const modelFactory = env.store._modelFactoryFor('super-villain'); assert.equal(modelFactory, hasFactoryFor ? factory : model, 'expected the factory itself to be returned'); }); diff --git a/tests/unit/model-test.js b/tests/unit/model-test.js index 172d6f5468c..a3848aad1e1 100644 --- a/tests/unit/model-test.js +++ b/tests/unit/model-test.js @@ -261,18 +261,18 @@ test("a record's id is included in its toString representation", function(assert assert.expect(1); env.adapter.shouldBackgroundReloadRecord = () => false; - return run(() => { - store.push({ - data: { - type: 'person', - id: '1' - } - }); + let person = run(() => store.push({ + data: { + type: 'person', + id: '1' + } + })); - return store.findRecord('person', 1).then(record => { - assert.equal(record.toString(), ``, 'reports id in toString'); - }); - }); + assert.equal( + person.toString(), + ``, + 'reports id in toString' + ); }); testInDebug('trying to set an `id` attribute should raise', function(assert) { diff --git a/tests/unit/store/asserts-test.js b/tests/unit/store/asserts-test.js index a610a8298cc..279bd62dbc2 100644 --- a/tests/unit/store/asserts-test.js +++ b/tests/unit/store/asserts-test.js @@ -16,7 +16,7 @@ const MODEL_NAME_METHODS = [ 'findAll', 'peekAll', 'modelFor', - 'modelFactoryFor', + '_modelFactoryFor', 'normalize', 'adapterFor', 'serializerFor' diff --git a/tests/unit/system/relationships/relationship-payload-manager-test.js b/tests/unit/system/relationships/relationship-payload-manager-test.js index af7159f0291..196302302d1 100644 --- a/tests/unit/system/relationships/relationship-payload-manager-test.js +++ b/tests/unit/system/relationships/relationship-payload-manager-test.js @@ -36,15 +36,9 @@ module('unit/system/relationships/relationship-payloads-manager', { }); test('get throws for invalid models', function(assert) { - this.relationshipPayloadsManager._store._modelFor = (name) => { - if (name === 'fish') { - throw new Error('What is fish?'); - } - }; - assert.throws(() => { this.relationshipPayloadsManager.get('fish', 9, 'hobbies'); - }, /What is fish/); + }, /No model was found for 'fish'/); }); test('get returns null for invalid relationships', function(assert) {