swagger-typescript-api icon indicating copy to clipboard operation
swagger-typescript-api copied to clipboard

nullable: true on an object type not adding union with null on types

Open Jackman3005 opened this issue 2 years ago • 4 comments

Hello,

Thanks so much for this solution, many of us really appreciate it.

I've been trying to get things to work with nullable: true. I have a situation where I want my end type to be:

interface MyResponse {
  myObject: {id: string, other: string} | null
}

This is a snippet of the spec I'm intending to provide

...
myObject:
  type: object
  nullable: true
  properties:
    id:
      type: string
    name:
      type: string
  required:
    - id
    - name
required:
  - myObject

The closest I am able to get is:

interface MyResponse {
  myObject?: {id: string, other: string}
}

My understanding is that required is used to specify whether or not undefined is a valid value. nullable is used to union the type with null or not. This is the desired functionality. From peaking around the repo it appears that nullable is only considered if the type is string. See this line in the checkAndAddNull function for context.

Is it as simple as just removing this? What kind of conditions were we trying to avoid when that line was added originally? I'm assuming it's appropriate for other types besides string and object to be nullable as well.

I do realize that in most situations there is no practical difference between null and undefined. However, unfortunately this issue is preventing another library (Prisma) from being able to play nicely with the generated swagger types.

Thanks in advance!

Jackman3005 avatar Mar 04 '22 05:03 Jackman3005

Having the same issue here.

As a temporary workaround I defined a schema:

"Null": {
  "description": "the JSON value `null`",
  "example": null,
  "nullable": true,
  "oneOf": [],
  "type": "string"
}

and used it like this:

"oneOf": [
  { "$ref": "#/components/schemas/OtherType" },
  { "$ref": "#/components/schemas/Null" }
]

this generates the correct typescript type: OtherType | null

lew avatar Mar 18 '22 21:03 lew

@lew Clever workaround. It would be nice to get some direction as to why the checkAndAddNull function is filtering to only include string types. I don't quite know what the consequence will be in removing the filtering and letting all things be nullable. In my mind, that's what we want.

Would it help for me to open a PR with just that line removed? Again, I need someone to provide some confidence that a change like that is correct/desired.

Jackman3005 avatar Mar 20 '22 22:03 Jackman3005

+1

mfleungac avatar Mar 29 '22 04:03 mfleungac

+1

besrezen90 avatar Apr 15 '22 08:04 besrezen90

Hello @Jackman3005 , can you say version of the swager schema ?
Because "nullable" field is exist only in OA 3.0+ but not in Swagger 2.0. (https://swagger.io/specification/v2/)
If you take a look at this tests
(swagger 3.0) https://github.com/acacode/swagger-typescript-api/blob/next/tests/spec/nullable-3.0/schema.json
https://github.com/acacode/swagger-typescript-api/blob/next/tests/spec/nullable-3.0/schema.ts
(swagger 2.0) https://github.com/acacode/swagger-typescript-api/blob/next/tests/spec/nullable-2.0/schema.json
https://github.com/acacode/swagger-typescript-api/blob/next/tests/spec/nullable-2.0/schema.ts
You will see that everything is ok

js2me avatar Oct 29 '22 06:10 js2me

It's Working!

@js2me We are using openapi: 3.0.0. I saw there has been some new versions since I last tested this out, so I upgraded from 9.3.1 to 11.1.2. After upgrading I see the issue resolved 🎉. Adding nullable: true to fields of type: object is correctly union-ing the generated object type with null. I don't know exactly which version resolved the issue for us, but I can confirm it existed back in 9.3.1. Thanks! 🙏

Test Improvements

In regards to your Swagger 3.0 test schema. I noticed that otherObjectMaybeNullA is likely not a good test. The Swagger Docs for $ref state the following:

Any sibling elements of a $ref are ignored. This is because $ref works by replacing itself and everything on its level with the definition it is pointing at.

Additionally, the referenced object already has nullable: true so even if this library does not ignore sibling elements to $ref, it wouldn't be a good test of that functionality. This point follows for otherObjectMaybeNullB, ... since it is referencing the object definition which already has nullable: true as well. I think it would be best to either remove nullable: true from the object definition or create a separate one that does not have it so the other ways of adding null are truly tested.

I love this library and am thankful for all its contributors! 🙇‍♂️

Jackman3005 avatar Oct 30 '22 23:10 Jackman3005

@Jackman3005 thanks for good point, will fix this issue!

js2me avatar Oct 31 '22 09:10 js2me