Address review feedback

dotnet · Apr 29, 2021 · 5d5b563 · 5d5b563
1 parent 050a476
commit 5d5b563
Show file tree

Hide file tree

Showing 5 changed files with 102 additions and 35 deletions.
diff --git a/...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
@@ -4,7 +4,6 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.Diagnostics;
-using System.Diagnostics.CodeAnalysis;
 using System.Reflection;
 using System.Text.Json.Serialization;
 using System.Text.Json.Serialization.Converters;
@@ -26,16 +25,6 @@ public sealed partial class JsonSerializerOptions
         // The cached converters (custom or built-in).
         private readonly ConcurrentDictionary<Type, JsonConverter?> _converters = new ConcurrentDictionary<Type, JsonConverter?>();
 
-        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
-        internal void RootBuiltInConvertersAndTypeInfoCreator()
-        {
-            RootBuiltInConverters();
-            _typeInfoCreationFunc ??= CreateJsonTypeInfo;
-
-            [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
-            static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options);
-        }
-
         internal void RootBuiltInConverters()
         {
             s_defaultSimpleConverters ??= GetDefaultSimpleConverters();

diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
@@ -4,6 +4,7 @@
 using System.Collections.Concurrent;
 using System.ComponentModel;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Runtime.CompilerServices;
 using System.Text.Encodings.Web;
 using System.Text.Json.Node;
@@ -569,6 +570,16 @@ internal MemberAccessor MemberAccessorStrategy
             }
         }
 
+        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+        internal void RootBuiltInConvertersAndTypeInfoCreator()
+        {
+            RootBuiltInConverters();
+            _typeInfoCreationFunc ??= CreateJsonTypeInfo;
+
+            [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+            static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) => new JsonTypeInfo(type, options);
+        }
+
         internal JsonTypeInfo GetOrAddClass(Type type)
         {
             _haveTypesBeenCreated = true;

diff --git a/...ystem.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSourceGeneratorTests.cs b/...ystem.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSourceGeneratorTests.cs
@@ -3,7 +3,9 @@
 
 using System.Collections.Generic;
 using System.Linq;
+using System.Reflection;
 using System.Text.Json.Serialization;
+using System.Text.Json.Serialization.Metadata;
 using System.Text.Json.SourceGeneration.Tests;
 using System.Text.Json.SourceGeneration.Tests.JsonSourceGeneration;
 using Xunit;
@@ -468,17 +470,82 @@ public class ClassWithEnumAndNullable
         }
 
         [Fact]
-        public static void PolymorphicSerializationTypeNotIndicated()
+        public static void Converters_AndTypeInfoCreator_NotRooted_WhenMetadataNotPresent()
         {
-            MyStruct myStruct = new();
+            object[] objArr = new object[] { new MyStruct() };
+
             // Metadata not generated for MyStruct without JsonSerializableAttribute.
             NotSupportedException ex = Assert.Throws<NotSupportedException>(
-                () => JsonSerializer.Serialize(new object[] { myStruct }, JsonContext.Default.ObjectArray));
+                () => JsonSerializer.Serialize(objArr, JsonContext.Default.ObjectArray));
             string exAsStr = ex.ToString();
             Assert.Contains(typeof(MyStruct).ToString(), exAsStr);
             Assert.Contains("JsonSerializerOptions", exAsStr);
