Tarmil / FSharp.SystemTextJson

System.Text.Json extensions for F# types
MIT License
323 stars 44 forks source link

Disallow null if not option-wrapped everywhere, not just for record fields #158

Closed cmeeren closed 1 year ago

cmeeren commented 1 year ago

The document Customizing the serialization format says:

By default, FSharp.SystemTextJson throws an exception when the following conditions are met:

  • it is deserializing a record or a union;
  • a field's JSON value is null or the field is unspecified;
  • that field's type isn't an explicitly nullable F# type (like option, voption and Skippable).

What is the reason for the first requirement (emphasis mine)?

I ask because I was recently bitten by a bug in production. I am deserializing a JSON document to the type MyRootRecord, but when the JSON body itself is null, then the returned MyRootRecord is also null (even though it is a record and F# does not allow null values of it), causing some NullReferenceExecption elsewhere in the code.

I would expect that if I deserialize a top-level null value to a record/union (not wrapped in option or similar), then I get a deserialization exception similar to if it had been a record field.

In other words, the "throw on null" behavior should be governed by the target type, not by whether or not it's a property. (This could therefore also apply to array items, which I haven't tested.)

Tarmil commented 1 year ago

Good point, when the value itself is a record or union, we should be able to handle it. I know that by default System.Text.Json just returns null without calling converters when it encounters a null during deserialization, but it looks like we can override HandleNull in FSharp.STJ's converters to change this.

cmeeren commented 1 year ago

Happy that this is fixed! When will it be released?

cmeeren commented 1 year ago

Hi again, just wondering if it would be possible to create a new release with this change. 🙂 Otherwise I may have to work around it in other ways, which seems unnecessary since this is already implemented here, just not released yet.

Tarmil commented 12 months ago

Hi, sorry I'll have a release out soon 🙂

Tarmil commented 10 months ago

Published in v1.2.

cmeeren commented 10 months ago

Thanks a lot!