Argument tests for CreateJsonTypeInfo and CreateJsonPropertyInfo#71324
Argument tests for CreateJsonTypeInfo and CreateJsonPropertyInfo#71324krwq merged 2 commits intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis Issue DetailsFixes: #71296 I went through all APIs and seems only CreateJsonTypeInfo and CreatePropertyInfo were missing checks. I also added some test gaps for null modifier but that was checking args already
|
krwq
force-pushed
the
contract-customization-arg-checks
branch
from
d87996f to
39b0578
Compare
| if (type == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(type)); | ||
| } | ||
|
|
||
| if (options == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(options)); | ||
| } |
There was a problem hiding this comment.
We should be using ArgumentNullException.ThrowIfnull for cases like that, see #68178.
There was a problem hiding this comment.
Actually we're also compiling against netfx and that's not available in there
There was a problem hiding this comment.
Yeah, I just realized. We have an equivalent method in ThrowHelper so you can just use that.
eiriktsarpalis
left a comment
eiriktsarpalis
left a comment
There was a problem hiding this comment.
Other than one minor nit, lgtm.
| if (IsInvalidForSerialization(propertyType)) | ||
| { | ||
| ThrowHelper.ThrowArgumentException_CannotSerializeInvalidType(nameof(propertyType), propertyType, Type, name); | ||
| } |
There was a problem hiding this comment.
Does the method check if TypeInfoKind == JsonTypeInfoKind.Object. We migth want to throw InvalidOperationException if we call the create method for an invalid declaring type.
There was a problem hiding this comment.
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.
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.
| [InlineData(typeof(RefStruct))] | ||
| public static void CreateJsonTypeInfoWithInappropriateTypeThrows(Type type) | ||
| { | ||
| Assert.Throws<ArgumentException>(() => JsonTypeInfo.CreateJsonTypeInfo(type, new JsonSerializerOptions())); |
There was a problem hiding this comment.
@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.
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.
I missed the message yesterday. Will take a look at the issue when I get a chance.
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes: #71296
I went through all APIs and seems only CreateJsonTypeInfo and CreatePropertyInfo were missing checks. I also added some test gaps for null modifier but that was checking args already