JsonSerializer.Serialize should check whether the passed-in Utf8JsonWriter is null. #42026
Conversation
Utf8JsonWriter is null.
| @@ -12,6 +12,9 @@ public static partial class JsonSerializer | |||
| /// <param name="writer">The writer to write.</param> | |||
| /// <param name="value">The value to convert and write.</param> | |||
| /// <param name="options">Options to control the behavior.</param> | |||
| /// <exception cref="ArgumentNullException"> | |||
| /// <paramref name="writer"/> is null. | |||
There was a problem hiding this comment.
@carlossanlop - will your tool pick up changes like these and port to docs or do I have to do it manually?
The exception section of the Serialize APIs is a bit light on details currently:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serialize?view=netcore-3.0
There was a problem hiding this comment.
Yes. The tool will pick it up.
The reason: This exception had not yet been added to the list of exceptions for this Serialize overload. If it had been there already, the modification would've had to be done manually.
There was a problem hiding this comment.
A couple of requests:
- Can you fix the spacing? In other words, put everything in one line?
- Can you make the null word a see langword?
Like this:
<exception cref="ArgumentNullException"><paramref name="writer" /> is <see langword="null" />.</exception>Same request for the exception below.
There was a problem hiding this comment.
A couple of requests
Can we fix this on the docs side? I'll keep the feedback in mind for future updates. Thanks @carlossanlop!
There was a problem hiding this comment.
Now that I think of it, the tool will probably detect the extra spacing and fix it, and will detect the null word and convert it. Let's see if it works.
There was a problem hiding this comment.
@ahsonkhan I just realized another thing: I don't see this overload in netcore-3.0 in Docs. I pasted the wrong one above (it has 3 parameters). This one has 4 parameters. Has it been sent to Maira via ref assemblies?
There was a problem hiding this comment.
Has it been sent to Maira via ref assemblies?
We have a generic overload (with 3 args) and the non-generic overload (with 4 args) for serializing into a Utf8JsonWriter.
| if (writer == null) | ||
| { | ||
| throw new ArgumentNullException(nameof(writer)); | ||
| } |
There was a problem hiding this comment.
Should this be checked here, or in a public method elsewhere?
There was a problem hiding this comment.
Both public methods call into this method so was following the pattern in the existing code that defers some of the arg validation to the common helper (similar to options/null stream/etc.). Otherwise, I would have done it in the public method.
I left refactoring out of this PR so I can port this change to 3.1.
…2026) Utf8JsonWriter is null.
…orefx#42026) Utf8JsonWriter is null. Commit migrated from dotnet/corefx@b8cb521
We should port this to 3.1.
cc @steveharter, @ericstj