From c21865abd97595e09276d8caf4a14f359f3e681a Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Wed, 12 Oct 2022 06:30:32 +0100 Subject: [PATCH] [release/7.0] Ensure source generated metadata properties are read-only. (#76540) (#76899) * Ensure source generated metadata properties are read-only. (#76540) * Add JsonTypeInfo.IsReadOnly/MakeReadOnly() APIs. --- .../gen/JsonSourceGenerator.Emitter.cs | 36 +++++------ .../System.Text.Json/ref/System.Text.Json.cs | 2 + .../src/Resources/Strings.resx | 6 +- .../Metadata/JsonPropertyInfo.cs | 2 +- .../Serialization/Metadata/JsonTypeInfo.cs | 24 ++++++- .../JsonSerializerContextTests.cs | 64 +++++++++++++++++++ ...tJsonTypeInfoResolverTests.JsonTypeInfo.cs | 15 ++++- .../Serialization/OptionsTests.cs | 6 ++ 8 files changed, 127 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs index 68b0bf76bf975..d9eef23cad5d5 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"; @@ -1015,6 +1013,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)} @@ -1090,16 +1090,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) + {{ + {JsonTypeInfoReturnValueLocalVariableName}.{MakeReadOnlyMethodName}(); + }} + return {JsonTypeInfoReturnValueLocalVariableName}; }} {additionalSource}"; @@ -1276,30 +1279,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 0cb93468eb15c..b31df9113c603 100644 --- a/src/libraries/System.Text.Json/ref/System.Text.Json.cs +++ b/src/libraries/System.Text.Json/ref/System.Text.Json.cs @@ -1190,7 +1190,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/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 838a56c7e10cc..3d8d5ece69ebc 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. 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 dee80c211cbc9..52a40a61567cc 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 @@ -268,7 +268,7 @@ internal static JsonPropertyInfo GetPropertyPlaceholder() private protected void VerifyMutable() { - if (_isConfigured) + 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 38868e5c1e7ab..c2c57d5b0c9ec 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; } @@ -482,7 +499,7 @@ internal JsonTypeInfo(Type type, JsonConverter converter, JsonSerializerOptions internal void VerifyMutable() { - if (_isConfigured) + if (IsReadOnly) { ThrowHelper.ThrowInvalidOperationException_TypeInfoImmutable(); } @@ -516,6 +533,7 @@ void ConfigureLocked() { Configure(); + IsReadOnly = true; _isConfigured = true; } catch (Exception e) @@ -699,6 +717,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) @@ -718,6 +737,7 @@ public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); } + VerifyMutable(); JsonPropertyInfo propertyInfo = CreatePropertyUsingReflection(propertyType); propertyInfo.Name = name; @@ -754,6 +774,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 b89fcf34ed5ce..edce3f1655eef 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,70 @@ public static void VariousNestingAndVisibilityLevelsAreSupported() Assert.NotNull(NestedPublicContext.NestedProtectedInternalClass.Default); } + [Fact] + 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()); + + 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.True(typeInfo.IsReadOnly); + 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); + 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 => { }; + + 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] public static void VariousGenericsAreSupported() { 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 a4a08fc290d62..03992b2767cc3 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,17 +401,25 @@ 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.CreateJsonPropertyInfo(typeof(string), "foo")); Assert.Throws(() => typeInfo.Properties.Clear()); - Assert.Throws(() => typeInfo.Properties.Add(prop)); - Assert.Throws(() => typeInfo.Properties.Insert(0, prop)); Assert.Throws(() => typeInfo.PolymorphismOptions = null); Assert.Throws(() => typeInfo.PolymorphismOptions = new()); + if (typeInfo.Properties.Count > 0) + { + JsonPropertyInfo prop = typeInfo.Properties[0]; + Assert.Throws(() => typeInfo.Properties.Add(prop)); + Assert.Throws(() => typeInfo.Properties.Insert(0, prop)); + Assert.Throws(() => typeInfo.Properties.RemoveAt(0)); + } + if (typeInfo.PolymorphismOptions is JsonPolymorphismOptions jpo) { Assert.True(jpo.DerivedTypes.IsReadOnly); 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 05cd443bccf5a..4cde8efc24dac 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 @@ -994,9 +994,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); @@ -1015,6 +1017,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); @@ -1026,6 +1029,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(); @@ -1034,11 +1038,13 @@ public static void GetTypeInfo_MutableOptions_CanModifyMetadata() Assert.Equal("{}", json); // Using JsonTypeInfo will lock JsonSerializerOptions + 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());