From a0f5c3e4110b80404a85a13280aafe73696d65c9 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Tue, 26 Jul 2022 16:17:41 +0200 Subject: [PATCH 01/11] Throw exception when 'required' keyword used but ctor does not have 'SetsRequiredMembers' --- .../src/Resources/Strings.resx | 5 +- .../src/System/ReflectionExtensions.cs | 1 + .../Object/KeyValuePairConverter.cs | 2 + .../Metadata/JsonPropertyInfo.cs | 12 + .../Serialization/Metadata/JsonTypeInfo.cs | 15 +- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 22 ++ .../Metadata/SourceGenJsonTypeInfoOfT.cs | 4 +- .../Text/Json/ThrowHelper.Serialization.cs | 20 +- .../Serialization/RequiredKeywordTests.cs | 267 ++++++++++++++++++ .../System.Text.Json.Tests.csproj | 1 + 10 files changed, 341 insertions(+), 8 deletions(-) create mode 100644 src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index f4f1cc96e76d51..9fa80fe3e79946 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -659,4 +659,7 @@ Parameter already associated with a different JsonTypeInfo instance. - + + The JSON property '{0}.{1}' is marked with 'required' keyword which is not supported. + + \ No newline at end of file diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index 9a5d6f4bb610d9..7dea62529075a1 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.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.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs index 48c6ab85ae2b34..4ae03c91cc7059 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs @@ -3,7 +3,9 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Diagnostics.CodeAnalysis; using System.Reflection; +using System.Text.Json.Reflection; namespace System.Text.Json.Serialization.Converters { 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 128ebf2356c813..772140a0083c32 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 @@ -203,6 +203,18 @@ public bool IsExtensionData private bool _isExtensionDataProperty; + internal bool IsRequired + { + get => _isRequired; + set + { + VerifyMutable(); + _isRequired = value; + } + } + + private bool _isRequired; + internal JsonPropertyInfo(Type declaringType, Type propertyType, JsonTypeInfo? declaringTypeInfo, JsonSerializerOptions options) { Debug.Assert(declaringTypeInfo is null || declaringTypeInfo.Type == declaringType); 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 99e75e28c6be70..c0219c6586b9a4 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 @@ -269,16 +269,17 @@ public JsonPolymorphismOptions? PolymorphismOptions // Avoids having to perform an expensive cast to JsonTypeInfo to check if there is a Serialize method. internal bool HasSerializeHandler { get; private protected set; } - // Configure would normally have thrown why initializing properties for source gen but type had SerializeHandler + // Exception used to throw on deserialization. In some scenarios i.e.: + // configure would have thrown while initializing properties for source gen but type had SerializeHandler // so it is allowed to be used for fast-path serialization but it will throw if used for metadata-based serialization - internal bool MetadataSerializationNotSupported { get; private protected set; } + internal Exception? DeserializationException { get; private protected set; } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void ValidateCanBeUsedForMetadataSerialization() { - if (MetadataSerializationNotSupported) + if (DeserializationException != null) { - ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); + throw DeserializationException; } } @@ -882,6 +883,12 @@ internal void InitializePropertyCache() { JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; + if (jsonPropertyInfo.IsRequired) + { + // Deserialization is not possible if member has 'required' keyword + DeserializationException = ThrowHelper.GetNotSupportedException_MemberWithRequiredKeywordNotSupported(jsonPropertyInfo); + } + jsonPropertyInfo.EnsureChildOf(this); jsonPropertyInfo.EnsureConfigured(); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index 89fe3eade4f775..2d0ffae3b19866 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -5,6 +5,7 @@ using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection; +using System.Runtime.CompilerServices; using System.Text.Json.Reflection; namespace System.Text.Json.Serialization.Metadata @@ -26,6 +27,27 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o if (PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object) { AddPropertiesAndParametersUsingReflection(); + +#if NET7_0_OR_GREATER + bool typeHasRequiredMemberAttribute = typeof(T).GetCustomAttribute() != null; + // Compiler adds RequiredMemberAttribute if any of the members is marked with 'required' keyword. + // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement + bool shouldCheckMembersForRequiredMemberAttribute = typeHasRequiredMemberAttribute + && !(converter.ConstructorInfo?.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true) ?? false); + + if (shouldCheckMembersForRequiredMemberAttribute) + { + Debug.Assert(PropertyCache != null); + foreach (var (_, property) in PropertyCache.List) + { + Debug.Assert(property.AttributeProvider != null); + if (property.AttributeProvider.IsDefined(typeof(RequiredMemberAttribute), inherit: true)) + { + property.IsRequired = true; + } + } + } +#endif } Func? createObject = Options.MemberAccessorStrategy.CreateConstructor(typeof(T)); 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 72df4963cd2e78..3a3b6667bdca6d 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 @@ -113,7 +113,7 @@ internal override JsonParameterInfoValues[] GetParameterInfoValues() } array = Array.Empty(); - MetadataSerializationNotSupported = true; + DeserializationException = ThrowHelper.GetInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); } return array; @@ -149,7 +149,7 @@ internal override void LateAddProperties() ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); } - MetadataSerializationNotSupported = true; + DeserializationException = ThrowHelper.GetInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); return; } 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 dee7b47adaa160..98f0458b2bae11 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 @@ -656,11 +656,18 @@ public static void ThrowInvalidOperationException_NoMetadataForType(Type type, I throw new InvalidOperationException(SR.Format(SR.NoMetadataForType, type, resolver?.GetType().FullName ?? "")); } + public static Exception GetInvalidOperationException_NoMetadataForTypeProperties(IJsonTypeInfoResolver? resolver, Type type) + { + return new InvalidOperationException(SR.Format(SR.NoMetadataForTypeProperties, resolver?.GetType().FullName ?? "", type)); + } + + [DoesNotReturn] public static void ThrowInvalidOperationException_NoMetadataForTypeProperties(IJsonTypeInfoResolver? resolver, Type type) { - throw new InvalidOperationException(SR.Format(SR.NoMetadataForTypeProperties, resolver?.GetType().FullName ?? "", type)); + throw GetInvalidOperationException_NoMetadataForTypeProperties(resolver, type); } + [DoesNotReturn] public static void ThrowInvalidOperationException_NoMetadataForTypeCtorParams(IJsonTypeInfoResolver? resolver, Type type) { throw new InvalidOperationException(SR.Format(SR.NoMetadataForTypeCtorParams, resolver?.GetType().FullName ?? "", type)); @@ -684,6 +691,17 @@ public static void ThrowNotSupportedException_DerivedConverterDoesNotSupportMeta throw new NotSupportedException(SR.Format(SR.Polymorphism_DerivedConverterDoesNotSupportMetadata, derivedType)); } + public static Exception GetNotSupportedException_MemberWithRequiredKeywordNotSupported(JsonPropertyInfo property) + { + return new NotSupportedException(SR.Format(SR.MemberWithRequiredKeywordNotSupported, property.DeclaringType, property.Name)); + } + + [DoesNotReturn] + public static void ThrowNotSupportedException_MemberWithRequiredKeywordNotSupported(JsonPropertyInfo property) + { + throw GetNotSupportedException_MemberWithRequiredKeywordNotSupported(property); + } + [DoesNotReturn] public static void ThrowNotSupportedException_RuntimeTypeNotSupported(Type baseType, Type runtimeType) { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs new file mode 100644 index 00000000000000..50a4e0ac83eb6c --- /dev/null +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs @@ -0,0 +1,267 @@ +// 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.Linq; +using System.Text.Json.Serialization.Metadata; +using Xunit; + +namespace System.Text.Json.Serialization.Tests +{ +#if !NETFRAMEWORK + public static partial class RequiredKeywordTests + { + [Fact] + public static void ClassWithRequiredKeywordFailsDeserialization() + { + var obj = new PersonWithRequiredMembers() + { + FirstName = "foo", + LastName = "bar" + }; + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + + json = """{"LastName":"bar"}"""; + // Related: https://github.com/dotnet/runtime/issues/29861 + Assert.Throws(() => JsonSerializer.Deserialize(json)); + } + + private class PersonWithRequiredMembers + { + public required string FirstName { get; set; } + public string MiddleName { get; set; } = ""; + public required string LastName { get; set; } + } + + [Fact] + public static void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeserialization() + { + var obj = new PersonWithRequiredMembersAndSmallParametrizedCtor("badfoo", "badbar") + { + // note: these must be set during initialize or otherwise we get compiler errors + FirstName = "foo", + LastName = "bar" + }; + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + + json = """{"LastName":"bar"}"""; + // Related: https://github.com/dotnet/runtime/issues/29861 + Assert.Throws(() => JsonSerializer.Deserialize(json)); + } + + private class PersonWithRequiredMembersAndSmallParametrizedCtor + { + public required string FirstName { get; set; } + public string MiddleName { get; set; } = ""; + public required string LastName { get; set; } + + public PersonWithRequiredMembersAndSmallParametrizedCtor(string firstName, string lastName) + { + FirstName = firstName; + LastName = lastName; + } + } + + [Fact] + public static void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeserialization() + { + var obj = new PersonWithRequiredMembersAndLargeParametrizedCtor("bada", "badb", "badc", "badd", "bade", "badf", "badg") + { + // note: these must be set during initialize or otherwise we get compiler errors + A = "a", + B = "b", + C = "c", + D = "d", + E = "e", + F = "f", + G = "g", + }; + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"A":"a","B":"b","C":"c","D":"d","E":"e","F":"f","G":"g"}""", json); + + // Related: https://github.com/dotnet/runtime/issues/29861 + Assert.Throws(() => JsonSerializer.Deserialize(json)); + } + + private class PersonWithRequiredMembersAndLargeParametrizedCtor + { + public required string A { get; set; } + public required string B { get; set; } + public required string C { get; set; } + public required string D { get; set; } + public required string E { get; set; } + public required string F { get; set; } + public required string G { get; set; } + + public PersonWithRequiredMembersAndLargeParametrizedCtor(string a, string b, string c, string d, string e, string f, string g) + { + A = a; + B = b; + C = c; + D = d; + E = e; + F = f; + G = g; + } + } + + [Fact] + public static void ClassWithRequiredKeywordAndSetsRequiredMembersOnCtorWorks() + { + var obj = new PersonWithRequiredMembersAndSetsRequiredMembers() + { + FirstName = "foo", + LastName = "bar" + }; + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + + json = """{"LastName":"bar"}"""; + var deserialized = JsonSerializer.Deserialize(json); + Assert.Equal("", deserialized.FirstName); + Assert.Equal("", deserialized.MiddleName); + Assert.Equal("bar", deserialized.LastName); + } + + private class PersonWithRequiredMembersAndSetsRequiredMembers + { + public required string FirstName { get; set; } + public string MiddleName { get; set; } = ""; + public required string LastName { get; set; } + + [SetsRequiredMembers] + public PersonWithRequiredMembersAndSetsRequiredMembers() + { + FirstName = ""; + LastName = ""; + } + } + + [Fact] + public static void ClassWithRequiredKeywordSmallParametrizedCtorAndSetsRequiredMembersOnCtorWorks() + { + var obj = new PersonWithRequiredMembersAndSmallParametrizedCtorAndSetsRequiredMembers("foo", "bar"); + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + + var deserialized = JsonSerializer.Deserialize(json); + Assert.Equal("foo", deserialized.FirstName); + Assert.Equal("", deserialized.MiddleName); + Assert.Equal("bar", deserialized.LastName); + } + + private class PersonWithRequiredMembersAndSmallParametrizedCtorAndSetsRequiredMembers + { + public required string FirstName { get; init; } + public string MiddleName { get; init; } = ""; + public required string LastName { get; init; } + + [SetsRequiredMembers] + public PersonWithRequiredMembersAndSmallParametrizedCtorAndSetsRequiredMembers(string firstName, string lastName) + { + FirstName = firstName; + LastName = lastName; + } + } + + [Fact] + public static void ClassWithRequiredKeywordLargeParametrizedCtorAndSetsRequiredMembersOnCtorWorks() + { + var obj = new PersonWithRequiredMembersAndLargeParametrizedCtorndSetsRequiredMembers("a", "b", "c", "d", "e", "f", "g"); + + string json = JsonSerializer.Serialize(obj); + Assert.Equal("""{"A":"a","B":"b","C":"c","D":"d","E":"e","F":"f","G":"g"}""", json); + + var deserialized = JsonSerializer.Deserialize(json); + Assert.Equal("a", deserialized.A); + Assert.Equal("b", deserialized.B); + Assert.Equal("c", deserialized.C); + Assert.Equal("d", deserialized.D); + Assert.Equal("e", deserialized.E); + Assert.Equal("f", deserialized.F); + Assert.Equal("g", deserialized.G); + } + + private class PersonWithRequiredMembersAndLargeParametrizedCtorndSetsRequiredMembers + { + public required string A { get; set; } + public required string B { get; set; } + public required string C { get; set; } + public required string D { get; set; } + public required string E { get; set; } + public required string F { get; set; } + public required string G { get; set; } + + [SetsRequiredMembers] + public PersonWithRequiredMembersAndLargeParametrizedCtorndSetsRequiredMembers(string a, string b, string c, string d, string e, string f, string g) + { + A = a; + B = b; + C = c; + D = d; + E = e; + F = f; + G = g; + } + } + + [Fact] + public static void RemovingRequiredPropertiesAllowsDeserialization() + { + JsonSerializerOptions options = new() + { + TypeInfoResolver = new DefaultJsonTypeInfoResolver() + { + Modifiers = + { + (ti) => + { + for (int i = 0; i < ti.Properties.Count; i++) + { + if (ti.Properties[i].Name == nameof(PersonWithRequiredMembers.FirstName)) + { + JsonPropertyInfo property = ti.CreateJsonPropertyInfo(typeof(string), nameof(PersonWithRequiredMembers.FirstName)); + property.Get = (obj) => ((PersonWithRequiredMembers)obj).FirstName; + property.Set = (obj, val) => ((PersonWithRequiredMembers)obj).FirstName = (string)val; + ti.Properties[i] = property; + } + else if (ti.Properties[i].Name == nameof(PersonWithRequiredMembers.LastName)) + { + JsonPropertyInfo property = ti.CreateJsonPropertyInfo(typeof(string), nameof(PersonWithRequiredMembers.LastName)); + property.Get = (obj) => ((PersonWithRequiredMembers)obj).LastName; + property.Set = (obj, val) => ((PersonWithRequiredMembers)obj).LastName = (string)val; + ti.Properties[i] = property; + } + } + } + } + } + }; + + var obj = new PersonWithRequiredMembers() + { + FirstName = "foo", + LastName = "bar" + }; + + string json = JsonSerializer.Serialize(obj, options); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + + json = """{"LastName":"bar"}"""; + PersonWithRequiredMembers deserialized = JsonSerializer.Deserialize(json, options); + Assert.Null(deserialized.FirstName); + Assert.Equal("", deserialized.MiddleName); + Assert.Equal("bar", deserialized.LastName); + } + } +#endif +} diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj index d68f600d0ca832..49d8daf97de312 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj @@ -159,6 +159,7 @@ + From ae9b84b4e63524a39494004d97acd700f11c81cb Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Wed, 27 Jul 2022 14:58:56 +0200 Subject: [PATCH 02/11] Support required keyword (and internal IsRequired) --- .../src/Resources/Strings.resx | 15 +- .../Object/ObjectDefaultConverter.cs | 4 + ...ParameterizedConstructorConverter.Large.cs | 1 + ...ParameterizedConstructorConverter.Small.cs | 12 +- ...ctWithParameterizedConstructorConverter.cs | 6 +- .../Metadata/JsonParameterInfo.cs | 3 + .../Metadata/JsonPropertyInfo.cs | 15 + .../Metadata/JsonPropertyInfoOfT.cs | 17 +- .../Serialization/Metadata/JsonTypeInfo.cs | 46 +- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 13 +- .../Text/Json/Serialization/ReadStackFrame.cs | 12 + .../Text/Json/ThrowHelper.Serialization.cs | 39 +- .../Serialization/RequiredKeywordTests.cs | 396 +++++++++++++++--- 13 files changed, 505 insertions(+), 74 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 9fa80fe3e79946..d907293d173a2c 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -659,7 +659,16 @@ Parameter already associated with a different JsonTypeInfo instance. - - The JSON property '{0}.{1}' is marked with 'required' keyword which is not supported. + + JsonPropertyInfo with name '{0}' for type '{1}' is required but is not deserializable. - \ No newline at end of file + + JsonPropertyInfo with name '{0}' for type '{1}' cannot be required and be an extension data property. + + + Type '{0}' contains required properties which were not set during deserialization: + + + - {0} + + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 9871d76f83d5b0..64700f4d48aba4 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -39,6 +39,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, obj = jsonTypeInfo.CreateObject()!; jsonTypeInfo.OnDeserializing?.Invoke(obj); + jsonTypeInfo.InitializeRequiredProperties(ref state); // Process all properties. while (true) @@ -143,6 +144,8 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, state.Current.ReturnValue = obj; state.Current.ObjectState = StackFrameObjectState.CreatedObject; + + jsonTypeInfo.InitializeRequiredProperties(ref state); } else { @@ -250,6 +253,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, } jsonTypeInfo.OnDeserialized?.Invoke(obj); + jsonTypeInfo.CheckRequiredProperties(ref state); // Unbox Debug.Assert(obj != null); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index a369e8a349bcf6..4bb5c1eed50618 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -24,6 +24,7 @@ protected sealed override bool ReadAndCacheConstructorArgument(ref ReadStack sta if (success && !(arg == null && jsonParameterInfo.IgnoreNullTokensOnRead)) { ((object[])state.Current.CtorArgumentState!.Arguments)[jsonParameterInfo.ClrInfo.Position] = arg!; + state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty); } return success; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index 913198dd364199..053f3330ec3e3c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -67,9 +67,15 @@ private static bool TryRead( bool success = converter.TryRead(ref reader, info.PropertyType, info.Options!, ref state, out TArg? value); - arg = value == null && jsonParameterInfo.IgnoreNullTokensOnRead - ? (TArg?)info.DefaultValue! // Use default value specified on parameter, if any. - : value!; + if (value == null && jsonParameterInfo.IgnoreNullTokensOnRead) + { + arg = (TArg?)info.DefaultValue!; // Use default value specified on parameter, if any. + } + else + { + arg = value!; + state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty); + } return success; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index 9a2b85469f9821..bb0a35a0f36329 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -184,7 +184,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo if (dataExtKey == null) { - jsonPropertyInfo.SetExtensionDictionaryAsObject(obj, propValue); + jsonPropertyInfo.SetValueAsObject(obj, propValue, ref state); } else { @@ -211,6 +211,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo } jsonTypeInfo.OnDeserialized?.Invoke(obj); + jsonTypeInfo.CheckRequiredProperties(ref state); // Unbox Debug.Assert(obj != null); @@ -272,6 +273,7 @@ private void ReadConstructorArguments(ref ReadStack state, ref Utf8JsonReader re continue; } + Debug.Assert(jsonParameterInfo.MatchingProperty != null); ReadAndCacheConstructorArgument(ref state, ref reader, jsonParameterInfo); state.Current.EndConstructorParameter(); @@ -532,6 +534,8 @@ private void BeginRead(ref ReadStack state, ref Utf8JsonReader reader, JsonSeria ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(TypeToConvert); } + jsonTypeInfo.InitializeRequiredProperties(ref state); + // Set current JsonPropertyInfo to null to avoid conflicts on push. state.Current.JsonPropertyInfo = null; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs index 247f58db58cdc3..2f9ea58106e13c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonParameterInfo.cs @@ -53,8 +53,11 @@ public JsonTypeInfo JsonTypeInfo public bool ShouldDeserialize { get; private set; } + public JsonPropertyInfo MatchingProperty { get; private set; } = null!; + public virtual void Initialize(JsonParameterInfoValues parameterInfo, JsonPropertyInfo matchingProperty, JsonSerializerOptions options) { + MatchingProperty = matchingProperty; ClrInfo = parameterInfo; Options = options; ShouldDeserialize = 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 772140a0083c32..4c975029ff0eec 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 @@ -291,6 +291,19 @@ internal void Configure() DetermineIgnoreCondition(); DetermineSerializationCapabilities(); } + + if (IsRequired) + { + if (!CanDeserialize) + { + ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(this); + } + + if (IsExtensionData) + { + ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndExtensionData(this); + } + } } private protected abstract void DetermineEffectiveConverter(JsonTypeInfo jsonTypeInfo); @@ -774,6 +787,8 @@ internal JsonTypeInfo JsonTypeInfo internal abstract void SetExtensionDictionaryAsObject(object obj, object? extensionDict); + internal abstract void SetValueAsObject(object obj, object? value, ref ReadStack state); + internal bool IsIgnored => _ignoreCondition == JsonIgnoreCondition.Always; /// diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index 31d0e699ef78d8..e7290d6975e755 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -339,6 +339,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref { T? value = default; Set!(obj, value!); + state.Current.MarkRequiredPropertyAsRead(this); } success = true; @@ -353,6 +354,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref // Optimize for internal converters by avoiding the extra call to TryRead. T? fastValue = TypedEffectiveConverter.Read(ref reader, PropertyType, Options); Set!(obj, fastValue!); + state.Current.MarkRequiredPropertyAsRead(this); } success = true; @@ -366,6 +368,7 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref if (success) { Set!(obj, value!); + state.Current.MarkRequiredPropertyAsRead(this); } } } @@ -408,13 +411,25 @@ internal override bool ReadJsonAsObject(ref ReadStack state, ref Utf8JsonReader return success; } - internal override void SetExtensionDictionaryAsObject(object obj, object? extensionDict) + internal sealed override void SetExtensionDictionaryAsObject(object obj, object? extensionDict) { Debug.Assert(HasSetter); T typedValue = (T)extensionDict!; Set!(obj, typedValue); } + internal sealed override void SetValueAsObject(object obj, object? value, ref ReadStack state) + { + Debug.Assert(HasSetter); + + if (value is not null || !IgnoreNullTokensOnRead || default(T) is not null) + { + T typedValue = (T)value!; + Set!(obj, typedValue); + state.Current.MarkRequiredPropertyAsRead(this); + } + } + private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ignoreCondition) { switch (ignoreCondition) 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 c0219c6586b9a4..e37e1e98995c1e 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 @@ -25,6 +25,7 @@ public abstract partial class JsonTypeInfo internal delegate T ParameterizedConstructorDelegate(TArg0 arg0, TArg1 arg1, TArg2 arg2, TArg3 arg3); private JsonPropertyInfoList? _properties; + internal JsonPropertyInfo[]? RequiredProperties { get; private set; } private Action? _onSerializing; private Action? _onSerialized; @@ -879,19 +880,36 @@ internal void InitializePropertyCache() ExtensionDataProperty.EnsureConfigured(); } + int numberOfRequiredProperties = 0; foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) { JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; if (jsonPropertyInfo.IsRequired) { - // Deserialization is not possible if member has 'required' keyword - DeserializationException = ThrowHelper.GetNotSupportedException_MemberWithRequiredKeywordNotSupported(jsonPropertyInfo); + numberOfRequiredProperties++; } jsonPropertyInfo.EnsureChildOf(this); jsonPropertyInfo.EnsureConfigured(); } + + if (numberOfRequiredProperties > 0) + { + JsonPropertyInfo[] requiredProperties = new JsonPropertyInfo[numberOfRequiredProperties]; + int idx = 0; + foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) + { + JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; + + if (jsonPropertyInfo.IsRequired) + { + requiredProperties[idx++] = jsonPropertyInfo; + } + } + + RequiredProperties = requiredProperties; + } } internal void InitializeConstructorParameters(JsonParameterInfoValues[] jsonParameters, bool sourceGenMode = false) @@ -1009,6 +1027,30 @@ internal JsonPropertyDictionary CreatePropertyCache(int capaci return new JsonPropertyDictionary(Options.PropertyNameCaseInsensitive, capacity); } + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void InitializeRequiredProperties(ref ReadStack state) + { + if (RequiredProperties != null) + { + Debug.Assert(state.Current.RequiredPropertiesLeft == null); + state.Current.RequiredPropertiesLeft = new HashSet(RequiredProperties); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void CheckRequiredProperties(ref ReadStack state) + { + if (RequiredProperties != null) + { + Debug.Assert(state.Current.RequiredPropertiesLeft != null); + + if (state.Current.RequiredPropertiesLeft.Count != 0) + { + ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(this, state.Current.RequiredPropertiesLeft); + } + } + } + private static JsonParameterInfo CreateConstructorParameter( JsonParameterInfoValues parameterInfo, JsonPropertyInfo jsonPropertyInfo, diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index 2d0ffae3b19866..fd072ddd8043a9 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -29,7 +29,7 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o AddPropertiesAndParametersUsingReflection(); #if NET7_0_OR_GREATER - bool typeHasRequiredMemberAttribute = typeof(T).GetCustomAttribute() != null; + bool typeHasRequiredMemberAttribute = typeof(T).IsDefined(typeof(RequiredMemberAttribute), inherit: true); // Compiler adds RequiredMemberAttribute if any of the members is marked with 'required' keyword. // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement bool shouldCheckMembersForRequiredMemberAttribute = typeHasRequiredMemberAttribute @@ -46,6 +46,17 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o property.IsRequired = true; } } + + if (ExtensionDataProperty != null) + { + Debug.Assert(ExtensionDataProperty.AttributeProvider != null); + if (ExtensionDataProperty.AttributeProvider.IsDefined(typeof(RequiredMemberAttribute), inherit: true)) + { + // This will end up in error in Configure + // but we need to give contract resolver a chance to fix this + ExtensionDataProperty.IsRequired = true; + } + } } #endif } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 253ebbbd170e6c..8dec1ff57e1ed5 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -66,6 +66,9 @@ public JsonTypeInfo BaseJsonTypeInfo // Whether to use custom number handling. public JsonNumberHandling? NumberHandling; + // Required properties left + public HashSet? RequiredPropertiesLeft; + public void EndConstructorParameter() { CtorArgumentState!.JsonParameterInfo = null; @@ -107,6 +110,15 @@ public bool IsProcessingEnumerable() return (JsonTypeInfo.PropertyInfoForTypeInfo.ConverterStrategy & ConverterStrategy.Enumerable) != 0; } + public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) + { + if (propertyInfo.IsRequired) + { + Debug.Assert(RequiredPropertiesLeft != null); + RequiredPropertiesLeft.Remove(propertyInfo); + } + } + [DebuggerBrowsable(DebuggerBrowsableState.Never)] private string DebuggerDisplay => $"ConverterStrategy.{JsonTypeInfo?.PropertyInfoForTypeInfo.ConverterStrategy}, {JsonTypeInfo?.Type.Name}"; } 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 98f0458b2bae11..dda9736e8e7148 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 @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; using System.Reflection; @@ -200,6 +201,33 @@ public static void ThrowInvalidOperationException_SerializerPropertyNameNull(Jso throw new InvalidOperationException(SR.Format(SR.SerializerPropertyNameNull, jsonPropertyInfo.DeclaringType, jsonPropertyInfo.MemberName)); } + [DoesNotReturn] + public static void ThrowInvalidOperationException_JsonPropertyRequiredAndNotDeserializable(JsonPropertyInfo jsonPropertyInfo) + { + throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndNotDeserializable, jsonPropertyInfo.Name, jsonPropertyInfo.DeclaringType)); + } + + [DoesNotReturn] + public static void ThrowInvalidOperationException_JsonPropertyRequiredAndExtensionData(JsonPropertyInfo jsonPropertyInfo) + { + throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndExtensionData, jsonPropertyInfo.Name, jsonPropertyInfo.DeclaringType)); + } + + [DoesNotReturn] + public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, IEnumerable missingJsonProperties) + { + StringBuilder errorMessageBuilder = new(); + errorMessageBuilder.AppendLine(SR.Format(SR.JsonRequiredPropertiesMissingHeader, parent.Type)); + + foreach (var missingProperty in missingJsonProperties) + { + Debug.Assert(missingProperty.IsRequired); + errorMessageBuilder.AppendLine(SR.Format(SR.JsonRequiredPropertiesMissingItem, missingProperty.Name)); + } + + throw new JsonException(errorMessageBuilder.ToString()); + } + [DoesNotReturn] public static void ThrowInvalidOperationException_NamingPolicyReturnNull(JsonNamingPolicy namingPolicy) { @@ -691,17 +719,6 @@ public static void ThrowNotSupportedException_DerivedConverterDoesNotSupportMeta throw new NotSupportedException(SR.Format(SR.Polymorphism_DerivedConverterDoesNotSupportMetadata, derivedType)); } - public static Exception GetNotSupportedException_MemberWithRequiredKeywordNotSupported(JsonPropertyInfo property) - { - return new NotSupportedException(SR.Format(SR.MemberWithRequiredKeywordNotSupported, property.DeclaringType, property.Name)); - } - - [DoesNotReturn] - public static void ThrowNotSupportedException_MemberWithRequiredKeywordNotSupported(JsonPropertyInfo property) - { - throw GetNotSupportedException_MemberWithRequiredKeywordNotSupported(property); - } - [DoesNotReturn] public static void ThrowNotSupportedException_RuntimeTypeNotSupported(Type baseType, Type runtimeType) { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs index 50a4e0ac83eb6c..ccda06d991c84f 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs @@ -6,15 +6,65 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Text.Json.Serialization.Metadata; +using Newtonsoft.Json; using Xunit; namespace System.Text.Json.Serialization.Tests { #if !NETFRAMEWORK - public static partial class RequiredKeywordTests + public class RequiredKeywordTests_Span : RequiredKeywordTests { + public RequiredKeywordTests_Span() : base(JsonSerializerWrapper.SpanSerializer) { } + } + + public class RequiredKeywordTests_String : RequiredKeywordTests + { + public RequiredKeywordTests_String() : base(JsonSerializerWrapper.StringSerializer) { } + } + + public class RequiredKeywordTests_AsyncStream : RequiredKeywordTests + { + public RequiredKeywordTests_AsyncStream() : base(JsonSerializerWrapper.AsyncStreamSerializer) { } + } + + public class RequiredKeywordTests_AsyncStreamWithSmallBuffer : RequiredKeywordTests + { + public RequiredKeywordTests_AsyncStreamWithSmallBuffer() : base(JsonSerializerWrapper.AsyncStreamSerializerWithSmallBuffer) { } + } + + public class RequiredKeywordTests_SyncStream : RequiredKeywordTests + { + public RequiredKeywordTests_SyncStream() : base(JsonSerializerWrapper.SyncStreamSerializer) { } + } + + public class RequiredKeywordTests_Writer : RequiredKeywordTests + { + public RequiredKeywordTests_Writer() : base(JsonSerializerWrapper.ReaderWriterSerializer) { } + } + + public class RequiredKeywordTests_Document : RequiredKeywordTests + { + public RequiredKeywordTests_Document() : base(JsonSerializerWrapper.DocumentSerializer) { } + } + + public class RequiredKeywordTests_Element : RequiredKeywordTests + { + public RequiredKeywordTests_Element() : base(JsonSerializerWrapper.ElementSerializer) { } + } + + public class RequiredKeywordTests_Node : RequiredKeywordTests + { + public RequiredKeywordTests_Node() : base(JsonSerializerWrapper.NodeSerializer) { } + } + + public abstract class RequiredKeywordTests : SerializerTests + { + public RequiredKeywordTests(JsonSerializerWrapper serializer) : base(serializer) + { + } + [Fact] - public static void ClassWithRequiredKeywordFailsDeserialization() + public async void ClassWithRequiredKeywordDeserialization() { var obj = new PersonWithRequiredMembers() { @@ -22,12 +72,52 @@ public static void ClassWithRequiredKeywordFailsDeserialization() LastName = "bar" }; - string json = JsonSerializer.Serialize(obj); + string json = await Serializer.SerializeWrapper(obj); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + PersonWithRequiredMembers deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal(obj.FirstName, deserialized.FirstName); + Assert.Equal(obj.MiddleName, deserialized.MiddleName); + Assert.Equal(obj.LastName, deserialized.LastName); + json = """{"LastName":"bar"}"""; - // Related: https://github.com/dotnet/runtime/issues/29861 - Assert.Throws(() => JsonSerializer.Deserialize(json)); + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + + json = """{"LastName":null}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + + json = "{}"; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.Contains("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + } + + [Fact] + public async void IgnoreNullValuesParameterlessCtorNullValuesAreNotAcceptedAsRequired() + { + JsonSerializerOptions options = new() + { + IgnoreNullValues = true, + }; + + string json = """{"FirstName":null,"MiddleName":"","LastName":null}"""; + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.Contains("FirstName", exception.Message); + Assert.Contains("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + + json = """{"LastName":"bar","FirstName":null}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); } private class PersonWithRequiredMembers @@ -38,21 +128,83 @@ private class PersonWithRequiredMembers } [Fact] - public static void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeserialization() + public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeserialization() { var obj = new PersonWithRequiredMembersAndSmallParametrizedCtor("badfoo", "badbar") { // note: these must be set during initialize or otherwise we get compiler errors FirstName = "foo", - LastName = "bar" + LastName = "bar", + Info1 = "info1", + Info2 = "info2", }; - string json = JsonSerializer.Serialize(obj); - Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); + string json = await Serializer.SerializeWrapper(obj); + Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar","Info1":"info1","Info2":"info2"}""", json); + + PersonWithRequiredMembersAndSmallParametrizedCtor deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal(obj.FirstName, deserialized.FirstName); + Assert.Equal(obj.MiddleName, deserialized.MiddleName); + Assert.Equal(obj.LastName, deserialized.LastName); + Assert.Equal(obj.Info1, deserialized.Info1); + Assert.Equal(obj.Info2, deserialized.Info2); + + json = """{"FirstName":"foo","MiddleName":"","LastName":null,"Info1":null,"Info2":"info2"}"""; + deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal(obj.FirstName, deserialized.FirstName); + Assert.Equal(obj.MiddleName, deserialized.MiddleName); + Assert.Null(deserialized.LastName); + Assert.Null(deserialized.Info1); + Assert.Equal(obj.Info2, deserialized.Info2); + + json = """{"LastName":"bar","Info1":"info1"}"""; + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + Assert.DoesNotContain("Info1", exception.Message); + Assert.Contains("Info2", exception.Message); + + json = """{"LastName":null,"Info1":null}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + Assert.DoesNotContain("Info1", exception.Message); + Assert.Contains("Info2", exception.Message); + + json = "{}"; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("FirstName", exception.Message); + Assert.Contains("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + Assert.Contains("Info1", exception.Message); + Assert.Contains("Info2", exception.Message); + } - json = """{"LastName":"bar"}"""; - // Related: https://github.com/dotnet/runtime/issues/29861 - Assert.Throws(() => JsonSerializer.Deserialize(json)); + [Fact] + public async void IgnoreNullValuesSmallParametrizedCtorNullValuesAreNotAcceptedAsRequired() + { + JsonSerializerOptions options = new() + { + IgnoreNullValues = true, + }; + + string json = """{"FirstName":null,"MiddleName":"","LastName":"bar","Info1":"info1","Info2":"info2"}"""; + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.Contains("FirstName", exception.Message); + Assert.DoesNotContain("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + Assert.DoesNotContain("Info1", exception.Message); + Assert.DoesNotContain("Info2", exception.Message); + + json = """{"LastName":null,"FirstName":null,"MiddleName":"","Info1":null,"Info2":null}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.Contains("FirstName", exception.Message); + Assert.Contains("LastName", exception.Message); + Assert.DoesNotContain("MiddleName", exception.Message); + Assert.Contains("Info1", exception.Message); + Assert.Contains("Info2", exception.Message); } private class PersonWithRequiredMembersAndSmallParametrizedCtor @@ -60,6 +212,8 @@ private class PersonWithRequiredMembersAndSmallParametrizedCtor public required string FirstName { get; set; } public string MiddleName { get; set; } = ""; public required string LastName { get; set; } + public required string Info1 { get; set; } + public required string Info2 { get; set; } public PersonWithRequiredMembersAndSmallParametrizedCtor(string firstName, string lastName) { @@ -69,51 +223,133 @@ public PersonWithRequiredMembersAndSmallParametrizedCtor(string firstName, strin } [Fact] - public static void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeserialization() + public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeserialization() { var obj = new PersonWithRequiredMembersAndLargeParametrizedCtor("bada", "badb", "badc", "badd", "bade", "badf", "badg") { // note: these must be set during initialize or otherwise we get compiler errors - A = "a", - B = "b", - C = "c", - D = "d", - E = "e", - F = "f", - G = "g", + AProp = "a", + BProp = "b", + CProp = "c", + DProp = "d", + EProp = "e", + FProp = "f", + GProp = "g", + HProp = "h", + IProp = "i", }; - string json = JsonSerializer.Serialize(obj); - Assert.Equal("""{"A":"a","B":"b","C":"c","D":"d","E":"e","F":"f","G":"g"}""", json); + string json = await Serializer.SerializeWrapper(obj); + Assert.Equal("""{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":"e","FProp":"f","GProp":"g","HProp":"h","IProp":"i"}""", json); + + var deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal(obj.AProp, deserialized.AProp); + Assert.Equal(obj.BProp, deserialized.BProp); + Assert.Equal(obj.CProp, deserialized.CProp); + Assert.Equal(obj.DProp, deserialized.DProp); + Assert.Equal(obj.EProp, deserialized.EProp); + Assert.Equal(obj.FProp, deserialized.FProp); + Assert.Equal(obj.GProp, deserialized.GProp); + Assert.Equal(obj.HProp, deserialized.HProp); + Assert.Equal(obj.IProp, deserialized.IProp); + + json = """{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":null,"FProp":"f","GProp":"g","HProp":null,"IProp":"i"}"""; + deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal(obj.AProp, deserialized.AProp); + Assert.Equal(obj.BProp, deserialized.BProp); + Assert.Equal(obj.CProp, deserialized.CProp); + Assert.Equal(obj.DProp, deserialized.DProp); + Assert.Null(deserialized.EProp); + Assert.Equal(obj.FProp, deserialized.FProp); + Assert.Equal(obj.GProp, deserialized.GProp); + Assert.Null(deserialized.HProp); + Assert.Equal(obj.IProp, deserialized.IProp); + + json = """{"AProp":"a","IProp":"i"}"""; + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.DoesNotContain("AProp", exception.Message); + Assert.Contains("BProp", exception.Message); + Assert.Contains("CProp", exception.Message); + Assert.Contains("DProp", exception.Message); + Assert.Contains("EProp", exception.Message); + Assert.Contains("FProp", exception.Message); + Assert.Contains("GProp", exception.Message); + Assert.Contains("HProp", exception.Message); + Assert.DoesNotContain("IProp", exception.Message); + + json = """{"AProp":null,"IProp":null}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.DoesNotContain("AProp", exception.Message); + Assert.Contains("BProp", exception.Message); + Assert.Contains("CProp", exception.Message); + Assert.Contains("DProp", exception.Message); + Assert.Contains("EProp", exception.Message); + Assert.Contains("FProp", exception.Message); + Assert.Contains("GProp", exception.Message); + Assert.Contains("HProp", exception.Message); + Assert.DoesNotContain("IProp", exception.Message); + + json = """{"BProp":"b","CProp":"c","DProp":"d","EProp":"e","FProp":"f","HProp":"h"}"""; + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains("AProp", exception.Message); + Assert.DoesNotContain("BProp", exception.Message); + Assert.DoesNotContain("CProp", exception.Message); + Assert.DoesNotContain("DProp", exception.Message); + Assert.DoesNotContain("EProp", exception.Message); + Assert.DoesNotContain("FProp", exception.Message); + Assert.Contains("GProp", exception.Message); + Assert.DoesNotContain("HProp", exception.Message); + Assert.Contains("IProp", exception.Message); + } - // Related: https://github.com/dotnet/runtime/issues/29861 - Assert.Throws(() => JsonSerializer.Deserialize(json)); + [Fact] + public async void IgnoreNullValuesLargeParametrizedCtorNullValuesAreNotAcceptedAsRequired() + { + JsonSerializerOptions options = new() + { + IgnoreNullValues = true, + }; + + string json = """{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":null,"FProp":"f","GProp":"g","HProp":null,"IProp":"i"}"""; + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.DoesNotContain("AProp", exception.Message); + Assert.DoesNotContain("BProp", exception.Message); + Assert.DoesNotContain("CProp", exception.Message); + Assert.DoesNotContain("DProp", exception.Message); + Assert.Contains("EProp", exception.Message); + Assert.DoesNotContain("FProp", exception.Message); + Assert.DoesNotContain("GProp", exception.Message); + Assert.Contains("HProp", exception.Message); + Assert.DoesNotContain("IProp", exception.Message); } private class PersonWithRequiredMembersAndLargeParametrizedCtor { - public required string A { get; set; } - public required string B { get; set; } - public required string C { get; set; } - public required string D { get; set; } - public required string E { get; set; } - public required string F { get; set; } - public required string G { get; set; } - - public PersonWithRequiredMembersAndLargeParametrizedCtor(string a, string b, string c, string d, string e, string f, string g) + // Using suffix for names so that checking if required property is missing can be done with simple string.Contains without false positives + public required string AProp { get; set; } + public required string BProp { get; set; } + public required string CProp { get; set; } + public required string DProp { get; set; } + public required string EProp { get; set; } + public required string FProp { get; set; } + public required string GProp { get; set; } + public required string HProp { get; set; } + public required string IProp { get; set; } + + public PersonWithRequiredMembersAndLargeParametrizedCtor(string aprop, string bprop, string cprop, string dprop, string eprop, string fprop, string gprop) { - A = a; - B = b; - C = c; - D = d; - E = e; - F = f; - G = g; + AProp = aprop; + BProp = bprop; + CProp = cprop; + DProp = dprop; + EProp = eprop; + FProp = fprop; + GProp = gprop; } } [Fact] - public static void ClassWithRequiredKeywordAndSetsRequiredMembersOnCtorWorks() + public async void ClassWithRequiredKeywordAndSetsRequiredMembersOnCtorWorks() { var obj = new PersonWithRequiredMembersAndSetsRequiredMembers() { @@ -121,11 +357,11 @@ public static void ClassWithRequiredKeywordAndSetsRequiredMembersOnCtorWorks() LastName = "bar" }; - string json = JsonSerializer.Serialize(obj); + string json = await Serializer.SerializeWrapper(obj); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); json = """{"LastName":"bar"}"""; - var deserialized = JsonSerializer.Deserialize(json); + var deserialized = await Serializer.DeserializeWrapper(json); Assert.Equal("", deserialized.FirstName); Assert.Equal("", deserialized.MiddleName); Assert.Equal("bar", deserialized.LastName); @@ -146,14 +382,14 @@ public PersonWithRequiredMembersAndSetsRequiredMembers() } [Fact] - public static void ClassWithRequiredKeywordSmallParametrizedCtorAndSetsRequiredMembersOnCtorWorks() + public async void ClassWithRequiredKeywordSmallParametrizedCtorAndSetsRequiredMembersOnCtorWorks() { var obj = new PersonWithRequiredMembersAndSmallParametrizedCtorAndSetsRequiredMembers("foo", "bar"); - string json = JsonSerializer.Serialize(obj); + string json = await Serializer.SerializeWrapper(obj); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); - var deserialized = JsonSerializer.Deserialize(json); + var deserialized = await Serializer.DeserializeWrapper(json); Assert.Equal("foo", deserialized.FirstName); Assert.Equal("", deserialized.MiddleName); Assert.Equal("bar", deserialized.LastName); @@ -174,14 +410,14 @@ public PersonWithRequiredMembersAndSmallParametrizedCtorAndSetsRequiredMembers(s } [Fact] - public static void ClassWithRequiredKeywordLargeParametrizedCtorAndSetsRequiredMembersOnCtorWorks() + public async void ClassWithRequiredKeywordLargeParametrizedCtorAndSetsRequiredMembersOnCtorWorks() { var obj = new PersonWithRequiredMembersAndLargeParametrizedCtorndSetsRequiredMembers("a", "b", "c", "d", "e", "f", "g"); - string json = JsonSerializer.Serialize(obj); + string json = await Serializer.SerializeWrapper(obj); Assert.Equal("""{"A":"a","B":"b","C":"c","D":"d","E":"e","F":"f","G":"g"}""", json); - var deserialized = JsonSerializer.Deserialize(json); + var deserialized = await Serializer.DeserializeWrapper(json); Assert.Equal("a", deserialized.A); Assert.Equal("b", deserialized.B); Assert.Equal("c", deserialized.C); @@ -215,7 +451,7 @@ public PersonWithRequiredMembersAndLargeParametrizedCtorndSetsRequiredMembers(st } [Fact] - public static void RemovingRequiredPropertiesAllowsDeserialization() + public async void RemovingRequiredPropertiesAllowsDeserialization() { JsonSerializerOptions options = new() { @@ -253,15 +489,71 @@ public static void RemovingRequiredPropertiesAllowsDeserialization() LastName = "bar" }; - string json = JsonSerializer.Serialize(obj, options); + string json = await Serializer.SerializeWrapper(obj, options); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); json = """{"LastName":"bar"}"""; - PersonWithRequiredMembers deserialized = JsonSerializer.Deserialize(json, options); + PersonWithRequiredMembers deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Null(deserialized.FirstName); Assert.Equal("", deserialized.MiddleName); Assert.Equal("bar", deserialized.LastName); } + + [Fact] + public async void RequiredNonDeserializablePropertyThrows() + { + JsonSerializerOptions options = new() + { + TypeInfoResolver = new DefaultJsonTypeInfoResolver() + { + Modifiers = + { + (ti) => + { + for (int i = 0; i < ti.Properties.Count; i++) + { + if (ti.Properties[i].Name == nameof(PersonWithRequiredMembers.FirstName)) + { + ti.Properties[i].Set = null; + } + } + } + } + } + }; + + string json = """{"FirstName":"foo","MiddleName":"","LastName":"bar"}"""; + InvalidOperationException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + Assert.Contains(nameof(PersonWithRequiredMembers.FirstName), exception.Message); + } + + [Fact] + public async void RequiredInitOnlyPropertyDoesNotThrow() + { + string json = """{"Prop":"foo"}"""; + ClassWithInitOnlyRequiredProperty deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal("foo", deserialized.Prop); + } + + private class ClassWithInitOnlyRequiredProperty + { + public required string Prop { get; init; } + } + + [Fact] + public async void RequiredExtensionDataPropertyThrows() + { + string json = """{"Foo":"foo","Bar":"bar"}"""; + InvalidOperationException exception = await Assert.ThrowsAsync( + async () => await Serializer.DeserializeWrapper(json)); + Assert.Contains(nameof(ClassWithRequiredExtensionDataProperty.TestExtensionData), exception.Message); + } + + private class ClassWithRequiredExtensionDataProperty + { + [JsonExtensionData] + public required Dictionary? TestExtensionData { get; set; } + } } #endif } From fddae304b326d2f3ac3c94c23f5cb1260e9c39c4 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Wed, 27 Jul 2022 15:08:51 +0200 Subject: [PATCH 03/11] remove local variable (forgot to press Save All) --- .../Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index fd072ddd8043a9..cfd876b0bd4ed2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -29,10 +29,10 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o AddPropertiesAndParametersUsingReflection(); #if NET7_0_OR_GREATER - bool typeHasRequiredMemberAttribute = typeof(T).IsDefined(typeof(RequiredMemberAttribute), inherit: true); - // Compiler adds RequiredMemberAttribute if any of the members is marked with 'required' keyword. + // Compiler adds RequiredMemberAttribute to type if any of the members is marked with 'required' keyword. // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement - bool shouldCheckMembersForRequiredMemberAttribute = typeHasRequiredMemberAttribute + bool shouldCheckMembersForRequiredMemberAttribute = + typeof(T).IsDefined(typeof(RequiredMemberAttribute), inherit: true) && !(converter.ConstructorInfo?.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true) ?? false); if (shouldCheckMembersForRequiredMemberAttribute) From 2df48d243606a34e42e0ad6331fb730c084f0763 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Wed, 27 Jul 2022 15:19:54 +0200 Subject: [PATCH 04/11] revert added namespaces in two files --- .../System.Text.Json/src/System/ReflectionExtensions.cs | 1 - .../Serialization/Converters/Object/KeyValuePairConverter.cs | 2 -- 2 files changed, 3 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index 7dea62529075a1..9a5d6f4bb610d9 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs @@ -1,7 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs index 4ae03c91cc7059..48c6ab85ae2b34 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/KeyValuePairConverter.cs @@ -3,9 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Diagnostics.CodeAnalysis; using System.Reflection; -using System.Text.Json.Reflection; namespace System.Text.Json.Serialization.Converters { From 20ea379f9c98d9ffc4b5b3908346e80ec50c2ce2 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 11:32:58 +0200 Subject: [PATCH 05/11] Apply feedback --- .../src/Resources/Strings.resx | 10 ++-- .../Object/ObjectDefaultConverter.cs | 7 ++- ...ParameterizedConstructorConverter.Large.cs | 2 + ...ParameterizedConstructorConverter.Small.cs | 11 ++--- ...ctWithParameterizedConstructorConverter.cs | 14 ++++-- .../JsonSerializer.Read.HandlePropertyName.cs | 3 +- .../Metadata/JsonPropertyInfo.cs | 30 ++++++++++-- .../Metadata/JsonPropertyInfoOfT.cs | 23 +-------- .../Serialization/Metadata/JsonTypeInfo.cs | 47 ++++++------------- .../Metadata/SourceGenJsonTypeInfoOfT.cs | 4 +- .../Text/Json/Serialization/ReadStackFrame.cs | 30 +++++++++++- .../Text/Json/ThrowHelper.Serialization.cs | 31 +++++++++--- .../Serialization/RequiredKeywordTests.cs | 37 ++------------- 13 files changed, 128 insertions(+), 121 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index d907293d173a2c..25f47453302380 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -665,10 +665,10 @@ JsonPropertyInfo with name '{0}' for type '{1}' cannot be required and be an extension data property. - - Type '{0}' contains required properties which were not set during deserialization: + + Type '{0}' contains required properties which were not set during deserialization: {1} - - - {0} + + Required JSON properties cannot be used in conjunction with JsonSerializerOptions.IgnoreNullValues. - + \ No newline at end of file diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 64700f4d48aba4..3275a4541bc471 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -39,7 +39,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, obj = jsonTypeInfo.CreateObject()!; jsonTypeInfo.OnDeserializing?.Invoke(obj); - jsonTypeInfo.InitializeRequiredProperties(ref state); + state.Current.InitializeRequiredProperties(jsonTypeInfo); // Process all properties. while (true) @@ -144,8 +144,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, state.Current.ReturnValue = obj; state.Current.ObjectState = StackFrameObjectState.CreatedObject; - - jsonTypeInfo.InitializeRequiredProperties(ref state); + state.Current.InitializeRequiredProperties(jsonTypeInfo); } else { @@ -253,7 +252,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, } jsonTypeInfo.OnDeserialized?.Invoke(obj); - jsonTypeInfo.CheckRequiredProperties(ref state); + state.Current.ValidateAllRequiredPropertiesAreRead(jsonTypeInfo); // Unbox Debug.Assert(obj != null); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs index 4bb5c1eed50618..eabe8e6a62fec6 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Large.cs @@ -24,6 +24,8 @@ protected sealed override bool ReadAndCacheConstructorArgument(ref ReadStack sta if (success && !(arg == null && jsonParameterInfo.IgnoreNullTokensOnRead)) { ((object[])state.Current.CtorArgumentState!.Arguments)[jsonParameterInfo.ClrInfo.Position] = arg!; + + // if this is required property IgnoreNullTokensOnRead will always be false because we don't allow for both to be true state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs index 053f3330ec3e3c..1482a32144ec4f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs @@ -67,13 +67,12 @@ private static bool TryRead( bool success = converter.TryRead(ref reader, info.PropertyType, info.Options!, ref state, out TArg? value); - if (value == null && jsonParameterInfo.IgnoreNullTokensOnRead) - { - arg = (TArg?)info.DefaultValue!; // Use default value specified on parameter, if any. - } - else + arg = value == null && jsonParameterInfo.IgnoreNullTokensOnRead + ? (TArg?)info.DefaultValue! // Use default value specified on parameter, if any. + : value!; + + if (success) { - arg = value!; state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty); } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index bb0a35a0f36329..f31dfd07481d0b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -184,7 +184,15 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo if (dataExtKey == null) { - jsonPropertyInfo.SetValueAsObject(obj, propValue, ref state); + Debug.Assert(jsonPropertyInfo.Set != null); + + if (propValue is not null || !jsonPropertyInfo.IgnoreNullTokensOnRead || default(T) is not null) + { + jsonPropertyInfo.Set(obj, propValue); + + // if this is required property IgnoreNullTokensOnRead will always be false because we don't allow for both to be true + state.Current.MarkRequiredPropertyAsRead(jsonPropertyInfo); + } } else { @@ -211,7 +219,7 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo } jsonTypeInfo.OnDeserialized?.Invoke(obj); - jsonTypeInfo.CheckRequiredProperties(ref state); + state.Current.ValidateAllRequiredPropertiesAreRead(jsonTypeInfo); // Unbox Debug.Assert(obj != null); @@ -534,7 +542,7 @@ private void BeginRead(ref ReadStack state, ref Utf8JsonReader reader, JsonSeria ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(TypeToConvert); } - jsonTypeInfo.InitializeRequiredProperties(ref state); + state.Current.InitializeRequiredProperties(jsonTypeInfo); // Set current JsonPropertyInfo to null to avoid conflicts on push. state.Current.JsonPropertyInfo = null; diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs index 598d7157542b84..f374fc70daf79c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandlePropertyName.cs @@ -139,7 +139,8 @@ internal static void CreateExtensionDataProperty( } extensionData = createObjectForExtensionDataProp(); - jsonPropertyInfo.SetExtensionDictionaryAsObject(obj, extensionData); + Debug.Assert(jsonPropertyInfo.Set != null); + jsonPropertyInfo.Set(obj, extensionData); } // We don't add the value to the dictionary here because we need to support the read-ahead functionality for Streams. 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 4c975029ff0eec..4bddf992e83ca6 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 @@ -303,6 +303,11 @@ internal void Configure() { ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndExtensionData(this); } + + if (IgnoreNullTokensOnRead) + { + ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndIgnoreNullValues(); + } } } @@ -785,10 +790,6 @@ internal JsonTypeInfo JsonTypeInfo } } - internal abstract void SetExtensionDictionaryAsObject(object obj, object? extensionDict); - - internal abstract void SetValueAsObject(object obj, object? value, ref ReadStack state); - internal bool IsIgnored => _ignoreCondition == JsonIgnoreCondition.Always; /// @@ -850,6 +851,27 @@ public JsonNumberHandling? NumberHandling /// internal abstract object? DefaultValue { get; } + /// + /// Property index on the list of JsonTypeInfo properties. + /// Can be used as a unique identifier for i.e. required properties. + /// It is set just before property is configured and does not change afterward. + /// + internal int Index + { + get + { + Debug.Assert(_isConfigured); + return _index; + } + set + { + Debug.Assert(!_isConfigured); + _index = value; + } + } + + private int _index; + [DebuggerBrowsable(DebuggerBrowsableState.Never)] private string DebuggerDisplay => $"PropertyType = {PropertyType}, Name = {Name}, DeclaringType = {DeclaringType}"; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs index e7290d6975e755..d7ed57757927b1 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/JsonPropertyInfoOfT.cs @@ -339,10 +339,10 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref { T? value = default; Set!(obj, value!); - state.Current.MarkRequiredPropertyAsRead(this); } success = true; + state.Current.MarkRequiredPropertyAsRead(this); } else if (TypedEffectiveConverter.CanUseDirectReadOrWrite && state.Current.NumberHandling == null) { @@ -354,10 +354,10 @@ internal override bool ReadJsonAndSetMember(object obj, ref ReadStack state, ref // Optimize for internal converters by avoiding the extra call to TryRead. T? fastValue = TypedEffectiveConverter.Read(ref reader, PropertyType, Options); Set!(obj, fastValue!); - state.Current.MarkRequiredPropertyAsRead(this); } success = true; + state.Current.MarkRequiredPropertyAsRead(this); } else { @@ -411,25 +411,6 @@ internal override bool ReadJsonAsObject(ref ReadStack state, ref Utf8JsonReader return success; } - internal sealed override void SetExtensionDictionaryAsObject(object obj, object? extensionDict) - { - Debug.Assert(HasSetter); - T typedValue = (T)extensionDict!; - Set!(obj, typedValue); - } - - internal sealed override void SetValueAsObject(object obj, object? value, ref ReadStack state) - { - Debug.Assert(HasSetter); - - if (value is not null || !IgnoreNullTokensOnRead || default(T) is not null) - { - T typedValue = (T)value!; - Set!(obj, typedValue); - state.Current.MarkRequiredPropertyAsRead(this); - } - } - private protected override void ConfigureIgnoreCondition(JsonIgnoreCondition? ignoreCondition) { switch (ignoreCondition) 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 e37e1e98995c1e..a274c097ff9888 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 @@ -25,7 +25,11 @@ public abstract partial class JsonTypeInfo internal delegate T ParameterizedConstructorDelegate(TArg0 arg0, TArg1 arg1, TArg2 arg2, TArg3 arg3); private JsonPropertyInfoList? _properties; - internal JsonPropertyInfo[]? RequiredProperties { get; private set; } + + /// + /// Indices of required properties. + /// + internal int[]? RequiredPropertiesIndices { get; private set; } private Action? _onSerializing; private Action? _onSerialized; @@ -270,17 +274,16 @@ public JsonPolymorphismOptions? PolymorphismOptions // Avoids having to perform an expensive cast to JsonTypeInfo to check if there is a Serialize method. internal bool HasSerializeHandler { get; private protected set; } - // Exception used to throw on deserialization. In some scenarios i.e.: - // configure would have thrown while initializing properties for source gen but type had SerializeHandler + // Configure would normally have thrown why initializing properties for source gen but type had SerializeHandler // so it is allowed to be used for fast-path serialization but it will throw if used for metadata-based serialization - internal Exception? DeserializationException { get; private protected set; } + internal bool MetadataSerializationNotSupported { get; private protected set; } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void ValidateCanBeUsedForMetadataSerialization() { - if (DeserializationException != null) + if (MetadataSerializationNotSupported) { - throw DeserializationException; + ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); } } @@ -881,6 +884,7 @@ internal void InitializePropertyCache() } int numberOfRequiredProperties = 0; + int propertyIndex = 0; foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) { JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; @@ -890,13 +894,14 @@ internal void InitializePropertyCache() numberOfRequiredProperties++; } + jsonPropertyInfo.Index = propertyIndex++; jsonPropertyInfo.EnsureChildOf(this); jsonPropertyInfo.EnsureConfigured(); } if (numberOfRequiredProperties > 0) { - JsonPropertyInfo[] requiredProperties = new JsonPropertyInfo[numberOfRequiredProperties]; + int[] requiredPropertiesIndices = new int[numberOfRequiredProperties]; int idx = 0; foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) { @@ -904,11 +909,11 @@ internal void InitializePropertyCache() if (jsonPropertyInfo.IsRequired) { - requiredProperties[idx++] = jsonPropertyInfo; + requiredPropertiesIndices[idx++] = jsonPropertyInfo.Index; } } - RequiredProperties = requiredProperties; + RequiredPropertiesIndices = requiredPropertiesIndices; } } @@ -1027,30 +1032,6 @@ internal JsonPropertyDictionary CreatePropertyCache(int capaci return new JsonPropertyDictionary(Options.PropertyNameCaseInsensitive, capacity); } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void InitializeRequiredProperties(ref ReadStack state) - { - if (RequiredProperties != null) - { - Debug.Assert(state.Current.RequiredPropertiesLeft == null); - state.Current.RequiredPropertiesLeft = new HashSet(RequiredProperties); - } - } - - [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void CheckRequiredProperties(ref ReadStack state) - { - if (RequiredProperties != null) - { - Debug.Assert(state.Current.RequiredPropertiesLeft != null); - - if (state.Current.RequiredPropertiesLeft.Count != 0) - { - ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(this, state.Current.RequiredPropertiesLeft); - } - } - } - private static JsonParameterInfo CreateConstructorParameter( JsonParameterInfoValues parameterInfo, JsonPropertyInfo jsonPropertyInfo, 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 3a3b6667bdca6d..72df4963cd2e78 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 @@ -113,7 +113,7 @@ internal override JsonParameterInfoValues[] GetParameterInfoValues() } array = Array.Empty(); - DeserializationException = ThrowHelper.GetInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); + MetadataSerializationNotSupported = true; } return array; @@ -149,7 +149,7 @@ internal override void LateAddProperties() ThrowHelper.ThrowInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); } - DeserializationException = ThrowHelper.GetInvalidOperationException_NoMetadataForTypeProperties(Options.TypeInfoResolver, Type); + MetadataSerializationNotSupported = true; return; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 8dec1ff57e1ed5..21166e2e9f3d11 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Runtime.CompilerServices; using System.Runtime.InteropServices; using System.Text.Json.Serialization; using System.Text.Json.Serialization.Metadata; @@ -67,7 +68,7 @@ public JsonTypeInfo BaseJsonTypeInfo public JsonNumberHandling? NumberHandling; // Required properties left - public HashSet? RequiredPropertiesLeft; + public HashSet? RequiredPropertiesLeft; public void EndConstructorParameter() { @@ -110,12 +111,37 @@ public bool IsProcessingEnumerable() return (JsonTypeInfo.PropertyInfoForTypeInfo.ConverterStrategy & ConverterStrategy.Enumerable) != 0; } + [MethodImpl(MethodImplOptions.AggressiveInlining)] public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) { if (propertyInfo.IsRequired) { Debug.Assert(RequiredPropertiesLeft != null); - RequiredPropertiesLeft.Remove(propertyInfo); + RequiredPropertiesLeft.Remove(propertyInfo.Index); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void InitializeRequiredProperties(JsonTypeInfo typeInfo) + { + if (typeInfo.RequiredPropertiesIndices != null) + { + Debug.Assert(RequiredPropertiesLeft == null); + RequiredPropertiesLeft = new HashSet(typeInfo.RequiredPropertiesIndices); + } + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + internal void ValidateAllRequiredPropertiesAreRead(JsonTypeInfo typeInfo) + { + if (typeInfo.RequiredPropertiesIndices != null) + { + Debug.Assert(RequiredPropertiesLeft != null); + + if (RequiredPropertiesLeft.Count != 0) + { + ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); + } } } 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 dda9736e8e7148..8e49ee0acbec0f 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 @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Globalization; using System.Reflection; using System.Runtime.CompilerServices; using System.Text.Json.Serialization; @@ -214,18 +215,36 @@ public static void ThrowInvalidOperationException_JsonPropertyRequiredAndExtensi } [DoesNotReturn] - public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, IEnumerable missingJsonProperties) + public static void ThrowInvalidOperationException_JsonPropertyRequiredAndIgnoreNullValues() { - StringBuilder errorMessageBuilder = new(); - errorMessageBuilder.AppendLine(SR.Format(SR.JsonRequiredPropertiesMissingHeader, parent.Type)); + throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndIgnoreNullValues)); + } + + [DoesNotReturn] + public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, IEnumerable missingJsonProperties) + { + StringBuilder listOfMissingPropertiesBuilder = new(); + bool first = true; + + Debug.Assert(parent.PropertyCache != null); - foreach (var missingProperty in missingJsonProperties) + foreach (int missingPropertyIndex in missingJsonProperties) { + JsonPropertyInfo missingProperty = parent.PropertyCache.List[missingPropertyIndex].Value; Debug.Assert(missingProperty.IsRequired); - errorMessageBuilder.AppendLine(SR.Format(SR.JsonRequiredPropertiesMissingItem, missingProperty.Name)); + Debug.Assert(missingProperty.Index == missingPropertyIndex); + + if (!first) + { + listOfMissingPropertiesBuilder.Append(CultureInfo.CurrentUICulture.TextInfo.ListSeparator); + listOfMissingPropertiesBuilder.Append(' '); + } + + listOfMissingPropertiesBuilder.Append(missingProperty.Name); + first = false; } - throw new JsonException(errorMessageBuilder.ToString()); + throw new JsonException(SR.Format(SR.JsonRequiredPropertiesMissing, parent.Type, listOfMissingPropertiesBuilder.ToString())); } [DoesNotReturn] diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs index ccda06d991c84f..b6b8fb2bf2d759 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs @@ -108,16 +108,7 @@ public async void IgnoreNullValuesParameterlessCtorNullValuesAreNotAcceptedAsReq }; string json = """{"FirstName":null,"MiddleName":"","LastName":null}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - Assert.Contains("FirstName", exception.Message); - Assert.Contains("LastName", exception.Message); - Assert.DoesNotContain("MiddleName", exception.Message); - - json = """{"LastName":"bar","FirstName":null}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - Assert.Contains("FirstName", exception.Message); - Assert.DoesNotContain("LastName", exception.Message); - Assert.DoesNotContain("MiddleName", exception.Message); + await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); } private class PersonWithRequiredMembers @@ -191,20 +182,7 @@ public async void IgnoreNullValuesSmallParametrizedCtorNullValuesAreNotAcceptedA }; string json = """{"FirstName":null,"MiddleName":"","LastName":"bar","Info1":"info1","Info2":"info2"}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - Assert.Contains("FirstName", exception.Message); - Assert.DoesNotContain("LastName", exception.Message); - Assert.DoesNotContain("MiddleName", exception.Message); - Assert.DoesNotContain("Info1", exception.Message); - Assert.DoesNotContain("Info2", exception.Message); - - json = """{"LastName":null,"FirstName":null,"MiddleName":"","Info1":null,"Info2":null}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - Assert.Contains("FirstName", exception.Message); - Assert.Contains("LastName", exception.Message); - Assert.DoesNotContain("MiddleName", exception.Message); - Assert.Contains("Info1", exception.Message); - Assert.Contains("Info2", exception.Message); + await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); } private class PersonWithRequiredMembersAndSmallParametrizedCtor @@ -311,16 +289,7 @@ public async void IgnoreNullValuesLargeParametrizedCtorNullValuesAreNotAcceptedA }; string json = """{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":null,"FProp":"f","GProp":"g","HProp":null,"IProp":"i"}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - Assert.DoesNotContain("AProp", exception.Message); - Assert.DoesNotContain("BProp", exception.Message); - Assert.DoesNotContain("CProp", exception.Message); - Assert.DoesNotContain("DProp", exception.Message); - Assert.Contains("EProp", exception.Message); - Assert.DoesNotContain("FProp", exception.Message); - Assert.DoesNotContain("GProp", exception.Message); - Assert.Contains("HProp", exception.Message); - Assert.DoesNotContain("IProp", exception.Message); + await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); } private class PersonWithRequiredMembersAndLargeParametrizedCtor From b422a9af9dad3da55b1d0bcbaaa80f12aa4549ab Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 15:16:48 +0200 Subject: [PATCH 06/11] apply second round of feedback (minus HashSet optimization) --- .../src/Resources/Strings.resx | 3 - .../src/System/ReflectionExtensions.cs | 34 ++++++ .../Object/ObjectDefaultConverter.cs | 4 +- ...ctWithParameterizedConstructorConverter.cs | 2 +- .../Metadata/JsonPropertyInfo.cs | 7 +- .../Metadata/ReflectionJsonTypeInfoOfT.cs | 14 +-- .../Text/Json/Serialization/ReadStackFrame.cs | 5 +- .../Text/Json/ThrowHelper.Serialization.cs | 6 - .../Serialization/RequiredKeywordTests.cs | 107 ++++++++---------- .../System.Text.Json.Tests.csproj | 11 +- 10 files changed, 105 insertions(+), 88 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 25f47453302380..3681d4fc793ba4 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -668,7 +668,4 @@ Type '{0}' contains required properties which were not set during deserialization: {1} - - Required JSON properties cannot be used in conjunction with JsonSerializerOptions.IgnoreNullValues. - \ No newline at end of file diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index 9a5d6f4bb610d9..3f856b0bdd1220 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.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.Diagnostics.CodeAnalysis; using System.Reflection; using System.Runtime.CompilerServices; using System.Runtime.ExceptionServices; @@ -42,6 +43,39 @@ public static bool IsInSubtypeRelationshipWith(this Type type, Type other) => private static bool HasJsonConstructorAttribute(ConstructorInfo constructorInfo) => constructorInfo.GetCustomAttribute() != null; + public static bool HasRequiredMemberAttribute(this ICustomAttributeProvider memberInfo) + { +#if NET7_0_OR_GREATER + return memberInfo.IsDefined(typeof(RequiredMemberAttribute), inherit: true); +#else + return memberInfo.HasCustomAttributeWithName("System.Runtime.CompilerServices.RequiredMemberAttribute"); +#endif + } + + public static bool HasSetsRequiredMembersAttribute(this ICustomAttributeProvider memberInfo) + { +#if NET7_0_OR_GREATER + return memberInfo.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true); +#else + return memberInfo.HasCustomAttributeWithName("System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute"); +#endif + } + +#if !NET7_0_OR_GREATER + private static bool HasCustomAttributeWithName(this ICustomAttributeProvider memberInfo, string name) + { + foreach (object attribute in memberInfo.GetCustomAttributes(inherit: true)) + { + if (attribute.GetType().FullName == name) + { + return true; + } + } + + return false; + } +#endif + public static TAttribute? GetUniqueCustomAttribute(this MemberInfo memberInfo, bool inherit) where TAttribute : Attribute { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs index 3275a4541bc471..365a8d2901279c 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs @@ -39,7 +39,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, obj = jsonTypeInfo.CreateObject()!; jsonTypeInfo.OnDeserializing?.Invoke(obj); - state.Current.InitializeRequiredProperties(jsonTypeInfo); + state.Current.InitializeRequiredPropertiesValidationState(jsonTypeInfo); // Process all properties. while (true) @@ -144,7 +144,7 @@ internal override bool OnTryRead(ref Utf8JsonReader reader, Type typeToConvert, state.Current.ReturnValue = obj; state.Current.ObjectState = StackFrameObjectState.CreatedObject; - state.Current.InitializeRequiredProperties(jsonTypeInfo); + state.Current.InitializeRequiredPropertiesValidationState(jsonTypeInfo); } else { diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs index f31dfd07481d0b..d0faaa3c3d238b 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.cs @@ -542,7 +542,7 @@ private void BeginRead(ref ReadStack state, ref Utf8JsonReader reader, JsonSeria ThrowHelper.ThrowInvalidOperationException_ConstructorParameterIncompleteBinding(TypeToConvert); } - state.Current.InitializeRequiredProperties(jsonTypeInfo); + state.Current.InitializeRequiredPropertiesValidationState(jsonTypeInfo); // Set current JsonPropertyInfo to null to avoid conflicts on push. state.Current.JsonPropertyInfo = null; 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 4bddf992e83ca6..5b25ed2801cb6f 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 @@ -304,10 +304,7 @@ internal void Configure() ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndExtensionData(this); } - if (IgnoreNullTokensOnRead) - { - ThrowHelper.ThrowInvalidOperationException_JsonPropertyRequiredAndIgnoreNullValues(); - } + Debug.Assert(!IgnoreNullTokensOnRead); } } @@ -371,7 +368,7 @@ private void DetermineIgnoreCondition() Debug.Assert(Options.DefaultIgnoreCondition == JsonIgnoreCondition.Never); if (PropertyTypeCanBeNull) { - IgnoreNullTokensOnRead = !_isUserSpecifiedSetter; + IgnoreNullTokensOnRead = !_isUserSpecifiedSetter && !IsRequired; IgnoreDefaultValuesOnWrite = ShouldSerialize is null; } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index cfd876b0bd4ed2..6a274fb0ea8aef 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.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; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -28,20 +29,20 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o { AddPropertiesAndParametersUsingReflection(); -#if NET7_0_OR_GREATER // Compiler adds RequiredMemberAttribute to type if any of the members is marked with 'required' keyword. // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement bool shouldCheckMembersForRequiredMemberAttribute = - typeof(T).IsDefined(typeof(RequiredMemberAttribute), inherit: true) - && !(converter.ConstructorInfo?.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true) ?? false); + typeof(T).HasRequiredMemberAttribute() + && !(converter.ConstructorInfo?.HasSetsRequiredMembersAttribute() ?? false); if (shouldCheckMembersForRequiredMemberAttribute) { Debug.Assert(PropertyCache != null); - foreach (var (_, property) in PropertyCache.List) + foreach (KeyValuePair kv in PropertyCache.List) { + JsonPropertyInfo property = kv.Value; Debug.Assert(property.AttributeProvider != null); - if (property.AttributeProvider.IsDefined(typeof(RequiredMemberAttribute), inherit: true)) + if (property.AttributeProvider.HasRequiredMemberAttribute()) { property.IsRequired = true; } @@ -50,7 +51,7 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o if (ExtensionDataProperty != null) { Debug.Assert(ExtensionDataProperty.AttributeProvider != null); - if (ExtensionDataProperty.AttributeProvider.IsDefined(typeof(RequiredMemberAttribute), inherit: true)) + if (ExtensionDataProperty.AttributeProvider.HasRequiredMemberAttribute()) { // This will end up in error in Configure // but we need to give contract resolver a chance to fix this @@ -58,7 +59,6 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o } } } -#endif } Func? createObject = Options.MemberAccessorStrategy.CreateConstructor(typeof(T)); diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 21166e2e9f3d11..865bafcbe8dcfd 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -122,11 +122,12 @@ public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) } [MethodImpl(MethodImplOptions.AggressiveInlining)] - internal void InitializeRequiredProperties(JsonTypeInfo typeInfo) + internal void InitializeRequiredPropertiesValidationState(JsonTypeInfo typeInfo) { + Debug.Assert(RequiredPropertiesLeft == null); + if (typeInfo.RequiredPropertiesIndices != null) { - Debug.Assert(RequiredPropertiesLeft == null); RequiredPropertiesLeft = new HashSet(typeInfo.RequiredPropertiesIndices); } } 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 8e49ee0acbec0f..1ec89aec7bc0f0 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 @@ -214,12 +214,6 @@ public static void ThrowInvalidOperationException_JsonPropertyRequiredAndExtensi throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndExtensionData, jsonPropertyInfo.Name, jsonPropertyInfo.DeclaringType)); } - [DoesNotReturn] - public static void ThrowInvalidOperationException_JsonPropertyRequiredAndIgnoreNullValues() - { - throw new InvalidOperationException(SR.Format(SR.JsonPropertyRequiredAndIgnoreNullValues)); - } - [DoesNotReturn] public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, IEnumerable missingJsonProperties) { diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs index b6b8fb2bf2d759..9517d94c2ce1ad 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/Serialization/RequiredKeywordTests.cs @@ -11,7 +11,6 @@ namespace System.Text.Json.Serialization.Tests { -#if !NETFRAMEWORK public class RequiredKeywordTests_Span : RequiredKeywordTests { public RequiredKeywordTests_Span() : base(JsonSerializerWrapper.SpanSerializer) { } @@ -63,52 +62,57 @@ public RequiredKeywordTests(JsonSerializerWrapper serializer) : base(serializer) { } - [Fact] - public async void ClassWithRequiredKeywordDeserialization() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async void ClassWithRequiredKeywordDeserialization(bool ignoreNullValues) { + JsonSerializerOptions options = new() + { + IgnoreNullValues = ignoreNullValues + }; + var obj = new PersonWithRequiredMembers() { FirstName = "foo", LastName = "bar" }; - string json = await Serializer.SerializeWrapper(obj); + string json = await Serializer.SerializeWrapper(obj, options); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar"}""", json); - PersonWithRequiredMembers deserialized = await Serializer.DeserializeWrapper(json); + PersonWithRequiredMembers deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Equal(obj.FirstName, deserialized.FirstName); Assert.Equal(obj.MiddleName, deserialized.MiddleName); Assert.Equal(obj.LastName, deserialized.LastName); json = """{"LastName":"bar"}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.DoesNotContain("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); json = """{"LastName":null}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.DoesNotContain("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); json = "{}"; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.Contains("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); } [Fact] - public async void IgnoreNullValuesParameterlessCtorNullValuesAreNotAcceptedAsRequired() + public async void RequiredPropertyOccuringTwiceInThePayloadWorksAsExpected() { - JsonSerializerOptions options = new() - { - IgnoreNullValues = true, - }; - - string json = """{"FirstName":null,"MiddleName":"","LastName":null}"""; - await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); + string json = """{"FirstName":"foo","MiddleName":"","LastName":"bar","FirstName":"newfoo"}"""; + PersonWithRequiredMembers deserialized = await Serializer.DeserializeWrapper(json); + Assert.Equal("newfoo", deserialized.FirstName); + Assert.Equal("", deserialized.MiddleName); + Assert.Equal("bar", deserialized.LastName); } private class PersonWithRequiredMembers @@ -118,9 +122,16 @@ private class PersonWithRequiredMembers public required string LastName { get; set; } } - [Fact] - public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeserialization() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeserialization(bool ignoreNullValues) { + JsonSerializerOptions options = new() + { + IgnoreNullValues = ignoreNullValues + }; + var obj = new PersonWithRequiredMembersAndSmallParametrizedCtor("badfoo", "badbar") { // note: these must be set during initialize or otherwise we get compiler errors @@ -130,10 +141,10 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Info2 = "info2", }; - string json = await Serializer.SerializeWrapper(obj); + string json = await Serializer.SerializeWrapper(obj, options); Assert.Equal("""{"FirstName":"foo","MiddleName":"","LastName":"bar","Info1":"info1","Info2":"info2"}""", json); - PersonWithRequiredMembersAndSmallParametrizedCtor deserialized = await Serializer.DeserializeWrapper(json); + var deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Equal(obj.FirstName, deserialized.FirstName); Assert.Equal(obj.MiddleName, deserialized.MiddleName); Assert.Equal(obj.LastName, deserialized.LastName); @@ -141,7 +152,7 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Assert.Equal(obj.Info2, deserialized.Info2); json = """{"FirstName":"foo","MiddleName":"","LastName":null,"Info1":null,"Info2":"info2"}"""; - deserialized = await Serializer.DeserializeWrapper(json); + deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Equal(obj.FirstName, deserialized.FirstName); Assert.Equal(obj.MiddleName, deserialized.MiddleName); Assert.Null(deserialized.LastName); @@ -149,7 +160,7 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Assert.Equal(obj.Info2, deserialized.Info2); json = """{"LastName":"bar","Info1":"info1"}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.DoesNotContain("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); @@ -157,7 +168,7 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Assert.Contains("Info2", exception.Message); json = """{"LastName":null,"Info1":null}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.DoesNotContain("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); @@ -165,7 +176,7 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Assert.Contains("Info2", exception.Message); json = "{}"; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("FirstName", exception.Message); Assert.Contains("LastName", exception.Message); Assert.DoesNotContain("MiddleName", exception.Message); @@ -173,18 +184,6 @@ public async void ClassWithRequiredKeywordAndSmallParametrizedCtorFailsDeseriali Assert.Contains("Info2", exception.Message); } - [Fact] - public async void IgnoreNullValuesSmallParametrizedCtorNullValuesAreNotAcceptedAsRequired() - { - JsonSerializerOptions options = new() - { - IgnoreNullValues = true, - }; - - string json = """{"FirstName":null,"MiddleName":"","LastName":"bar","Info1":"info1","Info2":"info2"}"""; - await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - } - private class PersonWithRequiredMembersAndSmallParametrizedCtor { public required string FirstName { get; set; } @@ -200,9 +199,16 @@ public PersonWithRequiredMembersAndSmallParametrizedCtor(string firstName, strin } } - [Fact] - public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeserialization() + [Theory] + [InlineData(true)] + [InlineData(false)] + public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeserialization(bool ignoreNullValues) { + JsonSerializerOptions options = new() + { + IgnoreNullValues = ignoreNullValues + }; + var obj = new PersonWithRequiredMembersAndLargeParametrizedCtor("bada", "badb", "badc", "badd", "bade", "badf", "badg") { // note: these must be set during initialize or otherwise we get compiler errors @@ -217,10 +223,10 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali IProp = "i", }; - string json = await Serializer.SerializeWrapper(obj); + string json = await Serializer.SerializeWrapper(obj, options); Assert.Equal("""{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":"e","FProp":"f","GProp":"g","HProp":"h","IProp":"i"}""", json); - var deserialized = await Serializer.DeserializeWrapper(json); + var deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Equal(obj.AProp, deserialized.AProp); Assert.Equal(obj.BProp, deserialized.BProp); Assert.Equal(obj.CProp, deserialized.CProp); @@ -232,7 +238,7 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali Assert.Equal(obj.IProp, deserialized.IProp); json = """{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":null,"FProp":"f","GProp":"g","HProp":null,"IProp":"i"}"""; - deserialized = await Serializer.DeserializeWrapper(json); + deserialized = await Serializer.DeserializeWrapper(json, options); Assert.Equal(obj.AProp, deserialized.AProp); Assert.Equal(obj.BProp, deserialized.BProp); Assert.Equal(obj.CProp, deserialized.CProp); @@ -244,7 +250,7 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali Assert.Equal(obj.IProp, deserialized.IProp); json = """{"AProp":"a","IProp":"i"}"""; - JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + JsonException exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.DoesNotContain("AProp", exception.Message); Assert.Contains("BProp", exception.Message); Assert.Contains("CProp", exception.Message); @@ -256,7 +262,7 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali Assert.DoesNotContain("IProp", exception.Message); json = """{"AProp":null,"IProp":null}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.DoesNotContain("AProp", exception.Message); Assert.Contains("BProp", exception.Message); Assert.Contains("CProp", exception.Message); @@ -268,7 +274,7 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali Assert.DoesNotContain("IProp", exception.Message); json = """{"BProp":"b","CProp":"c","DProp":"d","EProp":"e","FProp":"f","HProp":"h"}"""; - exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json)); + exception = await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); Assert.Contains("AProp", exception.Message); Assert.DoesNotContain("BProp", exception.Message); Assert.DoesNotContain("CProp", exception.Message); @@ -280,18 +286,6 @@ public async void ClassWithRequiredKeywordAndLargeParametrizedCtorFailsDeseriali Assert.Contains("IProp", exception.Message); } - [Fact] - public async void IgnoreNullValuesLargeParametrizedCtorNullValuesAreNotAcceptedAsRequired() - { - JsonSerializerOptions options = new() - { - IgnoreNullValues = true, - }; - - string json = """{"AProp":"a","BProp":"b","CProp":"c","DProp":"d","EProp":null,"FProp":"f","GProp":"g","HProp":null,"IProp":"i"}"""; - await Assert.ThrowsAsync(async () => await Serializer.DeserializeWrapper(json, options)); - } - private class PersonWithRequiredMembersAndLargeParametrizedCtor { // Using suffix for names so that checking if required property is missing can be done with simple string.Contains without false positives @@ -524,5 +518,4 @@ private class ClassWithRequiredExtensionDataProperty public required Dictionary? TestExtensionData { get; set; } } } -#endif } diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj index 49d8daf97de312..2bd76ae43071f8 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.Tests/System.Text.Json.Tests.csproj @@ -25,10 +25,6 @@ - - - - @@ -237,10 +233,15 @@ - + + + + + + From ca11095e8dc9f6937f40daabc1c4d91fb81404fd Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 16:32:05 +0200 Subject: [PATCH 07/11] Use BitArray rather than HashSet --- .../src/System/ReflectionExtensions.cs | 10 ++-- .../Metadata/JsonPropertyInfo.cs | 19 +++++-- .../Serialization/Metadata/JsonTypeInfo.cs | 23 ++------ .../Metadata/ReflectionJsonTypeInfoOfT.cs | 52 ++++++------------- .../Text/Json/Serialization/ReadStackFrame.cs | 19 ++++--- .../Text/Json/ThrowHelper.Serialization.cs | 30 +++++++++-- 6 files changed, 75 insertions(+), 78 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index 3f856b0bdd1220..87e2fd0183bc1d 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs @@ -48,7 +48,7 @@ public static bool HasRequiredMemberAttribute(this ICustomAttributeProvider memb #if NET7_0_OR_GREATER return memberInfo.IsDefined(typeof(RequiredMemberAttribute), inherit: true); #else - return memberInfo.HasCustomAttributeWithName("System.Runtime.CompilerServices.RequiredMemberAttribute"); + return memberInfo.HasCustomAttributeWithName("System.Runtime.CompilerServices.RequiredMemberAttribute", inherit: true); #endif } @@ -57,16 +57,16 @@ public static bool HasSetsRequiredMembersAttribute(this ICustomAttributeProvider #if NET7_0_OR_GREATER return memberInfo.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true); #else - return memberInfo.HasCustomAttributeWithName("System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute"); + return memberInfo.HasCustomAttributeWithName("System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute", inherit: true); #endif } #if !NET7_0_OR_GREATER - private static bool HasCustomAttributeWithName(this ICustomAttributeProvider memberInfo, string name) + private static bool HasCustomAttributeWithName(this ICustomAttributeProvider memberInfo, string fullName, bool inherit) { - foreach (object attribute in memberInfo.GetCustomAttributes(inherit: true)) + foreach (object attribute in memberInfo.GetCustomAttributes(inherit)) { - if (attribute.GetType().FullName == name) + if (attribute.GetType().FullName == fullName) { return 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 5b25ed2801cb6f..7605421549e759 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 @@ -504,6 +504,14 @@ private bool NumberHandingIsApplicable() potentialNumberType == JsonTypeInfo.ObjectType; } + private void DetermineIsRequired(MemberInfo memberInfo, bool shouldCheckForRequiredKeyword) + { + if (shouldCheckForRequiredKeyword && memberInfo.HasRequiredMemberAttribute()) + { + IsRequired = true; + } + } + internal abstract bool GetMemberAndWriteJson(object obj, ref WriteStack state, Utf8JsonWriter writer); internal abstract bool GetMemberAndWriteJsonExtensionData(object obj, ref WriteStack state, Utf8JsonWriter writer); @@ -531,7 +539,7 @@ internal string GetDebugInfo(int indent = 0) internal bool HasGetter => _untypedGet is not null; internal bool HasSetter => _untypedSet is not null; - internal void InitializeUsingMemberReflection(MemberInfo memberInfo, JsonConverter? customConverter, JsonIgnoreCondition? ignoreCondition) + internal void InitializeUsingMemberReflection(MemberInfo memberInfo, JsonConverter? customConverter, JsonIgnoreCondition? ignoreCondition, bool shouldCheckForRequiredKeyword) { Debug.Assert(AttributeProvider == null); @@ -558,6 +566,7 @@ internal void InitializeUsingMemberReflection(MemberInfo memberInfo, JsonConvert CustomConverter = customConverter; DeterminePoliciesFromMember(memberInfo); DeterminePropertyNameFromMember(memberInfo); + DetermineIsRequired(memberInfo, shouldCheckForRequiredKeyword); if (ignoreCondition != JsonIgnoreCondition.Always) { @@ -849,15 +858,17 @@ public JsonNumberHandling? NumberHandling internal abstract object? DefaultValue { get; } /// - /// Property index on the list of JsonTypeInfo properties. - /// Can be used as a unique identifier for i.e. required properties. + /// Required property index on the list of JsonTypeInfo properties. + /// It is used as a unique identifier for required properties. /// It is set just before property is configured and does not change afterward. + /// It is not equivalent to index on the properties list /// - internal int Index + internal int RequiredPropertyIndex { get { Debug.Assert(_isConfigured); + Debug.Assert(IsRequired); return _index; } set 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 a274c097ff9888..2b41583d5dd92d 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 @@ -29,7 +29,7 @@ public abstract partial class JsonTypeInfo /// /// Indices of required properties. /// - internal int[]? RequiredPropertiesIndices { get; private set; } + internal int NumberOfRequiredProperties { get; private set; } private Action? _onSerializing; private Action? _onSerialized; @@ -884,37 +884,20 @@ internal void InitializePropertyCache() } int numberOfRequiredProperties = 0; - int propertyIndex = 0; foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) { JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; if (jsonPropertyInfo.IsRequired) { - numberOfRequiredProperties++; + jsonPropertyInfo.RequiredPropertyIndex = numberOfRequiredProperties++; } - jsonPropertyInfo.Index = propertyIndex++; jsonPropertyInfo.EnsureChildOf(this); jsonPropertyInfo.EnsureConfigured(); } - if (numberOfRequiredProperties > 0) - { - int[] requiredPropertiesIndices = new int[numberOfRequiredProperties]; - int idx = 0; - foreach (KeyValuePair jsonPropertyInfoKv in PropertyCache.List) - { - JsonPropertyInfo jsonPropertyInfo = jsonPropertyInfoKv.Value; - - if (jsonPropertyInfo.IsRequired) - { - requiredPropertiesIndices[idx++] = jsonPropertyInfo.Index; - } - } - - RequiredPropertiesIndices = requiredPropertiesIndices; - } + NumberOfRequiredProperties = numberOfRequiredProperties; } internal void InitializeConstructorParameters(JsonParameterInfoValues[] jsonParameters, bool sourceGenMode = false) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs index 6a274fb0ea8aef..f176e154011b85 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Metadata/ReflectionJsonTypeInfoOfT.cs @@ -28,37 +28,6 @@ internal ReflectionJsonTypeInfo(JsonConverter converter, JsonSerializerOptions o if (PropertyInfoForTypeInfo.ConverterStrategy == ConverterStrategy.Object) { AddPropertiesAndParametersUsingReflection(); - - // Compiler adds RequiredMemberAttribute to type if any of the members is marked with 'required' keyword. - // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement - bool shouldCheckMembersForRequiredMemberAttribute = - typeof(T).HasRequiredMemberAttribute() - && !(converter.ConstructorInfo?.HasSetsRequiredMembersAttribute() ?? false); - - if (shouldCheckMembersForRequiredMemberAttribute) - { - Debug.Assert(PropertyCache != null); - foreach (KeyValuePair kv in PropertyCache.List) - { - JsonPropertyInfo property = kv.Value; - Debug.Assert(property.AttributeProvider != null); - if (property.AttributeProvider.HasRequiredMemberAttribute()) - { - property.IsRequired = true; - } - } - - if (ExtensionDataProperty != null) - { - Debug.Assert(ExtensionDataProperty.AttributeProvider != null); - if (ExtensionDataProperty.AttributeProvider.HasRequiredMemberAttribute()) - { - // This will end up in error in Configure - // but we need to give contract resolver a chance to fix this - ExtensionDataProperty.IsRequired = true; - } - } - } } Func? createObject = Options.MemberAccessorStrategy.CreateConstructor(typeof(T)); @@ -88,6 +57,11 @@ private void AddPropertiesAndParametersUsingReflection() Dictionary? ignoredMembers = null; bool propertyOrderSpecified = false; + // Compiler adds RequiredMemberAttribute to type if any of the members is marked with 'required' keyword. + // SetsRequiredMembersAttribute means that all required members are assigned by constructor and therefore there is no enforcement + bool shouldCheckMembersForRequiredMemberAttribute = + typeof(T).HasRequiredMemberAttribute() + && !(Converter.ConstructorInfo?.HasSetsRequiredMembersAttribute() ?? false); // Walk through the inheritance hierarchy, starting from the most derived type upward. for (Type? currentType = Type; currentType != null; currentType = currentType.BaseType) @@ -118,7 +92,8 @@ private void AddPropertiesAndParametersUsingReflection() typeToConvert: propertyInfo.PropertyType, memberInfo: propertyInfo, ref propertyOrderSpecified, - ref ignoredMembers); + ref ignoredMembers, + shouldCheckMembersForRequiredMemberAttribute); } else { @@ -150,7 +125,8 @@ private void AddPropertiesAndParametersUsingReflection() typeToConvert: fieldInfo.FieldType, memberInfo: fieldInfo, ref propertyOrderSpecified, - ref ignoredMembers); + ref ignoredMembers, + shouldCheckMembersForRequiredMemberAttribute); } } else @@ -177,9 +153,10 @@ private void CacheMember( Type typeToConvert, MemberInfo memberInfo, ref bool propertyOrderSpecified, - ref Dictionary? ignoredMembers) + ref Dictionary? ignoredMembers, + bool shouldCheckForRequiredKeyword) { - JsonPropertyInfo? jsonPropertyInfo = CreateProperty(typeToConvert, memberInfo, Options); + JsonPropertyInfo? jsonPropertyInfo = CreateProperty(typeToConvert, memberInfo, Options, shouldCheckForRequiredKeyword); if (jsonPropertyInfo == null) { // ignored invalid property @@ -199,7 +176,8 @@ private void CacheMember( private JsonPropertyInfo? CreateProperty( Type typeToConvert, MemberInfo memberInfo, - JsonSerializerOptions options) + JsonSerializerOptions options, + bool shouldCheckForRequiredKeyword) { JsonIgnoreCondition? ignoreCondition = memberInfo.GetCustomAttribute(inherit: false)?.Condition; @@ -224,7 +202,7 @@ private void CacheMember( } JsonPropertyInfo jsonPropertyInfo = CreatePropertyUsingReflection(typeToConvert); - jsonPropertyInfo.InitializeUsingMemberReflection(memberInfo, customConverter, ignoreCondition); + jsonPropertyInfo.InitializeUsingMemberReflection(memberInfo, customConverter, ignoreCondition, shouldCheckForRequiredKeyword); return jsonPropertyInfo; } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 865bafcbe8dcfd..54a319590eefdc 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.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; using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -68,7 +69,7 @@ public JsonTypeInfo BaseJsonTypeInfo public JsonNumberHandling? NumberHandling; // Required properties left - public HashSet? RequiredPropertiesLeft; + public BitArray? RequiredPropertiesLeft; public void EndConstructorParameter() { @@ -117,7 +118,7 @@ public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) if (propertyInfo.IsRequired) { Debug.Assert(RequiredPropertiesLeft != null); - RequiredPropertiesLeft.Remove(propertyInfo.Index); + RequiredPropertiesLeft.Set(propertyInfo.RequiredPropertyIndex, false); } } @@ -126,22 +127,26 @@ internal void InitializeRequiredPropertiesValidationState(JsonTypeInfo typeInfo) { Debug.Assert(RequiredPropertiesLeft == null); - if (typeInfo.RequiredPropertiesIndices != null) + if (typeInfo.NumberOfRequiredProperties > 0) { - RequiredPropertiesLeft = new HashSet(typeInfo.RequiredPropertiesIndices); + RequiredPropertiesLeft = new BitArray(typeInfo.NumberOfRequiredProperties, defaultValue: true); } } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void ValidateAllRequiredPropertiesAreRead(JsonTypeInfo typeInfo) { - if (typeInfo.RequiredPropertiesIndices != null) + if (typeInfo.NumberOfRequiredProperties > 0) { Debug.Assert(RequiredPropertiesLeft != null); - if (RequiredPropertiesLeft.Count != 0) + // Optimize this when https://github.com/dotnet/runtime/issues/72999 is fixed + for (int i = 0; i < RequiredPropertiesLeft.Count; i++) { - ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); + if (RequiredPropertiesLeft[i]) + { + ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); + } } } } 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 1ec89aec7bc0f0..f5ee8c66cf5d2b 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 @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Diagnostics.CodeAnalysis; @@ -215,18 +216,37 @@ public static void ThrowInvalidOperationException_JsonPropertyRequiredAndExtensi } [DoesNotReturn] - public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, IEnumerable missingJsonProperties) + public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, BitArray missingJsonProperties) { StringBuilder listOfMissingPropertiesBuilder = new(); bool first = true; Debug.Assert(parent.PropertyCache != null); - foreach (int missingPropertyIndex in missingJsonProperties) + int propertyIdx = 0; + + for (int requiredPropertyIdx = 0; requiredPropertyIdx < missingJsonProperties.Length; requiredPropertyIdx++) { - JsonPropertyInfo missingProperty = parent.PropertyCache.List[missingPropertyIndex].Value; - Debug.Assert(missingProperty.IsRequired); - Debug.Assert(missingProperty.Index == missingPropertyIndex); + if (!missingJsonProperties[requiredPropertyIdx]) + { + continue; + } + + JsonPropertyInfo? missingProperty = null; + + // requiredPropertyIdx indices occur consecutively so we can resume iteration + for (; propertyIdx < parent.PropertyCache.List.Count; propertyIdx++) + { + JsonPropertyInfo maybeMissingProperty = parent.PropertyCache.List[propertyIdx].Value; + if (maybeMissingProperty.IsRequired && maybeMissingProperty.RequiredPropertyIndex == requiredPropertyIdx) + { + missingProperty = maybeMissingProperty; + break; + } + } + + Debug.Assert(propertyIdx != parent.PropertyCache.List.Count); + Debug.Assert(missingProperty != null); if (!first) { From 12f5df2e5495a299166c95e61dfe2c06547f7904 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 17:15:50 +0200 Subject: [PATCH 08/11] simplify validation code --- .../src/System/Text/Json/JsonHelpers.cs | 15 +++++++++++ .../Text/Json/Serialization/ReadStackFrame.cs | 10 +++---- .../Text/Json/ThrowHelper.Serialization.cs | 26 ++++--------------- 3 files changed, 23 insertions(+), 28 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs b/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs index 0bdd85880bd0fa..6aced2cc4b9145 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/JsonHelpers.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. using System.Buffers; +using System.Collections; using System.Collections.Generic; using System.Diagnostics; using System.Runtime.CompilerServices; @@ -147,5 +148,19 @@ public static void ValidateInt32MaxArrayLength(uint length) ThrowHelper.ThrowOutOfMemoryException(length); } } + + public static bool AllBitsEqual(this BitArray bitArray, bool value) + { + // Optimize this when https://github.com/dotnet/runtime/issues/72999 is fixed + for (int i = 0; i < bitArray.Count; i++) + { + if (bitArray[i] != value) + { + return false; + } + } + + return true; + } } } diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 54a319590eefdc..576e99778e54d2 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -118,7 +118,7 @@ public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) if (propertyInfo.IsRequired) { Debug.Assert(RequiredPropertiesLeft != null); - RequiredPropertiesLeft.Set(propertyInfo.RequiredPropertyIndex, false); + RequiredPropertiesLeft[propertyInfo.RequiredPropertyIndex] = false; } } @@ -140,13 +140,9 @@ internal void ValidateAllRequiredPropertiesAreRead(JsonTypeInfo typeInfo) { Debug.Assert(RequiredPropertiesLeft != null); - // Optimize this when https://github.com/dotnet/runtime/issues/72999 is fixed - for (int i = 0; i < RequiredPropertiesLeft.Count; i++) + if (!RequiredPropertiesLeft.AllBitsEqual(false)) { - if (RequiredPropertiesLeft[i]) - { - ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); - } + ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); } } } 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 f5ee8c66cf5d2b..7a079c72067cf1 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 @@ -223,38 +223,22 @@ public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo p Debug.Assert(parent.PropertyCache != null); - int propertyIdx = 0; - - for (int requiredPropertyIdx = 0; requiredPropertyIdx < missingJsonProperties.Length; requiredPropertyIdx++) + for (int propertyIdx = 0; propertyIdx < parent.PropertyCache.List.Count; propertyIdx++) { - if (!missingJsonProperties[requiredPropertyIdx]) - { - continue; - } + JsonPropertyInfo property = parent.PropertyCache.List[propertyIdx].Value; - JsonPropertyInfo? missingProperty = null; - - // requiredPropertyIdx indices occur consecutively so we can resume iteration - for (; propertyIdx < parent.PropertyCache.List.Count; propertyIdx++) + if (!property.IsRequired || !missingJsonProperties[property.RequiredPropertyIndex]) { - JsonPropertyInfo maybeMissingProperty = parent.PropertyCache.List[propertyIdx].Value; - if (maybeMissingProperty.IsRequired && maybeMissingProperty.RequiredPropertyIndex == requiredPropertyIdx) - { - missingProperty = maybeMissingProperty; - break; - } + continue; } - Debug.Assert(propertyIdx != parent.PropertyCache.List.Count); - Debug.Assert(missingProperty != null); - if (!first) { listOfMissingPropertiesBuilder.Append(CultureInfo.CurrentUICulture.TextInfo.ListSeparator); listOfMissingPropertiesBuilder.Append(' '); } - listOfMissingPropertiesBuilder.Append(missingProperty.Name); + listOfMissingPropertiesBuilder.Append(property.Name); first = false; } From 223e76131f6078c79d28a45ff887e7d47fd34aa7 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 19:48:28 +0200 Subject: [PATCH 09/11] Truncate the message if too long, flip meaning of bit array bits --- .../src/Resources/Strings.resx | 8 ++++---- .../Text/Json/Serialization/ReadStackFrame.cs | 18 +++++++++--------- .../Text/Json/ThrowHelper.Serialization.cs | 12 ++++++++++-- 3 files changed, 23 insertions(+), 15 deletions(-) diff --git a/src/libraries/System.Text.Json/src/Resources/Strings.resx b/src/libraries/System.Text.Json/src/Resources/Strings.resx index 3681d4fc793ba4..01d0af8f556137 100644 --- a/src/libraries/System.Text.Json/src/Resources/Strings.resx +++ b/src/libraries/System.Text.Json/src/Resources/Strings.resx @@ -660,12 +660,12 @@ Parameter already associated with a different JsonTypeInfo instance. - JsonPropertyInfo with name '{0}' for type '{1}' is required but is not deserializable. + JsonPropertyInfo '{0}' defined in type '{1}' is marked required but does not specify a setter. - JsonPropertyInfo with name '{0}' for type '{1}' cannot be required and be an extension data property. + JsonPropertyInfo '{0}' defined in type '{1}' is marked both as required and as an extension data property. This combination is not supported. - Type '{0}' contains required properties which were not set during deserialization: {1} + JSON deserialization for type '{0}' was missing required properties, including the following: {1} - \ No newline at end of file + diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 576e99778e54d2..25e70584ba9404 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -68,8 +68,8 @@ public JsonTypeInfo BaseJsonTypeInfo // Whether to use custom number handling. public JsonNumberHandling? NumberHandling; - // Required properties left - public BitArray? RequiredPropertiesLeft; + // Required properties which have value assigned + public BitArray? RequiredPropertiesSet; public void EndConstructorParameter() { @@ -117,19 +117,19 @@ public void MarkRequiredPropertyAsRead(JsonPropertyInfo propertyInfo) { if (propertyInfo.IsRequired) { - Debug.Assert(RequiredPropertiesLeft != null); - RequiredPropertiesLeft[propertyInfo.RequiredPropertyIndex] = false; + Debug.Assert(RequiredPropertiesSet != null); + RequiredPropertiesSet[propertyInfo.RequiredPropertyIndex] = true; } } [MethodImpl(MethodImplOptions.AggressiveInlining)] internal void InitializeRequiredPropertiesValidationState(JsonTypeInfo typeInfo) { - Debug.Assert(RequiredPropertiesLeft == null); + Debug.Assert(RequiredPropertiesSet == null); if (typeInfo.NumberOfRequiredProperties > 0) { - RequiredPropertiesLeft = new BitArray(typeInfo.NumberOfRequiredProperties, defaultValue: true); + RequiredPropertiesSet = new BitArray(typeInfo.NumberOfRequiredProperties, defaultValue: false); } } @@ -138,11 +138,11 @@ internal void ValidateAllRequiredPropertiesAreRead(JsonTypeInfo typeInfo) { if (typeInfo.NumberOfRequiredProperties > 0) { - Debug.Assert(RequiredPropertiesLeft != null); + Debug.Assert(RequiredPropertiesSet != null); - if (!RequiredPropertiesLeft.AllBitsEqual(false)) + if (!RequiredPropertiesSet.AllBitsEqual(true)) { - ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesLeft); + ThrowHelper.ThrowJsonException_JsonRequiredPropertyMissing(typeInfo, RequiredPropertiesSet); } } } 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 7a079c72067cf1..66384b01b4ce97 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 @@ -216,18 +216,21 @@ public static void ThrowInvalidOperationException_JsonPropertyRequiredAndExtensi } [DoesNotReturn] - public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, BitArray missingJsonProperties) + public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo parent, BitArray requiredPropertiesSet) { StringBuilder listOfMissingPropertiesBuilder = new(); bool first = true; Debug.Assert(parent.PropertyCache != null); + // Soft cut-off length - once message becomes longer than that we won't be adding more elements + const int CutOffLength = 50; + for (int propertyIdx = 0; propertyIdx < parent.PropertyCache.List.Count; propertyIdx++) { JsonPropertyInfo property = parent.PropertyCache.List[propertyIdx].Value; - if (!property.IsRequired || !missingJsonProperties[property.RequiredPropertyIndex]) + if (!property.IsRequired || requiredPropertiesSet[property.RequiredPropertyIndex]) { continue; } @@ -240,6 +243,11 @@ public static void ThrowJsonException_JsonRequiredPropertyMissing(JsonTypeInfo p listOfMissingPropertiesBuilder.Append(property.Name); first = false; + + if (listOfMissingPropertiesBuilder.Length >= CutOffLength) + { + break; + } } throw new JsonException(SR.Format(SR.JsonRequiredPropertiesMissing, parent.Type, listOfMissingPropertiesBuilder.ToString())); From c041037ae6e7a3482c0e8454d8a07b473a991c37 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 20:03:13 +0200 Subject: [PATCH 10/11] remove default value, add description for BitArray --- .../src/System/Text/Json/Serialization/ReadStackFrame.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs index 25e70584ba9404..abf8aa0bd39b5f 100644 --- a/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs +++ b/src/libraries/System.Text.Json/src/System/Text/Json/Serialization/ReadStackFrame.cs @@ -68,7 +68,11 @@ public JsonTypeInfo BaseJsonTypeInfo // Whether to use custom number handling. public JsonNumberHandling? NumberHandling; - // Required properties which have value assigned + // Represents required properties which have value assigned. + // Each bit corresponds to a required property. + // False means that property is not set (not yet occured in the payload). + // Length of the BitArray is equal to number of required properties. + // Every required JsonPropertyInfo has RequiredPropertyIndex property which maps to an index in this BitArray. public BitArray? RequiredPropertiesSet; public void EndConstructorParameter() @@ -129,7 +133,7 @@ internal void InitializeRequiredPropertiesValidationState(JsonTypeInfo typeInfo) if (typeInfo.NumberOfRequiredProperties > 0) { - RequiredPropertiesSet = new BitArray(typeInfo.NumberOfRequiredProperties, defaultValue: false); + RequiredPropertiesSet = new BitArray(typeInfo.NumberOfRequiredProperties); } } From 71e99e7a0015acd307abdf816de0f3d6fe37fc76 Mon Sep 17 00:00:00 2001 From: Krzysztof Wicher Date: Thu, 28 Jul 2022 21:47:18 +0200 Subject: [PATCH 11/11] for compiler attributes check for full type name rather than using typeof on net7.0 --- .../src/System/ReflectionExtensions.cs | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs index 87e2fd0183bc1d..e15e8993889e43 100644 --- a/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs +++ b/src/libraries/System.Text.Json/src/System/ReflectionExtensions.cs @@ -45,23 +45,17 @@ private static bool HasJsonConstructorAttribute(ConstructorInfo constructorInfo) public static bool HasRequiredMemberAttribute(this ICustomAttributeProvider memberInfo) { -#if NET7_0_OR_GREATER - return memberInfo.IsDefined(typeof(RequiredMemberAttribute), inherit: true); -#else + // For compiler related attributes we should only look at full type name rather than trying to do something different for version when attribute was introduced. + // I.e. library is targetting netstandard2.0 with polyfilled attributes and is being consumed by app targetting net7.0. return memberInfo.HasCustomAttributeWithName("System.Runtime.CompilerServices.RequiredMemberAttribute", inherit: true); -#endif } public static bool HasSetsRequiredMembersAttribute(this ICustomAttributeProvider memberInfo) { -#if NET7_0_OR_GREATER - return memberInfo.IsDefined(typeof(SetsRequiredMembersAttribute), inherit: true); -#else + // See comment for HasRequiredMemberAttribute for why we need to always only look at full name return memberInfo.HasCustomAttributeWithName("System.Diagnostics.CodeAnalysis.SetsRequiredMembersAttribute", inherit: true); -#endif } -#if !NET7_0_OR_GREATER private static bool HasCustomAttributeWithName(this ICustomAttributeProvider memberInfo, string fullName, bool inherit) { foreach (object attribute in memberInfo.GetCustomAttributes(inherit)) @@ -74,7 +68,6 @@ private static bool HasCustomAttributeWithName(this ICustomAttributeProvider mem return false; } -#endif public static TAttribute? GetUniqueCustomAttribute(this MemberInfo memberInfo, bool inherit) where TAttribute : Attribute