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.

Sign up for GitHub

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

Jump to bottom

fix #164: NRE on deserializing missing field with JsonIgnoreCondition.WhenWritingNull #166

Merged
merged 3 commits into from
Jul 22, 2023
Merged

fix #164: NRE on deserializing missing field with JsonIgnoreCondition.WhenWritingNull #166

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c08a9e3
Add failing unit test demonstrating an issue
Jul 5, 2023
fca5a58
fix #164: NRE on deserializing missing field with JsonIgnoreCondition…
Tarmil Jul 7, 2023
7070c4b
fix formatting
Tarmil Jul 7, 2023
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
5 changes: 3 additions & 2 deletions src/FSharp.SystemTextJson/Record.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,8 @@ 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()
Expand Down Expand Up @@ -226,7 +227,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]
Expand Down
20 changes: 18 additions & 2 deletions tests/FSharp.SystemTextJson.Tests/Test.Record.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@ module NonStruct =
let actual = JsonSerializer.Deserialize """{"ax":1,"ay":"b"}"""
Assert.Equal({ ax = 1; ay = "b" }, actual)

[<Fact>]
let ``deserialize empty record with ignore-null-values on`` () =
let options =
JsonSerializerOptions(DefaultIgnoreCondition = Serialization.JsonIgnoreCondition.WhenWritingNull)
try
JsonSerializer.Deserialize<A>("{}", 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.
Copy link

@bartelink bartelink Jul 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

<@ JsonSerializer.Deserialize<A>("{}", options) |> ignore @> |> 
raisesWith<Exception>(fun e -> <@ e.Message.Contains("Missing field for record type" @>)  ()

or something similar.
edit: Pardon my assuming the world is using unquote...

let ex = Record.Exception(fun () -> JsonSerializer.Deserialize<A>("{}", options) |> ignore )
Assert.Contains("Missing field for record type", e.Message)

(or Assert.Throws<InvalidOperationException> or similar)
(I've just removed a heck of a lot of these catches from a test codebase, and can't bear the sight of them!)

Copy link
Owner Author

Choose a reason for hiding this comment

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

This is nicer indeed!

Also I actually use Unquote in most of my projects, I'm not even sure why this one doesn't but I might convert it some day.


[<Fact>]
let ``serialize via explicit converter`` () =
let actual = JsonSerializer.Serialize { ax = 1; ay = "b" }
Expand Down Expand Up @@ -291,10 +301,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)

[<AllowNullLiteral>]
Expand Down Expand Up @@ -754,10 +767,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)

[<AllowNullLiteral>]
Expand Down
Loading