NullRefereceException when using a JsonConverter without a parameterless constructor

[hillin]

https://github.com/dotnet/corefx/blob/dfe3ab2f0205436e23b93c369ee0b2e41a4973e0/src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs#L208-L209

ctor will be null if type does not have a parameterless constructor.

[piotrsz1234]

I thought, does this could be fixed, by using another constructor, to which would be given, parameters, which all, are null?
If that solution is good, I would like to work on that issue.

[hillin]

@piotrsz1234 It's perfectly fine that the JsonConverterAttribute expects a converter type which has a public parameterless constructor, and throws an exception otherwise. It's just the exception should be a well-defined one (like the ThrowHelper.ThrowInvalidOperationException_SerializationConverterOnAttributeInvalid below), rather than a simple NullReferenceException which is more difficult to track down. So I guess a null check would do the trick.

[piotrsz1234]

@hillin Right.
I guess everyone remembers to put empty constructor for class which is serializable.

[ahsonkhan]

Here's a repro of the issue. The null ref only occurs when the converter is used within the JsonConverterAttribute.

[Fact]
public static void ConverterWithoutDefaultCtor()
{
    string json = @"{""MyType"":""ABC""}";
    Foo obj = JsonSerializer.Deserialize<Foo>(json);
    Assert.Null(obj.MyType);
}

[JsonConverter(typeof(MyConverter))]
public class Foo
{
    public string MyType { get; set; }
}

internal class MyConverter : JsonConverter<Foo>
{
    public MyConverter(int x)
    {

    }

    public override Foo Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }

    public override void Write(Utf8JsonWriter writer, Foo value, JsonSerializerOptions options)
    {
        throw new NotImplementedException();
    }
}

System.Text.Json.Serialization.Tests.ReadValueTests.ConverterWithoutDefaultCtor [FAIL]
      System.NullReferenceException : Object reference not set to an instance of an object.
      Stack Trace:
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializerOptions.Converters.cs(209,0): at System.Text.Json.JsonSerializerOptions.GetConverterFromAttribute(JsonConverterAttribute converterAttribute, Type typeToConvert, Type classTypeAttributeIsOn, PropertyInfo propertyInfo)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializerOptions.Converters.cs(130,0): at System.Text.Json.JsonSerializerOptions.GetConverter(Type typeToConvert)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializerOptions.Converters.cs(86,0): at System.Text.Json.JsonSerializerOptions.DetermineConverterForProperty(Type parentClassType, Type runtimePropertyType, PropertyInfo propertyInfo)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonClassInfo.cs(560,0): at System.Text.Json.JsonClassInfo.GetClassType(Type type, Type parentClassType, PropertyInfo propertyInfo, Type& runtimeType, Type& elementType, Type& nullableUnderlyingType, MethodInfo& addMethod, JsonConverter& converter, Boolean checkForAddMethod, JsonSerializerOptions options)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonClassInfo.cs(121,0): at System.Text.Json.JsonClassInfo..ctor(Type type, JsonSerializerOptions options)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializerOptions.cs(324,0): at System.Text.Json.JsonSerializerOptions.GetOrAddClass(Type classType)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\ReadStackFrame.cs(147,0): at System.Text.Json.ReadStackFrame.Initialize(Type type, JsonSerializerOptions options)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.Helpers.cs(15,0): at System.Text.Json.JsonSerializer.ReadCore(Type returnType, JsonSerializerOptions options, Utf8JsonReader& reader)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(89,0): at System.Text.Json.JsonSerializer.Deserialize(String json, Type returnType, JsonSerializerOptions options)
        E:\GitHub\Fork\corefx\src\System.Text.Json\src\System\Text\Json\Serialization\JsonSerializer.Read.String.cs(31,0): at System.Text.Json.JsonSerializer.Deserialize[TValue](String json, JsonSerializerOptions options)
        E:\GitHub\Fork\corefx\src\System.Text.Json\tests\Serialization\ReadValueTests.cs(75,0): at System.Text.Json.Serialization.Tests.ReadValueTests.ConverterWithoutDefaultCtor()

Ideally, such issues will get fixed by annotating nulls: #114 (comment)

[MarcoRossignoli]

@piotrsz1234 are you working on this?

[piotrsz1234]

@MarcoRossignoli No

[piotrsz1234]

But it isn't hard fix. I guess some work with finding class using Assembly.

[MarcoRossignoli]

I think that the fix is simply throw a clear exception right @ahsonkhan?

[ahsonkhan]

Yes, I believe this is being fixed here:
https://github.com/dotnet/runtime/pull/528/files#diff-bc4374ba33eacef0bf43a48fd7059eb9R210

See @buyaa-n's comment: #528 (comment)

[MarcoRossignoli]

I see

[layomia]

Fixed in #528.