FoalTS / foal

Full-featured Node.js framework, with no complexity. 🚀 Simple and easy to use, TypeScript-based and well-documented.
https://foalts.org/
MIT License
1.89k stars 139 forks source link

Easy way to shape validation errors from API? #737

Closed jondot closed 3 years ago

jondot commented 4 years ago

Hi, I'm looking at validation errors, when using a validation hook/decorator. They mostly look like this:

  "body": Array [
    Object {
      "dataPath": "",
      "keyword": "required",
      "message": "should have required property 'email'",
      "params": Object {
        "missingProperty": "email",
      },
      "schemaPath": "#/required",
    },
  ],

Thing is, the body response shape is now rooted as an array. A few problems:

In short what I'd like to have is a generic FoalHttpMessage:

{
  content:
  error?:
  code?:
}

Or something along these lines.

And lastly, if not possible as a framework-driven opinion, the ability to override and set this shape myself. Right now it seems that to get this I can't use the validation hook (I need to build this shape myself and run validation manually)

To suggest a fix, specifically, change 'body' to 'errors' here: https://github.com/FoalTS/foal/blob/ad5646bd37398900c58dec76e74c5d6464164a60/packages/core/src/common/hooks/validate-body.hook.ts#L23

LoicPoullain commented 4 years ago
  • Some languages/technologies/http clients don't expect arrays to be the body shape (they are wrong in this, but nevertheless it's a kind of de-facto standard to have a JSON body start off as a dict shape)

The framework does return a JSON object and not an array. The body name might be confusing in this case. The @ValidateBody hook returns a response which body has this shape: { body: [ ... ] }. The body key indicates that the validation failed on the request body. In the same way, response bodies returned by @ValidateHeader have this shape { headers: [ ... ] }.

And lastly, if not possible as a framework-driven opinion, the ability to override and set this shape myself. Right now it seems that to get this I can't use the validation hook (I need to build this shape myself and run validation manually)

It is possible to override this behavior by using a post-hook function and so "reshape" the errors returned by the validation hook.

Maybe something like that:

import { MergeHooks, ValidateBody as InitialValidateBody, Hook, isHttpResponseBadRequest } from '@foal/core';

export function ValidateBody(schema: any): Hook {
  return MergeHooks(
    Hook(() => response => {
       if (isHttpResponseBadRequest(response) && response.body.hasOwnProperty('body')) {
           response.body = { error: response.body.body[0] };
       }
    })
    InitialValidateBody(schema),
  )
}

Does it help?

jondot commented 4 years ago

Yup That's what I did. And I believe that unless you're outputting a status code that says "this is strictly a validation error" (or maybe in the headers) it will be a bit difficult for frontends to discover this class of errors.

What I eventually did is reshape into these:

// validation error
{ 
  validation_errors: [ .. ]
}
// error
{
  errors: [ .. ]
}

and any kind of content:

{
   person: { .. }
}

So the frontend code is quite easy, and by-convention we don't really rely on HTTP status codes for kinds of error semantics (people make lots of mistakes there).

LoicPoullain commented 3 years ago

Thank you for the details.

I think it might be a matter of taste here and unfortunately we won't be able to satisfy everyone in this case. I'll keep the current behavior of the validation hooks and leave the liberty to users to override them with a post-hook function or a completely separate new custom validation hook.