+
+            // This test uses reflection to:
+            // - Access JsonSerializerOptions.s_defaultSimpleConverters
+            // - Access JsonSerializerOptions.s_defaultFactoryConverters
+            // - Access JsonSerializerOptions._typeInfoCreationFunc
+            //
+            // If any of them changes, this test will need to be kept in sync.
+
+            // Confirm built-in converters not set.
+            AssertFieldNull("s_defaultSimpleConverters", optionsInstance: null);
+            AssertFieldNull("s_defaultFactoryConverters", optionsInstance: null);
+
+            // Confirm type info dynamic creator not set.
+            AssertFieldNull("_typeInfoCreationFunc", JsonContext.Default.Options);
+
+            static void AssertFieldNull(string fieldName, JsonSerializerOptions? optionsInstance)
+            {
+                BindingFlags bindingFlags = BindingFlags.NonPublic | (optionsInstance == null ? BindingFlags.Static : BindingFlags.Instance);
+                FieldInfo fieldInfo = typeof(JsonSerializerOptions).GetField(fieldName, bindingFlags);
+                Assert.NotNull(fieldInfo);
+                Assert.Null(fieldInfo.GetValue(optionsInstance));
+            }
+        }
+
+        private const string ExceptionMessageFromCustomContext = "Exception thrown from custom context.";
+
+        [Fact]
+        public static void GetTypeInfoCalledDuringPolymorphicSerialization()
+        {
+            CustomContext context = new(new JsonSerializerOptions());
+
+            // Empty array is fine since we don't need metadata for children.
+            Assert.Equal("[]", JsonSerializer.Serialize(Array.Empty<object>(), context.ObjectArray));
+            Assert.Equal("[]", JsonSerializer.Serialize(Array.Empty<object>(), typeof(object[]), context));
+
+            // GetTypeInfo method called to get metadata for element run-time type.
+            object[] objArr = new object[] { new MyStruct() };
+
+            InvalidOperationException ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(objArr, context.ObjectArray));
+            Assert.Contains(ExceptionMessageFromCustomContext, ex.ToString());
+
+            ex = Assert.Throws<InvalidOperationException>(() => JsonSerializer.Serialize(objArr, typeof(object[]), context));
+            Assert.Contains(ExceptionMessageFromCustomContext, ex.ToString());
         }
 
         internal struct MyStruct { }
+
+        internal class CustomContext : JsonSerializerContext
+        {
+            public CustomContext(JsonSerializerOptions options) : base(options) { }
+
+            private JsonTypeInfo<object> _object;
+            public JsonTypeInfo<object> Object => _object ??= JsonMetadataServices.CreateValueInfo<object>(Options, JsonMetadataServices.ObjectConverter);
+
+            private JsonTypeInfo<object[]> _objectArray;
+            public JsonTypeInfo<object[]> ObjectArray => _objectArray ??= JsonMetadataServices.CreateArrayInfo<object>(Options, Object, default);
+
+            public override JsonTypeInfo GetTypeInfo(Type type)
+            {
+                if (type == typeof(object[]))
+                {
+                    return ObjectArray;
+                }
+
+                throw new InvalidOperationException(ExceptionMessageFromCustomContext);
+            }
+        }
     }
 }
diff --git a/...n/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs b/...n/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.cs
@@ -160,5 +160,26 @@ public static void VerifyObjectConverterWithPreservedReferences()
             Assert.IsType<bool>(obj);
             Assert.Equal(true, obj);
         }
+
+        [Fact]
+        public static void GetConverterRootsBuiltInConverters()
+        {
+            JsonSerializerOptions options = new();
+            RunTest<DateTime>();
+            RunTest<Point_2D>();
+
+            void RunTest<TConverterReturn>()
+            {
+                JsonConverter converter = options.GetConverter(typeof(TConverterReturn));
+                Assert.NotNull(converter);
+                Assert.True(converter is JsonConverter<TConverterReturn>);
+            }
+        }
+
+        [Fact]
+        public static void GetConverterTypeToConvertNull()
+        {
+            Assert.Throws<ArgumentNullException>(() => (new JsonSerializerOptions()).GetConverter(typeToConvert: null!));
+        }
     }
 }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs
@@ -738,26 +738,5 @@ public static void ConverterRead_VerifyInvalidTypeToConvertFails()
                 }
             }
         }
-
-        [Fact]
-        public static void GetConverterRootsBuiltInConverters()
-        {
-            JsonSerializerOptions options = new();
-            RunTest<DateTime>();
-            RunTest<Point_2D>();
-
-            void RunTest<TConverterReturn>()
-            {
-                JsonConverter converter = options.GetConverter(typeof(TConverterReturn));
-                Assert.NotNull(converter);
-                Assert.True(converter is JsonConverter<TConverterReturn>);
-            }
-        }
-
-        [Fact]
-        public static void GetConverterTypeToConvertNull()
-        {
-            Assert.Throws<ArgumentNullException>(() => (new JsonSerializerOptions()).GetConverter(typeToConvert: null!));
-        }
     }
 }