dchester / epilogue

Create flexible REST endpoints and controllers from Sequelize models in your Express app
846 stars 116 forks source link

Having hooks enabling Sequelize errors management #157

Open Mumeii opened 8 years ago

Mumeii commented 8 years ago

As it's a big part of the delicate art of tuning a REST API, it would be cool to be able to hook on error return.

Maybe I've missed a key feature, but so far I didn't found how to do it easily.

For example here is one of my current use cases :

  1. A delete request is forbidden at database level due to a foreign key constraint.
  2. The corresponding Sequelize error is caught by Epilogue and turned into an EpilogueError (internal server error)
  3. The client receive an error I didn't had the opportunity to tune the way I want.

I'd like to send a 400 answer instead and change the error message to have it more developer friendly.

Thus so far, as a workaround, I've hooked on the error method of the Controller class in base.js, but I really don't feel like it's the right thing to do ;)

Notice : I'd be glade to give an hand tuning this feature if it's not incredibly time consuming and/or complex ( I'm a newcomer to JS / Sequelize / Node ... etc )

mikemimik commented 8 years ago

I'm pretty sure you can handle this by hooking into one of the milestone after hooks. Depending on when the error happens you could hook all the after milestones to a single function to simple check for errors in the context. I haven't used Epilogue in awhile though I will be diving back into it for the next month.

@mbroadst might have a better answer for you but what you're asking about, you should already be able to handle within Epilogue.

Mumeii commented 8 years ago

@mikemimik : Well, I've been debugging down to the hook chain and, from what I've seen before posting this [issue / feature request], I guess it all boils down to the following code in base.js file (version 0.6.6) :

  hookChain
    .catch(errors.RequestCompleted, _.noop)
    .catch(self.model.sequelize.ValidationError, function(err) {
      var errorList = _.reduce(err.errors, function(result, error) {
        result.push({ field: error.path, message: error.message });
        return result;
      }, []);

      self.error(req, res, new errors.BadRequestError(err.message, errorList, err));
    })
    .catch(errors.EpilogueError, function(err) {
      self.error(req, res, err);
    })
    .catch(function(err) {
      self.error(req, res, new errors.EpilogueError(500, 'internal error', [err.message], err));
    });

As you can see all exceptions but RequestCompleted end into the error function I've been talking about without mentioning any milestones of any kind.

It seems logical as the previous code extract is located outside of the hook processing workflow :( :

Controller.milestones.forEach(function(milestone) { ... }
mbroadst commented 8 years ago

@Mumeii you're correct that they end up in the error function, but you'll also note that there are a number of ways to customize this process:

I think things are pretty flexible here, but by all means please do play around and see if you can come up with something better/more logical. Contributions are gladly accepted (as I'm no longer actively developing on this project, but rather simply maintaining it).

HTH

matmar10 commented 7 years ago

@Mumeii Did you make any progress on this? Error handling is really the only thing I haven't got properly sorted/figured out and I find it's a consistent source of pain/confusion with the default being 500 server error.

What did you end up doing, @Mumeii ?

I'm happy to contribute some docs on this if you can give me a crash course in what you did.

Mumeii commented 7 years ago

Hello Matthew

Unfortunately I didn't had time to spend on this subject since I reported this enhancement request on github.

I guess you've seen on the discussion thread the answer of the creator of this project, and, as he said more than a year ago, he is simply maintaining it.

I've been using epilogue for a rest API proof of concept based on a legacy database that we've wrapped in sequelize. But after a month struggling with epilogue, it's been clear to us that it was to complex to use without a real community behind this library involved in its debugging and enhancement.

Plus, as far as I remember, the code is hard to read because it's kneeting the process logic in such a complexe way with promises everywhere.

I don't know if a design pattern could simplify all this part of the code, but it would be great. Maybe using rx ?

Anyway, I doubt I'll have another look at this library for a while.

I hope you'll find a way or another to sort it out.

Regards

Benoit

Le 24 août 2017 7:11 PM, "Matthew Joseph Martin" notifications@github.com a écrit :

@Mumeii https://github.com/mumeii Did you make any progress on this? Error handling is really the only thing I haven't got properly sorted/figured out and I find it's a consistent source of pain/confusion with the default being 500 server error.

What did you end up doing, @Mumeii https://github.com/mumeii ?

I'm happy to contribute some docs on this if you can give me a crash course in what you did.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/dchester/epilogue/issues/157#issuecomment-324698090, or mute the thread https://github.com/notifications/unsubscribe-auth/AQqxL7bOx1jlRQ8SDe77OTs8zYX6Saq8ks5sba6qgaJpZM4IO1ip .