-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Argument tests for CreateJsonTypeInfo and CreateJsonPropertyInfo #71324
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -412,6 +412,11 @@ internal virtual void LateAddProperties() { } | |
[RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")] | ||
public static JsonTypeInfo<T> CreateJsonTypeInfo<T>(JsonSerializerOptions options) | ||
{ | ||
if (options == null) | ||
{ | ||
throw new ArgumentNullException(nameof(options)); | ||
} | ||
|
||
return new CustomJsonTypeInfo<T>(options); | ||
} | ||
|
||
|
@@ -427,6 +432,21 @@ public static JsonTypeInfo<T> CreateJsonTypeInfo<T>(JsonSerializerOptions option | |
[RequiresDynamicCode("JSON serialization and deserialization might require types that cannot be statically analyzed and might need runtime code generation. Use generic overload or System.Text.Json source generation for native AOT applications.")] | ||
public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions options) | ||
{ | ||
if (type == null) | ||
{ | ||
throw new ArgumentNullException(nameof(type)); | ||
} | ||
|
||
if (options == null) | ||
{ | ||
throw new ArgumentNullException(nameof(options)); | ||
} | ||
|
||
if (IsInvalidForSerialization(type)) | ||
{ | ||
ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(type), type, null, null); | ||
} | ||
|
||
s_createJsonTypeInfo ??= typeof(JsonTypeInfo).GetMethod(nameof(CreateJsonTypeInfo), new Type[] { typeof(JsonSerializerOptions) })!; | ||
return (JsonTypeInfo)s_createJsonTypeInfo.MakeGenericMethod(type) | ||
.Invoke(null, new object[] { options })!; | ||
|
@@ -440,7 +460,20 @@ public static JsonTypeInfo CreateJsonTypeInfo(Type type, JsonSerializerOptions o | |
/// <returns>JsonPropertyInfo instance</returns> | ||
public JsonPropertyInfo CreateJsonPropertyInfo(Type propertyType, string name) | ||
{ | ||
ValidateType(propertyType, Type, null, Options); | ||
if (propertyType == null) | ||
{ | ||
throw new ArgumentNullException(nameof(propertyType)); | ||
} | ||
|
||
if (name == null) | ||
{ | ||
throw new ArgumentNullException(nameof(name)); | ||
} | ||
|
||
if (IsInvalidForSerialization(propertyType)) | ||
{ | ||
ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the method check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I thought we discussed this and decided not to throw. We won't let you add such property but no problem with creating There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough -- probably not too important. But we won't be able to change this in the future because it would create a likely breaking change. |
||
|
||
JsonConverter converter = GetConverter(propertyType, | ||
parentClassType: null, | ||
|
@@ -609,7 +642,7 @@ internal static void ValidateType(Type type, Type? parentClassType, MemberInfo? | |
|
||
internal static bool IsInvalidForSerialization(Type type) | ||
{ | ||
return type.IsPointer || type.IsByRef || IsByRefLike(type) || type.ContainsGenericParameters; | ||
return type == typeof(void) || type.IsPointer || type.IsByRef || IsByRefLike(type) || type.ContainsGenericParameters; | ||
} | ||
|
||
private static bool IsByRefLike(Type type) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -580,6 +580,54 @@ public static void AddingNullJsonPropertyInfoIsNotPossible() | |
Assert.NotNull(typeInfo.Properties[0]); | ||
} | ||
|
||
[Fact] | ||
public static void CreateJsonTypeInfoWithNullArgumentsThrows() | ||
{ | ||
Assert.Throws<ArgumentNullException>(() => JsonTypeInfo.CreateJsonTypeInfo(null, new JsonSerializerOptions())); | ||
Assert.Throws<ArgumentNullException>(() => JsonTypeInfo.CreateJsonTypeInfo(typeof(string), null)); | ||
Assert.Throws<ArgumentNullException>(() => JsonTypeInfo.CreateJsonTypeInfo(null, null)); | ||
Assert.Throws<ArgumentNullException>(() => JsonTypeInfo.CreateJsonTypeInfo<string>(null)); | ||
} | ||
|
||
[Theory] | ||
[InlineData(typeof(void))] | ||
[InlineData(typeof(Dictionary<,>))] | ||
[InlineData(typeof(List<>))] | ||
[InlineData(typeof(Nullable<>))] | ||
[InlineData(typeof(int*))] | ||
[InlineData(typeof(RefStruct))] | ||
public static void CreateJsonTypeInfoWithInappropriateTypeThrows(Type type) | ||
{ | ||
Assert.Throws<ArgumentException>(() => JsonTypeInfo.CreateJsonTypeInfo(type, new JsonSerializerOptions())); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @fanyang-mono One of the types being passed in (probably void) results in a runtime crash. Would you be able to take a look and see why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've filed #71339 as I'm most likely going to try to workaround the issue in this PR There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed the message yesterday. Will take a look at the issue when I get a chance. |
||
} | ||
|
||
ref struct RefStruct | ||
{ | ||
public int Foo { get; set; } | ||
} | ||
|
||
[Fact] | ||
public static void CreateJsonPropertyInfoWithNullArgumentsThrows() | ||
{ | ||
JsonTypeInfo ti = JsonTypeInfo.CreateJsonTypeInfo<MyClass>(new JsonSerializerOptions()); | ||
Assert.Throws<ArgumentNullException>(() => ti.CreateJsonPropertyInfo(null, "test")); | ||
Assert.Throws<ArgumentNullException>(() => ti.CreateJsonPropertyInfo(typeof(string), null)); | ||
Assert.Throws<ArgumentNullException>(() => ti.CreateJsonPropertyInfo(null, null)); | ||
} | ||
|
||
[Theory] | ||
[InlineData(typeof(void))] | ||
[InlineData(typeof(Dictionary<,>))] | ||
[InlineData(typeof(List<>))] | ||
[InlineData(typeof(Nullable<>))] | ||
[InlineData(typeof(int*))] | ||
[InlineData(typeof(RefStruct))] | ||
public static void CreateJsonPropertyInfoWithInappropriateTypeThrows(Type type) | ||
{ | ||
JsonTypeInfo ti = JsonTypeInfo.CreateJsonTypeInfo<MyClass>(new JsonSerializerOptions()); | ||
Assert.Throws<ArgumentException>(() => ti.CreateJsonPropertyInfo(type, "test")); | ||
} | ||
|
||
[Theory] | ||
[InlineData(typeof(object))] | ||
[InlineData(typeof(string))] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should be using
ArgumentNullException.ThrowIfnull
for cases like that, see #68178.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually we're also compiling against netfx and that's not available in there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I just realized. We have an equivalent method in
ThrowHelper
so you can just use that.