Fix NullReference on JsonExtensionData #34569

RezaJooyandeh · 2020-04-06T01:54:04Z

Fix NullReferenceException on serialization when JsonExtensionData dictionary has a custom JsonConverter.

layomia

Thanks for the PR. Just a few comments.

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

layomia · 2020-04-06T05:24:53Z

src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs

+        {
+            public override Dictionary<string, object> Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options)
+            {
+                throw new NotImplementedException();


Let's add a test for deserialization that ensures that the converter is not called.

Prior to this PR the cache gets populated based on the converter that is specified by [JsonConverter] or through JsonSerializerOptions, but upon serialization/deserialization it is assuming that we have a JsonDictionaryConverter<>. My PR makes sure for writing we do not make that assumption, but for reading it is more complicated, because we store only one converter per property. If we want different behaviors for read and write, we need to have the custom serializer registered for write and JsonDictionaryConverter<> for read, at lease for the extension data.

Current code

JsonConverter<object> converter = (JsonConverter<object>)state.Current.JsonPropertyInfo!.RuntimeClassInfo.ElementClassInfo!.PropertyInfoForClassInfo.ConverterBase;

Assumes that ElementClassInfo is not null which is not correct in this case and currently it throws with NullReferenceException so the suggested test would fail. I think better to fix that in a separate PR.

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

Line 219 in c653291

JsonConverter<object> converter = (JsonConverter<object>)

We can mitigate the null ref by fetching the converter from the Options instance, rather than ElementClassInfo, similar to

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

Line 265 in c653291

JsonConverter<JsonElement> converter = (JsonConverter<JsonElement>)Options.GetConverter(typeof(JsonElement));

We should make the change in this PR.

The other option is to assign ElementType when handling ClassType.Value types during warm up, similar to

runtime/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs

Line 170 in c653291

ElementType = converter.ElementType;

.

We probably shouldn't go down this route since ElementType and ElementClassInfo are logically reserved for collections (de)serialized without custom converters.

In light of this discussion, let's add and test two more classes for deserialization (where the extension data element type is JsonElement), to ensure that the converter (another class to add) not called.

a test for deserialization where the converter is added to the options.Converters collection

a test for deserialization where JsonConverterAttribute is placed on the property.

Updated and tests are added

src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs

layomia · 2020-04-13T02:32:17Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs

@@ -217,7 +217,7 @@ public bool ReadJsonAndAddExtensionProperty(object obj, ref ReadStack state, ref
                else
                {
                    JsonConverter<object> converter = (JsonConverter<object>)
-                        state.Current.JsonPropertyInfo!.RuntimeClassInfo.ElementClassInfo!.PropertyInfoForClassInfo.ConverterBase;
+                        (state.Current.JsonPropertyInfo!.RuntimeClassInfo.ElementClassInfo?.PropertyInfoForClassInfo.ConverterBase ?? Options.GetConverter(typeof(object)));


For simplicity, this should just be

JsonConverter<object> converter = (JsonConverter<object>)Options.GetConverter(typeof(object)));

This change shouldn't have any noticeable perf impact.

We need a similar change on line 237. The tests I suggest below will fail if this change isn't made.

JsonConverter<JsonElement> converter = (JsonConverter<JsonElement>)Options.GetConverter(typeof(JsonElement)));

layomia · 2020-04-13T05:48:16Z

src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs

+            Assert.Equal("TestValue", ((JsonElement)obj.Overflow["TestKey"]).GetString());
+        }
+
+        private class ClassWithJsonElementExtensionData


We need another class where the extension property doesn't have the converter attribute and a test (for serialization and deserialization) where the converter is added at runtime with options.Converter.Add.

For completeness, we should also add tests (serialization and deserialization) for a class where the extension property dictionary is a custom type, and the JsonConverterAttribute is applied on the custom type itself.

The new test covers them

layomia · 2020-04-13T05:51:29Z

src/libraries/System.Text.Json/tests/Serialization/ExtensionDataTests.cs

+            ClassWithJsonElementExtensionData obj
+                = JsonSerializer.Deserialize<ClassWithJsonElementExtensionData>(@"{""TestKey"":""TestValue""}");
+
+            Assert.Equal("TestValue", obj.Overflow["TestKey"].GetString());


