Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RejectNullConverter PoC attempt #112

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<Compile Include="TypeSafeEnumConverter.fs" />
<Compile Include="RejectNullStringConverter.fs" />
<Compile Include="UnionOrTypeSafeEnumConverterFactory.fs" />
<Compile Include="RejectNullConverter.fs" />
<Compile Include="Options.fs" />
<Compile Include="Serdes.fs" />
<Compile Include="Codec.fs" />
Expand All @@ -22,7 +23,7 @@

<PackageReference Include="FSharp.Core" Version="4.5.4" />

<PackageReference Include="System.Text.Json" Version="6.0.1" />
<PackageReference Include="System.Text.Json" Version="8.0.1" />
</ItemGroup>

<ItemGroup>
Expand Down
6 changes: 5 additions & 1 deletion src/FsCodec.SystemTextJson/Options.fs
Original file line number Diff line number Diff line change
Expand Up @@ -60,14 +60,18 @@ type Options private () =
[<Optional; DefaultParameterValue(null)>] ?autoUnionToJsonObject: bool,
// Apply <c>RejectNullStringConverter</c> in order to have serialization throw on <c>null</c> strings.
// Use <c>string option</c> to represent strings that can potentially be <c>null</c>.
[<Optional; DefaultParameterValue(null)>] ?rejectNullStrings: bool) =
[<Optional; DefaultParameterValue(null)>] ?rejectNullStrings: bool,
// Apply <c>RejectNullConverter</c> in order to have serialization throw on <c>null</c> on <c>null</c> or missing <c>list</c> or <c>Set</c> values.
// Wrap the type in <c>option</c> to represent values that can potentially be <c>null</c> or missing
[<Optional; DefaultParameterValue(null)>] ?rejectNull: bool) =
let autoTypeSafeEnumToJsonString = defaultArg autoTypeSafeEnumToJsonString false
let autoUnionToJsonObject = defaultArg autoUnionToJsonObject false
let rejectNullStrings = defaultArg rejectNullStrings false

Options.CreateDefault(
converters = [|
if rejectNullStrings then RejectNullStringConverter()
if defaultArg rejectNull false then RejectNullConverterFactory()
if autoTypeSafeEnumToJsonString || autoUnionToJsonObject then
UnionOrTypeSafeEnumConverterFactory(typeSafeEnum = autoTypeSafeEnumToJsonString, union = autoUnionToJsonObject)
if converters <> null then yield! converters
Expand Down
36 changes: 36 additions & 0 deletions src/FsCodec.SystemTextJson/RejectNullConverter.fs
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
namespace FsCodec.SystemTextJson

open System
open System.Text.Json
open System.Text.Json.Serialization

type RejectNullConverter<'T>() =
inherit System.Text.Json.Serialization.JsonConverter<'T>()

let defaultConverter = JsonSerializerOptions.Default.GetConverter(typeof<'T>) :?> JsonConverter<'T>
let msg () = sprintf "Expected value, got null. When rejectNull is true you must explicitly wrap optional %s values in an 'option'" typeof<'T>.Name

override _.HandleNull = true

override _.Read(reader, typeToConvert, options) =
if reader.TokenType = JsonTokenType.Null then msg () |> nullArg else
// PROBLEM: Fails with NRE when RejectNullConverter delegates to Default list<int> Converter
// System.NullReferenceException
// at System.Text.Json.Serialization.JsonCollectionConverter`2.OnTryRead(Utf8JsonReader& reader, Type typeToConvert, JsonSerializerOptions options, ReadStack& state, TCollection& value)
// https://github.com/dotnet/runtime/issues/50205 via https://github.com/jet/FsCodec/pull/112#issuecomment-1907633798
defaultConverter.Read(&reader, typeToConvert, options)
// Pretty sure the above is the correct approach (and this unsurprisingly loops, blowing the stack)
// JsonSerializer.Deserialize(&reader, typeToConvert, options) :?> 'T

override _.Write(writer, value, options) =
if value |> box |> isNull then msg () |> nullArg
defaultConverter.Write(writer, value, options)
// JsonSerializer.Serialize<'T>(writer, value, options)

type RejectNullConverterFactory(predicate) =
inherit JsonConverterFactory()
static let isListOrSet (t: Type) = t.IsGenericType && let g = t.GetGenericTypeDefinition() in g = typedefof<Set<_>> || g = typedefof<list<_>>
new() = RejectNullConverterFactory(isListOrSet)

override _.CanConvert(t: Type) = predicate t
override _.CreateConverter(t, _options) = typedefof<RejectNullConverter<_>>.MakeGenericType(t).GetConstructors().[0].Invoke[||] :?> _
37 changes: 37 additions & 0 deletions tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ module FsCodec.SystemTextJson.Tests.SerdesTests

open System
open System.Collections.Generic
open System.Text.Json.Serialization.Metadata
open FsCodec.SystemTextJson
open Swensen.Unquote
open Xunit
Expand Down Expand Up @@ -79,6 +80,42 @@ let [<Fact>] ``RejectNullStringConverter rejects null strings`` () =
let value = { c = 1; d = null }
raises<ArgumentNullException> <@ serdes.Serialize value @>

type WithList = { x: int; y: list<int>; z: Set<int> }

let [<Fact>] ``RejectNullConverter rejects null lists and Sets`` () =
#if false // requires WithList to be CLIMutable, which would be a big imposition
let tir =
DefaultJsonTypeInfoResolver()
.WithAddedModifier(fun x ->
// if x.Kind <> JsonTypeInfoKind.Object then
for p in x.Properties do
let pt = p.PropertyType
if pt.IsGenericType && (let d = pt.GetGenericTypeDefinition() in d = typedefof<list<_>> || d = typedefof<Set<_>>) then
p.IsRequired <- true)
let serdes = Options.Create(TypeInfoResolver = tir) |> Serdes
raises<exn> <@ serdes.Deserialize<WithList> """{"x":0}""" @>
#else
let serdes = Options.Create(rejectNull = true) |> Serdes

// PROBLEM: Fails with NRE when RejectNullConverter delegates to Default list<int> Converter
// let res = serdes.Deserialize<WithList> """{"x":0,"y":[1]}"""
// test <@ [1] = res.y @>

// PROBLEM: Doesn't raise as Converter not called
raises<exn> <@ serdes.Deserialize<WithList> """{"x":0}""" @>

// PROBLEM: doesnt raise - there doesn't seem to be a way to intercept explicitly passed nulls
// raises<JsonException> <@ serdes.Deserialize<WithList> """{"x":0,"y":null}""" @>
#endif

#if false // I guess TypeShape is doing a reasonable thing not propagating
// PROBLEM: TypeShape.Generic.exists does not call the predicate if the list or set is `null`
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original workaround that came to mind was to walk the properties after the fact with TypeShape's Generic App, but I see I need to drop a level from that (as nulls are not passed to the predicate, and I want to do a generic check in any case, so disregard this piece for now...

let res = serdes.Deserialize<WithList> """{"x":0}"""
let hasNullList = TypeShape.Generic.exists (fun (x: int list) -> obj.ReferenceEquals(x, null))
let hasNullSet = TypeShape.Generic.exists (fun (x: Set<int>) -> obj.ReferenceEquals(x, null))
test <@ hasNullList res && hasNullSet res @>
#endif

let [<Fact>] ``RejectNullStringConverter serializes strings correctly`` () =
let serdes = Serdes(Options.Create(rejectNullStrings = true))
let value = { c = 1; d = "some string" }
Expand Down
Loading