JSONSchemaType incorrectly requires optional properties to be nullable

[swisener]

What version of Ajv are you using? Does the issue happen if you use the latest version?

Ajv 8.6.0, Typescript 4.3.4

Your typescript code

import { JSONSchemaType } from 'ajv/dist/2020';

interface Example {
  foo: string;
  bar?: string;
}

const schema: JSONSchemaType<Example> = {
  additionalProperties: false,
  type: 'object',
  properties: {
    foo: {
      type: 'string'
    },
    bar: {
      type: 'string'
    }
  },
  required: ['foo']
};

Typescript compiler error messages

TS2322: Type '{ additionalProperties: false; type: "object"; properties: { foo: { type: "string"; }; bar: { type: "string"; }; }; required: "foo"[]; }' is not assignable to type 'UncheckedJSONSchemaType<Example, false>'.
  Type '{ additionalProperties: false; type: "object"; properties: { foo: { type: "string"; }; bar: { type: "string"; }; }; required: "foo"[]; }' is not assignable to type '{ type: "object"; additionalProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; unevaluatedProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; ... 7 more ...; maxProperties?: number | undefined; } & { ...; } & { ...; } & { ...; }'.
    Type '{ additionalProperties: false; type: "object"; properties: { foo: { type: "string"; }; bar: { type: "string"; }; }; required: "foo"[]; }' is not assignable to type '{ type: "object"; additionalProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; unevaluatedProperties?: boolean | UncheckedJSONSchemaType<unknown, false> | undefined; ... 7 more ...; maxProperties?: number | undefined; }'.
      The types of 'properties.bar' are incompatible between these types.
        Type '{ type: "string"; }' is not assignable to type '{ $ref: string; } | (UncheckedJSONSchemaType<string | undefined, false> & { nullable: true; const?: undefined; enum?: readonly (string | null | undefined)[] | undefined; default?: string | ... 1 more ... | undefined; })'.
          Type '{ type: "string"; }' is not assignable to type '{ type: "string"; } & StringKeywords & { allOf?: readonly UncheckedPartialSchema<string | undefined>[] | undefined; anyOf?: readonly UncheckedPartialSchema<string | undefined>[] | undefined; ... 4 more ...; not?: UncheckedPartialSchema<...> | undefined; } & { ...; } & { ...; }'.
            Property 'nullable' is missing in type '{ type: "string"; }' but required in type '{ nullable: true; const?: undefined; enum?: readonly (string | null | undefined)[] | undefined; default?: string | null | undefined; }'.

Describe the change that should be made to address the issue?

This code will not compile unless bar is marked as nullable in the schema. However, I don't want to accept null as a value for bar if it is present. It appears that JSONSchemaType expects all properties not mentioned in the required array to be marked as nullable. null and undefined are different values and should not be conflated.

Are you going to resolve the issue? 🤷

[epoberezkin]

This is a deliberate decision to require that optional properties are allowed to be null values, as it is how APIs are commonly designed...

Whether it was a correct choice is a good question but it'll definitely stay like that until the next major version...

[swisener]

It's not that optional properties are "allowed" to be null values, it's that they must be marked as nullable in the schema. This means that it is impossible to use JSONSchemaType with any schema that has a non-nullable optional property. This seems like a pretty common use case...

[epoberezkin]

For API payload validation allowing null for all optional properties is a common choice...

The initial version of JSONSchemaType did not support unions, so it should either had the problem you describe, or it would not be possible to use it for nullable fields.

As JSONSchemaType now supports unions, thanks to @erikbrinkman, it can be changed in the next major version to only require nullable: true in case the property is defined as p?: ... | null or p: ... | null

[erikbrinkman]

Duplicate of #1375

I'm broadly in agreement as this as it's a duplicate of my first issue. I don't really see how this is an issue with unions. In general I would expect the behavior to match the type, so required manages whether the field can be optional, and nullable handles whether it can be null. In ts terms it could technically also be undefined, but since that's not in the JSON spec it's somewhat irrelevant.

