swagger-api / swagger-node

Swagger module for node.js
http://swagger.io
Apache License 2.0
3.97k stars 585 forks source link

error handling before bagpipes version #312

Open GiladShoham opened 8 years ago

GiladShoham commented 8 years ago

Hi, i want to show my users some custom error in case of validation error. currently swagger show all the call stack which of course i don't want to show to the user. i want some mapping mw to convert the call stack / error to some clear json. i speak on the version before the bagpipes. any help will be welcome. thx.

theganyo commented 8 years ago

If you are using one of the connect-based frameworks (connect, express, etc.), you would configure it to not use the json_error_handler and write your own connect middleware to handle the errors.

tanzim commented 8 years ago

@GiladShoham you can do something like the following to use your own error handling using a middleware. The code below leverages [1] and [2] from express and you can always fork them to meet requirements you may have.

// Server
var errorHandler = require('api-error-handler');
var successHandler = require('api-success-handler');
...
app.use(successHandler);
app.use(errorHandler);
...
// Controller
// Error and Success response classes create objects representing either an error or a success
var errorResponse = require('http-errors');
var successResponse = require('http-success');
...
function petPost(req, res, next) {
  var pet = req.swagger.params.body.value;
  petService.create(pet, function onCreatePet(error, result) {
    if (error) return next(errorResponse(500, error.message));
    return next(successResponse(201, result, new Date()));
  });
}

[1] https://github.com/expressjs/api-error-handler [2] https://github.com/jshttp/http-errors

@theganyo Would it be possible to shed some light on how fittings can be used for success and error handling instead of using connect middlewares like above?

theganyo commented 8 years ago

@tanzim Basically, if you have an error handler that you want to use, you can just register it on the appropriate pipe using the "onError" registration fitting. Once registered, the specified fitting/pipe will be run if an error is passed (like connect: next(err)) or set (context.error = xxx). The error handler should pick check context.error and do any appropriate work based on that error. Otherwise, if you have a handler that you always want to run last (non-error), you can just list it last on your pipe definition. It will receive a context object that has an "input" attribute that came from the previous fitting you can work with. (Alternatively, it also has request and response attributes that you can work with directly if necessary.) Please let me know if you have a more specific question or need a specific example and I'll do my best to clarify for you.

GiladShoham commented 8 years ago

Hi, Thx for your help. for now i just added this.jsonErrorHandler() to the middlewares array right after this.validator() so it looks like this:

this.stack = function stack(includeErrorHandler) {

    includeErrorHandler = !!includeErrorHandler;

    var middlewares = [
      this.metadata(),
      this.security(),
      this.sysConfig.authenticationMiddleware,
      this.sysConfig.permissionMiddleware,
      this.validator(),
      this.jsonErrorHandler(),
      this.sysConfig.parametersSeparationMiddleware,
      this.expressCompatibilityMW(),
      this.router()
    ];

    if (this.sysConfig.corsOptions) {
      middlewares.unshift(this.cors());
    }

    if (includeErrorHandler && this.sysConfig.mapErrorsToJson) {
      middlewares.unshift(this.jsonErrorHandler());
    }

    if (this.sysConfig.docEndpoints && this.sysConfig.docEndpoints.raw) {
      middlewares.push(this.swaggerDoc());
    }

    return middlewares;
  };

But there is still few thing i don't understand. It looks like it's possible to configure it from out side with this condition

if (includeErrorHandler && this.sysConfig.mapErrorsToJson) {
      middlewares.unshift(this.jsonErrorHandler());
    }

so

  1. why it put this mw to be the first in the mw array, if it will be first it will run before the validator and do nothing.
  2. how can i change the includeErrorHandler param from outside? this stack function called from this code:
this.register = function register(app) {
    this.stack().forEach(function(mw) { app.use(mw); });
  };

so it will always be undefined. why isn't the condition is just like this (to check only this.sysConfig.mapErrorsToJson which i can change from outside)?:

if (this.sysConfig.mapErrorsToJson) {
      middlewares.unshift(this.jsonErrorHandler());
    }
tanzim commented 8 years ago

@theganyo Thanks for the tips. Using my own fittings for error and response handling worked like a charm.

I had to slightly change how I sent success objects to next though:

next(null, successResponse);

as opposed to

next(successresponse);

That's because https://github.com/apigee-127/bagpipes/blob/master/lib/bagpipes.js#L173 expects the first parameter to be an error always, which is fair enough. Previously with my middleware approach, I was inspecting the first parameter and determining if it was an error or not and then invoking the appropriate action.

theganyo commented 8 years ago

@tanzim You are correct. Sorry about the typo.

@GiladShoham Good questions. You are supposed to be able to configure the error handler by just adding "mapErrorsToJson: true" to your config file under the swagger section. If that's not working, it's a bug. However, it looks like maybe you aren't on the most recent non-bagpipes based release. Can you ensure you're on swagger-node-runner@0.4.6?

GiladShoham commented 8 years ago

Hi, Im on version 0.4.5 and i saw you did change to this on v0.4.6 here: https://github.com/theganyo/swagger-node-runner/commit/90446e74a6fe6d19c8ff033a2f6ba46ce1b0dd3b but it looks like even with this changes it won't work because there is still no way to change the includeErrorHandler and it always have a false value