From c08a9e31845c0cadb79f610a8b33932ed56644b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Melvyn=20La=C3=AFly?= Date: Wed, 5 Jul 2023 11:56:43 +0200 Subject: [PATCH 1/3] Add failing unit test demonstrating an issue The record converter throws a NRE when trying to deserialize an empty json object, if the ignore nulls option is enabled. --- tests/FSharp.SystemTextJson.Tests/Test.Record.fs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs index 0b760b0..25a4ee9 100644 --- a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs +++ b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs @@ -14,6 +14,15 @@ module NonStruct = let actual = JsonSerializer.Deserialize """{"ax":1,"ay":"b"}""" Assert.Equal({ ax = 1; ay = "b" }, actual) + [] + let ``deserialize empty record with ignore-null-values on`` () = + let options = JsonSerializerOptions(DefaultIgnoreCondition = Serialization.JsonIgnoreCondition.WhenWritingNull) + try + JsonSerializer.Deserialize("{}", options) |> ignore + with + | :? System.NullReferenceException -> failwith "Unexpected NRE." + | ex when ex.Message.Contains("Missing field for record type") -> () // It's expected to fail since the record requires its fields to be initialized. + [] let ``serialize via explicit converter`` () = let actual = JsonSerializer.Serialize { ax = 1; ay = "b" } From fca5a5891b347ce3a3b5c013e4ab615d9817883f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Denuzi=C3=A8re?= Date: Fri, 7 Jul 2023 18:36:14 +0200 Subject: [PATCH 2/3] fix #164: NRE on deserializing missing field with JsonIgnoreCondition.WhenWritingNull --- src/FSharp.SystemTextJson/Record.fs | 4 ++-- tests/FSharp.SystemTextJson.Tests/Test.Record.fs | 10 ++++++++-- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/FSharp.SystemTextJson/Record.fs b/src/FSharp.SystemTextJson/Record.fs index 9585a43..94e14b9 100644 --- a/src/FSharp.SystemTextJson/Record.fs +++ b/src/FSharp.SystemTextJson/Record.fs @@ -27,7 +27,7 @@ type private RecordField let isSkippableType = isSkippableType fsOptions p.PropertyType - let canBeSkipped = ignore || ignoreNullValues options || isSkippableType + let canBeSkipped = ignore || (ignoreNullValues options && nullValue.IsSome) || isSkippableType let read = let m = p.GetGetMethod() @@ -226,7 +226,7 @@ type JsonRecordConverter<'T> internal (options: JsonSerializerOptions, fsOptions | _ -> reader.Skip() | _ -> () - if requiredFieldCount < minExpectedFieldCount && not (ignoreNullValues options) then + if requiredFieldCount < minExpectedFieldCount then for i in 0 .. fieldCount - 1 do if isNull fields[i] && fieldProps[i].MustBePresent then failf "Missing field for record type %s: %s" recordType.FullName fieldProps[i].Names[0] diff --git a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs index 25a4ee9..18673af 100644 --- a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs +++ b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs @@ -300,10 +300,13 @@ module NonStruct = Assert.Equal("""{"unignoredX":1}""", actual) let ignoreNullOptions = - JsonFSharpOptions().ToJsonSerializerOptions(IgnoreNullValues = true) + JsonFSharpOptions() + .WithAllowNullFields() + .ToJsonSerializerOptions(IgnoreNullValues = true) let newIgnoreNullOptions = JsonFSharpOptions() + .WithAllowNullFields() .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull) [] @@ -763,10 +766,13 @@ module Struct = Assert.Equal("""{"unignoredX":1}""", actual) let ignoreNullOptions = - JsonFSharpOptions().ToJsonSerializerOptions(IgnoreNullValues = true) + JsonFSharpOptions() + .WithAllowNullFields() + .ToJsonSerializerOptions(IgnoreNullValues = true) let newIgnoreNullOptions = JsonFSharpOptions() + .WithAllowNullFields() .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull) [] From 7070c4bb6372098280f2cfe7b0507555ff99f320 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lo=C3=AFc=20Denuzi=C3=A8re?= Date: Fri, 7 Jul 2023 18:43:16 +0200 Subject: [PATCH 3/3] fix formatting --- src/FSharp.SystemTextJson/Record.fs | 3 ++- tests/FSharp.SystemTextJson.Tests/Test.Record.fs | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/FSharp.SystemTextJson/Record.fs b/src/FSharp.SystemTextJson/Record.fs index 94e14b9..6927bee 100644 --- a/src/FSharp.SystemTextJson/Record.fs +++ b/src/FSharp.SystemTextJson/Record.fs @@ -27,7 +27,8 @@ type private RecordField let isSkippableType = isSkippableType fsOptions p.PropertyType - let canBeSkipped = ignore || (ignoreNullValues options && nullValue.IsSome) || isSkippableType + let canBeSkipped = + ignore || (ignoreNullValues options && nullValue.IsSome) || isSkippableType let read = let m = p.GetGetMethod() diff --git a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs index 18673af..f5370c8 100644 --- a/tests/FSharp.SystemTextJson.Tests/Test.Record.fs +++ b/tests/FSharp.SystemTextJson.Tests/Test.Record.fs @@ -16,12 +16,13 @@ module NonStruct = [] let ``deserialize empty record with ignore-null-values on`` () = - let options = JsonSerializerOptions(DefaultIgnoreCondition = Serialization.JsonIgnoreCondition.WhenWritingNull) + let options = + JsonSerializerOptions(DefaultIgnoreCondition = Serialization.JsonIgnoreCondition.WhenWritingNull) try JsonSerializer.Deserialize("{}", options) |> ignore with - | :? System.NullReferenceException -> failwith "Unexpected NRE." - | ex when ex.Message.Contains("Missing field for record type") -> () // It's expected to fail since the record requires its fields to be initialized. + | :? System.NullReferenceException -> failwith "Unexpected NRE." + | ex when ex.Message.Contains("Missing field for record type") -> () // It's expected to fail since the record requires its fields to be initialized. [] let ``serialize via explicit converter`` () =