samchon / nestia

NestJS Helper Libraries + TypeScript OpenAPI generator
https://nestia.io/
MIT License
1.85k stars 95 forks source link

ExceptionFilter returned different response #353

Closed mlazuardy closed 1 year ago

mlazuardy commented 1 year ago

Bug Report

here is the min reproduce repo

https://github.com/mlazuardy/nestjs-starter

npm install
npm run start:dev

post to /

im trying to check the response by only sending empty object {} using POSTMAN

it return the expected result like this

{
    "errors": [
        {
            "path": "$input.firstName",
            "expected": "string"
        },
        {
            "path": "$input.lastName",
            "expected": "string"
        }
    ],
    "message": "Request body data is not following the promised type."
}

however, because usually we need to beautify the response so im trying to use nestjs exception filter to modify the response

you can check common.exception-filter.ts,it returned different response

exception filter called { path: '$input', reason: 'Error on typia.assertStringify(): invalid type on $input, expect to be TestInput', expected: 'TestInput', value: undefined, message: 'Response body data is not following the promised type.' }

samchon commented 1 year ago

You're meaning that ExceptionFilter has been called, but error message is not changed, right?

mlazuardy commented 1 year ago

@samchon yes correct, should i change the title to ExceptionFilter does not changed ?

samchon commented 1 year ago

Looking at NestJS guide documents, when creating exception filter, you've send response by yourself.

It's the reason why succeeded to capture the exception, but failed to response as you want.

https://docs.nestjs.com/exception-filters#exception-filters-1

image

samchon commented 1 year ago

By the way, nestia still needs to handle the exception filter.

mlazuardy commented 1 year ago

@samchon if we set the response using response context like this

import { ArgumentsHost, Catch, ExceptionFilter, HttpException } from "@nestjs/common";
import { Response } from "express";

@Catch(HttpException)
export class CommonExceptionFilter implements ExceptionFilter {
  catch(exception: HttpException, host: ArgumentsHost) {
    console.log("exception filter called", exception.getResponse());
    const res = host.switchToHttp().getResponse<Response>();
    const status = exception.getStatus();
    res.status(status).json({
      statusCode: status,
      errors: exception.getResponse(),
      message: "Common exception message"
    })
  }
}

it return another error

Error: Cannot set headers after they are sent to the client

if im googled the error it said that send or json method from response already send to client

mlazuardy commented 1 year ago

@samchon

i wonder if this is the caused of the errors as you directly use send inside the TypedBody decorator https://github.com/samchon/nestia/blob/master/packages/core/src/decorators/internal/send_bad_request.ts#L5

maybe is it better to just throw an exception instead ?

samchon commented 1 year ago

If throw statement be used, it becomes InternalServerError in NestJS.

In my old memory, throwing exception was possible, but someday it became InternalServerError.

I need to ask to NestJS core team developer. May require few days.

mlazuardy commented 1 year ago

@samchon i believe so, and i think the way nestjs class-validator validate the body is through the pipe instead directly from the Body decorator

samchon commented 1 year ago

I found the solution.

The reason why NestJS could not catch HttpException instance from custom decorator was, NestJS is using instanceof statement, and as dependency relationship of NestJS is complicated twisted in, instanceof statement sometimes be failed when one NestJS project imports another NestJS project.

  1. Project A imports B
  2. B implemented custom decorator
  3. A and B, both of them installed NestJS, therefore instanceof statement from A be failed

In nestia case, many test automation programs are compsed as each project, and as their instanceof statement be failed, I just misunderstood that NestJS stopped catching exception from the custom decorator. To fix this bug, I'll roll back custom decorators to throw exception, and resolve dependency relationship.


Also, this issue need to be reported to the NestJS core team members - @micalevisk and @jmcdo29. I think in the HttpException case, you should implement another type checking logic instanceof statement. When a NestJS project another NestJS project, such bugs can be enoughly occured.

I urgently researched such error case, and found same situation. There's a shopping mall backend project in my company, and it is importing another payment server for composing Test Automation Program. In that case, it also occurs same instanceof statement error. Everyone have not known that issue, due to exception filtering on custom decorator is very rare case.

samchon commented 1 year ago

@mlazuardy Upgrade @nestia/core to 1.2.4, then be fixed.

By the way, as NestJS does not exception filtering in interceptor level, type error from TypedRoute.Get() cannot be filtered.

Except the TypedRoute case, every exceptions would be filtered.

mlazuardy commented 1 year ago

@samchon okay thanks! Also i found an issue too on TypedRoute.Post but need to do more test as im testing the decorator on my full project

mlazuardy commented 1 year ago

@samchon confirmed that now i can use ExceptionFilter and it returned expected validation errors, thanks!