Return 400 Bad Request on request validation error

[apigee-127]

Return 400 Bad Request on request validation error

[whitlockjc]

As mentioned in the StackOverflow thread, I think swagger-validator is working as designed but I do think I have a solution which will help you and still work within Node.js best practices. I will make a proposable below but before I do, let me explain the reasoning behind this.

Best practices for Connect/Express/... middleware is to pass errors downstream whenever there is a problem. By doing this, you allow the application author full control over how the error is handled. I realize this can be a bummer but having swagger-validator automatically generate responses is not a foolproof plan. For example, should the response body be plain text, JSON, HTML or something else? If it's a structured responses, what should the structure of said error be? Express and others have gotten away with not having to answer these questions by requiring application authors to write error handlers. (Here is an example: https://github.com/expressjs/errorhandler)

What I can do is just set the status code to be 400 before sending the error downstream and when there is no error handler, at least you'll get a 400 instead of a 500. Of course, if you want to structure the response beyond that (headers, response content, ...) you'll need to use an error handler as suggested above. Thoughts?

[hjpbarcelos]

Let me put the problem I'm facing.

Some of my requests have a very specific schema for the request body.

For example, my authentication request has a very customized structure that cannot be configured directly in out-of-the-box provider, such as volos-oauth. This is the configuration I'm providing:

  AuthenticationRequest:
    required:
      - code
      - grant_type
      - client_id
    properties:
      code:
        type: string
      grant_type:
        type: string
      client_id:
        type: string
      redirect_uri:
        type: string

If the incoming request misses some of the required parameters, a 500 internal server error will be returned, but this error is too generic.

IMHO, when a request schema is not fulfilled, it means that we have a malformed request, not an internal server error. Therefore, for most cases, we should only return a 400 Bad Request, like saying to the client:

Look, you've screwed up something, don't do it again.

This is exactly what is stated at the RFC I've posted:

The 400 (Bad Request) status code indicates that the server cannot or
will not process the request due to something that is perceived to be
a client error (e.g., malformed request syntax, invalid request
message framing, or deceptive request routing).

I think that form most of the cases, this will be enough, since the APIs should be very well defined and documented. If the client is doing something wrong, it is its responsibility, not ours. Since 5xx errors indicate server faults and 4xx error indicate client faults, I think it is more logical to return a 400 Bad Request response whenever a request validation fails.

Keep in mind that this is only applicable for the request validation.

For those cases you've mentioned that need to provide a feedback, of course we will still need a customized middleware.

[whitlockjc]

I already understand the issue and I've proposed a fix that will solve your problem. When I say swagger-validator is working as designed, I mean based on Node.js middleware practices. With Node.js middleware, when an error is encountered it is passed down the middleware chain which is what is happening now. If there is no other middleware to handle the error, the default error handler is invoked which will indicate a 500 Internal Server Error.

In your case, or any other Node.js API that is returning an error for a validation issue, of course a 500 is not ideal because a 400 is the appropriate status code to indicate a malformed request, like an invalid request parameter. Node.js middleware best practices suggests that you as the API author would need to use, or write your own, error handling middleware to remedy this.

As proposed above, I am saying that instead of requiring you to use/write middleware to set the proper status code to 400 for validation errors, I will instead set it for you and you will only need to use/write middleware if you want to change or add to the response in any other way.

This is the third time I've explained this so I hope it's more clear now. I am not disagreeing with you nor do I misunderstand the situation/problem.

[hjpbarcelos]

I'm sorry if I'm not understanding everything you are saying, maybe it's because english is not my first language and I'm a little out of practice. Re-reading carefully your previous comment I could realise that your suggestion was exactly what I was thinking:

What I can do is just set the status code to be 400 before sending the error downstream and when there is no error handler, at least you'll get a 400 instead of a 500.

Thank you for your attention to this issue.

[whitlockjc]

Fix incoming, I'll update you when it's done. No sweat on the misunderstanding, that's why I kept trying to explain. :)

[whitlockjc]

Henrique,
I've just pushed the fix for this. Once Travis tells me it built fine, I'll perform a release to get this to you. I'll update this issue when that is complete.

[whitlockjc]

[email protected] has been released with this fix in it. From now on, any validation error will be sent downstream with a 400 status code.

[hjpbarcelos]

Thank you very much for your effort on this issue.
Regards.

[whitlockjc]

Anytime. If/When you find new bugs or have feedback/ideas, don't hesitate to file an issue.

[AhsanNissar]

can anybody help me with this error?