Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove fallback to reflection converter resolution #72228

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions src/libraries/System.Text.Json/src/Resources/Strings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -581,9 +581,6 @@
<data name="ConverterForPropertyMustBeValid" xml:space="preserve">
<value>The generic type of the converter for property '{0}.{1}' must match with the specified converter type '{2}'. The converter must not be 'null'.</value>
</data>
<data name="BuiltInConvertersNotRooted" xml:space="preserve">
krwq marked this conversation as resolved.
Show resolved Hide resolved
<value>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'.</value>
</data>
<data name="NoMetadataForType" xml:space="preserve">
<value>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.</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<TTarget> CreateCastingConverter<TTarget>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,6 @@ protected JsonConverterFactory() { }
/// </returns>
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.");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,14 +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<T>(declaringTypeInfo.Type, declaringTypeInfo, options)
{
DefaultConverterForType = this
};
}

internal sealed override JsonParameterInfo CreateJsonParameterInfo()
{
return new JsonParameterInfo<T>();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

/// <summary>
/// This method returns configured non-null JsonTypeInfo
/// Same as GetTypeInfo but without validation and additional knobs.
/// </summary>
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);
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
}

if (typeInfo == null)
{
ThrowHelper.ThrowNotSupportedException_NoMetadataForType(type, TypeInfoResolver);
return null;
}

typeInfo.EnsureConfigured();
return typeInfo;
}

Expand All @@ -108,7 +98,7 @@ internal JsonTypeInfo GetTypeInfoForRootType(Type type)

if (jsonTypeInfo?.Type != type)
{
jsonTypeInfo = GetTypeInfoCached(type);
jsonTypeInfo = GetTypeInfoInternal(type);
_lastTypeInfo = jsonTypeInfo;
}

Expand All @@ -125,12 +115,7 @@ internal void ClearCaches()
{
Debug.Assert(IsLockedInstance);

if (_cachingContext is null && _typeInfoResolver is not null)
krwq marked this conversation as resolved.
Show resolved Hide resolved
{
_cachingContext = TrackedCachingContexts.GetOrCreate(this);
}

return _cachingContext;
return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this);
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -48,35 +47,21 @@ 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);
krwq marked this conversation as resolved.
Show resolved Hide resolved
}

return GetConverterFromTypeInfo(typeToConvert);
return GetConverterInternal(typeToConvert);
}

/// <summary>
/// Same as GetConverter but does not root converters
/// Same as GetConverter but without defaulting to reflection converters.
/// </summary>
internal JsonConverter GetConverterFromTypeInfo(Type typeToConvert)
internal JsonConverter GetConverterInternal(Type typeToConvert)
{
JsonConverter? converter;

if (IsLockedInstance)
{
converter = GetCachingContext()?.GetOrAddJsonTypeInfo(typeToConvert)?.Converter;
}
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.
converter = GetTypeInfoNoCaching(typeToConvert)?.Converter;
}

return converter ?? GetConverterFromListOrBuiltInConverter(typeToConvert);
JsonTypeInfo jsonTypeInfo = GetTypeInfoInternal(typeToConvert, ensureConfigured: false, resolveIfMutable: true);
return jsonTypeInfo.Converter;
}

internal JsonConverter? GetConverterFromList(Type typeToConvert)
Expand All @@ -92,28 +77,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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@ internal sealed class CustomJsonTypeInfo<T> : JsonTypeInfo<T>
/// <summary>
/// Creates serialization metadata for a type using a simple converter.
/// </summary>
internal CustomJsonTypeInfo(JsonSerializerOptions options)
: base(options.GetConverterFromListOrBuiltInConverter(typeof(T)), options)
internal CustomJsonTypeInfo(JsonConverter converter, JsonSerializerOptions options)
: base(converter, options)
{
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <i.e. JsonSerializerOptions.GetConverter(Type)>, 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))
Expand All @@ -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;
}
}
Expand All @@ -125,39 +119,28 @@ 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 GetConverterForMember(
Type typeToConvert,
MemberInfo memberInfo,
JsonSerializerOptions options,
out JsonConverter? customConverter)
internal static JsonConverter? GetCustomConverterForMember(Type typeToConvert, MemberInfo memberInfo, JsonSerializerOptions options)
eiriktsarpalis marked this conversation as resolved.
Show resolved Hide resolved
{
Debug.Assert(memberInfo is FieldInfo or PropertyInfo);
Debug.Assert(typeToConvert != null);

JsonConverterAttribute? converterAttribute = memberInfo.GetUniqueCustomAttribute<JsonConverterAttribute>(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<JsonConverterAttribute>(inherit: false);
if (converterAttribute != null)
Expand All @@ -166,8 +149,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)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,11 @@ private JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options

[RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)]
[RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)]
private static JsonTypeInfo<T> CreateReflectionJsonTypeInfo<T>(JsonSerializerOptions options) => new ReflectionJsonTypeInfo<T>(options);
private static JsonTypeInfo<T> CreateReflectionJsonTypeInfo<T>(JsonSerializerOptions options)
{
JsonConverter converter = GetConverterForType(typeof(T), options);
return new ReflectionJsonTypeInfo<T>(converter, options);
}

/// <summary>
/// List of JsonTypeInfo modifiers. Modifying callbacks are called consecutively after initial resolution
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ public static JsonConverter<T> GetEnumConverter<T>(JsonSerializerOptions options
ThrowHelper.ThrowArgumentNullException(nameof(options));
}

JsonConverter<T> underlyingConverter = GetTypedConverter<T>(options.GetConverterFromTypeInfo(typeof(T)));
JsonConverter<T> underlyingConverter = GetTypedConverter<T>(options.GetConverterInternal(typeof(T)));

return new NullableConverter<T>(underlyingConverter);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down
Loading