Skip to content
This repository was archived by the owner on Jun 20, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 1 commit
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
20 changes: 19 additions & 1 deletion lib/waterline/query/finders/basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ var usageError = require('../../utils/usageError'),
waterlineCriteria = require('waterline-criteria'),
_ = require('lodash'),
async = require('async'),
hasOwnProperty = utils.object.hasOwnProperty;
hasOwnProperty = utils.object.hasOwnProperty,
callbacks = require('../../utils/callbacksRunner');

module.exports = {

Expand Down Expand Up @@ -45,6 +46,10 @@ module.exports = {
// Normalize criteria
criteria = normalize.criteria(criteria);

this.beforeCallbacks.call(self,criteria.where,function(err){

Choose a reason for hiding this comment

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

So, this guys first point is a problem I think. If no criteria is passed to Model.find then values will be null when it gets into the beforeFind callback.

balderdashy#902 (comment)

Might need to do criteria.where || {} or something like that...not totally sure of the implications there.

Choose a reason for hiding this comment

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

Solution suggested here balderdashy#902 (comment)

if (err) {return cb(err);}
});

Choose a reason for hiding this comment

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

The code flow here is so weird. Basically, we could have cb called more than once, if one of the beforeCallbacks returns an error. I know it's how they do it in the original waterline PR, but feels totally bizarre. Can't help but wonder if there's a better way to do it, but also don't want to get too far away from how they do things normally with waterline.

Choose a reason for hiding this comment

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

Looking at how beforeDestroy and beforeUpdate work, the code flow is much nicer. They appropriately handle all async callbacks and such. It feels like the right thing to do would be to put everything after this inside the callback. That way folks could do async operations if they wanted to.

Would up the line count change significantly, but would just be indentation.


// Return Deferred or pass to adapter
if(typeof cb !== 'function') {
return new Deferred(this, this.findOne, criteria);
Expand Down Expand Up @@ -224,6 +229,10 @@ module.exports = {
return usageError('Invalid options specified!', usage, cb);
}

this.beforeCallbacks.call(self,criteria.where,function(err){
if (err) return cb(err);
})

Choose a reason for hiding this comment

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

Missing ;


// Return Deferred or pass to adapter
if(typeof cb !== 'function') {
return new Deferred(this, this.find, criteria, options);
Expand Down Expand Up @@ -395,6 +404,15 @@ module.exports = {
});
},

beforeCallbacks:function(criteria,cb){

Choose a reason for hiding this comment

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

It's not clear to me why this function exists. All it does is call callbacks.beforeFind, but it's wrapped in this beforeCallbacks function that then wraps that one call in async.series. Is the idea that eventually there may be more than just beforeFind?

var self = this;
async.series([
function(cb){
callbacks.beforeFind(self,criteria,cb);
}
]);
},

where: function() {
this.find.apply(this, Array.prototype.slice.call(arguments));
},
Expand Down
3 changes: 2 additions & 1 deletion lib/waterline/utils/callbacks.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,5 +10,6 @@ module.exports = [
'beforeCreate',
'afterCreate',
'beforeDestroy',
'afterDestroy'
'afterDestroy',
'beforeFind'
];
18 changes: 18 additions & 0 deletions lib/waterline/utils/callbacksRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,3 +138,21 @@ runner.afterDestroy = function(context, values, cb) {

async.eachSeries(context._callbacks.afterDestroy, fn, cb);
};

/**
* Run before Find Callbacks
*
* @param {Object} context
* @param {Object} values
* @param {Function} cb
* @api public
*/

runner.beforeFind = function(context, values, cb) {

var fn = function(item, next) {
item(values, next);
};

async.eachSeries(context._callbacks.beforeFind, fn, cb);
};
155 changes: 155 additions & 0 deletions test/unit/callbacks/beforeFind.find.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
var Waterline = require('../../../lib/waterline'),
assert = require('assert');

describe('.beforeFind()', function() {

describe('basic function', function() {
var person;

before(function(done) {
var waterline = new Waterline();
var Model = Waterline.Collection.extend({
identity: 'user',
connection: 'foo',
attributes: {
name: 'string',
isDeleted:'boolean'
},

beforeFind: function(values, cb) {
values.isDeleted = false;
cb();
}
});

waterline.loadCollection(Model);

// Fixture Adapter Def
var adapterDef = {
find: function(con, col, values, cb) { return cb(null, [values.where]);},
findOne:function(conn,col,values,cb){return cb(null,values.where);}
};

var connections = {
'foo': {
adapter: 'foobar'
}
};

waterline.initialize({ adapters: { foobar: adapterDef }, connections: connections }, function(err, colls) {
if(err) done(err);
person = colls.collections.user;
done();
});
});

// /**
// * find
// */

describe('.find()', function() {

it('should run beforeFind and mutate values', function(done) {

person.find({name:'billy'}, function(err, users) {
assert(!err);
assert(users[0].isDeleted === false);
done();
});

});
});


// /**
// * findOne
// */

describe('.findOne()', function() {

it('should run beforefind and mutate values', function(done) {

person.findOne({name:'billy'}, function(err, user) {
assert(!err);
assert(user.isDeleted === false);
done();
});

});
});



});


/**
* Test Callbacks can be defined as arrays and run in order.
*/

describe('array of functions', function() {
var person;

before(function(done) {
var waterline = new Waterline();
var Model = Waterline.Collection.extend({
identity: 'user',
connection: 'foo',
attributes: {
name: 'string'
},

beforeFind: [
// Function 1
function(values, cb) {
values.name = values.name + ' fn1';
cb();
},

// Function 2
function(values, cb) {
values.name = values.name + ' fn2';
cb();
}
]
});

waterline.loadCollection(Model);

// Fixture Adapter Def
var adapterDef = {
find: function(con, col, values, cb) { return cb(null, [values.where,]);},
findOne:function(conn,col,values,cb){return cb(null,values.where);}
};

var connections = {
'foo': {
adapter: 'foobar'
}
};

waterline.initialize({ adapters: { foobar: adapterDef }, connections: connections }, function(err, colls) {
if(err) done(err);
person = colls.collections.user;
done();
});
});

it('should run the functions in order on find()', function(done) {
person.find({ name: 'test' }, function(err, users) {
assert(!err);
assert(users[0].name === 'test fn1 fn2');
done();
});
});
it('should run the functions in order on findOne()', function(done) {
person.findOne({ name: 'test' }, function(err, user) {
assert(!err);
assert(user.name === 'test fn1 fn2');
done();
});
});

});

});