Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update doc for exceptions thrown during serialization #33400

Merged
merged 2 commits into from
Mar 13, 2020
Merged

Update doc for exceptions thrown during serialization #33400

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b03d6d2
Update doc for exceptions thrown during serialization
steveharter Mar 9, 2020
a9da412
Add more detail to NSE and use null keyword
steveharter Mar 13, 2020
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
10 changes: 9 additions & 1 deletion ...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Span.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,10 @@ public static partial class JsonSerializer
/// <typeparamref name="TValue"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
Copy link
Member

Choose a reason for hiding this comment

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

We should make sure to update these manually in https://github.com/dotnet/dotnet-api-docs as well.

cc @carlossanlop

/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
Copy link
Member

Choose a reason for hiding this comment

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

Is this the only case when we would throw an NSE for this API?

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so. What else were you thinking?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing specific came to mind, but I thought we might have other checks around types we don't support/etc. or if a type has NSE characteristics (internal ctors, non-default ctors, fields, private setters, etc.), we'd throw as well. Maybe audit all places we throw NSE and verify they all fall under the "JsonConverter doesn't exist" camp.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point about ctor checks -- I do think however that it still means the "converter is not compatible" since you could in theory have a converter that would support non-default ctors and not throw NSE. i.e. the current "built-in" converter doesn't support non-default ctors.

I plan on leaving as-is except perhaps append the "other serializable members" per above

