-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Don't allow null converters and add Path support to NotSupportedException #32669
Don't allow null converters and add Path support to NotSupportedException #32669
Conversation
78b056f
to
3d21aca
Compare
Some value-typed benchmarks added in dotnet/performance#1211 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feedback from offline review session (me, @steveharter, @jozkee), primarily around removing the delayed initialization for JsonClassInfo
has been addressed.
Pls take a pass at filling code coverage for new code.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonClassInfo.cs
Outdated
Show resolved
Hide resolved
Options); | ||
|
||
ClassType = converter.ClassType; | ||
PolicyProperty = CreatePolicyProperty(Type, runtimeType, converter, Options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you consider caching this lazily (like ElementClassInfo
) as discussed, or is it guaranteed to be used by all ClassType
s anywhere they occur in the type graph? Maybe we could measure if/how much allocations could be saved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't change with this PR (PropertyPolicy was always created).
Currently PolicyProperty is always accessed for a given property, so deferring the construction will cause an extra if
check at runtime instead of doing the creation up-front (which is cached).
Since there's a reference back to the parent, caching would only help for properties on a given type if they're the same Type. PropertyPolicy could pehaps be improved here, but not in scope for this PR anyway.
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.cs
Outdated
Show resolved
Hide resolved
...raries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.ByteArray.cs
Show resolved
Hide resolved
...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
Show resolved
Hide resolved
...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.Stream.cs
Show resolved
Hide resolved
...libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Write.String.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
...tem.Text.Json/src/System/Text/Json/Serialization/Converters/Object/ObjectDefaultConverter.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterFactory.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.WriteCore.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/Serialization/JsonConverterOfT.ReadCore.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
...ries/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializerOptions.Converters.cs
Outdated
Show resolved
Hide resolved
f78ca13
to
faa61a7
Compare
c6b3151
to
4e34106
Compare
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
Assert.Contains(typeof(int[,]).ToString(), ex.ToString()); | ||
Assert.Contains(typeof(ChildPocoWithNoConverterAndInvalidProperty).ToString(), ex.ToString()); | ||
Assert.Contains("Path: $.InvalidProperty | LineNumber: 0 | BytePositionInLine: 20.", ex.ToString()); | ||
Assert.Equal(2, ex.ToString().Split("Path:", StringSplitOptions.None).Length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test failure - Serialization\CustomConverterTests.Callback.cs(139,58): error CS1503: (NETCORE_ENGINEERING_TELEMETRY=Build) Argument 2: cannot convert from 'System.StringSplitOptions' to 'char'
Here and below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like that is supported in ns2.1 but not 2.0. Need to pass array instead of string.
Debug.Assert(writer != null); | ||
|
||
if (value == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we were not calling Initialize when the root was null
; now we do and that causes a change in behavior for unsupported root types.
[Fact]
public static void QuickTest()
{
int[,] arr = null;
string json = JsonSerializer.Serialize(arr);
Console.WriteLine(json);
}
on master, the previous example returns null. with these changes we will throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be fixed if you just keep this condition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we want to support converters writing their own null
we must ask the converter if they want to handle null. For example a custom String converter may want to write null
as ""
. So we just can't write null
here.
I'll add a test to assert this and track for any potential breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the pending discussion of moving the logic from the NotSupportingException
catch to the helper #32669 (comment), and the change of behavior pointed on #32669 (comment); this looks good to me.
@@ -216,16 +211,86 @@ public static void AddExceptionInformation(in ReadStack state, in Utf8JsonReader | |||
} | |||
} | |||
|
|||
public static NotSupportedException GetNotSupportedException(in ReadStack state, in Utf8JsonReader reader, NotSupportedException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static NotSupportedException GetNotSupportedException(in ReadStack state, in Utf8JsonReader reader, NotSupportedException ex) | |
[DoesNotReturn] | |
[MethodImpl(MethodImplOptions.NoInlining)] | |
public static void GetNotSupportedException(in ReadStack state, in Utf8JsonReader reader, NotSupportedException ex) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned earlier, we need to use plain "throw" statement to re-throw the current exception. We don't want to throw the same exception instance since that will create a new call stack unnecessarily. So this method can't be [DoesNotReturn]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: added check for "Path" to caller and changed method to add [DoesNotReturn]
. We still keep the original "throw" in the caller's catch statement in order to preserve the original call stack.
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
4e34106
to
417254e
Compare
Unrelated and previously hit CI failures in: |
|
||
return WriteAsyncCore(utf8Json, value, inputType, options, cancellationToken); | ||
return WriteAsyncCore<object>(utf8Json, value!, inputType, options, cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect usage of nullability annotation. If value can be null (and looking at the null checks happening later, it can), the TValue value
parameter on WriteAsyncCore
should be correctly annotated to be nullable.
Avoid using !
to circumvent null warnings unless absolutely necessary, and instead annotate the implementation to reflect nullability accurately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. This should be called with WriteAsyncCore<object?>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a helpful note for reviewers: Make sure to pay close attention to uses of !
in the code :)
This PR introduced a breaking change - #35422. See that issue for more info. I'll file a breaking change doc. |
Filed a breaking change doc - dotnet/docs#21376. |
Update: a breaking change doc was already filed: dotnet/docs#20823, https://docs.microsoft.com/en-us/dotnet/core/compatibility/3.1-5.0#jsonserializerserialize-throws-argumentnullexception-when-type-parameter-is-null. |
dotnet/docs#20823 was meant to cover #528 (comment) not this, but if they surface in the same API / behavior then agree we don't need two. |
Address feedback and issues from #2259 including
JsonConverterFactory.CreateConverter()
returning nullNote after this PR is in, there will be a follow-up PR that adds the correct XML doc regarding exceptions.
Details on JsonConverterFactory.CreateConverter()` should not return null.
After this PR we align with 3.0\3.1 behavior of not supporting null.
When a null value was returned from
CreateConverter()
the previous code (and in 3.0\3.1) threwArgumentNullException
. This was changed toInvalidOperationException
and an exception message was added.Background: allowing null from
CreateConverter()
was done in #2259 because before that PR the collection code threw a "nice" exception that contained the parent Type and property name. Before that PR, this could not be done inCreateConverter()
because collections were not converters and didn't share that code path. With #2259 collections became converters, and thus needed a way to identify a "not supported" case so code further up the call stack could throw the "nice" exception message. If aNotSupportedException
was thrown directly from withinCreateConverter()
then the message would only contain the name of the Type that is not supported, not the parent Type or the property name.Per offline discussion, it was decided we would add Path information to
NotSupportedException
similarly like we do forJsonException
. This means throwing aNotSupportedException
directly from a converter (or converter factory) would re-throw the exception with Path appended to the original message.Examples of the previous exceptions:
The type 'System.Int32[,]' on 'System.Text.Json.Serialization.Tests.ExceptionTests+ClassWithInvalidArray.UnsupportedArray' is not supported.
or if a converter threw its own
NotSupportedException
or the exception occurred in a collection element Type, not the property Type:The type 'System.Int32[,]' is not supported.
The latter case is especially bad because it provided no way to determine where the unsupported Type exists in the JSON or the object graph.
Examples of exceptions with this PR:
The type 'System.Int32[,]' is not supported. The unsupported member type is located on type 'System.Text.Json.Serialization.Tests.GenericIEnumerableWrapper`1[System.Int32[]]'. Path: $.UnsupportedDictionary."
or this (for read cases):
The type 'System.Int32[,]' is not supported. The unsupported member type is located on type 'System.Text.Json.Serialization.Tests.GenericIEnumerableWrapper`1[System.Int32[]]'. Path: $.UnsupportedDictionary | LineNumber: 0 | BytePositionInLine: 10."
Details on extra boxing avoided
#2259 added an extra box+unbox for value types passed into the root. This PR fixes that. In addition, for structs that have their own converter (and thus are not treated as "POCO structs" with properties) no boxing occurs at all (previously and in 3.0\3.1 a box was done).
In addition, boxing operations were removed that helped reduce non-stack boxing. The result is less stack allocations and boxing\unboxing. There doesn't appear to be any significant CPU performance changes but there is some minor reductions in allocations:
WriteJson<SimpleStructWithProperties>
before:ReadJson<SimpleStructWithProperties>
before:WriteJson<Int32>
beforeReadJson<Int32>
before:Here are all benchmarks with >=3% threshold. No noted regression with latest changes. The benchmarks were run twice; below is the slightly less favorable results: