From 152db423e06f6d93a560b45b4330fac6507c7aa7 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 21 Jan 2021 02:46:19 -0800 Subject: [PATCH 1/9] Add option to ignore reference cycles on serialization --- .../System.Text.Json/ref/System.Text.Json.cs | 1 + .../src/System.Text.Json.csproj | 3 + .../Collection/DictionaryDefaultConverter.cs | 5 +- .../Collection/IEnumerableDefaultConverter.cs | 14 +- .../Object/ObjectDefaultConverter.cs | 8 +- .../Serialization/IgnoreReferenceHandler.cs | 12 + .../Serialization/IgnoreReferenceResolver.cs | 42 +++ .../JsonConverterOfT.WriteCore.cs | 8 + .../Json/Serialization/JsonConverterOfT.cs | 78 +++-- .../Json/Serialization/JsonPropertyInfoOfT.cs | 8 + .../JsonSerializer.Read.HandlePropertyName.cs | 2 +- .../JsonSerializer.Write.HandleMetadata.cs | 6 + .../Text/Json/Serialization/ReadStack.cs | 2 +- .../Serialization/ReferenceEqualsWrapper.cs | 16 + .../Json/Serialization/ReferenceHandler.cs | 10 + .../Json/Serialization/ReferenceResolver.cs | 8 + .../Text/Json/Serialization/WriteStack.cs | 2 +- .../ReferenceHandlerTests.IgnoreCycle.cs | 324 ++++++++++++++++++ .../tests/System.Text.Json.Tests.csproj | 3 +- 19 files changed, 512 insertions(+), 40 deletions(-) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceEqualsWrapper.cs create mode 100644 src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs diff --git a/src/libraries/System.Text.Json/ref/System.Text.Json.cs b/src/libraries/System.Text.Json/ref/System.Text.Json.cs index a70874667f20f..1829e653ca971 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -559,6 +559,7 @@ public abstract partial class ReferenceHandler { protected ReferenceHandler() { } public static System.Text.Json.Serialization.ReferenceHandler Preserve { get { throw null; } } + public static System.Text.Json.Serialization.ReferenceHandler IgnoreCycle { get { throw null; } } public abstract System.Text.Json.Serialization.ReferenceResolver CreateResolver(); } public sealed partial class ReferenceHandler : System.Text.Json.Serialization.ReferenceHandler where T : System.Text.Json.Serialization.ReferenceResolver, new() diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index a72d7c6d2beb7..2018f5cb04093 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -64,6 +64,8 @@ + + @@ -128,6 +130,7 @@ + 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 7ada24a994116..8f3e63ec33b04 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 @@ -133,7 +133,7 @@ internal sealed override bool OnTryRead( } // Handle the metadata properties. - bool preserveReferences = options.ReferenceHandler != null; + bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); if (preserveReferences && state.Current.ObjectState < StackFrameObjectState.PropertyValue) { if (JsonSerializer.ResolveMetadataForJsonObject(ref reader, ref state, options)) @@ -272,8 +272,7 @@ internal sealed override bool OnTryWrite( { state.Current.ProcessedStartToken = true; writer.WriteStartObject(); - - if (options.ReferenceHandler != null) + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { if (JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer) == MetadataPropertyName.Ref) { 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 e7038ac9ec28b..8978d0553e1e0 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 @@ -90,7 +90,7 @@ internal override bool OnTryRead( { // Slower path that supports continuation and preserved references. - bool preserveReferences = options.ReferenceHandler != null; + bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); if (state.Current.ObjectState == StackFrameObjectState.None) { if (reader.TokenType == JsonTokenType.StartArray) @@ -236,12 +236,7 @@ internal sealed override bool OnTryWrite( if (!state.Current.ProcessedStartToken) { state.Current.ProcessedStartToken = true; - - if (options.ReferenceHandler == null) - { - writer.WriteStartArray(); - } - else + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, value, ref state, writer); if (metadata == MetadataPropertyName.Ref) @@ -251,6 +246,11 @@ internal sealed override bool OnTryWrite( state.Current.MetadataPropertyName = metadata; } + else + { + writer.WriteStartArray(); + + } state.Current.DeclaredJsonPropertyInfo = state.Current.JsonClassInfo.ElementClassInfo!.PropertyInfoForClassInfo; } 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 9dde207e48c21..b0fcdee0d1de3 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 @@ -75,7 +75,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, // Handle the metadata properties. if (state.Current.ObjectState < StackFrameObjectState.PropertyValue) { - if (options.ReferenceHandler != null) + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { if (JsonSerializer.ResolveMetadataForJsonObject(ref reader, ref state, options)) { @@ -233,8 +233,7 @@ internal sealed override bool OnTryWrite( if (!state.SupportContinuation) { writer.WriteStartObject(); - - if (options.ReferenceHandler != null) + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref) { @@ -289,8 +288,7 @@ internal sealed override bool OnTryWrite( if (!state.Current.ProcessedStartToken) { writer.WriteStartObject(); - - if (options.ReferenceHandler != null) + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs new file mode 100644 index 0000000000000..db4235bd239a0 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs @@ -0,0 +1,12 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.Text.Json.Serialization +{ + internal sealed class IgnoreReferenceHandler : ReferenceHandler + { + public IgnoreReferenceHandler() => UsePreserveSemantics = false; + + public override ReferenceResolver CreateResolver() => new IgnoreReferenceResolver(); + } +} diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs new file mode 100644 index 0000000000000..f38ce69f0220a --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs @@ -0,0 +1,42 @@ +// 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.Generic; +using System.Diagnostics; + +namespace System.Text.Json.Serialization +{ + internal sealed class IgnoreReferenceResolver : ReferenceResolver + { + // The stack of references on the branch of the current object graph, used to detect reference cycles. + private Stack? _stackForCycleDetection; + + internal override void PopReferenceForCycleDetection() + { + if (_stackForCycleDetection != null) + { + _stackForCycleDetection.Pop(); + } + } + + internal override bool ContainsReferenceForCycleDetection(object value) + => _stackForCycleDetection?.Contains(new ReferenceEqualsWrapper(value)) ?? false; + + internal override void PushReferenceForCycleDetection(object value) + { + var wrappedValue = new ReferenceEqualsWrapper(value); + + if (_stackForCycleDetection is null) + { + _stackForCycleDetection = new Stack(); + } + + Debug.Assert(!_stackForCycleDetection.Contains(wrappedValue)); + _stackForCycleDetection.Push(wrappedValue); + } + + public override void AddReference(string referenceId, object value) => throw new InvalidOperationException(); + public override string GetReference(object value, out bool alreadyExists) => throw new InvalidOperationException(); + public override object ResolveReference(string referenceId) => throw new InvalidOperationException(); + } +} 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 94240f3368829..26cd22e17f48c 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 @@ -17,6 +17,14 @@ internal sealed override bool WriteCoreAsObject( ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); } + // Root object is a boxed value type. + // Since value types are ignored when detecting cycles + // We need to push it before it gets unboxed. + if (value != null && IsValueType && JsonSerializer.IsIgnoreCyclesEnabled(options)) + { + state.ReferenceResolver.PushReferenceForCycleDetection(value); + } + T actualValue = (T)value!; return WriteCore(writer, actualValue, options, ref state); } 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 d9cda3bafd97e..ce633a4a35530 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 @@ -196,7 +196,7 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali ref reader); } - if (CanBePolymorphic && options.ReferenceHandler != null && value is JsonElement element) + if (CanBePolymorphic && JsonSerializer.IsPreserveReferencesEnabled(options) && value is JsonElement element) { // Edge case where we want to lookup for a reference when parsing into typeof(object) // instead of return `value` as a JsonElement. @@ -303,23 +303,47 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth); } + bool ignoreCyclesEnabled = JsonSerializer.IsIgnoreCyclesEnabled(options); + bool ignoreCyclesPopReference = false; + + if (CanBeNull && IsNull(value)) + { + if (!HandleNullOnWrite) + { + // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were + // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here. + writer.WriteNullValue(); + return true; + } + } + else if (ignoreCyclesEnabled && !IsValueType) + { + ReferenceResolver resolver = state.ReferenceResolver; + // Write null to break reference cycles. + if (resolver.ContainsReferenceForCycleDetection(value)) + { + writer.WriteNullValue(); + return true; + } + // For boxed reference types: do not push when boxed in order to avoid false positives when we run above check for the converter of the unboxed value. + if (!CanBePolymorphic) + { + resolver.PushReferenceForCycleDetection(value); + ignoreCyclesPopReference = true; + } + } + if (CanBePolymorphic) { if (value == null) { - if (!HandleNullOnWrite) - { - writer.WriteNullValue(); - } - else - { - Debug.Assert(ClassType == ClassType.Value); - Debug.Assert(!state.IsContinuation); + Debug.Assert(ClassType == ClassType.Value); + Debug.Assert(!state.IsContinuation); + Debug.Assert(HandleNullOnWrite); - int originalPropertyDepth = writer.CurrentDepth; - Write(writer, value, options); - VerifyWrite(originalPropertyDepth, writer); - } + int originalPropertyDepth = writer.CurrentDepth; + Write(writer, value, options); + VerifyWrite(originalPropertyDepth, writer); return true; } @@ -339,18 +363,25 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); if (jsonConverter != this) { + if (ignoreCyclesEnabled && jsonConverter.IsValueType) + { + // For boxed value types: push the value before it gets unboxed. + state.ReferenceResolver.PushReferenceForCycleDetection(value); + ignoreCyclesPopReference = true; + } + // We found a different converter; forward to that. - return jsonConverter.TryWriteAsObject(writer, value, options, ref state); + bool success2 = jsonConverter.TryWriteAsObject(writer, value, options, ref state); + + if (ignoreCyclesPopReference) + { + state.ReferenceResolver.PopReferenceForCycleDetection(); + } + + return success2; } } } - else if (CanBeNull && !HandleNullOnWrite && IsNull(value)) - { - // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were - // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here. - writer.WriteNullValue(); - return true; - } if (ClassType == ClassType.Value) { @@ -390,6 +421,11 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions state.Pop(success); + if (ignoreCyclesPopReference) + { + state.ReferenceResolver.PopReferenceForCycleDetection(); + } + return success; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs index 797a96c43ceb0..8be268ab3c08a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs @@ -138,6 +138,14 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf { T value = Get!(obj); + if (JsonSerializer.IsIgnoreCyclesEnabled(Options) && !Converter.IsValueType && + value != null && state.ReferenceResolver.ContainsReferenceForCycleDetection(value)) + { + // If a reference cycle is detected, treat value as null. + value = default!; + Debug.Assert(value == null); + } + if (IgnoreDefaultValuesOnWrite) { // If value is null, it is a reference type or nullable. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs index 17a0669b2a4ab..5f1d91e6d6e99 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs @@ -79,7 +79,7 @@ internal static ReadOnlySpan GetPropertyName( unescapedPropertyName = propertyName; } - if (options.ReferenceHandler != null) + if (JsonSerializer.IsPreserveReferencesEnabled(options)) { if (propertyName.Length > 0 && propertyName[0] == '$') { 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 45c1462ea68f3..7732c2f703a7f 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 @@ -84,5 +84,11 @@ internal static MetadataPropertyName WriteReferenceForCollection( return writtenMetadataName; } + + internal static bool IsPreserveReferencesEnabled(JsonSerializerOptions options) + => options.ReferenceHandler?.UsePreserveSemantics ?? false; + + internal static bool IsIgnoreCyclesEnabled(JsonSerializerOptions options) + => !options.ReferenceHandler?.UsePreserveSemantics ?? false; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs index ac03e3b6adba6..daa94277aa0d2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs @@ -89,7 +89,7 @@ public void Initialize(Type type, JsonSerializerOptions options, bool supportCon Current.NumberHandling = Current.JsonPropertyInfo.NumberHandling; - bool preserveReferences = options.ReferenceHandler != null; + bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); if (preserveReferences) { ReferenceResolver = options.ReferenceHandler!.CreateResolver(writing: false); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceEqualsWrapper.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceEqualsWrapper.cs new file mode 100644 index 0000000000000..dc292e2213096 --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceEqualsWrapper.cs @@ -0,0 +1,16 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using System.Runtime.CompilerServices; + +namespace System.Text.Json.Serialization +{ + internal struct ReferenceEqualsWrapper : IEquatable + { + private object _object; + public ReferenceEqualsWrapper(object obj) => _object = obj; + public override bool Equals(object? obj) => obj is ReferenceEqualsWrapper otherObj && Equals(otherObj); + public bool Equals(ReferenceEqualsWrapper obj) => ReferenceEquals(_object, obj._object); + public override int GetHashCode() => RuntimeHelpers.GetHashCode(_object); + } +} diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs index 5a951c6fb5a1a..71f8fdf6d965b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs @@ -10,6 +10,11 @@ namespace System.Text.Json.Serialization /// public abstract class ReferenceHandler { + /// + /// Whether we read and write preserve references or we ignore references when writing. + /// + internal bool UsePreserveSemantics = true; + /// /// Metadata properties will be honored when deserializing JSON objects and arrays into reference types and written when serializing reference types. This is necessary to create round-trippable JSON from objects that contain cycles or duplicate references. /// @@ -39,6 +44,11 @@ public abstract class ReferenceHandler /// public static ReferenceHandler Preserve { get; } = new PreserveReferenceHandler(); + /// + /// Ignores an object when a reference cycle is detected during serialization. + /// + public static ReferenceHandler IgnoreCycle { get; } = new IgnoreReferenceHandler(); + /// /// Returns the used for each serialization call. /// diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceResolver.cs index a90e7f6c5ae10..4fbd68715426c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceResolver.cs @@ -33,5 +33,13 @@ public abstract class ReferenceResolver /// The reference id related to the returned object. /// The reference type object related to specified reference id. public abstract object ResolveReference(string referenceId); + + // We are breaking single responsibility on this class internally. + // In the future, if this model is required to be exposed, we can add a base class and extend this class and a new class containing below members from that base class. + internal virtual void PopReferenceForCycleDetection() => throw new InvalidOperationException(); + + internal virtual void PushReferenceForCycleDetection(object value) => throw new InvalidOperationException(); + + internal virtual bool ContainsReferenceForCycleDetection(object value) => throw new InvalidOperationException(); } } 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 e3378711eb1eb..f5bc5c41e191f 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 @@ -75,7 +75,7 @@ public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool s if (options.ReferenceHandler != null) { - ReferenceResolver = options.ReferenceHandler!.CreateResolver(writing: true); + ReferenceResolver = options.ReferenceHandler.CreateResolver(writing: true); } SupportContinuation = supportContinuation; diff --git a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs new file mode 100644 index 0000000000000..075105bdd01e9 --- /dev/null +++ b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs @@ -0,0 +1,324 @@ +// 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.Concurrent; +using System.Collections.Generic; +using System.Collections.ObjectModel; +using System.IO; +using System.Threading.Tasks; +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ + public class ReferenceHandlerTests_IgnoreCycle + { + private static readonly JsonSerializerOptions s_optionsIgnoreCycles = + new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.IgnoreCycle }; + + [Fact] + public async Task IgnoreCycles_OnObject() + { + var root = new Node(); + root.Next = root; + + await Test_Serialize_And_SerializeAsync(root, @"{""Next"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnObject_NonProperty() + { + var node = new Node(); + var rootList = new List() { node }; + node.Next = rootList; + + await Test_Serialize_And_SerializeAsync(rootList, @"[{""Next"":null}]", s_optionsIgnoreCycles); + } + + [Theory] + [InlineData(typeof(Dictionary))] + [InlineData(typeof(GenericIDictionaryWrapper))] + public async Task IgnoreCycles_OnDictionary(Type typeToSerialize) + { + var root = (ICollection>)Activator.CreateInstance(typeToSerialize); + root.Add(new KeyValuePair("self", root)); + + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnReadOnlyDictionary() + { + var innerDictionary = new Dictionary(); + var root = new ReadOnlyDictionary(innerDictionary); + + innerDictionary.Add("self", root); + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnIDictionary() + { + var root = new WrapperForIDictionary(); + root.Add("self", root); + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnArray() + { + var root = new object[1]; + root[0] = root; + await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); + } + + [Theory] + [InlineData(typeof(List))] + [InlineData(typeof(GenericIListWrapper))] + public async Task IgnoreCycles_OnLists(Type typeToSerialize) + { + var root = (IList)Activator.CreateInstance(typeToSerialize); + root.Add(root); + await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); + } + + [Theory] + [InlineData(typeof(GenericISetWrapper))] + [InlineData(typeof(GenericICollectionWrapper))] + public async Task IgnoreCycles_OnCollections(Type typeToSerialize) + { + var root = (ICollection)Activator.CreateInstance(typeToSerialize); + root.Add(root); + await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnCollections_WithoutAddMethod() + { + var root = new Stack(); + root.Push(root); + await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); + + var root2 = new Queue(); + root2.Enqueue(root2); + await Test_Serialize_And_SerializeAsync(root2, "[null]", s_optionsIgnoreCycles); + + var root3 = new ConcurrentStack(); + root3.Push(root3); + await Test_Serialize_And_SerializeAsync(root3, "[null]", s_optionsIgnoreCycles); + + var root4 = new ConcurrentQueue(); + root4.Enqueue(root4); + await Test_Serialize_And_SerializeAsync(root4, "[null]", s_optionsIgnoreCycles); + + var root5 = new Stack(); + root5.Push(root5); + await Test_Serialize_And_SerializeAsync(root5, "[null]", s_optionsIgnoreCycles); + + var root6 = new Queue(); + root6.Enqueue(root6); + await Test_Serialize_And_SerializeAsync(root6, "[null]", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnExtensionData() + { + var root = new EmptyClassWithExtensionProperty(); + root.MyOverflow.Add("root", root); + await Test_Serialize_And_SerializeAsync(root, @"{""root"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnBoxedValueTypes() + { + IValueNode root = new ValueNode(); + root.Next = root; + + await Test_Serialize_And_SerializeAsync(root, @"{""Next"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnBoxedValueTypes_AsProperty() + { + IValueNode node = new ValueNode(); + node.Next = node; + + var root = new Node(); + root.Next = node; + + await Test_Serialize_And_SerializeAsync(root, @"{""Next"":{""Next"":null}}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_DoesNotSupportPreserveSemantics() + { + // Object + var node = new NodeWithExtensionData(); + node.Next = node; + string json = SerializeWithPreserve(node); + + node = JsonSerializer.Deserialize(json, s_optionsIgnoreCycles); + Assert.True(node.MyOverflow.ContainsKey("$id")); + Assert.True(node.Next.MyOverflow.ContainsKey("$ref")); + + using var ms = new MemoryStream(Encoding.UTF8.GetBytes(json)); + node = await JsonSerializer.DeserializeAsync(ms, s_optionsIgnoreCycles); + Assert.True(node.MyOverflow.ContainsKey("$id")); + Assert.True(node.Next.MyOverflow.ContainsKey("$ref")); + + // Dictionary + var dictionary = new RecursiveDictionary(); + dictionary.Add("self", dictionary); + json = SerializeWithPreserve(dictionary); + + Assert.Throws(() => JsonSerializer.Deserialize(json, s_optionsIgnoreCycles)); + using var ms2 = new MemoryStream(Encoding.UTF8.GetBytes(json)); + await Assert.ThrowsAsync(() => JsonSerializer.DeserializeAsync(ms2, s_optionsIgnoreCycles).AsTask()); + + // List + var list = new RecursiveList(); + list.Add(list); + json = SerializeWithPreserve(list); + + Assert.Throws(() => JsonSerializer.Deserialize(json, s_optionsIgnoreCycles)); + using var ms3 = new MemoryStream(Encoding.UTF8.GetBytes(json)); + await Assert.ThrowsAsync(() => JsonSerializer.DeserializeAsync(ms3, s_optionsIgnoreCycles).AsTask()); + } + + [Fact] + public async Task IgnoreCycles_DoesNotSupportPreserveSemantics_Polymorphic() + { + // Object + var node = new Node(); + node.Next = node; + string json = SerializeWithPreserve(node); + + node = JsonSerializer.Deserialize(json, s_optionsIgnoreCycles); + JsonElement nodeAsJsonElement = Assert.IsType(node.Next); + Assert.True(nodeAsJsonElement.GetProperty("$ref").GetString() == "1"); + + using var ms = new MemoryStream(Encoding.UTF8.GetBytes(json)); + node = await JsonSerializer.DeserializeAsync(ms, s_optionsIgnoreCycles); + nodeAsJsonElement = Assert.IsType(node.Next); + Assert.True(nodeAsJsonElement.GetProperty("$ref").GetString() == "1"); + + // Dictionary + var dictionary = new Dictionary(); + dictionary.Add("self", dictionary); + json = SerializeWithPreserve(dictionary); + + dictionary = JsonSerializer.Deserialize>(json, s_optionsIgnoreCycles); + } + + private string SerializeWithPreserve(T value) + { + var opts = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.Preserve }; + return JsonSerializer.Serialize(value, opts); + } + + // Test for correctness when the object reference is found on a sibling branch. + [Theory] + [InlineData(typeof(EmptyClass), "{}")] + [InlineData(typeof(EmptyStruct), "{}")] + [InlineData(typeof(object), "{}")] + [InlineData(typeof(Dictionary), "{}")] + [InlineData(typeof(List), "[]")] + public async Task AlreadySeenInstance_ShouldNotBeIgnoredOnSecondBranch(Type objectType, string objectPayload) + { + object obj = Activator.CreateInstance(objectType); + var root = new TreeNode(); + root.Left = obj; + root.Right = obj; + + await Test_Serialize_And_SerializeAsync(root, $@"{{""Left"":{objectPayload},""Right"":{objectPayload}}}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_WhenWritingNull() + { + var opts = new JsonSerializerOptions + { + ReferenceHandler = ReferenceHandler.IgnoreCycle, + DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull + }; + + // Reference cycles are treated as null, hence the JsonIgnoreCondition can be used to actually ignore the property. + var rootObj = new Node(); + rootObj.Next = rootObj; + + await Test_Serialize_And_SerializeAsync(rootObj, "{}", opts); + + // JsonIgnoreCondition does not ignore nulls in collections, hence a reference loop should not omit writing it. + // This also helps us to avoid changing the length of a collection when the loop is detected in one of the elements. + var rootList = new List(); + rootList.Add(rootList); + + await Test_Serialize_And_SerializeAsync(rootList, "[null]", opts); + + var rootDictionary = new Dictionary(); + rootDictionary.Add("self", rootDictionary); + + await Test_Serialize_And_SerializeAsync(rootDictionary, @"{""self"":null}", opts); + } + + private async Task Test_Serialize_And_SerializeAsync(object obj, string expected, JsonSerializerOptions options) + { + string json = JsonSerializer.Serialize(obj, options); + Assert.Equal(expected, json); + + using var ms = new MemoryStream(); + await JsonSerializer.SerializeAsync(ms, obj, options).ConfigureAwait(false); + json = Encoding.UTF8.GetString(ms.ToArray()); + Assert.Equal(expected, json); + } + + private class Employee + { + public string Name { get; set; } + public Employee Manager { get; set; } + public List Subordinates { get; set; } + public Dictionary Colleagues { get; set; } + } + + private class Node + { + public object Next { get; set; } + } + + private class TreeNode + { + public object Left { get; set; } + public object Right { get; set; } + } + + private struct ValueNode : IValueNode + { + public object Next { get; set; } + } + + interface IValueNode + { + public object Next { get; set; } + } + + private class EmptyClass { } + private struct EmptyStruct { } + + private class EmptyClassWithExtensionProperty + { + [JsonExtensionData] + public Dictionary MyOverflow { get; set; } = new Dictionary(); + } + + private class NodeWithExtensionData + { + [JsonExtensionData] + public Dictionary MyOverflow { get; set; } = new Dictionary(); + public NodeWithExtensionData Next { get; set; } + } + + private class RecursiveDictionary : Dictionary { } + + private class RecursiveList : List { } + } +} diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj index 7632275629fe7..a99a92f92eb17 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj @@ -1,4 +1,4 @@ - + $(NetCoreAppCurrent);net461 true @@ -117,6 +117,7 @@ + From f980ad0be68e685f7c8ef8d5eb595a7a2d9b2394 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 21 Jan 2021 03:10:09 -0800 Subject: [PATCH 2/9] Fix whitespace --- .../Converters/Collection/IEnumerableDefaultConverter.cs | 1 - 1 file changed, 1 deletion(-) 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 8978d0553e1e0..098e7aeaa9aa6 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 @@ -249,7 +249,6 @@ internal sealed override bool OnTryWrite( else { writer.WriteStartArray(); - } state.Current.DeclaredJsonPropertyInfo = state.Current.JsonClassInfo.ElementClassInfo!.PropertyInfoForClassInfo; From 0eaf1496380ebd0e4f76f5a473467c59030e6224 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 21 Jan 2021 03:14:12 -0800 Subject: [PATCH 3/9] Combine IsValueType check on WriteCoreAsObject --- .../JsonConverterOfT.WriteCore.cs | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) 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 26cd22e17f48c..259afdfb508dc 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 @@ -11,18 +11,19 @@ internal sealed override bool WriteCoreAsObject( JsonSerializerOptions options, ref WriteStack state) { - // Value types can never have a null except for Nullable. - if (value == null && IsValueType && Nullable.GetUnderlyingType(TypeToConvert) == null) + if (IsValueType) { - ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); - } + // Value types can never have a null except for Nullable. + if (value == null && Nullable.GetUnderlyingType(TypeToConvert) == null) + { + ThrowHelper.ThrowJsonException_DeserializeUnableToConvertValue(TypeToConvert); + } - // Root object is a boxed value type. - // Since value types are ignored when detecting cycles - // We need to push it before it gets unboxed. - if (value != null && IsValueType && JsonSerializer.IsIgnoreCyclesEnabled(options)) - { - state.ReferenceResolver.PushReferenceForCycleDetection(value); + // Root object is a boxed value type, we need to push it to the reference stack before it gets unboxed here. + if (value != null && JsonSerializer.IsIgnoreCyclesEnabled(options)) + { + state.ReferenceResolver.PushReferenceForCycleDetection(value); + } } T actualValue = (T)value!; From 2e55d738d9899d9623c809f8d79479dc081fa524 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 21 Jan 2021 03:14:23 -0800 Subject: [PATCH 4/9] Add missing null-forgiving operator --- .../src/System/Text/Json/Serialization/JsonConverterOfT.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 ce633a4a35530..e869c024cc141 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 @@ -320,7 +320,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions { ReferenceResolver resolver = state.ReferenceResolver; // Write null to break reference cycles. - if (resolver.ContainsReferenceForCycleDetection(value)) + if (resolver.ContainsReferenceForCycleDetection(value!)) { writer.WriteNullValue(); return true; From 1905462b9b48b694eb898941d4f1019c14dbdce5 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Mon, 25 Jan 2021 23:31:50 -0800 Subject: [PATCH 5/9] Fix perf regression by using ReferenceHandlerStrategy field in JsonSerializerOptions --- .../src/System.Text.Json.csproj | 1 + .../Collection/DictionaryDefaultConverter.cs | 4 +-- .../Collection/IEnumerableDefaultConverter.cs | 4 +-- .../Object/ObjectDefaultConverter.cs | 6 ++-- .../Serialization/IgnoreReferenceHandler.cs | 2 +- .../JsonConverterOfT.WriteCore.cs | 2 +- .../Json/Serialization/JsonConverterOfT.cs | 29 +++++++++---------- .../Json/Serialization/JsonPropertyInfoOfT.cs | 5 ++-- .../JsonSerializer.Read.HandlePropertyName.cs | 2 +- .../JsonSerializer.Write.HandleMetadata.cs | 6 ---- .../Serialization/JsonSerializerOptions.cs | 5 ++++ .../Text/Json/Serialization/ReadStack.cs | 2 +- .../Json/Serialization/ReferenceHandler.cs | 5 ++-- .../ReferenceHandlingStrategy.cs | 12 ++++++++ .../Text/Json/Serialization/WriteStack.cs | 3 +- 15 files changed, 51 insertions(+), 37 deletions(-) create mode 100644 src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs diff --git a/src/libraries/System.Text.Json/src/System.Text.Json.csproj b/src/libraries/System.Text.Json/src/System.Text.Json.csproj index 2018f5cb04093..59a986e928060 100644 --- a/src/libraries/System.Text.Json/src/System.Text.Json.csproj +++ b/src/libraries/System.Text.Json/src/System.Text.Json.csproj @@ -177,6 +177,7 @@ + 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 8f3e63ec33b04..56ffdb52e62d2 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 @@ -133,7 +133,7 @@ internal sealed override bool OnTryRead( } // Handle the metadata properties. - bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); + bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve; if (preserveReferences && state.Current.ObjectState < StackFrameObjectState.PropertyValue) { if (JsonSerializer.ResolveMetadataForJsonObject(ref reader, ref state, options)) @@ -272,7 +272,7 @@ internal sealed override bool OnTryWrite( { state.Current.ProcessedStartToken = true; writer.WriteStartObject(); - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { if (JsonSerializer.WriteReferenceForObject(this, dictionary, ref state, writer) == MetadataPropertyName.Ref) { 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 098e7aeaa9aa6..67613d90973a3 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 @@ -90,7 +90,7 @@ internal override bool OnTryRead( { // Slower path that supports continuation and preserved references. - bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); + bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve; if (state.Current.ObjectState == StackFrameObjectState.None) { if (reader.TokenType == JsonTokenType.StartArray) @@ -236,7 +236,7 @@ internal sealed override bool OnTryWrite( if (!state.Current.ProcessedStartToken) { state.Current.ProcessedStartToken = true; - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { MetadataPropertyName metadata = JsonSerializer.WriteReferenceForCollection(this, value, ref state, writer); if (metadata == MetadataPropertyName.Ref) 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 b0fcdee0d1de3..636a0037e0620 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 @@ -75,7 +75,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, // Handle the metadata properties. if (state.Current.ObjectState < StackFrameObjectState.PropertyValue) { - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { if (JsonSerializer.ResolveMetadataForJsonObject(ref reader, ref state, options)) { @@ -233,7 +233,7 @@ internal sealed override bool OnTryWrite( if (!state.SupportContinuation) { writer.WriteStartObject(); - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref) { @@ -288,7 +288,7 @@ internal sealed override bool OnTryWrite( if (!state.Current.ProcessedStartToken) { writer.WriteStartObject(); - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { if (JsonSerializer.WriteReferenceForObject(this, objectValue, ref state, writer) == MetadataPropertyName.Ref) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs index db4235bd239a0..ae7f8496a63f7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs @@ -5,7 +5,7 @@ namespace System.Text.Json.Serialization { internal sealed class IgnoreReferenceHandler : ReferenceHandler { - public IgnoreReferenceHandler() => UsePreserveSemantics = false; + public IgnoreReferenceHandler() => HandlingStrategy = ReferenceHandlingStrategy.IgnoreCycle; public override ReferenceResolver CreateResolver() => new IgnoreReferenceResolver(); } 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 259afdfb508dc..fd98064a5b69f 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 @@ -20,7 +20,7 @@ internal sealed override bool WriteCoreAsObject( } // Root object is a boxed value type, we need to push it to the reference stack before it gets unboxed here. - if (value != null && JsonSerializer.IsIgnoreCyclesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && value != null) { state.ReferenceResolver.PushReferenceForCycleDetection(value); } 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 e869c024cc141..1c485b1460d00 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 @@ -196,7 +196,8 @@ internal bool TryRead(ref Utf8JsonReader reader, Type typeToConvert, JsonSeriali ref reader); } - if (CanBePolymorphic && JsonSerializer.IsPreserveReferencesEnabled(options) && value is JsonElement element) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve && + CanBePolymorphic && value is JsonElement element) { // Edge case where we want to lookup for a reference when parsing into typeof(object) // instead of return `value` as a JsonElement. @@ -303,20 +304,17 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions ThrowHelper.ThrowJsonException_SerializerCycleDetected(options.EffectiveMaxDepth); } - bool ignoreCyclesEnabled = JsonSerializer.IsIgnoreCyclesEnabled(options); - bool ignoreCyclesPopReference = false; - - if (CanBeNull && IsNull(value)) + if (CanBeNull && !HandleNullOnWrite && IsNull(value)) { - if (!HandleNullOnWrite) - { - // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were - // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here. - writer.WriteNullValue(); - return true; - } + // We do not pass null values to converters unless HandleNullOnWrite is true. Null values for properties were + // already handled in GetMemberAndWriteJson() so we don't need to check for IgnoreNullValues here. + writer.WriteNullValue(); + return true; } - else if (ignoreCyclesEnabled && !IsValueType) + + bool ignoreCyclesPopReference = false; + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + !IsValueType && !IsNull(value)) { ReferenceResolver resolver = state.ReferenceResolver; // Write null to break reference cycles. @@ -363,9 +361,10 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); if (jsonConverter != this) { - if (ignoreCyclesEnabled && jsonConverter.IsValueType) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + jsonConverter.IsValueType) { - // For boxed value types: push the value before it gets unboxed. + // For boxed value types: push the value before it gets unboxed on TryWriteAsObject. state.ReferenceResolver.PushReferenceForCycleDetection(value); ignoreCyclesPopReference = true; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs index 8be268ab3c08a..a04a4487eca97 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs @@ -138,8 +138,9 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf { T value = Get!(obj); - if (JsonSerializer.IsIgnoreCyclesEnabled(Options) && !Converter.IsValueType && - value != null && state.ReferenceResolver.ContainsReferenceForCycleDetection(value)) + if (Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + !Converter.IsValueType && value != null && + state.ReferenceResolver.ContainsReferenceForCycleDetection(value)) { // If a reference cycle is detected, treat value as null. value = default!; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs index 5f1d91e6d6e99..f0138ebf9997d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs @@ -79,7 +79,7 @@ internal static ReadOnlySpan GetPropertyName( unescapedPropertyName = propertyName; } - if (JsonSerializer.IsPreserveReferencesEnabled(options)) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve) { if (propertyName.Length > 0 && propertyName[0] == '$') { 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 7732c2f703a7f..45c1462ea68f3 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 @@ -84,11 +84,5 @@ internal static MetadataPropertyName WriteReferenceForCollection( return writtenMetadataName; } - - internal static bool IsPreserveReferencesEnabled(JsonSerializerOptions options) - => options.ReferenceHandler?.UsePreserveSemantics ?? false; - - internal static bool IsIgnoreCyclesEnabled(JsonSerializerOptions options) - => !options.ReferenceHandler?.UsePreserveSemantics ?? false; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index 8ade08277fae7..411f37139860d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -89,6 +89,7 @@ public JsonSerializerOptions(JsonSerializerOptions options) Converters = new ConverterList(this, (ConverterList)options.Converters); EffectiveMaxDepth = options.EffectiveMaxDepth; + ReferenceHandlingStrategy = options.ReferenceHandlingStrategy; // _classes is not copied as sharing the JsonClassInfo and JsonPropertyInfo caches can result in // unnecessary references to type metadata, potentially hindering garbage collection on the source options. @@ -487,9 +488,13 @@ public ReferenceHandler? ReferenceHandler { VerifyMutable(); _referenceHandler = value; + ReferenceHandlingStrategy = value?.HandlingStrategy ?? ReferenceHandlingStrategy.None; } } + // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycle semanitcs or None of them. + internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None; + internal MemberAccessor MemberAccessorStrategy { get diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs index daa94277aa0d2..c74e84b4ee020 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStack.cs @@ -89,7 +89,7 @@ public void Initialize(Type type, JsonSerializerOptions options, bool supportCon Current.NumberHandling = Current.JsonPropertyInfo.NumberHandling; - bool preserveReferences = JsonSerializer.IsPreserveReferencesEnabled(options); + bool preserveReferences = options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.Preserve; if (preserveReferences) { ReferenceResolver = options.ReferenceHandler!.CreateResolver(writing: false); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs index 71f8fdf6d965b..72cf6cb088b2e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs @@ -11,9 +11,10 @@ namespace System.Text.Json.Serialization public abstract class ReferenceHandler { /// - /// Whether we read and write preserve references or we ignore references when writing. + /// One of the enumeration values that indicates whether this ReferenceHandler implementation should use Preserve semantics or IgnoreCycle semantics. + /// The defualt is Preserve. /// - internal bool UsePreserveSemantics = true; + internal ReferenceHandlingStrategy HandlingStrategy = ReferenceHandlingStrategy.Preserve; /// /// Metadata properties will be honored when deserializing JSON objects and arrays into reference types and written when serializing reference types. This is necessary to create round-trippable JSON from objects that contain cycles or duplicate references. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs new file mode 100644 index 0000000000000..5d4f46d5b177f --- /dev/null +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs @@ -0,0 +1,12 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +namespace System.Text.Json.Serialization +{ + internal enum ReferenceHandlingStrategy + { + None, + Preserve, + IgnoreCycle + } +} 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 f5bc5c41e191f..7f231046fc036 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 @@ -73,8 +73,9 @@ public JsonConverter Initialize(Type type, JsonSerializerOptions options, bool s Current.DeclaredJsonPropertyInfo = jsonClassInfo.PropertyInfoForClassInfo; Current.NumberHandling = Current.DeclaredJsonPropertyInfo.NumberHandling; - if (options.ReferenceHandler != null) + if (options.ReferenceHandlingStrategy != ReferenceHandlingStrategy.None) { + Debug.Assert(options.ReferenceHandler != null); ReferenceResolver = options.ReferenceHandler.CreateResolver(writing: true); } From aafaf2f5ecaa4f6086a7138d49e3a246c298a4fa Mon Sep 17 00:00:00 2001 From: David Cantu Date: Wed, 10 Feb 2021 13:56:37 -0800 Subject: [PATCH 6/9] Address suggestions --- .../Serialization/IgnoreReferenceResolver.cs | 8 +- .../Json/Serialization/JsonConverterOfT.cs | 3 +- .../Json/Serialization/ReferenceHandler.cs | 2 +- .../ReferenceHandlerTests.IgnoreCycle.cs | 248 +++++++++++++----- 4 files changed, 190 insertions(+), 71 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs index f38ce69f0220a..53a13a2b66c91 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceResolver.cs @@ -13,10 +13,8 @@ internal sealed class IgnoreReferenceResolver : ReferenceResolver internal override void PopReferenceForCycleDetection() { - if (_stackForCycleDetection != null) - { - _stackForCycleDetection.Pop(); - } + Debug.Assert(_stackForCycleDetection != null); + _stackForCycleDetection.Pop(); } internal override bool ContainsReferenceForCycleDetection(object value) @@ -36,7 +34,9 @@ internal override void PushReferenceForCycleDetection(object value) } public override void AddReference(string referenceId, object value) => throw new InvalidOperationException(); + public override string GetReference(object value, out bool alreadyExists) => throw new InvalidOperationException(); + public override object ResolveReference(string referenceId) => throw new InvalidOperationException(); } } 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 1c485b1460d00..0683c7e2703e4 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 @@ -323,7 +323,8 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions writer.WriteNullValue(); return true; } - // For boxed reference types: do not push when boxed in order to avoid false positives when we run above check for the converter of the unboxed value. + // For boxed reference types: do not push when boxed in order to avoid false positives + // when we run the ContainsReferenceForCycleDetection check for the converter of the unboxed value. if (!CanBePolymorphic) { resolver.PushReferenceForCycleDetection(value); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs index 72cf6cb088b2e..56db3c4aaed1a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs @@ -11,7 +11,7 @@ namespace System.Text.Json.Serialization public abstract class ReferenceHandler { /// - /// One of the enumeration values that indicates whether this ReferenceHandler implementation should use Preserve semantics or IgnoreCycle semantics. + /// Indicates whether this ReferenceHandler implementation should use semantics or semantics. /// The defualt is Preserve. /// internal ReferenceHandlingStrategy HandlingStrategy = ReferenceHandlingStrategy.Preserve; diff --git a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs index 075105bdd01e9..7b766f1eb89b4 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs @@ -19,20 +19,101 @@ public class ReferenceHandlerTests_IgnoreCycle [Fact] public async Task IgnoreCycles_OnObject() { - var root = new Node(); + await Verify(); + await Verify(); + + async Task Verify() where T : class, new() + { + T root = new T(); + SetNextProperty(typeof(T), root, root); + + await Test_Serialize_And_SerializeAsync(root, @"{""Next"":null}", s_optionsIgnoreCycles); + + // Verify that object property is not mutated on serialization. + object rootNext = GetNextProperty(typeof(T), root); + Assert.NotNull(rootNext); + Assert.Same(rootNext, root); + } + } + + [Fact] + public async Task IgnoreCycles_OnObject_AsProperty() + { + await Verify(); + await Verify(); + + async Task Verify() where T : class, new() + { + var node = new T(); + SetNextProperty(typeof(T), node, node); + + var root = new ClassWithGenericProperty(); + root.Foo = node; + await Test_Serialize_And_SerializeAsync(root, expected: @"{""Foo"":{""Next"":null}}", s_optionsIgnoreCycles); + + object nodeNext = GetNextProperty(typeof(T), node); + Assert.NotNull(nodeNext); + Assert.Same(nodeNext, node); + + var rootWithObjProperty = new ClassWithGenericProperty(); + rootWithObjProperty.Foo = node; + await Test_Serialize_And_SerializeAsync(rootWithObjProperty, expected: @"{""Foo"":{""Next"":null}}", s_optionsIgnoreCycles); + + nodeNext = GetNextProperty(typeof(T), node); + Assert.NotNull(nodeNext); + Assert.Same(nodeNext, node); + } + } + + [Fact] + public async Task IgnoreCycles_OnBoxedValueType() + { + await Verify(); + await Verify(); + + async Task Verify() where T : new() + { + object root = new T(); + SetNextProperty(typeof(T), root, root); + await Test_Serialize_And_SerializeAsync(root, expected: @"{""Next"":null}", s_optionsIgnoreCycles); + + object rootNext = GetNextProperty(typeof(T), root); + Assert.NotNull(rootNext); + Assert.Same(rootNext, root); + } + } + + [Fact] + public async Task IgnoreCycles_OnBoxedValueType_Interface() + { + IValueNodeWithIValueNodeProperty root = new ValueNodeWithIValueNodeProperty(); root.Next = root; + await Test_Serialize_And_SerializeAsync(root, expected: @"{""Next"":null}", s_optionsIgnoreCycles); - await Test_Serialize_And_SerializeAsync(root, @"{""Next"":null}", s_optionsIgnoreCycles); + IValueNodeWithObjectProperty root2 = new ValueNodeWithObjectProperty(); + root2.Next = root2; + await Test_Serialize_And_SerializeAsync(root2, expected: @"{""Next"":null}", s_optionsIgnoreCycles); } [Fact] - public async Task IgnoreCycles_OnObject_NonProperty() + public async Task IgnoreCycles_OnBoxedValueType_AsProperty() { - var node = new Node(); - var rootList = new List() { node }; - node.Next = rootList; + await Verify(); + await Verify(); + + async Task Verify() where T : new() + { + object node = new T(); + SetNextProperty(typeof(T), node, node); + + var rootWithObjProperty = new ClassWithGenericProperty(); + rootWithObjProperty.Foo = node; + await Test_Serialize_And_SerializeAsync(rootWithObjProperty, expected: @"{""Foo"":{""Next"":null}}", s_optionsIgnoreCycles); - await Test_Serialize_And_SerializeAsync(rootList, @"[{""Next"":null}]", s_optionsIgnoreCycles); + object nodeNext = GetNextProperty(typeof(T), node); + Assert.NotNull(nodeNext); + Assert.Same(nodeNext, node); + } } [Theory] @@ -40,8 +121,17 @@ public async Task IgnoreCycles_OnObject_NonProperty() [InlineData(typeof(GenericIDictionaryWrapper))] public async Task IgnoreCycles_OnDictionary(Type typeToSerialize) { - var root = (ICollection>)Activator.CreateInstance(typeToSerialize); - root.Add(new KeyValuePair("self", root)); + var root = (IDictionary)Activator.CreateInstance(typeToSerialize); + root.Add("self", root); + + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); + } + + [Fact] + public async Task IgnoreCycles_OnRecursiveDictionary() + { + var root = new RecursiveDictionary(); + root.Add("self", root); await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); } @@ -51,8 +141,8 @@ public async Task IgnoreCycles_OnReadOnlyDictionary() { var innerDictionary = new Dictionary(); var root = new ReadOnlyDictionary(innerDictionary); - innerDictionary.Add("self", root); + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); } @@ -61,6 +151,7 @@ public async Task IgnoreCycles_OnIDictionary() { var root = new WrapperForIDictionary(); root.Add("self", root); + await Test_Serialize_And_SerializeAsync(root, @"{""self"":null}", s_optionsIgnoreCycles); } @@ -75,13 +166,21 @@ public async Task IgnoreCycles_OnArray() [Theory] [InlineData(typeof(List))] [InlineData(typeof(GenericIListWrapper))] - public async Task IgnoreCycles_OnLists(Type typeToSerialize) + public async Task IgnoreCycles_OnList(Type typeToSerialize) { var root = (IList)Activator.CreateInstance(typeToSerialize); root.Add(root); await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); } + [Fact] + public async Task IgnoreCycles_OnRecursiveList() + { + var root = new RecursiveList(); + root.Add(root); + await Test_Serialize_And_SerializeAsync(root, "[null]", s_optionsIgnoreCycles); + } + [Theory] [InlineData(typeof(GenericISetWrapper))] [InlineData(typeof(GenericICollectionWrapper))] @@ -128,27 +227,6 @@ public async Task IgnoreCycles_OnExtensionData() await Test_Serialize_And_SerializeAsync(root, @"{""root"":null}", s_optionsIgnoreCycles); } - [Fact] - public async Task IgnoreCycles_OnBoxedValueTypes() - { - IValueNode root = new ValueNode(); - root.Next = root; - - await Test_Serialize_And_SerializeAsync(root, @"{""Next"":null}", s_optionsIgnoreCycles); - } - - [Fact] - public async Task IgnoreCycles_OnBoxedValueTypes_AsProperty() - { - IValueNode node = new ValueNode(); - node.Next = node; - - var root = new Node(); - root.Next = node; - - await Test_Serialize_And_SerializeAsync(root, @"{""Next"":{""Next"":null}}", s_optionsIgnoreCycles); - } - [Fact] public async Task IgnoreCycles_DoesNotSupportPreserveSemantics() { @@ -189,16 +267,16 @@ public async Task IgnoreCycles_DoesNotSupportPreserveSemantics() public async Task IgnoreCycles_DoesNotSupportPreserveSemantics_Polymorphic() { // Object - var node = new Node(); + var node = new NodeWithObjectProperty(); node.Next = node; string json = SerializeWithPreserve(node); - node = JsonSerializer.Deserialize(json, s_optionsIgnoreCycles); + node = JsonSerializer.Deserialize(json, s_optionsIgnoreCycles); JsonElement nodeAsJsonElement = Assert.IsType(node.Next); Assert.True(nodeAsJsonElement.GetProperty("$ref").GetString() == "1"); using var ms = new MemoryStream(Encoding.UTF8.GetBytes(json)); - node = await JsonSerializer.DeserializeAsync(ms, s_optionsIgnoreCycles); + node = await JsonSerializer.DeserializeAsync(ms, s_optionsIgnoreCycles); nodeAsJsonElement = Assert.IsType(node.Next); Assert.True(nodeAsJsonElement.GetProperty("$ref").GetString() == "1"); @@ -216,21 +294,24 @@ private string SerializeWithPreserve(T value) return JsonSerializer.Serialize(value, opts); } - // Test for correctness when the object reference is found on a sibling branch. - [Theory] - [InlineData(typeof(EmptyClass), "{}")] - [InlineData(typeof(EmptyStruct), "{}")] - [InlineData(typeof(object), "{}")] - [InlineData(typeof(Dictionary), "{}")] - [InlineData(typeof(List), "[]")] - public async Task AlreadySeenInstance_ShouldNotBeIgnoredOnSecondBranch(Type objectType, string objectPayload) + [Fact] + public async Task AlreadySeenInstance_ShouldNotBeIgnoredOnSiblingBranch() { - object obj = Activator.CreateInstance(objectType); - var root = new TreeNode(); - root.Left = obj; - root.Right = obj; + await Verify(expectedPayload: "{}"); + await Verify(expectedPayload: "{}"); + await Verify(expectedPayload: "{}"); + await Verify>(expectedPayload: "{}"); + await Verify>(expectedPayload: "[]"); + + async Task Verify(string expectedPayload) where T : new() + { + T value = new(); + var root = new TreeNode { Left = value, Right = value }; + await Test_Serialize_And_SerializeAsync(root, $@"{{""Left"":{expectedPayload},""Right"":{expectedPayload}}}", s_optionsIgnoreCycles); - await Test_Serialize_And_SerializeAsync(root, $@"{{""Left"":{objectPayload},""Right"":{objectPayload}}}", s_optionsIgnoreCycles); + var rootWithObjectProperties = new TreeNode { Left = value, Right = value }; + await Test_Serialize_And_SerializeAsync(rootWithObjectProperties, $@"{{""Left"":{expectedPayload},""Right"":{expectedPayload}}}", s_optionsIgnoreCycles); + } } [Fact] @@ -243,7 +324,7 @@ public async Task IgnoreCycles_WhenWritingNull() }; // Reference cycles are treated as null, hence the JsonIgnoreCondition can be used to actually ignore the property. - var rootObj = new Node(); + var rootObj = new NodeWithObjectProperty(); rootObj.Next = rootObj; await Test_Serialize_And_SerializeAsync(rootObj, "{}", opts); @@ -261,46 +342,83 @@ public async Task IgnoreCycles_WhenWritingNull() await Test_Serialize_And_SerializeAsync(rootDictionary, @"{""self"":null}", opts); } - private async Task Test_Serialize_And_SerializeAsync(object obj, string expected, JsonSerializerOptions options) + private async Task Test_Serialize_And_SerializeAsync(T obj, string expected, JsonSerializerOptions options) { - string json = JsonSerializer.Serialize(obj, options); + string json; + Type objType = typeof(T); + + if (objType != typeof(object)) + { + json = JsonSerializer.Serialize(obj, options); + Assert.Equal(expected, json); + + using var ms1 = new MemoryStream(); + await JsonSerializer.SerializeAsync(ms1, obj, options).ConfigureAwait(false); + json = Encoding.UTF8.GetString(ms1.ToArray()); + Assert.Equal(expected, json); + } + + json = JsonSerializer.Serialize(obj, objType, options); Assert.Equal(expected, json); - using var ms = new MemoryStream(); - await JsonSerializer.SerializeAsync(ms, obj, options).ConfigureAwait(false); - json = Encoding.UTF8.GetString(ms.ToArray()); + using var ms2 = new MemoryStream(); + await JsonSerializer.SerializeAsync(ms2, obj, objType, options).ConfigureAwait(false); + json = Encoding.UTF8.GetString(ms2.ToArray()); Assert.Equal(expected, json); } - private class Employee + private const string Next = nameof(Next); + private void SetNextProperty(Type type, object obj, object value) { - public string Name { get; set; } - public Employee Manager { get; set; } - public List Subordinates { get; set; } - public Dictionary Colleagues { get; set; } + type.GetProperty(Next).SetValue(obj, value); } - private class Node + private object GetNextProperty(Type type, object obj) + { + return type.GetProperty(Next).GetValue(obj); + } + + private class NodeWithObjectProperty { public object Next { get; set; } } - private class TreeNode + private class NodeWithNodeProperty + { + public NodeWithNodeProperty Next { get; set; } + } + + private class ClassWithGenericProperty { - public object Left { get; set; } - public object Right { get; set; } + public T Foo { get; set; } } - private struct ValueNode : IValueNode + private class TreeNode + { + public T Left { get; set; } + public T Right { get; set; } + } + + interface IValueNodeWithObjectProperty { public object Next { get; set; } } - interface IValueNode + private struct ValueNodeWithObjectProperty : IValueNodeWithObjectProperty { public object Next { get; set; } } + interface IValueNodeWithIValueNodeProperty + { + public IValueNodeWithIValueNodeProperty Next { get; set; } + } + + private struct ValueNodeWithIValueNodeProperty : IValueNodeWithIValueNodeProperty + { + public IValueNodeWithIValueNodeProperty Next { get; set; } + } + private class EmptyClass { } private struct EmptyStruct { } From b56257939f3fed3cdcd4a86f46cd38dcd0dd90c7 Mon Sep 17 00:00:00 2001 From: David Cantu Date: Wed, 10 Feb 2021 14:00:44 -0800 Subject: [PATCH 7/9] Rename API to IgnoreCycles --- src/libraries/System.Text.Json/ref/System.Text.Json.cs | 2 +- .../System/Text/Json/Serialization/IgnoreReferenceHandler.cs | 2 +- .../Text/Json/Serialization/JsonConverterOfT.WriteCore.cs | 2 +- .../src/System/Text/Json/Serialization/JsonConverterOfT.cs | 4 ++-- .../src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs | 2 +- .../System/Text/Json/Serialization/JsonSerializerOptions.cs | 2 +- .../src/System/Text/Json/Serialization/ReferenceHandler.cs | 4 ++-- .../Text/Json/Serialization/ReferenceHandlingStrategy.cs | 2 +- ...s.IgnoreCycle.cs => ReferenceHandlerTests.IgnoreCycles.cs} | 4 ++-- .../System.Text.Json/tests/System.Text.Json.Tests.csproj | 2 +- 10 files changed, 13 insertions(+), 13 deletions(-) rename src/libraries/System.Text.Json/tests/Serialization/{ReferenceHandlerTests.IgnoreCycle.cs => ReferenceHandlerTests.IgnoreCycles.cs} (99%) diff --git a/src/libraries/System.Text.Json/ref/System.Text.Json.cs b/src/libraries/System.Text.Json/ref/System.Text.Json.cs index 1829e653ca971..2ab1dabba10b5 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -559,7 +559,7 @@ public abstract partial class ReferenceHandler { protected ReferenceHandler() { } public static System.Text.Json.Serialization.ReferenceHandler Preserve { get { throw null; } } - public static System.Text.Json.Serialization.ReferenceHandler IgnoreCycle { get { throw null; } } + public static System.Text.Json.Serialization.ReferenceHandler IgnoreCycles { get { throw null; } } public abstract System.Text.Json.Serialization.ReferenceResolver CreateResolver(); } public sealed partial class ReferenceHandler : System.Text.Json.Serialization.ReferenceHandler where T : System.Text.Json.Serialization.ReferenceResolver, new() diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs index ae7f8496a63f7..23f174c831ee6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/IgnoreReferenceHandler.cs @@ -5,7 +5,7 @@ namespace System.Text.Json.Serialization { internal sealed class IgnoreReferenceHandler : ReferenceHandler { - public IgnoreReferenceHandler() => HandlingStrategy = ReferenceHandlingStrategy.IgnoreCycle; + public IgnoreReferenceHandler() => HandlingStrategy = ReferenceHandlingStrategy.IgnoreCycles; public override ReferenceResolver CreateResolver() => new IgnoreReferenceResolver(); } 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 fd98064a5b69f..1d60dbf4e38c3 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 @@ -20,7 +20,7 @@ internal sealed override bool WriteCoreAsObject( } // Root object is a boxed value type, we need to push it to the reference stack before it gets unboxed here. - if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && value != null) + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && value != null) { state.ReferenceResolver.PushReferenceForCycleDetection(value); } 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 0683c7e2703e4..8674937c0855e 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 @@ -313,7 +313,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions } bool ignoreCyclesPopReference = false; - if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && !IsValueType && !IsNull(value)) { ReferenceResolver resolver = state.ReferenceResolver; @@ -362,7 +362,7 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions JsonConverter jsonConverter = state.Current.InitializeReEntry(type, options); if (jsonConverter != this) { - if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && jsonConverter.IsValueType) { // For boxed value types: push the value before it gets unboxed on TryWriteAsObject. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs index a04a4487eca97..075832706db1f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoOfT.cs @@ -138,7 +138,7 @@ public override bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf { T value = Get!(obj); - if (Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycle && + if (Options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && !Converter.IsValueType && value != null && state.ReferenceResolver.ContainsReferenceForCycleDetection(value)) { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs index 411f37139860d..e832865f9518a 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs @@ -492,7 +492,7 @@ public ReferenceHandler? ReferenceHandler } } - // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycle semanitcs or None of them. + // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycles semanitcs or None of them. internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None; internal MemberAccessor MemberAccessorStrategy diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs index 56db3c4aaed1a..ff88d0adb7c88 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandler.cs @@ -11,7 +11,7 @@ namespace System.Text.Json.Serialization public abstract class ReferenceHandler { /// - /// Indicates whether this ReferenceHandler implementation should use semantics or semantics. + /// Indicates whether this ReferenceHandler implementation should use semantics or semantics. /// The defualt is Preserve. /// internal ReferenceHandlingStrategy HandlingStrategy = ReferenceHandlingStrategy.Preserve; @@ -48,7 +48,7 @@ public abstract class ReferenceHandler /// /// Ignores an object when a reference cycle is detected during serialization. /// - public static ReferenceHandler IgnoreCycle { get; } = new IgnoreReferenceHandler(); + public static ReferenceHandler IgnoreCycles { get; } = new IgnoreReferenceHandler(); /// /// Returns the used for each serialization call. diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs index 5d4f46d5b177f..a99390da2c7d0 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReferenceHandlingStrategy.cs @@ -7,6 +7,6 @@ internal enum ReferenceHandlingStrategy { None, Preserve, - IgnoreCycle + IgnoreCycles } } diff --git a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs similarity index 99% rename from src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs rename to src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs index 7b766f1eb89b4..aa16d188039d5 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycle.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs @@ -14,7 +14,7 @@ namespace System.Text.Json.Serialization.Tests public class ReferenceHandlerTests_IgnoreCycle { private static readonly JsonSerializerOptions s_optionsIgnoreCycles = - new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.IgnoreCycle }; + new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.IgnoreCycles }; [Fact] public async Task IgnoreCycles_OnObject() @@ -319,7 +319,7 @@ public async Task IgnoreCycles_WhenWritingNull() { var opts = new JsonSerializerOptions { - ReferenceHandler = ReferenceHandler.IgnoreCycle, + ReferenceHandler = ReferenceHandler.IgnoreCycles, DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull }; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj index a99a92f92eb17..dbebf9b1814f3 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests.csproj @@ -117,7 +117,7 @@ - + From 216caaed23f9e41cdda0702c6f42638d2f413acc Mon Sep 17 00:00:00 2001 From: David Cantu Date: Wed, 10 Feb 2021 15:11:38 -0800 Subject: [PATCH 8/9] Add missing letter to test class name --- .../tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs index aa16d188039d5..c7de09f8199f3 100644 --- a/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs +++ b/src/libraries/System.Text.Json/tests/Serialization/ReferenceHandlerTests.IgnoreCycles.cs @@ -11,7 +11,7 @@ namespace System.Text.Json.Serialization.Tests { - public class ReferenceHandlerTests_IgnoreCycle + public class ReferenceHandlerTests_IgnoreCycles { private static readonly JsonSerializerOptions s_optionsIgnoreCycles = new JsonSerializerOptions { ReferenceHandler = ReferenceHandler.IgnoreCycles }; From 75ef177d53f3b78bdaa8fdf29057249ccfc9849d Mon Sep 17 00:00:00 2001 From: David Cantu Date: Thu, 11 Feb 2021 14:53:03 -0800 Subject: [PATCH 9/9] Fix CI issue with nullable annotation --- .../src/System/Text/Json/Serialization/JsonConverterOfT.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) 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 8674937c0855e..b55bc531c8cd8 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 @@ -316,13 +316,16 @@ internal bool TryWrite(Utf8JsonWriter writer, in T value, JsonSerializerOptions if (options.ReferenceHandlingStrategy == ReferenceHandlingStrategy.IgnoreCycles && !IsValueType && !IsNull(value)) { + Debug.Assert(value != null); ReferenceResolver resolver = state.ReferenceResolver; + // Write null to break reference cycles. - if (resolver.ContainsReferenceForCycleDetection(value!)) + if (resolver.ContainsReferenceForCycleDetection(value)) { writer.WriteNullValue(); return true; } + // For boxed reference types: do not push when boxed in order to avoid false positives // when we run the ContainsReferenceForCycleDetection check for the converter of the unboxed value. if (!CanBePolymorphic)