From 1374897e99a97814374a8e2802765ccc84a79280 Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Thu, 17 Sep 2020 09:58:11 -0500 Subject: [PATCH] Fix default handling for value types when converter based on interface (#42319) --- .../src/System.Text.Json.csproj | 1 + .../Json/Serialization/GenericMethodHolder.cs | 34 ++++ .../Text/Json/Serialization/JsonClassInfo.cs | 18 ++ .../Json/Serialization/JsonPropertyInfo.cs | 1 + .../Json/Serialization/JsonPropertyInfoOfT.cs | 58 ++++-- .../Serialization/PropertyVisibilityTests.cs | 184 ++++++++++++++++++ 6 files changed, 284 insertions(+), 12 deletions(-) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index fc5c8ce5591c9..b915e3ec37bf3 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -142,6 +142,7 @@ + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs new file mode 100644 index 0000000000000..ef8e362e1a695 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/GenericMethodHolder.cs @@ -0,0 +1,34 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Collections.Generic; +using System.Diagnostics; + +namespace System.Text.Json.Serialization +{ + /// + /// Allows virtual dispatch to GenericMethodHolder{T}. + /// + internal abstract class GenericMethodHolder + { + /// + /// Returns true if contains only default values. + /// + public abstract bool IsDefaultValue(object value); + } + + /// + /// Generic methods for {T}. + /// + internal sealed class GenericMethodHolder : GenericMethodHolder + { + public override bool IsDefaultValue(object value) + { + // For performance, we only want to call this method for non-nullable value types. + // Nullable types should be checked againt 'null' before calling this method. + Debug.Assert(Nullable.GetUnderlyingType(value.GetType()) == null); + + return EqualityComparer.Default.Equals(default, (T)value); + } + } +} diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs index 6f523b19b8294..8f2f195517ecb 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs @@ -79,6 +79,24 @@ public JsonClassInfo? ElementClassInfo /// public JsonPropertyInfo PropertyInfoForClassInfo { get; private set; } + private GenericMethodHolder? _genericMethods; + /// + /// Returns a helper class used when generic methods need to be invoked on Type. + /// + public GenericMethodHolder GenericMethods + { + get + { + if (_genericMethods == null) + { + Type runtimePropertyClass = typeof(GenericMethodHolder<>).MakeGenericType(new Type[] { Type })!; + _genericMethods = (GenericMethodHolder)Activator.CreateInstance(runtimePropertyClass)!; + } + + return _genericMethods; + } + } + public JsonClassInfo(Type type, JsonSerializerOptions options) { Type = type; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs index cfb6e69833408..2552f30a5a378 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfo.cs @@ -12,6 +12,7 @@ namespace System.Text.Json internal abstract class JsonPropertyInfo { public static readonly JsonPropertyInfo s_missingProperty = GetPropertyPlaceholder(); + public static readonly Type s_NullableType = typeof(Nullable<>); private JsonClassInfo? _runtimeClassInfo; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs index 48f952a8a19a7..98482c8906c34 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs @@ -23,6 +23,11 @@ internal sealed class JsonPropertyInfo : JsonPropertyInfo /// private bool _converterIsExternalAndPolymorphic; + // Since a converter's TypeToConvert (which is the T value in this type) can be different than + // the property's type, we track that and whether the property type can be null. + private bool _propertyTypeEqualsTypeToConvert; + private bool _propertyTypeCanBeNull; + public Func? Get { get; private set; } public Action? Set { get; private set; } @@ -100,7 +105,11 @@ public override void Initialize( } _converterIsExternalAndPolymorphic = !converter.IsInternalConverter && DeclaredPropertyType != converter.TypeToConvert; - GetPolicies(ignoreCondition, parentTypeNumberHandling, defaultValueIsNull: Converter.CanBeNull); + _propertyTypeCanBeNull = !DeclaredPropertyType.IsValueType || + (DeclaredPropertyType.IsGenericType && DeclaredPropertyType.GetGenericTypeDefinition() == s_NullableType); + _propertyTypeEqualsTypeToConvert = typeof(T) == DeclaredPropertyType; + + GetPolicies(ignoreCondition, parentTypeNumberHandling, defaultValueIsNull: _propertyTypeCanBeNull); } public override JsonConverter ConverterBase @@ -131,18 +140,40 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf { T value = Get!(obj); - // Since devirtualization only works in non-shared generics, - // the default comparer is used only for value types for now. - // For reference types there is a quick check for null. - if (IgnoreDefaultValuesOnWrite && ( - default(T) == null ? value == null : EqualityComparer.Default.Equals(default, value))) + if (IgnoreDefaultValuesOnWrite) { - return true; + // If value is null, it is a reference type or nullable. + if (value == null) + { + return true; + } + + if (!_propertyTypeCanBeNull) + { + if (_propertyTypeEqualsTypeToConvert) + { + // The converter and property types are the same, so we can use T for EqualityComparer<>. + if (EqualityComparer.Default.Equals(default, value)) + { + return true; + } + } + else + { + Debug.Assert(RuntimeClassInfo.Type == DeclaredPropertyType); + + // Use a late-bound call to EqualityComparer. + if (RuntimeClassInfo.GenericMethods.IsDefaultValue(value)) + { + return true; + } + } + } } if (value == null) { - Debug.Assert(Converter.CanBeNull); + Debug.Assert(_propertyTypeCanBeNull); if (Converter.HandleNullOnWrite) { @@ -205,7 +236,7 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U bool isNullToken = reader.TokenType == JsonTokenType.Null; if (isNullToken && !Converter.HandleNullOnRead && !state.IsContinuation) { - if (!Converter.CanBeNull) + if (!_propertyTypeCanBeNull) { ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(Converter.TypeToConvert); } @@ -222,7 +253,10 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U } else if (Converter.CanUseDirectReadOrWrite && state.Current.NumberHandling == null) { - if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) + // CanUseDirectReadOrWrite == false when using streams + Debug.Assert(!state.IsContinuation); + + if (!isNullToken || !IgnoreDefaultValuesOnRead || !_propertyTypeCanBeNull) { // Optimize for internal converters by avoiding the extra call to TryRead. T fastValue = Converter.Read(ref reader, RuntimePropertyType!, Options); @@ -234,7 +268,7 @@ public override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref U else { success = true; - if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) + if (!isNullToken || !IgnoreDefaultValuesOnRead || !_propertyTypeCanBeNull || state.IsContinuation) { success = Converter.TryRead(ref reader, RuntimePropertyType!, Options, ref state, out T value); if (success) @@ -271,7 +305,7 @@ public override bool ReadJsonAsObject(ref ReadStack state, ref Utf8JsonReader re bool isNullToken = reader.TokenType == JsonTokenType.Null; if (isNullToken && !Converter.HandleNullOnRead && !state.IsContinuation) { - if (!Converter.CanBeNull) + if (!_propertyTypeCanBeNull) { ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(Converter.TypeToConvert); } diff --git a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs index 062813373ae21..539ef0d89daec 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/PropertyVisibilityTests.cs @@ -2367,5 +2367,189 @@ private struct StructWithBadIgnoreAttribute [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] public Point_2D_Struct MyBadMember { get; set; } } + + private interface IUseCustomConverter { } + + [JsonConverter(typeof(MyCustomConverter))] + private struct MyValueTypeWithProperties : IUseCustomConverter + { + public int PrimitiveValue { get; set; } + public object RefValue { get; set; } + } + + private class MyCustomConverter : JsonConverter + { + public override bool CanConvert(Type typeToConvert) + { + return typeof(IUseCustomConverter).IsAssignableFrom(typeToConvert); + } + + public override IUseCustomConverter Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) => + throw new NotImplementedException(); + + public override void Write(Utf8JsonWriter writer, IUseCustomConverter value, JsonSerializerOptions options) + { + MyValueTypeWithProperties obj = (MyValueTypeWithProperties)value; + writer.WriteNumberValue(obj.PrimitiveValue + 100); + // Ignore obj.RefValue + } + } + + private class MyClassWithValueType + { + public MyClassWithValueType() { } + + public MyValueTypeWithProperties Value { get; set; } + } + + [Fact] + public static void JsonIgnoreCondition_WhenWritingDefault_OnValueTypeWithCustomConverter() + { + var obj = new MyClassWithValueType(); + + // Baseline without custom options. + Assert.True(EqualityComparer.Default.Equals(default, obj.Value)); + string json = JsonSerializer.Serialize(obj); + Assert.Equal("{\"Value\":100}", json); + + var options = new JsonSerializerOptions + { + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault + }; + + // Verify ignored. + Assert.True(EqualityComparer.Default.Equals(default, obj.Value)); + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{}", json); + + // Change a primitive value so it's no longer a default value. + obj.Value = new MyValueTypeWithProperties { PrimitiveValue = 1 }; + Assert.False(EqualityComparer.Default.Equals(default, obj.Value)); + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"Value\":101}", json); + + // Change reference value so it's no longer a default value. + obj.Value = new MyValueTypeWithProperties { RefValue = 1 }; + Assert.False(EqualityComparer.Default.Equals(default, obj.Value)); + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"Value\":100}", json); + } + + [Fact] + public static void JsonIgnoreCondition_ConverterCalledOnDeserialize() + { + // Verify converter is called. + Assert.Throws(() => JsonSerializer.Deserialize("{}")); + + var options = new JsonSerializerOptions + { + IgnoreNullValues = true + }; + + Assert.Throws(() => JsonSerializer.Deserialize("{}", options)); + } + + [Fact] + public static void JsonIgnoreCondition_WhenWritingNull_OnValueTypeWithCustomConverter() + { + string json; + var obj = new MyClassWithValueType(); + + // Baseline without custom options. + Assert.True(EqualityComparer.Default.Equals(default, obj.Value)); + json = JsonSerializer.Serialize(obj); + Assert.Equal("{\"Value\":100}", json); + + var options = new JsonSerializerOptions + { + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull + }; + + // Verify not ignored; MyValueTypeWithProperties is not null. + Assert.True(EqualityComparer.Default.Equals(default, obj.Value)); + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"Value\":100}", json); + } + + [Fact] + public static void JsonIgnoreCondition_WhenWritingDefault_OnRootTypes() + { + string json; + int i = 0; + object obj = null; + + // Baseline without custom options. + json = JsonSerializer.Serialize(obj); + Assert.Equal("null", json); + + json = JsonSerializer.Serialize(i); + Assert.Equal("0", json); + + var options = new JsonSerializerOptions + { + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault + }; + + // We don't ignore when applied to root types; only properties. + + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("null", json); + + json = JsonSerializer.Serialize(i, options); + Assert.Equal("0", json); + } + + private struct MyValueTypeWithBoxedPrimitive + { + public object BoxedPrimitive { get; set; } + } + + [Fact] + public static void JsonIgnoreCondition_WhenWritingDefault_OnBoxedPrimitive() + { + string json; + + MyValueTypeWithBoxedPrimitive obj = new MyValueTypeWithBoxedPrimitive { BoxedPrimitive = 0 }; + + // Baseline without custom options. + json = JsonSerializer.Serialize(obj); + Assert.Equal("{\"BoxedPrimitive\":0}", json); + + var options = new JsonSerializerOptions + { + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingDefault + }; + + // No check if the boxed object's value type is a default value (0 in this case). + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{\"BoxedPrimitive\":0}", json); + + obj = new MyValueTypeWithBoxedPrimitive(); + json = JsonSerializer.Serialize(obj, options); + Assert.Equal("{}", json); + } + + private class MyClassWithValueTypeInterfaceProperty + { + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingDefault)] + public IInterface MyProp { get; set; } + + public interface IInterface { } + public struct MyStruct : IInterface { } + } + + [Fact] + public static void JsonIgnoreCondition_WhenWritingDefault_OnInterface() + { + // MyProp should be ignored due to [JsonIgnore]. + var obj = new MyClassWithValueTypeInterfaceProperty(); + string json = JsonSerializer.Serialize(obj); + Assert.Equal("{}", json); + + // No check if the interface property's value type is a default value. + obj = new MyClassWithValueTypeInterfaceProperty { MyProp = new MyClassWithValueTypeInterfaceProperty.MyStruct() }; + json = JsonSerializer.Serialize(obj); + Assert.Equal("{\"MyProp\":{}}", json); + } } }