Allow non-skippable option fields regardless of DefaultIgnoreCondition

src/FSharp.SystemTextJson/All.fs

@@ -130,7 +130,7 @@ type JsonFSharpConverterAttribute(fsOptions: JsonFSharpOptions) =
     member _.UnionFieldNamesFromTypes
         with set v = fsOptions <- fsOptions.WithUnionFieldNamesFromTypes(v)
     member _.SkippableOptionFields
-        with set v = fsOptions <- fsOptions.WithSkippableOptionFields(v)
+        with set v = fsOptions <- fsOptions.WithSkippableOptionFields(v: SkippableOptionFields)
 
     new() = JsonFSharpConverterAttribute(JsonFSharpOptions())
 

src/FSharp.SystemTextJson/Helpers.fs

@@ -38,12 +38,17 @@ let isNullableUnion (ty: Type) =
         x.Flags.HasFlag(CompilationRepresentationFlags.UseNullAsTrueValue)
     )
 
+let isOptionType (ty: Type) =
+    ty.IsGenericType
+    && let genTy = ty.GetGenericTypeDefinition() in
+       genTy = typedefof<option<_>> || genTy = typedefof<voption<_>>
+
 let isSkippableType (fsOptions: JsonFSharpOptionsRecord) (ty: Type) =
     if ty.IsGenericType then
         let genTy = ty.GetGenericTypeDefinition()
         genTy = typedefof<Skippable<_>>
-        || (fsOptions.SkippableOptionFields
-            && (genTy = typedefof<option<_>> || genTy = typedefof<voption<_>>))
+        || (fsOptions.SkippableOptionFields = SkippableOptionFields.Always
+            && isOptionType ty)
     else
         false
 
@@ -146,7 +151,12 @@ type FieldHelper
         options.IgnoreNullValues
         || options.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull
     let canBeSkipped =
-        (ignoreNullValues && (nullValue.IsSome || isClass ty)) || isSkippableWrapperType
+        if isOptionType ty then
+            (fsOptions.SkippableOptionFields = SkippableOptionFields.Always)
+            || (ignoreNullValues
+                && fsOptions.SkippableOptionFields = SkippableOptionFields.FromJsonSerializerOptions)
+        else
+            (ignoreNullValues && (nullValue.IsSome || isClass ty)) || isSkippableWrapperType
     let deserializeType =
         if isSkippableWrapperType then ty.GenericTypeArguments[0] else ty
 
@@ -166,7 +176,7 @@ type FieldHelper
             fun _ -> false
 
     let ignoreOnWrite (v: obj) =
-        isSkip v || (ignoreNullValues && isNull v)
+        canBeSkipped && (isSkip v || isNull v)
 
     let defaultValue =
         if isSkippableWrapperType || isValueOptionType ty then

src/FSharp.SystemTextJson/Options.fs

@@ -120,6 +120,14 @@ type JsonFSharpTypes =
     /// All supported types.
     | All = 0xfff
 
+type SkippableOptionFields =
+    /// None and ValueNone fields in records and unions are skippable if the JsonSerializerOptions has DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull.
+    | FromJsonSerializerOptions = 0
+    /// None and ValueNone fields in records and unions are never skippable.
+    | Never = 1
+    /// None and ValueNone fields in records and unions are always skippable.
+    | Always = 2
+
 module internal Default =
 
     [<Literal>]
@@ -158,7 +166,7 @@ type internal JsonFSharpOptionsRecord =
       UnionTagCaseInsensitive: bool
       AllowNullFields: bool
       IncludeRecordProperties: bool
-      SkippableOptionFields: bool
+      SkippableOptionFields: SkippableOptionFields
       Types: JsonFSharpTypes
       AllowOverride: bool
       Overrides: JsonFSharpOptions -> IDictionary<Type, JsonFSharpOptions> }
@@ -191,7 +199,7 @@ and JsonFSharpOptions internal (options: JsonFSharpOptionsRecord) =
               UnionTagCaseInsensitive = unionTagCaseInsensitive
               AllowNullFields = allowNullFields
               IncludeRecordProperties = includeRecordProperties
-              SkippableOptionFields = false
+              SkippableOptionFields = SkippableOptionFields.FromJsonSerializerOptions
               Types = types
               AllowOverride = allowOverride
               Overrides = emptyOverrides }
@@ -262,17 +270,25 @@ and JsonFSharpOptions internal (options: JsonFSharpOptionsRecord) =
     member _.WithIncludeRecordProperties([<Optional; DefaultParameterValue true>] includeRecordProperties) =
         JsonFSharpOptions({ options with IncludeRecordProperties = includeRecordProperties })
 
-    member _.WithSkippableOptionFields([<Optional; DefaultParameterValue true>] skippableOptionFields) =
+    member _.WithSkippableOptionFields(skippableOptionFields) =
         JsonFSharpOptions(
             { options with
                 SkippableOptionFields = skippableOptionFields
                 UnionEncoding =
-                    if skippableOptionFields then
+                    if skippableOptionFields = SkippableOptionFields.Always then
                         options.UnionEncoding ||| JsonUnionEncoding.UnwrapOption
                     else
                         options.UnionEncoding }
         )
 
+
+    member this.WithSkippableOptionFields([<Optional; DefaultParameterValue true>] skippableOptionFields) =
+        if skippableOptionFields then
+            SkippableOptionFields.Always
+        else
+            SkippableOptionFields.FromJsonSerializerOptions
+        |> this.WithSkippableOptionFields
+
     member _.WithTypes(types) =
         JsonFSharpOptions({ options with Types = types })
 

src/FSharp.SystemTextJson/Record.fs

@@ -199,7 +199,7 @@ type JsonRecordConverter<'T> internal (options: JsonSerializerOptions, fsOptions
 
         if requiredFieldCount < minExpectedFieldCount then
             for i in 0 .. fieldCount - 1 do
-                if isNull fields[i] && fieldProps[i].MustBePresent then
+                if fields[i] = defaultFields[i] && fieldProps[i].MustBePresent then
                     failf "Missing field for record type %s: %s" recordType.FullName fieldProps[i].Names[0]
 
         ctor fields :?> 'T

tests/FSharp.SystemTextJson.Tests/Test.Record.fs

@@ -223,6 +223,39 @@ module NonStruct =
             )
         Assert.Equal("""{"sa":1,"sb":2,"sc":3,"sd":4}""", actual)
 
+    type NonSkO = { x: int option }
+
+    [<Fact>]
+    let ``deserialize non-skippable option field even with WhenWritingNull`` () =
+        let options =
+            JsonFSharpOptions
+                .Default()
+                .WithSkippableOptionFields(SkippableOptionFields.Never)
+                .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
+
+        let actual = JsonSerializer.Deserialize("""{"x":1}""", options)
+        Assert.Equal({ x = Some 1 }, actual)
+
+        let actual = JsonSerializer.Deserialize("""{"x":null}""", options)
+        Assert.Equal({ x = None }, actual)
+
+        let ex =
+            Assert.Throws<JsonException>(fun () -> JsonSerializer.Deserialize<NonSkO>("{}", options) |> ignore)
+        Assert.Contains("", ex.Message)
+
+    [<Fact>]
+    let ``serialize non-skippable option field even with WhenWritingNull`` () =
+        let options =
+            JsonFSharpOptions
+                .Default()
+                .WithSkippableOptionFields(SkippableOptionFields.Never)
+                .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
+        let actual = JsonSerializer.Serialize({ x = Some 1 }, options)
+        Assert.Equal("""{"x":1}""", actual)
+
+        let actual = JsonSerializer.Serialize({ x = None }, options)
+        Assert.Equal("""{"x":null}""", actual)
+
     type C = { cx: B }
 
     [<Fact>]
@@ -695,6 +728,40 @@ module Struct =
             )
         Assert.Equal("""{"sa":1,"sb":2,"sc":3,"sd":4}""", actual)
 
+    [<Struct>]
+    type NonSkO = { x: int option }
+
+    [<Fact>]
+    let ``deserialize non-skippable option field even with WhenWritingNull`` () =
+        let options =
+            JsonFSharpOptions
+                .Default()
+                .WithSkippableOptionFields(SkippableOptionFields.Never)
+                .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
+
+        let actual = JsonSerializer.Deserialize("""{"x":1}""", options)
+        Assert.Equal({ x = Some 1 }, actual)
+
+        let actual = JsonSerializer.Deserialize("""{"x":null}""", options)
+        Assert.Equal({ x = None }, actual)
+
+        let ex =
+            Assert.Throws<JsonException>(fun () -> JsonSerializer.Deserialize<NonSkO>("{}", options) |> ignore)
+        Assert.Contains("", ex.Message)
+
+    [<Fact>]
+    let ``serialize non-skippable option field even with WhenWritingNull`` () =
+        let options =
+            JsonFSharpOptions
+                .Default()
+                .WithSkippableOptionFields(SkippableOptionFields.Never)
+                .ToJsonSerializerOptions(DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull)
+        let actual = JsonSerializer.Serialize({ x = Some 1 }, options)
+        Assert.Equal("""{"x":1}""", actual)
+
+        let actual = JsonSerializer.Serialize({ x = None }, options)
+        Assert.Equal("""{"x":null}""", actual)
+
     [<Struct>]
     type C = { cx: B }
 

tests/FSharp.SystemTextJson.Tests/Test.Regression.fs

@@ -99,3 +99,15 @@ let ``regression #123`` () =
         { FirstName = "yarr"; LastName = None; age = 5 },
         JsonSerializer.Deserialize<Person>("""{"FirstName": "yarr", "age": 5 }""", skipOptions2)
     )
+
+type R = { x: int option }
+type RV = { x: int voption }
+
+[<Fact>]
+let ``regression #154`` () =
+    let o =
+        JsonFSharpOptions().WithSkippableOptionFields(false).ToJsonSerializerOptions()
+    Assert.Throws<JsonException>(fun () -> JsonSerializer.Deserialize<R>("{}", o) |> ignore)
+    |> ignore
+    Assert.Throws<JsonException>(fun () -> JsonSerializer.Deserialize<RV>("{}", o) |> ignore)
+    |> ignore