Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

JsonSerializer.Serialize should check whether the passed-in Utf8JsonWriter is null. #42026

Merged
merged 1 commit into from
Oct 23, 2019
Merged

JsonSerializer.Serialize should check whether the passed-in Utf8JsonWriter is null. #42026

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,11 @@ private static void WriteValueCore(Utf8JsonWriter writer, object value, Type typ
options = JsonSerializerOptions.s_defaultOptions;
}

if (writer == null)
{
throw new ArgumentNullException(nameof(writer));
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be checked here, or in a public method elsewhere?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


WriteCore(writer, value, type, options);
}

Expand All @@ -111,6 +116,7 @@ private static void WriteCore(PooledByteBufferWriter output, object value, Type
private static void WriteCore(Utf8JsonWriter writer, object value, Type type, JsonSerializerOptions options)
{
Debug.Assert(type != null || value == null);
Debug.Assert(writer != null);

if (value == null)
{
Expand Down
6 changes: 6 additions & 0 deletions ...ystem.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Utf8JsonWriter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@carlossanlop carlossanlop Oct 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of requests:

  1. Can you fix the spacing? In other words, put everything in one line?
  2. 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of requests

Can we fix this on the docs side? I'll keep the feedback in mind for future updates. Thanks @carlossanlop!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has it been sent to Maira via ref assemblies?

Yes, it's there:
https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.serialize?view=netcore-3.0#System_Text_Json_JsonSerializer_Serialize_System_Text_Json_Utf8JsonWriter_System_Object_System_Type_System_Text_Json_JsonSerializerOptions_

We have a generic overload (with 3 args) and the non-generic overload (with 4 args) for serializing into a Utf8JsonWriter.

/// </exception>
public static void Serialize<TValue>(Utf8JsonWriter writer, TValue value, JsonSerializerOptions options = null)
{
WriteValueCore(writer, value, typeof(TValue), options);
Expand All @@ -24,6 +27,9 @@ public static void Serialize<TValue>(Utf8JsonWriter writer, TValue value, JsonSe
/// <param name="value">The value to convert and write.</param>
/// <param name="inputType">The type of the <paramref name="value"/> to convert.</param>
/// <param name="options">Options to control the behavior.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="writer"/> is null.
/// </exception>
public static void Serialize(Utf8JsonWriter writer, object value, Type inputType, JsonSerializerOptions options = null)
{
VerifyValueAndType(value, inputType);
Expand Down
7 changes: 7 additions & 0 deletions src/System.Text.Json/tests/Serialization/WriteValueTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ namespace System.Text.Json.Serialization.Tests
{
public static partial class WriteValueTests
{
[Fact]
public static void NullWriterThrows()
{
Assert.Throws<ArgumentNullException>(() => JsonSerializer.Serialize(null, 1));
Assert.Throws<ArgumentNullException>(() => JsonSerializer.Serialize(null, 1, typeof(int)));
}

[Fact]
public static void CanWriteValueToJsonArray()
{
Expand Down