From 7e9eb391aedd85a9ea361615088bec8619aae440 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 3 Oct 2022 15:44:21 +0100 Subject: [PATCH 1/5] Ensure source generated metadata properties are read-only. --- .../gen/JsonSourceGenerator.Emitter.cs | 32 +++++++++---------- .../System.Text.Json/ref/System.Text.Json.cs | 1 + .../Metadata/JsonMetadataServices.cs | 15 +++++++++ .../Metadata/JsonPropertyInfo.cs | 2 +- .../Serialization/Metadata/JsonTypeInfo.cs | 6 +++- .../JsonSerializerContextTests.cs | 16 ++++++++++ 6 files changed, 53 insertions(+), 19 deletions(-) diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs index db7b990e095d92..4d8ac66e187cc4 100644 --- a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs +++ b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs @@ -1010,6 +1010,8 @@ private static string GenerateFastPathFuncForType(TypeGenerationSpec typeGenSpec return $@" +// Intentionally not a static method because we create a delegate to it. Invoking delegates to instance +// methods is almost as fast as virtual calls. Static methods need to go through a shuffle thunk. private void {serializeMethodName}({Utf8JsonWriterTypeRef} {WriterVarName}, {valueTypeRef} {ValueVarName}) {{ {GetEarlyNullCheckSource(emitNullCheck)} @@ -1085,16 +1087,19 @@ private static string GenerateForType(TypeGenerationSpec typeMetadata, string me /// public {typeInfoPropertyTypeRef} {typeFriendlyName} {{ - get => _{typeFriendlyName} ??= {typeMetadata.CreateTypeInfoMethodName}({OptionsInstanceVariableName}); + get => _{typeFriendlyName} ??= {typeMetadata.CreateTypeInfoMethodName}({OptionsInstanceVariableName}, makeReadOnly: true); }} -// Intentionally not a static method because we create a delegate to it. Invoking delegates to instance -// methods is almost as fast as virtual calls. Static methods need to go through a shuffle thunk. -private {typeInfoPropertyTypeRef} {typeMetadata.CreateTypeInfoMethodName}({JsonSerializerOptionsTypeRef} {OptionsLocalVariableName}) +private {typeInfoPropertyTypeRef} {typeMetadata.CreateTypeInfoMethodName}({JsonSerializerOptionsTypeRef} {OptionsLocalVariableName}, bool makeReadOnly) {{ {typeInfoPropertyTypeRef}? {JsonTypeInfoReturnValueLocalVariableName} = null; {WrapWithCheckForCustomConverter(metadataInitSource, typeCompilableName)} + if (makeReadOnly) + {{ + {JsonMetadataServicesTypeRef}.MakeReadOnly({JsonTypeInfoReturnValueLocalVariableName}); + }} + return {JsonTypeInfoReturnValueLocalVariableName}; }} {additionalSource}"; @@ -1271,30 +1276,23 @@ private string GetGetTypeInfoImplementation() // Explicit IJsonTypeInfoResolver implementation sb.AppendLine(); sb.Append(@$"{JsonTypeInfoTypeRef}? {JsonTypeInfoResolverTypeRef}.GetTypeInfo({TypeTypeRef} type, {JsonSerializerOptionsTypeRef} {OptionsLocalVariableName}) -{{ - if ({OptionsInstanceVariableName} == {OptionsLocalVariableName}) - {{ - return this.GetTypeInfo(type); - }} - else - {{"); +{{"); // TODO (https://github.com/dotnet/runtime/issues/52218): Make this Dictionary-lookup-based if root-serializable type count > 64. foreach (TypeGenerationSpec metadata in types) { if (metadata.ClassType != ClassType.TypeUnsupportedBySourceGen) { sb.Append($@" - if (type == typeof({metadata.TypeRef})) - {{ - return {metadata.CreateTypeInfoMethodName}({OptionsLocalVariableName}); - }} + if (type == typeof({metadata.TypeRef})) + {{ + return {metadata.CreateTypeInfoMethodName}({OptionsLocalVariableName}, makeReadOnly: false); + }} "); } } sb.Append($@" - return null; - }} + return null; }} "); 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 760445230c506a..cc3691faa468a1 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -1081,6 +1081,7 @@ public static partial class JsonMetadataServices public static System.Text.Json.Serialization.JsonConverter JsonNodeConverter { get { throw null; } } public static System.Text.Json.Serialization.JsonConverter JsonObjectConverter { get { throw null; } } public static System.Text.Json.Serialization.JsonConverter JsonValueConverter { get { throw null; } } + public static void MakeReadOnly(System.Text.Json.Serialization.Metadata.JsonTypeInfo jsonTypeInfo) { throw null; } public static System.Text.Json.Serialization.JsonConverter ObjectConverter { get { throw null; } } [System.CLSCompliantAttribute(false)] public static System.Text.Json.Serialization.JsonConverter SByteConverter { get { throw null; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs index bdaec174b731b6..4fbd6fb5a17b82 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs @@ -94,5 +94,20 @@ public static JsonTypeInfo CreateValueInfo(JsonSerializerOptions options, JsonTypeInfo info = new SourceGenJsonTypeInfo(converter, options); return info; } + + /// + /// Marks the provided instance as locked for further modification. + /// + /// The metadata instance to lock for modification. + /// Thrown when is null. + public static void MakeReadOnly(JsonTypeInfo jsonTypeInfo) + { + if (jsonTypeInfo is null) + { + ThrowHelper.ThrowArgumentNullException(nameof(jsonTypeInfo)); + } + + jsonTypeInfo.IsReadOnly = true; + } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index c2f904306d494c..731e35478891d7 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -266,7 +266,7 @@ internal static JsonPropertyInfo GetPropertyPlaceholder() private protected void VerifyMutable() { - if (_isConfigured) + if (_isConfigured || ParentTypeInfo?.IsReadOnly == true) { ThrowHelper.ThrowInvalidOperationException_PropertyInfoImmutable(); } 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 87d43184054929..74e787fb8843f0 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 @@ -477,7 +477,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, JsonSerializerOptions internal void VerifyMutable() { - if (_isConfigured) + if (_isConfigured || IsReadOnly) { ThrowHelper.ThrowInvalidOperationException_TypeInfoImmutable(); } @@ -489,6 +489,8 @@ internal void VerifyMutable() internal bool IsConfigured => _isConfigured; + internal bool IsReadOnly { get; set; } + internal void EnsureConfigured() { Debug.Assert(!Monitor.IsEntered(_configureLock), "recursive locking detected."); @@ -749,6 +751,8 @@ internal void CacheMember(JsonPropertyInfo jsonPropertyInfo, JsonPropertyDiction Debug.Assert(jsonPropertyInfo.MemberName != null, "MemberName can be null in custom JsonPropertyInfo instances and should never be passed in this method"); string memberName = jsonPropertyInfo.MemberName; + jsonPropertyInfo.EnsureChildOf(this); + if (jsonPropertyInfo.IsExtensionData) { if (ExtensionDataProperty != null) 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 b89fcf34ed5ce0..c1e73757245a2a 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 @@ -21,6 +21,22 @@ public static void VariousNestingAndVisibilityLevelsAreSupported() Assert.NotNull(NestedPublicContext.NestedProtectedInternalClass.Default); } + [Fact] + public static void ContextMetadataIsImmutable() + { + JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; + + Assert.Throws(() => typeInfo.CreateObject = null); + Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); + Assert.Throws(() => typeInfo.Properties.Clear()); + + JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; + Assert.Throws(() => propertyInfo.Name = "differentName"); + Assert.Throws(() => propertyInfo.IsExtensionData = true); + Assert.Throws(() => propertyInfo.IsRequired = true); + Assert.Throws(() => propertyInfo.Order = -1); + } + [Fact] public static void VariousGenericsAreSupported() { From b140ae46059c4ce8523974974f5b65708a42092c Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Mon, 3 Oct 2022 16:06:04 +0100 Subject: [PATCH 2/5] Use RemoteExecutor when modifying static metadata. --- .../JsonSerializerContextTests.cs | 25 +++++++++++-------- 1 file changed, 15 insertions(+), 10 deletions(-) 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 c1e73757245a2a..36b8194be73d26 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 @@ -21,20 +21,25 @@ public static void VariousNestingAndVisibilityLevelsAreSupported() Assert.NotNull(NestedPublicContext.NestedProtectedInternalClass.Default); } - [Fact] + [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] + [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] public static void ContextMetadataIsImmutable() { - JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; + RemoteExecutor.Invoke( + static () => + { + JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; - Assert.Throws(() => typeInfo.CreateObject = null); - Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); - Assert.Throws(() => typeInfo.Properties.Clear()); + Assert.Throws(() => typeInfo.CreateObject = null); + Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); + Assert.Throws(() => typeInfo.Properties.Clear()); - JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; - Assert.Throws(() => propertyInfo.Name = "differentName"); - Assert.Throws(() => propertyInfo.IsExtensionData = true); - Assert.Throws(() => propertyInfo.IsRequired = true); - Assert.Throws(() => propertyInfo.Order = -1); + JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; + Assert.Throws(() => propertyInfo.Name = "differentName"); + Assert.Throws(() => propertyInfo.IsExtensionData = true); + Assert.Throws(() => propertyInfo.IsRequired = true); + Assert.Throws(() => propertyInfo.Order = -1); + }).Dispose(); } [Fact] From b1c641cb326d92161939bc904b2508d36af7b267 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Fri, 7 Oct 2022 15:20:29 +0100 Subject: [PATCH 3/5] Add testing validating that interface metadata is mutable. --- .../JsonSerializerContextTests.cs | 71 ++++++++++++++----- 1 file changed, 55 insertions(+), 16 deletions(-) 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 36b8194be73d26..a36fa305c166ce 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 @@ -21,25 +21,64 @@ public static void VariousNestingAndVisibilityLevelsAreSupported() Assert.NotNull(NestedPublicContext.NestedProtectedInternalClass.Default); } - [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework)] - [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))] - public static void ContextMetadataIsImmutable() + [Fact] + public static void PropertyMetadataIsImmutable() { - RemoteExecutor.Invoke( - static () => - { - JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; + JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; - Assert.Throws(() => typeInfo.CreateObject = null); - Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); - Assert.Throws(() => typeInfo.Properties.Clear()); + Assert.Throws(() => typeInfo.CreateObject = null); + Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); + Assert.Throws(() => typeInfo.Properties.Clear()); - JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; - Assert.Throws(() => propertyInfo.Name = "differentName"); - Assert.Throws(() => propertyInfo.IsExtensionData = true); - Assert.Throws(() => propertyInfo.IsRequired = true); - Assert.Throws(() => propertyInfo.Order = -1); - }).Dispose(); + JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; + Assert.Throws(() => propertyInfo.Name = "differentName"); + Assert.Throws(() => propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString); + Assert.Throws(() => propertyInfo.IsRequired = true); + Assert.Throws(() => propertyInfo.Order = -1); + } + + [Fact] + public static void JsonSerializerContext_GetTypeInfo_MetadataIsImmutable() + { + JsonTypeInfo typeInfo = (JsonTypeInfo)PersonJsonContext.Default.GetTypeInfo(typeof(Person)); + + Assert.Throws(() => typeInfo.CreateObject = null); + Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); + Assert.Throws(() => typeInfo.Properties.Clear()); + + JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; + Assert.Throws(() => propertyInfo.Name = "differentName"); + Assert.Throws(() => propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString); + Assert.Throws(() => propertyInfo.IsRequired = true); + Assert.Throws(() => propertyInfo.Order = -1); + } + + [Fact] + public static void IJsonTypeInfoResolver_GetTypeInfo_MetadataIsMutable() + { + IJsonTypeInfoResolver resolver = PersonJsonContext.Default; + JsonTypeInfo typeInfo = (JsonTypeInfo)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options); + + Assert.NotSame(typeInfo, PersonJsonContext.Default.Person); + + JsonTypeInfo typeInfo2 = (JsonTypeInfo)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options); + Assert.NotSame(typeInfo, typeInfo2); + + typeInfo.CreateObject = null; + typeInfo.OnDeserializing = obj => { }; + + JsonPropertyInfo propertyInfo = typeInfo.Properties[0]; + propertyInfo.Name = "differentName"; + propertyInfo.NumberHandling = JsonNumberHandling.AllowReadingFromString; + propertyInfo.IsRequired = true; + propertyInfo.Order = -1; + + typeInfo.Properties.Clear(); + Assert.Equal(0, typeInfo.Properties.Count); + + // Changes should not impact other metadata instances + Assert.Equal(2, typeInfo2.Properties.Count); + Assert.Equal(2, PersonJsonContext.Default.Person.Properties.Count); } [Fact] From de354ee41e598d41d18fbefa0481aa4426c84084 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Sat, 8 Oct 2022 18:07:08 +0100 Subject: [PATCH 4/5] Update error messages --- src/libraries/System.Text.Json/src/Resources/Strings.resx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index ecd19e18c447dc..69a214e20a2ee2 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -247,10 +247,10 @@ Cannot add callbacks to the 'Modifiers' property after the resolver has been used for the first time. - JsonTypeInfo cannot be changed after first usage. + This JsonTypeInfo instance is marked read-only or has already been used in serialization or deserialization. - JsonPropertyInfo cannot be changed after first usage. + This JsonTypeInfo instance is marked read-only or has already been used in serialization or deserialization. Max depth must be positive. @@ -337,7 +337,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. + This JsonSerializerOptions instance is read-only or has already been used in serialization or deserialization. Stream is not writable. From 2bc1180b24efe2ebd80bc6d878277b6dd6f59f91 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Tue, 11 Oct 2022 10:49:35 +0100 Subject: [PATCH 5/5] Add JsonTypeInfo.IsReadOnly/MakeReadOnly() APIs. --- .../gen/JsonSourceGenerator.Emitter.cs | 6 ++--- .../System.Text.Json/ref/System.Text.Json.cs | 3 ++- .../Metadata/JsonMetadataServices.cs | 15 ----------- .../Metadata/JsonPropertyInfo.cs | 2 +- .../Serialization/Metadata/JsonTypeInfo.cs | 25 ++++++++++++++++--- .../JsonSerializerContextTests.cs | 4 +++ ...tJsonTypeInfoResolverTests.JsonTypeInfo.cs | 13 +++++++--- .../Serialization/OptionsTests.cs | 6 +++++ 8 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs index 4d8ac66e187cc4..c5d949f29283a4 100644 --- a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs +++ b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs @@ -3,11 +3,8 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Reflection; -using System.Reflection.Metadata; -using System.Text.Json; using System.Text.Json.Reflection; using System.Text.Json.Serialization; using Microsoft.CodeAnalysis; @@ -28,6 +25,7 @@ private sealed partial class Emitter private const string DefaultOptionsStaticVarName = "s_defaultOptions"; private const string DefaultContextBackingStaticVarName = "s_defaultContext"; internal const string GetConverterFromFactoryMethodName = "GetConverterFromFactory"; + private const string MakeReadOnlyMethodName = "MakeReadOnly"; private const string InfoVarName = "info"; private const string PropertyInfoVarName = "propertyInfo"; internal const string JsonContextVarName = "jsonContext"; @@ -1097,7 +1095,7 @@ private static string GenerateForType(TypeGenerationSpec typeMetadata, string me if (makeReadOnly) {{ - {JsonMetadataServicesTypeRef}.MakeReadOnly({JsonTypeInfoReturnValueLocalVariableName}); + {JsonTypeInfoReturnValueLocalVariableName}.{MakeReadOnlyMethodName}(); }} return {JsonTypeInfoReturnValueLocalVariableName}; 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 cc3691faa468a1..76c5ca1d55fbfe 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -1081,7 +1081,6 @@ public static partial class JsonMetadataServices public static System.Text.Json.Serialization.JsonConverter JsonNodeConverter { get { throw null; } } public static System.Text.Json.Serialization.JsonConverter JsonObjectConverter { get { throw null; } } public static System.Text.Json.Serialization.JsonConverter JsonValueConverter { get { throw null; } } - public static void MakeReadOnly(System.Text.Json.Serialization.Metadata.JsonTypeInfo jsonTypeInfo) { throw null; } public static System.Text.Json.Serialization.JsonConverter ObjectConverter { get { throw null; } } [System.CLSCompliantAttribute(false)] public static System.Text.Json.Serialization.JsonConverter SByteConverter { get { throw null; } } @@ -1195,7 +1194,9 @@ public abstract partial class JsonTypeInfo internal JsonTypeInfo() { } public System.Text.Json.Serialization.JsonConverter Converter { get { throw null; } } public System.Func? CreateObject { get { throw null; } set { } } + public bool IsReadOnly { get { throw null; } } public System.Text.Json.Serialization.Metadata.JsonTypeInfoKind Kind { get { throw null; } } + public void MakeReadOnly() { throw null; } public System.Text.Json.Serialization.JsonNumberHandling? NumberHandling { get { throw null; } set { } } public System.Action? OnDeserialized { get { throw null; } set { } } public System.Action? OnDeserializing { get { throw null; } set { } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs index 4fbd6fb5a17b82..bdaec174b731b6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonMetadataServices.cs @@ -94,20 +94,5 @@ public static JsonTypeInfo CreateValueInfo(JsonSerializerOptions options, JsonTypeInfo info = new SourceGenJsonTypeInfo(converter, options); return info; } - - /// - /// Marks the provided instance as locked for further modification. - /// - /// The metadata instance to lock for modification. - /// Thrown when is null. - public static void MakeReadOnly(JsonTypeInfo jsonTypeInfo) - { - if (jsonTypeInfo is null) - { - ThrowHelper.ThrowArgumentNullException(nameof(jsonTypeInfo)); - } - - jsonTypeInfo.IsReadOnly = true; - } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs index 731e35478891d7..c752e30292c6f5 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfo.cs @@ -266,7 +266,7 @@ internal static JsonPropertyInfo GetPropertyPlaceholder() private protected void VerifyMutable() { - if (_isConfigured || ParentTypeInfo?.IsReadOnly == true) + if (ParentTypeInfo?.IsReadOnly == true) { ThrowHelper.ThrowInvalidOperationException_PropertyInfoImmutable(); } 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 74e787fb8843f0..ee20ae7b12356a 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 @@ -256,6 +256,23 @@ public JsonPolymorphismOptions? PolymorphismOptions } } + /// + /// 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 { get; private set; } + + /// + /// Locks the current instance for further modification. + /// + /// This method is idempotent. + public void MakeReadOnly() => IsReadOnly = true; + private protected JsonPolymorphismOptions? _polymorphismOptions; internal object? CreateObjectWithArgs { get; set; } @@ -477,7 +494,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, JsonSerializerOptions internal void VerifyMutable() { - if (_isConfigured || IsReadOnly) + if (IsReadOnly) { ThrowHelper.ThrowInvalidOperationException_TypeInfoImmutable(); } @@ -489,8 +506,6 @@ internal void VerifyMutable() internal bool IsConfigured => _isConfigured; - internal bool IsReadOnly { get; set; } - internal void EnsureConfigured() { Debug.Assert(!Monitor.IsEntered(_configureLock), "recursive locking detected."); @@ -528,6 +543,7 @@ internal void Configure() { Debug.Assert(Monitor.IsEntered(_configureLock), "Configure called directly, use EnsureConfigured which locks this method"); + IsReadOnly = true; if (!Options.IsReadOnly) { Options.MakeReadOnly(); @@ -695,6 +711,7 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o /// A blank instance. /// or is null. /// cannot be used for serialization. + /// The instance has been locked for further modification. [RequiresUnreferencedCode(MetadataFactoryRequiresUnreferencedCode)] [RequiresDynamicCode(MetadataFactoryRequiresUnreferencedCode)] public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) @@ -714,6 +731,8 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); } + VerifyMutable(); + JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType); propertyInfo.Name = name; 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 a36fa305c166ce..edce3f1655eef7 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 @@ -26,6 +26,7 @@ public static void PropertyMetadataIsImmutable() { JsonTypeInfo typeInfo = PersonJsonContext.Default.Person; + Assert.True(typeInfo.IsReadOnly); Assert.Throws(() => typeInfo.CreateObject = null); Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); Assert.Throws(() => typeInfo.Properties.Clear()); @@ -42,6 +43,7 @@ public static void JsonSerializerContext_GetTypeInfo_MetadataIsImmutable() { JsonTypeInfo typeInfo = (JsonTypeInfo)PersonJsonContext.Default.GetTypeInfo(typeof(Person)); + Assert.True(typeInfo.IsReadOnly); Assert.Throws(() => typeInfo.CreateObject = null); Assert.Throws(() => typeInfo.OnDeserializing = obj => { }); Assert.Throws(() => typeInfo.Properties.Clear()); @@ -60,9 +62,11 @@ public static void IJsonTypeInfoResolver_GetTypeInfo_MetadataIsMutable() JsonTypeInfo typeInfo = (JsonTypeInfo)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options); Assert.NotSame(typeInfo, PersonJsonContext.Default.Person); + Assert.False(typeInfo.IsReadOnly); JsonTypeInfo typeInfo2 = (JsonTypeInfo)resolver.GetTypeInfo(typeof(Person), PersonJsonContext.Default.Options); Assert.NotSame(typeInfo, typeInfo2); + Assert.False(typeInfo.IsReadOnly); typeInfo.CreateObject = null; typeInfo.OnDeserializing = obj => { }; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs index a4a08fc290d629..33b46dd6163701 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/MetadataTests/DefaultJsonTypeInfoResolverTests.JsonTypeInfo.cs @@ -36,6 +36,7 @@ public static void TypeInfoPropertiesDefaults(Type type) JsonTypeInfo ti = r.GetTypeInfo(type, o); + Assert.False(ti.IsReadOnly); Assert.Same(o, ti.Options); Assert.NotNull(ti.Properties); @@ -400,14 +401,20 @@ private static void TestTypeInfoImmutability(JsonTypeInfo typeInfo) Assert.Equal(typeof(T), typeInfo.Type); Assert.True(typeInfo.Converter.CanConvert(typeof(T))); - JsonPropertyInfo prop = typeInfo.CreateJsonPropertyInfo(typeof(string), "foo"); + Assert.True(typeInfo.IsReadOnly); Assert.True(typeInfo.Properties.IsReadOnly); + Assert.Throws(() => typeInfo.CreateJsonPropertyInfo(typeof(string), "foo")); Assert.Throws(() => untyped.CreateObject = untyped.CreateObject); Assert.Throws(() => typeInfo.CreateObject = typeInfo.CreateObject); Assert.Throws(() => typeInfo.NumberHandling = typeInfo.NumberHandling); Assert.Throws(() => typeInfo.Properties.Clear()); - Assert.Throws(() => typeInfo.Properties.Add(prop)); - Assert.Throws(() => typeInfo.Properties.Insert(0, prop)); + + if (typeInfo.Properties.Count > 0) + { + Assert.Throws(() => typeInfo.Properties.RemoveAt(0)); + Assert.Throws(() => typeInfo.Properties.Add(typeInfo.Properties[0])); + } + Assert.Throws(() => typeInfo.PolymorphismOptions = null); Assert.Throws(() => typeInfo.PolymorphismOptions = new()); 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 da548831a1a7a1..1c85fcfe105bcc 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 @@ -1045,9 +1045,11 @@ public static void GetTypeInfo_MutableOptionsInstance(Type type) options.TypeInfoResolver = new DefaultJsonTypeInfoResolver(); JsonTypeInfo typeInfo = options.GetTypeInfo(type); Assert.Equal(type, typeInfo.Type); + Assert.False(typeInfo.IsReadOnly); JsonTypeInfo typeInfo2 = options.GetTypeInfo(type); Assert.Equal(type, typeInfo2.Type); + Assert.False(typeInfo2.IsReadOnly); Assert.NotSame(typeInfo, typeInfo2); @@ -1066,6 +1068,7 @@ public static void GetTypeInfo_ImmutableOptionsInstance(Type type) JsonTypeInfo typeInfo = options.GetTypeInfo(type); Assert.Equal(type, typeInfo.Type); + Assert.True(typeInfo.IsReadOnly); JsonTypeInfo typeInfo2 = options.GetTypeInfo(type); Assert.Same(typeInfo, typeInfo2); @@ -1077,6 +1080,7 @@ public static void GetTypeInfo_MutableOptions_CanModifyMetadata() var options = new JsonSerializerOptions { TypeInfoResolver = new DefaultJsonTypeInfoResolver() }; JsonTypeInfo jti = (JsonTypeInfo)options.GetTypeInfo(typeof(TestClassForEncoding)); + Assert.False(jti.IsReadOnly); Assert.Equal(1, jti.Properties.Count); jti.Properties.Clear(); @@ -1088,11 +1092,13 @@ public static void GetTypeInfo_MutableOptions_CanModifyMetadata() // Using JsonTypeInfo will lock JsonSerializerOptions Assert.True(options.IsReadOnly); + Assert.True(jti.IsReadOnly); Assert.Throws(() => options.IncludeFields = false); // Getting JsonTypeInfo now should return a fresh immutable instance JsonTypeInfo jti2 = (JsonTypeInfo)options.GetTypeInfo(typeof(TestClassForEncoding)); Assert.NotSame(jti, jti2); + Assert.True(jti2.IsReadOnly); Assert.Equal(1, jti2.Properties.Count); Assert.Throws(() => jti2.Properties.Clear());