-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
[JsonSerializer] Relax restrictions on ctor param type to immutable property type matching where reasonable #44428
Comments
Why make the constructor parameter nullable? The |
Thanks for the reply @layomia To present a minimal use-case the example is intentionally trivial. My situation is rather more complex. We have several scenarios where having the parameter as nullable was desirable (and worked well using Newtonsoft.Json), some of which we can, and have, resolved by making the parameter non-nullable. But there are other scenarios where a nullable parameter with a non-nullable property is still preferred. One being where we have an unknown number of legacy objects that are serialised to a store with null values. The use-cases have since been updated where new instances are not serialised with null, but we still want to support the de-serialization of the legacy objects. If we had been using System.Text.Json at the time, we probably would have implemented the solution differently, but as I highlighted above Newtonsoft.Json worked. Hope the context helps. Ultimately though, this is a difference in behaviour with Newtonsoft.Json. It looks like you are trying to resolve edge cases in to make System.Text.Json "the standard JSON stack for .NET." (See Issue #43620). And this is one such edge case. |
From @https://github.com/NN--- in #46480:
|
We could look into relaxing the matching algorithm here if it proves to be unwieldy. The workaround here is to refactor the POCO code slightly to make the property type and the constructor parameter type the same. This issue will not be treated as high priority until a blocked and non-trivial scenario is provided. |
Deserializing JSON into an object that can be bound to WPF\XAML is likely very common place and converting an incoming IEnumerable into a observable collection that XAML can use is also likely common. Not supporting this limits System.Text.Json's use with an XAML\WPF\UWP apps. |
@layomia As I explained, the example is trivial to present a minimal use-case. I also went on to explain the non-trivial scenario:
We are also blocked. Our workaround is to stick with Newtonsoft.Json. |
Thanks for the responses and expanding on the importance here. There are definitely various scenarios where loosening the matching restrictions is helpful. We can address this in the .NET 6.0 release. |
From @GabeDeBacker in #47422:
|
Today, we expect an exact match between the constructor parameter type and the immutable property type. This is too restrictive in two major cases:
We can loosen the restriction and support these scenarios by checking that the ctor parameter is assignable from the immutable property type. This new algorithm is in accordance with the serializer always round-tripping (i.e being able to deserialize whatever we serialize), and maintains a high probability that the incoming JSON is compatible with the target ctor param. This is important to avoid unintentional data loss. If there are more reasonable scenarios that will not be satisfied with this proposal, we can evaluate them and perhaps adjust further. |
We can also, or alternatively, consider a property on /// <summary>
/// When placed on a constructor, indicates that the constructor should be used to create
/// instances of the type on deserialization.
/// </summary>
[AttributeUsage(AttributeTargets.Constructor, AllowMultiple = false)]
public sealed class JsonConstructorAttribute : JsonAttribute
{
/// <summary>
/// When <see cref="true" />, indicates that no restriction should be placed on the types of a constructor
/// parameter and a property when there is a case-insensitive match between their names.
/// </summary>
public bool UseRelaxedPropertyMatching { get; set; }
/// <summary>
/// Initializes a new instance of <see cref="JsonConstructorAttribute"/>.
/// </summary>
public JsonConstructorAttribute() { }
} A global option can be considered as well, to support non-owned types where we can't decorate with an attribute: public sealed class JsonSerializerOptions
{
/// <summary>
/// When <see cref="true" />, indicates that no restriction should be placed on the types of a constructor
/// parameter and a property when there is a case-insensitive match between their names.
/// </summary>
public bool ConstructorUseRelaxedPropertyMatching { get; set; }
} All of this design assumes that there will always be a requirement that every constructor parameter binds to an object property, per the original spec for this feature: #33095. |
@layomia - Implementing either (or both) of what you mentioned above (the JsonConstructorAttribute argument or is the constructor argument assignable from the property type) would be great additions! Thanks for the conversation |
We would likely still need to support the "mapping nullable value-type ctor args to immutable properties of a non-nullable version of the type" case by default - #44428 (comment). |
Co-assigning @GabeDeBacker to provide the implementation for this feature, as discussed offline. |
I did find an example of where a property type is not assignable from the constructor argument type. public class ClassThatStoresSecureStrings
{
using System;
using System.Security;
public ClassThatStoresSecureStrings(string userId, string password)
{
// This code repeats for password.
this.UserId = new SecureString();
foreach (var ch in userId)
{
this.UserId.AppendChar(ch);
}
this.UserId.MakeReadOnly();
}
public SecureString UserId { get; }
public SecureString Password {get; }
} |
One possible solution might be to expose a flag on either namespace System.Text.Json;
public class JsonSerializerOptions
{
public bool RequireMatchingPropertyForConstructorParamers { get; set; } = true;
}
public class JsonConstructorAttribute
{
public bool RequireMatchingPropertyForConstructorParamers { get; set; } = true;
} which would make the following POCOs legal from a serialization perspective: JsonSerializer.Deserialize<MyPoco>("{ ... }"); // succeeds deserialization
public class MyPoco
{
[JsonConstructor(RequireMatchingPropertyForConstructorParamers = true)]
public MyPoco(int p1, int p2, int p3) { }
} |
@eiriktsarpalis |
[Triage] This work should include a fix for #56999. Repro below. Also see #56999 (comment).
|
Something that no one mentioned : If the error message could at least indicate what parameter caused the issue then that would be the bare minimum. This question is asked countless times in StackOverflow, it's a huge waste of time for many devs, who have to go through each parameter one by one like cavemen, and wonder : "is it the spelling? Is it that it is nullable?", etc. |
Hi @mathieubergouniouxcab would you be interested in contributing a PR that improves the error message? |
Hi Eirik, |
It is also an open source project that happily accepts community contributions. The team can only deliver a limited amount of features and fixes per release -- this particular issue is not slated for .NET 8 hence my suggestion. |
Of course not, why would it be? It is only a huge waste of time for a lot of people and people have been struggling with it for at least three years only. I have personally witnessed a fair amount of people go back to newtonsoft and miss all the other good things about this api because they did not want to put up with this anymore. I wish I had more time to go down the rabbit hole as I consider it personal and would gladly submit a PR as By far the aspect that caused more waste of my time I have ever encountered. But since I really don't have enough time to work on this one, all it's left for now is hope. And with that hope that I soon get some time to spend on side projects. |
This is the norm it seems. When I keep trying out System.Text.Json every.single.relaease, year over year. And the same fundamental ergonomic & usability failures just keep on keeping on. This is particularly egregious when I had a new C# dev (C++ & TS dev before) approach me yesterday and say that he really doesn't like or want to continue working with C#. His experience with System.Text.Json set the whole tone & stage for what he thinks C# is. While that is a bit premature, it's not exactly uncommon for new devs to have a terrible experience with some native support and then just write off the ecosystem entirely. It's bad for image, it's bad for adoption, and it's bad for ergonomics. And there is active pressure to keep it that way, to explicitly keep it's surface area awkward and full of 'gotchas'. It should not be a default recommendation. It's great for advanced & internal use, don't get me wrong, but for your Java/Pythos/JS/...etc devs poking at C# to see if they like it, it's a turnoff. Pushing people to make FOSS contributions to something that has user-facing design issues, that are explicit isn't really solution. Even moreso when one sees the graveyard of pushed-back feature requests for foundational concerns. That's gotta start at the top. I love C#/.Net. Which is why I'm so passionate about this, it's frustrating. /rant 😒 |
well @eiriktsarpalis I'm not going to enter the polemics, but I guess the bottled up frustration in the messages above is a good hint that maybe the logging part should be re-prioritized after 3 years (this thread started in Nov 2020) 😄 |
Hi, are there any plans to address problems mentioned earlier in .NET 8.0? As I described in this issue #55318 which was closed and linked here two years ago, this serializer is not very useful for scenarios when you want to use it for serializing Aggregates modeled using Domain Driven Design principles and store them in document database. That's because it can't handled encapsulation (which is quite important in DDD :)). I would understand that the goal of creating this serializer in the first place was to create fast general purpose JSON serializer for .NET and not something that is only for serializing simple DTOs in web API endpoints. |
@douglasg14b it would help if you could open issues that specifically break down the issues you are facing and we might even be able to offer recommendations, workarounds or even prioritize relevant improvements.
@mathieubergouniouxcab the System.Text.Json backlog currently consists of 210 open issues which we want to address at some point. While I understand the frustration of being impacted by one specific issue, we generally do try to prioritize based on impact and popularity and this issue has been cut from .NET 8. |
Understood. I haven't found one specifically dealing with logging which parameter is causing a deserialization fail. Could you confirm that such entry does not exist? If so, then I will create it -- in order for it to have its own thread and its own voting system (and stop polluting this one). |
AFAIK you're the first to bring up that concern. I think it's reasonable to factor into a separate issue since it's of much smaller scope. |
I found 8+ entries in your bug tracker related to the error "Each parameter in constructor (...) must bind to a field on deserialization". Devs so obsessed with grinding the issues one by one that they forgot it's 1,000 different flavours of the same issue. https://i.imgur.com/B0St5fv.png |
Care to share the issues that specifically requests this concern?
|
The google query I used :
I only clicked on the 8 first ones but there's 20+.
|
What is the point you are trying to make? Most of the issues in that query are closed which means that either the error string appears tangentially or the issue has been closed as duplicate of the very issue we are discussing in right now. |
Precisely. They are duplicates. Which means the same issue keeps coming back like a boomerang because users are not sure if it's by design and report it as a bug, when a clearer message would help. So anyways, here's the issue created : |
What exactly are you proposing? That we stop closing duplicates so that it somehow makes the issue more pronounced or discoverable? That never works in practice. When we close a duplicate we link to the original issue and encourage folks to upvote and contribute there. |
No, I'm not suggesting anything. I think a misunderstanding happened at some point in the recent messages. Let's move on: I'm done with this topic, I'm interested only in the error message, as you saw in #88048 . I'll stop polluting this thread now. Thanks for the time and efforts! <3 |
Description
I'm trying to deserialise an object from a json string. In this case the constructor allows a nullable value type as the parameter, setting a non-null property (defaulting if
null
).I expect (since it is the behaviour with Newtonsoft.Json) the serializer to handle compatible constructor and bound value type properties (or fields) where the only difference is one is nullable.
The following demonstrates the issue:
An
InvalidOperationException
is thrown from the methodSystem.Text.Json.JsonSerializer.Deserialize
:Configuration
I'm building an ASP.NET Core app targeting
netcoreapp3.1
, and using version 5.0.0-rc.2.20475.5 of System.Text.Json. I'm also using version 12.0.3 of Newtonsoft.Json.Other information
Stack Trace:
The text was updated successfully, but these errors were encountered: