Fix default handling for value types when converter based on interface (#42319)

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 (file)
--- 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 @@
     <Compile Include="System\Text\Json\Serialization\JsonParameterInfoOfT.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonPropertyInfo.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonPropertyInfoOfT.cs" />
+    <Compile Include="System\Text\Json\Serialization\GenericMethodHolder.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonResumableConverterOfT.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonSerializer.cs" />
     <Compile Include="System\Text\Json\Serialization\JsonSerializer.Read.HandleMetadata.cs" />

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 (file)
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
+{
+    /// <summary>
+    /// Allows virtual dispatch to GenericMethodHolder{T}.
+    /// </summary>
+    internal abstract class GenericMethodHolder
+    {
+        /// <summary>
+        /// Returns true if <param name="value"/> contains only default values.
+        /// </summary>
+        public abstract bool IsDefaultValue(object value);
+    }
+
+    /// <summary>
+    /// Generic methods for {T}.
+    /// </summary>
+    internal sealed class GenericMethodHolder<T> : 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<T>.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 (file)
--- 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
         /// </remarks>
         public JsonPropertyInfo PropertyInfoForClassInfo { get; private set; }
 
+        private GenericMethodHolder? _genericMethods;
+        /// <summary>
+        /// Returns a helper class used when generic methods need to be invoked on Type.
+        /// </summary>
+        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 (file)
--- 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 (file)
--- 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
         /// </summary>
         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<object, T>? Get { get; private set; }
         public Action<object, T>? 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<T>.Default.Equals(default, value)))
+            if (IgnoreDefaultValuesOnWrite)
             {
-                return true;
+                // If value is null, it is a reference type or nullable<T>.
+                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<T>.Default.Equals(default, value))
+                        {
+                            return true;
+                        }
+                    }
+                    else
+                    {
+                        Debug.Assert(RuntimeClassInfo.Type == DeclaredPropertyType);
+
+                        // Use a late-bound call to EqualityComparer<DeclaredPropertyType>.
+                        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 (file)
--- 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<IUseCustomConverter>
+        {
+            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<MyValueTypeWithProperties>.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<MyValueTypeWithProperties>.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<MyValueTypeWithProperties>.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<MyValueTypeWithProperties>.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<NotImplementedException>(() => JsonSerializer.Deserialize<MyValueTypeWithProperties>("{}"));
+
+            var options = new JsonSerializerOptions
+            {
+                IgnoreNullValues = true
+            };
+
+            Assert.Throws<NotImplementedException>(() => JsonSerializer.Deserialize<MyValueTypeWithProperties>("{}", options));
+        }
+
+        [Fact]
+        public static void JsonIgnoreCondition_WhenWritingNull_OnValueTypeWithCustomConverter()
+        {
+            string json;
+            var obj = new MyClassWithValueType();
+
+            // Baseline without custom options.
+            Assert.True(EqualityComparer<MyValueTypeWithProperties>.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<MyValueTypeWithProperties>.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);
+        }
     }
 }