From ce69a5a63bef19ea62d82168dad05183c1acf3fb Mon Sep 17 00:00:00 2001 From: Ruben Bartelink Date: Wed, 24 Jan 2024 00:50:31 +0000 Subject: [PATCH 1/3] RejectNullConverter WIP --- .../FsCodec.SystemTextJson.fsproj | 3 +- src/FsCodec.SystemTextJson/Options.fs | 6 ++- .../RejectNullConverter.fs | 42 +++++++++++++++++++ .../SerdesTests.fs | 27 ++++++++++++ 4 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 src/FsCodec.SystemTextJson/RejectNullConverter.fs diff --git a/src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj b/src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj index e221a36..9af7a97 100644 --- a/src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj +++ b/src/FsCodec.SystemTextJson/FsCodec.SystemTextJson.fsproj @@ -10,6 +10,7 @@ + @@ -22,7 +23,7 @@ - + diff --git a/src/FsCodec.SystemTextJson/Options.fs b/src/FsCodec.SystemTextJson/Options.fs index 288de0c..37e335d 100755 --- a/src/FsCodec.SystemTextJson/Options.fs +++ b/src/FsCodec.SystemTextJson/Options.fs @@ -60,7 +60,10 @@ type Options private () = [] ?autoUnionToJsonObject: bool, // Apply RejectNullStringConverter in order to have serialization throw on null strings. // Use string option to represent strings that can potentially be null. - [] ?rejectNullStrings: bool) = + [] ?rejectNullStrings: bool, + // Apply RejectNullConverter in order to have serialization throw on null on null or missing list or Set values. + // Wrap the type in option to represent values that can potentially be null or missing + [] ?rejectNull: bool) = let autoTypeSafeEnumToJsonString = defaultArg autoTypeSafeEnumToJsonString false let autoUnionToJsonObject = defaultArg autoUnionToJsonObject false let rejectNullStrings = defaultArg rejectNullStrings false @@ -68,6 +71,7 @@ type Options private () = 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 diff --git a/src/FsCodec.SystemTextJson/RejectNullConverter.fs b/src/FsCodec.SystemTextJson/RejectNullConverter.fs new file mode 100644 index 0000000..b81dfbd --- /dev/null +++ b/src/FsCodec.SystemTextJson/RejectNullConverter.fs @@ -0,0 +1,42 @@ +namespace FsCodec.SystemTextJson + +open System +open System.Linq.Expressions +open System.Text.Json +open System.Text.Json.Serialization + +type RejectNullConverter<'T>() = + inherit System.Text.Json.Serialization.JsonConverter<'T>() + + static 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 + 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() + new() = + RejectNullConverterFactory(fun (t: Type) -> + t.IsGenericType + && let gtd = t.GetGenericTypeDefinition() in gtd = typedefof> || gtd = typedefof>) + override _.CanConvert(t: Type) = predicate t + + override _.CreateConverter(t, _options) = + let openConverterType = typedefof> + let constructor = openConverterType.MakeGenericType(t).GetConstructors() |> Array.head + let newExpression = Expression.New(constructor) + let lambda = Expression.Lambda(typeof, newExpression) + + let activator = lambda.Compile() :?> ConverterActivator + activator.Invoke() diff --git a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs index 017a153..760cebb 100644 --- a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs +++ b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs @@ -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 @@ -79,6 +80,32 @@ let [] ``RejectNullStringConverter rejects null strings`` () = let value = { c = 1; d = null } raises <@ serdes.Serialize value @> +type WithList = { x: int; y: list } + +let [] ``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 gtd = pt.GetGenericTypeDefinition() in gtd = typedefof> || gtd = typedefof>) then + p.IsRequired <- true) + let serdes = Options.Create(TypeInfoResolver = tir) |> Serdes +#else + let serdes = Options.Create(rejectNull = true) |> Serdes +#endif + + // Fails with NRE when RejectNullConverter delegates to Default list Converter + // seems akin to https://github.com/dotnet/runtime/issues/86483 + let res = serdes.Deserialize """{"x":0,"y":[1]}""" + test <@ [1] = res.y @> + + raises <@ serdes.Deserialize """{"x":0}""" @> + // PROBLEM: there doesn't seem to be a way to intercept explicitly passed nulls + // raises <@ serdes.Deserialize """{"x":0,"y":null}""" @> + let [] ``RejectNullStringConverter serializes strings correctly`` () = let serdes = Serdes(Options.Create(rejectNullStrings = true)) let value = { c = 1; d = "some string" } From 697cbf8b1f647e6ca088287aee88f2261a7de93c Mon Sep 17 00:00:00 2001 From: Ruben Bartelink Date: Wed, 24 Jan 2024 02:01:25 +0000 Subject: [PATCH 2/3] Polish tests --- .../SerdesTests.fs | 20 ++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs index 760cebb..bf7540e 100644 --- a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs +++ b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs @@ -80,7 +80,7 @@ let [] ``RejectNullStringConverter rejects null strings`` () = let value = { c = 1; d = null } raises <@ serdes.Serialize value @> -type WithList = { x: int; y: list } +type WithList = { x: int; y: list; z: Set } let [] ``RejectNullConverter rejects null lists and Sets`` () = #if false // requires WithList to be CLIMutable, which would be a big imposition @@ -93,18 +93,28 @@ let [] ``RejectNullConverter rejects null lists and Sets`` () = if pt.IsGenericType && (let gtd = pt.GetGenericTypeDefinition() in gtd = typedefof> || gtd = typedefof>) then p.IsRequired <- true) let serdes = Options.Create(TypeInfoResolver = tir) |> Serdes + raises <@ serdes.Deserialize """{"x":0}""" @> #else let serdes = Options.Create(rejectNull = true) |> Serdes -#endif // Fails with NRE when RejectNullConverter delegates to Default list Converter // seems akin to https://github.com/dotnet/runtime/issues/86483 - let res = serdes.Deserialize """{"x":0,"y":[1]}""" - test <@ [1] = res.y @> + // let res = serdes.Deserialize """{"x":0,"y":[1]}""" + // test <@ [1] = res.y @> + // PROBLEM: Doesn't raise raises <@ serdes.Deserialize """{"x":0}""" @> - // PROBLEM: there doesn't seem to be a way to intercept explicitly passed nulls + // PROBLEM: doesnt raise - there doesn't seem to be a way to intercept explicitly passed nulls // raises <@ serdes.Deserialize """{"x":0,"y":null}""" @> +#endif + +#if false // I guess TypeShape is doing a reasaonable thing not propagating + // PROBLEM: TypeShape.Generic.exists does not call the predicate if the list or set is `null` + let res = serdes.Deserialize """{"x":0}""" + let hasNullList = TypeShape.Generic.exists (fun (x: int list) -> obj.ReferenceEquals(x, null)) + let hasNullSet = TypeShape.Generic.exists (fun (x: Set) -> obj.ReferenceEquals(x, null)) + test <@ hasNullList res && hasNullSet res @> +#endif let [] ``RejectNullStringConverter serializes strings correctly`` () = let serdes = Serdes(Options.Create(rejectNullStrings = true)) From 216cdd9c3c8030ceb7a4aeccdf04e077bb99f092 Mon Sep 17 00:00:00 2001 From: Ruben Bartelink Date: Wed, 24 Jan 2024 09:01:14 +0000 Subject: [PATCH 3/3] Feedback updates/integration --- .../RejectNullConverter.fs | 24 +++++++------------ .../SerdesTests.fs | 10 ++++---- 2 files changed, 14 insertions(+), 20 deletions(-) diff --git a/src/FsCodec.SystemTextJson/RejectNullConverter.fs b/src/FsCodec.SystemTextJson/RejectNullConverter.fs index b81dfbd..7fd63cd 100644 --- a/src/FsCodec.SystemTextJson/RejectNullConverter.fs +++ b/src/FsCodec.SystemTextJson/RejectNullConverter.fs @@ -1,20 +1,23 @@ namespace FsCodec.SystemTextJson open System -open System.Linq.Expressions open System.Text.Json open System.Text.Json.Serialization type RejectNullConverter<'T>() = inherit System.Text.Json.Serialization.JsonConverter<'T>() - static let defaultConverter = JsonSerializerOptions.Default.GetConverter(typeof<'T>) :?> 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 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 @@ -26,17 +29,8 @@ type RejectNullConverter<'T>() = type RejectNullConverterFactory(predicate) = inherit JsonConverterFactory() - new() = - RejectNullConverterFactory(fun (t: Type) -> - t.IsGenericType - && let gtd = t.GetGenericTypeDefinition() in gtd = typedefof> || gtd = typedefof>) - override _.CanConvert(t: Type) = predicate t - - override _.CreateConverter(t, _options) = - let openConverterType = typedefof> - let constructor = openConverterType.MakeGenericType(t).GetConstructors() |> Array.head - let newExpression = Expression.New(constructor) - let lambda = Expression.Lambda(typeof, newExpression) + static let isListOrSet (t: Type) = t.IsGenericType && let g = t.GetGenericTypeDefinition() in g = typedefof> || g = typedefof> + new() = RejectNullConverterFactory(isListOrSet) - let activator = lambda.Compile() :?> ConverterActivator - activator.Invoke() + override _.CanConvert(t: Type) = predicate t + override _.CreateConverter(t, _options) = typedefof>.MakeGenericType(t).GetConstructors().[0].Invoke[||] :?> _ diff --git a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs index bf7540e..64e4cfd 100644 --- a/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs +++ b/tests/FsCodec.SystemTextJson.Tests/SerdesTests.fs @@ -90,25 +90,25 @@ let [] ``RejectNullConverter rejects null lists and Sets`` () = // if x.Kind <> JsonTypeInfoKind.Object then for p in x.Properties do let pt = p.PropertyType - if pt.IsGenericType && (let gtd = pt.GetGenericTypeDefinition() in gtd = typedefof> || gtd = typedefof>) then + if pt.IsGenericType && (let d = pt.GetGenericTypeDefinition() in d = typedefof> || d = typedefof>) then p.IsRequired <- true) let serdes = Options.Create(TypeInfoResolver = tir) |> Serdes raises <@ serdes.Deserialize """{"x":0}""" @> #else let serdes = Options.Create(rejectNull = true) |> Serdes - // Fails with NRE when RejectNullConverter delegates to Default list Converter - // seems akin to https://github.com/dotnet/runtime/issues/86483 + // PROBLEM: Fails with NRE when RejectNullConverter delegates to Default list Converter // let res = serdes.Deserialize """{"x":0,"y":[1]}""" // test <@ [1] = res.y @> - // PROBLEM: Doesn't raise + // PROBLEM: Doesn't raise as Converter not called raises <@ serdes.Deserialize """{"x":0}""" @> + // PROBLEM: doesnt raise - there doesn't seem to be a way to intercept explicitly passed nulls // raises <@ serdes.Deserialize """{"x":0,"y":null}""" @> #endif -#if false // I guess TypeShape is doing a reasaonable thing not propagating +#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` let res = serdes.Deserialize """{"x":0}""" let hasNullList = TypeShape.Generic.exists (fun (x: int list) -> obj.ReferenceEquals(x, null))