@swisener in the short term, if you're not doing any fancy JSONSchema validation, then JTD does distinguish between undefined and nullable. Alternatively you can just cast to fool typescript as the schema itself is still valid. An easy way if you have many optional fields is to declare a function<T>(schema: T): T & { nullable: true } => schema as T & { nullable: true } and just wrap each non-required field. It's inelegant, but it will work.

@epoberezkin As you already stated, the current behavior is intentional, because "most people" also want to check for null as missing. This seems counterintuitive from a typescript perspective as { a: number; b?: string | null } is not assignable to { a: number; b?: string }, potentially creating a hidden type error later on as you could do something like:

const valid = ajv.validate<{ a: number; b?: string }>(schema, obj);
if ("b" in valid) {
   valid.b.split(" ");  // TypeError: Cannot read property 'split' of null
}

What's the correct way to fix it that still preserves usability for many people who want to treat null as missing? I have two general ideas:

1. We could release another type transformer that just adds null onto optional properties. This would convert { a: number; b?: string } to { a: number; b?: string | null } so then the correct transform also does what people expect, and preserves typescript guarantees.
2. I'm not super familiar with advanced JSONSchema (or maybe ajv extensions), but I believe there's ways to modify json while validating it. It seems like an appropriate solution instead of forcing nullable to be true, would be an option, or global option to treat input nulls when the field is marked as optional but not nullable as omitted, and just to remove them. It seems like if this is an option (or the default?), then this provides all of the expected behaviors. In some instances obj.field will now be undefined instead of undefined | null but I can't think of many cases where that breaks something.

[epoberezkin]

@erikbrinkman i think the correct way to improve it in the next major version is to only require nullable: true if the property has null as possible value in typescript, independently of whether it’s required or optional. That will be a breaking change.

[erikbrinkman]

I looked into this, and there's one big problem with making the change. Currently SomeJSONSchema uses the trick of passing in a union of all types to attempt to produce a union of all valid schemas. The problem with this approach is that when changing how nullable is handled, it's not really feasible to change the current way that const and enum are handled. The hard part here is that if null is in the union it expects one type, and if it's not in the union it expects a different type. This works fine for real schemas, but doesn't work to get SomeJSONSchema to expand correctly.

I didn't write this originally, so I don't know all the ideas that went into the original design. My approach would be to fix it, and to redesign SomeJSONSchema from scratch, in the same way that SomeJTDSchema is. Does this seem like the right approach, or do you think it should be done differently?

[epoberezkin]

Not much thinking went into the original design I should admit - it was an experiment.

I don't mind it being re-done from scratch; the main question is whether it's a justified investment.

It would be worth looking at other points of confusion / limitations.

In any case it would require a major version change.

[erikbrinkman]

I should be clear that I think what needs to be redone from scratch is just SomeJSONSchema e.g. just a type for unchecked JSON Schemas. This should be independent of the actual implementation of JSONSchemaType which is why I think it's an easier change. It still won't be backwards compatible thought.

As a side note, it may fix: #1652 as I think that also stems from misalignment of SomeJSONSchma. When / if implementing this, it probably makes sense to add a test for that too, but I'm ultimately not sure if this will fix that.

[epoberezkin]

great!

[reggi]

Hey future traveler 👋

I just discovering this post and was about to give up, when I saw nullable: true and it worked!

Here's a clear example:

export const schema: JSONSchemaType<{ value: string, embed?: string, page?: string}> = {
    type: "object",
    properties: {
        value: {
            type: 'string',
        },
        embed: {
            type: 'string',
            nullable: true
        },
        page: {
            type: 'string',
            nullable: true
        },
    },
    required: ['value'],
    additionalProperties: false
}

[kilahm]

Hey future traveler 👋

I just discovering this post and was about to give up, when I saw nullable: true and it worked!

