[#123] Fix: reject absent field of type option unless IgnoreNullValues

Tarmil · Jul 9, 2022 · a4893db · a4893db
1 parent e8a5732
commit a4893db
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 7 deletions.
diff --git a/src/FSharp.SystemTextJson/Helpers.fs b/src/FSharp.SystemTextJson/Helpers.fs
@@ -108,9 +108,6 @@ let rec tryGetNullValue (fsOptions: JsonFSharpOptions) (ty: Type) : obj voption
 let rec isNullableFieldType (fsOptions: JsonFSharpOptions) (ty: Type) =
     tryGetNullValue fsOptions ty |> ValueOption.isSome
 
-let isSkippableFieldType (fsOptions: JsonFSharpOptions) (ty: Type) =
-    isNullableFieldType fsOptions ty || isSkippableType ty
-
 let overrideOptions (ty: Type) (defaultOptions: JsonFSharpOptions) (overrides: IDictionary<Type, JsonFSharpOptions>) =
     let inheritUnionEncoding (options: JsonFSharpOptions) =
         if options.UnionEncoding.HasFlag(JsonUnionEncoding.Inherit) then

diff --git a/src/FSharp.SystemTextJson/Record.fs b/src/FSharp.SystemTextJson/Record.fs
@@ -76,9 +76,7 @@ type JsonRecordConverter<'T>(options: JsonSerializerOptions, fsOptions: JsonFSha
                 p.GetCustomAttributes(typeof<JsonIgnoreAttribute>, true) |> Array.isEmpty |> not
             let nullValue = tryGetNullValue fsOptions p.PropertyType
             let canBeSkipped =
-                ignore
-                || ignoreNullValues options
-                || isSkippableFieldType fsOptions p.PropertyType
+                ignore || ignoreNullValues options || isSkippableType p.PropertyType
             let read =
                 let m = p.GetGetMethod()
                 fun o -> m.Invoke(o, Array.empty)

diff --git a/src/FSharp.SystemTextJson/Union.fs b/src/FSharp.SystemTextJson/Union.fs
@@ -90,6 +90,7 @@ type JsonUnionConverter<'T>
                             name
                         else
                             name + string nameIndex
+                    let canBeSkipped = ignoreNullValues options || isSkippableType p.PropertyType
                     { Type = p.PropertyType
                       Name =
                         let policy =
@@ -100,7 +101,7 @@ type JsonUnionConverter<'T>
                         | null -> name
                         | policy -> policy.ConvertName name
                       NullValue = tryGetNullValue fsOptions p.PropertyType
-                      MustBePresent = not (isSkippableFieldType fsOptions p.PropertyType)
+                      MustBePresent = not canBeSkipped
                       IsSkip = isSkip p.PropertyType }
                 )
             let fieldsByName =

diff --git a/tests/FSharp.SystemTextJson.Tests/Test.Regression.fs b/tests/FSharp.SystemTextJson.Tests/Test.Regression.fs
@@ -63,3 +63,39 @@ let ``regression #106`` () =
     let options = JsonSerializerOptions()
     options.Converters.Add(JsonFSharpConverter())
     Assert.Equal({ MyUnion = MyUnion ValueNone }, JsonSerializer.Deserialize("""{"MyUnion":null}""", options))
+
+type Person = { FirstName: string; LastName: string option; age: int }
+
+[<Fact>]
+let ``regression #123`` () =
+    let fsOptions =
+        JsonFSharpConverter(
+            JsonUnionEncoding.UnwrapSingleCaseUnions
+            ||| JsonUnionEncoding.UnwrapOption
+            ||| JsonUnionEncoding.NamedFields
+            ||| JsonUnionEncoding.UnwrapFieldlessTags
+        )
+
+    let noSkipOptions = JsonSerializerOptions()
+    noSkipOptions.Converters.Add(fsOptions)
+    let ex =
+        Assert.Throws<JsonException>(fun () ->
+            JsonSerializer.Deserialize<Person>("""{"FirstName": "yarr", "age": 5 }""", noSkipOptions)
+            |> ignore
+        )
+    Assert.Equal("Missing field for record type Tests.Regression+Person: LastName", ex.Message)
+
+    let skipOptions =
+        JsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
+    skipOptions.Converters.Add(fsOptions)
+    Assert.Equal(
+        { FirstName = "yarr"; LastName = None; age = 5 },
+        JsonSerializer.Deserialize<Person>("""{"FirstName": "yarr", "age": 5 }""", skipOptions)
+    )
+
+    let skipOptions2 = JsonSerializerOptions(IgnoreNullValues = true)
+    skipOptions2.Converters.Add(fsOptions)
+    Assert.Equal(
+        { FirstName = "yarr"; LastName = None; age = 5 },
+        JsonSerializer.Deserialize<Person>("""{"FirstName": "yarr", "age": 5 }""", skipOptions2)
+    )