runtime: System.Text.Json no longer disregards members with JsonConverterAttribute and JsonIgnoreAttribute

Hi @eiriktsarpalis / @Mike-E-angelo,

I work for Syncfusion. We have faced serialization issue with DataGrid, TreeGrid and Toolbar components alone when migrating from .NET 6 Preview 7 to .NET 6 RC1. Since, we have used TemplateConverter in these components. We have resolved the compatibility related issues and included with 2021 Volume 3 release, which is ready to roll-out within this week.

Currently, we are compiling our Blazor source using netstandard 2.1 and net 5.0. We planned to compile our source using .NET 6 Target from upcoming 2021 Volume 4 releases.

Thanks for providing the reproduction @mayur-ekbote. I’ve trimmed it down to the following:

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

Console.WriteLine(JsonSerializer.Serialize(new MyClass()));

public class MyClass
{
    [JsonIgnore] // comment out to force the same error in .NET 5
    [JsonConverter(typeof(BadConverter))]
    public string? Property { get; set; }
}

public class BadConverter
{
}

Effectively it seems like .NET 6 is attempting to instantiate the custom converter even though a JsonIgnoreAttribute has been attached to the member. In .NET 5 this doesn’t happen, and you need to comment out JsonIgnore to force the same exception.

Even though the class is clearly misconfigured, I do think that JsonIgnore should act like an escape hatch and completely disregard the member, no matter how badly configured. As such it is reasonable to consider this a regression, especially if the misconfiguration in question is made in third-party libraries that users have no direct control over. Even though customer impact is relatively small, we might consider servicing this for .NET 6.

FWIW I tried combining JsonIgnore with other unsupported configuration, for example:

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

Console.WriteLine(JsonSerializer.Serialize(new MyClass()));

public class MyClass
{
    [JsonIgnore]
    [JsonInclude]
    private string? Property { get; set; }
}

And here the serializer is failing consistently across versions. For .NET 7 we should consider refining the semantics of JsonIgnoreAttribute so that any other misconfiguration is ignored completely.

cc @layomia @steveharter

I can confirm that I have working SfGrid’s again! Thank you all!

https://www.nuget.org/packages/Syncfusion.Blazor/19.3.0.43

@TimLovellSmith,

We have rolled the 2021 Volume 3 release by today.

https://www.syncfusion.com/forums/169291/essential-studio-2021-volume-3-main-release-v19-3-0-43-is-available-for-download

Release notes - https://blazor.syncfusion.com/documentation/release-notes/19.3.43?type=all#common

Works for me, @eiriktsarpalis. Thank you for all your efforts out there, both to you and your team. 🙏

Devs might be using [JsonIgnore] like the left-hand operand of && that always evaluates to a fixed value. Possibly to postpone making more structural changes when .net moved from newtonsoft to system.text in 3.1 (clearly the case with Syncfusion).

This will introduce runtime-breaking changes that might go unnoticed for a while. Unlike some other breaking changes that impact a startup-config or EF core (entirely in the domain of the app developer to fix), this change will affect libraries that would be completely out of the control of the developer. Till the 3rd parties (and their transitive dependents) fix this, the developer won’t be able to migrate to .net 6. There is a lot riding on .net 6: MAUI, hot reload, blazor mobile bindings, and so on.

Since it is breaking the [JsonIgnore] contracts of prior versions, it would be futile to keep it on the roadmap for .net 7. If it is not fixed in .net 6, the workaround would have been implemented by the time .net 7 arrives.

I hope a wider consultation is held before any call is made on this issue.

Only speculating here, but since the SDK also ships with blazor components, it would be conceivable that an rc1 change resulted in the problematic converter only being consumed now. I honestly don’t believe this was a STJ issue, we’ve been checking for invalid JsonConverterAttribute arguments for quite a while now.

Not entirely familiar with the blazor architecture, but is it possible that Syncfusion was somehow using Newtonsoft until recently?

