Swagger: Feature Request: add @ApiException() decorator

[ruscon]

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

no decorator to automate swagger schema generation for HttpException

Expected behavior

Minimal reproduction of the problem with instructions

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

Environment

Nest version: 7.x
Nest swagger: 4.x

Example of implementation

import { HttpException, Type } from '@nestjs/common';
import { ApiResponse, ApiResponseOptions, getSchemaPath } from '@nestjs/swagger';
import { getTypeIsArrayTuple } from '@nestjs/swagger/dist/decorators/helpers';
import { ResponseObject, SchemaObject } from '@nestjs/swagger/dist/interfaces/open-api-spec.interface';

export type Func = () => void;

export interface ApiExceptionOptions extends Omit<ResponseObject, 'description'> {
    description?: string;
    type?: Func | [Func];
    isArray?: boolean;
    schema?: SchemaObject;
}

export const ApiException = <T extends HttpException>(exception: () => Type<T>, options: ApiExceptionOptions = {}): MethodDecorator & ClassDecorator => {
    const instance = new (exception())();

    const apiResponseOptions: ApiResponseOptions = {
        status: instance.getStatus(),
        description: options?.description || instance.message,
        schema: {
            type: 'object',
            properties: {
                statusCode: {
                    type: 'number',
                    example: instance.getStatus(),
                },
                message: {
                    type: 'string',
                },
                error: {
                    type: 'string',
                    example: instance.message,
                },
            },
            required: ['statusCode', 'message'],
        },
    };

    if (options.schema) {
        apiResponseOptions.schema.properties!.message = options.schema;
    } else if (options.type) {
        const [type, isArray] = getTypeIsArrayTuple(options.type, !!options.isArray);
       // For standard data types (Object, String, etc), an error will be shown on the swagger api page, I don't know how to get around it
        apiResponseOptions.schema.properties!.message['$ref'] = getSchemaPath(type!());

        if (isArray) {
            apiResponseOptions.schema.properties!.message = {
                type: 'array',
                items: {
                    type: 'object',
                    $ref: apiResponseOptions.schema.properties!.message['$ref'],
                },
            };
        }
    }

    return ApiResponse(apiResponseOptions);
};

examples

