From beb561f152e5b16b575fa54db8d92453ce9ed2db Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 9 Aug 2021 15:48:36 +0300 Subject: [PATCH] Fix reference preservation for boxed structs (#56412) * Fix reference preservation for boxed structs. * add null assertion for BoxedStructReferenceId when writing a new instance --- .../Collection/DictionaryDefaultConverter.cs | 2 + .../Collection/IEnumerableDefaultConverter.cs | 2 + .../Converters/Object/ObjectDefaultConverter.cs | 2 + .../Text/Json/Serialization/JsonConverter.cs | 2 +- .../Text/Json/Serialization/JsonConverterOfT.cs | 24 +++++-- .../JsonSerializer.Write.HandleMetadata.cs | 58 +++++++++++++++-- .../System/Text/Json/Serialization/WriteStack.cs | 5 ++ .../Serialization/ReferenceHandlerTests.cs | 74 ++++++++++++++++++++++ 8 files changed, 159 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs index 9f7fa9d..ba83667 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/DictionaryDefaultConverter.cs @@ -16,6 +16,8 @@ namespace System.Text.Json.Serialization.Converters where TDictionary : IEnumerable> where TKey : notnull { + internal override bool CanHaveIdMetadata => true; + protected internal override bool OnWriteResume( Utf8JsonWriter writer, TDictionary value, diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs index b623f42..c8d1c5f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Collection/IEnumerableDefaultConverter.cs @@ -12,6 +12,8 @@ namespace System.Text.Json.Serialization.Converters internal abstract class IEnumerableDefaultConverter : JsonCollectionConverter where TCollection : IEnumerable { + internal override bool CanHaveIdMetadata => true; + protected override bool OnWriteResume(Utf8JsonWriter writer, TCollection value, JsonSerializerOptions options, ref WriteStack state) { Debug.Assert(value is not null); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 2f9bc99..a1f70ed 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -14,6 +14,8 @@ namespace System.Text.Json.Serialization.Converters /// internal class ObjectDefaultConverter : JsonObjectConverter where T : notnull { + internal override bool CanHaveIdMetadata => true; + internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSerializerOptions options, ref ReadStack state, [MaybeNullWhen(false)] out T value) { JsonTypeInfo jsonTypeInfo = state.Current.JsonTypeInfo; 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 f590292..f1626c4 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 @@ -30,7 +30,7 @@ namespace System.Text.Json.Serialization /// /// Can the converter have $id metadata. /// - internal virtual bool CanHaveIdMetadata => true; + internal virtual bool CanHaveIdMetadata => false; internal bool CanBePolymorphic { get; set; } 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 e083653..89d6c38 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 @@ -379,12 +379,26 @@ namespace System.Text.Json.Serialization JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); Debug.Assert(jsonConverter != this); - if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && - jsonConverter.IsValueType) + // For boxed value types: invoke the reference handler + // before the instance gets unboxed by the subtype converter. + if (jsonConverter.IsValueType) { - // For boxed value types: push the value before it gets unboxed on TryWriteAsObject. - state.ReferenceResolver.PushReferenceForCycleDetection(value); - ignoreCyclesPopReference = true; + switch (options.ReferenceHandlingStrategy) + { + case ReferenceHandlingStrategy.Preserve when (jsonConverter.CanHaveIdMetadata && !state.IsContinuation): + if (JsonSerializer.TryWriteReferenceForBoxedStruct(value, ref state, writer)) + { + return true; + } + break; + + case ReferenceHandlingStrategy.IgnoreCycles: + state.ReferenceResolver.PushReferenceForCycleDetection(value); + ignoreCyclesPopReference = true; + break; + default: + break; + } } // We found a different converter; forward to that. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs index 45c1462..e792bcf 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.HandleMetadata.cs @@ -21,9 +21,19 @@ namespace System.Text.Json { MetadataPropertyName writtenMetadataName; - // If the jsonConverter supports immutable dictionaries or value types, don't write any metadata - if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) + if (state.BoxedStructReferenceId != null) { + // We're serializing a struct that has been handled by a polymorphic converter; + // emit the reference id that was recorded for the boxed instance. + + Debug.Assert(jsonConverter.IsValueType && jsonConverter.CanHaveIdMetadata); + writer.WriteString(s_metadataId, state.BoxedStructReferenceId); + writtenMetadataName = MetadataPropertyName.Id; + state.BoxedStructReferenceId = null; + } + else if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) + { + // If the jsonConverter supports immutable dictionaries or value types, don't write any metadata writtenMetadataName = MetadataPropertyName.NoMetadata; } else @@ -55,9 +65,22 @@ namespace System.Text.Json { MetadataPropertyName writtenMetadataName; - // If the jsonConverter supports immutable enumerables or value type collections, don't write any metadata - if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) + if (state.BoxedStructReferenceId != null) { + // We're serializing a struct that has been handled by a polymorphic converter; + // emit the reference id that was recorded for the boxed instance. + + Debug.Assert(jsonConverter.IsValueType && jsonConverter.CanHaveIdMetadata); + + writer.WriteStartObject(); + writer.WriteString(s_metadataId, state.BoxedStructReferenceId); + writer.WriteStartArray(s_metadataValues); + writtenMetadataName = MetadataPropertyName.Id; + state.BoxedStructReferenceId = null; + } + else if (!jsonConverter.CanHaveIdMetadata || jsonConverter.IsValueType) + { + // If the jsonConverter supports immutable enumerables or value type collections, don't write any metadata writer.WriteStartArray(); writtenMetadataName = MetadataPropertyName.NoMetadata; } @@ -84,5 +107,32 @@ namespace System.Text.Json return writtenMetadataName; } + + /// + /// Used by polymorphic converters that are handling references for values that are boxed structs. + /// + internal static bool TryWriteReferenceForBoxedStruct(object currentValue, ref WriteStack state, Utf8JsonWriter writer) + { + Debug.Assert(state.BoxedStructReferenceId == null); + Debug.Assert(currentValue.GetType().IsValueType); + + string referenceId = state.ReferenceResolver.GetReference(currentValue, out bool alreadyExists); + Debug.Assert(referenceId != null); + + if (alreadyExists) + { + writer.WriteStartObject(); + writer.WriteString(s_metadataRef, referenceId); + writer.WriteEndObject(); + } + else + { + // Since we cannot run `ReferenceResolver.GetReference` twice for newly encountered instances, + // need to store the reference id for use by the subtype converter we're dispatching to. + state.BoxedStructReferenceId = referenceId; + } + + return alreadyExists; + } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs index e9a9f1e..bc313da 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/WriteStack.cs @@ -70,6 +70,11 @@ namespace System.Text.Json /// public bool SupportContinuation; + /// + /// Stores a reference id that has been calculated by a polymorphic converter handling a newly encountered boxed struct. + /// + public string? BoxedStructReferenceId; + private void EnsurePushCapacity() { if (_stack is null) diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ReferenceHandlerTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ReferenceHandlerTests.cs index 030f637..45e5ff4 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ReferenceHandlerTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/ReferenceHandlerTests.cs @@ -1,6 +1,7 @@ // 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.Collections.Immutable; using System.IO; @@ -780,6 +781,56 @@ namespace System.Text.Json.Serialization.Tests Assert.Equal("$.Sibling", ex.Path); } + [Fact] + public static void BoxedStructReferencePreservation_NestedStructObject() + { + IBoxedStructWithObjectProperty value = new StructWithObjectProperty(); + value.Property = new object[] { value }; + + string json = JsonSerializer.Serialize(value, new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve }); + Assert.Equal(@"{""$id"":""1"",""Property"":[{""$ref"":""1""}]}", json); + } + + [Fact] + public static void BoxedStructReferencePreservation_NestedStructCollection() + { + IBoxedStructWithObjectProperty value = new StructCollection(); + value.Property = new object[] { value }; + + string json = JsonSerializer.Serialize(value, s_serializerOptionsPreserve); + Assert.Equal(@"{""$id"":""1"",""Property"":[{""$ref"":""1""}]}", json); + } + + [Fact] + public static void BoxedStructReferencePreservation_SiblingStructObjects() + { + object box = new StructWithObjectProperty { Property = 42 }; + var array = new object[] { box, box }; + + string json = JsonSerializer.Serialize(array, s_serializerOptionsPreserve); + Assert.Equal(@"[{""$id"":""1"",""Property"":42},{""$ref"":""1""}]", json); + } + + [Fact] + public static void BoxedStructReferencePreservation_SiblingStructCollections() + { + object box = new StructCollection { Property = 42 }; + var array = new object[] { box, box }; + + string json = JsonSerializer.Serialize(array, s_serializerOptionsPreserve); + Assert.Equal(@"[{""$id"":""1"",""$values"":[42]},{""$ref"":""1""}]", json); + } + + [Fact] + public static void BoxedStructReferencePreservation_SiblingPrimitiveValues() + { + object box = 42; + var array = new object[] { box, box }; + + string json = JsonSerializer.Serialize(array, s_serializerOptionsPreserve); + Assert.Equal(@"[42,42]", json); + } + private class ClassWithObjectProperty { public ClassWithObjectProperty Child { get; set; } @@ -791,5 +842,28 @@ namespace System.Text.Json.Serialization.Tests public ClassWithListOfObjectProperty Child { get; set; } public List ListOfObjects { get; set; } } + + private interface IBoxedStructWithObjectProperty + { + object? Property { get; set; } + } + + private struct StructWithObjectProperty : IBoxedStructWithObjectProperty + { + public object? Property { get; set; } + } + + private struct StructCollection : IBoxedStructWithObjectProperty, IEnumerable + { + public object? Property { get; set; } + + public IEnumerator GetEnumerator() + { + yield return Property; + } + + IEnumerator IEnumerable.GetEnumerator() => GetEnumerator(); + } + } } -- 2.7.4