nestjs / swagger

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

Metadata gets incorrectly applied/merged when using loadPluginMetadata #2974

Closed ReneZeidler closed 2 weeks ago

ReneZeidler commented 5 months ago

Is there an existing issue for this?

Current behavior

When using SwaggerModule.loadPluginMetadata to load metadata generated by the Swagger CLI plugin (which is necessary when using the SWC builder), some of the metadata gets overriden by @ApiProperty annotations even if the annotation doesn't explicitly set that metadata (e.g. the type).

This behavior is different from using the TSC builder with the Swagger CLI plugin directly. @ApiProperty don't override the type inferred by the CLI plugin unless the annotation explicitly contains the type or schema.

Minimum reproduction code

https://stackblitz.com/edit/nestjs-typescript-starter-r45zy5?file=src%2Fapp.dto.ts,src%2Fmain.ts

Steps to reproduce

  1. Open the minimal reproduction. This reproduction contains a metadata.ts file that has been generated by the Swagger CLI plugin and is loaded with SwaggerModule.loadPluginMetadata in main.ts. The content of the metadata file is correct, i.e. the type of both properties myValues and myOtherValues in HelloResponse is number[].
    • Note: Usually you would use nest build -b swc --type-check with this setup, but SWC doesn't work in StackBlitz. However the behavior with TSC is exactly the same when the Swagger CLI plugin is disabled and the metadata is loaded with loadPluginMetadata.
    • Note: The metadata.ts file was generated by temporarily enabling the CLI plugin, then running nest build -b swc --type-check. The build will fail but the metadata will still be generated. You can also run this locally instead of on StackBlitz to actually use a working SWC build, but the bug seems to be in loadPluginMetadata and also reproduces with TSC.
  2. Observe the schema of the return value of the /hello endpoint:
    • myValues has type [string] even though it should be type [number]. This is caused by the @ApiResponseProperty annotation, which doesn't explicitly override the type and should only set readOnly: true.
      • It's interesting that the type is [string] and not string, which suggests that isArray: true is kept from the metadata, while type: "number" gets overriden by the default value type: "string". The resulting type [string] is incorrect and is neither a default value nor makes sense from the source code.
    • myOtherValues has the correct type [number], which is loaded from the metadata.
  3. Compare with the same minimal reproduction with TSC and the CLI plugin. This example doesn't use loadPluginMetadata and instead uses the Swagger CLI Plugin directly (which is only possible with the TSC builder).
    • myValues has the expected type [number]. The @ApiResponseProperty annotation just sets readOnly: true, while the complete type is inferred by the CLI plugin.

Expected behavior

Using SwaggerModule.loadPluginMetadata with a metadata.ts file generated by nest build -b swc --type-check should result in the same API schema as when using nest build -b tsc with the Swagger CLI plugin.

@ApiProperty annotations should not override metadata with values that are not explicitly set by the annotation itself. All the metadata included in the metadata.ts should be used and not get overriden by default values unless explicitly set.

Package version

7.3.1

NestJS version

10.3.9

Node.js version

20.14.0 / 18.20.3

In which operating systems have you tested?

Other

No response

ReneZeidler commented 5 months ago

I added another example to the same reproduction URL. The endpoint /badRequest has the annotation @ApiBadRequestResponse({ description: 'Bad request' }). With TSC and the plugin, this gets added to the list of return codes for the endpoint (the default 200 response with the return type inferred from the method return type is still there).
With loadPluginMetadata, the annotation overwrites the defaut response code.

kamilmysliwiec commented 4 months ago

Would you like to create a PR for this issue?

ReneZeidler commented 4 months ago

I tried to look if there was a simple fix, but I'm not familiar enough with the codebase to find the root cause of this issue. So I unfortunately can't provide a PR.

kamilmysliwiec commented 2 weeks ago

Let's track this here https://github.com/nestjs/swagger/pull/3051