Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use collectionName for ModelStore key when modelName isn't set #253

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions shared/base/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,12 +69,12 @@ module.exports = BaseView = Backbone.View.extend({
parse: true
});
}
options.model_name = options.model_name || this.app.modelUtils.modelName(options.model.constructor);
options.model_name = options.model_name || this.app.modelUtils.resourceName(options.model.constructor);
options.model_id = options.model.id;
}

if (options.collection != null) {
options.collection_name = options.collection_name || this.app.modelUtils.modelName(options.collection.constructor);
options.collection_name = options.collection_name || this.app.modelUtils.resourceName(options.collection.constructor);
options.collection_params = options.collection.params;
}

Expand Down
16 changes: 10 additions & 6 deletions shared/fetcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ Fetcher.prototype.fetchFromApi = function(spec, callback) {
body = resp.body;
resp.body = typeof body === 'string' ? body.slice(0, 150) : body;
respOutput = JSON.stringify(resp);
err = new Error("ERROR fetching model '" + fetcher.modelUtils.modelName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput);
err = new Error("ERROR fetching model '" + fetcher.modelUtils.resourceName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput);
err.status = resp.status;
err.body = body;
callback(err);
Expand All @@ -237,12 +237,16 @@ Fetcher.prototype.fetchFromApi = function(spec, callback) {

Fetcher.prototype.retrieveModelsForCollectionName = function(collectionName, modelIds) {
var modelName = this.modelUtils.getModelNameForCollectionName(collectionName);
return this.retrieveModels(modelName, modelIds);
if (modelName) {
return this.retrieveModels(modelName, modelIds);
} else {
return this.retrieveModels(collectionName, modelIds);
}
};

Fetcher.prototype.retrieveModels = function(modelName, modelIds) {
Fetcher.prototype.retrieveModels = function(modelOrCollectionName, modelIds) {
return modelIds.map(function(id) {
return this.modelStore.get(modelName, id);
return this.modelStore.get(modelOrCollectionName, id);
}, this);
};

Expand All @@ -253,15 +257,15 @@ Fetcher.prototype.summarize = function(modelOrCollection) {
if (this.modelUtils.isCollection(modelOrCollection)) {
idAttribute = modelOrCollection.model.prototype.idAttribute;
summary = {
collection: this.modelUtils.modelName(modelOrCollection.constructor),
collection: this.modelUtils.resourceName(modelOrCollection.constructor),
ids: modelOrCollection.pluck(idAttribute),
params: modelOrCollection.params,
meta: modelOrCollection.meta
};
} else if (this.modelUtils.isModel(modelOrCollection)) {
idAttribute = modelOrCollection.idAttribute;
summary = {
model: this.modelUtils.modelName(modelOrCollection.constructor),
model: this.modelUtils.resourceName(modelOrCollection.constructor),
id: modelOrCollection.get(idAttribute)
};
}
Expand Down
16 changes: 12 additions & 4 deletions shared/modelUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ function ModelUtils(entryPath) {
this._classMap = {};
}

ModelUtils.prototype.getModel = function(path, attrs, options, callback) {
ModelUtils.prototype.getModel = function(path, attrs, options, callback, fallbackToBaseModel) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Under what circumstances would fallbackToBaseModel be true vs false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fallbackToBaseModel is true in ModelStore#get when returnModelInstance is true. In all other cases, it defaults to false.

This option makes getModel return an instance of baseModel when the path does not exist. We need this behavior for instances where a collection uses BaseModel. In that case, ModelStore#get passes a collectionName as the path instead of a modelName.

We could make this behavior the default (and remove the fallbackToBaseModel parameter) without causing any tests to fail, but I elected to make this behavior optional because I can imagine scenarios in which you would want an exception when passing a non-existent path to getModel.

Another approach would be to leave getModel unchanged and handle the exception in ModelStore#get, but that seems like it would lead to needless code repetition.

var Model;
attrs = attrs || {};
options = options || {};
Expand All @@ -27,7 +27,15 @@ ModelUtils.prototype.getModel = function(path, attrs, options, callback) {
callback(new Model(attrs, options));
});
} else {
Model = this.getModelConstructor(path);
try {
Model = this.getModelConstructor(path)
} catch (e) {
if ((e.code === 'MODULE_NOT_FOUND' || e.match(/module '.*' not found/)) && fallbackToBaseModel) {
Model = BaseModel;
} else {
throw e;
}
}
return new Model(attrs, options);
}
};
Expand Down Expand Up @@ -93,7 +101,7 @@ ModelUtils.prototype.isCollection = function(obj) {
ModelUtils.prototype.getModelNameForCollectionName = function(collectionName) {
var Collection;
Collection = this.getCollectionConstructor(collectionName);
return this.modelName(Collection.prototype.model);
return this.resourceName(Collection.prototype.model);
};

ModelUtils.uppercaseRe = /([A-Z])/g;
Expand Down Expand Up @@ -127,7 +135,7 @@ ModelUtils.prototype.underscorize = function(name) {
* -> ""
* MyClass.id = "MyClass"
*/
ModelUtils.prototype.modelName = function(modelOrCollectionClass) {
ModelUtils.prototype.resourceName = function(modelOrCollectionClass) {
return this.underscorize(modelOrCollectionClass.id || modelOrCollectionClass.name);
};

Expand Down
2 changes: 1 addition & 1 deletion shared/store/collection_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ CollectionStore.prototype.constructor = CollectionStore;
CollectionStore.prototype.set = function(collection, params) {
var data, idAttribute, key;
params = params || collection.params;
key = this._getStoreKey(this.modelUtils.modelName(collection.constructor), params);
key = this._getStoreKey(this.modelUtils.resourceName(collection.constructor), params);
idAttribute = collection.model.prototype.idAttribute;
data = {
ids: collection.pluck(idAttribute),
Expand Down
38 changes: 22 additions & 16 deletions shared/store/model_store.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,46 +14,52 @@ ModelStore.prototype = Object.create(Super.prototype);
ModelStore.prototype.constructor = ModelStore;

ModelStore.prototype.set = function(model) {
var existingAttrs, id, key, modelName, newAttrs;
var existingAttrs, id, key, keyPrefix, newAttrs, constructor;

id = model.get(model.idAttribute);
modelName = this.modelUtils.modelName(model.constructor);
if (modelName == null) {
throw new Error('Undefined modelName for model');
keyPrefix = this.modelUtils.resourceName(model.constructor);
if (!keyPrefix && model.collection) {
keyPrefix = this.modelUtils.resourceName(model.collection.constructor);
}
key = this._getModelStoreKey(modelName, id);
/**
* If the model is not named and not part of a named collection,
* fall back to an empty string to preserve existing behavior.
*/
keyPrefix = keyPrefix || '';
key = this._getModelStoreKey(keyPrefix, id);

/**
* We want to merge the model attrs with whatever is already
* present in the store.
*/
existingAttrs = this.get(modelName, id) || {};
existingAttrs = this.get(keyPrefix, id) || {};
newAttrs = _.extend({}, existingAttrs, model.toJSON());
return Super.prototype.set.call(this, key, newAttrs, null);
};

ModelStore.prototype.get = function(modelName, id, returnModelInstance) {
ModelStore.prototype.get = function(resourceName, id, returnModelInstance) {
var key, modelData;

if (returnModelInstance == null) {
returnModelInstance = false;
}
key = this._getModelStoreKey(modelName, id);
key = this._getModelStoreKey(resourceName, id);

modelData = Super.prototype.get.call(this, key);
if (modelData) {
if (returnModelInstance) {
return this.modelUtils.getModel(modelName, modelData, {
return this.modelUtils.getModel(resourceName, modelData, {
app: this.app
});
}, null, true);
} else {
return modelData;
}
}
};

ModelStore.prototype.find = function(modelName, params) {
ModelStore.prototype.find = function(resourceName, params) {
var prefix, foundCachedObject, _this, data, foundCachedObjectKey;
prefix = this._formatKey(this._keyPrefix(modelName));
prefix = this._formatKey(this._keyPrefix(resourceName));
_this = this;
// find the cached object that has attributes which are a subset of the params
foundCachedObject = _.find(this.cache, function(cacheObject, key) {
Expand Down Expand Up @@ -88,10 +94,10 @@ function isObjectSubset(potentialSubset, objectToTest) {
});
}

ModelStore.prototype._keyPrefix = function(modelName) {
return this.modelUtils.underscorize(modelName);
ModelStore.prototype._keyPrefix = function(resourceName) {
return this.modelUtils.underscorize(resourceName);
}

ModelStore.prototype._getModelStoreKey = function(modelName, id) {
return this._keyPrefix(modelName) + ":" + id;
ModelStore.prototype._getModelStoreKey = function(resourceName, id) {
return this._keyPrefix(resourceName) + ":" + id;
}
6 changes: 3 additions & 3 deletions test/shared/modelUtils.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ describe('modelUtils', function () {
});
});

describe('modelName', function () {
describe('resourceName', function () {
var ModelConstructor;

beforeEach(function () {
Expand All @@ -289,11 +289,11 @@ describe('modelUtils', function () {

it('should return the underscorized model id if it is set', function () {
ModelConstructor.id = 'modelId';
modelUtils.modelName(ModelConstructor).should.equal('model_id');
modelUtils.resourceName(ModelConstructor).should.equal('model_id');
});

it('should return the underscorized constructor name if the id is not set', function () {
modelUtils.modelName(ModelConstructor).should.equal('model_name');
modelUtils.resourceName(ModelConstructor).should.equal('model_name');
});
});

Expand Down
51 changes: 47 additions & 4 deletions test/shared/store/model_store.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,22 @@ var util = require('util'),
ModelUtils = require('../../../shared/modelUtils'),
modelUtils = new ModelUtils(),
AddClassMapping = require('../../helpers/add_class_mapping'),
addClassMapping = new AddClassMapping(modelUtils);
addClassMapping = new AddClassMapping(modelUtils),
BaseCollection = require('../../../shared/base/collection');

function MyModel() {
MyModel.super_.apply(this, arguments);
}
util.inherits(MyModel, BaseModel);

function MyCollection() {
MyCollection.super_.apply(this, arguments);
}
util.inherits(MyCollection, BaseCollection);

function App() {}

addClassMapping.add(modelUtils.modelName(MyModel), MyModel);
addClassMapping.add(modelUtils.resourceName(MyModel), MyModel);

describe('ModelStore', function() {
beforeEach(function() {
Expand Down Expand Up @@ -56,7 +62,7 @@ describe('ModelStore', function() {

model = new MyCustomModel(modelAttrs);
this.store.set(model);
result = this.store.get(modelUtils.modelName(MyCustomModel), modelAttrs.login);
result = this.store.get(modelUtils.resourceName(MyCustomModel), modelAttrs.login);
result.should.eql(modelAttrs);
});

Expand Down Expand Up @@ -100,7 +106,7 @@ describe('ModelStore', function() {
}
util.inherits(MySecondModel, BaseModel);

addClassMapping.add(modelUtils.modelName(MySecondModel), MySecondModel);
addClassMapping.add(modelUtils.resourceName(MySecondModel), MySecondModel);

it('should find a model on custom attributes', function(){
var model, modelAttrs, result;
Expand All @@ -126,4 +132,41 @@ describe('ModelStore', function() {
should.equal(result, undefined);
});
});

it("should support storing models without an id if they are in a collection", function() {
var collection, model, collectionAttrs, modelAttrs, resultModel;
collectionAttrs = {
id: 'my_collection'
};
collection = new MyCollection(collectionAttrs);
modelAttrs = {
id : 1,
foo : 'bar'
};
model = new BaseModel(modelAttrs);
model.collection = collection;
this.store.set(model);
resultModel = this.store.get('my_collection', 1);
resultModel.should.eql(modelAttrs);
});

it("should return and instance of BaseModel if a model doesn't have an id and is in a collection", function() {
var collection, model, collectionAttrs, modelAttrs, resultModel;
collectionAttrs = {
id: 'my_collection'
};
collection = new MyCollection(collectionAttrs);
modelAttrs = {
id : 1,
foo : 'bar'
};
model = new BaseModel(modelAttrs);
model.collection = collection;
this.store.set(model);
resultModel = this.store.get('my_collection', 1, true);
resultModel.should.be.an.instanceOf(BaseModel);
resultModel.toJSON().should.eql(modelAttrs);
resultModel.app.should.eql(this.app);
});

});