nestjs / swagger

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

Custom `type` is not retained as type, but rewritten as `allOf: - $ref: ...` breaking `nullable: true` spec in `@ApiProperty` #2948

Open gregoryorton-ws opened 4 months ago

gregoryorton-ws commented 4 months ago

Is there an existing issue for this?

Current behavior

Custom type is not retained as type, but rewritten as allOf: - $ref: ... breaking nullable: true spec in @ApiProperty

In @ApiProperty when using something like:

@ApiProperty({ nullable: true, type: MyCustomDto })
readonly myVar: MyCustomDto | null;
...

the rendered openapi.yaml is like:

myVar:
  nullable: true
  allOf:
     - $ref: "#/components/schemas/MyCustomDto"

tools like Redoc.ly then start to throw errors:

image

Minimum reproduction code

https://nothing.com

Steps to reproduce

see above

Expected behavior

myVar:
  type: object
  properties:
    id:
      type: integer

Package version

7.3.0

NestJS version

10.3.3

Node.js version

20

In which operating systems have you tested?

Other

maybe related: https://github.com/nestjs/swagger/issues/2645 maybe related: https://github.com/nestjs/swagger/pull/2646

kamilmysliwiec commented 4 months ago

Would you like to create a PR for this issue?

gregoryorton-ws commented 4 months ago

Sure, but I don't know what the solution would be here -

what should the value of type be? Would we need to put the custom DTO into a schemas and then reference it?

gregoryorton-ws commented 4 months ago

One solution looks like:

myVar:
  nullable: true
  allOf:
    - $ref: "#/components/schemas/MyCustomDto"
    - type: object

I will test with redoc.ly to see if it's acceptable. If it is, then I think we can just add this type to the output for the custom type.

gregoryorton-ws commented 4 months ago

The only think redoc.ly will accept is:

myVar:
  nullable: true
  type: object
  allOf:
    - $ref: "#/components/schemas/MyCustomDto"

so it seems like an easy fix

clintonb commented 4 months ago

Here is the source of our woes: https://github.com/nestjs/swagger/blob/540490563b73aae942e00495647ecd675c42eb21/lib/services/schema-object-factory.ts#L368-L379

Extra keys that are not in the set of ['type', 'isArray', 'required'] will result in an allOf instead of a direct $ref. In this case (and #2645), the key is nullable. In my case, it's name.

The list of supported keys needs to be expanded, or removed altogether. I'm not sure which one as I haven't read the OpenAPI spec to any level of detail.

armaniito commented 2 months ago

Hello, i have the same issue as well. Setting a property other than "type, isArray, required" results in an allOf, causing my DTO schemas to lack the reference to another DTO. This causes problems with OpenAPI Generator because it generates the same models twice, except the file names are different.

It would be greatly appreciated if there could be a fix for this so that we get a $ref instead of an allOf.

Thank you very much

MorelSerge commented 2 months ago

The only think redoc.ly will accept is:

myVar:
  nullable: true
  type: object
  allOf:
    - $ref: "#/components/schemas/MyCustomDto"

so it seems like an easy fix

This is indeed accepted, but breaks UIs as the type=object no longer "opens up" the nested object.

Edit: My solution ended up being patching the package:

diff --git a/dist/services/schema-object-factory.js b/dist/services/schema-object-factory.js
index b73f013c79218af537f720e71aeabf83a35f9775..5f1cdebf6c89a7373d6f4b98a8d8d67aac91b3e2 100644
--- a/dist/services/schema-object-factory.js
+++ b/dist/services/schema-object-factory.js
@@ -201,6 +201,9 @@ class SchemaObjectFactory {
         const keysToRemove = ['type', 'isArray', 'required', 'name'];
         const validMetadataObject = (0, lodash_1.omit)(metadata, keysToRemove);
         const extraMetadataKeys = Object.keys(validMetadataObject);
+        if (metadata.nullable) {
+            return Object.assign(Object.assign({ name: metadata.name || key, required: metadata.required }, validMetadataObject), { $ref });
+        }
         if (extraMetadataKeys.length > 0) {
             return Object.assign(Object.assign({ name: metadata.name || key, required: metadata.required }, validMetadataObject), { allOf: [{ $ref }] });
         }
PierreKiwi commented 1 month ago

Same issue with description.

With description set like this

@ApiProperty({ description: "Pagination details.", type: PaginationOutputDto })

The result is

image

Without description and just the type like this

@ApiProperty({ type: PaginationOutputDto })

The result is

image