Skip to content

Commit

Permalink
Ensure required properties are validated before invoking the deserial…
Browse files Browse the repository at this point in the history
…ization constructor. (#107083)
  • Loading branch information
eiriktsarpalis authored Aug 29, 2024
1 parent 2e7ee69 commit aaec1c4
Show file tree
Hide file tree
Showing 5 changed files with 33 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
}
Expand Down

0 comments on commit aaec1c4

Please sign in to comment.