Allow custom converters to feed back to the POCO converter not to write the property name

[NinoFloris]

This is mainly a rehash of comments in #33433 but I'm pulling it out into it's own issue for visibility.

Ah! Yes agreed, this is for instance critical to Option/ValueOption in F#. These conceptually map their None value to null yet they are usually called too late to stop the property name from getting written. I tested this by turning on IgnoreNullValues, and making my Option converter not write anything for the None case, this even causes invalid json to be written (netcore 3.1).

Newtonsoft does support this, and it's a reason why our F# responses with System.Text.Json have more nulls in the results than we would like when IgnoreNullValues is enabled. We literally can't do anything else than emit null for F# (Value)Option.None cases as any F# discriminated union must be serialized with the help of a custom converter.

I just wanted to bring it to the attention specifically so it could be taken into consideration for the new poco converter model #36785.

Ideally it should be possible to have a value converter opt out of its property getting written. However I guess with #36785 in the 'worst case' you could create your own generic POCO converter that does support this.

[ghost]

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

---

This is mainly a rehash of comments in #33433 but I'm pulling it out into it's own issue for visibility.

Ah! Yes agreed, this is for instance critical to Option/ValueOption in F#. These conceptually map their None value to null yet they are usually called too late to stop the property name from getting written. I tested this by turning on IgnoreNullValues, and making my Option converter not write anything for the None case, this even causes invalid json to be written (netcore 3.1).

Newtonsoft does support this, and it's a reason why our F# responses with System.Text.Json have more nulls in the results than we would like when IgnoreNullValues is enabled. We literally can't do anything else than emit null for F# (Value)Option.None cases as any F# discriminated union must be serialized with the help of a custom converter.

I just wanted to bring it to the attention specifically so it could be taken into consideration for the new poco converter model #36785.

Ideally it should be possible to have a value converter opt out of its property getting written. However I guess with #36785 in the 'worst case' you could create your own generic POCO converter that does support this.

Author:	NinoFloris
Assignees:	-
Labels:	area-System.Text.Json, untriaged
Milestone:	-

[eiriktsarpalis]

Related to #29812.

[NinoFloris]

The new ValueOptionConverter (and the normal option converter) will unconditionally write null values and does not respect IgnoreNullValues. For external converters you could label this as being just an enhancement but for converters that ship in the box I'd say it's a bug.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpValueOptionConverter.cs

Line 82 in dc2fe34

writer.WriteNullValue();

Will this still get fixed in 6.0, is a PR welcome?

[eiriktsarpalis]

Null/default value handling is extraneous to the converter implementation. The changes include testing that validates this for F# option and value option types.

[eiriktsarpalis]

@NinoFloris going back to the original issue, are you still experiencing this with .NET 5? I tried this code

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault };
var value = new { Struct = default(MyStruct), Class = default(MyClass), Int = default(int?) };
string json = JsonSerializer.Serialize(value, options);
Console.WriteLine(json); // prints "{}"

[JsonConverter(typeof(MyCoolConverterFactory))]
public struct MyStruct
{
    public int X { get; set; }
}

[JsonConverter(typeof(MyCoolConverterFactory))]
public class MyClass
{
    public int X { get; set; }
}

public class MyCoolConverter<T> : JsonConverter<T>
{
    public override T? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
    public override void Write(Utf8JsonWriter writer, T _, JsonSerializerOptions options) => writer.WriteNumberValue(42);
}

public class MyCoolConverterFactory : JsonConverterFactory
{
    public override bool CanConvert(Type _) => true;

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions _)
    {
        return (JsonConverter)Activator.CreateInstance(typeof(MyCoolConverter<>).MakeGenericType(typeToConvert))!;
    }
}

and ignored default values seem to be honored, even for custom converters. Changing JsonSerializerOptions to set the obsoleted IgnoreNullValues property results in all null properties being ignored.

If I'm misunderstanding the issue, could you please share a minimal repro? Thanks.

[NinoFloris]

@eiriktsarpalis Something like this is the best we can do as a library supplying ValueOption conversion without internals access and it still emits nulls, Option only works because, as you know null = Option.None, if it had pointed to a singleton it too would not have worked.

The idea is, no attributes or other nonsense to tune per property. Just app wide proper ValueOption support without using WhenWritingDefault (as its important things like 0 and false still show up), which leaves WhenWritingNull previously known as IgnoreNullValues. On top of that WhenWritingNull seems exceedingly poorly named, * when writing null * via the writer a null still gets written, you can see how that's unfortunate (even though I understand why you opted for this layered model where the writer is agnostic of these options).

using System;
using System.Text.Json;
using System.Text.Json.Serialization;

var options = new JsonSerializerOptions { DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull };
options.Converters.Add(new ValueOptionConverterFactory());
var value = new MyDTO { Description = new ValueOption<string>() };
string json = JsonSerializer.Serialize(value, options);
Console.WriteLine(json); // prints "{ "description": null }"

public struct ValueOption<T>
{
    public enum Cases
    {
        ValueNone = 0,
        ValueSome = 1
    }

    public Cases Tag { get; set; }
    public T Value { get; set; }
}

public class MyDTO
{
    public ValueOption<string> Description { get; set; }
}

public class ValueOptionConverter<T> : JsonConverter<ValueOption<T>>
{
    public override ValueOption<T> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => throw new NotSupportedException();
    public override void Write(Utf8JsonWriter writer, ValueOption<T> value, JsonSerializerOptions options)
    {
        switch (value.Tag)
        {
            case ValueOption<T>.Cases.ValueNone:
                writer.WriteNullValue();
                break;
            case ValueOption<T>.Cases.ValueSome:
                JsonSerializer.Serialize<T>(writer, value.Value, options);
                break;
        };
    }
}

public class ValueOptionConverterFactory : JsonConverterFactory
{
    public override bool CanConvert(Type t)
        => t.IsGenericType && t.GetGenericTypeDefinition() == typeof(ValueOption<>);

    public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions _)
    {
        return (JsonConverter)Activator.CreateInstance(typeof(ValueOptionConverter<>).MakeGenericType(typeToConvert.GetGenericArguments()[0]))!;
    }
}

[eiriktsarpalis]

Ah I see now, so you're requesting for something like conditional property serialization but specified on the converter level? Could you provide an API proposal?

On top of that WhenWritingNull seems exceedingly poorly named, * when writing null * via the writer a null still gets written, you can see how that's unfortunate (even though I understand why you opted for this layered model where the writer is agnostic of these options).

I wasn't around when these were named, but I would think that the "Null" in both WhenWritingNull and IgnoreNullValues refers to .NET nulls versus null the JSON token. It would be impractical to have this feature kick in every time a converter calls writer.WriteNullValue(). In any case, renaming this now would be a breaking change so that's the name STJ will use for all eternity :-)

[eiriktsarpalis]

Closing in favor of #55781 (comment)