Update doc for exceptions thrown during serialization #33400
Conversation
steveharter
added
documentation
steveharter
force-pushed
the
UpdateDocsForExceptions
branch
from
49b9467
to
b03d6d2
Compare
| @@ -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"> | |||
There was a problem hiding this comment.
We should make sure to update these manually in https://github.com/dotnet/dotnet-api-docs as well.
| @@ -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"> | |||
| /// There is no compatible <see cref="System.Text.Json.Serialization.JsonConverter"/> | |||
| /// for <typeparamref name="TValue"/>. | |||
There was a problem hiding this comment.
Is this the only case when we would throw an NSE for this API?
There was a problem hiding this comment.
I believe so. What else were you thinking?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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> | ||
| /// <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"/>. |
There was a problem hiding this comment.
We'd also throw NSE if any of the types within the object graph of returnType don't have a JsonConverter, not just returnType.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Is this better? I'm no writer :P
"There is no compatible JsonConverter for {0} or its serializable members."
| @@ -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"> | |||
There was a problem hiding this comment.
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
| /// <exception cref="InvalidOperationException"> | ||
| /// The configured <see cref="JsonConverter"/> for <paramref name="typeToConvert"/> returned an invalid converter. | ||
| /// </exception> | ||
| /// <exception cref="NotSupportedException"> |
There was a problem hiding this comment.
Where do the checks happen that result in NSE here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
ghost
locked as resolved and limited conversation to collaborators
The doc for the various serialization APIs was not complete or correct.