From aaec1c494bec68bb0022a8d78005982a32377327 Mon Sep 17 00:00:00 2001 From: Eirik Tsarpalis Date: Thu, 29 Aug 2024 11:15:36 +0300 Subject: [PATCH] Ensure required properties are validated before invoking the deserialization constructor. (#107083) --- ...ParameterizedConstructorConverter.Large.cs | 3 --- ...ParameterizedConstructorConverter.Small.cs | 2 -- ...ctWithParameterizedConstructorConverter.cs | 17 +++++++++++++---- .../tests/Common/RequiredKeywordTests.cs | 19 +++++++++++++++++++ .../Serialization/RequiredKeywordTests.cs | 1 + 5 files changed, 33 insertions(+), 9 deletions(-) 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 6aa1117eefac8..b65c994abd1ee 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 @@ -28,9 +28,6 @@ protected sealed override bool ReadAndCacheConstructorArgument(scoped ref ReadSt } ((object[])state.Current.CtorArgumentState!.Arguments)[jsonParameterInfo.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); } 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 b6ad9089c4810..292f19b3d0ff2 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 @@ -80,8 +80,6 @@ private static bool TryRead( ThrowHelper.ThrowJsonException_ConstructorParameterDisallowNull(info.Name, state.Current.JsonTypeInfo.Type); } } - - state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty); } arg = value; 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 6ae225b57457f..f18b9060acf1f 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 @@ -58,6 +58,11 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo ReadConstructorArguments(ref state, ref reader, options); + // We've read all ctor parameters and properties, + // validate that all required parameters were provided + // before calling the constructor which may throw. + state.Current.ValidateAllRequiredPropertiesAreRead(jsonTypeInfo); + obj = (T)CreateObject(ref state.Current); jsonTypeInfo.OnDeserializing?.Invoke(obj); @@ -192,6 +197,11 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo return false; } + // We've read all ctor parameters and properties, + // validate that all required parameters were provided + // before calling the constructor which may throw. + state.Current.ValidateAllRequiredPropertiesAreRead(jsonTypeInfo); + obj = (T)CreateObject(ref state.Current); if ((state.Current.MetadataPropertyNames & MetadataPropertyName.Id) != 0) @@ -219,9 +229,6 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo 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 @@ -249,7 +256,6 @@ internal sealed override bool OnTryRead(ref Utf8JsonReader reader, Type typeToCo } jsonTypeInfo.OnDeserialized?.Invoke(obj); - state.Current.ValidateAllRequiredPropertiesAreRead(jsonTypeInfo); // Unbox Debug.Assert(obj != null); @@ -606,6 +612,9 @@ protected static bool TryLookupConstructorParameter( out bool useExtensionProperty, createExtensionProperty: false); + // Mark the property as read from the payload if required. + state.Current.MarkRequiredPropertyAsRead(jsonPropertyInfo); + jsonParameterInfo = jsonPropertyInfo.AssociatedParameter; if (jsonParameterInfo != null) { diff --git a/src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs index 22c50becfa020..9b70145773653 100644 --- a/src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs +++ b/src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs @@ -727,6 +727,25 @@ public static IEnumerable InheritedPersonWithRequiredMembersSetsRequir }; } + [Fact] + public async Task ClassWithNullValidatingConstructor_ValidatesRequiredParameterBeforeCallingCtor() + { + // Regression test for https://github.com/dotnet/runtime/issues/107065 + JsonSerializerOptions options = new(Serializer.DefaultOptions) { RespectRequiredConstructorParameters = true }; + JsonException ex = await Assert.ThrowsAsync(() => Serializer.DeserializeWrapper("{}", options)); + Assert.Null(ex.InnerException); + } + + public class ClassWithNullValidatingConstructor + { + public ClassWithNullValidatingConstructor(string value) + { + Value = value ?? throw new ArgumentNullException(nameof(value)); + } + + public string Value { get; } + } + private static JsonTypeInfo GetTypeInfo(JsonSerializerOptions options) { options.TypeInfoResolver ??= JsonSerializerOptions.Default.TypeInfoResolver; diff --git a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/RequiredKeywordTests.cs b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/RequiredKeywordTests.cs index 0515f0553bc42..045ee191d7cab 100644 --- a/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/RequiredKeywordTests.cs +++ b/src/libraries/System.Text.Json/tests/System.Text.Json.SourceGeneration.Tests/Serialization/RequiredKeywordTests.cs @@ -33,6 +33,7 @@ public RequiredKeywordTests_SourceGen() [JsonSerializable(typeof(ClassWithRequiredKeywordAndJsonRequiredCustomAttribute))] [JsonSerializable(typeof(ClassWithCustomRequiredPropertyName))] [JsonSerializable(typeof(DerivedClassWithRequiredInitOnlyProperty))] + [JsonSerializable(typeof(ClassWithNullValidatingConstructor))] internal sealed partial class RequiredKeywordTestsContext : JsonSerializerContext { }