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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d44286e
Use collectionName for ModelStore key when modelName isn't set
shebson Jan 6, 2014
894572e
Renamed getModelOrCollectionName and handle module not found in client
shebson Feb 19, 2014
78aa90b
Merge branch 'airbnb-master' into fix-modelname-and-maintain-classica…
shebson Feb 19, 2014
c0dccfd
Merged upstream changes and updated resourceName tests
shebson Feb 19, 2014
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,12 +62,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.modelOrCollectionName(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.modelOrCollectionName(options.collection.constructor);
options.collection_params = options.collection.params;
}

Expand Down
16 changes: 10 additions & 6 deletions shared/fetcher.js
View file
Open in desktop
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.modelOrCollectionName(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.modelOrCollectionName(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.modelOrCollectionName(modelOrCollection.constructor),
id: modelOrCollection.get(idAttribute)
};
}
Expand Down
16 changes: 12 additions & 4 deletions shared/modelUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,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 @@ -29,7 +29,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' && fallbackToBaseModel) {
Copy link
Member

Choose a reason for hiding this comment

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

That e.code will only be set on the server, correct? Does Browserify set the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, e.code is not set in the browser. We could do something like e.code === 'MODULE_NOT_FOUND' || e.match(/module '.*' not found/).

Model = BaseModel;
} else {
throw e;
}
}
return new Model(attrs, options);
}
};
Expand Down Expand Up @@ -87,7 +95,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.modelOrCollectionName(Collection.prototype.model);
};

ModelUtils.uppercaseRe = /([A-Z])/g;
Expand Down Expand Up @@ -121,7 +129,7 @@ ModelUtils.prototype.underscorize = function(name) {
* -> ""
* MyClass.id = "MyClass"
*/
ModelUtils.prototype.modelName = function(modelOrCollectionClass) {
ModelUtils.prototype.modelOrCollectionName = function(modelOrCollectionClass) {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can find a less verbose name for this? Perhaps we could use resource instead of modelOrCollection?

Copy link
Member

Choose a reason for hiding this comment

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

It actually describes exactly what the method does. I think the problem here is not the name but rather doing two things at once.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I absolutely agree that modelOrCollectionName is unwieldy. I chose it for consistency with modelOrCollection, a variable which is used in the fetcher and view engine, but I agree that resourceName is better.

I think that this is just a naming issue, though, and not a problem of trying to do two things in one method. This method takes a constructor and returns its underscorized id. I see that as one task with multiple applications.

return this.underscorize(modelOrCollectionClass.id || modelOrCollectionClass.name);
};

Expand Down
2 changes: 1 addition & 1 deletion shared/store/collection_store.js
View file
Open in desktop
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.modelOrCollectionName(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
View file
Open in desktop
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, modelOrCollectionName, newAttrs, constructor;

id = model.get(model.idAttribute);
modelName = this.modelUtils.modelName(model.constructor);
if (modelName == null) {
throw new Error('Undefined modelName for model');
modelOrCollectionName = this.modelUtils.modelOrCollectionName(model.constructor);
Copy link
Member

Choose a reason for hiding this comment

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

In this case, it will always be a modelName returned -- var modelOrCollectionName may be confusing.

Copy link
Member

Choose a reason for hiding this comment

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

No, as @shebson said in the PR description:

When a model does not have a modelName and belongs to collection that is named, use the collectionName in place of the modelName ...

So this method may return the collectionName even for a model.

But you are right this behaviour is very confusing and we should probably try to find a better solution to fix #151.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@spikebrehm is right. On this line, the modelOrCollectionName method will always return a modelName, as we're passing a model's constructor to it. This is a good example of why modelOrCollectionName is a bad name for the method and something like resourceName would be better.

I agree that var modelName is much clearer than var modelOrCollectionName.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I haven't seen that this is implemented different as in #252.

If you rename it to modelName than you have to use an extra variable for the next line where modelOrCollectionName get called with the collection constructor.
Maybe something like keyPrefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think keyPrefix is a very good name for this. Much clearer than modelOrCollectionName, and it doesn't require using a separate variable for collectionName.

if (!modelOrCollectionName && model.collection) {
modelOrCollectionName = this.modelUtils.modelOrCollectionName(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.
*/
modelOrCollectionName = modelOrCollectionName || '';
key = this._getModelStoreKey(modelOrCollectionName, id);

/**
* We want to merge the model attrs with whatever is already
* present in the store.
*/
existingAttrs = this.get(modelName, id) || {};
existingAttrs = this.get(modelOrCollectionName, 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(modelOrCollectionName, id, returnModelInstance) {
var key, modelData;

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

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

ModelStore.prototype.find = function(modelName, params) {
ModelStore.prototype.find = function(modelOrCollectionName, params) {
var prefix, foundCachedObject, _this, data, foundCachedObjectKey;
prefix = this._formatKey(this._keyPrefix(modelName));
prefix = this._formatKey(this._keyPrefix(modelOrCollectionName));
_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(modelOrCollectionName) {
return this.modelUtils.underscorize(modelOrCollectionName);
}

ModelStore.prototype._getModelStoreKey = function(modelName, id) {
return this._keyPrefix(modelName) + ":" + id;
ModelStore.prototype._getModelStoreKey = function(modelOrCollectionName, id) {
return this._keyPrefix(modelOrCollectionName) + ":" + id;
}
51 changes: 47 additions & 4 deletions test/shared/store/model_store.test.js
View file
Open in desktop
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.modelOrCollectionName(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.modelOrCollectionName(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.modelOrCollectionName(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);
});

});