From 4b14c9217d5b9c65ca697818400e79f5c03874e3 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Wed, 24 Aug 2022 15:06:21 +0100 Subject: [PATCH] Expose JsonSerializer.IsReadOnly and MakeReadOnly() APIs. (#74431) * Expose JsonSerializer.IsReadOnly and MakeReadOnly() APIs. * Address feedback --- .../System.Text.Json/ref/System.Text.Json.cs | 2 + .../src/Resources/Strings.resx | 8 +-- .../Serialization/JsonSerializerContext.cs | 18 ++++- .../JsonSerializerOptions.Caching.cs | 8 +-- .../Serialization/JsonSerializerOptions.cs | 55 +++++++++------- .../Serialization/Metadata/JsonTypeInfo.cs | 4 +- .../Text/Json/ThrowHelper.Serialization.cs | 10 +-- .../Serialization/OptionsTests.cs | 66 +++++++++++++++++-- 8 files changed, 122 insertions(+), 49 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 0cb93468eb15c..6beb61a122200 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -350,6 +350,7 @@ public JsonSerializerOptions(System.Text.Json.JsonSerializerOptions options) { } public bool IgnoreReadOnlyFields { get { throw null; } set { } } public bool IgnoreReadOnlyProperties { get { throw null; } set { } } public bool IncludeFields { get { throw null; } set { } } + public bool IsReadOnly { get { throw null; } } public int MaxDepth { get { throw null; } set { } } public System.Text.Json.Serialization.JsonNumberHandling NumberHandling { get { throw null; } set { } } public bool PropertyNameCaseInsensitive { get { throw null; } set { } } @@ -364,6 +365,7 @@ public JsonSerializerOptions(System.Text.Json.JsonSerializerOptions options) { } [System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Getting a converter for a type may require reflection which depends on unreferenced code.")] public System.Text.Json.Serialization.JsonConverter GetConverter(System.Type typeToConvert) { throw null; } public System.Text.Json.Serialization.Metadata.JsonTypeInfo GetTypeInfo(System.Type type) { throw null; } + public void MakeReadOnly() { } } public enum JsonTokenType : byte { diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 838a56c7e10cc..2f466b9c05d44 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -336,7 +336,7 @@ The JSON object contains a trailing comma at the end which is not supported in this mode. Change the reader options. - + Serializer options cannot be changed once serialization or deserialization has occurred. @@ -578,7 +578,7 @@ An item with the same key has already been added. Key: {0} - + JsonSerializerOptions instances cannot be modified once encapsulated by a JsonSerializerContext. Such encapsulation can happen either when calling 'JsonSerializerOptions.AddContext' or when passing the options instance to a JsonSerializerContext constructor. @@ -659,8 +659,8 @@ JsonPropertyInfo with name '{0}' for type '{1}' is already bound to different JsonTypeInfo. - - JsonTypeInfo metadata references a JsonSerializerOptions instance that doesn't specify a TypeInfoResolver. + + JsonSerializerOptions instance must specify a TypeInfoResolver setting before being marked as read-only. Parameter already associated with a different JsonTypeInfo instance. 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 30c2c4968417a..c396eb9858671 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 @@ -24,13 +24,25 @@ public abstract partial class JsonSerializerContext : IJsonTypeInfoResolver /// public JsonSerializerOptions Options { - get => _options ??= new JsonSerializerOptions { TypeInfoResolver = this, IsImmutable = true }; + get + { + JsonSerializerOptions? options = _options; + + if (options is null) + { + options = new JsonSerializerOptions { TypeInfoResolver = this }; + options.MakeReadOnly(); + _options = options; + } + + return options; + } internal set { - Debug.Assert(!value.IsImmutable); + Debug.Assert(!value.IsReadOnly); value.TypeInfoResolver = this; - value.IsImmutable = true; + value.MakeReadOnly(); _options = value; } } 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 f0a7e71dbc965..24152a5c716ae 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 @@ -57,7 +57,7 @@ internal JsonTypeInfo GetTypeInfoInternal(Type type, bool ensureConfigured = tru { JsonTypeInfo? typeInfo = null; - if (IsImmutable) + if (IsReadOnly) { typeInfo = GetCachingContext()?.GetOrAddJsonTypeInfo(type); if (ensureConfigured) @@ -111,7 +111,7 @@ internal JsonTypeInfo ObjectTypeInfo { get { - Debug.Assert(IsImmutable); + Debug.Assert(IsReadOnly); return _objectTypeInfo ??= GetTypeInfoInternal(JsonTypeInfo.ObjectType); } } @@ -127,7 +127,7 @@ internal void ClearCaches() private CachingContext? GetCachingContext() { - Debug.Assert(IsImmutable); + Debug.Assert(IsReadOnly); return _cachingContext ??= TrackedCachingContexts.GetOrCreate(this); } @@ -178,7 +178,7 @@ internal static class TrackedCachingContexts public static CachingContext GetOrCreate(JsonSerializerOptions options) { - Debug.Assert(options.IsImmutable, "Cannot create caching contexts for mutable JsonSerializerOptions instances"); + Debug.Assert(options.IsReadOnly, "Cannot create caching contexts for mutable JsonSerializerOptions instances"); Debug.Assert(options._typeInfoResolver != null); ConcurrentDictionary> cache = s_cache; 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 596a9c37bae48..e237a7b7c3420 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 @@ -74,7 +74,7 @@ public static JsonSerializerOptions Default private bool _propertyNameCaseInsensitive; private bool _writeIndented; - private volatile bool _isImmutable; + private volatile bool _isReadOnly; /// /// Constructs a new instance. @@ -602,20 +602,35 @@ public ReferenceHandler? ReferenceHandler new ReflectionEmitCachingMemberAccessor() : new ReflectionMemberAccessor(); #elif NETFRAMEWORK - new ReflectionEmitCachingMemberAccessor(); + new ReflectionEmitCachingMemberAccessor(); #else - new ReflectionMemberAccessor(); + new ReflectionMemberAccessor(); #endif - internal bool IsImmutable + /// + /// Specifies whether the current instance has been locked for modification. + /// + /// + /// A instance can be locked either if + /// it has been passed to one of the methods, + /// has been associated with a instance, + /// or a user explicitly called the method on the instance. + /// + public bool IsReadOnly => _isReadOnly; + + /// + /// Locks the current instance for further modification. + /// + /// The instance does not specify a setting. + /// This method is idempotent. + public void MakeReadOnly() { - get => _isImmutable; - set + if (_typeInfoResolver is null) { - Debug.Assert(value, "cannot unlock options instances"); - Debug.Assert(_typeInfoResolver != null, "cannot lock without a resolver."); - _isImmutable = true; + ThrowHelper.ThrowInvalidOperationException_JsonSerializerOptionsNoTypeInfoResolverSpecified(); } + + _isReadOnly = true; } /// @@ -629,23 +644,13 @@ internal void InitializeForReflectionSerializer() // the default resolver to gain access to the default converters. DefaultJsonTypeInfoResolver defaultResolver = DefaultJsonTypeInfoResolver.RootDefaultInstance(); _typeInfoResolver ??= defaultResolver; - IsImmutable = true; + MakeReadOnly(); _isInitializedForReflectionSerializer = true; } internal bool IsInitializedForReflectionSerializer => _isInitializedForReflectionSerializer; private volatile bool _isInitializedForReflectionSerializer; - internal void InitializeForMetadataGeneration() - { - if (_typeInfoResolver is null) - { - ThrowHelper.ThrowInvalidOperationException_JsonTypeInfoUsedButTypeInfoResolverNotSet(); - } - - IsImmutable = true; - } - private JsonTypeInfo? GetTypeInfoNoCaching(Type type) { JsonTypeInfo? info = _typeInfoResolver?.GetTypeInfo(type, this); @@ -709,9 +714,9 @@ internal JsonWriterOptions GetWriterOptions() internal void VerifyMutable() { - if (_isImmutable) + if (_isReadOnly) { - ThrowHelper.ThrowInvalidOperationException_SerializerOptionsImmutable(_typeInfoResolver as JsonSerializerContext); + ThrowHelper.ThrowInvalidOperationException_SerializerOptionsReadOnly(_typeInfoResolver as JsonSerializerContext); } } @@ -725,7 +730,7 @@ public ConverterList(JsonSerializerOptions options, IList? source _options = options; } - protected override bool IsImmutable => _options.IsImmutable; + protected override bool IsImmutable => _options.IsReadOnly; protected override void VerifyMutable() => _options.VerifyMutable(); } @@ -736,12 +741,12 @@ private static JsonSerializerOptions GetOrCreateDefaultOptionsInstance() var options = new JsonSerializerOptions { TypeInfoResolver = DefaultJsonTypeInfoResolver.RootDefaultInstance(), - IsImmutable = true + _isReadOnly = true }; return Interlocked.CompareExchange(ref s_defaultOptions, options, null) ?? options; } - private string DebuggerDisplay => $"TypeInfoResolver = {TypeInfoResolver?.GetType()?.Name}, IsImmutable = {IsImmutable}"; + private string DebuggerDisplay => $"TypeInfoResolver = {TypeInfoResolver?.GetType()?.Name}, IsImmutable = {IsReadOnly}"; } } 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 ef345aa231d6b..a334e5b055409 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 @@ -531,9 +531,9 @@ internal void Configure() { Debug.Assert(Monitor.IsEntered(_configureLock), "Configure called directly, use EnsureConfigured which locks this method"); - if (!Options.IsImmutable) + if (!Options.IsReadOnly) { - Options.InitializeForMetadataGeneration(); + Options.MakeReadOnly(); } PropertyInfoForTypeInfo.EnsureChildOf(this); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs index b8ce2bacfd3f8..23c0006fa37d7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs @@ -139,9 +139,9 @@ public static void ThrowInvalidOperationException_ResolverTypeInfoOptionsNotComp } [DoesNotReturn] - public static void ThrowInvalidOperationException_JsonTypeInfoUsedButTypeInfoResolverNotSet() + public static void ThrowInvalidOperationException_JsonSerializerOptionsNoTypeInfoResolverSpecified() { - throw new InvalidOperationException(SR.JsonTypeInfoUsedButTypeInfoResolverNotSet); + throw new InvalidOperationException(SR.JsonSerializerOptionsNoTypeInfoResolverSpecified); } [DoesNotReturn] @@ -170,11 +170,11 @@ public static void ThrowInvalidOperationException_SerializationConverterOnAttrib } [DoesNotReturn] - public static void ThrowInvalidOperationException_SerializerOptionsImmutable(JsonSerializerContext? context) + public static void ThrowInvalidOperationException_SerializerOptionsReadOnly(JsonSerializerContext? context) { string message = context == null - ? SR.SerializerOptionsImmutable - : SR.SerializerContextOptionsImmutable; + ? SR.SerializerOptionsReadOnly + : SR.SerializerContextOptionsReadOnly; throw new InvalidOperationException(message); } 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 a91467a9943a3..0e01eb9f8717f 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 @@ -135,11 +135,49 @@ public static void NewDefaultOptions_TypeInfoResolverIsNull() Assert.Null(options.TypeInfoResolver); } + [Fact] + public static void NewDefaultOptions_IsNotReadOnly() + { + var options = new JsonSerializerOptions { TypeInfoResolver = new DefaultJsonTypeInfoResolver() }; + Assert.False(options.IsReadOnly); + + options.MakeReadOnly(); + Assert.True(options.IsReadOnly); + options.MakeReadOnly(); // Is idempotent operation + Assert.True(options.IsReadOnly); + + Assert.Throws(() => options.MaxDepth = 13); + Assert.Throws(() => options.Converters.Add(new JsonStringEnumConverter())); + Assert.Throws(() => options.AddContext()); + Assert.Throws(() => options.TypeInfoResolver = null); + + options.MakeReadOnly(); // Is still idempotent operation + Assert.True(options.IsReadOnly); + } + + [Fact] + public static void NewDefaultOptions_MakeReadOnly_NoTypeInfoResolver_ThrowsInvalidOperation() + { + var options = new JsonSerializerOptions(); + Assert.False(options.IsReadOnly); + + Assert.Throws(() => options.MakeReadOnly()); + Assert.False(options.IsReadOnly); + + options.TypeInfoResolver = new DefaultJsonTypeInfoResolver(); + options.MakeReadOnly(); + Assert.True(options.IsReadOnly); + } + [Fact] public static void TypeInfoResolverCannotBeSetAfterAddingContext() { var options = new JsonSerializerOptions(); + Assert.False(options.IsReadOnly); + options.AddContext(); + Assert.True(options.IsReadOnly); + Assert.IsType(options.TypeInfoResolver); Assert.Throws(() => options.TypeInfoResolver = new DefaultJsonTypeInfoResolver()); } @@ -614,12 +652,16 @@ private static void GenericConverterTestHelper(string converterName, object o public static void CopyConstructor_OriginalLocked() { JsonSerializerOptions options = new JsonSerializerOptions { Encoder = JavaScriptEncoder.UnsafeRelaxedJsonEscaping }; + Assert.False(options.IsReadOnly); // Perform serialization with options, after which it will be locked. JsonSerializer.Serialize("1", options); + + Assert.True(options.IsReadOnly); Assert.Throws(() => options.ReferenceHandler = ReferenceHandler.Preserve); var newOptions = new JsonSerializerOptions(options); + Assert.False(newOptions.IsReadOnly); VerifyOptionsEqual(options, newOptions); // No exception is thrown on mutating the new options instance because it is "unlocked". @@ -710,6 +752,7 @@ public static void JsonSerializerOptions_Default_ReturnsSameInstance() public static void JsonSerializerOptions_Default_IsReadOnly() { var optionsSingleton = JsonSerializerOptions.Default; + Assert.True(optionsSingleton.IsReadOnly); Assert.Throws(() => optionsSingleton.IncludeFields = true); Assert.Throws(() => optionsSingleton.Converters.Add(new JsonStringEnumConverter())); Assert.Throws(() => optionsSingleton.AddContext()); @@ -719,6 +762,8 @@ public static void JsonSerializerOptions_Default_IsReadOnly() Assert.Throws(() => resolver.Modifiers.Clear()); Assert.Throws(() => resolver.Modifiers.Add(ti => { })); Assert.Throws(() => resolver.Modifiers.Insert(0, ti => { })); + + optionsSingleton.MakeReadOnly(); // MakeReadOnly is idempontent. } [Fact] @@ -726,6 +771,7 @@ public static void DefaultSerializerOptions_General() { var options = new JsonSerializerOptions(); var newOptions = new JsonSerializerOptions(JsonSerializerDefaults.General); + Assert.False(newOptions.IsReadOnly); VerifyOptionsEqual(options, newOptions); } @@ -757,8 +803,8 @@ private static JsonSerializerOptions GetFullyPopulatedOptionsInstance() if (propertyType == typeof(bool)) { - // IgnoreNullValues and DefaultIgnoreCondition cannot be active at the same time. - if (property.Name != "IgnoreNullValues") + if (property.Name is not nameof(JsonSerializerOptions.IgnoreNullValues) // IgnoreNullValues and DefaultIgnoreCondition cannot be active at the same time. + and not nameof(JsonSerializerOptions.IsReadOnly)) // Property is not structural and cannot be set. { property.SetValue(options, true); } @@ -815,6 +861,11 @@ private static void VerifyOptionsEqual(JsonSerializerOptions options, JsonSerial if (propertyType == typeof(bool)) { + if (property.Name == nameof(JsonSerializerOptions.IsReadOnly)) + { + break; // readonly-ness is not a structural property of JsonSerializerOptions. + } + Assert.Equal((bool)property.GetValue(options), (bool)property.GetValue(newOptions)); } else if (propertyType == typeof(int)) @@ -834,19 +885,19 @@ private static void VerifyOptionsEqual(JsonSerializerOptions options, JsonSerial } else if (propertyType.IsValueType) { - if (property.Name == "ReadCommentHandling") + if (property.Name == nameof(JsonSerializerOptions.ReadCommentHandling)) { Assert.Equal(options.ReadCommentHandling, newOptions.ReadCommentHandling); } - else if (property.Name == "DefaultIgnoreCondition") + else if (property.Name == nameof(JsonSerializerOptions.DefaultIgnoreCondition)) { Assert.Equal(options.DefaultIgnoreCondition, newOptions.DefaultIgnoreCondition); } - else if (property.Name == "NumberHandling") + else if (property.Name == nameof(JsonSerializerOptions.NumberHandling)) { Assert.Equal(options.NumberHandling, newOptions.NumberHandling); } - else if (property.Name == "UnknownTypeHandling") + else if (property.Name == nameof(JsonSerializerOptions.UnknownTypeHandling)) { Assert.Equal(options.UnknownTypeHandling, newOptions.UnknownTypeHandling); } @@ -988,10 +1039,13 @@ public static void GetTypeInfo_MutableOptions_CanModifyMetadata() jti.Properties.Clear(); var value = new TestClassForEncoding { MyString = "SomeValue" }; + Assert.False(options.IsReadOnly); + string json = JsonSerializer.Serialize(value, jti); Assert.Equal("{}", json); // Using JsonTypeInfo will lock JsonSerializerOptions + Assert.True(options.IsReadOnly); Assert.Throws(() => options.IncludeFields = false); // Getting JsonTypeInfo now should return a fresh immutable instance