You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Calling next(error) in hook does not bubble error up to initial orm call #301
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();},
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.
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.
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:
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.
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.
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.
Activity
dxg commentedon Aug 18, 2013
From the wiki:
The
nextparameter is not available forafter*hooks.dirkmc commentedon Aug 18, 2013
I think maybe the wiki needs to be updated. This code works for me:
dxg commentedon Aug 19, 2013
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 commentedon Aug 19, 2013
I'll take a look later today.
dresende commentedon Aug 19, 2013
Your problem is you're using
afterLoadwhich is called well... after load. Only usebefore*hooks to stop loading/saving.. Thenextwill really work on those as you expect. Forafter*hooks, although anextis passed to the function, they won't care about it.In general, only
before*hooks can block code. Please note that if you define abefore*hook without thenextparameter it will not block, but if you define it you really have to call it.dirkmc commentedon Aug 19, 2013
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:
If there's an error, I want to inform the client. eg
dxg commentedon Aug 19, 2013
This should be possible for persisted models, however not for new ones (
new Persondoesn't take a callback).And if we did support errors for
afterLoadthen people might also expect this behaviour forafterSave.. 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
nextcallback, but we don't support it returning an error.It should be all or nothing for consistency.
dirkmc commentedon Aug 20, 2013
new Personmight not take a callback, butPerson.createdoes.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
afterLoadbecause 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 commentedon Aug 20, 2013
I made some changes, I'm going to commit them now, just want to add a test case for this.
Adds test for afterLoad hook next(err) (#301)
dresende commentedon Aug 20, 2013
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 commentedon Aug 20, 2013
Ok thanks, I'll give it a try.
dirkmc commentedon Aug 20, 2013
It works for afterAutoFetch but not for afterLoad
dresende commentedon Aug 21, 2013
I actually tested for
afterLoadand it worked. Didn't even think aboutafterAutoFetch, will check it out.Fixes afterAutoFetch next(err) bubling up just like afterLoad (#301)