Address pending feedback and tidy up DefaultJsonTypeInfoResolver rooting logic.

eiriktsarpalis · eiriktsarpalis · commit 0d679d097c84 · 2023-11-27T15:50:17.000Z
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
@@ -41,12 +41,7 @@ public static JsonSerializerOptions Default
             [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
             get
             {
-                if (s_defaultOptions is not JsonSerializerOptions options)
-                {
-                    options = GetOrCreateDefaultOptionsInstance();
-                }
-
-                return options;
+                return s_defaultOptions ?? GetOrCreateSingleton(ref s_defaultOptions, JsonSerializerDefaults.General);
             }
         }
 
@@ -66,12 +61,7 @@ public static JsonSerializerOptions Web
             [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
             get
             {
-                if (s_webOptions is not JsonSerializerOptions options)
-                {
-                    options = GetOrCreateWebOptionsInstance();
-                }
-
-                return options;
+                return s_webOptions ?? GetOrCreateSingleton(ref s_webOptions, JsonSerializerDefaults.Web);
             }
         }
 
@@ -777,7 +767,7 @@ private void ConfigureForJsonSerializer()
             {
                 // Even if a resolver has already been specified, we need to root
                 // the default resolver to gain access to the default converters.
-                DefaultJsonTypeInfoResolver defaultResolver = DefaultJsonTypeInfoResolver.RootDefaultInstance();
+                DefaultJsonTypeInfoResolver defaultResolver = DefaultJsonTypeInfoResolver.DefaultInstance;
 
                 switch (_typeInfoResolver)
                 {
@@ -963,30 +953,24 @@ protected override void OnCollectionModifying()
 
         [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
         [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
-        private static JsonSerializerOptions GetOrCreateDefaultOptionsInstance()
+        private static JsonSerializerOptions GetOrCreateSingleton(
+            ref JsonSerializerOptions? location,
+            JsonSerializerDefaults defaults)
         {
-            var options = new JsonSerializerOptions
+            var options = new JsonSerializerOptions(defaults)
             {
                 // Because we're marking the default instance as read-only,
                 // we need to specify a resolver instance for the case where
                 // reflection is disabled by default: use one that returns null for all types.
 
                 TypeInfoResolver = JsonSerializer.IsReflectionEnabledByDefault
-                    ? DefaultJsonTypeInfoResolver.RootDefaultInstance()
+                    ? DefaultJsonTypeInfoResolver.DefaultInstance
                     : JsonTypeInfoResolver.Empty,
 
                 _isReadOnly = true,
             };
 
-            return Interlocked.CompareExchange(ref s_defaultOptions, options, null) ?? options;
-        }
-
-        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
-        [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
-        private static JsonSerializerOptions GetOrCreateWebOptionsInstance()
-        {
-            var options = new JsonSerializerOptions(JsonSerializerDefaults.Web) { _isReadOnly = true };
-            return Interlocked.CompareExchange(ref s_webOptions, options, null) ?? s_webOptions;
+            return Interlocked.CompareExchange(ref location, options, null) ?? options;
         }
 
         [DebuggerBrowsable(DebuggerBrowsableState.Never)]
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.Converters.cs
@@ -19,8 +19,8 @@ public partial class DefaultJsonTypeInfoResolver
         [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
         private static JsonConverterFactory[] GetDefaultFactoryConverters()
         {
-            return new JsonConverterFactory[]
-            {
+            return
+            [
                 // Check for disallowed types.
                 new UnsupportedTypeConverterFactory(),
                 // Nullable converter should always be next since it forwards to any nullable type.
@@ -35,7 +35,7 @@ private static JsonConverterFactory[] GetDefaultFactoryConverters()
                 new IEnumerableConverterFactory(),
                 // Object should always be last since it converts any type.
                 new ObjectConverterFactory()
-            };
+            ];
         }
 
         private static Dictionary<Type, JsonConverter> GetDefaultSimpleConverters()
@@ -89,13 +89,14 @@ void Add(JsonConverter converter) =>
                 converters.Add(converter.Type!, converter);
         }
 
+        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+        [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
         private static JsonConverter GetBuiltInConverter(Type typeToConvert)
         {
-            Debug.Assert(s_defaultSimpleConverters != null);
-            Debug.Assert(s_defaultFactoryConverters != null);
+            s_defaultSimpleConverters ??= GetDefaultSimpleConverters();
+            s_defaultFactoryConverters ??= GetDefaultFactoryConverters();
 
-            JsonConverter? converter;
-            if (s_defaultSimpleConverters.TryGetValue(typeToConvert, out converter))
+            if (s_defaultSimpleConverters.TryGetValue(typeToConvert, out JsonConverter? converter))
             {
                 return converter;
             }
@@ -142,8 +143,6 @@ internal static bool TryGetDefaultSimpleConverter(Type typeToConvert, [NotNullWh
         [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
         internal static JsonConverter GetConverterForType(Type typeToConvert, JsonSerializerOptions options, bool resolveJsonConverterAttribute = true)
         {
-            RootDefaultInstance(); // Ensure default converters are rooted.
-
             // Priority 1: Attempt to get custom converter from the Converters list.
             JsonConverter? converter = options.GetConverterFromList(typeToConvert);
 
diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs
@@ -31,9 +31,6 @@ public DefaultJsonTypeInfoResolver() : this(mutable: true)
         private DefaultJsonTypeInfoResolver(bool mutable)
         {
             _mutable = mutable;
-
-            s_defaultFactoryConverters ??= GetDefaultFactoryConverters();
-            s_defaultSimpleConverters ??= GetDefaultSimpleConverters();
         }
 
         /// <summary>
@@ -127,21 +124,22 @@ bool IBuiltInJsonTypeInfoResolver.IsCompatibleWithOptions(JsonSerializerOptions
             // provided that no user extensions have been made on the class.
             => _modifiers is null or { Count: 0 } && GetType() == typeof(DefaultJsonTypeInfoResolver);
 
-        internal static bool IsDefaultInstanceRooted => s_defaultInstance is not null;
-        private static DefaultJsonTypeInfoResolver? s_defaultInstance;
-
-        [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
-        [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
-        internal static DefaultJsonTypeInfoResolver RootDefaultInstance()
+        internal static DefaultJsonTypeInfoResolver DefaultInstance
         {
-            if (s_defaultInstance is DefaultJsonTypeInfoResolver result)
+            [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
+            [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
+            get
             {
-                return result;
-            }
+                if (s_defaultInstance is DefaultJsonTypeInfoResolver result)
+                {
+                    return result;
+                }
 
-            var newInstance = new DefaultJsonTypeInfoResolver(mutable: false);
-            DefaultJsonTypeInfoResolver? originalInstance = Interlocked.CompareExchange(ref s_defaultInstance, newInstance, comparand: null);
-            return originalInstance ?? newInstance;
+                var newInstance = new DefaultJsonTypeInfoResolver(mutable: false);
+                return Interlocked.CompareExchange(ref s_defaultInstance, newInstance, comparand: null) ?? newInstance;
+            }
         }
+
+        private static DefaultJsonTypeInfoResolver? s_defaultInstance;
     }
 }
diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CacheTests.cs
@@ -257,7 +257,6 @@ static Func<JsonSerializerOptions, int> CreateCacheCountAccessor()
 
         [ActiveIssue("https://github.com/dotnet/runtime/issues/66232", TargetFrameworkMonikers.NetFramework)]
         [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]
-        [MemberData(nameof(GetJsonSerializerOptions))]
         public static void JsonSerializerOptions_ReuseConverterCaches()
         {
             // This test uses reflection to:
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
@@ -1039,11 +1039,27 @@ public static void JsonSerializerOptions_Default_IsReadOnly()
         [Fact]
         public static void JsonSerializerOptions_Web_MatchesConstructorWithJsonSerializerDefaults()
         {
-            var options = new JsonSerializerOptions(JsonSerializerDefaults.Web);
+            var options = new JsonSerializerOptions(JsonSerializerDefaults.Web)
+            {
+                TypeInfoResolver = JsonSerializerOptions.Default.TypeInfoResolver
+            };
+
             JsonSerializerOptions optionsSingleton = JsonSerializerOptions.Web;
+
+            Assert.Equal(JsonNamingPolicy.CamelCase, options.PropertyNamingPolicy);
+            Assert.True(options.PropertyNameCaseInsensitive);
+            Assert.Equal(JsonNumberHandling.AllowReadingFromString, options.NumberHandling);
+
             JsonTestHelper.AssertOptionsEqual(options, optionsSingleton);
         }
 
+        [Fact]
+        public static void JsonSerializerOptions_Web_SerializesWithExpectedSettings()
+        {
+            string json = JsonSerializer.Serialize(new { PropertyName = 42 }, JsonSerializerOptions.Web);
+            Assert.Equal("""{"propertyName":42}""", json);
+        }
+
         [Fact]
         public static void JsonSerializerOptions_Web_ReturnsSameInstance()
         {

Original file line number	Diff line number	Diff line change
`@@ -257,7 +257,6 @@ static Func<JsonSerializerOptions, int> CreateCacheCountAccessor()`
`257`	`257`
`258`	`258`	`[ActiveIssue("https://github.com/dotnet/runtime/issues/66232", TargetFrameworkMonikers.NetFramework)]`
`259`	`259`	`[ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]`
`260`		`- [MemberData(nameof(GetJsonSerializerOptions))]`
`261`	`260`	`public static void JsonSerializerOptions_ReuseConverterCaches()`
`262`	`261`	`{`
`263`	`262`	`// This test uses reflection to:`