From 710118d5d3c6175ec02c6f4a719de702496884f7 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) --- .../System.Text.Json/src/System.Text.Json.csproj | 1 + .../Text/Json/Serialization/GenericMethodHolder.cs | 34 ++++ .../Text/Json/Serialization/JsonClassInfo.cs | 18 ++ .../Text/Json/Serialization/JsonPropertyInfo.cs | 1 + .../Text/Json/Serialization/JsonPropertyInfoOfT.cs | 55 ++++-- .../tests/Serialization/PropertyVisibilityTests.cs | 184 +++++++++++++++++++++ 6 files changed, 281 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 e7bec05..7542db8 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -143,6 +143,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 0000000..ef8e362 --- /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 6f523b1..8f2f195 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 @@ namespace System.Text.Json /// 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 4d7d9f4..f132019 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 5bf21f3..612db8c 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 @@ namespace System.Text.Json /// 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 @@ namespace System.Text.Json } _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 @@ namespace System.Text.Json { 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 @@ namespace System.Text.Json bool isNullToken = reader.TokenType == JsonTokenType.Null; if (isNullToken && !Converter.HandleNullOnRead && !state.IsContinuation) { - if (!Converter.CanBeNull) + if (!_propertyTypeCanBeNull) { ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(Converter.TypeToConvert); } @@ -225,7 +256,7 @@ namespace System.Text.Json // CanUseDirectReadOrWrite == false when using streams Debug.Assert(!state.IsContinuation); - if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull) + if (!isNullToken || !IgnoreDefaultValuesOnRead || !_propertyTypeCanBeNull) { // Optimize for internal converters by avoiding the extra call to TryRead. T fastValue = Converter.Read(ref reader, RuntimePropertyType!, Options); @@ -237,7 +268,7 @@ namespace System.Text.Json else { success = true; - if (!isNullToken || !IgnoreDefaultValuesOnRead || !Converter.CanBeNull || state.IsContinuation) + if (!isNullToken || !IgnoreDefaultValuesOnRead || !_propertyTypeCanBeNull || state.IsContinuation) { success = Converter.TryRead(ref reader, RuntimePropertyType!, Options, ref state, out T value); if (success) @@ -274,7 +305,7 @@ namespace System.Text.Json 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 0628133..539ef0d 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 @@ namespace System.Text.Json.Serialization.Tests [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); + } } } -- 2.7.4