Support nullable

[jackfrankland]

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 adding null as a possible type.

[bcherny]

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.

[jackfrankland]

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?

[bcherny]

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.

[jackfrankland]

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.

[mdmower]

@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 nullable used without type?"

Could we revive the conversation and pick a path forward?

[zrosenbauer]

@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 nullable used without type?"

Could we revive the conversation and pick a path forward?

EDIT Aug 12: 🦗🦗🦗

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).

[bcherny]

Closing out stale PRs.