We should test serialization here (Assert.Throws is fine).

The new test covers both serialization and deserialization

layomia · 2020-04-13T05:54:04Z

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs

@@ -285,8 +285,17 @@ internal bool TryWrite(Utf8JsonWriter writer, T value, JsonSerializerOptions opt

        internal bool TryWriteDataExtensionProperty(Utf8JsonWriter writer, T value, JsonSerializerOptions options, ref WriteStack state)
        {
+            Debug.Assert(!(value is null));


nit:

Suggested change

Debug.Assert(!(value is null));

Debug.Assert(value != null);

for consistency with the rest of the project.

RezaJooyandeh · 2020-04-14T06:32:51Z

@layomia I changed the tests to ensure we cover all the combinations:

{Serialization|Deserialization}
{IDictionary<string, object|JsonElement>}
{Dictionary|CustomDictionary}
{Explict|Using options}

Fix NullReferenceException on serialization when JsonExtensionData dictionary has a custom JsonConverter. Fix #32903

layomia

Thanks!

It would be good to add a test where the converter is placed on the type itself as described in #34569 (comment). I'll pick this up in a follow up.

layomia · 2020-04-14T23:54:47Z

Test failure unrelated.

RezaJooyandeh requested review from ahsonkhan, jozkee, layomia and steveharter as code owners April 6, 2020 01:54

Dotnet-GitSync-Bot added the area-System.Text.Json label Apr 6, 2020

layomia reviewed Apr 6, 2020

View reviewed changes

layomia added this to the 5.0 milestone Apr 7, 2020

layomia assigned RezaJooyandeh Apr 7, 2020

RezaJooyandeh requested a review from layomia April 12, 2020 15:59

layomia reviewed Apr 13, 2020

View reviewed changes

RezaJooyandeh added 4 commits April 14, 2020 07:27

Fix NullReference on JsonExtensionData

0809f98

Fix NullReferenceException on serialization when JsonExtensionData dictionary has a custom JsonConverter. Fix #32903

Add test for converters declared through attribute

5c0093e

Ensure custom serialization is not used for reading JsonExtensionData

5f78135

Cover all the combinations in tests

e13ab10

jaredpar mentioned this pull request Apr 14, 2020

Microsoft.Extensions.Caching.Memory.Test hitting FailFast in CI #34952

Closed

layomia approved these changes Apr 14, 2020

View reviewed changes

layomia merged commit cfb109b into dotnet:master Apr 14, 2020

RezaJooyandeh deleted the issue-32903 branch April 15, 2020 00:46

layomia mentioned this pull request May 6, 2020

Add opt-in for converters to handle null #35826

Merged

ghost locked as resolved and limited conversation to collaborators Dec 9, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NullReference on JsonExtensionData #34569

Fix NullReference on JsonExtensionData #34569

RezaJooyandeh commented Apr 6, 2020

layomia left a comment

layomia Apr 6, 2020

RezaJooyandeh Apr 7, 2020

layomia Apr 7, 2020

layomia Apr 7, 2020

RezaJooyandeh Apr 12, 2020

layomia Apr 13, 2020

layomia Apr 13, 2020

RezaJooyandeh Apr 14, 2020 •

edited

Loading

layomia Apr 13, 2020

layomia Apr 13, 2020

RezaJooyandeh Apr 14, 2020

layomia Apr 13, 2020

RezaJooyandeh Apr 14, 2020

layomia Apr 13, 2020 •

edited

Loading

RezaJooyandeh Apr 14, 2020

RezaJooyandeh commented Apr 14, 2020 •

edited

Loading

layomia left a comment

layomia commented Apr 14, 2020

Fix NullReference on JsonExtensionData #34569

Fix NullReference on JsonExtensionData #34569

Conversation

RezaJooyandeh commented Apr 6, 2020

layomia left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RezaJooyandeh Apr 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

layomia Apr 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RezaJooyandeh commented Apr 14, 2020 • edited Loading

layomia left a comment

Choose a reason for hiding this comment

layomia commented Apr 14, 2020

RezaJooyandeh Apr 14, 2020 •

edited

Loading

layomia Apr 13, 2020 •

edited

Loading

RezaJooyandeh commented Apr 14, 2020 •

edited

Loading