Runtime: System.Text.Json does not support TypeConverters

[MithrilMan]

Describe the bug

I was used to have custom TypeConverter to convert complex types from/to strings
It was working when the serializator was Newtonsoft, now that you moved to internal dotnet serializator it doesn't work anymore

To Reproduce

Create a controller that returns a class with a complex type where you implemented a TypeConverter for (and registered using TypeDescriptor.AddAttributes )
Look the result of the controller and you'll see that the output will not be serialized according to the TypeConverter but will be serialized fully as a complex object

We will close this issue if:

• the behavior will be consistent with newtonsoft serializator behavior

Further technical details

• tried on ASP.NET Core version 3.1

Note that using the package Microsoft.AspNetCore.Mvc.NewtonsoftJson and doing services.AddControllers().AddNewtonsoftJson() (so basically restoring newtonsoft serializator) fixes this problem (basically bypass internal json serializator)

[MithrilMan]

an example of complex type that has to be converted to string

public class DomainEntityId {

      public string Id { get; }

      public long RemoteId { get; }


      public string WAYDomainName { get; }

      public DomainEntityId(string domainEntityId) {
         string[] parts = domainEntityId?.Split('-');

         if (parts.Length != 2) {
            throw new ArgumentException(nameof(domainEntityId), "Wrong domainEntityId format.");
         }

         if (string.IsNullOrWhiteSpace(parts[0])) {
            throw new ArgumentException(nameof(WAYDomainName), "Invalid domain name. Domain name cannot be null or empty.");
         }

         if (!long.TryParse(parts[1], out long remoteId)) {
            throw new ArgumentException(nameof(domainEntityId), "Wrong domainEntityId ID value, numeric value expected.");
         }

         if (remoteId < 0) {
            throw new ArgumentOutOfRangeException(nameof(RemoteId), "id must be equal or greather than 0");
         }

         this.WAYDomainName = parts[0];
         this.RemoteId = remoteId;
         this.Id = domainEntityId;
      }

      public DomainEntityId(string domain, long id) : this($"{domain}-{id}") {
      }

      public DomainEntityId(string domain, string id) : this($"{domain}-{id}") {
      }

      public override string ToString() {
         return this.Id;
      }
   }

converter

public class DomainEntityIdConverter : TypeConverter {
   public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType) {
      if (sourceType == typeof(string)) {
         return true;
      }

      return base.CanConvertFrom(context, sourceType);
   }

   public override object ConvertFrom(ITypeDescriptorContext context, CultureInfo culture, object value) {
      return new DomainEntityId((string)value);
   }

   public override object ConvertTo(ITypeDescriptorContext context, CultureInfo culture, object value, Type destinationType) {
      if (destinationType == typeof(string)) {
         return ((DomainEntityId)value).Id;
      }
      return base.ConvertTo(context, culture, value, destinationType);
   }
}

registered with
TypeDescriptor.AddAttributes(typeof(DomainEntityId), new TypeConverterAttribute(typeof(DomainEntityIdConverter )));

[msftbot[bot]]

Tagging subscribers to this area: @safern
Notify danmosemsft if you want to be subscribed.

[buvinghausen]

The easiest way to reproduce the issue is to use NodaTime 3.0 which has type converter support added.

This class makes a good test harness..

public class NodaValues
{
    public NodaValues()
    {
        try
        {
            DateTimeZone = DateTimeZoneProviders.Tzdb.GetSystemDefault();
        }
        catch (DateTimeZoneNotFoundException)
        {
            DateTimeZone = DateTimeZoneProviders.Tzdb["America/New_York"];
        }

        Instant = SystemClock.Instance.GetCurrentInstant();
        ZonedDateTime = Instant.InZone(DateTimeZone);
        OffsetDateTime = Instant.WithOffset(ZonedDateTime.Offset);
        OffsetDate = new OffsetDate(LocalDate, Offset);
        OffsetTime = new OffsetTime(LocalTime, Offset);
        Interval = new Interval(Instant,
            Instant
                .PlusTicks(TimeSpan.TicksPerDay)
                .PlusTicks(TimeSpan.TicksPerHour)
                .PlusTicks(TimeSpan.TicksPerMinute)
                .PlusTicks(TimeSpan.TicksPerSecond)
                .PlusTicks(TimeSpan.TicksPerMillisecond));
        Period = Period.Between(LocalDateTime,
            Interval.End.InZone(DateTimeZone).LocalDateTime, PeriodUnits.AllUnits);
    }

