Closed vsfeedback closed 3 years ago
Tagging subscribers to this area: @eiriktsarpalis, @layomia See info in area-owners.md if you want to be subscribed.
We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.
#### DragonSpark on 8/23/2021, 04:14 AM:FWIW this is also being tracked on Syncfusionโs side here:
https://www.syncfusion.com/feedback/28117/serialization-exception-throws-in-sfgrid-while-upgrading-to-latest-net-version
Author: | vsfeedback |
---|---|
Assignees: | - |
Labels: | `area-System.Text.Json`, `untriaged` |
Milestone: | - |
The exception message:
The converter specified on 'Syncfusion.Blazor.Grids.GridColumn.EditTemplate' does not derive from JsonConverter or have a public parameterless constructor.
Seems to provide a clear suggestion on what is causing the error. Not discounting the possibility of this being a regression, however we would need to see a reproducing app to better understand the nature of the error.
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. ๐
Thanks, we'll investigate.
I can reproduce the issue locally. As the error message suggests, this is an issue with the syncfusion components. The Syncfusion.Blazor.Grids.GridColumn.EditTemplate
property has a System.Text.Json.Serialization.JsonConverterAttribute
that specifies a TemplateConverter
converter that derives from Newtonsoft.Json.JsonConverter
instead of System.Text.Json.JsonConverter
. This should explain the error message.
I would recommend filing an issue with the maintainers of syncfusion.
That was my impression as well @eiriktsarpalis. As stated in a comment on the developercommunity ticket this has been reported to Syncfusion as well.
However, the confusing part to me is that this works if you switch to preview-7. Why does it work in preview-7 and not RC1?
However, the confusing part to me is that this works if you switch to preview-7. Why does it work in preview-7 and not RC1?
Are you sure no other moving parts have come into play? The particular syncfusion converter is clearly broken and would fail with this error in all releases of System.Text.Json.
Try switching to preview7 on your side and watch the magic, @eiriktsarpalis. :)
I was able to get it to work/break simply by switching between the two SDKs. In fact, Syncfusion couldn't reproduce it on their side when I initially reported it to them. It wasn't until I switched back to preview7 that it worked as they saw it as well. If you follow the thread there I requested they try RC1 and that is when they filed the bug on their end.
This is definitely a versioning regression, and I agree with your conclusion on what is wrong, but I am still unclear on why this worked in preview7 (and net5.0
as well).
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?
Not entirely familiar with the blazor architecture, but is it possible that syncfusion was somehow using Newtonsoft until recently?
So I am not entirely sure this is a syncfusion-specific issue @eiriktsarpalis, and cannot speak for them. What I do know is I can take the same package version that works in net5.0
and all the way up to net6.0-preview7
and it works without issue. When I use any flavor of RC1 it breaks with the provided error.
Again, I agree with you that the error is very obvious and seems like it should have been happening since, like, .NET4. ๐
However, a bigger concern here is that if Syncfusion was configuring their controls in a certain way that worked from net5.0
up to net6.0-preview7
, others will be running into the same issue as well once they upgrade to rc1+.
Have you been to confirm on your side that the provided solution does indeed work for preview7?
What I did to test this was put a global.json
at the SLN root and toggle between preview-7 and RC1:
{
"sdk": {
"version": "6.0.100-preview.7.21379.14"
}
}
{
"sdk": {
"version": "6.0.100-rc.1.21416.1"
}
}
(Note that I am pegged to 6.0.100-rc.1.21416.1
due to this issue.)
Finally, it should be noted that this is the only regression issue that I have encountered with upgrading from netcoreapp3.1
-> net5.0
-> net6.0
, which is quite impressive. And really, it didn't actually occur until RC1, which is equally impressive and perplexing. :)
Thank you for any consideration and for any continued assistance you can provide. ๐
I wanted to check in on this issue @eiriktsarpalis. IMO it should not be closed as it has not been determined what the root cause is.
To provide additional clarity, the issue is not that the exception occurs in RC1, but that it occurs in RC1 and not occur in any release from net5.0
through net6.0-preview7
.
Note that I reported this to both MSFT and Syncfusion for different reasons:
Thank you for any further insight/clarification you can provide.
To MSFT to report an obvious regression.
We would need to understand the nature of the supposed regression. From our perspective the behavior as reported is by-design behavior which is why I've closed the issue. In order for us to take another look at this we would need evidence of regressed behavior specifically impacting System.Text.Json components: this would require producing a reproduction that should leave out any dependency on syncfusion or blazor components.
Ok great @eiriktsarpalis thank you for the continued dialogue. The only way I know to reproduce this issue is via Syncfusion which is how I encountered it and as described it is clear and obvious: it works in preview7 (and before) but not RC1.
We would need to understand the nature of the supposed regression.
And this is why I reported this to you, so that you can find out. :)
From our perspective the behavior as reported is by-design behavior which is why I've closed the issue.
We're in full agreement that the provided exception is clear, obvious, and very much understood in RC1. If it ended there, I would have no problem closing the issue.
Unfortunately, the issue does continue from there with a wrinkle whereby the very same code without modification or recompilation works in every major/minor build prior to RC1. This implies a regression with RC1 that did not exist in preview7 (or before).
Finally, I am unaware of how to reproduce this outside of the very simple sample project already reported to developercommunity and this issue. It was and is intended as a starting point for further investigation and analysis, preferably not done by me. ๐
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?
Yes now that Visual Studio 2022 Preview 4 w/ net6.0-RC1
officially deployed I am thinking this thread may get a little busy. :P
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.
I very much appreciate you digging into the weeds and providing some clarity to the scenario here, @mayur-ekbote. ๐
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
.
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
Thank you for reopening this issue, @eiriktsarpalis. ๐
Even though customer impact is relatively small
You say that but half of my Blazor application no longer works due to this issue for nearly four weeks now. ๐ Although Syncfusion is working on its own fix (I am nagging them as much as I am nagging you), it is not scheduled to be released for another 2 weeks. That will make it nearly 6 weeks of a major/fundamental broken control because an exception is now being thrown in net6.0
that wasn't being thrown in net5.0
.
Fortunately, I've also had plenty of other work during this time to keep me busy away from that control. It turns out I was using user-scoped DbContext
s w/ EFCore in my Blazor application rather than making use of IDbContextFactory
, which is a no-no. That's basically a rewrite of my data access which essentially touches the entire application and has ended up being an extensive rewrite. That's nearly done now but I have to get creative these next two weeks to make it to the finish line for the next Syncfusion release. ๐ค
@eiriktsarpalis Did you get a chance to test it between .net 6 previews vs RC1? Specifically, was this JsonIgnore behavior introduced in RC1?
Culprit seems to be this change: https://github.com/dotnet/runtime/commit/91021fe27b1b845f51ff29cfd47b2405fa91b683#diff-814722ef5303e65815d350762206450b283510f6246b658047c4dffd31c97920L59-L64
@layomia could you take a look? Not sure why order was swapped. Should we consider reverting for .NET 6?
The order was swapped for infrastructural reasons to support the src-gen parameterized ctor feature. I'll investigate a fix for the regression here. I do hope that the maintainers of Syncfusion can address the converter misconfiguration issue in the event that this doesn't meet the bar for a 6.0 fix.
FWIW my concern is not only for Syncfusion but others who may be in employing the same method in their serialization(s) as well. It will break for them too once they onboard to RC1+. If it is documented under breaking changes somewhere then I am OK with that as well. But that's just my vote/take/preference.
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.
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.
Works for me, @eiriktsarpalis. Thank you for all your efforts out there, both to you and your team. ๐
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 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.
@timkempster ,
Sure, we have planned to roll-out the 2021 Volume 3 release on or before 1st October.
I will update here once release will be rolled-out.
@TimLovellSmith,
We have rolled the 2021 Volume 3 release by today.
Release notes - https://blazor.syncfusion.com/documentation/release-notes/19.3.43?type=all#common
I can confirm that I have working SfGrid
's again! Thank you all!
This issue has been moved from a ticket on Developer Community.
[severity:It's more difficult to complete my work] Please see the attached
csproj
file.It is a very basic Blazor project that utilizes a Syncfusion control. This control works in . NET6 preview-7 but throws the following exception in rc1 and the rc2 build available today:
Again this works on
6.0.100-preview.7.21379.14
but not on6.0.100-rc.1.21416.1
or6.0.100-rc.2.21420.30
.I have attached a recording of the exception occurring.
Original Comments
Feedback Bot on 8/23/2021, 01:57 AM:
We have directed your feedback to the appropriate engineering team for further evaluation. The team will review the feedback and notify you about the next steps.
DragonSpark on 8/23/2021, 04:14 AM:
FWIW this is also being tracked on Syncfusionโs side here:
https://www.syncfusion.com/feedback/28117/serialization-exception-throws-in-sfgrid-while-upgrading-to-latest-net-version
Original Solutions
(no solutions)