-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
System.Text.Json no longer disregards members with JsonConverterAttribute and JsonIgnoreAttribute #58469
Comments
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue DetailsThis issue has been moved from a ticket on Developer Community. [severity:It's more difficult to complete my work] 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 I have attached a recording of the exception occurring. Original CommentsFeedback 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: Original Solutions(no solutions)
|
The exception message:
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. |
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 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? |
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 |
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? |
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 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 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
(Note that I am pegged to Finally, it should be noted that this is the only regression issue that I have encountered with upgrading from 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 Note that I reported this to both MSFT and Syncfusion for different reasons:
Thank you for any further insight/clarification you can provide. |
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.
And this is why I reported this to you, so that you can find out. :)
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/ |
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:
Which is used to serialize the class:
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.
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. |
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 |
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 Even though the class is clearly misconfigured, I do think that FWIW I tried combining 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 |
Thank you for reopening this issue, @eiriktsarpalis. 👍
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 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 |
@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: 91021fe#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 91021fe#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 |
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. |
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. |
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 |
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)
The text was updated successfully, but these errors were encountered: