Enforce no nulls in record types unless type is FSharpOption

[drhumlen]

There should probably be check on [<AllowNullLiteral>](?) for the given type on the off chance somebody actually wants nulls in their F# record type.

[cmeeren]

I would perhaps improve the error message, saying that it's not nullable or allowed to be null, instead of not "optional". Nullability is a different concern than optionality (i.e. whether or not it's present). Differentiating between these is needed for e.g. JSON:API implementations.

[drhumlen]

Yeah, I agree. I think the non-option(al) feedback was more targeted at the F# developer than the API-consumer/user – since F# developers will understand that it is the option type that is missing.

I will change it so it is more understandable to frontenders by using the word null

I was also thinking that the error message is the same for a missing field and a field with a null. But that depedends on the ignoreNulls config(?).

[drhumlen]

@cmeeren What do you think about the error message output now? What would be ideal error message in this case? We have the chance to make an excellent error messsage here.

[drhumlen]

I'm a little worried @Tarmil is on a holiday or something at the moment. Maybe this repo should have a backup maintainer to merge PRs if one person is away..? 🤔 This could become the goto F# library for json.

[cmeeren]

@cmeeren What do you think about the error message output now? What would be ideal error message in this case? We have the chance to make an excellent error messsage here.

I have no idea, to be honest. It all depends on who the message is for - front-end (API clients) or back-end devs. I don't want to meddle too much in that; I'll leave it up to @Tarmil.

[Tarmil]

Thanks for the PR! Not on vacation, just a bit busy :)

I think the error message as it is is good, maybe it could just mention the type of the field too.

Now, for allowing some nulls:

• At a minimum, we need to check whether the field type is a union with CompilationRepresentationFlags.UseNullAsTrueValue, because ignoring it would straight up break idiomatic F# code. You can just take usesNull from the union converter and move it to Helpers. And since option has this flag, that check can even replace the type check for FSharpOption.

• Allowing nulls on types with AllowNullLiteral would probably be a good idea too, as you suggested.

• Finally, maybe we can add an explicit escape hatch in the form of an attribute on the field itself (rather than its type)? I think we can do without for now, but it's something to keep in mind for the future.

I also think that these checks should be precomputed and stored in RecordProperty, so that actual deserialization just has to test whether fieldProps.[i].CanBeNull.

(Also a small optimization: isNull x is faster than x = null)

[drhumlen]

I tried to add a test for this, but got this error. This means checking for the attribute [<AllowNullLiteral>] is redundant, right? 🤔

[drhumlen]

Perhaps we can remove this?

[drhumlen]

Is this a reasonable name?

[Tarmil]

AllowNullLiteral would actually be on the type of the field, not of the record itself. And you're right, it can't be applied on a record, only on a class. But even for a class it's not really a good indication of whether a null can be expected: if a class is defined in F#, then yes, it can be null iff it has AllowNullLiteral attribute; but if a class is defined in C#, then it won't have that attribute regardless of nullability. So I think we should drop the test, and add a flag in the options to bypass this check for people who really need null classes.

[drhumlen]

🎉 😄