sequelize/sequelize — pull request #1788 by overlookmotel (2014-05-20 11:25:17+00:00)
Title: Changes to hooks to fix some problems

Hi all.

This PR is intended more as a starting point for discussion rather than a PR ready to merge. One of the tests is failing at present but I think I need some input to fix it.

While writing a module to support nested hierarchies (discussed here: https://github.com/sequelize/sequelize/issues/1751), I found a few oddities in the way sequelize deal with hooks:

### Model#bulkCreate bug

While doing `model.bulkCreate()` with option `{hooks: true}`, the `beforeCreate` and `afterCreate` hooks were being run twice each. This PR fixes that.

### Model#update bug

When doing `Model#update()` (i.e. bulk update) with option `{hooks: true}`, any changes made to the daos by the beforeUpdate hooks are not reflected in what actually gets written to the DB.

So... I have added code in Model#update to run the beforeUpdate hooks on each dao and take note of any changes they make to the daos. If the changes to all the daos are consistent (i.e. if they alter every dao in the same way), then `attrValueHash` is altered to reflect these changes and a bulk update is run (i.e. a single SQL statement to alter all the rows at once). If the daos are being changed inconsistently, so it's not possible to alter them all with a single SQL statement, a dao is built for each record and then `.save()` run on each.

This is the most efficient way of doing it, though it makes the code a bit complex.

I found the code in `Model#update()` hard to get my head around, so in the process I also re-wrote it to make it more "promisey" with the order of the code more closely following the order of execution. This is just a matter of coding style though - maybe you guys disgree that it's now easier to read?

I also made it a bit more parallel - the `beforeUpdate` and `afterUpdate` hooks and `.save()` calls are all run in parallel using `Promise.map()` rather than in series as it was before. I feel this could be a performance improvement, but I may be missing something.

#### skipHooks option in dao.update()

In order to make this work, I needed to add an option `skipHooks` that can be passed to `dao.update()`. If set to true, it does not run the hooks on the update operation (needed because `Model#update()` is running them already). If this feels like a good idea, perhaps it should be implemented in `Model#create()` etc too.

### Questions

I'm unclear on a couple of things about how the hooks API works:

#### Specifics

* `Model#runHooks()` returns an array and therefore needs `.spread()` rather than `.then()` to deal with the return values, but I see `.then()` is used in various places in the sequelize code. Is this an error in that `.spread()` should be used, or that `runHooks()` should not be returning an array? Or should `runHooks()` sometimes return an array and sometimes not? Maybe different kinds of hooks have different signatures which make `.then()` and `.spread()` more natural to support in different cases?
* The return value of `Model#update()` is inconsistent in my code, depending on whether each dao is saved individually or whether all the records are updated simultaneously. This is what's causing the test to fail on my PR, but I'm not sure what the correct behaviour should be - should it return the records updated as an array of daos or the number of rows affected? Either way, I feel it should be consistent regardless of whether you're running the `model.update()` command with hooks or not.

#### General hooks stuff

What are hook functions meant to return? They can call `fn()` or return a Promise, but do they have to return anything? I mean, can a hook just alter the dao directly and not return the altered dao.

e.g. is this legal?:

Model.hook('beforeCreate', function (dao) {
dao.someField = 123
})

Or does it need to be:

Model.hook('beforeCreate', function (dao) {
dao.someField = 123
return dao
})

If the former is legal, that affects some of the code in `Model#update()`

### Final points

Hope the above makes some sense.

If you're interested in what I'm up to with hierarchy support, have a look at https://github.com/overlookmotel/sequelize-hierarchy