Skip to content

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

Closed

Description

@dirkmc

eg

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

// ...

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

Activity

  1. dxg commented on Aug 18, 2013

    @dxg
    Collaborator

    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.

  2. dirkmc commented on Aug 18, 2013

    @dirkmc
    Author

    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();
    },
  3. dxg commented on Aug 19, 2013

    @dxg
    Collaborator

    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.

  4. dresende commented on Aug 19, 2013

    @dresende
    Owner

    I'll take a look later today.

  5. dresende commented on Aug 19, 2013

    @dresende
    Owner

    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.

  6. dirkmc commented on Aug 19, 2013

    @dirkmc
    Author

    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});
    })
  7. dxg commented on Aug 19, 2013

    @dxg
    Collaborator

    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.

  8. dirkmc commented on Aug 20, 2013

    @dirkmc
    Author

    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.

  9. dresende commented on Aug 20, 2013

    @dresende
    Owner

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

  10. dresende commented on Aug 20, 2013

    @dresende
    Owner

    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.

  11. dirkmc commented on Aug 20, 2013

    @dirkmc
    Author

    Ok thanks, I'll give it a try.

  12. dirkmc commented on Aug 20, 2013

    @dirkmc
    Author

    It works for afterAutoFetch but not for afterLoad

  13. dresende commented on Aug 21, 2013

    @dresende
    Owner

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions