From 555e599032de13a075356263441575ee92d35346 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Thu, 5 Aug 2021 18:06:57 +0300 Subject: [PATCH] Do not overwrite JsonException metadata (#56903) * Do not overwrite JsonException metadata. Fix #51537. * remove redundant semicolon --- .../Serialization/JsonConverterOfT.ReadCore.cs | 17 ++++--- .../Serialization/JsonConverterOfT.WriteCore.cs | 13 +++-- .../JsonSerializer.Read.Utf8JsonReader.cs | 2 +- .../System/Text/Json/ThrowHelper.Serialization.cs | 55 +++++++++------------- .../CustomConverterTests.Array.cs | 20 +++----- .../Serialization/ExceptionTests.cs | 33 +++++++++++++ 6 files changed, 81 insertions(+), 59 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs index 5b7d102..ae5ae40 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs @@ -76,22 +76,27 @@ namespace System.Text.Json.Serialization } catch (JsonReaderException ex) { - ThrowHelper.ReThrowWithPath(state, ex); + ThrowHelper.ReThrowWithPath(ref state, ex); return default; } catch (FormatException ex) when (ex.Source == ThrowHelper.ExceptionSourceValueToRethrowAsJsonException) { - ThrowHelper.ReThrowWithPath(state, reader, ex); + ThrowHelper.ReThrowWithPath(ref state, reader, ex); return default; } catch (InvalidOperationException ex) when (ex.Source == ThrowHelper.ExceptionSourceValueToRethrowAsJsonException) { - ThrowHelper.ReThrowWithPath(state, reader, ex); + ThrowHelper.ReThrowWithPath(ref state, reader, ex); return default; } - catch (JsonException ex) + catch (JsonException ex) when (ex.Path == null) { - ThrowHelper.AddJsonExceptionInformation(state, reader, ex); + // JsonExceptions where the Path property is already set + // typically originate from nested calls to JsonSerializer; + // treat these cases as any other exception type and do not + // overwrite any exception information. + + ThrowHelper.AddJsonExceptionInformation(ref state, reader, ex); throw; } catch (NotSupportedException ex) @@ -103,7 +108,7 @@ namespace System.Text.Json.Serialization throw; } - ThrowHelper.ThrowNotSupportedException(state, reader, ex); + ThrowHelper.ThrowNotSupportedException(ref state, reader, ex); return default; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs index 1d60dbf..da003ec 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs @@ -42,12 +42,17 @@ namespace System.Text.Json.Serialization } catch (InvalidOperationException ex) when (ex.Source == ThrowHelper.ExceptionSourceValueToRethrowAsJsonException) { - ThrowHelper.ReThrowWithPath(state, ex); + ThrowHelper.ReThrowWithPath(ref state, ex); throw; } - catch (JsonException ex) + catch (JsonException ex) when (ex.Path == null) { - ThrowHelper.AddJsonExceptionInformation(state, ex); + // JsonExceptions where the Path property is already set + // typically originate from nested calls to JsonSerializer; + // treat these cases as any other exception type and do not + // overwrite any exception information. + + ThrowHelper.AddJsonExceptionInformation(ref state, ex); throw; } catch (NotSupportedException ex) @@ -59,7 +64,7 @@ namespace System.Text.Json.Serialization throw; } - ThrowHelper.ThrowNotSupportedException(state, ex); + ThrowHelper.ThrowNotSupportedException(ref state, ex); return default; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs index 8a2abde..b1cee07 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs @@ -399,7 +399,7 @@ namespace System.Text.Json { reader = restore; // Re-throw with Path information. - ThrowHelper.ReThrowWithPath(state, ex); + ThrowHelper.ReThrowWithPath(ref state, ex); } int length = valueSpan.IsEmpty ? checked((int)valueSequence.Length) : valueSpan.Length; 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 b8df6c1..7a3984b 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 @@ -52,9 +52,7 @@ namespace System.Text.Json [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException_DeserializeUnableToConvertValue(Type propertyType) { - var ex = new JsonException(SR.Format(SR.DeserializeUnableToConvertValue, propertyType)); - ex.AppendPathInformation = true; - throw ex; + throw new JsonException(SR.Format(SR.DeserializeUnableToConvertValue, propertyType)) { AppendPathInformation = true }; } [DoesNotReturn] @@ -75,43 +73,28 @@ namespace System.Text.Json [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException_SerializationConverterRead(JsonConverter? converter) { - var ex = new JsonException(SR.Format(SR.SerializationConverterRead, converter)); - ex.AppendPathInformation = true; - throw ex; + throw new JsonException(SR.Format(SR.SerializationConverterRead, converter)) { AppendPathInformation = true }; } [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException_SerializationConverterWrite(JsonConverter? converter) { - var ex = new JsonException(SR.Format(SR.SerializationConverterWrite, converter)); - ex.AppendPathInformation = true; - throw ex; + throw new JsonException(SR.Format(SR.SerializationConverterWrite, converter)) { AppendPathInformation = true }; } [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException_SerializerCycleDetected(int maxDepth) { - throw new JsonException(SR.Format(SR.SerializerCycleDetected, maxDepth)); + throw new JsonException(SR.Format(SR.SerializerCycleDetected, maxDepth)) { AppendPathInformation = true }; } [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowJsonException(string? message = null) { - JsonException ex; - if (string.IsNullOrEmpty(message)) - { - ex = new JsonException(); - } - else - { - ex = new JsonException(message); - ex.AppendPathInformation = true; - } - - throw ex; + throw new JsonException(message) { AppendPathInformation = true }; } [DoesNotReturn] @@ -296,12 +279,12 @@ namespace System.Text.Json NotSupportedException ex = new NotSupportedException( SR.Format(SR.ObjectWithParameterizedCtorRefMetadataNotHonored, state.Current.JsonTypeInfo.Type)); - ThrowNotSupportedException(state, reader, ex); + ThrowNotSupportedException(ref state, reader, ex); } [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] - public static void ReThrowWithPath(in ReadStack state, JsonReaderException ex) + public static void ReThrowWithPath(ref ReadStack state, JsonReaderException ex) { Debug.Assert(ex.Path == null); @@ -328,15 +311,17 @@ namespace System.Text.Json [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] - public static void ReThrowWithPath(in ReadStack state, in Utf8JsonReader reader, Exception ex) + public static void ReThrowWithPath(ref ReadStack state, in Utf8JsonReader reader, Exception ex) { JsonException jsonException = new JsonException(null, ex); - AddJsonExceptionInformation(state, reader, jsonException); + AddJsonExceptionInformation(ref state, reader, jsonException); throw jsonException; } - public static void AddJsonExceptionInformation(in ReadStack state, in Utf8JsonReader reader, JsonException ex) + public static void AddJsonExceptionInformation(ref ReadStack state, in Utf8JsonReader reader, JsonException ex) { + Debug.Assert(ex.Path is null); // do not overwrite existing path information + long lineNumber = reader.CurrentState._lineNumber; ex.LineNumber = lineNumber; @@ -370,15 +355,17 @@ namespace System.Text.Json [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] - public static void ReThrowWithPath(in WriteStack state, Exception ex) + public static void ReThrowWithPath(ref WriteStack state, Exception ex) { JsonException jsonException = new JsonException(null, ex); - AddJsonExceptionInformation(state, jsonException); + AddJsonExceptionInformation(ref state, jsonException); throw jsonException; } - public static void AddJsonExceptionInformation(in WriteStack state, JsonException ex) + public static void AddJsonExceptionInformation(ref WriteStack state, JsonException ex) { + Debug.Assert(ex.Path is null); // do not overwrite existing path information + string path = state.PropertyPath(); ex.Path = path; @@ -432,7 +419,7 @@ namespace System.Text.Json } [DoesNotReturn] - public static void ThrowNotSupportedException(in ReadStack state, in Utf8JsonReader reader, NotSupportedException ex) + public static void ThrowNotSupportedException(ref ReadStack state, in Utf8JsonReader reader, NotSupportedException ex) { string message = ex.Message; @@ -464,7 +451,7 @@ namespace System.Text.Json } [DoesNotReturn] - public static void ThrowNotSupportedException(in WriteStack state, NotSupportedException ex) + public static void ThrowNotSupportedException(ref WriteStack state, NotSupportedException ex) { string message = ex.Message; @@ -508,14 +495,14 @@ namespace System.Text.Json message = SR.Format(SR.DeserializeNoConstructor, nameof(JsonConstructorAttribute), type); } - ThrowNotSupportedException(state, reader, new NotSupportedException(message)); + ThrowNotSupportedException(ref state, reader, new NotSupportedException(message)); } [DoesNotReturn] [MethodImpl(MethodImplOptions.NoInlining)] public static void ThrowNotSupportedException_CannotPopulateCollection(Type type, ref Utf8JsonReader reader, ref ReadStack state) { - ThrowNotSupportedException(state, reader, new NotSupportedException(SR.Format(SR.CannotPopulateCollection, type))); + ThrowNotSupportedException(ref state, reader, new NotSupportedException(SR.Format(SR.CannotPopulateCollection, type))); } [DoesNotReturn] diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.Array.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.Array.cs index 5ea4fe8..2d5f921 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.Array.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/CustomConverterTests/CustomConverterTests.Array.cs @@ -70,22 +70,14 @@ namespace System.Text.Json.Serialization.Tests [Fact] public static void CustomArrayConverterFail() { - string json = $"\"{Int64.MaxValue.ToString()}0\""; + string json = $"\"{long.MaxValue}0\""; - var options = new JsonSerializerOptions(); - options.Converters.Add(new LongArrayConverter()); + var options = new JsonSerializerOptions { Converters = { new LongArrayConverter() } }; + JsonException ex = Assert.Throws(() => JsonSerializer.Deserialize(json, options)); - try - { - JsonSerializer.Deserialize(json, options); - Assert.True(false, "Expected exception"); - } - catch (JsonException ex) - { - Assert.Null(ex.InnerException); - Assert.Equal("$", ex.Path); - Assert.Equal("Too big for a long", ex.Message); - } + Assert.Null(ex.InnerException); + Assert.Equal("$", ex.Path); + Assert.Equal("Too big for a long", ex.Message); } private class ClassWithProperty diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ExceptionTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ExceptionTests.cs index 05cc4f4..9346a64 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ExceptionTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ExceptionTests.cs @@ -677,5 +677,38 @@ namespace System.Text.Json.Serialization.Tests public StructWithBadCtor_WithProps(SerializationInfo info, StreamingContext ctx) => (Info, Ctx) = (info, ctx); } + + [Fact] + public static void CustomConverterThrowingJsonException_Serialization_ShouldNotOverwriteMetadata() + { + JsonException ex = Assert.Throws(() => JsonSerializer.Serialize(new { Value = new PocoUsingCustomConverterThrowingJsonException() })); + Assert.Equal(PocoConverterThrowingCustomJsonException.ExceptionMessage, ex.Message); + Assert.Equal(PocoConverterThrowingCustomJsonException.ExceptionPath, ex.Path); + } + + [Fact] + public static void CustomConverterThrowingJsonException_Deserialization_ShouldNotOverwriteMetadata() + { + JsonException ex = Assert.Throws(() => JsonSerializer.Deserialize(@"[{}]")); + Assert.Equal(PocoConverterThrowingCustomJsonException.ExceptionMessage, ex.Message); + Assert.Equal(PocoConverterThrowingCustomJsonException.ExceptionPath, ex.Path); + } + + [JsonConverter(typeof(PocoConverterThrowingCustomJsonException))] + public class PocoUsingCustomConverterThrowingJsonException + { + } + + public class PocoConverterThrowingCustomJsonException : JsonConverter + { + public const string ExceptionMessage = "Custom JsonException mesage"; + public const string ExceptionPath = "$.CustomPath"; + + public override PocoUsingCustomConverterThrowingJsonException? Read(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options) + => throw new JsonException(ExceptionMessage, ExceptionPath, 0, 0); + + public override void Write(Utf8JsonWriter writer, PocoUsingCustomConverterThrowingJsonException value, JsonSerializerOptions options) + => throw new JsonException(ExceptionMessage, ExceptionPath, 0, 0); + } } } -- 2.7.4