Skip to content

Support nullable #522

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Support nullable #522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/normalizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,20 @@ rules.set('Transform const to singleton enum', schema => {
}
})

rules.set('Transform nullable to null type', schema => {
if (schema.nullable !== true) {
return
}

delete schema.nullable

if (!schema.type) {
return
}

schema.type = [...[schema.type].flatMap(value => value), 'null']
Copy link
Owner

@bcherny bcherny Apr 23, 2023

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.

Copy link
Author

@jackfrankland jackfrankland Apr 23, 2023 •
edited
Loading

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?

Copy link
Owner

@bcherny bcherny May 4, 2023 •
edited
Loading

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:

  1. Use anyOf instead of type. Adjust your normalizer rule: if a schema has both enum that does not include null and nullable: true, emit a warning that this is a user error and the user should fix their schema, then proceed to normalize to anyOf(schema, null). Same for const, per the AJV docs. We could then separately fix the bug you called out, where if type contains a type not captured by the enum, we should normalize away the excess type and emit a warning about the likely user error.
  2. Normalize so schemas always have an explicit type. Add a normalizer rule to infer an explicit type 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.

Copy link
Author

@jackfrankland jackfrankland May 4, 2023

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.

})

export function normalize(
rootSchema: LinkedJSONSchema,
dereferencedPaths: DereferencedPaths,
Expand Down
18 changes: 18 additions & 0 deletions test/__snapshots__/test/test.ts.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1791,6 +1791,24 @@ Generated by [AVA](https://avajs.dev).
}␊
`

## nullable.js

> Expected output to match snapshot for e2e test: nullable.js

`/* eslint-disable */␊
/**␊
* This file was automatically generated by json-schema-to-typescript.␊
* DO NOT MODIFY IT BY HAND. Instead, modify the source JSONSchema file,␊
* and run json-schema-to-typescript to regenerate this file.␊
*/␊
␊
export interface Nullable {␊
foo?: string | null;␊
bar?: string | number | null;␊
[k: string]: unknown;␊
}␊
`

## oneOf.js

> Expected output to match snapshot for e2e test: oneOf.js
Expand Down
Binary file modified test/__snapshots__/test/test.ts.snap
View file
Open in desktop
Binary file not shown.
13 changes: 13 additions & 0 deletions test/e2e/nullable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export const input = {
type: 'object',
properties: {
foo: {
type: 'string',
nullable: true
},
bar: {
type: ['string', 'number'],
nullable: true
}
}
}
33 changes: 33 additions & 0 deletions test/normalizer/nullableAddsNullToType.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
{
"name": "Nullable adds null to type",
"in": {
"$id": "a",
"type": "object",
"properties": {
"foo": {
"type": "string",
"nullable": true
},
"bar": {
"type": ["string", "number"],
"nullable": true
}
},
"required": [],
"additionalProperties": false
},
"out": {
"$id": "a",
"type": "object",
"properties": {
"foo": {
"type": ["string", "null"]
},
"bar": {
"type": ["string", "number", "null"]
}
},
"required": [],
"additionalProperties": false
}
}