JTDSchemaType null fixes

[erikbrinkman]

What issue does this pull request resolve?
After the last PR, I realized that the way JTDSchemaType handles nulls is wrong. Specifically, an empty schema with nullable doesn't match null, it matches everything. In other words, there's no way to validate a const null field with JTD. In fact, according to the spec { nullable: false } is a valid schema, but it would validate a null value, which makes me think it should be not allowed.

What changes did you make?

Moved the empty matching up a "level" in the schema, since it changes the behavior of nullable. I also removed Record<string, never> from matching an empty schema, because actually, an empty would validate anything, which would not necessarily be an empty record. As a result I also had to add an extra type check for refs. Omitting the ref type would allow { ref: string } to be a valid type since the keys of ref were still a string. As a result, I had to add an extra type guard to ensure they were only string literals.

Is there anything that requires more attention while reviewing?
Double check that my ideas about nullable and empty schema are correct. They seem like somewhat of an edge case, but it's important to get right.

Github doesn't seem to like stacked diffs, so it's showing an accepted but unlanded change too. You should just look at the most recent commit.

[epoberezkin]

I might be wrong but I think nullable: false is noop in all cases, so the empty form still should allow all values with or without nullable. @ucarion - is that correct?

[epoberezkin]

Possibly we should have prohibited nullable in empty form - it adds some confusion...

[epoberezkin]

@erikbrinkman I believe it is correct - there is the test case that specifically shows that nullable:false should be ignored.

[erikbrinkman]

What do you mean it should be ignored? That { nullable: false } should throw an error, or than in an empty schema the nullable keyword is ignored, and { nullable: false } still accepts everything? This is certainly an opinion, but that seems very counterintuitive to me.

Either way it only has one change that actually affects this diff. Currently the diff says that JTDSchemaType<unknown> maps to { nullable?: true, metadata?: ... } and if you really want to ignore nullable, then this just needs to change to { nullable?: boolean, metadata?: ... }. I still personally feel that this is not a great validator, but I'm okay with the change. Every other change in this diff is still necessary as before JTDSchemaType<null> mapped to { nullable: true, metadata?: ... } and it should map to never.

[ucarion]

nullable: false is always a no-op, correct. You can in all cases remove nullable: false, and end up with a valid schema that does exactly the same thing.

nullable is permitted in the empty form. It simply has no effect there.

[epoberezkin]

This is certainly an opinion, but that seems very counterintuitive to me.

that is how it is per RFC

[epoberezkin]

Possibly, ajv in strict mode should log the warning (because I agree - it is counterintuitive).

nullable can only be used to make it more general (by allowing null), not to narrow it down (by prohibiting null).

[erikbrinkman]

I see. It does make sense, that nullable: false is always a no-op. @ucarion is there a place for comment on the RFC? If nullable: false never has an effect then why include it in the spec? Shouldn't the spec itself only allow nullable?: true?

[ucarion]

@erikbrinkman I'll answer your broader question about feedback first, and then the specific question about nullable second.

You can send me feedback in whatever form is most convenient to you, I'm all ears. You can also bring up stuff to the json@ietf.org mailing list, if you want an audience with the JSON bigwigs. The RFC talks a bit more about the JTD experimentation process here:

https://www.rfc-editor.org/rfc/rfc8927.html#name-scope-of-experiment

At the end of the day, an RFC is an immutable document. But it can be obsoleted by another RFC, and your feedback can inform any follow-on RFC. Moreover, the documentation surrounding JTD can always be improved. One sorta-exception to the "RFCs are immutable" rule: if you find a typo or other error, we can submit an erratum for the RFC to the IETF. They show up here: https://www.rfc-editor.org/errata_search.php?rfc=8927 (currently there aren't any errata)

To your specific question about prohibiting nullable: false. I thought about it, but in a way it would just add a hoop to jump through for implementations (they now have to do one extra check on their nullable property), with no real impact on end-users. Plus, making nullable: false be equivalent to leaving out nullable entirely makes things work out nicely in a lot of programming languages, where JSON deserialization libraries will initialize a Boolean-typed field as false if the property is not included in the JSON message.

I suppose that the fact that this is legal could confuse people:

{ "type": "string", "nullable": false }

into thinking that if you were to remove the nullable in the above, that { "type": "string" } would allow null? I admit this is a downside, but I think this can be mostly avoided by convention/culture, where people just very rarely use nullable: false, and this corner case of the spec doesn't cross people's minds. Plus, as Evgeny suggested, a linter of schemas could always look at nullable: false and warn you that it's useless.

[epoberezkin]

@erikbrinkman should I close it or are there some fixes that are still needed (and PR needs to be amended)? Thank you!

[erikbrinkman]

@ucarion thanks for addressing both parts. I asked the first just because I didn't want to ask for random comments in a PR for AJV. I'm still not sure I love the syntax, but your arguments are compelling.

@epoberezkin this PR still addresses some issues with the way JTD schema was typing null / empty inputs. I just updated this to now allow nullable: false for unknown. It should be ready to review.

[epoberezkin]

@erikbrinkman it has conflicts now - possibly because I merged #1455 and then reverted?

[epoberezkin]

@erikbrinkman - sorry to chase - will you check the conflicts out so we can release it?

[erikbrinkman]

Sorry for the delay, it is probably because #1455 was reverted. Have you checked those tests in? I just rebased and it seems fine. Those types generally seemed unrelated, but I could be wrong.

As for why typescript isn't inferring the schema type properly, I'm not sure, but I'm looking into it.

[epoberezkin]

great - thank you!