Skip to content

Support nullable #1

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

Merged
merged 2 commits into from
Oct 23, 2023
Merged

Support nullable #1

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cda84ce
Support nullable
mdmower-csnw Oct 20, 2023
a5db6da
(Revisions) Support nullable
mdmower-csnw Oct 23, 2023
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
29 changes: 28 additions & 1 deletion src/normalizer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import {JSONSchemaTypeName, LinkedJSONSchema, NormalizedJSONSchema, Parent} from './types/JSONSchema'
import {appendToDescription, escapeBlockComment, isSchemaLike, justName, toSafeString, traverse} from './utils'
import {appendToDescription, escapeBlockComment, isSchemaLike, justName, log, toSafeString, traverse} from './utils'
import {Options} from './'
import {DereferencedPaths} from './resolver'
import {isDeepStrictEqual} from 'util'
import {link} from './linker'

type Rule = (
schema: LinkedJSONSchema,
Expand Down Expand Up @@ -222,6 +223,32 @@ 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.const !== undefined) {
if (schema.const !== null) {
schema.enum = [schema.const, null]
delete schema.const
}
} else if (schema.enum) {
if (!schema.enum.includes(null)) {
schema.enum.push(null)
log('yellow', 'normalizer', 'enum should include null when schema is nullable', schema)
Copy link

@tvharris tvharris Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdmower-csnw:

  1. Should there be a warning for the const case, too? AJV docs say with nullable: true:

const would fail, unless it is "const": null

  1. I wouldn't be opposed to throwing instead of the warnings, because it's critical that the schema is updated or the type will be at odds with how AJV validates.

  2. What if const and enum are both defined? Is that case caught earlier by validation?

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The const case is mostly moot because normalizer Transform const to singleton enum runs just before Transform nullable to null type:

rules.set('Transform const to singleton enum', schema => {
  if (schema.const !== undefined) {
    schema.enum = [schema.const]
    delete schema.const
  }
})

I included const handling just in case normalizers are reordered in the future.

What if const and enum are both defined? Is that case caught earlier by validation?

schema.enum actually gets wiped out in this case due to the above normalizer. This is a great example of how this tool is not a validator. There's an expectation that the user has performed validation prior to using this tool.

Should there be a warning for the const case, too? AJV docs say with nullable: true.

That's fine. Can do.

I wouldn't be opposed to throwing instead of the warnings, because it's critical that the schema is updated or the type will be at odds with how AJV validates.

Like mentioned before, this is not a validator. It's a tool that seldom throws.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opted to reorder transforms so that Transform nullable to null type runs before Transform const to singleton enum in a5db6da. Also created a warning() function that prints to the console even when VERBOSE is not used.

}
} else if (schema.type) {
schema.type = [...[schema.type].flatMap(value => value), 'null']
} else if (schema.anyOf) {
schema.anyOf.push(link({type: 'null'}, schema.anyOf))
} else if (schema.oneOf) {
schema.oneOf.push(link({type: 'null'}, schema.oneOf))
Copy link

@tvharris tvharris Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdmower-csnw: Is this how AJV handles nullable: true for anyOf and oneOf? It doesn't say explicitly in the docs.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is a rabbit hole.

The OpenAPI spec writers (i.e. not the full picture, just one example of a team defining a JSON schema) eventually decided to restrict use of nullable to only schemas that define type in 3.0.3 (see 3.0.3: Fields). In an early draft of 3.1, when nullable was planned for deprecation rather than removal, wording implied that nullable wasn't forbidden when paired with something like allOf, but that it could be overridden (see 3.1(draft): Fields). The start of the rabbit hole is here: OpenAPI-Specification: Reference objects don't combine well with “nullable”.

I approached this from the standpoint that nullable paired with anyOf and oneOf is unambiguous (but not allOf). After more reading, it looks like this was a bad assumption and there's good reason the OpenAPI team opted to limit use of nullable to schemas with type. The most helpful comments were this inqury: OAI/OpenAPI-Specification#1368 (comment), and this response: OAI/OpenAPI-Specification#1368 (comment) .

I lean towards leaving the const and enum pairings with nullable as is, i.e. allowed, but logged if null not in value(s) -- the intent is clear even if not 100% spec compliant. On the other hand, I no longer feel comfortable pairing nullable with anyOf, oneOf, and allOf. These will be removed.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Handling of anyOf and oneOf removed in a5db6da.

}
})

