From 055ce33efe30e6c5a3f76662449a4d3db057681a Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 31 May 2022 15:58:02 +0100 Subject: [PATCH] Minor refactorings (#1) * Makes the following changes: * Move singleton initialization for DefaultTypeInfoResolver behind a static property. * Consolidate JsonSerializerContext & IJsonTypeInfoResolver values to a single field. * Move reflection fallback logic away from JsonSerializerContext and into JsonSerializerOptions * Update src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs * remove testing of removed field --- .../System.Text.Json/ref/System.Text.Json.cs | 1 + .../Json/Serialization/ConfigurationList.cs | 39 ++--- .../JsonMetadataServicesConverter.cs | 2 +- .../Serialization/JsonSerializer.Helpers.cs | 5 +- .../JsonSerializer.Read.Stream.cs | 5 +- .../JsonSerializer.Write.Helpers.cs | 6 +- .../Serialization/JsonSerializerContext.cs | 18 +- .../JsonSerializerOptions.Caching.cs | 46 +---- .../JsonSerializerOptions.Converters.cs | 32 +--- .../Serialization/JsonSerializerOptions.cs | 163 +++++++++--------- .../Metadata/DefaultJsonTypeInfoResolver.cs | 47 +++-- .../Serialization/Metadata/JsonTypeInfo.cs | 4 +- .../Serialization/Metadata/JsonTypeInfoOfT.cs | 1 + .../Metadata/JsonTypeInfoResolver.cs | 6 +- .../Metadata/SourceGenJsonTypeInfoOfT.cs | 6 +- .../JsonSerializerContextTests.cs | 4 - .../JsonSerializerWrapper.Reflection.cs | 18 +- .../MetadataTests/MetadataTests.Options.cs | 6 +- .../Serialization/OptionsTests.cs | 5 +- 19 files changed, 179 insertions(+), 235 deletions(-) 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 a67ca4203917a..07e3cdf07c243 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -1215,6 +1215,7 @@ public abstract partial class JsonTypeInfo : System.Text.Json.Serialization.M { internal JsonTypeInfo() { } public new System.Func? CreateObject { get { throw null; } set { } } + [System.ComponentModel.EditorBrowsableAttribute(System.ComponentModel.EditorBrowsableState.Never)] public System.Action? SerializeHandler { get { throw null; } } } public enum JsonTypeInfoKind diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs index a99ee51784530..5199a93bba29c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ConfigurationList.cs @@ -4,29 +4,25 @@ using System.Collections; using System.Collections.Generic; using System.Diagnostics; +using System.Globalization; namespace System.Text.Json.Serialization { /// - /// A list of configuration items that respects the options class being immutable once (de)serialization occurs. + /// A list of configuration items that can be locked for modification /// - internal sealed class ConfigurationList : IList + internal abstract class ConfigurationList : IList { private readonly List _list; - public Action? OnElementAdded { get; set; } - public Func? IsReadOnlyFunc { get; set; } - public Action? ThrowImmutableFunc { get; set; } - - public ConfigurationList() + public ConfigurationList(IList? source = null) { - _list = new List(); + _list = source is null ? new List() : new List(source); } - public ConfigurationList(IList source) - { - _list = new List(source is ConfigurationList cl ? cl._list : source); - } + protected abstract bool IsLockedInstance { get; } + protected abstract void VerifyMutable(); + protected virtual void OnItemAdded(TItem item) { } public TItem this[int index] { @@ -43,24 +39,13 @@ public TItem this[int index] VerifyMutable(); _list[index] = value; - OnElementAdded?.Invoke(value); + OnItemAdded(value); } } public int Count => _list.Count; - public bool IsReadOnly => IsReadOnlyFunc != null ? IsReadOnlyFunc() : false; - - private void VerifyMutable() - { - Debug.Assert((IsReadOnlyFunc == null) == (ThrowImmutableFunc == null), "IsReadOnlyFunc and ThrowImmutableFunc should be either both set or both unset"); - - if (IsReadOnlyFunc != null && IsReadOnlyFunc()) - { - Debug.Assert(ThrowImmutableFunc != null); - ThrowImmutableFunc(); - } - } + public bool IsReadOnly => IsLockedInstance; public void Add(TItem item) { @@ -71,7 +56,7 @@ public void Add(TItem item) VerifyMutable(); _list.Add(item); - OnElementAdded?.Invoke(item); + OnItemAdded(item); } public void Clear() @@ -109,7 +94,7 @@ public void Insert(int index, TItem item) VerifyMutable(); _list.Insert(index, item); - OnElementAdded?.Invoke(item); + OnItemAdded(item); } public bool Remove(TItem item) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs index b597c4d16da6a..a0efc0a884331 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/JsonMetadataServicesConverter.cs @@ -67,7 +67,7 @@ internal override bool OnTryWrite(Utf8JsonWriter writer, T value, JsonSerializer jsonTypeInfo is JsonTypeInfo info && info.SerializeHandler != null && !state.CurrentContainsMetadata && // Do not use the fast path if state needs to write metadata. - info.Options.JsonSerializerContext?.CanUseSerializationLogic == true) + info.Options.SerializerContext?.CanUseSerializationLogic == true) { info.SerializeHandler(writer, value); return true; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs index 182e1a7701dbf..7e56f52f29ab9 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Helpers.cs @@ -20,7 +20,10 @@ private static JsonTypeInfo GetTypeInfo(JsonSerializerOptions? options, Type run Debug.Assert(runtimeType != null); options ??= JsonSerializerOptions.Default; - options.EnsureInitializedForReflectionSerializer(); + if (!options.IsInitializedForReflectionSerializer) + { + options.InitializeForReflectionSerializer(); + } return options.GetOrAddJsonTypeInfoForRootType(runtimeType); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs index ce80791cde4d1..98a91bff74e4b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs @@ -369,7 +369,10 @@ public static partial class JsonSerializer } options ??= JsonSerializerOptions.Default; - options.EnsureInitializedForReflectionSerializer(); + if (!options.IsInitializedForReflectionSerializer) + { + options.InitializeForReflectionSerializer(); + } JsonTypeInfo jsonTypeInfo = options.GetOrAddJsonTypeInfoForRootType(typeof(TValue)); return CreateAsyncEnumerableDeserializer(utf8Json, jsonTypeInfo, cancellationToken); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs index c824aa5656bda..42da3e131bd4d 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs @@ -41,7 +41,7 @@ private static void WriteUsingGeneratedSerializer(Utf8JsonWriter writer, if (jsonTypeInfo.HasSerialize && jsonTypeInfo is JsonTypeInfo typedInfo && - typedInfo.Options.JsonSerializerContext?.CanUseSerializationLogic == true) + typedInfo.Options.SerializerContext?.CanUseSerializationLogic == true) { Debug.Assert(typedInfo.SerializeHandler != null); typedInfo.SerializeHandler(writer, value); @@ -59,8 +59,8 @@ private static void WriteUsingSerializer(Utf8JsonWriter writer, in TValu Debug.Assert(!jsonTypeInfo.HasSerialize || jsonTypeInfo is not JsonTypeInfo || - jsonTypeInfo.Options.JsonSerializerContext == null || - !jsonTypeInfo.Options.JsonSerializerContext.CanUseSerializationLogic, + jsonTypeInfo.Options.SerializerContext == null || + !jsonTypeInfo.Options.SerializerContext.CanUseSerializationLogic, "Incorrect method called. WriteUsingGeneratedSerializer() should have been called instead."); WriteStack state = default; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs index c3071a1c37d58..a9f6c90714020 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerContext.cs @@ -20,12 +20,9 @@ public abstract partial class JsonSerializerContext : IJsonTypeInfoResolver /// when instanciating the context, then a new instance is bound and returned. /// /// - /// The instance cannot be mutated once it is bound with the context instance. + /// The instance cannot be mutated once it is bound to the context instance. /// - public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { JsonSerializerContext = this }; - - // Currently JsonSerializerContext starts falling back to DefaultJsonTypeInfoResolver when some methods are called. - internal IJsonTypeInfoResolver? FallbackResolver { get; set; } + public JsonSerializerOptions Options => _options ??= new JsonSerializerOptions { TypeInfoResolver = this }; /// /// Indicates whether pre-generated serialization logic for types in the context @@ -87,8 +84,7 @@ protected JsonSerializerContext(JsonSerializerOptions? options) { if (options != null) { - options.JsonSerializerContext = this; - _options = options; + options.TypeInfoResolver = this; } } @@ -101,15 +97,13 @@ protected JsonSerializerContext(JsonSerializerOptions? options) JsonTypeInfo? IJsonTypeInfoResolver.GetTypeInfo(Type type, JsonSerializerOptions options) { - if (_options != options) + if (options != null && _options != options) { - Debug.Assert(_options != null, "_options are null"); + // TODO is this the appropriate exception message to throw? ThrowHelper.ThrowInvalidOperationException_SerializerContextOptionsImmutable(); } - JsonTypeInfo? typeInfo = GetTypeInfo(type); - typeInfo ??= FallbackResolver?.GetTypeInfo(type, options); - return typeInfo; + return GetTypeInfo(type); } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs index 5fffd5035e4b8..bd8b8a72c33ba 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Caching.cs @@ -28,7 +28,6 @@ internal JsonTypeInfo GetOrAddJsonTypeInfo(Type type) if (_cachingContext == null) { InitializeCachingContext(); - Debug.Assert(_cachingContext != null); } return _cachingContext.GetOrAddJsonTypeInfo(type); @@ -71,15 +70,11 @@ internal void ClearCaches() _lastTypeInfo = null; } + [MemberNotNull(nameof(_cachingContext))] private void InitializeCachingContext() { + _isLockedInstance = true; _cachingContext = TrackedCachingContexts.GetOrCreate(this); - if (_typeInfoResolver != null) - { - Debug.Assert(_cachingContext.Options._typeInfoResolver != null || _typeInfoResolver == s_defaultTypeInfoResolver, - "EqualityComparer should guarantee that null is equivalent to s_defaultTypeInfoResolver"); - _cachingContext.Options._typeInfoResolver ??= s_defaultTypeInfoResolver; - } } /// @@ -131,6 +126,7 @@ internal static class TrackedCachingContexts public static CachingContext GetOrCreate(JsonSerializerOptions options) { + Debug.Assert(options._isLockedInstance, "Cannot create caching contexts for mutable JsonSerializerOptions instances"); ConcurrentDictionary> cache = s_cache; if (cache.TryGetValue(options, out WeakReference? wr) && wr.TryGetTarget(out CachingContext? ctx)) @@ -169,7 +165,6 @@ public static CachingContext GetOrCreate(JsonSerializerOptions options) { // Copy fields ignored by the copy constructor // but are necessary to determine equivalence. - _serializerContext = options._serializerContext, _typeInfoResolver = options._typeInfoResolver, }; Debug.Assert(key._cachingContext == null); @@ -273,25 +268,6 @@ public bool Equals(JsonSerializerOptions? left, JsonSerializerOptions? right) { Debug.Assert(left != null && right != null); - // We cache these values in this specific order enforcing default resolver to be read last - // this is done in case _typeInfoResolver or s_defaultTypeInfoResolver is not initialized yet - // the behavior will still be correct - IJsonTypeInfoResolver? leftResolver = left._typeInfoResolver; - IJsonTypeInfoResolver? rightResolver = right._typeInfoResolver; - IJsonTypeInfoResolver? defaultResolver = Volatile.Read(ref s_defaultTypeInfoResolver); - - // we ensure null and s_defaultTypeInfoResolver mean the same thing - // we normalize to null so that calculating hash code is consistent - if (leftResolver == defaultResolver) - { - leftResolver = null; - } - - if (rightResolver == defaultResolver) - { - rightResolver = null; - } - return left._dictionaryKeyPolicy == right._dictionaryKeyPolicy && left._jsonPropertyNamingPolicy == right._jsonPropertyNamingPolicy && @@ -310,8 +286,7 @@ public bool Equals(JsonSerializerOptions? left, JsonSerializerOptions? right) left._includeFields == right._includeFields && left._propertyNameCaseInsensitive == right._propertyNameCaseInsensitive && left._writeIndented == right._writeIndented && - leftResolver == rightResolver && - left._serializerContext == right._serializerContext && + left._typeInfoResolver == right._typeInfoResolver && CompareLists(left._converters, right._converters) && #pragma warning disable CA2252 // This API requires opting into preview features CompareLists(left._polymorphicTypeConfigurations, right._polymorphicTypeConfigurations); @@ -339,16 +314,6 @@ static bool CompareLists(ConfigurationList left, ConfigurationLi public int GetHashCode(JsonSerializerOptions options) { - IJsonTypeInfoResolver? typeInfoResolver = options._typeInfoResolver; - IJsonTypeInfoResolver? defaultResolver = Volatile.Read(ref s_defaultTypeInfoResolver); - - // we normalize s_defaultTypeInfoResolver to null so that hash code is the same in case s_defaultTypeInfoResolver - // is not initialized yet - if (typeInfoResolver == defaultResolver) - { - typeInfoResolver = null; - } - HashCode hc = default; hc.Add(options._dictionaryKeyPolicy); @@ -368,8 +333,7 @@ public int GetHashCode(JsonSerializerOptions options) hc.Add(options._includeFields); hc.Add(options._propertyNameCaseInsensitive); hc.Add(options._writeIndented); - hc.Add(typeInfoResolver); - hc.Add(options._serializerContext); + hc.Add(options._typeInfoResolver); GetHashCode(ref hc, options._converters); #pragma warning disable CA2252 // This API requires opting into preview features GetHashCode(ref hc, options._polymorphicTypeConfigurations); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs index 1f632cc7aa657..4bf25a6d5dfac 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs @@ -24,30 +24,8 @@ public sealed partial class JsonSerializerOptions // The global list of built-in converters that override CanConvert(). private static JsonConverter[]? s_defaultFactoryConverters; - // Stores the JsonTypeInfo factory, which requires unreferenced code and must be rooted by the reflection-based serializer. - private static IJsonTypeInfoResolver? s_defaultTypeInfoResolver; - - [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] - [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - [MemberNotNull(nameof(s_defaultTypeInfoResolver))] - private static void RootReflectionSerializerDependencies() - { - // We need to ensure that all threads use same instance of s_defaultTypeInfoResolver or - // otherwise EqualityComparer for CachingContext may fail even though - // two options might assign _typeInfoResolver to s_defaultTypeInfoResolver - Interlocked.CompareExchange(ref s_defaultTypeInfoResolver, new DefaultJsonTypeInfoResolver(mutable: false), null); - - // constructor for DefaultJsonTypeInfoResolver calls RootConverters() - // and therefore both fields are initialized now - Debug.Assert(s_defaultSimpleConverters != null); - Debug.Assert(s_defaultFactoryConverters != null); - } - [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - // This method should be called only within DefaultJsonTypeInfoResolver constructor - // We need to do this in case DefaultJsonTypeInfoResolver is called standalone without - // serialization happening first internal static void RootConverters() { // s_defaultFactoryConverters is the last field assigned. @@ -236,7 +214,7 @@ public JsonConverter GetConverter(Type typeToConvert) ThrowHelper.ThrowArgumentNullException(nameof(typeToConvert)); } - RootReflectionSerializerDependencies(); + RootConverters(); return GetConverterInternal(typeToConvert); } @@ -256,7 +234,7 @@ internal JsonConverter GetConverterFromType(Type typeToConvert) Debug.Assert(typeToConvert != null); // Priority 1: If there is a JsonSerializerContext, fetch the converter from there. - JsonConverter? converter = _serializerContext?.GetTypeInfo(typeToConvert)?.Converter; + JsonConverter? converter = SerializerContext?.GetTypeInfo(typeToConvert)?.Converter; // Priority 2: Attempt to get custom converter added at runtime. // Currently there is not a way at runtime to override the [JsonConverter] when applied to a property. @@ -386,9 +364,9 @@ private JsonConverter GetConverterFromAttribute(JsonConverterAttribute converter internal bool TryGetDefaultSimpleConverter(Type typeToConvert, [NotNullWhen(true)] out JsonConverter? converter) { - if (_serializerContext == null && // For consistency do not return any default converters for - // options instances linked to a JsonSerializerContext, - // even if the default converters might have been rooted. + if (SerializerContext is null && // For consistency do not return any default converters for + // options instances linked to a JsonSerializerContext, + // even if the default converters might have been rooted. s_defaultSimpleConverters != null && s_defaultSimpleConverters.TryGetValue(typeToConvert, out converter)) { 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 0e3c04dd1a6a9..952d8ebf32166 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 @@ -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.Generic; using System.ComponentModel; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -9,7 +10,6 @@ using System.Text.Json.Nodes; using System.Text.Json.Serialization; using System.Text.Json.Serialization.Metadata; -using System.Threading; namespace System.Text.Json { @@ -35,7 +35,7 @@ public sealed partial class JsonSerializerOptions public static JsonSerializerOptions Default { get; } = CreateDefaultImmutableInstance(); // For any new option added, adding it to the options copied in the copy constructor below must be considered. - private JsonSerializerContext? _serializerContext; + private IJsonTypeInfoResolver? _typeInfoResolver; private MemberAccessor? _memberAccessorStrategy; private JsonNamingPolicy? _dictionaryKeyPolicy; private JsonNamingPolicy? _jsonPropertyNamingPolicy; @@ -60,26 +60,15 @@ public sealed partial class JsonSerializerOptions private bool _propertyNameCaseInsensitive; private bool _writeIndented; + private bool _isLockedInstance; + /// /// Constructs a new instance. /// public JsonSerializerOptions() { - _converters = new ConfigurationList() - { - IsReadOnlyFunc = IsReadOnly, - ThrowImmutableFunc = ThrowOptionsImmutable, - }; - -#pragma warning disable CA2252 // This API requires opting into preview features - _polymorphicTypeConfigurations = new ConfigurationList() - { - IsReadOnlyFunc = IsReadOnly, - ThrowImmutableFunc = ThrowOptionsImmutable, - OnElementAdded = static config => { config.IsAssignedToOptionsInstance = true; } - }; -#pragma warning restore CA2252 // This API requires opting into preview features - + _converters = new ConverterList(this); + _polymorphicTypeConfigurations = new PolymorphicConfigurationList(this); TrackOptionsInstance(this); } @@ -102,17 +91,8 @@ public JsonSerializerOptions(JsonSerializerOptions options) _jsonPropertyNamingPolicy = options._jsonPropertyNamingPolicy; _readCommentHandling = options._readCommentHandling; _referenceHandler = options._referenceHandler; - _converters = new ConfigurationList(options._converters){ - IsReadOnlyFunc = IsReadOnly, - ThrowImmutableFunc = ThrowOptionsImmutable, - }; -#pragma warning disable CA2252 // This API requires opting into preview features - _polymorphicTypeConfigurations = new ConfigurationList(options._polymorphicTypeConfigurations) - { - IsReadOnlyFunc = IsReadOnly, - ThrowImmutableFunc = ThrowOptionsImmutable, - }; -#pragma warning restore CA2252 // This API requires opting into preview features + _converters = new ConverterList(this, options._converters); + _polymorphicTypeConfigurations = new PolymorphicConfigurationList(this, options._polymorphicTypeConfigurations); _encoder = options._encoder; _defaultIgnoreCondition = options._defaultIgnoreCondition; _numberHandling = options._numberHandling; @@ -127,12 +107,9 @@ public JsonSerializerOptions(JsonSerializerOptions options) _includeFields = options._includeFields; _propertyNameCaseInsensitive = options._propertyNameCaseInsensitive; _writeIndented = options._writeIndented; - - if (_serializerContext == null && _typeInfoResolver != s_defaultTypeInfoResolver) - { - _typeInfoResolver = options._typeInfoResolver; - } - + // Preserve backward compatibility with .NET 6 + // This should almost certainly be changed, cf. https://github.com/dotnet/aspnetcore/issues/38720 + _typeInfoResolver = options._typeInfoResolver is JsonSerializerContext ? null : options._typeInfoResolver; EffectiveMaxDepth = options.EffectiveMaxDepth; ReferenceHandlingStrategy = options.ReferenceHandlingStrategy; @@ -184,8 +161,8 @@ public JsonSerializerOptions(JsonSerializerDefaults defaults) : this() { VerifyMutable(); TContext context = new(); - _serializerContext = context; _typeInfoResolver = context; + _isLockedInstance = true; context._options = this; } @@ -198,27 +175,31 @@ public IJsonTypeInfoResolver TypeInfoResolver [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] get { - if (_typeInfoResolver != null) - { - return _typeInfoResolver; - } - - EnsureInitializedForReflectionSerializer(); - return _typeInfoResolver; + return _typeInfoResolver ?? DefaultJsonTypeInfoResolver.DefaultInstance; } set { VerifyMutable(); - if (value == null) + if (value is null) { throw new ArgumentNullException(nameof(value)); } + if (value is JsonSerializerContext ctx) + { + if (ctx._options != null && ctx._options != this) + { + // TODO evaluate if this is the appropriate behaviour; + ThrowHelper.ThrowInvalidOperationException_SerializerContextOptionsImmutable(); + } + + // Associate options instance with context and lock for further modification + ctx._options = this; + _isLockedInstance = true; + } + _typeInfoResolver = value; - // If _typeInfoResolver is JsonSerializerContext we still assign _serializerContext to null. - // This way we prevent any automatic fallback logic while at the same time not changing existing behavior - _serializerContext = null; } } @@ -611,16 +592,7 @@ public ReferenceHandler? ReferenceHandler } } - internal JsonSerializerContext? JsonSerializerContext - { - get => _serializerContext; - set - { - VerifyMutable(); - _serializerContext = value; - _typeInfoResolver = value; - } - } + internal JsonSerializerContext? SerializerContext => _typeInfoResolver as JsonSerializerContext; // The cached value used to determine if ReferenceHandler should use Preserve or IgnoreCycles semanitcs or None of them. internal ReferenceHandlingStrategy ReferenceHandlingStrategy = ReferenceHandlingStrategy.None; @@ -649,43 +621,45 @@ internal MemberAccessor MemberAccessorStrategy } } - /// - /// JsonTypeInfo resolver used for serialization. - /// - internal IJsonTypeInfoResolver? _typeInfoResolver; + internal bool IsInitializedForReflectionSerializer { get; private set; } + // Effective resolver, populated when enacting reflection-based fallback + // Should not be taken into account when calculating options equality. + private IJsonTypeInfoResolver? _effectiveJsonTypeInfoResolver; /// /// Initializes the converters for the reflection-based serializer. /// [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - [MemberNotNull(nameof(_typeInfoResolver))] - internal void EnsureInitializedForReflectionSerializer() + internal void InitializeForReflectionSerializer() { - if (s_defaultTypeInfoResolver != null && _typeInfoResolver != null && (_serializerContext == null || _serializerContext.FallbackResolver != null)) + RootConverters(); + + if (_typeInfoResolver is JsonSerializerContext ctx) { - return; + // .NET 6 backward compatibility; use fallback to reflection serialization + // TODO: Consider removing this behaviour (needs to be filed as a breaking change). + _effectiveJsonTypeInfoResolver = JsonTypeInfoResolver.Combine(ctx, DefaultJsonTypeInfoResolver.DefaultInstance); } - - RootReflectionSerializerDependencies(); - _typeInfoResolver ??= s_defaultTypeInfoResolver; - - if (_serializerContext != null) + else { - _serializerContext.FallbackResolver ??= s_defaultTypeInfoResolver; + _effectiveJsonTypeInfoResolver = _typeInfoResolver ?? DefaultJsonTypeInfoResolver.DefaultInstance; } - if (_cachingContext != null) + if (_cachingContext != null && _cachingContext.Options != this) { - // This only applies if resolver wasn't previously initialized - Debug.Assert(_cachingContext.Options._typeInfoResolver != null || _typeInfoResolver == s_defaultTypeInfoResolver); - _cachingContext.Options._typeInfoResolver ??= s_defaultTypeInfoResolver; + // We're using a shared caching context deriving from a different options instance; + // for coherence ensure that it has been opted in for reflection-based serialization as well. + _cachingContext.Options.InitializeForReflectionSerializer(); } + + IsInitializedForReflectionSerializer = true; } private JsonTypeInfo GetJsonTypeInfoFromContextOrCreate(Type type) { - JsonTypeInfo? info = _typeInfoResolver?.GetTypeInfo(type, this); + IJsonTypeInfoResolver? resolver = _effectiveJsonTypeInfoResolver ?? _typeInfoResolver; + JsonTypeInfo? info = resolver?.GetTypeInfo(type, this); if (info == null) { @@ -738,21 +712,46 @@ internal JsonWriterOptions GetWriterOptions() }; } - internal bool IsReadOnly() => _cachingContext != null || _serializerContext != null; - internal void ThrowOptionsImmutable() => ThrowHelper.ThrowInvalidOperationException_SerializerOptionsImmutable(_serializerContext); - internal void VerifyMutable() { - if (IsReadOnly()) + if (_isLockedInstance) + { + ThrowHelper.ThrowInvalidOperationException_SerializerOptionsImmutable(_typeInfoResolver as JsonSerializerContext); + } + } + + private sealed class ConverterList : ConfigurationList + { + private readonly JsonSerializerOptions _options; + + public ConverterList(JsonSerializerOptions options, IList? source = null) + : base(source) { - ThrowOptionsImmutable(); + _options = options; } + + protected override bool IsLockedInstance => _options._isLockedInstance; + protected override void VerifyMutable() => _options.VerifyMutable(); + } + + private sealed class PolymorphicConfigurationList : ConfigurationList + { + private readonly JsonSerializerOptions _options; + + public PolymorphicConfigurationList(JsonSerializerOptions options, IList? source = null) + : base(source) + { + _options = options; + } + + protected override bool IsLockedInstance => _options._isLockedInstance; + protected override void VerifyMutable() => _options.VerifyMutable(); + protected override void OnItemAdded(JsonPolymorphicTypeConfiguration config) => config.IsAssignedToOptionsInstance = true; } private static JsonSerializerOptions CreateDefaultImmutableInstance() { - var options = new JsonSerializerOptions(); - options.InitializeCachingContext(); // eagerly initialize caching context to close type for modification. + var options = new JsonSerializerOptions { _isLockedInstance = true }; return options; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs index 9087636fb3bd0..6b8ffbe059bac 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/DefaultJsonTypeInfoResolver.cs @@ -1,11 +1,11 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System; using System.Collections.Generic; using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.ExceptionServices; +using System.Threading; namespace System.Text.Json.Serialization.Metadata { @@ -27,15 +27,11 @@ public DefaultJsonTypeInfoResolver() : this(mutable: true) [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] - internal DefaultJsonTypeInfoResolver(bool mutable) + private DefaultJsonTypeInfoResolver(bool mutable) { JsonSerializerOptions.RootConverters(); - Modifiers = new ConfigurationList>() - { - IsReadOnlyFunc = IsReadOnly, - ThrowImmutableFunc = ThrowTypeInfoResolverImmutable, - }; _mutable = mutable; + Modifiers = new ModifierCollection(this); } /// @@ -94,11 +90,42 @@ private JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options /// public IList> Modifiers { get; } - private bool IsReadOnly() => !_mutable; + private sealed class ModifierCollection : ConfigurationList> + { + private readonly DefaultJsonTypeInfoResolver _resolver; - private static void ThrowTypeInfoResolverImmutable() + public ModifierCollection(DefaultJsonTypeInfoResolver resolver) + { + _resolver = resolver; + } + + protected override bool IsLockedInstance => !_resolver._mutable; + protected override void VerifyMutable() + { + if (!_resolver._mutable) + { + ThrowHelper.ThrowInvalidOperationException_TypeInfoResolverImmutable(); + } + } + } + + internal static DefaultJsonTypeInfoResolver DefaultInstance { - ThrowHelper.ThrowInvalidOperationException_TypeInfoResolverImmutable(); + [RequiresUnreferencedCode(JsonSerializer.SerializationUnreferencedCodeMessage)] + [RequiresDynamicCode(JsonSerializer.SerializationRequiresDynamicCodeMessage)] + get + { + if (s_defaultInstance is DefaultJsonTypeInfoResolver resolver) + { + return resolver; + } + + var newInstance = new DefaultJsonTypeInfoResolver(mutable: false); + DefaultJsonTypeInfoResolver? originalInstance = Interlocked.CompareExchange(ref s_defaultInstance, newInstance, comparand: null); + return originalInstance ?? newInstance; + } } + + private static DefaultJsonTypeInfoResolver? s_defaultInstance; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs index 4f732683b2323..fcb715f91c31e 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfo.cs @@ -100,7 +100,7 @@ internal void ValidateCanBeUsedForDeserialization() { if (ThrowOnDeserialize) { - ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.JsonSerializerContext, Type); + ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.SerializerContext, Type); } } @@ -331,7 +331,7 @@ internal virtual void Configure() if (converter.ConstructorIsParameterized) { - InitializeConstructorParameters(GetParameterInfoValues(), sourceGenMode: Options.JsonSerializerContext != null); + InitializeConstructorParameters(GetParameterInfoValues(), sourceGenMode: Options.SerializerContext != null); } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs index 8850f7eadc5a4..c2c5003ea1ca6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoOfT.cs @@ -49,6 +49,7 @@ internal JsonTypeInfo(JsonConverter converter, JsonSerializerOptions options) /// values specified at design time. /// /// The writer is not flushed after writing. + [EditorBrowsable(EditorBrowsableState.Never)] public Action? SerializeHandler { get diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoResolver.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoResolver.cs index 9e94c2ed3656a..bf390ef774267 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoResolver.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonTypeInfoResolver.cs @@ -29,16 +29,16 @@ public static IJsonTypeInfoResolver Combine(params IJsonTypeInfoResolver[] resol private sealed class CombiningJsonTypeInfoResolver : IJsonTypeInfoResolver { - private IJsonTypeInfoResolver[] _resolvers; + private readonly IJsonTypeInfoResolver[] _resolvers; public CombiningJsonTypeInfoResolver(IJsonTypeInfoResolver[] resolvers) { - _resolvers = resolvers; + _resolvers = resolvers.AsSpan().ToArray(); } public JsonTypeInfo? GetTypeInfo(Type type, JsonSerializerOptions options) { - foreach (var resolver in _resolvers) + foreach (IJsonTypeInfoResolver resolver in _resolvers) { JsonTypeInfo? typeInfo = resolver.GetTypeInfo(type, options); if (typeInfo != null) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs index 7c902542dbf5a..ab43cb5b75a9c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/SourceGenJsonTypeInfoOfT.cs @@ -99,7 +99,7 @@ internal override void LateAddProperties() internal override JsonParameterInfoValues[] GetParameterInfoValues() { - JsonSerializerContext? context = Options.JsonSerializerContext; + JsonSerializerContext? context = Options.SerializerContext; JsonParameterInfoValues[] array; if (context == null || CtorParamInitFunc == null || (array = CtorParamInitFunc()) == null) { @@ -117,7 +117,7 @@ internal void AddPropertiesUsingSourceGenInfo() return; } - JsonSerializerContext? context = Options.JsonSerializerContext; + JsonSerializerContext? context = Options.SerializerContext; JsonPropertyInfo[] array; if (context == null || PropInitFunc == null || (array = PropInitFunc(context)) == null) { @@ -132,7 +132,7 @@ internal void AddPropertiesUsingSourceGenInfo() return; } - if (SerializeHandler != null && Options.JsonSerializerContext?.CanUseSerializationLogic == true) + if (SerializeHandler != null && Options.SerializerContext?.CanUseSerializationLogic == true) { ThrowOnDeserialize = true; return; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs index a81c7d99a9a49..3d5c02776e9bd 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/JsonSerializerContextTests.cs @@ -39,7 +39,6 @@ public static void Converters_AndTypeInfoCreator_NotRooted_WhenMetadataNotPresen // This test uses reflection to: // - Access JsonSerializerOptions.s_defaultSimpleConverters // - Access JsonSerializerOptions.s_defaultFactoryConverters - // - Access JsonSerializerOptions.s_typeInfoCreationFunc // // If any of them changes, this test will need to be kept in sync. @@ -47,9 +46,6 @@ public static void Converters_AndTypeInfoCreator_NotRooted_WhenMetadataNotPresen AssertFieldNull("s_defaultSimpleConverters", optionsInstance: null); AssertFieldNull("s_defaultFactoryConverters", optionsInstance: null); - // Confirm type info dynamic creator not set. - AssertFieldNull("s_defaultTypeInfoResolver", optionsInstance: null); - static void AssertFieldNull(string fieldName, JsonSerializerOptions? optionsInstance) { BindingFlags bindingFlags = BindingFlags.NonPublic | (optionsInstance == null ? BindingFlags.Static : BindingFlags.Instance); diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonSerializerWrapper.Reflection.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonSerializerWrapper.Reflection.cs index 2e3912a7572e9..7f6b4ebd56dd8 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonSerializerWrapper.Reflection.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/JsonSerializerWrapper.Reflection.cs @@ -572,7 +572,6 @@ private static class JsonSerializerOptionsSmallBufferMapper { private static readonly ConditionalWeakTable s_smallBufferMap = new(); private static readonly JsonSerializerOptions s_DefaultOptionsWithSmallBuffer = new JsonSerializerOptions { DefaultBufferSize = 1 }; - private static readonly FieldInfo s_optionsContextField = typeof(JsonSerializerOptions).GetField("_serializerContext", BindingFlags.NonPublic | BindingFlags.Instance); public static JsonSerializerOptions ResolveOptionsInstanceWithSmallBuffer(JsonSerializerOptions? options) { @@ -592,20 +591,15 @@ public static JsonSerializerOptions ResolveOptionsInstanceWithSmallBuffer(JsonSe return resolvedValue; } - JsonSerializerOptions smallBufferCopy = new JsonSerializerOptions(options) { DefaultBufferSize = 1 }; - CopyJsonSerializerContext(options, smallBufferCopy); + JsonSerializerOptions smallBufferCopy = new JsonSerializerOptions(options) + { + // Copy the resolver explicitly until https://github.com/dotnet/aspnetcore/issues/38720 is resolved. + TypeInfoResolver = options.TypeInfoResolver, + DefaultBufferSize = 1, + }; s_smallBufferMap.Add(options, smallBufferCopy); return smallBufferCopy; } - - private static void CopyJsonSerializerContext(JsonSerializerOptions source, JsonSerializerOptions target) - { - JsonSerializerContext context = (JsonSerializerContext)s_optionsContextField.GetValue(source); - if (context != null) - { - s_optionsContextField.SetValue(target, context); - } - } } } } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs index 1a092379ee891..c1f53298ac5df 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/MetadataTests.Options.cs @@ -55,9 +55,9 @@ public void AddContextOverwritesOptionsForFreshContext() // Those options are overwritten when context is binded via options.AddContext(); JsonSerializerOptions options = new(); options.AddContext(); // No error. - FieldInfo contextField = typeof(JsonSerializerOptions).GetField("_serializerContext", BindingFlags.NonPublic | BindingFlags.Instance); - Assert.NotNull(contextField); - Assert.Same(options, ((JsonSerializerContext)contextField.GetValue(options)).Options); + FieldInfo resolverField = typeof(JsonSerializerOptions).GetField("_typeInfoResolver", BindingFlags.NonPublic | BindingFlags.Instance); + Assert.NotNull(resolverField); + Assert.Same(options, ((JsonSerializerContext)resolverField.GetValue(options)).Options); } [Fact] diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs index 3caa428b1439f..951441d8b482c 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/OptionsTests.cs @@ -171,7 +171,7 @@ public static void WhenAddingContextTypeInfoResolverAsContextOptionsAreSameAsOpt } [Fact] - public static void TypeInfoResolverCanBeSetAfterContextIsSetThroughTypeInfoResolver() + public static void TypeInfoResolverCannotBeSetAfterContextIsSetThroughTypeInfoResolver() { var options = new JsonSerializerOptions(); IJsonTypeInfoResolver resolver = new JsonContext(); @@ -179,8 +179,7 @@ public static void TypeInfoResolverCanBeSetAfterContextIsSetThroughTypeInfoResol Assert.Same(resolver, options.TypeInfoResolver); resolver = new DefaultJsonTypeInfoResolver(); - options.TypeInfoResolver = resolver; - Assert.Same(resolver, options.TypeInfoResolver); + Assert.Throws(() => options.TypeInfoResolver = resolver); } [Fact]