Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ValidationPipe with custom error #1267

Closed
lazarljubenovic opened this issue Nov 6, 2018 · 9 comments
Closed

ValidationPipe with custom error #1267

lazarljubenovic opened this issue Nov 6, 2018 · 9 comments

Comments

@lazarljubenovic
Copy link

I'm submitting a...


[ ] Regression 
[ ] Bug report
[x] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead post your question on Stack Overflow.

Current behavior

A validation error returns a response with status code 400.

Expected behavior

The ValidationPipe is configurable and accepts a function used to create an error instance which will be thrown.

Minimal reproduction of the problem with instructions

async function bootstrap () {
  const app = await NestFactory.create(AppModule)
  app.useGlobalPipes(new ValidationPipe())
  await app.listen(3000)
}

What is the motivation / use case for changing the behavior?

Per-field validation errors are, in my opinion, a 422, not a 400. I'd also suggest changing this framework-wide as a default, but that might be a breaking change that people might not welcome with open arms.

Environment

"@nestjs/common": "^5.1.0",
"@nestjs/core": "^5.1.0",
"@nestjs/typeorm": "^5.2.2",
@kamilmysliwiec
Copy link
Member

kamilmysliwiec commented Nov 7, 2018

What about (2 ways):

  1. extending ValidationPipe and rethrowing different error?
  2. creating a filter that transforms BadRequestException (when ValidationError) to an instance expected by your application?

@lazarljubenovic
Copy link
Author

lazarljubenovic commented Nov 7, 2018

Thanks for the response. Here are my attempts at both approaches for reference.

Extend ValidationPipe

export class ValidationPipe422 extends ValidationPipe {
  public async transform (value, metadata: ArgumentMetadata) {
    try {
      return await super.transform(value, metadata)
    } catch (e) {
      if (e instanceof BadRequestException) {
        throw new UnprocessableEntityException(e.message.message)
      }
    }
  }
}

async function bootstrap () {
  const app = await NestFactory.create(AppModule)
  app.useGlobalPipes(new ValidationPipe422())
}

A small trick: you need return await here (although usually we can just write return since async makes it return a Promise anyway). Without await, the function simply returns a (pending) promise properly and thus nothing is caught. With await, the function stalls until the promise is rejected, the rejection is caught and the catch block executes.

With an exception filter

I'm not sure how to "properly" do the (when ValidationError) bit from your response. 🤔 I've got the following, which works, but now it will transform every 400 into a 422.

@Catch(BadRequestException)
export class ValidationExceptionFilter implements ExceptionFilter<BadRequestException> {
  public catch (exception, host: ArgumentsHost) {
    const ctx = host.switchToHttp()
    const response = ctx.getResponse() as express.Response
    response
      .status(422)
      .json({
        statusCode: 422,
        error: `Unprocessable Entity`,
        message: exception.message.message,
      })
  }
}

async function bootstrap () {
  const app = await NestFactory.create(AppModule)
  app.useGlobalPipes(new ValidationPipe())
  app.useGlobalFilters(new ValidationExceptionFilter())
}

One obvious way to fix this is to inspect the expection.message.message and see if it quacks like a "400 but I want a 422" error, but I really don't like that one.

@lazarljubenovic
Copy link
Author

As for my initial "feature request": I propose exceptionConstructor where could just pass in an UnprocessableEntityException, and then the ValidationPipe would user whatever I pass it in:

throw new BadRequestException(
this.isDetailedOutputDisabled ? undefined : errors,
);

It would default to BadRequest which would make it a non-breaking change.

Then usage would be just this:

async function bootstrap () {
  const app = await NestFactory.create(AppModule)
  app.useGlobalPipes(new ValidationPipe({
    exceptionConstructor: UnprocessableEntityException,
  }))
  await app.listen(3000)
}

@kamilmysliwiec
Copy link
Member

Since 5.5.0 you can override exception's factory:

new ValidationPipe({
  exceptionFactory: (errors: ValidationError[]) => 
    new BadRequestException('Validation error'),
});

@jbjhjm
Copy link

jbjhjm commented Mar 28, 2019

Since 5.5.0 you can override exception's factory:

new ValidationPipe({
  exceptionFactory: (errors: ValidationError[]) => 
    new BadRequestException('Validation error'),
});

There's one tiny bit missing that drove me mad... this code would end in throwing "undefined".
Which would lead to an error which can be debugged hardly.
Make sure to use

exceptionFactory: (errors: ValidationError[]) => RETURN new BadRequestException('Validation error'),

... then everything will be fine :)

@lazarljubenovic
Copy link
Author

@jbjhjm It shouldn't, unless you added { after =>.

@jbjhjm
Copy link

jbjhjm commented Mar 31, 2019

@jbjhjm It shouldn't, unless you added { after =>.

Sometimes I feel stupid. Of course. Didn't notice the brackets were skipped. Thanks for clearing up!

@evilive3000
Copy link

evilive3000 commented Apr 5, 2019

What about passing isDetailedOutputDisabled flag into exceptionFactory, otherwise it's unclear to call this.isDetailedOutputDisabled in the custom exceptionFactory, we do know nothing about context without digging into the source code.

@lock
Copy link

lock bot commented Sep 23, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Sep 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants