From a717fa88e2c2d5eb4325aaf105a8a89bf1a4964e Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Thu, 14 Jul 2022 22:52:06 +0100 Subject: [PATCH 1/7] Remove fallback to reflection converter resolution --- .../src/Resources/Strings.resx | 3 - .../Json/Serialization/JsonConverterOfT.cs | 5 +- .../JsonSerializerOptions.Caching.cs | 2 +- .../JsonSerializerOptions.Converters.cs | 39 ++++-------- .../Metadata/CustomJsonTypeInfoOfT.cs | 4 +- .../DefaultJsonTypeInfoResolver.Converters.cs | 50 +++++++-------- .../Metadata/DefaultJsonTypeInfoResolver.cs | 6 +- .../Metadata/JsonTypeInfo.Cache.cs | 23 ++++--- .../Serialization/Metadata/JsonTypeInfo.cs | 62 ++++++++++++++----- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 28 ++++----- .../Text/Json/ThrowHelper.Serialization.cs | 6 -- .../RecordTests.fs | 27 ++++++++ .../Serialization/NullableTests.cs | 29 +++++++++ .../Serialization/OptionsTests.cs | 12 ++-- 14 files changed, 180 insertions(+), 116 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index f08e3e8e57b4b..507b75ed8b379 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -581,9 +581,6 @@ The generic type of the converter for property '{0}.{1}' must match with the specified converter type '{2}'. The converter must not be 'null'. - - Built-in type converters have not been initialized. There is no converter available for type '{0}'. To root all built-in converters, use a 'JsonSerializerOptions'-based method of the 'JsonSerializer'. - Metadata for type '{0}' was not provided by TypeInfoResolver of type '{1}'. If using source generation, ensure that all root types passed to the serializer have been indicated with 'JsonSerializableAttribute', along with any types that might be serialized polymorphically. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs index 0b7afd0c555f0..8be4212a95431 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs @@ -71,10 +71,7 @@ public override bool CanConvert(Type typeToConvert) internal sealed override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) { - return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options) - { - DefaultConverterForType = this - }; + return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options); } internal sealed override JsonParameterInfo CreateJsonParameterInfo() diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index c0809f187894d..aa84beb4514b4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -125,7 +125,7 @@ internal void ClearCaches() { Debug.Assert(IsLockedInstance); - if (_cachingContext is null && _typeInfoResolver is not null) + if (_cachingContext is null) { _cachingContext = TrackedCachingContexts.GetOrCreate(this); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index c28edbdaf3fa4..c7c34acc03f2d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -48,9 +48,9 @@ public JsonConverter GetConverter(Type typeToConvert) if (_typeInfoResolver is null) { - // Backward compatibility -- root the default reflection converters + // Backward compatibility -- root & query the default reflection converters // but do not populate the TypeInfoResolver setting. - DefaultJsonTypeInfoResolver.RootDefaultInstance(); + return DefaultJsonTypeInfoResolver.GetConverterForType(typeToConvert, this); } return GetConverterFromTypeInfo(typeToConvert); @@ -61,11 +61,11 @@ public JsonConverter GetConverter(Type typeToConvert) /// internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert) { - JsonConverter? converter; + JsonTypeInfo? jsonTypeInfo; if (IsLockedInstance) { - converter = GetCachingContext()?.GetOrAddJsonTypeInfo(typeToConvert)?.Converter; + jsonTypeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(typeToConvert); } else { @@ -73,10 +73,15 @@ internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert) // which means we need to go through TypeInfoResolver but without caching because that's the // only place which will have correct converter for JsonSerializerContext and reflection // based resolver. It will also work correctly for combined resolvers. - converter = GetTypeInfoNoCaching(typeToConvert)?.Converter; + jsonTypeInfo = GetTypeInfoNoCaching(typeToConvert); } - return converter ?? GetConverterFromListOrBuiltInConverter(typeToConvert); + if (jsonTypeInfo is null) + { + ThrowHelper.ThrowNotSupportedException_NoMetadataForType(typeToConvert, TypeInfoResolver); + } + + return jsonTypeInfo.Converter; } internal JsonConverter? GetConverterFromList(Type typeToConvert) @@ -92,28 +97,6 @@ internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert) return null; } - internal JsonConverter GetConverterFromListOrBuiltInConverter(Type typeToConvert) - { - JsonConverter? converter = GetConverterFromList(typeToConvert); - return GetCustomOrBuiltInConverter(typeToConvert, converter); - } - - internal JsonConverter GetCustomOrBuiltInConverter(Type typeToConvert, JsonConverter? converter) - { - // Attempt to get built-in converter. - converter ??= DefaultJsonTypeInfoResolver.GetBuiltInConverter(typeToConvert); - // Expand potential convert converter factory. - converter = ExpandConverterFactory(converter, typeToConvert); - - if (!converter.TypeToConvert.IsInSubtypeRelationshipWith(typeToConvert)) - { - ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), converter.TypeToConvert); - } - - CheckConverterNullabilityIsSameAsPropertyType(converter, typeToConvert); - return converter; - } - [return: NotNullIfNotNull("converter")] internal JsonConverter? ExpandConverterFactory(JsonConverter? converter, Type typeToConvert) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/CustomJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/CustomJsonTypeInfoOfT.cs index 53e0a5a68bc01..4b48d7b2d72ae 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/CustomJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/CustomJsonTypeInfoOfT.cs @@ -18,8 +18,8 @@ internal sealed class CustomJsonTypeInfo : JsonTypeInfo /// /// Creates serialization metadata for a type using a simple converter. /// - internal CustomJsonTypeInfo(JsonSerializerOptions options) - : base(options.GetConverterFromListOrBuiltInConverter(typeof(T)), options) + internal CustomJsonTypeInfo(JsonConverter converter, JsonSerializerOptions options) + : base(converter, options) { } 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 index 690281c6e0949..6d9f808ddd9bf 100644 --- 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 @@ -81,16 +81,10 @@ void Add(JsonConverter converter) => converters.Add(converter.TypeToConvert, converter); } - internal static JsonConverter GetBuiltInConverter(Type typeToConvert) + private static JsonConverter GetBuiltInConverter(Type typeToConvert) { - if (s_defaultSimpleConverters == null || s_defaultFactoryConverters == null) - { - // (De)serialization using serializer's options-based methods has not yet occurred, so the built-in converters are not rooted. - // Even though source-gen code paths do not call this method , we do not root all the - // built-in converters here since we fetch converters for any type included for source generation from the binded context (Priority 1). - ThrowHelper.ThrowNotSupportedException_BuiltInConvertersNotRooted(typeToConvert); - return null!; - } + Debug.Assert(s_defaultSimpleConverters != null); + Debug.Assert(s_defaultFactoryConverters != null); JsonConverter? converter; if (s_defaultSimpleConverters.TryGetValue(typeToConvert, out converter)) @@ -99,11 +93,11 @@ internal static JsonConverter GetBuiltInConverter(Type typeToConvert) } else { - foreach (JsonConverter item in s_defaultFactoryConverters) + foreach (JsonConverterFactory factory in s_defaultFactoryConverters) { - if (item.CanConvert(typeToConvert)) + if (factory.CanConvert(typeToConvert)) { - converter = item; + converter = factory; break; } } @@ -132,32 +126,26 @@ internal static bool TryGetDefaultSimpleConverter(Type typeToConvert, [NotNullWh // - the converter (either native or custom), if one exists. [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - internal static JsonConverter GetConverterForMember( - Type typeToConvert, - MemberInfo memberInfo, - JsonSerializerOptions options, - out JsonConverter? customConverter) + internal static JsonConverter? GetCustomConverterForMember(Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options) { Debug.Assert(memberInfo is FieldInfo or PropertyInfo); Debug.Assert(typeToConvert != null); JsonConverterAttribute? converterAttribute = memberInfo.GetUniqueCustomAttribute(inherit: false); - customConverter = converterAttribute is null ? null : GetConverterFromAttribute(converterAttribute, typeToConvert, memberInfo, options); - - return options.TryGetTypeInfoCached(typeToConvert, out JsonTypeInfo? typeInfo) - ? typeInfo.Converter - : GetConverterForType(typeToConvert, options); + return converterAttribute is null ? null : GetConverterFromAttribute(converterAttribute, typeToConvert, memberInfo, options); } [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - internal static JsonConverter GetConverterForType(Type typeToConvert, JsonSerializerOptions options) + 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); // Priority 2: Attempt to get converter from [JsonConverter] on the type being converted. - if (converter == null) + if (resolveJsonConverterAttribute && converter == null) { JsonConverterAttribute? converterAttribute = typeToConvert.GetUniqueCustomAttribute(inherit: false); if (converterAttribute != null) @@ -166,8 +154,18 @@ internal static JsonConverter GetConverterForType(Type typeToConvert, JsonSerial } } - // Priority 3: Fall back to built-in converters and validate result - return options.GetCustomOrBuiltInConverter(typeToConvert, converter); + // Priority 3: Query the built-in converters. + converter ??= GetBuiltInConverter(typeToConvert); + + // Expand if factory converter & validate. + converter = options.ExpandConverterFactory(converter, typeToConvert); + if (!converter.TypeToConvert.IsInSubtypeRelationshipWith(typeToConvert)) + { + ThrowHelper.ThrowInvalidOperationException_SerializationConverterNotCompatible(converter.GetType(), converter.TypeToConvert); + } + + JsonSerializerOptions.CheckConverterNullabilityIsSameAsPropertyType(converter, typeToConvert); + return converter; } [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] 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 index 4b4990818e1ce..44ea6c1b06fa0 100644 --- 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 @@ -90,7 +90,11 @@ private JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - private static JsonTypeInfo CreateReflectionJsonTypeInfo(JsonSerializerOptions options) => new ReflectionJsonTypeInfo(options); + private static JsonTypeInfo CreateReflectionJsonTypeInfo(JsonSerializerOptions options) + { + JsonConverter converter = GetConverterForType(typeof(T), options); + return new ReflectionJsonTypeInfo(converter, options); + } /// /// List of JsonTypeInfo modifiers. Modifying callbacks are called consecutively after initial resolution diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index 9e97d59d73b0b..ba669654518e3 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -54,20 +54,29 @@ public abstract partial class JsonTypeInfo [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType, JsonConverter converter) + internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType, JsonConverter? converter) { - Debug.Assert(propertyType.IsInSubtypeRelationshipWith(converter.TypeToConvert)); + Debug.Assert(converter is null || propertyType.IsInSubtypeRelationshipWith(converter.TypeToConvert)); - if (converter.TypeToConvert == propertyType) + JsonPropertyInfo jsonPropertyInfo; + + if (converter?.TypeToConvert == propertyType) { // For the vast majority of use cases, the converter type matches the property type. // Avoid reflection-based initialization by delegating JsonPropertyInfo construction to the converter itself. - return converter.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); + jsonPropertyInfo = converter.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); + } + else + { + // We don't have a converter yet or the converter does not match the declared member type. + // Use reflection to instantiate the correct JsonPropertyInfo + s_createJsonPropertyInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonPropertyInfo), BindingFlags.NonPublic | BindingFlags.Static)!; + MethodInfo factoryInfo = s_createJsonPropertyInfo.MakeGenericMethod(propertyType); + jsonPropertyInfo = (JsonPropertyInfo)factoryInfo.Invoke(null, new object[] { this, Options })!; } - s_createJsonPropertyInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonPropertyInfo), BindingFlags.NonPublic | BindingFlags.Static)!; - MethodInfo factoryInfo = s_createJsonPropertyInfo.MakeGenericMethod(propertyType); - return (JsonPropertyInfo)factoryInfo.Invoke(null, new object[] { this, Options })!; + jsonPropertyInfo.DefaultConverterForType = converter; + return jsonPropertyInfo; } private static JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index b08e40a5af731..c1a75e16df16b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -508,11 +508,22 @@ internal string GetDebugInfo() internal virtual void LateAddProperties() { } /// - /// Creates JsonTypeInfo + /// Creates a blank instance. /// - /// Type for which JsonTypeInfo stores metadata for - /// Options associated with JsonTypeInfo - /// JsonTypeInfo instance + /// The type for which contract metadata is specified. + /// The instance the metadata is associated with. + /// A blank instance. + /// is null. + /// + /// The returned will be blank, with the exception of the + /// property which will be resolved either from + /// or the built-in converters for the type. + /// Any converters specified via on the type declaration + /// will not be resolved by this method. + /// + /// What converter does get resolved influences the value of , + /// which constrains the type of metadata that can be modified in the instance. + /// [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] [RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] public static JsonTypeInfo CreateJsonTypeInfo(JsonSerializerOptions options) @@ -522,18 +533,30 @@ public static JsonTypeInfo CreateJsonTypeInfo(JsonSerializerOptions option ThrowHelper.ThrowArgumentNullException(nameof(options)); } - DefaultJsonTypeInfoResolver.RootDefaultInstance(); - return new CustomJsonTypeInfo(options); + JsonConverter converter = DefaultJsonTypeInfoResolver.GetConverterForType(typeof(T), options, resolveJsonConverterAttribute: false); + return new CustomJsonTypeInfo(converter, options); } private static MethodInfo? s_createJsonTypeInfo; /// - /// Creates JsonTypeInfo + /// Creates a blank instance. /// - /// Type for which JsonTypeInfo stores metadata for - /// Options associated with JsonTypeInfo - /// JsonTypeInfo instance + /// The type for which contract metadata is specified. + /// The instance the metadata is associated with. + /// A blank instance. + /// or is null. + /// cannot be used for serialization. + /// + /// The returned will be blank, with the exception of the + /// property which will be resolved either from + /// or the built-in converters for the type. + /// Any converters specified via on the type declaration + /// will not be resolved by this method. + /// + /// What converter does get resolved influences the value of , + /// which constrains the type of metadata that can be modified in the instance. + /// [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] [RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) @@ -553,18 +576,19 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(type), type, null, null); } - DefaultJsonTypeInfoResolver.RootDefaultInstance(); s_createJsonTypeInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonTypeInfo), new Type[] { typeof(JsonSerializerOptions) })!; return (JsonTypeInfo)s_createJsonTypeInfo.MakeGenericMethod(type) .Invoke(null, new object[] { options })!; } /// - /// Creates JsonPropertyInfo + /// Creates a blank instance for the current . /// - /// Type of the property - /// Name of the property - /// JsonPropertyInfo instance + /// The declared type for the property. + /// The property name used in JSON serialization and deserialization. + /// A blank instance. + /// or is null. + /// cannot be used for serialization. [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] [RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) @@ -584,7 +608,11 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); } - JsonConverter converter = Options.GetConverterFromListOrBuiltInConverter(propertyType); + JsonConverter? converter = + Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo) + ? jsonTypeInfo.Converter + : null; + JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType, converter); propertyInfo.Name = name; @@ -885,7 +913,7 @@ internal static bool IsValidExtensionDataProperty(JsonPropertyInfo jsonPropertyI // Avoid a reference to typeof(JsonNode) to support trimming. (memberType.FullName == JsonObjectTypeName && ReferenceEquals(memberType.Assembly, typeof(JsonTypeInfo).Assembly)); - return typeIsValid && jsonPropertyInfo.Options.GetConverterFromTypeInfo(memberType) != null; + return typeIsValid; } internal JsonPropertyDictionary CreatePropertyCache(int capacity) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index f7b91f3127fc1..c09f53ce4d006 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -15,13 +15,6 @@ namespace System.Text.Json.Serialization.Metadata /// internal sealed class ReflectionJsonTypeInfo : JsonTypeInfo { - [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - internal ReflectionJsonTypeInfo(JsonSerializerOptions options) - : this(DefaultJsonTypeInfoResolver.GetConverterForType(typeof(T), options), options) - { - } - [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions options) @@ -160,7 +153,7 @@ private void CacheMember( ref bool propertyOrderSpecified, ref Dictionary? ignoredMembers) { - JsonPropertyInfo? jsonPropertyInfo = AddProperty(typeToConvert, memberInfo, Options); + JsonPropertyInfo? jsonPropertyInfo = CreateProperty(typeToConvert, memberInfo, Options); if (jsonPropertyInfo == null) { // ignored invalid property @@ -177,7 +170,7 @@ private void CacheMember( Justification = "The ctor is marked as RequiresUnreferencedCode")] [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", Justification = "The ctor is marked RequiresDynamicCode.")] - private JsonPropertyInfo? AddProperty( + private JsonPropertyInfo? CreateProperty( Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options) @@ -192,19 +185,22 @@ private void CacheMember( ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(typeToConvert, memberInfo.DeclaringType, memberInfo); } - JsonConverter? customConverter; - JsonConverter converter; + // Resolve the default converter for the member type, if it has already been cached. + // If not found, converter resolution will be delayed until the Configure() stage. + JsonConverter? converter = + options.TryGetTypeInfoCached(typeToConvert, out JsonTypeInfo? jsonTypeInfo) + ? jsonTypeInfo.Converter + : null; + // Resolve any custom converters on the attribute level. + JsonConverter? customConverter; try { - converter = DefaultJsonTypeInfoResolver.GetConverterForMember( - typeToConvert, - memberInfo, - options, - out customConverter); + customConverter = DefaultJsonTypeInfoResolver.GetCustomConverterForMember(typeToConvert, memberInfo, options); } catch (InvalidOperationException) when (ignoreCondition == JsonIgnoreCondition.Always) { + // skip property altogether if attribute is invalid and the property is ignored return null; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index 508ff39e20564..d5d9b77787a20 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs @@ -644,12 +644,6 @@ internal static void ThrowUnexpectedMetadataException( } } - [DoesNotReturn] - public static void ThrowNotSupportedException_BuiltInConvertersNotRooted(Type type) - { - throw new NotSupportedException(SR.Format(SR.BuiltInConvertersNotRooted, type)); - } - [DoesNotReturn] public static void ThrowNotSupportedException_NoMetadataForType(Type type, IJsonTypeInfoResolver? resolver) { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.FSharp.Tests/RecordTests.fs b/src/libraries/System.Text.Json/tests/System.Text.Json.FSharp.Tests/RecordTests.fs index 0d78940904b52..d9ac41cbd2b41 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.FSharp.Tests/RecordTests.fs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.FSharp.Tests/RecordTests.fs @@ -49,3 +49,30 @@ let ``Support F# struct record serialization``() = let ``Support F# struct record deserialization``() = let result = JsonSerializer.Deserialize(MyStructRecord.ExpectedJson) Assert.Equal(MyStructRecord.Value, result) + +type RecursiveRecord = + { + Next : RecursiveRecord option + } + +[] +let ``Recursive records are supported``() = + let value = { Next = Some { Next = None } } + let json = JsonSerializer.Serialize(value) + Assert.Equal("""{"Next":{"Next":null}}""", json) + let deserializedValue = JsonSerializer.Deserialize(json) + Assert.Equal(value, deserializedValue) + +[] +type RecursiveStructRecord = + { + Next : RecursiveStructRecord voption [] + } + +[] +let ``Recursive struct records are supported``() = + let value = { Next = [| ValueSome { Next = [| ValueNone |] } |] } + let json = JsonSerializer.Serialize(value) + Assert.Equal("""{"Next":[{"Next":[null]}]}""", json) + let deserializedValue = JsonSerializer.Deserialize(json) + Assert.Equal(value, deserializedValue) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NullableTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NullableTests.cs index a30e05ecf81bf..c3888c8dc9675 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NullableTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/NullableTests.cs @@ -439,5 +439,34 @@ public struct Person public int? Age { get; set; } public DateTime? Birthday { get; set; } } + + [Fact] + public static void RecursiveNullableStruct_Roundtrip() + { + var value = new RecursiveNullableStruct + { + Next = new RecursiveNullableStruct?[] + { + new() + { + Next = new RecursiveNullableStruct?[] + { + null + } + } + } + }; + + string json = JsonSerializer.Serialize(value); + Assert.Equal("""{"Next":[{"Next":[null]}]}""", json); + value = JsonSerializer.Deserialize(json); + string roundtripJson = JsonSerializer.Serialize(value); + Assert.Equal(roundtripJson, json); + } + + public struct RecursiveNullableStruct + { + public RecursiveNullableStruct?[] Next { get; set; } + } } } 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 index b1e6f2bac9b79..b15df9763c6a4 100644 --- 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 @@ -440,7 +440,7 @@ public static void Options_JsonSerializerContext_DoesNotFallbackToReflection() [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public static void Options_JsonSerializerContext_GetConverter_FallsBackToReflectionConverter() + public static void Options_JsonSerializerContext_GetConverter_DoesNotFallBackToReflectionConverter() { RemoteExecutor.Invoke(static () => { @@ -451,12 +451,14 @@ public static void Options_JsonSerializerContext_GetConverter_FallsBackToReflect Assert.Null(context.GetTypeInfo(typeof(MyClass))); Assert.Throws(() => context.Options.GetConverter(typeof(MyClass))); - // Root converters process-wide by calling a Serialize overload accepting options - Assert.Throws(() => JsonSerializer.Serialize(unsupportedValue, context.Options)); + // Root converters process-wide using a default options instance + var options = new JsonSerializerOptions(); + JsonConverter converter = options.GetConverter(typeof(MyClass)); + Assert.IsAssignableFrom>(converter); - // We still can't resolve metadata for MyClass, but we can now resolve a converter using the rooted converters. + // We still can't resolve metadata for MyClass or get a converter using the rooted converters. Assert.Null(context.GetTypeInfo(typeof(MyClass))); - Assert.IsAssignableFrom>(context.Options.GetConverter(typeof(MyClass))); + Assert.Throws(() => context.Options.GetConverter(typeof(MyClass))); }).Dispose(); } From 8e72dec9ed485fe4c6e08323aa8d672aab899329 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 11:24:23 +0100 Subject: [PATCH 2/7] Address feedback --- .../DefaultJsonTypeInfoResolver.Converters.cs | 5 ----- .../Metadata/JsonPropertyInfo.cs | 1 - .../Metadata/JsonTypeInfo.Cache.cs | 14 ++++++++----- .../Serialization/Metadata/JsonTypeInfo.cs | 19 +++++++---------- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 9 +------- .../Serialization/OptionsTests.cs | 21 +++++++++++++++++-- 6 files changed, 36 insertions(+), 33 deletions(-) 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 index 6d9f808ddd9bf..152b84e984931 100644 --- 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 @@ -119,11 +119,6 @@ internal static bool TryGetDefaultSimpleConverter(Type typeToConvert, [NotNullWh return s_defaultSimpleConverters.TryGetValue(typeToConvert, out converter); } - // This method gets the runtime information for a given type or property. - // The runtime information consists of the following: - // - class type, - // - element type (if the type is a collection), - // - the converter (either native or custom), if one exists. [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] internal static JsonConverter? GetCustomConverterForMember(Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 808698953a95f..2bbeb35fd68ca 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -734,7 +734,6 @@ internal JsonTypeInfo JsonTypeInfo } set { - // Used by JsonMetadataServices. // This could potentially be double initialized Debug.Assert(_jsonTypeInfo == null || _jsonTypeInfo == value); _jsonTypeInfo = value; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index ba669654518e3..aabc52584354b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -54,17 +54,20 @@ public abstract partial class JsonTypeInfo [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType, JsonConverter? converter) + internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) { - Debug.Assert(converter is null || propertyType.IsInSubtypeRelationshipWith(converter.TypeToConvert)); + // Resolve the JsonTypeInfo for the member type, if it has already been cached. + // If not found, type metadata resolution will be delayed until the Configure() stage. + Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo); + JsonConverter? converterForType = jsonTypeInfo?.Converter; JsonPropertyInfo jsonPropertyInfo; - if (converter?.TypeToConvert == propertyType) + if (converterForType?.TypeToConvert == propertyType) { // For the vast majority of use cases, the converter type matches the property type. // Avoid reflection-based initialization by delegating JsonPropertyInfo construction to the converter itself. - jsonPropertyInfo = converter.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); + jsonPropertyInfo = converterForType.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); } else { @@ -75,7 +78,8 @@ internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType, JsonC jsonPropertyInfo = (JsonPropertyInfo)factoryInfo.Invoke(null, new object[] { this, Options })!; } - jsonPropertyInfo.DefaultConverterForType = converter; + jsonPropertyInfo.DefaultConverterForType = converterForType; + jsonPropertyInfo.JsonTypeInfo = jsonTypeInfo; return jsonPropertyInfo; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index c1a75e16df16b..b5c79f3ccf26f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -516,12 +516,12 @@ internal virtual void LateAddProperties() { } /// is null. /// /// The returned will be blank, with the exception of the - /// property which will be resolved either from + /// property which will be resolved either from /// or the built-in converters for the type. /// Any converters specified via on the type declaration /// will not be resolved by this method. /// - /// What converter does get resolved influences the value of , + /// What converter does get resolved influences the value of , /// which constrains the type of metadata that can be modified in the instance. /// [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] @@ -544,17 +544,17 @@ public static JsonTypeInfo CreateJsonTypeInfo(JsonSerializerOptions option /// /// The type for which contract metadata is specified. /// The instance the metadata is associated with. - /// A blank instance. + /// A blank instance. /// or is null. /// cannot be used for serialization. /// - /// The returned will be blank, with the exception of the - /// property which will be resolved either from + /// The returned will be blank, with the exception of the + /// property which will be resolved either from /// or the built-in converters for the type. /// Any converters specified via on the type declaration /// will not be resolved by this method. /// - /// What converter does get resolved influences the value of , + /// What converter does get resolved influences the value of , /// which constrains the type of metadata that can be modified in the instance. /// [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] @@ -608,12 +608,7 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); } - JsonConverter? converter = - Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo) - ? jsonTypeInfo.Converter - : null; - - JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType, converter); + JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType); propertyInfo.Name = name; return propertyInfo; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index c09f53ce4d006..224d0c2b4616c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -185,13 +185,6 @@ private void CacheMember( ThrowHelper.ThrowInvalidOperationException_CannotSerializeInvalidType(typeToConvert, memberInfo.DeclaringType, memberInfo); } - // Resolve the default converter for the member type, if it has already been cached. - // If not found, converter resolution will be delayed until the Configure() stage. - JsonConverter? converter = - options.TryGetTypeInfoCached(typeToConvert, out JsonTypeInfo? jsonTypeInfo) - ? jsonTypeInfo.Converter - : null; - // Resolve any custom converters on the attribute level. JsonConverter? customConverter; try @@ -204,7 +197,7 @@ private void CacheMember( return null; } - JsonPropertyInfo jsonPropertyInfo = CreatePropertyUsingReflection(typeToConvert, converter); + JsonPropertyInfo jsonPropertyInfo = CreatePropertyUsingReflection(typeToConvert); jsonPropertyInfo.InitializeUsingMemberReflection(memberInfo, customConverter, ignoreCondition); return jsonPropertyInfo; } 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 index b15df9763c6a4..83ed3e65cf466 100644 --- 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 @@ -1,7 +1,6 @@ // 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; using System.Collections.Generic; using System.IO; using System.Reflection; @@ -9,7 +8,6 @@ using System.Text.Json.Serialization.Metadata; using System.Text.Json.Tests; using System.Text.Unicode; -using System.Threading; using Microsoft.DotNet.RemoteExecutor; using Xunit; @@ -450,6 +448,7 @@ public static void Options_JsonSerializerContext_GetConverter_DoesNotFallBackToR // Default converters have not been rooted yet Assert.Null(context.GetTypeInfo(typeof(MyClass))); Assert.Throws(() => context.Options.GetConverter(typeof(MyClass))); + Assert.Throws(() => JsonSerializer.Serialize(unsupportedValue, context.Options)); // Root converters process-wide using a default options instance var options = new JsonSerializerOptions(); @@ -459,6 +458,7 @@ public static void Options_JsonSerializerContext_GetConverter_DoesNotFallBackToR // We still can't resolve metadata for MyClass or get a converter using the rooted converters. Assert.Null(context.GetTypeInfo(typeof(MyClass))); Assert.Throws(() => context.Options.GetConverter(typeof(MyClass))); + Assert.Throws(() => JsonSerializer.Serialize(unsupportedValue, context.Options)); }).Dispose(); } @@ -559,6 +559,23 @@ public static void Options_GetConverter_GivesCorrectCustomConverterAndReadWriteS GenericConverterTestHelper("LongArrayConverter", new long[] { 1, 2, 3, 4 }, "\"1,2,3,4\"", options); } + [Theory] + [InlineData(typeof(int))] + [InlineData(typeof(string))] + [InlineData(typeof(int[]))] + [InlineData(typeof(Poco))] + [InlineData(typeof(Dictionary))] + public static void Options_GetConverter_CustomResolver_DoesNotReturnConverterForUnsupportedType(Type type) + { + var options = new JsonSerializerOptions { TypeInfoResolver = new NullResolver() }; + Assert.Throws(() => options.GetConverter(type)); + } + + public class NullResolver : IJsonTypeInfoResolver + { + public JsonTypeInfo? GetTypeInfo(Type type, JsonSerializerOptions options) => null; + } + private static void GenericConverterTestHelper(string converterName, object objectValue, string stringValue, JsonSerializerOptions options, bool nullOptionOkay = true) { JsonConverter converter = (JsonConverter)options.GetConverter(typeof(T)); From bb3ea0506fd0b51530a21b063918d2c8db63d961 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 13:07:27 +0100 Subject: [PATCH 3/7] Use JsonTypeInfo instead of JsonConverter when resolving JsonPropertyInfo & EffectiveConverter --- .../FSharp/FSharpTypeConverterFactory.cs | 4 +- .../Converters/Object/ObjectConverter.cs | 2 +- .../Value/NullableConverterFactory.cs | 2 +- .../Text/Json/Serialization/JsonConverter.cs | 2 - .../Serialization/JsonConverterFactory.cs | 7 --- .../Json/Serialization/JsonConverterOfT.cs | 5 -- .../JsonSerializerOptions.Caching.cs | 34 ++++------- .../JsonSerializerOptions.Converters.cs | 28 ++------- .../JsonMetadataServices.Converters.cs | 2 +- .../Metadata/JsonParameterInfo.cs | 2 +- .../Metadata/JsonPropertyInfo.cs | 57 +++++++------------ .../Metadata/JsonPropertyInfoOfT.cs | 6 +- .../Metadata/JsonTypeInfo.Cache.cs | 26 ++++----- .../Serialization/Metadata/JsonTypeInfo.cs | 4 +- .../Serialization/Metadata/JsonTypeInfoOfT.cs | 9 ++- .../Metadata/PolymorphicTypeResolver.cs | 2 +- .../Json/Serialization/WriteStackFrame.cs | 2 +- 17 files changed, 69 insertions(+), 125 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpTypeConverterFactory.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpTypeConverterFactory.cs index 19a3d3c74c348..f42adbc31fe40 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpTypeConverterFactory.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/FSharp/FSharpTypeConverterFactory.cs @@ -37,12 +37,12 @@ public override bool CanConvert(Type typeToConvert) => case FSharpKind.Option: elementType = typeToConvert.GetGenericArguments()[0]; converterFactoryType = typeof(FSharpOptionConverter<,>).MakeGenericType(typeToConvert, elementType); - constructorArguments = new object[] { options.GetConverterFromTypeInfo(elementType) }; + constructorArguments = new object[] { options.GetConverterInternal(elementType) }; break; case FSharpKind.ValueOption: elementType = typeToConvert.GetGenericArguments()[0]; converterFactoryType = typeof(FSharpValueOptionConverter<,>).MakeGenericType(typeToConvert, elementType); - constructorArguments = new object[] { options.GetConverterFromTypeInfo(elementType) }; + constructorArguments = new object[] { options.GetConverterInternal(elementType) }; break; case FSharpKind.List: elementType = typeToConvert.GetGenericArguments()[0]; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs index d077627720bda..6c0a095061fac 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectConverter.cs @@ -86,7 +86,7 @@ internal override void WriteAsPropertyNameCore(Utf8JsonWriter writer, object? va Debug.Assert(value != null); Type runtimeType = value.GetType(); - JsonConverter runtimeConverter = options.GetConverterFromTypeInfo(runtimeType); + JsonConverter runtimeConverter = options.GetConverterInternal(runtimeType); if (runtimeConverter == this) { ThrowHelper.ThrowNotSupportedException_DictionaryKeyTypeNotSupported(runtimeType, this); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs index 36f74f39bf8d0..cbc89a4fb7bba 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Value/NullableConverterFactory.cs @@ -22,7 +22,7 @@ public override JsonConverter CreateConverter(Type typeToConvert, JsonSerializer Type valueTypeToConvert = typeToConvert.GetGenericArguments()[0]; - JsonConverter valueConverter = options.GetConverterFromTypeInfo(valueTypeToConvert); + JsonConverter valueConverter = options.GetConverterInternal(valueTypeToConvert); Debug.Assert(valueConverter != null); // If the value type has an interface or object converter, just return that converter directly. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs index 631fdabc953a2..e8ff50c9dbfab 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverter.cs @@ -67,8 +67,6 @@ internal virtual void ReadElementAndSetProperty( throw new InvalidOperationException(SR.NodeJsonObjectCustomConverterNotAllowedOnExtensionProperty); } - internal abstract JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options); - internal abstract JsonParameterInfo CreateJsonParameterInfo(); internal abstract JsonConverter CreateCastingConverter(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs index 02f9cb940970b..f9bcd6e7b4878 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs @@ -32,13 +32,6 @@ protected JsonConverterFactory() { } /// public abstract JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options); - internal override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) - { - Debug.Fail("We should never get here."); - - throw new InvalidOperationException(); - } - internal override JsonParameterInfo CreateJsonParameterInfo() { Debug.Fail("We should never get here."); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs index 8be4212a95431..da4963acdb823 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.cs @@ -69,11 +69,6 @@ public override bool CanConvert(Type typeToConvert) internal override ConverterStrategy ConverterStrategy => ConverterStrategy.Value; - internal sealed override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) - { - return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options); - } - internal sealed override JsonParameterInfo CreateJsonParameterInfo() { return new JsonParameterInfo(); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index aa84beb4514b4..7338f54cfc7b4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -45,44 +45,34 @@ public JsonTypeInfo GetTypeInfo(Type type) ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(type), type, null, null); } - JsonTypeInfo? typeInfo; - if (IsLockedInstance) - { - typeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(type); - typeInfo?.EnsureConfigured(); - } - else - { - typeInfo = GetTypeInfoNoCaching(type); - } - - if (typeInfo is null) - { - ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type, TypeInfoResolver); - } - - return typeInfo; + return GetTypeInfoInternal(type, resolveIfMutable: true); } /// - /// This method returns configured non-null JsonTypeInfo + /// Same as GetTypeInfo but without validation and additional knobs. /// - internal JsonTypeInfo GetTypeInfoCached(Type type) + internal JsonTypeInfo GetTypeInfoInternal(Type type, bool ensureConfigured = true, bool resolveIfMutable = false) { JsonTypeInfo? typeInfo = null; if (IsLockedInstance) { typeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(type); + if (ensureConfigured) + { + typeInfo?.EnsureConfigured(); + } + } + else if (resolveIfMutable) + { + typeInfo = GetTypeInfoNoCaching(type); } if (typeInfo == null) { ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type, TypeInfoResolver); - return null; } - typeInfo.EnsureConfigured(); return typeInfo; } @@ -108,7 +98,7 @@ internal JsonTypeInfo GetTypeInfoForRootType(Type type) if (jsonTypeInfo?.Type != type) { - jsonTypeInfo = GetTypeInfoCached(type); + jsonTypeInfo = GetTypeInfoInternal(type); _lastTypeInfo = jsonTypeInfo; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index c7c34acc03f2d..33b0351655afa 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -2,7 +2,6 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Collections.Generic; -using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Text.Json.Reflection; using System.Text.Json.Serialization; @@ -53,34 +52,15 @@ public JsonConverter GetConverter(Type typeToConvert) return DefaultJsonTypeInfoResolver.GetConverterForType(typeToConvert, this); } - return GetConverterFromTypeInfo(typeToConvert); + return GetConverterInternal(typeToConvert); } /// - /// Same as GetConverter but does not root converters + /// Same as GetConverter but without defaulting to reflection converters. /// - internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert) + internal JsonConverter GetConverterInternal(Type typeToConvert) { - JsonTypeInfo? jsonTypeInfo; - - if (IsLockedInstance) - { - jsonTypeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(typeToConvert); - } - else - { - // We do not want to lock options instance here but we need to return correct answer - // which means we need to go through TypeInfoResolver but without caching because that's the - // only place which will have correct converter for JsonSerializerContext and reflection - // based resolver. It will also work correctly for combined resolvers. - jsonTypeInfo = GetTypeInfoNoCaching(typeToConvert); - } - - if (jsonTypeInfo is null) - { - ThrowHelper.ThrowNotSupportedException_NoMetadataForType(typeToConvert, TypeInfoResolver); - } - + JsonTypeInfo jsonTypeInfo = GetTypeInfoInternal(typeToConvert, ensureConfigured: false, resolveIfMutable: true); return jsonTypeInfo.Converter; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs index 879c1884a8a89..9bc156074bddc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.Converters.cs @@ -283,7 +283,7 @@ public static JsonConverter GetEnumConverter(JsonSerializerOptions options ThrowHelper.ThrowArgumentNullException(nameof(options)); } - JsonConverter underlyingConverter = GetTypedConverter(options.GetConverterFromTypeInfo(typeof(T))); + JsonConverter underlyingConverter = GetTypedConverter(options.GetConverterInternal(typeof(T))); return new NullableConverter(underlyingConverter); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index e5342b24c0791..247f58db58cdc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -39,7 +39,7 @@ public JsonTypeInfo JsonTypeInfo { Debug.Assert(Options != null); Debug.Assert(ShouldDeserialize); - return _jsonTypeInfo ??= Options.GetTypeInfoCached(PropertyType); + return _jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType); } set { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 2bbeb35fd68ca..1418afb476a22 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -22,23 +22,6 @@ public abstract class JsonPropertyInfo internal ConverterStrategy ConverterStrategy { get; private protected set; } - /// - /// Converter resolved from PropertyType and not taking in consideration any custom attributes or custom settings. - /// - for reflection we store the original value since we need it in order to construct typed JsonPropertyInfo - /// - for source gen it remains null, we will initialize it only if someone used resolver to remove CustomConverter - /// - internal JsonConverter? DefaultConverterForType - { - get => _defaultConverterForType; - set - { - _defaultConverterForType = value; - ConverterStrategy = value?.ConverterStrategy ?? default; - } - } - - private JsonConverter? _defaultConverterForType; - /// /// Converter after applying CustomConverter (i.e. JsonConverterAttribute) /// @@ -255,7 +238,8 @@ internal void Configure() CacheNameAsUtf8BytesAndEscapedNameSection(); } - DetermineEffectiveConverter(); + _jsonTypeInfo ??= Options.GetTypeInfoInternal(PropertyType, ensureConfigured: false); + DetermineEffectiveConverter(_jsonTypeInfo); if (IsForTypeInfo) { @@ -269,7 +253,7 @@ internal void Configure() } } - private protected abstract void DetermineEffectiveConverter(); + private protected abstract void DetermineEffectiveConverter(JsonTypeInfo jsonTypeInfo); private protected abstract void DetermineMemberAccessors(MemberInfo memberInfo); private void DeterminePoliciesFromMember(MemberInfo memberInfo) @@ -406,12 +390,15 @@ private void DetermineNumberHandlingForTypeInfo() private void DetermineNumberHandlingForProperty() { + Debug.Assert(!IsConfigured, "Should not be called post-configuration."); + Debug.Assert(_jsonTypeInfo != null, "Must have already been determined on configuration."); + bool numberHandlingIsApplicable = NumberHandingIsApplicable(); if (numberHandlingIsApplicable) { // Priority 1: Get handling from attribute on property/field, its parent class type or property type. - JsonNumberHandling? handling = NumberHandling ?? DeclaringTypeNumberHandling ?? JsonTypeInfo.NumberHandling; + JsonNumberHandling? handling = NumberHandling ?? DeclaringTypeNumberHandling ?? _jsonTypeInfo.NumberHandling; // Priority 2: Get handling from JsonSerializerOptions instance. if (!handling.HasValue && Options.NumberHandling != JsonNumberHandling.Strict) @@ -563,7 +550,11 @@ internal bool IgnoreReadOnlyMember /// public string Name { - get => _name; + get + { + Debug.Assert(_name != null); + return _name; + } set { VerifyMutable(); @@ -577,7 +568,7 @@ public string Name } } - private string _name = null!; + private string? _name; /// /// Utf8 version of Name. @@ -664,7 +655,7 @@ JsonConverter GetDictionaryValueConverter(Type dictionaryValueType) // Slower path for non-generic types that implement IDictionary<,>. // It is possible to cache this converter on JsonTypeInfo if we assume the property value // will always be the same type for all instances. - converter = Options.GetConverterFromTypeInfo(dictionaryValueType); + converter = Options.GetConverterInternal(dictionaryValueType); } Debug.Assert(converter != null); @@ -686,7 +677,7 @@ internal bool ReadJsonExtensionDataValue(ref ReadStack state, ref Utf8JsonReader return true; } - JsonConverter converter = (JsonConverter)Options.GetConverterFromTypeInfo(typeof(JsonElement)); + JsonConverter converter = (JsonConverter)Options.GetConverterInternal(typeof(JsonElement)); if (!converter.TryRead(ref reader, typeof(JsonElement), Options, ref state, out JsonElement jsonElement)) { // JsonElement is a struct that must be read in full. @@ -717,25 +708,17 @@ internal JsonTypeInfo JsonTypeInfo { get { - if (_jsonTypeInfo != null) - { - // We should not call it on set as it's usually called during initialization - // which is too early to `lock` the JsonTypeInfo - // If this property ever becomes public we should move this to callsites - _jsonTypeInfo.EnsureConfigured(); - } - else - { - // GetOrAddJsonTypeInfo already ensures it's configured. - _jsonTypeInfo = Options.GetTypeInfoCached(PropertyType); - } - + Debug.Assert(IsConfigured); + Debug.Assert(_jsonTypeInfo != null); + _jsonTypeInfo.EnsureConfigured(); return _jsonTypeInfo; } set { // This could potentially be double initialized Debug.Assert(_jsonTypeInfo == null || _jsonTypeInfo == value); + // Ensure the right strategy is surfaced in PropertyInfoForTypeInfo early + ConverterStrategy = value?.Converter.ConverterStrategy ?? default; _jsonTypeInfo = value; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index 6d6c4824321f8..31d0e699ef78d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -188,7 +188,6 @@ internal JsonPropertyInfo(JsonPropertyInfoValues propertyInfo, JsonSerializer SrcGen_IsPublic = propertyInfo.IsPublic; SrcGen_HasJsonInclude = propertyInfo.HasJsonInclude; IsExtensionData = propertyInfo.IsExtensionData; - DefaultConverterForType = propertyInfo.PropertyTypeInfo?.Converter as JsonConverter; CustomConverter = propertyInfo.Converter; if (propertyInfo.IgnoreCondition != JsonIgnoreCondition.Always) @@ -202,12 +201,11 @@ internal JsonPropertyInfo(JsonPropertyInfoValues propertyInfo, JsonSerializer NumberHandling = propertyInfo.NumberHandling; } - private protected override void DetermineEffectiveConverter() + private protected override void DetermineEffectiveConverter(JsonTypeInfo jsonTypeInfo) { JsonConverter converter = Options.ExpandConverterFactory(CustomConverter, PropertyType) - ?? DefaultConverterForType - ?? Options.GetConverterFromTypeInfo(PropertyType); + ?? jsonTypeInfo.Converter; TypedEffectiveConverter = converter is JsonConverter typedConv ? typedConv : converter.CreateCastingConverter(); ConverterStrategy = TypedEffectiveConverter.ConverterStrategy; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index aabc52584354b..36df5233cdb5a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -56,33 +56,33 @@ public abstract partial class JsonTypeInfo [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) { - // Resolve the JsonTypeInfo for the member type, if it has already been cached. - // If not found, type metadata resolution will be delayed until the Configure() stage. - Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo); - JsonConverter? converterForType = jsonTypeInfo?.Converter; + JsonPropertyInfo? jsonPropertyInfo; - JsonPropertyInfo jsonPropertyInfo; - - if (converterForType?.TypeToConvert == propertyType) + if (Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo)) { - // For the vast majority of use cases, the converter type matches the property type. - // Avoid reflection-based initialization by delegating JsonPropertyInfo construction to the converter itself. - jsonPropertyInfo = converterForType.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); + // If a JsonTypeInfo has already cached for the property type, + // avoid reflection-based initialization by delegating construction + // of JsonPropertyInfo construction to the property type metadata. + return jsonTypeInfo.CreateJsonPropertyInfo(declaringTypeInfo: this); } else { - // We don't have a converter yet or the converter does not match the declared member type. + // Metadata for `propertyType` has not been registered yet. // Use reflection to instantiate the correct JsonPropertyInfo s_createJsonPropertyInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonPropertyInfo), BindingFlags.NonPublic | BindingFlags.Static)!; MethodInfo factoryInfo = s_createJsonPropertyInfo.MakeGenericMethod(propertyType); jsonPropertyInfo = (JsonPropertyInfo)factoryInfo.Invoke(null, new object[] { this, Options })!; } - jsonPropertyInfo.DefaultConverterForType = converterForType; - jsonPropertyInfo.JsonTypeInfo = jsonTypeInfo; + Debug.Assert(jsonPropertyInfo.PropertyType == propertyType); return jsonPropertyInfo; } + /// + /// Creates a JsonPropertyInfo whose property type matches the type of this JsonTypeInfo instance. + /// + private protected abstract JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo); + private static JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) => new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index b5c79f3ccf26f..056a9b2cf6844 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -226,7 +226,7 @@ internal JsonTypeInfo? ElementTypeInfo { // GetOrAddJsonTypeInfo already ensures JsonTypeInfo is configured // also see comment on JsonPropertyInfo.JsonTypeInfo - _elementTypeInfo = Options.GetTypeInfoCached(ElementType); + _elementTypeInfo = Options.GetTypeInfoInternal(ElementType); } } else @@ -268,7 +268,7 @@ internal JsonTypeInfo? KeyTypeInfo // GetOrAddJsonTypeInfo already ensures JsonTypeInfo is configured // also see comment on JsonPropertyInfo.JsonTypeInfo - _keyTypeInfo = Options.GetTypeInfoCached(KeyType); + _keyTypeInfo = Options.GetTypeInfoInternal(KeyType); } } else diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs index 175efb0f60143..6fda3bf49f8ed 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs @@ -96,12 +96,19 @@ private protected override JsonPropertyInfo CreatePropertyInfoForTypeInfo() declaringTypeInfo: null, Options) { - DefaultConverterForType = Converter, JsonTypeInfo = this, IsForTypeInfo = true, }; } + private protected override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo) + { + return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, Options) + { + JsonTypeInfo = this + };; + } + private protected void MapInterfaceTypesToCallbacks() { // Callbacks currently only supported in object kinds diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs index b85200a2ca143..11f988190785f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/PolymorphicTypeResolver.cs @@ -249,7 +249,7 @@ public DerivedJsonTypeInfo(Type type, object? typeDiscriminator) public Type DerivedType { get; } public object? TypeDiscriminator { get; } public JsonTypeInfo GetJsonTypeInfo(JsonSerializerOptions options) - => _jsonTypeInfo ??= options.GetTypeInfoCached(DerivedType); + => _jsonTypeInfo ??= options.GetTypeInfoInternal(DerivedType); } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs index 705fe03d7f5d5..2d9aaabc2d46f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStackFrame.cs @@ -126,7 +126,7 @@ public JsonConverter InitializePolymorphicReEntry(Type runtimeType, JsonSerializ // if the current element is the same type as the previous element. if (PolymorphicJsonTypeInfo?.PropertyType != runtimeType) { - JsonTypeInfo typeInfo = options.GetTypeInfoCached(runtimeType); + JsonTypeInfo typeInfo = options.GetTypeInfoInternal(runtimeType); PolymorphicJsonTypeInfo = typeInfo.PropertyInfoForTypeInfo; } From 4f10c60b1586bdf1125de3f6aca616931719cbdd Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 15:41:50 +0100 Subject: [PATCH 4/7] Fix rare test failure caused by using a locked instance with a shared caching context. --- .../DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index cb8c53ae64624..a2d1b5e51551a 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -18,7 +18,7 @@ public static partial class DefaultJsonTypeInfoResolverTests [Fact] public static void JsonPropertyInfoOptionsAreSet() { - JsonSerializerOptions options = JsonSerializerOptions.Default; + JsonSerializerOptions options = new() { TypeInfoResolver = new DefaultJsonTypeInfoResolver() }; JsonTypeInfo typeInfo = JsonTypeInfo.CreateJsonTypeInfo(typeof(MyClass), options); CreatePropertyAndCheckOptions(options, typeInfo); @@ -28,6 +28,9 @@ public static void JsonPropertyInfoOptionsAreSet() typeInfo = options.TypeInfoResolver.GetTypeInfo(typeof(MyClass), options); CreatePropertyAndCheckOptions(options, typeInfo); + typeInfo = options.GetTypeInfo(typeof(MyClass)); + CreatePropertyAndCheckOptions(options, typeInfo); + static void CreatePropertyAndCheckOptions(JsonSerializerOptions expectedOptions, JsonTypeInfo typeInfo) { JsonPropertyInfo propertyInfo = typeInfo.CreateJsonPropertyInfo(typeof(string), "test"); From 5d22feaf88083ba1cd98610f5d7b481c29ada6c1 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 16:54:03 +0100 Subject: [PATCH 5/7] Address feedback --- .../Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs | 4 ++-- .../Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index 36df5233cdb5a..70519c95108f7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -63,7 +63,7 @@ internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) // If a JsonTypeInfo has already cached for the property type, // avoid reflection-based initialization by delegating construction // of JsonPropertyInfo construction to the property type metadata. - return jsonTypeInfo.CreateJsonPropertyInfo(declaringTypeInfo: this); + return jsonTypeInfo.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); } else { @@ -81,7 +81,7 @@ internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) /// /// Creates a JsonPropertyInfo whose property type matches the type of this JsonTypeInfo instance. /// - private protected abstract JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo); + private protected abstract JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options); private static JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) => new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs index 6fda3bf49f8ed..0b069b4145b3e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs @@ -101,12 +101,12 @@ private protected override JsonPropertyInfo CreatePropertyInfoForTypeInfo() }; } - private protected override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo) + private protected override JsonPropertyInfo CreateJsonPropertyInfo(JsonTypeInfo declaringTypeInfo, JsonSerializerOptions options) { - return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, Options) + return new JsonPropertyInfo(declaringTypeInfo.Type, declaringTypeInfo, options) { JsonTypeInfo = this - };; + }; } private protected void MapInterfaceTypesToCallbacks() From 51c18f8c11166e5b5d167bdee5d29e013a71af10 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 17:13:13 +0100 Subject: [PATCH 6/7] Revert "Fix rare test failure caused by using a locked instance with a shared caching context." This reverts commit 4f10c60b1586bdf1125de3f6aca616931719cbdd. --- .../DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs index a2d1b5e51551a..cb8c53ae64624 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonPropertyInfo.cs @@ -18,7 +18,7 @@ public static partial class DefaultJsonTypeInfoResolverTests [Fact] public static void JsonPropertyInfoOptionsAreSet() { - JsonSerializerOptions options = new() { TypeInfoResolver = new DefaultJsonTypeInfoResolver() }; + JsonSerializerOptions options = JsonSerializerOptions.Default; JsonTypeInfo typeInfo = JsonTypeInfo.CreateJsonTypeInfo(typeof(MyClass), options); CreatePropertyAndCheckOptions(options, typeInfo); @@ -28,9 +28,6 @@ public static void JsonPropertyInfoOptionsAreSet() typeInfo = options.TypeInfoResolver.GetTypeInfo(typeof(MyClass), options); CreatePropertyAndCheckOptions(options, typeInfo); - typeInfo = options.GetTypeInfo(typeof(MyClass)); - CreatePropertyAndCheckOptions(options, typeInfo); - static void CreatePropertyAndCheckOptions(JsonSerializerOptions expectedOptions, JsonTypeInfo typeInfo) { JsonPropertyInfo propertyInfo = typeInfo.CreateJsonPropertyInfo(typeof(string), "test"); From f2a55d2d8ea7bceb5af2ca873d580de33a9c8018 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 15 Jul 2022 19:35:15 +0100 Subject: [PATCH 7/7] minor cleanup --- .../Json/Serialization/JsonSerializerOptions.Caching.cs | 7 +------ .../Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs | 2 +- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index 7338f54cfc7b4..f372d8bd8336e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -115,12 +115,7 @@ internal void ClearCaches() { Debug.Assert(IsLockedInstance); - if (_cachingContext is null) - { - _cachingContext = TrackedCachingContexts.GetOrCreate(this); - } - - return _cachingContext; + return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this); } /// diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs index 70519c95108f7..eb991681847a7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.Cache.cs @@ -60,7 +60,7 @@ internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) if (Options.TryGetTypeInfoCached(propertyType, out JsonTypeInfo? jsonTypeInfo)) { - // If a JsonTypeInfo has already cached for the property type, + // If a JsonTypeInfo has already been cached for the property type, // avoid reflection-based initialization by delegating construction // of JsonPropertyInfo construction to the property type metadata. return jsonTypeInfo.CreateJsonPropertyInfo(declaringTypeInfo: this, Options);