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

[System.Text.Json]Improve error output for unsupported scenarios in System.Text.Json.Serialization #38061

Merged
merged 4 commits into from
May 31, 2019
Merged

[System.Text.Json]Improve error output for unsupported scenarios in System.Text.Json.Serialization #38061

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b630180
custom exception for parameterless constructors
MarcoRossignoli May 30, 2019
766c653
add polymorphic interface exception
MarcoRossignoli May 30, 2019
0a38ecd
fix ReflectionMaterializer
MarcoRossignoli May 30, 2019
5bb1f3c
address PR feedback
MarcoRossignoli May 31, 2019
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/Resources/Strings.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -368,4 +368,10 @@
<data name="JsonSerializerDoesNotSupportComments" xml:space="preserve">
<value>Comments cannot be stored when deserializing objects, only the Skip and Disallow comment handling modes are supported.</value>
</data>
<data name="DeserializeMissingParameterlessConstructor" xml:space="preserve">
<value>Deserialization of reference types without parameterless constructor is not supported. Type '{0}'</value>
</data>
<data name="DeserializePolymorphicInterface" xml:space="preserve">
<value>Deserialization of interface types is not supported. Type '{0}'</value>
</data>
</root>
13 changes: 13 additions & 0 deletions src/System.Text.Json/src/System/Text/Json/Serialization/JsonSerializer.Read.HandleObject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,19 @@ private static void HandleStartObject(JsonSerializerOptions options, ref Utf8Jso
}

JsonClassInfo classInfo = state.Current.JsonClassInfo;

if (classInfo.CreateObject is null && classInfo.ClassType == ClassType.Object)
{
if (classInfo.Type.IsInterface)
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

@ahsonkhan ahsonkhan May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@MarcoRossignoli MarcoRossignoli May 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

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)

{
ThrowHelper.ThrowInvalidOperationException_DeserializePolymorphicInterface(classInfo.Type);
}
else
{
ThrowHelper.ThrowInvalidOperationException_DeserializeMissingParameterlessConstructor(classInfo.Type);
}
}

state.Current.ReturnValue = classInfo.CreateObject();
}

Expand Down
8 changes: 8 additions & 0 deletions src/System.Text.Json/src/System/Text/Json/Serialization/ReflectionMaterializer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,14 @@ internal sealed class ReflectionMaterializer : ClassMaterializer
{
public override JsonClassInfo.ConstructorDelegate CreateConstructor(Type type)
{
Debug.Assert(type != null);
ConstructorInfo realMethod = type.GetConstructor(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance, binder: null, Type.EmptyTypes, modifiers: null);

if (realMethod == null && !type.IsValueType)
{
return null;
}

return () => Activator.CreateInstance(type);
}

Expand Down
12 changes: 12 additions & 0 deletions src/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,5 +122,17 @@ public static void ThrowInvalidOperationException_SerializationDataExtensionProp
{
throw new InvalidOperationException(SR.Format(SR.SerializationDataExtensionPropertyInvalidElement, jsonClassInfo.Type, jsonClassInfo.DataExtensionProperty.PropertyInfo.Name, invalidType));
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void ThrowInvalidOperationException_DeserializeMissingParameterlessConstructor(Type invalidType)
{
throw new NotSupportedException(SR.Format(SR.DeserializeMissingParameterlessConstructor, invalidType));
}

[MethodImpl(MethodImplOptions.NoInlining)]
public static void ThrowInvalidOperationException_DeserializePolymorphicInterface(Type invalidType)
{
throw new NotSupportedException(SR.Format(SR.DeserializePolymorphicInterface, invalidType));
}
}
}
19 changes: 19 additions & 0 deletions src/System.Text.Json/tests/Serialization/Object.ReadTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,25 @@ public static void ReadObjectFail()
Assert.Throws<JsonException>(() => JsonSerializer.Parse<object>("null."));
}

[Fact]
public static void ReadObjectFail_ReferenceTypeMissingParameterlessConstructor()
{
Assert.Throws<NotSupportedException>(() => JsonSerializer.Parse<PublicParameterizedConstructorTestClass>(@"{""Name"":""Name!""}"));
}

class PublicParameterizedConstructorTestClass
{
private readonly string _name;
public PublicParameterizedConstructorTestClass(string name)
{
_name = name;
}
public string Name
{
get { return _name; }
}
}

[Fact]
public static void ParseUntyped()
{
Expand Down
24 changes: 23 additions & 1 deletion src/System.Text.Json/tests/Serialization/PolymorphicTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,6 +386,28 @@ public static void StaticAnalysisWithRelationship()
Assert.Equal(typeof(UsaCustomer), deserializedCustomer.GetType());
Assert.Equal(typeof(Address), deserializedCustomer.Address.GetType());
((Customer)deserializedCustomer).VerifyNonVirtual();
}
}

[Fact]
public static void PolymorphicInterface_NotSupported()
{
Assert.Throws<NotSupportedException>(() => JsonSerializer.Parse<MyClass>(@"{ ""Value"": ""A value"", ""Thing"": { ""Number"": 123 } }"));
}

class MyClass
{
public string Value { get; set; }
public IThing Thing { get; set; }
}

interface IThing
{
int Number { get; set; }
}

class MyThing : IThing
{
public int Number { get; set; }
}
}
}