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
Support nullable #522
base: master
Are you sure you want to change the base?
Support nullable #522
Conversation
return | ||
} | ||
|
||
schema.type = [...[schema.type].flatMap(value => value), 'null'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work in all cases, since not every schema defines an explicit type
field.
For example, consider the following schema:
{
properties: {
a: {
properties: {
b: {type: 'string'}
},
nullable: true
}
}
}
The expected output would be:
export interface Demo {
a?: {
b?: string;
[k: string]: unknown;
} | null;
[k: string]: unknown;
}
But with your code, we wouldn't emit | null
because schema.type
is not defined.
I'd suggest a more general approach using anyOf
, similar to what @goodoldneon started in #411. The place to start would be to write a bunch of tests to make sure you're handling all the cases around anyOf
: titles, properties, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cheers for the review.
The key reason I think we should not use anyOf
is that these two are not equivalent when it comes to validation
{
type: 'object',
properties: {
foo: {
type: ['string', 'null'],
enum: ['a', 'b'],
},
},
}
{
type: 'object',
properties: {
foo: {
anyOf: [
{
type: 'string',
enum: ['a', 'b'],
},
{
type: 'null',
},
],
},
},
}
The following object will fail for 1, but succeed for 2:
{
foo: null,
}
I realise that currently your library resolves to the same thing for both:
interface Example {
foo: 'a' | 'b' | null;
}
But I'm not sure that's correct. In any case, perhaps it's best for the normalizer to resolve to the correct json schema equivalent, which for nullable: true
is 1.
If I were to add a check for the properties
property, and infer type: 'object'
, would that work? Are there any other edge cases I'm missing going down this route?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two are not equivalent when it comes to validation... I realise that currently your library resolves to the same thing for both
Yep, this is a separate bug for sure!
If I were to add a check for the properties property, and infer type: 'object', would that work? Are there any other edge cases I'm missing going down this route?
This is tricky, since when it isn't explicitly defined, type
is inferred based on a set of heuristics. Defaulting to object
would introduce | object
to emitted types for any input schema that doesn't explicitly supply a type
, which would be a bug. (Try making the change and see how tests break.)
The AJV docs are a little unclear to me, to be honest. If my schema is:
{
"type": "string",
"enum": ["a", "b"],
"nullable": true
}
Clearly my intention is to say "this schema can be "a"
, "b"
, or null
". I wonder why the AJV interpretation is "this schema can be "a"
or "b"
, and not null
".
Either way, some possible solutions:
- Use
anyOf
instead oftype
. Adjust your normalizer rule: if a schema has bothenum
that does not includenull
andnullable: true
, emit a warning that this is a user error and the user should fix their schema, then proceed to normalize toanyOf(schema, null)
. Same forconst
, per the AJV docs. We could then separately fix the bug you called out, where iftype
contains a type not captured by theenum
, we should normalize away the excesstype
and emit a warning about the likely user error. - Normalize so schemas always have an explicit
type
. Add a normalizer rule to infer an explicittype
for each schema -- essentially, hoisting part of our heuristics to the normalization phase. This may be a pretty big change, and tricky to get right.
Open to other ideas, if you have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I found this while looking for further background on the behaviour AJV have adopted with this OpenAPI extension: https://github.com/OAI/OpenAPI-Specification/blob/main/proposals/2019-10-31-Clarify-Nullable.md
It seems that although this is still in proposal stage, AJV have decided to follow the recommendations. Even as far as throwing an error when attempting to compile a schema where nullable: true
is used without a type
: https://github.com/ajv-validator/ajv/blob/1b07663f3954b48892c7210196f7c6ba08000091/spec/options/nullable.spec.ts
Is it best to follow this do you think? If so I think the current behaviour matches it, but I can add the same test scenarios as this ajv spec. Perhaps emit a warning where AJV throws an exception?
p.s. I do actually agree with you, to me it would seem more intuitive for nullable: true
to just allow for null regardless of the rest of the schema.
@bcherny and @jackfrankland - I'm interested in moving this feature forward. I've drafted some revisions to this PR and posted them for discussion in #535 (I don't want to hijack this PR without discussion). It seemed reasonable (and works like I expect) to simply add handling for additional ways of specifying types: enum, type, anyOf, oneOf (and const is converted to enum by another normalizer). I also aimed to be forgiving for issues like "is Could we revive the conversation and pick a path forward? |
I left a quick review on your PR, didn't take time to review this in depth but the other PR seemed feature complete (I've not reviewed any PRs in this lib until today 😄, but guessing the normalizer rules run recursively so should work). |
PR to replace #411 and #312.
Adding support for
nullable: true
. Using https://ajv.js.org/json-schema.html#nullable as reference, which states that it is the equivalent of addingnull
as a possible type.