Sequelize: Hooks refactor + new hooks

[mickhansen]

This is just a issue for the milestone tracker.
We'll keep track of the general hook update progress here.

• [x] context/options on all hooks (#2240)
• [x] find hooks
• [x] connection hooks
• [ ] query hooks
• [ ] build hooks

[mlegenhausen]

Is the new function signature for the currently existing hooks already defined?

[mickhansen]

@mlegenhausen not definitely but probably something like (instances/values, context/options[, done]).

We'll still provide done() for BC but ultimately would like people to return promises instead.

[mlegenhausen]

Sounds good. Need this for patching my own version for accessing the transaction.

[overlookmotel]

I've started work on this. Please see https://github.com/sequelize/sequelize/pull/2240

[overlookmotel]

Concerning https://github.com/sequelize/sequelize/issues/1430, I'm implementing an afterFind hook. Nearly done - just need to write the tests.

[overlookmotel]

Options passed to hooks and .find() hooks implemented by https://github.com/sequelize/sequelize/pull/2240

[mickhansen]

Removed the milestone since the signature change was the only thing relevant to the milestone.

[overlookmotel]

@cusspvz Do you plan to implement any other hooks? I think you were were talking about needing beforeBuild and afterBuild?

Personally I think it'd be useful to have hooks in Sequelize#define() too. But I don't have time right now to do it...

[cusspvz]

It is important to have hooks on Model and Instance instances, in other words after model definition and build. I don't think that before on both is necessarily needed.

[overlookmotel]

@cusspvz My attitude is the more hooks you put in the better. Who knows what people may need to use (they may want to do something quite different from me) and the performance penalty for having more hooks is minimal, so the more the merrier...

But reason I asked was that you mentioned that you needed hooks in Model#build(), so I wondered if you planned to implement it...

[RichAyotte]

@overlookmotel You already have an infinite amount of hooks in JavaScript via techniques such Monkey Patching the Decorator pattern and my favourite AOP. Adding static hooks is not necessary.

See https://github.com/sequelize/sequelize/issues/1485#issuecomment-43741478

[RichAyotte]

Static hooks are much less flexible and pollute the namespace with little to not added benefit. For example, what if you wanted to run a function before the beforeBuild function. Would you create a beforeBeforeBuild function? What about before that one, would we then have beforeBeforeBeforeBuild? What if you need to transform the input or maybe the output of the original function? With AOP you can add before, after and around function to any function, modify the input and output.

[overlookmotel]

@RichAyotte Unless I am misunderstanding something, the thing that monkey-patching or AOP cannot do is allow you to create a hook point in the _middle_ of another function, as for example the hook beforeFindAfterExpandIncludeAll does.

I agree that the schema set out for hooks so far isn't perfect - it's going to get creaky when you have multiple plugins each creating hooks on the same process - but it's a good start.

Or am I missing the point?

[overlookmotel]

@RichAyotte By the way, I have no training in computer science - I'm just a tinkerer who's making this stuff up as I go along! So more than happy to bow to greater knowledge...

[overlookmotel]

Another hooks PR... https://github.com/sequelize/sequelize/pull/2320

[mickhansen]

Build hooks is tricky since build is synchronous and hooks aren't.

[mickhansen]

Guess you're covering the sync issue in #2320 though.

[cusspvz]

@overlookmotel i've understood, if all of you agree i will implement those two and then we should discuss which more hooks sequelize needs.

[cusspvz]

@overlookmotel looks like you've already implemented #2320

[overlookmotel]

@cusspvz Apologies, I discovered I needed it for a project, so I went ahead and did it. Plenty more hooks in the sea if you want to get stuck in...

[RichAyotte]

@overlookmotel With AOP, you would add a before advice to findAll and an after advice to expandIncludeAll but you would need to add expandIncludeAll to the Model's prototype to make it pointcut. It's currently private and can't be accessed from the outside. In addition, with AOP you could implement your own version of expandIncludeAll and not call the original if you needed to which is something that you can't do with hooks.

Hooks are also not free performance wise. For every hook, extra memory and cpu is used whether that hook is implemented or not. FindAll currently has four hooks that are being called every time even if they don't do anything. With AOP you don't have that overhead, functions are only called when the advice has been implemented.

Lastly and most importantly, adding hooks increases the size and complexity of the code base with no added benefit. All the functionality that hooks provide is already available without hooks.

[mickhansen]

@RichAyotte yet to see a decent design pattern using AOP with promises. And in any case it seems like we would have to support the AOP pattern in the code base to make sure promises are propagated around properly.

[RichAyotte]

AOP works fine with promises. I've been combining AOP with promises for some time now. If you have some sample code that involves promises that you think might be difficult to solve with AOP, I would like to take a jab at it.

You wouldn't have to add any support for AOP in Sequelize. I'm using AOP with Sequelize now and it works great. I can invoke functions before and after any Sequelize function without using any of the hooks.

[overlookmotel]

@RichAyotte
I do see your point. As more and more hooks are implemented, the code is going to get cluttered and there will be a performance hit (though I don't know how significant). Using AOP methods would make everything hookable by default without having to code every hook explicitly.

I don't think it'd be hard to write a facility to patch functions AOP-style rather than hooks, which would deal with promises and async callbacks.

But also see two disadvantages. You'd have to break apart a lot of functions to get them to be hook-able in the right places. This would mean:

1. A pollution of the public API with lots of functions which are components of e.g. Model.find() and which would otherwise remain private functions.
2. Breaking apart functions into a lot of sub-functions would make the codebase harder to read.

NB: I think often the best place to be hooking into functions is not the very start and end. For example, I think it's best for options to be parsed before hooks are called.

Would you mind having a look at Model.prototype.update (https://github.com/sequelize/sequelize/blob/master/lib/model.js#L1385) and tell us how this could be re-written this to make it possible with AOP to do everything that the hooks are doing at present?

[cusspvz]

@RichAyotte I don't know much about AOP, but i was interested so i've researched a little bit, please point me if i'm wrong.

AOP works fine with promises. I've been combining AOP with promises for some time now. If you have some sample code that involves promises that you think might be difficult to solve with AOP, I would like to take a jab at it.

AOP method seems to be easily implemented on functions that are sync or accept a callback (I've digged throught aop and node-aop). But i didn't found anything about Promises support. Could you point me the repo?

In my opinion i don't see any advantage on using it instead of @overlookmotel hooks implementation since:

• They should performance similar, i don't see huge changes;
• The method that both repos ( aop and node-aop ) use ( by replacing a function by another with the older as .target and saving _hooks_ on it ) seems a dirty hack, it increases stack on each method and will stage a lot on memory instead of current hooks method, which won't stage behind while your function is running.

[RichAyotte]

@cusspvz A Promise is simply an object with a then function, some standardized parameters and behaviour so it's nothing special. AOP works with any functions and it doesn't require special function or parameters to these functions. There was probably no mention of Promises because there's really no need to single out Promises. Promises behave just like any other object from a JavaScript standpoint.

I haven't done any benchmarks yet but my intuition tells me that:

• Sequelize with hooks but none used would be slower than Sequelize with no hooks and no AOP used.
• Sequelize with hooks and hooks used vs Sequelize with no hooks but AOP used. I suspect that AOP would be faster but the difference would be minor. This isn't the main point though and I slightly regret mentioning it because it detracts from the main points, flexibility and code complexity.

Here a simple example. (I haven't run the code so there could be a typo)

User.beforeCreate(function(user, fn) {
    if (user.accessLevel > 10 && user.username !== "Boss") {
        return fn("You can't grant this user an access level above 10!");
    }
    return fn();
})

Could be written like this.

advise.around(User, 'create', function(origFunc) {
    return function(user) {
        if (user.accessLevel > 10 && user.username !== "Boss") {
            return Promise.reject(new Error("You can't grant this user an access level above 10!"));
        }
        return origFunc.apply(this, arguments);
    };
});

The advantages are

• I achieve the same result without any hooks. All the hooks code can be completely removed.
• As a user, I don't need to know about hook names and the hooks API
• I can run code after the create function and within the same advise. No need to define another hook for after.
• I can skip the original function completely and run something else instead.

[RichAyotte]

@overlookmotel I'll have a look at Model.prototype.update and get back to you.

[mickhansen]

@RichAyotte that is actually very interesting, what would it look like if you need to run an after hook and modify the returned result or similar?

[RichAyotte]

I've created a repo with an example but will be add more to better show this pattern. https://github.com/RichAyotte/sequelize-aop

The original function returns a Promise so you can run the then() and return a new value. In this example only the return value is updated, not the database. To change the database, you would need to update arguments in create.apply(this, arguments).

advise.around(User, 'create', function(create) {
    return function(user) {
        if (user.accessLevel > 10 && user.username !== "Boss") {
            return Promise.reject(new Error("You can't grant this user an access level above 10!"));
        }
        return create.apply(this, arguments)
        .then(function(newUser){
            newUser.dataValues.createdAt = new Date();
            return newUser;
        });
    };
});

[mickhansen]

@RichAyotte yeah i should've seen that immediatly, obviously you can just tack onto the promise chain ;p

[cusspvz]

@RichAyotte thanks for your time to supply an example.
@overlookmotel it works by wrapping the original function, so you don't need to change the original function, since it run hooks on top of the method instead of within.

@RichAyotte honestly I don't think it is a good approach, but that's only my opinion.

[RichAyotte]

@cusspvz You left out the best part, why don't you think it's a good approach?

[cusspvz]

There are some reasons:

• After understanding completely how AOP approach works, i think it seems a solution evolved from an hack, because i envolve functions when i need to trap variables or other methods and when i do that, even I don't like it.
• The main problem of trapped functions is stack, the more you have, bigger is your performance loss (even if it is minor, for those who uses big data like me, it becomes huge).
• I honestly prefer some sort of eventual internal function calling, so we can trigger reactions from an action, thats how Eventual Emitting works, and sort of @overlookmotel implementation aswell.

The only change i would do into @overlookmotel implementation .runHooks would be a return when there aren't hooks to run on.

[mickhansen]

@cusspvz performance isn't really an argument considered the performance overhead of using a promise library ;p

[RichAyotte]

AOP is way to separate concerns and create cohesive and testable code.

For example. Should the User.create function need to be aware of code that happens before or after it? If not, then why mix it all in together? I would argue that before and after functions are separate concerns and belong in their own functions so that they can be tested independently. The create function would be much simpler and easier to test without all the hooks code mixed in.

[RichAyotte]

I forgot to mention consistency. The hooks now depend on a callback with the fn(err, ...) signature. Sequelize has been moving away from callbacks and towards the Promises pattern so removing these callbacks would improve the consistency.

[cusspvz]

@RichAyotte

For example. Should the User.create function need to be aware of code that happens before or after it? If not, then why mix it all in together? I would argue that before and after functions are separate concerns and belong in their own functions so that they can be tested independently.

Well, you're right on some way, but I'm seeing some cases that won't fit this at all.

Imagine that i have to do something to an instance when it is created, before saving it. That wouldn't be practical since you can only access before options and after .then'able with AOP.

[cusspvz]

@mickhansen You're right, but if we can avoid it, better :)

[RichAyotte]

Imagine that i have to do something to an instance when it is created, before saving it. That wouldn't be practical since you can only access before options and after .then'able with AOP.

Sure you can.

advise.around(User, 'create', function(create) {
    return function(user) {
        user.name = 'Tandy 1000sx'
        return create.call(this, user);
    };
});

[cusspvz]

Imagine that i have to do something to an instance when it is created, before saving it.

You're accessing .create options, not the builded instance.

[overlookmotel]

I do in general tend to agree with @RichAyotte that AOP is a neat solution (despite my having just done a bunch of work on hooks which would now need to be re-done!). However, I think it could be tricky in some situations - Model.prototype.update being the most extreme example.

I'll be interested in what @RichAyotte suggests could be done to make that work with AOP.

[RichAyotte]

@cusspvz It can just as easily be done to an instance.

You can either add advice to one instance like this.

var user = {
    name: 'Fred Flinstone'
    , title: 'Stonemaster'
};

User.create(user)
.then(function(newUser){
    advise.around(newUser, 'save', function(save) {
        return function() {
            newUser.dataValues.title = 'Rocker';
            return save.apply(this, arguments);
        };
    });
});

Or all instances like this.

advise.around(User, 'create', function(create) {
    return function(user) {
        return create.apply(this, arguments)
        .then(function(newUser){
            advise.around(newUser, 'save', function(save) {
                return function() {
                    newUser.dataValues.title = 'Rocker';
                    return save.apply(this, arguments);
                };
            });
        });
    };
});

[cusspvz]

User.create(user)
.then(function(newUser){
    advise.around(newUser, 'save', function(save) {
        return function() {
            newUser.dataValues.title = 'Rocker';
            return save.apply(this, arguments);
        };
    });
});

I'm not sure if this will work, since onFulfilled parameter of .then will run only after internal .save call. And i don't think this is necessarily a hook-like, since i had to write it along my app. :S

Don't take it personal, it's my only my point of view, i think AOP approach isn't bad at all for minor projects, since you don't have the need to spend time implementing hooks, but in my opinion it shouldn't be implemented on sequelize.

I'm thinking about using AOP approach on my raspicopter project! :D

[RichAyotte]

@cusspvz I'm glad you're participating in the dicussion :+1:, none of this is personal. I do prefer arguments (the why) over opinions though.

It works, unless I don't understand what you're asking. I've updated my repo with a new test.
https://github.com/RichAyotte/sequelize-aop/blob/master/tests/instance.js

/**
 * @author Richard Ayotte <[email protected]>
 */

'use strict';

var advise = require('dcl/advise')
    , User = require('../models').User
;

var user = {
    name: 'Fred Flinstone'
    , title: 'Stonemaster'
};

User.create(user)
.then(function(newUser){
    advise.around(newUser, 'save', function(save) {
        return function() {
            newUser.name = 'Rocker';
            return save.apply(this, arguments);
        };
    });

    newUser.name = 'Rich';
    newUser.save();
});

Will generate:

INSERT INTO `User` (`id`,`name`,`createdAt`,`updatedAt`) VALUES (DEFAULT,'Fred Flinstone','2014-09-23 20:11:35','2014-09-23 20:11:35');

followed by

UPDATE `User` SET `id`=26,`name`='Rocker',`createdAt`='2014-09-23 20:11:35',`updatedAt`='2014-09-23 20:11:35' WHERE `id`=26

[RichAyotte]

@cusspvz I agree that AOP doesn't belong in Sequelize just as hooks don't either. AOP is not meant to be part of a library, but it can augment it from the outside quite nicely if the library has good cohesion. Which brings me to @overlookmotel, don't worry, I haven't forgotten about you. I need to get back to work atm but I should have some time tomorrow to test things out.

[cusspvz]

@cusspvz I'm glad you're participating in the dicussion :+1:, none of this is personal. I do prefer arguments (the why) over opinions though.

As you may noticed i have some problems with English yet, and sometimes i have afraid that others understand a completely different thing from what i'm trying to express.

It works, unless I don't understand what you're asking. I've updated my repo with a new test.

Well yes, in case of .create you're right, my example didn't cover what i was trying to explain. I thought that beforeCreate would receive an instance instead of options on hook, my fault. :)

But as @overlookmotel said, .update method is a quite good example because it covers following hooks:

• beforeBulkUpdate
• beforeUpdate
• afterBulkUpdate
• afterUpdate

I sincerely don't know how AOP would be implemented here.

AOP is not meant to be part of a library, but it can augment it from the outside quite nicely if the library has good cohesion.

Agree, but if i had to choose I would still prefer hooks on sequelize. :p

[RichAyotte]

I think the examples below cover all the scenarios but let me know if any situation was not covered.

I'm not sure what the dependency between individual updates and bulkUpdate is so for now, the bulk update always runs and the individual update runs only when options.individualHooks is true. This could easily be changed in my example to a different combination.

/**
 * @author Richard Ayotte <[email protected]>
 */

'use strict';

var advise = require('dcl/advise')
    , User = require('../../models').User
;

User.create({name: 'Sam', title: 'Web Lord'})
.then(function(newUser){
    console.log('User created:', newUser.dataValues);
});

// Before
advise.before(User, 'update', function(attrValueHash, where, options) {
    console.log('Running before bulk update');
    if (options && options.individualHooks) {
        console.log('running before update');
    }
});

// After
advise.after(User, 'update', function(args, result) {
    var attrValueHash = args[0];
    var where = args[1];
    var options = args[2];

    return result
    .then(function(){
        console.log('Running after bulk update');
        if (options && options.individualHooks) {
            console.log('running after update');
        }
    });
});

// Around
advise.around(User, 'update', function(update) {
    return function(user) {
        var attrValueHash = arguments[0];
        var where = arguments[1];
        var options = arguments[2];

        console.log('Running before bulk update');
        if (options && options.individualHooks) {
            console.log('running before update');
        }

        console.log(attrValueHash, where, options);
        if (attrValueHash.title == 'Webmaster') {
            attrValueHash.title = 'Web Guru';
        }

        return update.call(this, attrValueHash, where, options)
        .then(function(){
            console.log('Running after bulk update');
            if (options && options.individualHooks) {
                console.log('running after update');
            }
        })
    };
});

// Bulk Update
User.update({
    title: 'Webmaster'
}, {
    where: {
        name: 'Sam'
    }
})
.then(function(){
    console.log('User updated.');
}).catch(function(err){
    console.log(err);
});

// Individual update
User.update({
    title: 'Webmaster'
}, {
    where: {
        name: 'Sam'
    }
}, {
    individualHooks: true
})
.then(function(){
    console.log('User updated.');
}).catch(function(err){
    console.log(err);
});

[cusspvz]

@RichAyotte I'm not confident about this approach, seems more difficult to others implement on their code than current @overlookmotel one. I wouldn't certanely handle data the way around is handling it.

But you must wait for @mickhansen and @janmeier opinion.

[overlookmotel]

I have some thoughts on this - a middle way of sorts between the two approaches. I'll write up a proposal outlining it when I get a chance. But am pretty busy next few days, so it might be a little while...

[cusspvz]

@mickhansen @RichAyotte @overlookmotel @janmeier
Any ideas about hook capability to return data into method returned promise?

I will need this capability on our cache plugin, so we can return data on a Model.beforeFind hook.

[mickhansen]

@cusspvz i was thinking that beforeFind could return an array/object or undefined. If undefined, progress as normally, if array or object, return.

[mickhansen]

The only problem with that is that if people return a promise to the hook handler they will HAVE to use return(undefined)

[cusspvz]

@mickhansen nice.

Shouldn't also beforeFind and afterFind run when the model is included on other's model find?

[cusspvz]

Maybe a beforeIncludedFind and afterIncludedFind hook names should result better since you could not have the same logic for beforeFind and beforeIncludedFind.

[mickhansen]

@cusspvz hmm i suppose that might be a pretty cool feature. But if you're doing a cache plugin you have access to the includes and can call the specific finds directly.

[cusspvz]

@mickhansen We don't need that hook ((before|after)IncludedFind) on our cache plugin.

We are predicting some internal plugins (one of them we are already developing) that will need to append data into instance's VIRTUAL attributes, they won't work if beforeFind isn't called from included queries.

I could implement this hook after we agree so.

API Proposal

beforeIncludedFind( includeOptions, options, queryOptions )
afterIncludedFind( instances, includeOptions, results, options )

The reason why I separated results from instances is that, you could manipulate current model's instances or target model results.

[overlookmotel]

Just to throw in my two-pennies worth here:

1. Concerning returning results from a hook

This is really interesting. I hadn't thought of this possibility when I was mucking around with the hooks code.

Personally, I think returning results from the hook directly is a bad idea, for the reasons that @mickhansen gave - it would require every other hook function to return(undefined). That's a obvious cause of bugs, and also a bit of a code bloat for every hook author, balanced by supporting a feature that I think is fairly niche.

I'd suggest something like:

• Implement an option called something like exec passed to findAll()
• options.exec controls whether the actual db query executes or not. It defaults to true (usual behaviour)
• Setting options.exec to false skips the running of the actual query, much like setting options.hooks to false skips running the hooks
• A cache plugin could have a beforeFind hook which sets options.exec to false, so that no query actually gets run on the DB and then an afterFind hook that creates the result set which gets returned.

Bear in mind that these hooks should play nice with other hooks which may be defined on the model (such as those defined by another plugin). So leaving the beforeFind hook pretty small and the afterFind hook the one that produces the result set (which can then be passed to other hooks) makes sense to me.

Or... much simpler... just create a replaceFind hook which, if set, runs _instead_ of the db query.

2. Concerning beforeIncludedFind/afterIncludedFind

Am I wrong in thinking that this could impose quite a performance penalty to all queries?

In order to offer a hook that could execute on any include, as far as I can see, the whole include tree would have to be traversed looking for whether a beforeIncludedFind hook is defined on that included model. And that would happen on every single findAll() call. When you have complex queries with a lot of includes, this could be a bit of a penalty. And, again, I wonder if the use case is pretty niche to justify a performance hit if there was one.

@cusspvz I'm not quite sure what you're trying to achieve with the virtual fields, but you might want to look at sequelize-virtual-fields which does a similar trick of finding references to virtual fields throughout the include tree and doing things with them, while using only the hooks that already exist.

[overlookmotel]

In general (and I know this isn't a very cogent or useful thing to say) but I think the hooks implementation is creaking at the seams a bit. I think that in 6-12 months time, you'll be wishing they were done differently.

Here are the problems as I see them:

Bloat and performance

What we're seeing is different use cases coming up which require more and more hooks (and more and more bizarre and niche use kinds of hooks) to be implemented in order to support some particular functionality.

As the number of hooks grows, it's going to get slower and slower. And note that this is a performance penalty on _every_ query, regardless of whether the hooks are being used or not. The more I think about what @RichAyotte was talking about earlier in this thread, the more I think he was on the right lines...

Ordering & interaction

At present hooks are executed in order of which was defined first, and model-specific hooks execute before global hooks.

Right now, order of execution is not a big deal as most people won't have many hooks in use. But if the gameplan is to put more and more functionality out into plugins (which personally I think is the right way to go) and to hopefully grow a rich ecosystem of plugins, sooner or later we're going to start seeing plugins clashing with each other in ways that are unexpected and hard to debug.

Here's a simple example that I've already run into:

Plugin 1 defines hooks to do action A before the db query and action Z after
Plugin 2 defines hooks to do action B before the db query and action Y after

When you just have plugin 1 installed, order of execution goes: A -> db query -> Z
When you just have plugin 2 installed, order of execution goes: B -> db query -> Y

Now what happens when you install both plugins? Because both beforeFind and afterFind hooks execute in the order they are defined:

order of execution goes: A -> B -> db query -> Z -> Y

What's wrong with that? Well plugin 2 expects that it has it's getting a result into Y direct from the db, but in fact Z has come in between and may have monkeyed around with the result set.

Here's how I think you want it:

order of execution goes: A -> B -> db query -> Y -> Z

So it's like layers of an onion. Hooks are defined as pairs rather than individually, so each hook-pair that's added goes on the outside of what already there. Sort of "first in, last out".

A plugin's hooks may not be running straight after the db query, but that's fine - whatever transformation happens in B and Y won't break A and Z as long as B and Y mirror each other in what they do to the options on the way in and the results on the way out.

If you had 5 plugins it'd be:

order of execution goes: A -> B -> C -> D -> E -> db query -> V -> W -> X -> Y -> Z

(note pair A and Z on the very outside)

This is hard to explain. Would be easier with a more concrete example, but I can't think of one right now.

But, anyway, this is just the tip of the iceberg. It'll get much more complicated than this.

The problem will be that everyone will only run tests on their plugin in isolation, and then once you put multiple plugins together, they'll start interacting in mysterious ways and no-one will know both plugin code-bases well enough to easily figure out what's going wrong.

Basically this could make Sequelize plugin development a ball-ache, which will discourage people from doing it.

Namespacing

I mentioned this in another post concerning plugins. And mostly it applies to plugins, but since hooks are the basis of building plugins, it applies here to.

Basically, there needs to be some way of namespacing the options and state of plugins and hooks so that they don't clash with each other.

Conclusion

Well that wasn't very positive, was it?

I'm not intending to be a pain, just that I think this needs some further thought before Sequelize is ready to start shouting "come and make plugins for me!". Or in a year's time you could be finding yourself wanting to rip up the hooks/plugins system and do it again from scratch, but unable to because of backward compatibility.

Anyway, just trying to spur discussion....

[overlookmotel]

PS Sadly I don't have many answers to the issues I'm raising. I'm hoping wiser and more experienced minds may have some ideas...

[cusspvz]

1

Or... much simpler... just create a replaceFind hook which, if set, runs instead of the db query.

Currently I can't bring a better idea than this, but this proposal could lead to another problem, you may just need to eval if your plugin wants or not to replace response value. Just checking for undefined doesn't seems to be a nice approach.

I'm just thinking on a idiot solution while I write this, but it could work... Why we don't create a context with a method that returns a certain Array type?

Implementation example:

Sequelize.Instances = function ( instances ) {
  return Utils._.extend( this, instances );
};
Sequelize.Instances.prototype = Object.create( Array.prototype );

// ...

var context = Object.create( Model );

context.replace = function ( instances ) {
  return new Sequelize.Instances( instances );
};

hook.apply( context, /* ... */ );

// On `Model.findAll` or `runHook` logic we just need to check if returned value is instanceof Sequelize.Instances.

Usage example:

Model.hook( 'beforeFind', function ( options ) {
  return Cache.load( options )
    .then(function ( instances ) {
      return instances ? this.replace( instances ) : false;
    });
});

2

Am I wrong in thinking that this could impose quite a performance penalty to all queries?

You're completely right, which leads to this question: { hooks: false || true, } option is only boolean or could you specify which hooks you wan't to run?

If not, we could present an object with hooks we wan't to run, which we could control with default hooks options, and set this as not default. Just thinking...

[cusspvz]

Bloat and performance

As the number of hooks grows, it's going to get slower and slower. And note that this is a performance penalty on every query, regardless of whether the hooks are being used or not. The more I think about what @RichAyotte was talking about earlier in this thread, the more I think he was on the right lines...

I don't see a bottleneck on your hooks implementations, if you think so, you would need to say worse things around Promises, EventEmitting or even AOP.

Certainly we should think about the way we call hooks, so they don't tick a promise if they don't have hooks.

Ordering & interaction

This is a nice topic. Prioritization of hooks should be a nice feature request, unless we handle with .plug as connect does with .use, by placing priority at developer's hands.

Namespacing

Today i had some issues with namespacing since Models aren't extendable.
I can't access an user request from any hook, neither by placing it at Model, neither on CLS.

I had an issue talking about that: #2036

As i didn't fixed it yet, i'm thinking about creating an ugly workaround, like this:

function ( sequelize, app ) {
  // Wait for all configurations
  Promise.all([ sequelize.ready, app.ready ])
    .then(function () {
      app.use(function ( req, res, next ) {
        req.sequelize = Object.create( sequelize );
        req.sequelize.models = req.models = Object.create( req.sequelize.models );

        Util.each( req.models, function ( Model ) {
          Model = Object.create( Model );
          Model.sequelize = req.sequelize;
          Model.req = req; Model.res = res;
          return Model;
        });

        next();
      });
    })
}

[cusspvz]

@mickhansen I need to implement a caching layer on the next 2 days, could we discuss about how to implement it via hooks?

If I had the chance, i would vote to allow returnable values, only on beforeFind for now.

Related with #803

[mickhansen]

@cusspvz As i remember we don't have a good design for it yet. Having to remember to do .return(undefned) everytime is not ideal. Very open for suggestions on this (it could be attaching an option)

[cusspvz]

Attaching an option to allow returning something trough the hook option? I think the issue would maintain since a cache plugin would need to mark that option as default.

I was thinking on something internal, like assigning a sub-method on callback, or returning a specific wrapper Class.

Lets skip to approaches API examples...

Approach 1 - sub-method on callback, by calling done.return

Model.hook( 'beforeFind', function ( options, done ) {
  return Promise.try(function () { return []; })
    .then(function ( instances ) {
      done.return( instances );
    });
});

Approach 2 - returning using promise context

Model.hook( 'beforeFind', function ( options, done ) {
  return Promise.try(function () { return []; })
    .then(function ( instances ) {
      this.return = instances;
    });
});

Approach 3 - returning an instance of some WrapperClass

var Sequelize = require( 'sequelize' );

Model.hook( 'beforeFind', function ( options, done ) {
  return Promise.try(function () { return []; })
    .then(function ( instances ) {
      return new Sequelize.Return( instances );
    });
});

NOTE to all approaches: if there are results to be returned, we shouldn't run next hooks on stack.

[cusspvz]

PS: I've added a proposal (#2932) on a separated issue so we can have separated discussions.

[mickhansen]

I meant adding a flag to the options inside the hook, sorry if that wasn't clear.
Not sure adding to the promise context is not feasible since that's influenced by bind.
Agree that if values are returned it will short circuit hook execution.

Honestly not a huge fan of any of those 3 approached (not that i have any better suggestions at the moment).

[cusspvz]

The only one that seems more reasonable for me its the last one, but I agree with you, I don't think that it is good enough...

[mickhansen]

Another approach could be:

Model.hook( 'beforeFind', function (options) {
  this.returnsInstances = true;
  return Promise.try(function () { return []; });
});

Edit: Oh wait, this is the Model for all hooks.

[cusspvz]

Edit: Oh wait, this is the Model for all hooks.

I was thinking exacly that... :)

[cusspvz]

I was thinking about another approach, a dirty one, but since I'm on verbose mode caused by the lack of solutions... :D

Approach 5 - setting _sequelizeResult

Model.hook( 'beforeFind', function ( options, done ) {
  return Promise.try(function () { return []; })
    .then(function ( instances ) {
      instances._sequelizeResult = true;
      return instances;
    });
});

Then, obviously, after response evaluation it should be deleted before returned to original method returned promise.

[cusspvz]

I hope this last approach could help you to lead to a better one! :)

[freebrevik]

Is there any hook out there that can be called after a model is created no matter how its created? (through build , create, or whatever). I have some post processing I would like to do on some models..

[mickhansen]

@awdogsgo2heaven build/save, findOrCreate and create are handled by the beforeCreate, upsert is not handled by that.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue, just leave a comment 🙂

[jacob-israel-turner]

Just curious if afterFind was ever implemented? I'd love to use to to apply formatting (applying a base URL to an image column) across the board, rather than manually after each find.

[guylhermetabosa]

@jacob-israel-turner I don't think so, but would be great to have this feature.

[overlookmotel]

@jacob-israel-turner @guylhermetabosa Yes afterFind hook was implemented about 3 years ago!

[guylhermetabosa]

@overlookmotel Glad to know that, but I wasn't able to find in the docs.

[jacob-israel-turner]

@overlookmotel Awesome! Good to know! Would be nice to have in the docs, as @guylhermetabosa mentioned. If there's no update to the docs before I'm able to implement afterFind in my codebase, I'll take a whack at a PR updating them.

[overlookmotel]

@jacob-israel-turner Just to clarify, I'm not a maintainer of Sequelize. I've not contributed for a couple of years now as busy with other things. So if you want it in the docs, yes make a PR!