From 0b90ae4659e36b1f5d4f2bdf276473be718f53a8 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 19 Jul 2022 13:33:10 +0300 Subject: [PATCH] Ensure reflection-based metadata instantiations unwrap TargetInvocationException. (#72397) * Ensure reflection-based metadata instantiations unwrap TargetInvocationException. * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs Co-authored-by: Jan Kotas --- .../src/System/ReflectionExtensions.cs | 23 ++++++++++++ .../Metadata/DefaultJsonTypeInfoResolver.cs | 35 +++++++------------ .../Metadata/JsonTypeInfo.Cache.cs | 6 ++-- .../Serialization/Metadata/JsonTypeInfo.cs | 3 +- ...tJsonTypeInfoResolverTests.JsonTypeInfo.cs | 17 +++++++++ 5 files changed, 58 insertions(+), 26 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index e3fb49ff00f05..9a5d6f4bb610d 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs @@ -3,6 +3,7 @@ using System.Reflection; using System.Runtime.CompilerServices; +using System.Runtime.ExceptionServices; using System.Text.Json.Serialization; namespace System.Text.Json.Reflection @@ -59,5 +60,27 @@ private static bool HasJsonConstructorAttribute(ConstructorInfo constructorInfo) ThrowHelper.ThrowInvalidOperationException_SerializationDuplicateAttribute(typeof(TAttribute), memberInfo); return null; } + + /// + /// Polyfill for BindingFlags.DoNotWrapExceptions + /// + public static object? InvokeNoWrapExceptions(this MethodInfo methodInfo, object? obj, object?[] parameters) + { +#if NETCOREAPP + return methodInfo.Invoke(obj, BindingFlags.DoNotWrapExceptions, null, parameters, null); +#else + object? result = null; + try + { + result = methodInfo.Invoke(obj, parameters); + } + catch (TargetInvocationException ex) + { + ExceptionDispatchInfo.Capture(ex.InnerException).Throw(); + } + + return result; +#endif + } } } 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 44ea6c1b06fa0..01cd5430ee97e 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 @@ -4,7 +4,7 @@ using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Reflection; -using System.Runtime.ExceptionServices; +using System.Text.Json.Reflection; using System.Threading; namespace System.Text.Json.Serialization.Metadata @@ -36,6 +36,10 @@ private DefaultJsonTypeInfoResolver(bool mutable) } /// + [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", + Justification = "The ctor is marked RequiresUnreferencedCode.")] + [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", + Justification = "The ctor is marked RequiresDynamicCode.")] public virtual JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options) { if (type == null) @@ -64,28 +68,13 @@ public virtual JsonTypeInfo GetTypeInfo(Type type, JsonSerializerOptions options return typeInfo; } - [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:RequiresUnreferencedCode", - Justification = "The ctor is marked RequiresUnreferencedCode.")] - [UnconditionalSuppressMessage("AotAnalysis", "IL3050:RequiresDynamicCode", - Justification = "The ctor is marked RequiresDynamicCode.")] - private JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] + [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] + private static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) { - MethodInfo methodInfo = typeof(DefaultJsonTypeInfoResolver).GetMethod(nameof(CreateReflectionJsonTypeInfo), BindingFlags.NonPublic | BindingFlags.Static)!; -#if NETCOREAPP - return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(null, BindingFlags.NonPublic | BindingFlags.DoNotWrapExceptions, null, new[] { options }, null)!; -#else - try - { - return (JsonTypeInfo)methodInfo.MakeGenericMethod(type).Invoke(null, new[] { options })!; - } - catch (TargetInvocationException ex) - { - // Some of the validation is done during construction (i.e. validity of JsonConverter, inner types etc.) - // therefore we need to unwrap TargetInvocationException for better user experience - ExceptionDispatchInfo.Capture(ex.InnerException).Throw(); - throw null!; - } -#endif + s_createReflectionJsonTypeInfoMethodInfo ??= typeof(DefaultJsonTypeInfoResolver).GetMethod(nameof(CreateReflectionJsonTypeInfo), BindingFlags.NonPublic | BindingFlags.Static)!; + return (JsonTypeInfo)s_createReflectionJsonTypeInfoMethodInfo.MakeGenericMethod(type) + .InvokeNoWrapExceptions(null, new object[] { options })!; } [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] @@ -96,6 +85,8 @@ private static JsonTypeInfo CreateReflectionJsonTypeInfo(JsonSerializerOpt return new ReflectionJsonTypeInfo(converter, options); } + private static MethodInfo? s_createReflectionJsonTypeInfoMethodInfo; + /// /// List of JsonTypeInfo modifiers. Modifying callbacks are called consecutively after initial resolution /// and cannot be changed after GetTypeInfo is called. 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 eb991681847a7..7efaef692cd8e 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,15 +63,15 @@ internal JsonPropertyInfo CreatePropertyUsingReflection(Type propertyType) // 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); + jsonPropertyInfo = jsonTypeInfo.CreateJsonPropertyInfo(declaringTypeInfo: this, Options); } else { // 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 = (JsonPropertyInfo)s_createJsonPropertyInfo.MakeGenericMethod(propertyType) + .InvokeNoWrapExceptions(null, new object[] { this, Options })!; } Debug.Assert(jsonPropertyInfo.PropertyType == propertyType); 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 056a9b2cf6844..9d7dbb8a36db2 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 @@ -7,6 +7,7 @@ using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; +using System.Text.Json.Reflection; using System.Threading; namespace System.Text.Json.Serialization.Metadata @@ -578,7 +579,7 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o s_createJsonTypeInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonTypeInfo), new Type[] { typeof(JsonSerializerOptions) })!; return (JsonTypeInfo)s_createJsonTypeInfo.MakeGenericMethod(type) - .Invoke(null, new object[] { options })!; + .InvokeNoWrapExceptions(null, new object[] { options })!; } /// diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs index b66c4b1ceb01b..61aa1c243cbc4 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs @@ -673,6 +673,23 @@ ref struct RefStruct public int Foo { get; set; } } + [Fact] + public static void CreateJsonTypeInfo_ThrowingConverterFactory_DoesNotWrapException() + { + var options = new JsonSerializerOptions { Converters = { new ClassWithThrowingConverterFactory.Converter() } }; + // Should not be wrapped in TargetInvocationException. + Assert.Throws(() => JsonTypeInfo.CreateJsonTypeInfo(typeof(ClassWithThrowingConverterFactory), options)); + } + + public class ClassWithThrowingConverterFactory + { + public class Converter : JsonConverterFactory + { + public override bool CanConvert(Type typeToConvert) => typeToConvert == typeof(ClassWithThrowingConverterFactory); + public override JsonConverter? CreateConverter(Type typeToConvert, JsonSerializerOptions options) => throw new NotFiniteNumberException(); + } + } + [Fact] public static void CreateJsonPropertyInfoWithNullArgumentsThrows() {