Json: HandleNull property has misleading value for structs when not overridden explicitly in the converter

dlyz commented 1 year ago

Description

We have a custom JsonConverter (OuterConverter for OuterClass) that use another JsonConverter directly to read/write its property (in our case of type InnerStruct that serializes using InnerConverter). The pattern is generally demonstrated in Sample factory pattern converter, but a little simplified in the repro below.

[JsonConverter(typeof(OuterConverter))]
public class OuterClass<T>
{
    public T? Value { get; set; }
}

[JsonConverter(typeof(InnerConverter))]
public struct InnerStruct
{
    // ...
}

JsonSerializer.Serialize<OuterClass<InnerStruct>>(/*...*/);
JsonSerializer.Deserialize<OuterClass<InnerStruct>>(/*...*/);

OuterClass is generic and OuterConverter have to consider _innerConverter.HandleNull value before calling _innerConverter.Read()/Write(). InnerConverter does not override its HandleNull property, so the defaults will be used. And this default value will be HandleNull == false, but the framework does not use this value, it actually uses internal properties HandleNullOnRead and HandleNullOnWrite, and their values for value types (structs) are true and false respectively. This means that in the OuterConverter we think, that the _innerConverter can not read nulls (_innerConverter.HandleNull == false), but the framework and the InnerConverter's author expect, that InnerConverter must read nulls. And currently there is no way for OuterConverter to know expected HandleNullOnRead behavior from InnerConverter.

Reproduction Steps

Minimal repro is under the spoiler.

Minimal repro

```cs using System; using System.Diagnostics; using System.Text.Json; using System.Text.Json.Serialization; using Xunit; public class DefaultHandleNullTest { private class OuterConverter : JsonConverterFactory { public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) { var innerType = typeToConvert.GetGenericArguments()[0]; var innerConverter = options.GetConverter(innerType); return (JsonConverter)Activator.CreateInstance( typeof(Cvt<>).MakeGenericType(innerType), innerConverter )!; } public override bool CanConvert(Type typeToConvert) { return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(OuterClass<>); } private class Cvt : JsonConverter> { private readonly JsonConverter _innerConverter; public Cvt(JsonConverter innerConverter) { _innerConverter = innerConverter; } public override OuterClass? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType != JsonTokenType.StartObject) throw new JsonException(); reader.Read(); if (!reader.ValueTextEquals("v")) throw new JsonException(); reader.Read(); T? value; if (reader.TokenType == JsonTokenType.Null && !_innerConverter.HandleNull) { if (default(T) is not null) { // token is null, but this is a struct and inner converter can not handle that // this will throw in our test, but it shouldn't throw new JsonException(); } value = default; } else { value = _innerConverter.Read(ref reader, typeof(T), options); } reader.Read(); if (reader.TokenType != JsonTokenType.EndObject) throw new JsonException(); return new OuterClass { Value = value }; } public override void Write(Utf8JsonWriter writer, OuterClass value, JsonSerializerOptions options) { writer.WriteStartObject(); writer.WritePropertyName("v"); if (default(T) is null && !_innerConverter.HandleNull && value is null) { writer.WriteNullValue(); } else { _innerConverter.Write(writer, value.Value!, options); } writer.WriteEndObject(); } } } [JsonConverter(typeof(OuterConverter))] public class OuterClass { public T? Value { get; set; } } private class InnerConverter : JsonConverter { //// uncomment to pass the test // public override bool HandleNull => true; public override InnerStruct Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType == JsonTokenType.Null) { return new InnerStruct { IsNull = true }; } else { if (reader.TokenType != JsonTokenType.StartObject) throw new JsonException(); reader.Skip(); return default; } } public override void Write(Utf8JsonWriter writer, InnerStruct value, JsonSerializerOptions options) { if (value.IsNull) { writer.WriteNullValue(); } else { writer.WriteStartObject(); writer.WriteEndObject(); } } } [JsonConverter(typeof(InnerConverter))] public struct InnerStruct { public bool IsNull { get; init; } } [Fact] public void NestedDefaultNullHandlingStruct() { SDSCompare( new InnerStruct { }, @"{}", (l, r) => { Assert.False(r.IsNull); } ); SDSCompare( new InnerStruct { IsNull = true }, @"null", (l, r) => { Assert.True(r.IsNull); } ); SDSCompare( new OuterClass { }, @"{""v"":{}}", (l, r) => { Assert.NotNull(r); Assert.False(r!.Value.IsNull); } ); // here we receive JsonException during the deserialization from OuterConverter.Cvt<>.Read() SDSCompare( new OuterClass { Value = new() { IsNull = true } }, @"{""v"":null}", (l, r) => { Assert.NotNull(r); Assert.True(r!.Value.IsNull); } ); } // serialize-deserialize-serialize private static void SDSCompare( T obj, string expected, Action? sdComparer = null ) { var text = JsonSerializer.Serialize(obj); Debug.WriteLine(text); Assert.Equal(expected, text); var obj2 = JsonSerializer.Deserialize(text); sdComparer?.Invoke(obj, obj2); var text2 = JsonSerializer.Serialize(obj2); Assert.Equal(text, text2); } } ```

