Ensure required properties are validated before invoking the deserialization constructor. (dotnet#107083)

Author: eiriktsarpalis

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;

src/libraries/System.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectWithParameterizedConstructorConverter.Small.cs

@@ -80,8 +80,6 @@ private static bool TryRead<TArg>(
                         ThrowHelper.ThrowJsonException_ConstructorParameterDisallowNull(info.Name, state.Current.JsonTypeInfo.Type);
                     }
                 }
-
-                state.Current.MarkRequiredPropertyAsRead(jsonParameterInfo.MatchingProperty);
             }
 
             arg = value;

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)
             {

src/libraries/System.Text.Json/tests/Common/RequiredKeywordTests.cs

@@ -727,6 +727,25 @@ public static IEnumerable<object[]> 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<JsonException>(() => Serializer.DeserializeWrapper<ClassWithNullValidatingConstructor>("{}", 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<T>(JsonSerializerOptions options)
         {
             options.TypeInfoResolver ??= JsonSerializerOptions.Default.TypeInfoResolver;

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
         {
         }