    public AnnualDate AnnualDate => new AnnualDate(DateTime.Now.Month, DateTime.Now.Day);
    public DateTimeZone DateTimeZone { get; }
    public Duration Duration => Interval.Duration;

    public Instant Instant { get; }
    public Interval Interval { get; }

    public LocalDate LocalDate => ZonedDateTime.Date;
    public LocalDateTime LocalDateTime => ZonedDateTime.LocalDateTime;
    public LocalTime LocalTime => ZonedDateTime.TimeOfDay;

    public Offset Offset => ZonedDateTime.Offset;
    public OffsetDate OffsetDate { get; }
    public OffsetDateTime OffsetDateTime { get; }

    public OffsetTime OffsetTime { get; }

    public Period Period { get; }

    public YearMonth YearMonth => new YearMonth(DateTime.Now.Year, DateTime.Now.Month);

    public ZonedDateTime ZonedDateTime { get; }
}

[buvinghausen]

Another thing when serializing and deserializing dictionaries that have a key you should also check if a type converter to string exists for the key and if so use it.

Currently it blows up if you return a Dictionary even though DateTIme has a TypeConverter to string that could be used.

[buvinghausen]

@layomia where does this issue sit prioritization wise? It's a pretty big miss in the json library to not implicitly check for type converters and use them when relevant.

[layomia]

@buvinghausen

Currently it blows up if you return a Dictionary even though DateTIme has a TypeConverter to string that could be used.

We've added support for more primitives as dictionary keys in 5.0 (https://github.com/dotnet/runtime/pull/38056), including DateTime. To support this feature, we added new internal methods on JsonConverter<T> to handle reading/writing non-string types from/to JSON property names. This mechanism could be exposed and could also handle complex types like POCOs as dictionary keys. cc @Jozkee

The lookup and invocation of these TypeConverter instances is likely non-trivial wrt performance and would likely work against performance goals of System.Text.Json. It would also add new internal code to the library to support a legacy model.

Can you tell me more about why TypeConverters need to be used here, e.g. in your NodaValues example? Which types are not serializing correctly in System.Text.Json without TypeConverters? Can System.Text.Json's custom converters be used instead? A full repro app with all the types being (de)serialized along with the type converters would be useful here.

[buvinghausen]

@layomia see responses inline

We've added support for more primitives as dictionary keys in 5.0 (#38056), including DateTime. To support this feature, we added new internal methods on JsonConverter<T> to handle reading/writing non-string types from/to JSON property names. This mechanism could be exposed and could also handle complex types like POCOs as dictionary keys. cc @Jozkee

Thank you for the update. That is good news and definitely a step in the right direction. I did verify it in preview8 of the v5 SDK.

The lookup and invocation of these TypeConverter instances is likely non-trivial wrt performance and would likely work against performance goals of System.Text.Json. It would also add new internal code to the library to support a legacy model.

I put together a sample repository that benchmarks the type converter operations and it was running anywhere from 64ms to 45ms inside my VM on a Surface Pro 5. As far as the performance goals of System.Text.Json let's just say we have to agree to disagree here..... System.Text.Json performs way too close to Json.NET and not anywhere close to UTF8Json for that to continue to be the mantra if we're being honest. Outperforming Json.NET should definitely be the goal and you've hit that. Unfortunately feature parity still makes me prefer to use Json.NET over System.Text.Json 100% of the time because the gap in performance is not so substantial to close the feature parity gap. In .NET 5 I want to cut over to web assembly but that exclusively uses System.Text.Json so now the gaps are starting to become glaring.