Expected behavior

HandleNull == true for value types by default, or public access to HandleNullOnRead and HandleNullOnWrite.

Actual behavior

HandleNull == false by default for value types.

Regression?

No response

Known Workarounds

Use JsonSerializer.Serialize/Deserialize instead of JsonConverter.Read/Write. May impact performance. Can not be used in exotic cases when the converter should be acquired not from the current JsonSerializerOptions.
Always override HandleNull in converter (InnerConverter in repro case). Applicable only when you own the code of the converter.

Configuration

In theory reproduces since the introduction of the HandleNull - from .NET 5. Tested on .NET 6.

Other information

No response

ghost commented 1 year ago

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis See info in area-owners.md if you want to be subscribed.

Issue Details

### Description We have a custom JsonConverter (`OuterConverter` for `OuterClass`) that use another JsonConverter directly to read/write its property (in our case of type `InnerStruct` that serializes using `InnerConverter`). The pattern is generally demonstrated in [Sample factory pattern converter](https://learn.microsoft.com/en-us/dotnet/standard/serialization/system-text-json/converters-how-to?pivots=dotnet-7-0#sample-factory-pattern-converter), but a little simplified in the repro below. ```cs [JsonConverter(typeof(OuterConverter))] public class OuterClass { public T? Value { get; set; } } [JsonConverter(typeof(InnerConverter))] public struct InnerStruct { // ... } JsonSerializer.Serialize>(/*...*/); JsonSerializer.Deserialize>(/*...*/); ``` `OuterClass` is generic and `OuterConverter` have to consider `_innerConverter.HandleNull` value before calling `_innerConverter.Read()/Write()`. `InnerConverter` does not override its `HandleNull` property, so the defaults will be used. And this default value will be `HandleNull == false`, but the framework does not use this value, it actually uses internal properties `HandleNullOnRead` and `HandleNullOnWrite`, and their values for **value types** (structs) are `true` and `false` respectively. This means that in the `OuterConverter` we think, that the `_innerConverter` can not read nulls (`_innerConverter.HandleNull == false`), but the framework and the `InnerConverter`'s author expect, that `InnerConverter` must read nulls. And currently there is no way for `OuterConverter` to know expected `HandleNullOnRead` behavior from `InnerConverter`. ### Reproduction Steps **Minimal repro is under the spoiler.**

Minimal repro