/// </exception>
[return: MaybeNull]
public static TValue Deserialize<TValue>(ReadOnlySpan<byte> utf8Json, JsonSerializerOptions? options = null)
{
Expand All @@ -43,13 +47,17 @@ public static TValue Deserialize<TValue>(ReadOnlySpan<byte> utf8Json, JsonSerial
/// <param name="returnType">The type of the object to convert to and return.</param>
/// <param name="options">Options to control the behavior during parsing.</param>
/// <exception cref="System.ArgumentNullException">
/// Thrown if <paramref name="returnType"/> is null.
/// <paramref name="returnType"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
/// <paramref name="returnType"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="returnType"/>.
Copy link
Member

Choose a reason for hiding this comment

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

We'd also throw NSE if any of the types within the object graph of returnType don't have a JsonConverter, not just returnType.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I considered something like "There is no compatible JsonConverter for {0} or any of its properties" but that extra verbage I thought was implied. A converter includes anything it does including delegating to other converters on its properties, which also have converters, etc.

Also we don't explain that any exception is possible since we may call user-provided converters, but from past discussions didn't attempt to explain that either.

Copy link
Member

Choose a reason for hiding this comment

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

but that extra verbage I thought was implied.

The fact that we state for returnType, makes it ambiguous. I think being explicit could be useful. Up to you :)

Also we don't explain that any exception is possible since we may call user-provided converters, but from past discussions didn't attempt to explain that either.

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is this better? I'm no writer :P

"There is no compatible JsonConverter for {0} or its serializable members."

Copy link
Member

Choose a reason for hiding this comment

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

nice

/// </exception>
public static object? Deserialize(ReadOnlySpan<byte> utf8Json, Type returnType, JsonSerializerOptions? options = null)
{
if (returnType == null)
Expand Down
13 changes: 12 additions & 1 deletion ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,18 @@ public static partial class JsonSerializer
/// <param name="cancellationToken">
/// The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the read operation.
/// </param>
/// <exception cref="System.ArgumentNullException">
/// <paramref name="utf8Json"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
/// <typeparamref name="TValue"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
public static ValueTask<TValue> DeserializeAsync<TValue>(
Stream utf8Json,
JsonSerializerOptions? options = null,
Expand All @@ -53,13 +60,17 @@ public static ValueTask<TValue> DeserializeAsync<TValue>(
/// The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the read operation.
/// </param>
/// <exception cref="System.ArgumentNullException">
/// Thrown if <paramref name="utf8Json"/> or <paramref name="returnType"/> is null.
/// <paramref name="utf8Json"/> or <paramref name="returnType"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
/// the <paramref name="returnType"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="returnType"/>.
/// </exception>
public static ValueTask<object?> DeserializeAsync(
Stream utf8Json,
Type returnType,
Expand Down
12 changes: 10 additions & 2 deletions ...braries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,17 @@ public static partial class JsonSerializer
/// <param name="json">JSON text to parse.</param>
/// <param name="options">Options to control the behavior during parsing.</param>
/// <exception cref="System.ArgumentNullException">
/// Thrown if <paramref name="json"/> is null.
/// <paramref name="json"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
/// <typeparamref name="TValue"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
/// <remarks>Using a <see cref="string"/> is not as efficient as using the
/// UTF-8 methods since the implementation natively uses UTF-8.
/// </remarks>
Expand All @@ -51,13 +55,17 @@ public static TValue Deserialize<TValue>(string json, JsonSerializerOptions? opt
/// <param name="returnType">The type of the object to convert to and return.</param>
/// <param name="options">Options to control the behavior during parsing.</param>
/// <exception cref="System.ArgumentNullException">
/// Thrown if <paramref name="json"/> or <paramref name="returnType"/> is null.
/// <paramref name="json"/> or <paramref name="returnType"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
/// the <paramref name="returnType"/> is not compatible with the JSON,
/// or when there is remaining data in the Stream.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="returnType"/>.
/// </exception>
/// <remarks>Using a <see cref="string"/> is not as efficient as using the
/// UTF-8 methods since the implementation natively uses UTF-8.
/// </remarks>
Expand Down
10 changes: 9 additions & 1 deletion ...System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.Utf8JsonReader.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ internal static TValue Deserialize<TValue>(ref Utf8JsonReader reader, JsonSerial
/// <exception cref="ArgumentException">
/// <paramref name="reader"/> is using unsupported options.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
/// <remarks>
/// <para>
/// If the <see cref="Utf8JsonReader.TokenType"/> property of <paramref name="reader"/>
Expand Down Expand Up @@ -95,7 +99,7 @@ public static TValue Deserialize<TValue>(ref Utf8JsonReader reader, JsonSerializ
/// <param name="returnType">The type of the object to convert to and return.</param>
/// <param name="options">Options to control the serializer behavior during reading.</param>
/// <exception cref="ArgumentNullException">
/// Thrown if <paramref name="returnType"/> is null.
/// <paramref name="returnType"/> is null.
/// </exception>
/// <exception cref="JsonException">
/// Thrown when the JSON is invalid,
Expand All @@ -105,6 +109,10 @@ public static TValue Deserialize<TValue>(ref Utf8JsonReader reader, JsonSerializ
/// <exception cref="ArgumentException">
/// <paramref name="reader"/> is using unsupported options.
/// </exception>
/// <exception cref="NotSupportedException">
Copy link
Member

Choose a reason for hiding this comment

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

Reminder: Port these to api docs so they become visible on the docs site:
https://github.com/dotnet/dotnet-api-docs/blob/891ff666a6fb22862cc9fa1b8f868c8725e9bd8c/xml/System.Text.Json/JsonSerializer.xml#L172-L185

https://docs.microsoft.com/en-us/dotnet/api/system.text.json.jsonserializer.deserialize?view=netcore-3.1#System_Text_Json_JsonSerializer_Deserialize__1_System_Text_Json_Utf8JsonReader__System_Text_Json_JsonSerializerOptions_

/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="returnType"/>.
/// </exception>
/// <remarks>
/// <para>
/// If the <see cref="Utf8JsonReader.TokenType"/> property of <paramref name="reader"/>
Expand Down
14 changes: 14 additions & 0 deletions ...ies/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ public static partial class JsonSerializer
/// <returns>A UTF-8 representation of the value.</returns>
/// <param name="value">The value to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
public static byte[] SerializeToUtf8Bytes<TValue>(TValue value, JsonSerializerOptions? options = null)
{
return WriteCoreBytes<TValue>(value, typeof(TValue), options);
Expand All @@ -24,6 +28,16 @@ public static byte[] SerializeToUtf8Bytes<TValue>(TValue value, JsonSerializerOp
/// <param name="value">The value to convert.</param>
/// <param name="inputType">The type of the <paramref name="value"/> to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <exception cref="ArgumentException">
/// <paramref name="inputType"/> is not compatible with <paramref name="value"/>.
/// </exception>
/// <exception cref="System.ArgumentNullException">
/// <paramref name="inputType"/> is null.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="inputType"/>.
/// </exception>
public static byte[] SerializeToUtf8Bytes(object? value, Type inputType, JsonSerializerOptions? options = null)
{
if (inputType == null)
Expand Down
17 changes: 17 additions & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ public static partial class JsonSerializer
/// <param name="value">The value to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <param name="cancellationToken">The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the write operation.</param>
/// <exception cref="ArgumentNullException">
/// <paramref name="utf8Json"/> is null.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
public static Task SerializeAsync<TValue>(Stream utf8Json, TValue value, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
{
if (utf8Json == null)
Expand All @@ -36,6 +43,16 @@ public static Task SerializeAsync<TValue>(Stream utf8Json, TValue value, JsonSer
/// <param name="inputType">The type of the <paramref name="value"/> to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <param name="cancellationToken">The <see cref="System.Threading.CancellationToken"/> which may be used to cancel the write operation.</param>
/// <exception cref="ArgumentException">
/// <paramref name="inputType"/> is not compatible with <paramref name="value"/>.
/// </exception>
/// <exception cref="ArgumentNullException">
/// <paramref name="utf8Json"/> or <paramref name="inputType"/> is null.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="inputType"/>.
/// </exception>
public static Task SerializeAsync(Stream utf8Json, object? value, Type inputType, JsonSerializerOptions? options = null, CancellationToken cancellationToken = default)
{
if (utf8Json == null)
Expand Down
11 changes: 11 additions & 0 deletions ...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ public static partial class JsonSerializer
/// <returns>A <see cref="string"/> representation of the value.</returns>
/// <param name="value">The value to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
/// <remarks>Using a <see cref="string"/> is not as efficient as using UTF-8
/// encoding since the implementation internally uses UTF-8. See also <see cref="SerializeToUtf8Bytes"/>
/// and <see cref="SerializeAsync"/>.
Expand All @@ -28,6 +32,13 @@ public static string Serialize<TValue>(TValue value, JsonSerializerOptions? opti
/// <param name="value">The value to convert.</param>
/// <param name="inputType">The type of the <paramref name="value"/> to convert.</param>
/// <param name="options">Options to control the conversion behavior.</param>
/// <exception cref="ArgumentException">
/// <paramref name="inputType"/> is not compatible with <paramref name="value"/>.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="inputType"/>.
/// </exception>
/// <remarks>Using a <see cref="string"/> is not as efficient as using UTF-8
/// encoding since the implementation internally uses UTF-8. See also <see cref="SerializeToUtf8Bytes"/>
/// and <see cref="SerializeAsync"/>.
Expand Down
13 changes: 12 additions & 1 deletion ...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 @@ -34,6 +34,10 @@ internal static void Serialize<T>(Utf8JsonWriter writer, T value, JsonSerializer
/// <exception cref="ArgumentNullException">
/// <paramref name="writer"/> is null.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <typeparamref name="TValue"/>.
/// </exception>
public static void Serialize<TValue>(Utf8JsonWriter writer, TValue value, JsonSerializerOptions? options = null)
{
Serialize<TValue>(writer, value, typeof(TValue), options);
Expand All @@ -46,8 +50,15 @@ 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="ArgumentException">
/// <paramref name="inputType"/> is not compatible with <paramref name="value"/>.
/// </exception>
/// <exception cref="ArgumentNullException">
/// <paramref name="writer"/> is null.
/// <paramref name="writer"/> or <paramref name="inputType"/> is null.
/// </exception>
/// <exception cref="NotSupportedException">
/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="inputType"/>.
/// </exception>
public static void Serialize(Utf8JsonWriter writer, object? value, Type inputType, JsonSerializerOptions? options = null)
{
Expand Down
7 changes: 7 additions & 0 deletions ...s/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,13 @@ internal JsonConverter DetermineConverter(Type? parentClassType, Type runtimePro
/// <returns>
/// The converter for the given type.
/// </returns>
/// <exception cref="InvalidOperationException">
/// The configured <see cref="JsonConverter"/> for <paramref name="typeToConvert"/> returned an invalid converter.
/// </exception>
/// <exception cref="NotSupportedException">
Copy link
Member

Choose a reason for hiding this comment

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

Where do the checks happen that result in NSE here?

Copy link
Member Author

@steveharter steveharter Mar 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

If a converter is not yet cached, it is created from this method and currently the collection-based converters will throw NSE for not supported collections like multidimensional array.

Copy link
Member

Choose a reason for hiding this comment

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

he collection-based converters will throw NSE for not supported collections like multidimensional array.

What about the non-collection-based converters/other converters? Is it ever possible that a converter couldn't be created for a particular type (one that we don't support today)?

For example, what is the exception that we throw if someone happen to have a BigInteger property in their model? Presumably it would be this documented NSE. Is that what happens?

/// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/>
/// for <paramref name="typeToConvert"/>.
/// </exception>
public JsonConverter GetConverter(Type typeToConvert)
{
if (_converters.TryGetValue(typeToConvert, out JsonConverter? converter))
Expand Down