I guess the problem is that this check happens in the JsonSerializerOptions. Somehow between .Net 5 and .Net 6, the check got enforced at runtime.

Consider the following code:

using System;
using System.Collections.Generic;
using System.Formats.Asn1;
using System.Linq;
using System.Text.Json.Serialization;
using System.Text.Json;
using System.Threading.Tasks;

using Newtonsoft.Json;

namespace JsonIssue
{
    public class NewtonsoftConvertor : Newtonsoft.Json.JsonConverter
    {
        public override bool CanConvert(Type typeToConvert)
        {
            return typeToConvert == typeof(string);
        }

        public override object ReadJson(JsonReader reader, Type objectType, object existingValue, Newtonsoft.Json.JsonSerializer serializer)
        {
            return existingValue.ToString();
        }

        public override void WriteJson(JsonWriter writer, object value, Newtonsoft.Json.JsonSerializer serializer)
        {
            throw new NotImplementedException();
        }
    }
}

Which is used to serialize the class:

using JsonIssue;

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;
using System.Text.Json.Serialization;
using System.ComponentModel;

namespace JsonBug
{
    public class NewtonsoftClass
    {

        [JsonIgnore]
        [JsonPropertyName("editTemplate")]
        [JsonConverter(typeof(NewtonsoftConvertor))]
        public string EditTemplate { get; set; }
    }
}

This code does not throw an exception at runtime if the target framework is set as .Net 5 but it throws an exception if it is set as .Net 6.

using JsonIssue;
using Newtonsoft.Json;

using System;

namespace JsonBug
{
    internal class Program
    {
        static void Main(string[] args)
        {
            Console.WriteLine(System.Text.Json.JsonSerializer.Serialize(new NewtonsoftClass()));
        }
    }
}

It could also be something related to JsonIgnore, because, in the code above, NotImplementedException is not thrown in .net5. The output is simply { }. If the attribute was not ignored, the system should have thrown a “NotImplementedException”. So it might also mean that the sequence of the evaluation was changed somewhere.

In either case, it is not a compile-time error. My hunch is that Syncfusion assemblies still target .net5 so the code compiles. But when these libraries are run in .net6 application, they throw the exception. JsonBug.zip Deleted the earlier comment because the code was a little messy.

Facing the same issue (works in Preview7, exception in RC1). EditTemplate property also has the [JsonIgnore] attribute along with JsonPropertyName and JsonConverter. Has any behavior wrt JsonIgnore changed from Preview7 to RC1? In general, which attribute takes precedence?

Thanks for the update rajendranr-5483. Please can you post here when you have released so we can give it a spin. I’m really suffering with this one. I tried to go back to a downgrade but it casued all sorts of other issues so I’m stuck waiting for Synchfusion before I can continue to work in any meaningful way.

Triage: reverting the change in https://github.com/dotnet/runtime/commit/91021fe27b1b845f51ff29cfd47b2405fa91b683#diff-814722ef5303e65815d350762206450b283510f6246b658047c4dffd31c97920L59-L64 at this stage carries risk and it does not meet the bar for servicing RC2. Reported issue is due to a bug in a third-party library which depended on undefined behavior in System.Text.Json: its maintainers are already working on a fix.

I have created #59364 to track making JsonIgnoreAttribute semantics well-documented and consistent in a future release of System.Text.Json.

I keep calling this a “regression” but this is actually a change that improves the API. The “regression” is the experience when taking the same code and running it on net6.0, and that of resulting in an exception that did not occur before in net5.0 through net6.0-preview7.

I very much appreciate you digging into the weeds and providing some clarity to the scenario here, @mayur-ekbote. 👍

Thanks, we’ll investigate.

however we would need to see a reproducing app to better understand the nature of the error.

Hi @eiriktsarpalis thank you for your investigation of this issue. As stated in the ticket a csproj was provided and attached. You may also find the same project here for your review:

https://github.com/Mike-E-angelo/Stash/tree/master/Syncfusion.Error/WebApplication1

Please do let me know if you require any further information and I will do my best to assist. 👍