System.Text.Json code generator: custom converter ignored?

[Jericho]

I have a class that contains the following property (notice the presence of a custom converter):

[JsonPropertyName("tls")]
[JsonConverter(typeof(IntegerBooleanConverter))]
public bool Tls { get; set; }

The following snippet gets generated:

writer.WriteBoolean(tlsPropName, value.Tls);

Correct me if I'm wrong but this seems to ignore my custom converter. I was expecting something along the lines of:

writer.WritePropertyName(tlsPropName);
global::System.Text.Json.JsonSerializer.Serialize(writer, value.Tls, global::MyLibrary.Utilities.MyJsonSerializerContext.Default.Tls);

[ghost]

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

Issue Details

---

I have a class that contains the following property (notice the presence of a custom converter):

[JsonPropertyName("tls")]
[JsonConverter(typeof(IntegerBooleanConverter))]
public bool Tls { get; set; }

The following snippet gets generated:

writer.WriteBoolean(tlsPropName, value.Tls);

Correct me if I'm wrong but this seems to ignore my custom converter. I was expecting something along the lines of:

writer.WritePropertyName(tlsPropName);
global::System.Text.Json.JsonSerializer.Serialize(writer, value.Tls, global::MyLibrary.Utilities.MyJsonSerializerContext.Default.Tls);

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

[eiriktsarpalis]

This is a bug only impacting serialization. Minimal reproduction:

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

namespace Repro
{
    public static partial class Program
    {
        public static void Main()
        {
            MyClass value = JsonSerializer.Deserialize(@"{""MyProperty"":5}", MyJsonContext.Default.MyClass)!;
            Console.WriteLine(value.MyProperty); // Correctly deserializes as 4
            Console.WriteLine(JsonSerializer.Serialize(value, MyJsonContext.Default.MyClass)); // incorrectly serializes as {"MyProperty":4}
        }

        [JsonSerializable(typeof(MyClass))]
        internal partial class MyJsonContext : JsonSerializerContext
        {
        }

        internal class MyClass
        {
            [JsonConverter(typeof(MyIntegerConverter))]
            public int MyProperty { get; set; }
        }

        public class MyIntegerConverter : JsonConverter<int>
        {
            public override int Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => reader.GetInt32() - 1;
            public override void Write(Utf8JsonWriter writer, int value, JsonSerializerOptions options) => writer.WriteNumberValue(value + 1);
        }
    }
}

We have test coverage for deserialization but we seem to be missing equivalent testing for serialization.

[eiriktsarpalis]

Also, note that the issue can be worked around by attaching the JsonConverter attribute to the class definition rather than the property.

[Jericho]

Please note that the workaround you suggested does not work for the scenario I presented. I cannot change the bool class definition.

[eiriktsarpalis]

You can work around the issue by making the following change to your serialization context:

[JsonSourceGenerationOptions(GenerationMode = JsonSourceGenerationMode.Metadata)]
[JsonSerializable(typeof(MyClass))]
internal partial class MyJsonContext : JsonSerializerContext
{
}

Root cause of the issue is that source gen by default uses fast-path serialization, which by design does not implement all features found in metadata-based serialization. The reason this does not impact deserialization is that there's no fast-path deserialization implemented yet. Note that switching to metadata-based serialization will result in slower serialization performance.

@layomia @ericstj should we consider making metadata-based serialization the default? As seen in this example, the current default configuration can result in non-roundtrippable classes. We should make sure that the default configuration just works even if doesn't result in optimal performance.

[ericstj]

We shouldn't need to change defaults. Here's my understanding of how things should work.

1. If a type can't be supported through fast path deserialization based on information available at compile time we should tell the user and they can make the change. We should not generate serialization logic that ignores part of the serialization contract.
2. If a type can't be supported through fast path deserialization based on information available at runtime (EG: converter added at runtime), we should fallback to using metadata if present.