nestjs / swagger

OpenAPI (Swagger) module for Nest framework (node.js) :earth_americas:
https://nestjs.com
MIT License
1.69k stars 474 forks source link

[Bug] - Property/Field Overwriting Doesn't Manifest In Swagger When Extending A Class #1321

Open leevuli opened 3 years ago

leevuli commented 3 years ago

I'm submitting a...


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

Current behavior

When I extend a class and try overwrite an inherited property/field. The result of the overwriting doesn't manifest in the Swagger documentation. See code below how to reproduce.

current-behaviour

Expected behavior

The desired behaviour would be how the overwriting manifests in the TypeScript code: The property info of EntityPutDTO should be of type InfoPutDTO, not InfoPostDTO. (See code below)

expected-behaviour

Minimal reproduction of the problem with instructions

export class InfoPostDTO {
    @ApiProperty()
    name: string;
}
export class InfoPutDTO extends InfoPostDTO {
    @ApiProperty()
    id: number;
}

export class EntityPostDTO {
    @ApiProperty()
    id: number;

    @ApiProperty()
    info: InfoPostDTO;
}
export class EntityPutDTO extends EntityPostDTO {
    @ApiProperty()
    info: InfoPutDTO;
}

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

The motivation is that the classes manifest themselves the same way into the documentation that they do in the TypeScript code.

Environment


Nest version: 7.6.15
"@nestjs/common": "^7.6.15",
"@nestjs/core": "^7.6.15",
"@nestjs/platform-express": "^7.6.15",
"@nestjs/swagger": "^4.8.0",


For Tooling issues:
- Node version: v15.12.0  
- Platform:  Linux (Ubuntu 20.04)

Others:

kamilmysliwiec commented 3 years ago

Please provide a minimum reproduction repository.

leevuli commented 3 years ago

Here's the minimun reproduction repository: https://github.com/koxanybak/nestjs.swagger.issues.1321

leevuli commented 3 years ago

Any updates on this or planned fixes for future versions?

Or wasn't the minimun reproduction repository not clear enough? If that's the case, tell me and I'll make it better.

kamilmysliwiec commented 3 years ago

Would you like to create a PR for this issue?

leevuli commented 3 years ago

I worked on the bug and I came to the conclusion that the issue can be found in the file lib/services/schema-object-factory.ts, in the function mergePropertyWithMetadata where the metadata variable is set with the help of Reflect.getMetadata(DECORATORS.API_MODEL_PROPERTIES, prototype, key).

In the example issue above (and in the minimun reproduction repository), when schema objects are gathered for the EntityPutDTO and when looking for the type for the problematic info field, after setting the metadata variable, the variables in the function are as follows: image The metadata.type is the type of the info field in the EntityPostDTO from which the EntityPutDTO inherits. You can also see that the prototype is EntityPostDTO and not EntityPutDTO.

I couldn't figure out if the Reflect.getMetadata doesn't function correctly or if the prototype is not what it should be BUT, if the prototype is not what is should be, the problem can traced to the file lib/explorers/api-response.explorer.ts to the function exploreApiResponseMetadata where the responses array is defined with the help of Reflect.getMetadata(constants_2.DECORATORS.API_RESPONSE, method). The prototype is then get as response.type.prototype. So in any case, in my very unprofessional opinion, the issue stems from Reflect.getMetadata.

I don't really have energy to debug the issue further but if someone else wants to give it a try, this is a good place to start.

leevuli commented 3 years ago

I found a workaround/solution for the issue. The key is to not overwrite any fields. I solved this by creating an abstract base entity DTO with all the fields that will be not overwritten. Then all the other DTOs inherit that base entity. This way no fields are overwritten, thus avoiding the bug.

The bug is still there but with this workaround you can avoid it.

export class InfoPostDTO {
    @ApiProperty()
    name: string;
}
export class InfoPutDTO extends InfoPostDTO {
    @ApiProperty()
    id: number;
}

abstract class BaseEntityDTO {
    @ApiProperty()
    id: number;
}

export class EntityPostDTO extends BaseEntityDTO {
    @ApiProperty()
    info: InfoPostDTO;
}
export class EntityPutDTO extends BaseEntityDTO {
    @ApiProperty()
    info: InfoPutDTO;
}