fix(openapi-typescript): handle nullable schemas #2059
Conversation
|
gduliscouet-ubitransport
force-pushed
the
handle-nullable
branch
from
581e476 to
4339d72
Compare
|
Hello @drwpow , Once we resolve that I'll finalize the PR |
That’s my understanding. If you look at the OpenAPI 3.1 specification, you’ll often see typings like this: |
| obj1: { | ||
| type: "object", | ||
| nullable: true, | ||
| $ref: "#/components/schemas/obj1Ref", |
There was a problem hiding this comment.
As discussed, this is simply an invalid type. We shouldn’t be testing this. To combine obj1Ref and these properties, it must be within allOf or anyOf. We can also simply remove this test if it’s the only one failing (haven’t looked a the failure logs yet)
There was a problem hiding this comment.
Thank you for the quick reply.
Well you are right with the fact that in OAS 3.1 spec there are both the SchemaObject (a thing of JSON Schema spec) and the ReferenceObject (a thing of OAS spec). And that the ReferenceObject "cannot be extended with additional properties".
But, and this is what confused me initially, having a $ref in an object doesn't make it a ReferenceObject. I think this and this comments resume it best.
So in the end, here:
- obj1 is a SchemaObject
- additionnal properties are allowed
- the merging of properties should be done following JSON schema's spec
What I propose is to keep this PR only for the nullable subject. And I'll open another issue for the ref handling subject. What do you think ?
There was a problem hiding this comment.
I do see those comments, but that does seem to conflict with what I’m reading in the specs themselves.
OpenAPI 3.1 has language like the following:
In case a Path Item Object field appears both in the defined object and the referenced object, the behavior is undefined.
Also JSON Schema has language like so:
When an object contains a $ref property, the object is considered a reference, not a schema. Therefore, any other properties you put in that object will not be treated as JSON Schema keywords and will be ignored by the validator.
The fact that I can’t find this is explicitly allowed and has defined behavior means I’d rather not support it, mainly because going beyond the spec tends to come back to cause other unexpected behavior in the weirdest ways that are hard to backpedal from.
What I propose is to keep this PR only for the nullable subject. And I'll open another issue for the ref handling subject. What do you think ?
That sounds great to me! I think an issue explicitly discussing this behavior would be great. Like with anything I could be wrong. I would just like additional input and arguments that make a good case for what the behavior should be, and point to sources (and more importantly, how it differs from anyOf/allOf composition!)
There was a problem hiding this comment.
Ok I'll take care of opening the new issue and finishing this PR when I have a computer at hand.
For your 1st quote, on the Path Item Object: it is not a Schema Object. For some reason they introduced a 3rd use of the $ref here, instead of proposing to use a Reference Object. In places like the response body schema, the only definition of the $ref we should consider is the one of the Schema Object, and it is defined in JSON Schema's spec.
For your second quote: it is taken from an insert about previous draft versions (Draft 4 to Draft 7). Openapi 3.1 is relying on Draft 2020-12, which introduced the new way to handle $ref.
However openapi versions 3.0.x are relying on Draft 7, so the change came with the 3.1
Changes
Allow all types except UNKNOWN to be nullable.
Fixes #1821 and #1940 .
How to Review
I re-applied #1959 (it didn't move for a while)
I added tests for the new syntax (
oneOf: [SchemaFoo, { type: "null" }]).I also wanted to add test for both syntax, mainly for our use case.
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)