Can you tell me more about why TypeConverters need to be used here, e.g. in your NodaValues example? Which types are not serializing correctly in System.Text.Json without TypeConverters? Can System.Text.Json's custom converters be used instead? A full repro app with all the types being (de)serialized along with the type converters would be useful here.

TypeConverters don't need to be used other than they have been around for ages and work wonderfully as evidenced by how fluidly Json.NET handles custom structs vs System.Text.Json. As far as which types don't work that would be all of them here is the difference with the LocalTime struct...

Json.NET which uses the TypeConverter to string

"date":"2020-08-27"

System.Text.Json which fumbles across all the properties on the struct

    "date": {
        "calendar": {
            "id": "ISO",
            "name": "ISO",
            "minYear": -9998,
            "maxYear": 9999,
            "eras": [
                {
                    "name": "BCE"
                },
                {
                    "name": "CE"
                }
            ]
        },
        "year": 2020,
        "month": 8,
        "day": 27,
        "dayOfWeek": 4,
        "yearOfEra": 2020,
        "era": {
            "name": "CE"
        },
        "dayOfYear": 240
    }

I have added a JsonConverter for LocalDate that fixes the issue to and from the wire when used explicitly as a scalar property of an object. Unfortunately even with the TypeConverter registered it still blows up when you try to produce a dictionary with LocalDate as a key so I added a DictionaryKeyTypeConverter so yes all of that does work.

The problem is what happens when you make IDictionary, IReadOnlyDictionary, ReadOnlyDictionary, IImmutableDictionary, or ImmutableDictionary parameters? I get you don't want to use TypeConverters because they are viewed as legacy but in the case of only using them explicitly to and from strings to solve issues like dictionary keys or if the object is a struct and is missing a JsonConverter seems like a no brainer to me.

[Jozkee]

I get you don't want to use TypeConverters because they are viewed as legacy

@buvinghausen That's not the reason why we didn't took the approach of relying on TypeConverters for dictionary keys.
The main concern is that TypeConverters APIs onle receive and return object, which involves a lot of boxing when dealing with value types (structs) and boxing involves a lot of performance penalty and memory allocation:

Please see the spec for extending support of dictionary keys.
https://github.com/dotnet/runtime/blob/a5d96c0e280b56412d4614848f5ee3b1e0d7f216/src/libraries/System.Text.Json/docs/KeyConverter_spec.md#typeconverter

The results are form very early steps in the feature development but you can notice that the TypeConverter approach for Guid was 2x slower than using the "KeyConverter" approach that was using generics:

KeyConverter:
| Type | Method | Mean | Error | StdDev | Allocated |
|------------------------------------------- |---------------------------- |----------:|----------:|----------:|----------:|
| WriteDictionary> | SerializeToUtf8Bytes | 9.874 us | 0.1383 us | 0.1226 us | 5 KB |

TypeConverter:
| Type | Method | Mean | Error | StdDev | Allocated |
|------------------------------------------- |---------------------------- |----------:|----------:|----------:|----------:|
| WriteDictionary> | SerializeToUtf8Bytes | 19.067 us | 0.6886 us | 0.7930 us | 17.5 KB |

[buvinghausen]

@Jozkee thank you for the expedient follow up. So how do I plumb the JsonConverter into getting picked up as a key converter?

[Jozkee]

As @layomia mentioned, we could expose the mechanism to use JsonConverters for dictionary keys. That would require feature work and will need a new API so we would have to create an API proposal and present it for review.

[buvinghausen]

@Jozkee so here comes the compromise.... Since API proposals and reviews take time is there any chance we could meet at a compromise which is if no key converter is registered then as a last ditch fail over to checking for a type converter and it's ability to convert to and from a string and use it rather than throw the exception? That would leave the existing API as is and the only people who are paying the performance hit are those who don't have a key converter registered for the type they are using. Once the API to register key converters is live I then can register them and stop paying the performance tax from boxing.

[layomia]

In .NET 5 I want to cut over to web assembly but that exclusively uses System.Text.Json so now the gaps are starting to become glaring.

The ReadJsonAsync etc APIs in System.Net.Http.Formatting use Newtonsoft.Json by default. You can consume the package here: https://www.nuget.org/packages/Microsoft.AspNet.WebApi.Client.

I put together a sample repository that benchmarks the type converter operations and it was running anywhere from 64ms to 45ms inside my VM on a Surface Pro 5. As far as the performance goals of System.Text.Json let's just say we have to agree to disagree here..... System.Text.Json performs way too close to Json.NET and not anywhere close to UTF8Json for that to continue to be the mantra if we're being honest.

We understand that different scenarios/benchmarks have different perf characteristics and results/comparisons will vary depending on what is being tested. Performance is an area of continuous improvement for System.Text.Json. We've made many improvements to the library in 5.0 wrt to both perf and features. The goal is to maintain a good balance/trade-off between perf and features/usability. This involves maintaining a performance-first architecture, and taking a dependency on TypeConverter doesn't align with this especially given the boxing pointed out by @Jozkee.

There is an opportunity to improve perf in more scenarios (Blazor WASM, Xamarin iOS/Android) with the JSON compile time code-gen feature coming in 6.0.

Since API proposals and reviews take time is there any chance we could meet at a compromise which is if no key converter is registered then as a last ditch fail over to checking for a type converter and it's ability to convert to and from a string and use it rather than throw the exception? That would leave the existing API as is and the only people who are paying the performance hit are those who don't have a key converter registered for the type they are using. Once the API to register key converters is live I then can register them and stop paying the performance tax from boxing.

As a framework, we can't just introduce behavior like this flippantly, because it becomes part of the contract that callers expect and even though it won't be an API breaking change if we change our mind after designing the end-to-end, it will be a behavioral breaking change. Not to mention the level of testing that would have to go in it.

Either way we go (supporting TypeConverter vs new dictionary-key-converting overloads on JsonConverter<T>), the earliest either feature can land is in .NET 6.0. For aforementioned reasons, it is not currently on the roadmap to support TypeConverters in System.Text.Json. We can prioritize exposing the new overloads on JsonConverter<T> so that you can try them out in a .NET 6.0 preview.

The recommendation for anyone dependent on TypeConverter for general (de)serialization is to instead use custom converters instead. If they are needed for dictionary-key handling for complex objects, please stay tuned for new overloads on JsonConverter<T> that can help with this. In the meantime, custom converters for the dictionary type itself can be used. If none of these options is viable for your scenario, the recommendation is to stick with Newtonsoft.Json.

[julealgon]

I'm seeing the performance argument being used in this one, but wouldn't it only be a hit in case type converters are actually being used? If no type converters exist for a given type, then performance should stay the same, as it is not calling into the various legacy methods there. At the same time, if people want to rely on type converters, it is on them to evaluate the performance impact.

The benefit of the type converter approach is that it is extremely general purpose and used for various different things and frameworks. Since the interface is generic, it can be translated to pretty much any "custom conversion" interface in many libraries seamlessly.

There should at least be a JsonConverter<T> implementation that delegates to the TypeConverter call so that if no custom converter is provided, the fallback using TypeConverter is used.

[buvinghausen]

@julealgon that was essentially my point was the only time the perf penalty was paid was if it was being used as a last resort and that if I wanted to side step I would wire up a JsonConverter. Unfortunately the team has been very adamant about making this transition exceptionally painful. See things like not wanting to add kebab-case or snake_case as examples or even the flat out refusal to support the data contract attributes even though those are all I ever use because of their portability across json.net and the data contract serializer for XML.