Skip to content

Commit

Permalink
Merge pull request #410 from saponifi3d/fix-model-retrival-from-store
Browse files Browse the repository at this point in the history 
Fix model retrival from store
  • Loading branch information
@saponifi3d
saponifi3d committed Dec 4, 2014
2 parents 7939fc9 + 457dce8 commit ceffdb2
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 53 deletions.
52 changes: 24 additions & 28 deletions shared/fetcher.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,32 +109,23 @@ Fetcher.prototype._retrieve = function(fetchSpecs, options, callback) {

_.each(fetchSpecs, function(spec, name) {
batchedRequests[name] = function(cb) {
var modelData, modelOptions;
var model;

if (!options.readFromCache) {
this.fetchFromApi(spec, options, cb);
} else {
modelData = null;
modelOptions = {};
model = null;

// First, see if we have stored the model or collection.
if (spec.model != null) {

this._retrieveModel(spec, function(err, modelData) {
this._retrieveModelData(spec, modelData, modelOptions, options, cb);
this._retrieveModel(spec, function(err, model) {
this._refreshData(spec, model, options, cb);
}.bind(this));

} else if (spec.collection != null) {

this.collectionStore.get(spec.collection, spec.params, function(collection) {
if (collection) {
modelData = this.retrieveModelsForCollectionName(spec.collection, _.pluck(collection.models, 'id'));
modelOptions = {
meta: collection.meta,
params: collection.params
};
}
this._retrieveModelData(spec, modelData, modelOptions, options, cb);
this._refreshData(spec, collection, options, cb);
}.bind(this));

}
Expand All @@ -145,22 +136,21 @@ Fetcher.prototype._retrieve = function(fetchSpecs, options, callback) {
async.parallel(batchedRequests, callback);
};

Fetcher.prototype._retrieveModelData = function(spec, modelData, modelOptions, options, cb) {
Fetcher.prototype._refreshData = function(spec, modelOrCollection, options, cb) {

// If we found the model/collection in the store, then return that.
if (!this.needsFetch(modelData, spec)) {
model = this.getModelOrCollectionForSpec(spec, modelData, modelOptions);

if (!this.needsFetch(modelOrCollection, spec)) {
/**
* If 'checkFresh' is set (and we're in the client), then before we
* return the cached object we fire off a fetch, compare the results,
* and if the data is different, we trigger a 'refresh' event.
*/
if (spec.checkFresh && !isServer && this.shouldCheckFresh(spec)) {
model.checkFresh();
modelOrCollection.checkFresh();
this.didCheckFresh(spec);
}
cb(null, model);

cb(null, modelOrCollection);
} else {
/**
* Else, fetch anew.
Expand All @@ -174,9 +164,8 @@ Fetcher.prototype._retrieveModel = function(spec, callback) {

// Attempt to fetch from the modelStore based on the idAttribute
this.modelUtils.modelIdAttribute(spec.model, function(idAttribute) {
var modelData = fetcher.modelStore.get(spec.model, spec.params[idAttribute]);
if (modelData)
return callback(null, modelData);
var model = fetcher.modelStore.get(spec.model, spec.params[idAttribute]);
if (model) return callback(null, model);

// if there are no other keys than the id in the params, return null;
if (_.isEmpty(_.omit(spec.params, idAttribute)))
Expand All @@ -187,11 +176,15 @@ Fetcher.prototype._retrieveModel = function(spec, callback) {
});
};

Fetcher.prototype.needsFetch = function(modelData, spec) {
if (modelData == null) return true;
if (this.isMissingKeys(modelData, spec.ensureKeys)) return true;
Fetcher.prototype.needsFetch = function(modelOrCollection, spec) {
if (modelOrCollection == null) return true;

if (this.modelUtils.isModel(modelOrCollection) && this.isMissingKeys(modelOrCollection.attributes, spec.ensureKeys)) {
return true;
}

if (spec.needsFetch === true) return true;
if (typeof spec.needsFetch === 'function' && spec.needsFetch(modelData)) return true;
if (typeof spec.needsFetch === 'function' && spec.needsFetch(modelOrCollection)) return true;
return false;
};

Expand All @@ -201,9 +194,11 @@ Fetcher.prototype.isMissingKeys = function(modelData, keys) {
if (keys == null) {
return false;
}

if (!_.isArray(keys)) {
keys = [keys];
}

for (var i = 0, len = keys.length; i < len; i++) {
key = keys[i];
if (modelData[key] == null) {
Expand All @@ -216,6 +211,7 @@ Fetcher.prototype.isMissingKeys = function(modelData, keys) {
Fetcher.prototype.fetchFromApi = function(spec, options, callback) {
var model = this.getModelOrCollectionForSpec(spec),
fetcher = this;

model.fetch({
headers: options.headers || {},
data: spec.params,
Expand Down Expand Up @@ -308,7 +304,7 @@ Fetcher.prototype.hydrate = function(summaries, options, callback) {
async.forEach(_.keys(summaries), function(name, cb) {
var summary = summaries[name];
if (summary.model != null) {
results[name] = fetcher.modelStore.get(summary.model, summary.id, true);
results[name] = fetcher.modelStore.get(summary.model, summary.id);

if ((results[name] != null) && (options.app != null)) {
results[name].app = options.app;
Expand Down
20 changes: 7 additions & 13 deletions shared/store/model_store.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,26 +16,20 @@ _.extend(ModelStore.prototype, Super.prototype, {
if (modelName == null) {
throw new Error('Undefined modelName for model');
}

key = this._getModelStoreKey(modelName, id);

// Make sure we have a fully parsed model before we store the attributes
model.parse(model.attributes);

return Super.prototype.set.call(this, key, model, null);
},

get: function(modelName, id, returnModelInstance) {
get: function(modelName, id) {
var key, model;

if (returnModelInstance == null) {
returnModelInstance = false;
}
key = this._getModelStoreKey(modelName, id);
model = Super.prototype.get.call(this, key);
if (model) {
if (returnModelInstance) {
return model;
} else {
return model.toJSON();
}
}
return Super.prototype.get.call(this, key);
},

find: function(modelName, params) {
Expand All @@ -54,7 +48,7 @@ _.extend(ModelStore.prototype, Super.prototype, {
});

if (foundKey) {
return this.cache[foundKey].value.toJSON();
return this.cache[foundKey].value;
}
},

Expand Down
4 changes: 2 additions & 2 deletions test/shared/base/collection.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -175,9 +175,9 @@ describe('BaseCollection', function() {
app: this.app
});
collection.store();
models.forEach(function(modelAttrs) {
models.forEach(function(modelAttrs, i) {
var storedModel = _this.app.fetcher.modelStore.get(collection.model.name, modelAttrs.id);
storedModel.should.eql(modelAttrs);
storedModel.should.eql(collection.models[i]);
});
storedCollection = this.app.fetcher.collectionStore.get(this.MyCollection.name, collection.params);
_.pluck(storedCollection.models, 'id').should.eql(_.pluck(models, 'id'));
Expand Down
10 changes: 5 additions & 5 deletions test/shared/base/model.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ describe('BaseModel', function() {
app: this.app
});
stored = this.app.fetcher.modelStore.get(this.MyModel.id, model.id);
stored.should.deep.eql(attrs);
stored.should.deep.eql(model);
});

it("should update modelStore when id attribute changes", function() {
Expand All @@ -39,22 +39,22 @@ describe('BaseModel', function() {
app: this.app
});
stored = this.app.fetcher.modelStore.get(this.MyModel.id, model.id);
stored.should.deep.eql(attrs);
stored.should.deep.eql(model);

attrs.id = 10;
model.set({
id: attrs.id
});
stored = this.app.fetcher.modelStore.get(this.MyModel.id, model.id);
stored.should.deep.eql(attrs);
stored.should.deep.eql(model);

// Add an attribute, make sure the store gets updated.
attrs.name = 'Bobert';
model.set({
name: attrs.name
});
stored = this.app.fetcher.modelStore.get(this.MyModel.id, model.id);
stored.should.deep.eql(attrs);
stored.should.deep.eql(model);
});

describe('store', function() {
Expand All @@ -70,7 +70,7 @@ describe('BaseModel', function() {
});
model.store();
stored = this.app.fetcher.modelStore.get(this.MyModel.id, model.id);
stored.should.eql(attrs);
stored.should.eql(model);
});
});

Expand Down
26 changes: 24 additions & 2 deletions test/shared/fetcher.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -461,6 +461,7 @@ describe('fetcher', function() {
collection: 'Listings'
}
};

fetcher.fetch(fetchSpec, {
writeToCache: true
}, function(err, results) {
Expand All @@ -469,7 +470,10 @@ describe('fetcher', function() {
results.collection.toJSON().should.eql(buildCollectionResponse());

// Make sure that the basic version is stored in modelStore.
fetcher.modelStore.get('Listing', 1).should.eql(getModelResponse('basic', 1));
var model = results.collection.get(1)
var storedModel = fetcher.modelStore.get('Listing', 1);

storedModel.should.eql(model);

// Then, fetch the single model, which should be cached.
fetchSpec = {
Expand All @@ -485,7 +489,7 @@ describe('fetcher', function() {
readFromCache: true
}, function(err, results) {
if (err) return done(err);
results.model.toJSON().should.eql(getModelResponse('basic', 1));
results.model.should.eql(model);

// Finally, fetch the single model, but specifiy that certain key must be present.
fetchSpec = {
Expand All @@ -497,6 +501,7 @@ describe('fetcher', function() {
ensureKeys: ['city']
}
};

fetcher.fetch(fetchSpec, {
readFromCache: true
}, function(err, results) {
Expand Down Expand Up @@ -633,6 +638,23 @@ describe('fetcher', function() {
});
});

describe('retrieveModels', function() {
var modelAttrs;

beforeEach(function () {
modelAttrs = { id: 1 };

this.expectedModel = new Listing(modelAttrs)
fetcher.modelStore.set(this.expectedModel);
});

it('should return the models from the given ids', function () {
// it should be the exact same model
this.expectedModel.should.equal(fetcher.retrieveModels('Listing', [1])[0])
this.expectedModel.should.deep.equal(fetcher.retrieveModels('Listing', [1])[0])
});
});

describe('checkFresh', function() {
describe('didCheckFresh', function() {
beforeEach(function() {
Expand Down
13 changes: 10 additions & 3 deletions test/shared/store/model_store.test.js
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
var util = require('util'),
_ = require('underscore'),
should = require('chai').should(),
sinon = require('sinon'),
ModelStore = require('../../../shared/store/model_store'),
BaseModel = require('../../../shared/base/model'),
ModelUtils = require('../../../shared/modelUtils'),
Expand Down Expand Up @@ -33,10 +34,16 @@ describe('ModelStore', function() {
foo: 'bar',
id: 1
};

model = new MyModel(modelAttrs);
sinon.spy(model, 'parse');

this.store.set(model);
result = this.store.get('my_model', 1);
result.should.eql(modelAttrs);

result.should.eql(model);
model.parse.should.have.been.called;
model.parse.restore();
});

it("should support custom idAttribute", function() {
Expand All @@ -57,7 +64,7 @@ describe('ModelStore', function() {
model = new MyCustomModel(modelAttrs);
this.store.set(model);
result = this.store.get(modelUtils.modelName(MyCustomModel), modelAttrs.login);
result.should.eql(modelAttrs);
result.should.eql(model);
});

it("should support returning a model instance", function() {
Expand Down Expand Up @@ -93,7 +100,7 @@ describe('ModelStore', function() {
model = new MyModel(modelAttrs);
this.store.set(model);
result = this.store.find('my_model', {foo: 'bar'});
result.should.eql(modelAttrs);
result.should.eql(model);
});

it('should skip different models, even when they match the query', function(){
Expand Down

0 comments on commit ceffdb2

Please sign in to comment.