The problem with annotating a property with nullable: true is that allows for runtime values where the key exists and the value is null. This is different than the key being missing entirely or the value being undefined.

An example object that would pass validation of the schema you provided that I would not want to be valid:

{
  "value": "foo",
  "embed": null,
  "page": undefined
}

To say this another way, I want to be able to define a schema that says "if the embed key exists, the value must be a string". This type of rule is important when the object in question goes through some mapping process where Object.entries(([k, v]) => {}) is used.

Using your schema, the signature of the callback is:

([k, v]: [string, string | null | undefined]) => void

whereas I'd like to write a callback with a signature of

([k, v]: [string, string]) => void

[erikbrinkman]

@kilahm See my post previously. ajv does validate appropriately if you leave out the nullable, it's just the the type converter doesn't handle it. You can either wait until the next major version of ajv where it's fixed, or use my casting trick above to make it think the types work out, while still getting the validation you want.

[kilahm]

Yes, I'm aware that it's the type that's broken, not the validator runtime. 😄 I came here to check on the issue because I have a bunch of comments like the following in generated code throughout a few projects at work. Wanted to see if any new insights were had. I'm happily waiting for the next major version of ajv.

// schema should have type JSONSchemaType<Address>
// except Ajv doesn't model optional parameters entirely consistently
// @see https://github.com/ajv-validator/ajv/issues/1664
const schema = {

My comment above was in response to adding nullable: true to the schema and how that affects the runtime behavior of the validation in an undesirable (for me) way. At this point in time, I'll take the decrease in type safety with the JSON Schema object over a decrease in type safety in the validated runtime data.

[defunctzombie]

You can either wait until the next major version of ajv where it's fixed

Is there something we can follow to see when that happens? Would this be v9?

[defunctzombie]

I found https://github.com/ajv-validator/ajv/commit/b4b806fd03a9906e9126ad86cef233fa405c9a3e which is itself quite old at this point. Is this change not included because it is seen as a breaking change to the current typescript API?

[erikbrinkman]

Yes, this hasn't been landed yet because it's breaking, and hence why it's in the v9 branch. I can't speak for when v9 should be released.

[defunctzombie]

Yes, this hasn't been landed yet because it's breaking, and hence why it's in the v9 branch. I can't speak for when v9 should be released.

I've read this whole issue (and the referenced/related one) how is it breaking? It allows you to avoid using nullable: true in cases of only undefined but does it also mean that if you are using nullable: true you need to have | null on your types whereas before you did not?

I don't know what the appetite is for this but I think it could be nice to allow this new type behavior via an option. So if I specify some option to my validator like optionalUndefined: true (not the best name) then I am kicked into the new type codepath where I do not need to use nullable: true. This approach would allow releasing this feature on the v8 line and having users move over their validations opt-in.

[erikbrinkman]

This is breaking because it change the existing typechecks in a way that causes type errors where there wouldn't be before.

I think there's an argument for gradual migration, but I don't think it's very compelling. Gradual migration for types like this is really complicated. It's not about a flag in the validator, it'd be in constructing the type itself. The migration path would be more like creating two JSONSchemaTypes, and having validate work for either schema type. However this leaves a number of other problems:

1. The types already push the limit of what typescript wants to do, and in refactoring these types, it was not uncommon to run into errors related to type complexity
2. Since the migration path involves adding a new type, it extends the fix another generation, because you'd have to introduce the fix type in this version, then make them alias to the same type in 9, and then remove the fix type in 10, which is a lot of work for something that has been known about for two years #1375

However, there already exist a number of fixes for people who really care, and make the migration tradeoffs themselves:

1. use the wrapper types above to fool aspects of typescript to arrive at the correct behavior.
2. use the fixed type manually, and specify your own interfaces for validate. This should make migration for you to v9 (wrt to this problem) trivial, but puts the burden of migration on the libraries that care, rather than everyone else.

If none of this is compelling, you're always free to submit a PR with a proposed fix so we can further discuss the benefits versus costs of gradual migration.