Invalid deserialisation order for required fields

[Pzixel]

Source JSON

{}

Destination type

public class SampleType
{
    [JsonProperty(Required = Required.Always)]
    public int SampleField { get; }

    [JsonConstructor]
    public SampleType(int sampleField)
    {
        if (sampleField == 0) {
            throw new Exception("Should not be zero");
        }
        SampleField = sampleField;
    }
}

Expected behavior

Deserialization exception is thrown:

Required property 'SampleField' not found in JSON. Path '', line 1, position 2.

Actual behavior

User exception is thrown:

Should not be zero

Steps to reproduce

JsonConvert.DeserializeObject<SampleType>("{}");

[Pzixel]

Hello?..

[bartelink]

er, have you looked at the test suite? I personally would not expect it to be able to call that ctor as it does not have a sampleField - perhaps if it was Nullable ?

[Pzixel]

What test suite you are telling about? I'm just calling raw Json.Net api. And sure it can find and call constructor, you can just copypaste the example and see it yourself.

Of course it's not nullable because the whole point here is I want to have a non-nullable non-zero int, and have all invariants checking in deserialization step.

[bartelink]

TL; DR - I'm pretty sure this is not how JsonConstructor works - the inputs need to be present or the ctor params need to be Nullable (i.e. ?)

I'm talking about the test suite in this repo - if I need to figure out how something is expected to work, I cut out the middleman and read the tests to determine how such cases are handled. Consider them an alternate form of documentation.

In your particular case, you're saying "this case is wrong". I'm saying "go look at the other cases and see if you can find something similar; it's likely that there are specs saying the exact opposite based on the reason I am suggesting".

My reasoning for this is that a) you want it to work that way b) it doesn't so c) someoene will need to make it do what you want and not contravene existing rules so d) it might as well be you that has a quick scan to determine whether your request is realizable (or whether there's a different way to manage this)

[Pzixel]

TL; DR - I'm pretty sure this is not how JsonConstructor works - the inputs need to be present or the ctor params need to be Nullable (i.e. ?)

I expect if parameter is marked as non-nullable but missing in the scheme then serialized should throw "field not found" or somerhing. I don't expect field to be null, I want a proper exception when it happens.

My reasoning for this is that a) you want it to work that way b) it doesn't so c) someoene will need to make it do what you want and not contravene existing rules so d) it might as well be you that has a quick scan to determine whether your request is realizable (or whether there's a different way to manage this)

I expect scheme validation happening before deserializing. I think it's reasonable to expect it working that way. Of course I may make constructor arguments nullable and check them myself, but I don't see why this shouldn't happen by default, and the second problem is that I"m reusing this constructor in the whole solution, and everybody like to have typechecks instead of runtime exceptions.

[bartelink]

Apologies - seems I scanned too fast - I'm surprised it entered the constructor as you're saying; I agree there is a case to be understood. But, the truth re these edge cases will definitely be in the tests...

[dbc2]

Json.NET seems to be working as designed here. Two points:

1. When deserializing using a parameterized constructor, Json.NET will automatically provide a default value for any constructor arguments without matching JSON properties.

   This is described in, e.g. this answer to Newtonsoft json deserialise missing int values as nulls instead of zero and this answer to How does JSON deserialization in C# work.

2. When the c# data model has required properties and one or more are missing, then a JsonSerializationException is thrown after deserialization is complete.

   When deserializing an object with a default constructor this falls out naturally from Json.NET's streaming approach to deserialization. First the object is constructed. Next Json.NET streams through the JSON, deserializing and populating each property and tracking which ones were found. Finally, if a required property was not encountered, an exception is thrown at the end of the process.

   It seems as though Newtonsoft made a conscious decision to preserve this order when deserializing objects with parameterized constructors. They did this despite the fact that it would have been possible to throw the exception at the beginning of deserialization, since the JSON gets preloaded and deserialized into property and argument values before the object is constructed.

   To confirm this, see JsonSerializerInternalReader.CreateObjectUsingCreatorWithParameters(). Checking for missing properties is done in the call to EndProcessProperty() which happens is towards the end of the algorithm, just before calling finally OnDeserialized.

If I modify your type as follows, so that validation of sampleField is done in an OnDeserialized method and a factory method that checks for appropriate argument values replaces the public parameterized constructor:

public class SampleType
{
    int sampleField;

    [JsonProperty(Required = Required.Always)]
    public int SampleField { get { return sampleField; } }

    public static SampleType Create(int sampleField)
    {
        Validate(sampleField);
        return new SampleType(sampleField);
    }

    [JsonConstructor]
    internal SampleType(int sampleField)
    {
        this.sampleField = sampleField;
    }

    [System.Runtime.Serialization.OnDeserialized]
    void OnDeserializedMethod(System.Runtime.Serialization.StreamingContext context)
    {
        Validate(SampleField);
    }

    static void Validate(int sampleField)
    {
        if (sampleField == 0)
        {
            throw new Exception("Should not be zero");
        }
    }
}

Then a JsonSerializationException is thrown as expected, rather than the custom exception. Demo fiddle here.

I cannot, however, find anything in the official documentation that explains exactly how JSON properties are matched to constructor arguments, what happens when a constructor argument is missing, how and whether [JsonProperty] attributes applied to .Net properties affect the deserialization of similarly named constructor arguments, or when missing property validation occurrs. Those are all topics that Newtonsoft might want to clarify in their documentation.

[Pzixel]

@dbc2 thanks for detailed explanation, it really makes things clear.

The main purpose of the code written was reusability. I mean this constructor can be used in both (de)serialization and regular class creation. So having all validations in constructor instead of magically decorated methods are much more appropriate.

I agree that docs may be clearer here but I also think that constructor should be called after fields are populated. It calls constructor with actual values in the end so it may already validate that some values are missing at this point. I don't think it sets private fields via reflection at this point, it should merely call the constructor with values. So, being said, values are already deserialized at this point.

I see your approach with static factory method, it's probably the only suitable way of having this behavior in current version of the library. But I believe it should be changed.