Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
2 of 4 tasks
gregoryorton-ws opened this issue May 13, 2024 · 9 comments

Comments

@gregoryorton-ws
Copy link

gregoryorton-ws commented May 13, 2024

Is there an existing issue for this?

  • I have searched the existing issues

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?

  • macOS
  • Windows
  • Linux

Other

maybe related: #2645
maybe related: #2646

@kamilmysliwiec
Copy link
Member

Would you like to create a PR for this issue?

@gregoryorton-ws
Copy link
Author

gregoryorton-ws commented May 14, 2024

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
Copy link
Author

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
Copy link
Author

gregoryorton-ws commented May 14, 2024

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
Copy link
Contributor

Here is the source of our woes:

const keysToRemove = ['type', 'isArray', 'required'];
const validMetadataObject = omit(metadata, keysToRemove);
const extraMetadataKeys = Object.keys(validMetadataObject);
if (extraMetadataKeys.length > 0) {
return {
name: metadata.name || key,
required: metadata.required,
...validMetadataObject,
allOf: [{ $ref }]
} as SchemaObjectMetadata;
}

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
Copy link

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
Copy link

MorelSerge commented Jul 14, 2024

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
Copy link

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

@kamilmysliwiec
Copy link
Member

I finally got a chance to get back to this issue and I confirm that this is a valid behavior. All extra attributes, like description, nullable, etc. need to convert to allOf:

image

@nestjs nestjs locked and limited conversation to collaborators Oct 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants