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 )));

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<DateTime, string> 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<DateTime, string> 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 (#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.