Wrong ctor is chosen on deserialization
Opened this issue · 7 comments
Looks like there are two problems here. The parameterless ctor should be preferred, as much as I know. And in any case, it must not call a ctor with ignored parameters.
public static void Run()
{
var obj = new TestClass("test", null);
var text = JsonConvert.SerializeObject(obj);
var res = JsonConvert.DeserializeObject<TestClass>(text);
}
class TestClass
{
protected TestClass()
{
}
public TestClass(string name, TestClass parent)
{
// must not be called for deserialization
Name = name;
Parent = parent;
}
[JsonIgnore]
public string Name;
[JsonIgnore]
public TestClass Parent;
}
> JsonTests.dll!JsonTests.CtorTests.TestClass.TestClass(string name, JsonTests.CtorTests.TestClass parent) Line 34 C#
[Lightweight Function]
Newtonsoft.Json.dll!Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(Newtonsoft.Json.JsonReader reader, Newtonsoft.Json.Serialization.JsonObjectContract contract, Newtonsoft.Json.Serialization.JsonProperty containerProperty, Newtonsoft.Json.Serialization.ObjectConstructor<object> creator, string id) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateNewObject(Newtonsoft.Json.JsonReader reader, Newtonsoft.Json.Serialization.JsonObjectContract objectContract, Newtonsoft.Json.Serialization.JsonProperty containerMember, Newtonsoft.Json.Serialization.JsonProperty containerProperty, string id, out bool createdFromNonDefaultCreator) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateObject(Newtonsoft.Json.JsonReader reader, System.Type objectType, Newtonsoft.Json.Serialization.JsonContract contract, Newtonsoft.Json.Serialization.JsonProperty member, Newtonsoft.Json.Serialization.JsonContainerContract containerContract, Newtonsoft.Json.Serialization.JsonProperty containerMember, object existingValue) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.Serialization.JsonSerializerInternalReader.CreateValueInternal(Newtonsoft.Json.JsonReader reader, System.Type objectType, Newtonsoft.Json.Serialization.JsonContract contract, Newtonsoft.Json.Serialization.JsonProperty member, Newtonsoft.Json.Serialization.JsonContainerContract containerContract, Newtonsoft.Json.Serialization.JsonProperty containerMember, object existingValue) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.Serialization.JsonSerializerInternalReader.Deserialize(Newtonsoft.Json.JsonReader reader, System.Type objectType, bool checkAdditionalContent) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.JsonSerializer.DeserializeInternal(Newtonsoft.Json.JsonReader reader, System.Type objectType) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.JsonSerializer.Deserialize(Newtonsoft.Json.JsonReader reader, System.Type objectType) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.JsonConvert.DeserializeObject(string value, System.Type type, Newtonsoft.Json.JsonSerializerSettings settings) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.JsonConvert.DeserializeObject<JsonTests.CtorTests.TestClass>(string value, Newtonsoft.Json.JsonSerializerSettings settings) Unknown
Newtonsoft.Json.dll!Newtonsoft.Json.JsonConvert.DeserializeObject<JsonTests.CtorTests.TestClass>(string value) Unknown
JsonTests.dll!JsonTests.CtorTests.Run() Line 15 C#
JsonTests.dll!JsonTests.Program.Main(string[] args) Line 21 C#
The parameterless ctor should be preferred, as much as I know
Wrong. See documentation for ConstructorHandling, and in particular the behavior specified for ConstructorHandling.Default: https://www.newtonsoft.com/json/help/html/T_Newtonsoft_Json_ConstructorHandling.htm
Either configure ConstructorHandling.AllowNonPublicDefaultConstructor or annotate the ctor the deserializer should use with the [JsonConstructor] attribute, and Bob should be your uncle...
And in any case, it must not call a ctor with ignored parameters.
When parameterized ctors are being used for deserialization, the deserializer has to provide some value to the constructor parameters in any case. There is no way for the deserializer to pass no value to (i.e., ignore) a ctor parameter. The solution to that problem is to either use a parameterless ctor (see above), craft a ctor that fits the needs of the desired deserialization result, or - if that is not feasible - write a bespoke JsonConverter that implements the desired deserialization behavior for the respective type(s).
@elgonzo I see that you deleted "Note how it explains that this attribute affects serialization (and not deserialization)." So, it should ignore this field during deserialization too?
Wrong. See documentation for ConstructorHandling, and in particular the behavior specified for ConstructorHandling.Default
Interesting. In this case, why does it use the parameterless ctor for this class?
class TestClass
{
protected TestClass()
{
}
public TestClass(string name)
{
Name = name;
}
public TestClass(string name, TestClass parent)
{
Name = name;
Parent = parent;
}
[JsonIgnore]
public string Name;
[JsonIgnore]
public TestClass Parent;
}
@elgonzo I see that you deleted "Note how it explains that this attribute affects serialization (and not deserialization)." So, it should ignore this field during deserialization too?
I deleted it because it was not correct; i made a false statement. There is no point in discussing a false statement...
Interesting. In this case, why does it use the parameterless ctor for this class?
Read the documentation for ConstructorHandling.Default i linked to. It says "First attempt to use the public default constructor, then fall back to a single parameterized constructor, then to the non-public default constructor."
In your latter example class there is no single parameterized ctor, there are two parameterized ctors.
So, according to the documented behavior:
- Is there a public default ctor? No.
- Is there a single parameterized ctor? No.
- Is there a non-public default ctor? Yes!
Side note 1: Based on cursory tests, it seems the single parameterized ctor the deserializer is looking for has to be public, even though the documentation doesn't mention it explicitly.
Side note 2: I don't like the documentation using the term "default constructor" incorrectly also as a synonym for "parameterless constructor". But it shouldn't be an impediment to comprehending the documentation...
fall back to a single parameterized constructor
That's pretty confusing way to describe how it works. And really confusing resolution logic, too.
In any case, it must not try to assign the field that it was supposed to ignore. If it cannot choose the ctor that it wants to use, it should throw an exception rather than do something wrong and completely unexpected.
By the way, I had the same result when I had only 1 parameterized ctor, but I couldn't reproduce it in a small repro so far.
If it cannot choose the ctor that it wants to use, it should throw an exception rather than do something wrong and completely unexpected.
That would be a sensible behavior. But i don't think this is going to happen for two reasons: There is little development activities going on with Newtonsoft.Json for quite some time -- in my personal view pretty much a library in maintenance mode. But even so, given that Newtonsoft.Json has been around for a long time and enjoys (enjoyed?) wide-spread use, changing the existing behavior would carry a risk of breaking code that is far more substantial than the slight benefit this code change would provide.
By the way, I had the same result when I had only 1 parameterized ctor
Perhaps that parameterized ctor was non-public.
There is little development activities going on with Newtonsoft.Json for quite some time -- in my personal view pretty much a library in maintenance mode.
Hm. Any info from the author?
changing the existing behavior would carry a risk of breaking code
Only if that code was incorrect, in the first place.
And not changing it adds very real risk to break code in strange ways when you slightly modify your ctors.
Perhaps that parameterized ctor was non-public.
No. I only added another argument to the parameterized ctor.