```cs using System; using System.Diagnostics; using System.Text.Json; using System.Text.Json.Serialization; using Xunit; public class DefaultHandleNullTest { private class OuterConverter : JsonConverterFactory { public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) { var innerType = typeToConvert.GetGenericArguments()[0]; var innerConverter = options.GetConverter(innerType); return (JsonConverter)Activator.CreateInstance( typeof(Cvt<>).MakeGenericType(innerType), innerConverter )!; } public override bool CanConvert(Type typeToConvert) { return typeToConvert.IsGenericType && typeToConvert.GetGenericTypeDefinition() == typeof(OuterClass<>); } private class Cvt : JsonConverter> { private readonly JsonConverter _innerConverter; public Cvt(JsonConverter innerConverter) { _innerConverter = innerConverter; } public override OuterClass? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType != JsonTokenType.StartObject) throw new JsonException(); reader.Read(); if (!reader.ValueTextEquals("v")) throw new JsonException(); reader.Read(); T? value; if (reader.TokenType == JsonTokenType.Null && !_innerConverter.HandleNull) { if (default(T) is not null) { // token is null, but this is a struct and inner converter can not handle that // this will throw in our test, but it shouldn't throw new JsonException(); } value = default; } else { value = _innerConverter.Read(ref reader, typeof(T), options); } reader.Read(); if (reader.TokenType != JsonTokenType.EndObject) throw new JsonException(); return new OuterClass { Value = value }; } public override void Write(Utf8JsonWriter writer, OuterClass value, JsonSerializerOptions options) { writer.WriteStartObject(); writer.WritePropertyName("v"); if (default(T) is null && !_innerConverter.HandleNull && value is null) { writer.WriteNullValue(); } else { _innerConverter.Write(writer, value.Value!, options); } writer.WriteEndObject(); } } } [JsonConverter(typeof(OuterConverter))] public class OuterClass { public T? Value { get; set; } } private class InnerConverter : JsonConverter { //// uncomment to pass the test // public override bool HandleNull => true; public override InnerStruct Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) { if (reader.TokenType == JsonTokenType.Null) { return new InnerStruct { IsNull = true }; } else { if (reader.TokenType != JsonTokenType.StartObject) throw new JsonException(); reader.Skip(); return default; } } public override void Write(Utf8JsonWriter writer, InnerStruct value, JsonSerializerOptions options) { if (value.IsNull) { writer.WriteNullValue(); } else { writer.WriteStartObject(); writer.WriteEndObject(); } } } [JsonConverter(typeof(InnerConverter))] public struct InnerStruct { public bool IsNull { get; init; } } [Fact] public void NestedDefaultNullHandlingStruct() { SDSCompare( new InnerStruct { }, @"{}", (l, r) => { Assert.False(r.IsNull); } ); SDSCompare( new InnerStruct { IsNull = true }, @"null", (l, r) => { Assert.True(r.IsNull); } ); SDSCompare( new OuterClass { }, @"{""v"":{}}", (l, r) => { Assert.NotNull(r); Assert.False(r!.Value.IsNull); } ); // here we receive JsonException during the deserialization from OuterConverter.Cvt<>.Read() SDSCompare( new OuterClass { Value = new() { IsNull = true } }, @"{""v"":null}", (l, r) => { Assert.NotNull(r); Assert.True(r!.Value.IsNull); } ); } // serialize-deserialize-serialize private static void SDSCompare( T obj, string expected, Action? sdComparer = null ) { var text = JsonSerializer.Serialize(obj); Debug.WriteLine(text); Assert.Equal(expected, text); var obj2 = JsonSerializer.Deserialize(text); sdComparer?.Invoke(obj, obj2); var text2 = JsonSerializer.Serialize(obj2); Assert.Equal(text, text2); } } ```

### Expected behavior `HandleNull == true` for value types, or public access to `HandleNullOnRead` and `HandleNullOnWrite`. ### Actual behavior `HandleNull == false` by default for value types. ### Regression? _No response_ ### Known Workarounds 1. Use JsonSerializer.Serialize/Deserialize instead of JsonConverter.Read/Write. May impact performance. Can not be used in exotic cases when the converter should be acquired not from the current JsonSerializerOptions. 2. Always override HandleNull in converter (InnerConverter in repro case). Applicable only when you own the code of the converter. ### Configuration In theory reproduces since the introduction of the HandleNull - from .NET 5. Tested on .NET 6. ### Other information _No response_

Author:	dlyz
Assignees:	-
Labels:	`area-System.Text.Json`
Milestone:	-

layomia commented 1 year ago

We won't be able to change the default value as that would be a very impactful breaking change. I don't expect any PR/change to emerge from this issue but's worth taking another look, to re-assess/justify why ~~HandleNullOnWrite is true~~ different values are used in the internal converters.

Know workarounds

Have you run into any concrete scenarios where these workarounds weren't sufficient?

dlyz commented 1 year ago

We won't be able to change the default value as that would be a very impactful breaking change.

Yeah, I thought so, I've mentioned this option mostly to better explain the issue. Although at least documentation should be corrected because currently it states:

https://github.com/dotnet/runtime/blob/7d15ceb59982add236b6254b477294a5f3eff112/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs#L69

and this is correct for the framework behavior, but incorrect for actual HandleNull value.

Have you run into any concrete scenarios where these workarounds weren't sufficient?

In my specific case it was actually easier to use value of internal HandleNullOnRead property since my converters already use LINQ Expressions to build actual converter code. Totally not perfect, but it will work for the foreseeable future, and if/when it stops I probably go with JsonSerializer, it is suboptimal but sufficient.

dotnet / runtime