search

jet / FsCodec

F# Event-Union Contract Encoding with versioning tolerant converters supporting System.Text.Json and Newtonsoft.Json
https://github.com/jet/dotnet-templates
Apache License 2.0
83 stars 19 forks source link

Deserialization of null to non-optional Record property #65

Closed kevingentile closed 3 years ago

kevingentile commented 3 years ago

Refers to the test case in https://github.com/jet/FsCodec/pull/64

I'm seeing a null value where I would expect an initialized/zeroed value for the property.

Possibly related to https://github.com/JamesNK/Newtonsoft.Json/issues/2116

bartelink commented 3 years ago

Thanks for logging an issue, and taking the time to add a PR too. In this instance I'm not sure the best answer is to actually change the behavior of FsCodec; I'll try to explain why...

String can legitimately have null as a value. You have two choices:

  1. use string option to represent the null
  2. apply json.net rules to outlaw null for that field or in general
    • the Settings.Create has an overload for allowNulls which may or may not help
    • json.net has attributes to change the default for a type to outlaw nulls, which you can then provide exceptions to by tagging individual properties/fields

See https://github.com/jet/equinox/blob/master/src/Equinox.CosmosStore/CosmosStore.fs#L22 (I leave it in default mode but outlaw nulls that I am not handling intentionally)

Would appreciate if the PR could add a paragraph to the README to explain this technique / convention when using json.net (having a characterization test or two illustrating it too would be helpful too but from my perspective those tests should not be failing - i.e. this is by design, unless I'm really missing something)

bartelink commented 3 years ago

It should also be mentioned that FsCodec.SystemTextJson should be providing an analogous mechanism. I'm not sure how easy/possible that is, but in general FsCodec is seeking to provide a pragmatic middle way of using NSJ or STJ in F# without completely changing what one might expect to happen when using json.net in order to provide an F# only experience - i.e. the aim is to provide helpers to smooth the way for using reflection based serialization in a way that would not surprise people coming from a C# background and/or in mixed C#+F# codebases.

I hope this explains the intent a little better than the above

bartelink commented 3 years ago

@kevingentile Does this make sense to you? Can you please close the issue and PR if so and/or provide a PR that updates the README to better express the design in a manner that explains this in a way which would have saved you the confusion?

kevingentile commented 3 years ago

Hey @bartelink, apologies for the delay. This behavior makes sense and I'll update the PR.