[System.Text.Json]Improve error output for unsupported scenarios in System.Text.Json.Serialization #38061
Conversation
MarcoRossignoli
changed the title
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s), but failed to run 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s), but failed to run 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s), but failed to run 2 pipeline(s). |
|
@safern are there issue with azp? |
|
There might be, I just asked. Do you wanted to run the outerloop legs as well? Maybe close and reopen works? |
|
No I don't need outerloop only regular ci |
src/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
Outdated
Show resolved
Hide resolved
|
|
||
| if (classInfo.CreateObject is null && classInfo.ClassType == ClassType.Object) | ||
| { | ||
| if (classInfo.Type.IsInterface) |
There was a problem hiding this comment.
One item to discuss for future checks like this -- should we do these checks during warm-up (when reading properties via reflection) to not affect run-time perf? The disadvantage of doing that is the given property may not actually ever be deserialized if there is never any json payload for that property.
There was a problem hiding this comment.
Given this is a "NotSupported" check, I think we should do it up-front and not affect run-time perf. The user should know that their model is not currently supported to avoid surprising behavior when the payload changes.
There was a problem hiding this comment.
affect run-time perf
Do you have a sense of how impactful this change is to perf? Should we wait till next week or do you think its minimal?
There was a problem hiding this comment.
One item to discuss for future checks like this -- should we do these checks during warm-up (when reading properties via reflection) to not affect run-time perf? The disadvantage of doing that is the given property may not actually ever be deserialized if there is never any json payload for that property.
I thought about that and I initially did these check on property add...but this could block also "serialization" not only deserialization, so I thought to be close to "delegate usage" to have the check "only" if we want to use that, to have error only if we use it, otherwise we could block some unexpected scenario. I think that first check classInfo.CreateObject is null will be 99% of use cases false it's handled in a good way by jit AFAIK and maybe also by cpu preditcor, so it should be "only"(I know that in context of perf this is offensive 😄) one extra test x, y(not sure but my past test on similar code produce that)
|
Failures are unrelated, merging |
…ystem.Text.Json.Serialization (dotnet/corefx#38061) * custom exception for parameterless constructors * add polymorphic interface exception * fix ReflectionMaterializer * address PR feedback Commit migrated from dotnet/corefx@792eb85
Contrib to https://github.com/dotnet/corefx/issues/37545
fixes https://github.com/dotnet/corefx/issues/37885 polymorphic interface
fixes https://github.com/dotnet/corefx/issues/37537 parameterless ctor
Exception samples
System.InvalidOperationException : Deserialization of reference type without parameterless constructor is not supported. Type System.Text.Json.Serialization.Tests.ObjectTests+PublicParameterizedConstructorTestClassSystem.InvalidOperationException : Deserialization of interface types is not supported. Type System.Text.Json.Serialization.Tests.PolymorphicTests+IThingcc: @ahsonkhan @steveharter