koajs / joi-router

Configurable, input and output validated routing for koa
MIT License
450 stars 96 forks source link

trigger output validation only for response status code between 200 a… #8

Closed martinmicunda closed 8 years ago

martinmicunda commented 9 years ago

This pull fix the problem when I want to validate output respond body but there is 404, 409 or some other errors so in this case we should not validate respond and pass error to the user or some other middleware.

In current code when I return error 400 e.g. from my database the koa-joi-router try validate output however as this is 404 error it will throw error because respond body is invalid and it set response status to 500 and this incorrect behaviour instead it should not validate respond status and let pass error to parent or directly to user to notify that resource was not found.

martinmicunda commented 9 years ago

Could you also publish these changes to npm? Thanks

martinmicunda commented 9 years ago

@aheckmann is it possible you could merge this pull request and publish to npm? Thanks

aheckmann commented 9 years ago

Thanks for opening this issue and getting this started. It's not immediately obvious to me what the best solution is in this case, so allow me to outline potential scenarios people may have:

  1. Today: if output validation is defined, always validate with the same rules regardless of status code or anything else
  2. This PR: only run output validation if defined and status is >= 200 < 300
  3. Validate output depending on other status code patterns (pattern A if status 200, pattern B if status 201, pattern C if 400-499, pattern D if > 499)
  4. Only run output validation if defined AND a new context property validateOutput is set to true. validateOutput would be set to true by default (for backwards compatibility) but be easily overridden in our routes based on any logic we needed, thereby supporting every possible scenario.

If we make a change, seems like option 4 would be wisest (and simplest) so we don't paint ourselves into another corner.

Thoughts?

martinmicunda commented 9 years ago

@aheckmann hmm not sure which of these solutions would be the best...

  1. I think the current implementation is wrong and as I mentioned early if there is an internal error e.g. 50x, 40x and validation is turn on then we always passing 400 error which is incorrect and can be confusing to whoever is consuming the API. Another problem might be when I want to implement Error codes API in my application similar to this.
  2. is too complex for validation..
  3. doesn't really fix the problem because when I keep validateOutput set to true then if something failed if always pass 400 error status which I don't want to. If I set validateOutput to false it won't validate output at all however what I actually want is to validate output only for successful respond.

What if we add a new property validateOutputSuccess and if it set to true (by default it will be set to false) we only run validation for successful respond?

aheckmann commented 9 years ago

To restate our needs:

  1. some endpoints need response body validated
  2. some endpoints need response headers validated
  3. response validation rules may be different depending on status code (errors do not always have same rules as success)

I'm going to change the output validation to:

Proposed change:

Change validate.output from a Joi validation object to an object literal, each key being a status code pattern matcher, each value being a plain object (defined below).

Status code pattern matcher proposal:

If a key does not match the above rules, an error will be immediately thrown.

If the resulting rules overlap, an error will be thrown.

Any response status which does not match these rules will not have its output validated.

The value to which these status keys are set will be a plain object with the following optional keys:

The Joi validation object can either be a Joi instance or a plain object which will be passed to Joi.

Here is an example:

router.route({
  method: 'get',
  path: '/change',
  handler: something,
  validate: {
    output: {
      '200-203,205-299': { body: { email: Joi.string(), headers: .. }
      '301,302':  { body: Joi.object(..) }
      '400-499':  { headers: Joi.object(..) }
      '500': { body: Joi.object(..) }
    }
  }
});
martinmicunda commented 9 years ago

Yeah that's sound good.

aheckmann commented 8 years ago

landed in daab62af57