feature(): extract Enum out to its schema

[nartc]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior?

Current behavior is when an Enum is defined and used on a Model, the Enum values are put on the properties of the Model itself.

This causes Frontend Generation Tools (such as NSwag) to generate multiple enums even though they are meant for the same thing.

Issue Number: #372

What is the new behavior?

New behavior will expect: enum property on various decorator (ApiParam, ApiQuery, ApiProperty) to accept a lazy-function syntax.

This allows SwaggerExplorer to grab the Enum name at run-time which makes it possible to extract the Enum out to schemas to be reused on Parameters

Please note this is an addition, not replacement. enum property still accepts what it currently accepts. This PR makes it so it will solve the issue stated in Current Behavior with lazy function syntax.

Various decorators that accept enum now also accept enumName. If you want your enum to be store as a schema (reusable) then you need to provide enumName as well. This implementation might seem a little bit explicit compared to the lazy load function but its intention is also clearer.

// before
@ApiProperty({enum: Role})

// later
@ApiProperty({enum: Role, enumName: 'Role'})

Again, although the PR is working, maintain the current passing tests, passing two new tests, my implementation might contain flaws and might go against NestJS's design mental. That said, I'm open to ALL suggestions and feedbacks.

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

[flogh]

Ok , so I’ve just finished updated my project, which is quite big, I had about 60 enums. Some where in entities, in responses types, in queries, params.

Every single one has correctly been defined under the components section, and reused where needed with the « $ref ».

But I've noticed something undesirable :

• If the enum is defined in the same file as the class :

export enum Direction {
    UP = "Up",
    DOWN = "Down",
    LEFT = "Left",
    RIGHT = "Right",
}

export class Album {
    @ApiProperty({ enum: () => Direction })
    public direction: Direction;
}

It behaves normally :

"components": {
    "schemas": {
        "Direction": {
            "type": "string",
            "enum": [
               "Up",
               "Down",
               "Left",
               "Right"
           ]
        },
         "Album": {
             "type": "object",
             "properties": {
                 "direction": {
                     "$ref": "#/components/schemas/ValidTypes"
                 }
             }
        }
    }
}

• If the enum is imported in the class file :

import { Direction } from "whatever/my-enums";

export class Album {
    @ApiProperty({ enum: () => Direction })
    public direction: Direction;
}

It behaves like this :

"components": {
    "schemas": {
        "my_enums_1.Direction": {
            "type": "string",
            "enum": [
               "Up",
               "Down",
               "Left",
               "Right"
           ]
        },
         "Album": {
             "type": "object",
             "properties": {
                 "direction": {
                     "$ref": "#/components/schemas/my_enums_1.Direction"
                 }
             }
        }
    }
}

The enum name is prefixed by the file's name where it's imported

Would you be able to solve this issue ?

[nartc]

@flogh Thanks for testing. It's super helpful. The unexpected behavior is kind of expected but I wasn't sure. I am able to solve the issue but it's flaky. I'll update the PR but keep in mind that @kamilmysliwiec might have other suggestions. We might actually need to introduce a new property for some decorators instead of rely on lazy function syntax.

[flogh]

Ok, no problem :)

By the way, nice job ! 👍

[nartc]

@flogh getEnumName function has been modified. Please try again if you can.

[flogh]

Ok, working as expected 👏

[kamilmysliwiec]

Perhaps I missed something, but how can I control whether enum should be defined as a component or defined "inline" instead?

[flogh]

As @nartc said, this PR makes enums behave like this :

• Enum defined inline

@ApiProperty({ enum: Role })

• Enum defined under components

@ApiProperty({ enum: () => Role })

He supposed that we might introduce a new decorator like so :

@ApiProperty({ enum: Role, asComponent: true })

But I don't know if it's possible

[nartc]

Perhaps I missed something, but how can I control whether enum should be defined as a component or defined "inline" instead?

@kamilmysliwiec My intention for this PR is to not require users to change anything unless they want to have Enum defined as a Schema to be reusable (per OpenAPI Spec). Currently, we have:

// enum
@ApiProperty({ enum: SomeEnum })

// inline
@ApiProperty({ enum: ['a', 'b', 'c'] })

These two will stay working as expected, no Schema will be created for Enum.

This PR added a lazy-function syntax to enum as follow:

@ApiProperty({ enum: () => SomeEnum })

The lazy-function allows SwaggerExplorer to grab the Enum name at runtime. If the users pass in a lazy-function for their enum then I handle and construct an internal enumName on the param then delete that before it gets to being added to OpenAPI Spec file.

I am aware that () => SomeEnum is flaky but in order to prevent adding an additional field to these existing decorators: ApiProperty, ApiParam, ApiQuery (maybe ApiBody, I might have missed something), I went with () => SomeEnum instead of ApiProperty({ enum: SomeEnum, enumName: 'SomeEnum' })

[nartc]

unit tests and e2e test have been updated.

[flogh]

I'm using this PR for more than a month now on my project, and it's working great. Would it be able to merge it on the master branch ?

[kamilmysliwiec]

@nartc can we somehow add a way to more explicitly control whether I want to extract an enum or not (additional property)? Instead of implicitly assuming that every lazy-function = extract an enum.

[nartc]

@kamilmysliwiec Implicitly based on the lazy-function is to just allowing TS to extract the Enum name at run time. I didn't want to add more options to ApiProperty, that's why I just used lazy-function. If we are able to make it clear that using lazy-function to declare an enum on ApiProperty, then it will be extracted.

With that said, if it's still essential that we need to explicitly control the extraction, then instead of relying on lazy-function, I can just add another property called enumName to ApiProperty.

You have any suggestions?

[kamilmysliwiec]

With that said, if it's still essential that we need to explicitly control the extraction, then instead of relying on lazy-function, I can just add another property called enumName to ApiProperty.

Even though this may sound like an additional, extra overhead, I think this will be more straightforward for people.

[nartc]

@kamilmysliwiec Alrighty then. It makes the PR even simpler with enumName.
@flogh Since it's likely that the API is going to change for the Enum extraction, there will be changes to your code if the new change (what Kamil and I are talking about) lands on master.

[flogh]

@nartc No prob ! 👍

[prevostc]

Would be a great addition!

[nartc]

enum is no longer taking in a lazy function to "guess" the enumName. Instead, enumName is a separate property to be passed in if the consumers want to have this enum to be extracted out to its separate schema.

// beginning of this PR
@ApiProperty({ enum: () => Role }) // Role will be extracted out

// now
@ApiProperty({ enum: Role, enumName: 'Role' }) // Role will be extracted out

The motivation for this change was in the discussion in this PR. Mainly, we don't want to change the behavior of existing API, so introducing enumName as an addition property to the options is a better trade-off.

[kamilmysliwiec]

@nartc would you like to create a PR to the docs? :)

[nartc]

@kamilmysliwiec Will do in couple of hours.

[kamilmysliwiec]

@nartc awesome, thank you!

[kamilmysliwiec]

Thanks!