export function normalize(
rootSchema: LinkedJSONSchema,
dereferencedPaths: DereferencedPaths,
Expand Down
31 changes: 31 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 @@ -1883,6 +1883,37 @@ 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 {␊
a?: "" | null;␊
b?: null;␊
c?: "a" | "b" | null;␊
d?: "" | null;␊
e?: string | null;␊
f?: null;␊
g?: "" | null;␊
h?: null;␊
i?: "a" | "b" | null;␊
j?: "" | null;␊
k?: string | number | null;␊
l?: string | null;␊
m?: string | number | null;␊
n?: string | null;␊
o?: string | number | null;␊
p?: string | null;␊
}␊
`

## 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.
74 changes: 74 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,74 @@
export const input = {
type: 'object',
properties: {
a: {
const: '',
nullable: true,
},
b: {
const: null,
nullable: true,
},
c: {
enum: ['a', 'b'],
nullable: true,
},
d: {
enum: ['', null],
nullable: true,
},
e: {
type: 'string',
nullable: true,
},
f: {
type: 'null',
nullable: true,
},
g: {
type: 'string',
const: '',
nullable: true,
},
h: {
type: 'string',
const: null,
nullable: true,
},
Comment on lines +33 to +37
Copy link

@tvharris tvharris Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdmower-csnw: This might not be directly related to the changes in this PR, but I was wondering if there's a warning for a schema like this where the type and const (or enum values) are inconsistent. I don't know how AJV handles such a schema.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not a warning output by this tool. You can test this.

i: {
type: 'string',
enum: ['a', 'b'],
nullable: true,
},
j: {
type: 'string',
enum: ['', null],
nullable: true,
},
k: {
type: ['string', 'integer'],
nullable: true,
},
l: {
type: ['string', 'null'],
nullable: true,
},
m: {
anyOf: [{type: 'string'}, {type: 'integer'}],
nullable: true,
},
n: {
anyOf: [{type: 'string'}, {type: 'null'}],
nullable: true,
},
o: {
oneOf: [{type: 'string'}, {type: 'integer'}],
nullable: true,
},
p: {
oneOf: [{type: 'string'}, {type: 'null'}],
nullable: true,
},
},
additionalProperties: false,
}
139 changes: 139 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,139 @@
{
"name": "Nullable adds null to type",
"in": {
"$id": "a",
"type": "object",
"properties": {
"a": {
"const": "",
"nullable": true
},
"b": {
"const": null,
"nullable": true
},
"c": {
"enum": ["a", "b"],
"nullable": true
},
"d": {
"enum": ["", null],
"nullable": true
},
"e": {
"type": "string",
"nullable": true
},
"f": {
"type": "null",
"nullable": true
},
"g": {
"type": "string",
"const": "",
"nullable": true
},
"h": {
"type": "string",
"const": null,
"nullable": true
},
"i": {
"type": "string",
"enum": ["a", "b"],
"nullable": true
},
"j": {
"type": "string",
"enum": ["", null],
"nullable": true
},
"k": {
"type": ["string", "integer"],
"nullable": true
},
"l": {
"type": ["string", "null"],
"nullable": true
},
"m": {
"anyOf": [{"type": "string"}, {"type": "integer"}],
"nullable": true
},
"n": {
"anyOf": [{"type": "string"}, {"type": "null"}],
"nullable": true
},
"o": {
"oneOf": [{"type": "string"}, {"type": "integer"}],
"nullable": true
},
"p": {
"oneOf": [{"type": "string"}, {"type": "null"}],
"nullable": true
}
},
"required": [],
"additionalProperties": false
},
"out": {
"$id": "a",
"type": "object",
"properties": {
"a": {
"enum": ["", null]
},
"b": {
"enum": [null]
},
"c": {
"enum": ["a", "b", null]
},
"d": {
"enum": ["", null]
},
"e": {
"type": ["string", "null"]
},
"f": {
"type": ["null", "null"]
},
Copy link

@tvharris tvharris Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdmower-csnw: The output here and cases l, n, p don't seem ideal with the redundant null. I suppose it doesn't matter since the types are generated correctly.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I don't think there's need for deduplication of types at this stage since the end user doesn't see it and the TypeScript conversion handles duplicates just fine.

Copy link
Author

@mdmower-csnw mdmower-csnw Oct 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deduplication was trivial so opted to handle it in a5db6da.

"g": {
"type": "string",
"enum": ["", null]
},
"h": {
"type": "string",
"enum": [null]
},
"i": {
"type": "string",
"enum": ["a", "b", null]
},
"j": {
"type": "string",
"enum": ["", null]
},
"k": {
"type": ["string", "integer", "null"]
},
"l": {
"type": ["string", "null", "null"]
},
"m": {
"anyOf": [{"type": "string"}, {"type": "integer"}, {"type": "null"}]
},
"n": {
"anyOf": [{"type": "string"}, {"type": "null"}, {"type": "null"}]
},
"o": {
"oneOf": [{"type": "string"}, {"type": "integer"}, {"type": "null"}]
},
"p": {
"oneOf": [{"type": "string"}, {"type": "null"}, {"type": "null"}]
}
},
"required": [],
"additionalProperties": false
}
}