Calling next(error) in hook does not bubble error up to initial orm call

[dirkmc]

eg

afterLoad: function(next) {
    return next(new Error("after load error"));
},

// ...

db.Post.get(1, function(err, data) { console.log(err); // null

[dxg]

From the wiki:

For all before* hooks, you can add an additional parameter to the hook function.

The next parameter is not available for after* hooks.

[dirkmc]

I think maybe the wiki needs to be updated. This code works for me:

afterLoad: function(next) {
    console.log(next); // [Function]
    // If I include this code then it (correctly) stops executing other hooks
    //return next(new Error("after load error"));
    next();
},

[dxg]

Ahh indeed the feature was added here.

I had a play with the code and tried to make it work. Whilst I did get a test case with your use case passing, it broke other things inexplicably.

I was going to atleast put up the test case, but it also broke mocha unexpectedly so... I'll let someone braver work on this one. It's jinxed and doesn't smell right.

[dresende]

I'll take a look later today.

[dresende]

Your problem is you're using afterLoad which is called well... after load. Only use before* hooks to stop loading/saving.. The next will really work on those as you expect. For after* hooks, although a next is passed to the function, they won't care about it.

In general, only before* hooks can block code. Please note that if you define a before* hook without the next parameter it will not block, but if you define it you really have to call it.

[dirkmc]

I found that the same thing happened in afterLoad - if I define next but don't call it, it hangs (which is what I would expect). The use case for me is that I want to load some other stuff in afterAutoFetch(). For example I load a Channel object and auto-fetch the Users in that channel. In afterAutoFetch I set a user_ids property, which is a list of each user in the channel's id. When everything completes I convert select fields to json. eg:

GET /channel/7
{
  name: 'MyChannel',
  user_ids: [1,6,3,7]
}

If there's an error, I want to inform the client. eg

db.Channel.get(7, function(err, channel) {
  if(err) { return res.json({error: 'Could not load channel'}) }
  res.json({name: channel.name, user_ids: channel.user_ids});
})

[dxg]

This should be possible for persisted models, however not for new ones (new Person doesn't take a callback).
And if we did support errors for afterLoad then people might also expect this behaviour for afterSave.. however there it wouldn't make sense because you've already saved and then you raise an error.. leaving the model in an unexpected state.

It's a bit strange that we support passing a next callback, but we don't support it returning an error.
It should be all or nothing for consistency.

[dirkmc]

new Person might not take a callback, but Person.create does.

I can see what you're saying about not wanting to leave the model in an inconsistent state. On the other hand I feel that there needs to be some way for the caller to know if there was an error in afterLoad because he's expecting the model to have a user_ids array as in my example above, and if there's no error but that field is missing it would also be an inconsistent state. It seems like this kind of problem is what transactions were designed to overcome.

[dresende]

I made some changes, I'm going to commit them now, just want to add a test case for this.

[dresende]

Please test this latest commit. Please remember that in case of an error, the instance was already saved on database so you might want to do something about it.

[dirkmc]

Ok thanks, I'll give it a try.

[dirkmc]

It works for afterAutoFetch but not for afterLoad

[dresende]

I actually tested for afterLoad and it worked. Didn't even think about afterAutoFetch, will check it out.