@ApiException(() => NotFoundException)
@ApiException(() => NotFoundException, { description: 'Custom description' })
@ApiException(() => NotFoundException, { description: 'Custom description', schema: { /** custom schema for message prop */} }
@ApiException(() => NotFoundException, { description: 'Custom description', type: () => SomeClassWithSwaggerAnnotitions, isArray: true }

[nartc]

What’s wrong with the followings?

ApiNotFoundResponse()
ApiNotFoundResponse({ description: “”, schema: ... })
ApiNotFoundResponse({ type: () => SomeExceptionType, isArray: true })

[ruscon]

@nartc
example of difference

export class SomeSto {
    @ApiPropertyOptional({ type: () => Number, minimum: 0, default: 0 })
    prop: number;
}


@ApiException(() => ForbiddenException, { type: () => SomeSto, isArray: true })
@ApiNotFoundResponse({ type: () => SomeSto, isArray: true })

result:

[nartc]

Would you really need the statusCode and the error to be part of the response? They seem a little redundant to me.

[ruscon]

This is the standard nestjs response for HttpException

[nartc]

@ruscon ahh I see what you're saying now. Still, a new decorator for exception responses would be confusing. Now we have two different ways of decorate responses: ApiResponse and ApiException. Removing the Api***Response like (ApiBadRequestResponse) would cause breaking changes. Hence, I would propose two approaches:

1. nestjs/swagger will expose a DefaultException class to act as the response schema for these Exception responses.
2. nestjs/swagger will default the type of Api***Response to have a DefaultException

export class DefaultException {
   statusCode: HttpStatus;
   message: any;
   error: string;
}

Although, I'd recommend the first approach to defer to the consumers whether or not they want to decorate their controller's exceptions to have the DefaultException. One reason is the consumers DO NOT necessarily need to throw new HttpException(), they can use the Response object from express and return an exception to the client like res.status(HttpStatus.BAD_REQUEST).json({...}) or res.status(HttpStatus.BAD_REQUEST).end()

What do you think?

[ruscon]

Perhaps I did not fully understand your proposal...
If it's not difficult for you, show examples of the code and what swagger will show.

[nartc]

Sure. Let me try my best to give you my thoughts as well as explaining my proposals.

First of all, let's talk about your proposal. You propose that nestjs/swagger exposes a new decorator called ApiException() which:

• Add corresponding response to the responses portion of a path in OpenAPI Specification file (aka swagger.json). (I marked this as (1) to reference later in this comment).
• Each response added by ApiException() would have the schema that mimics HttpException's properties

{
   statusCode: ...,
   message: ...,
   error: ...
}

• ApiException() also allows the consumers to pass in a type to set the schema of message property.

While the intention (and potentially the benefit) of your proposal is good and helpful, I don't think it's necessary to have a new decorator ApiException. Here's why:

• One reason is he consumers DO NOT necessarily need to throw new HttpException(), they can use the Response object from express and return an exception to the client like res.status(HttpStatus.BAD_REQUEST).json({...}) or res.status(HttpStatus.BAD_REQUEST).end() so I don't think "dictating/defaulting" the schema as HttpException is good. Hence, we should let the consumers define their own exception type.
• nesjts/swagger already has ApiResponse and Api***Response (where * is a status like BadRequest or NotFound etc...) that do exactly what **(1) does. With ApiException() available, we would have two different ways to declare a response for OpenAPI which I think would be confusing.

With that said, I quote my previous comment below

@ruscon ahh I see what you're saying now. Still, a new decorator for exception responses would be confusing. Now we have two different ways of decorate responses: ApiResponse and ApiException. Removing the Api***Response like (ApiBadRequestResponse) would cause breaking changes. Hence, I would propose two approaches:

1. nestjs/swagger will expose a DefaultException class to act as the response schema for these Exception responses.
2. nestjs/swagger will default the type of Api***Response to have a DefaultException

export class DefaultException {
   statusCode: HttpStatus;
   message: any;
   error: string;
}

Although, I'd recommend the first approach to defer to the consumers whether or not they want to decorate their controller's exceptions to have the DefaultException. One reason is the consumers DO NOT necessarily need to throw new HttpException(), they can use the Response object from express and return an exception to the client like res.status(HttpStatus.BAD_REQUEST).json({...}) or res.status(HttpStatus.BAD_REQUEST).end()

What do you think?

Code example would be:

import { DefaultException } from 'nestjs/swagger';

@ApiBadRequestResponse({ description: ..., type: () => DefaultException })

the above would produce:

400: {
   description: ...,
   schema: {
      $ref: '/path/to/DefaultException'
   }
}

[ruscon]

Thanks for the explanation...
I think your version is also good.

The only question is whether it will be part of the nestjs swigger itself or will each developer have to create their own DefaultException class.
I think most programmers do not change the standard response structure.

[jsproede]

Hello :) As @ruscon already mentioned before, we've developed a small @ApiException() decorator module. We're using it mainly together with custom created API exceptions which subclass HttpException. I'd like to explain with a short example, why I think it makes sense, to have such a decorator directly built-in in the Swagger module.

Let's assume we have the following custom exception which subclasses the NotFoundException:

export class UserNotFoundException extends NotFoundException {
  constructor() {
    super('User was not found');
  }
}

If we'd specify this possible response with NestJS swagger built-in decorators, we've to do it like this:

@ApiNotFoundResponse({ description: 'User was not found' })

If we'd use our @ApiException() decorator, it looks a lot cleaner to me and it does not have any duplicate code as the description and HttpException is already defined in the UserNotFoundException:

@ApiException(UserNotFoundException)

[nartc]

@jsproede Let me counter with a question. It seems that you use description as an annotation about what exception it is on the SwaggerUI. What about when you have different exception message for the same HttpStatus?

Assume in a login() endpoint, I can have:

// Validation error
throw new BadRequestException('validation error');

// Email does not match
throw new NotFoundException('email not found');

// Password does not match
throw new BadRequestException('password not match');

How are you going to differentiate between these error messages? My argument is each project has a very different approach to how they'd want to handle the exceptions so nestjs/swagger does not want to dictate any exception's related schema.

Also, to reiterate my point from previous comment

While the intention (and potentially the benefit) of your proposal is good and helpful, I don't think it's necessary to have a new decorator ApiException. Here's why:

• One reason is he consumers DO NOT necessarily need to throw new HttpException(), they can use the Response object from express and return an exception to the client like res.status(HttpStatus.BAD_REQUEST).json({...}) or res.status(HttpStatus.BAD_REQUEST).end() so I don't think "dictating/defaulting" the schema as HttpException is good. Hence, we should let the consumers define their own exception type.
• nesjts/swagger already has ApiResponse and Api***Response (where * is a status like BadRequest or NotFound etc...) that do exactly what **(1) does. With ApiException() available, we would have two different ways to declare a response for OpenAPI which I think would be confusing.

[ruscon]

@nartc I think your solution is better, so we can close the discussion if no one has any other suggestions.

[ruscon]

@nartc Can you please implement your solution?

[nartc]

@ruscon As soon as I have some time this weekend, I'll implement it. I'll leave the issue open to keep track though.

[kamilmysliwiec]

@nartc should I leave this issue open?

[nartc]

@kamilmysliwiec Yes please

[jsproede]

Hi @nartc, sorry that my response took so long. Since your last message to me, I realized that multiple exceptions with the same status code per controller method/route are really an issue. But I've found out, that in Swagger UI you can define multiple examples per status code. Over the last few weeks I extended our @ApiException decorator, to be able to handle multiple exceptions with the same status code.

The newest version of our decorator already implements this feature, as you can see in our decorator README.md file.
There are still some issues with exception grouping (such as handling multiple exceptions with the same exception name), but I'm currently working on a feature that consecutively numbers the exceptions with the same name as separated examples.

I've added some example screenshots and some documentation to the README.md file. I'm hoping that I can finalize the exception-grouping feature before holidays.

What do you think about using examples to show multiple exceptions per status code?

~The grouping feature is already in a feature branch, if you want to take a look at the exception grouping implementation.~

Edit: I've just released a new version of our decorator including the exception grouping feature and added some more examples to the documentation.

[kamilmysliwiec]

There's currently no bandwidth to fix this issue/implement this feature, unfortunately. Since we don't plan to address this in the foreseeable future, I'm gonna close this issue. If anyone is looking for such a feature, consider using the community-written library mentioned in the messages above.