Skip to content

Overhaul ValuesJsonConverter #109

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

Merged
RehanSaeed merged 19 commits into from
Dec 19, 2019
Merged

Overhaul ValuesJsonConverter #109

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
4589fd0
Initial refactor of ValuesJsonConverter
Turnerj Dec 12, 2019
908d9e6
Remove branch logic for casting to OneOrMany
Turnerj Dec 13, 2019
bb28364
Improve memory allocation for writes
Turnerj Dec 13, 2019
7df50c5
Updating enum string URI logic
Turnerj Dec 13, 2019
3c34f84
Improving hot path for serialization
Turnerj Dec 13, 2019
33ce37f
Use reader directly instead of JToken
Turnerj Dec 13, 2019
f6b24cc
Prevent list allocation for single item
Turnerj Dec 13, 2019
58cc155
Added missing success marker
Turnerj Dec 13, 2019
536a53a
Better support for external types
Turnerj Dec 14, 2019
d4f08f5
Add GUID-from-string support
Turnerj Dec 14, 2019
a3eb4d3
Adds full support for implicit external types in any namespace
Turnerj Dec 16, 2019
2197c96
Add TimeSpan support for reads
Turnerj Dec 17, 2019
b073901
Add support for implicit array conversion
Turnerj Dec 17, 2019
86c4c66
Add write tests for time-related types
Turnerj Dec 17, 2019
1d40a8a
Add support for TimeSpan as ISO 8601 duration
Turnerj Dec 17, 2019
1b0f726
WriteObject to only take individual items
Turnerj Dec 17, 2019
280d05e
TimeSpan converter doesn't need to handle reads
Turnerj Dec 17, 2019
a546cac
Moved model classes to separate folder
Turnerj Dec 17, 2019
6dc7814
Updated test naming to make intention clearer
Turnerj Dec 17, 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
Prev Previous commit
Next Next commit
Adds full support for implicit external types in any namespace 
Implicit external types refers to properties where `Values` or `OneOrMany` refers to a shared interface and not the concrete type.

Types must be assignable to `IThing` and will need the full namespace and assembly.
  • Loading branch information
@Turnerj
Turnerj committed Dec 16, 2019
commit a3eb4d3148a04d92f6edf38f3b7b3007544e554d
33 changes: 25 additions & 8 deletions Source/Schema.NET/ValuesJsonConverter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,16 @@ namespace Schema.NET
/// <seealso cref="JsonConverter" />
public class ValuesJsonConverter : JsonConverter
{
private const string NamespacePrefix = "Schema.NET.";

private static readonly TypeInfo ThingInterfaceTypeInfo = typeof(IThing).GetTypeInfo();
private static readonly Dictionary<string, Type> BuiltInThingTypeLookup = new Dictionary<string, Type>(StringComparer.Ordinal);

static ValuesJsonConverter()
{
var iThingTypeInfo = typeof(IThing).GetTypeInfo();
var thisAssembly = iThingTypeInfo.Assembly;

var thisAssembly = ThingInterfaceTypeInfo.Assembly;
foreach (var type in thisAssembly.ExportedTypes)
{
var typeInfo = type.GetTypeInfo();
Copy link
Owner

@RehanSaeed RehanSaeed Dec 13, 2019

Choose a reason for hiding this comment

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

What if someone has a custom type? There are devs who have extended Schema.NET with custom schema types built on top of the ones in this project.

Copy link
Collaborator Author

@Turnerj Turnerj Dec 14, 2019

Choose a reason for hiding this comment

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

Yeah, that is a fair call. The old converter did support any type in the "Schema.Net" namespace so if they added any types in a different assembly to that namespace they would be missed by that type caching.

Copy link
Collaborator Author

@Turnerj Turnerj Dec 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Going through how the old support worked, it is a little interesting. Technically it would only support custom types if the generic arguments to Values<> or OneOrMany<> listed that type specifically. That is, the custom type wouldn't work if an interface or any parent type was used instead.

There was a loophole (I say loophole as I'm not sure it was intended this way) but if you added your type to the namespace "Schema.NET" and put the assembly name in the @type property (eg. "MyFancyType, MyAssembly"), it would load your custom type. I didn't pick that up at first as it just seemed like that code was just for loading built-in types. It does however work as a good security measure by requiring the namespace as the idea of loading ANY type and instantiating it could be a big security problem (some poorly written types might actually do work in their constructor or at least be very allocate-y).

So the code as it stood when you saw it supported the former - if a concrete type is used as a generic argument, it would correctly deserialize to it. To have a concrete type as the generic argument is a bit of a chicken-and-egg scenario as to have a custom property with your concrete type requires a concrete type.

What I've done in 536a53a is extend support to allow the latter - if a type is in the "Schema.NET" namespace, it will look the type up the old way. I've kept the cached type lookup as a means to mitigate some allocations from string concatenation and likely are still faster lookups from the dictionary (though I might try a benchmark testing that specific theory out).

Copy link
Owner

@RehanSaeed RehanSaeed Dec 16, 2019

Choose a reason for hiding this comment

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

Interesting. I have never tried adding custom types myself but I have seen a few devs mention that they do so, so it would be good to continue to support them.

If we could extend support and lift the limitation on the Schema.NET namespace, I think that would be a nice improvement.

Copy link
Collaborator Author

@Turnerj Turnerj Dec 16, 2019

Choose a reason for hiding this comment

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

Yeah, happy to try and make support better for custom types though want to be careful we don't start instantiating any-old type otherwise it might open up security issues as we are deriving the type from the payload - might need to look into this more.

Copy link
Owner

@RehanSaeed RehanSaeed Dec 16, 2019

Choose a reason for hiding this comment

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

As long we check that types inherit from Thing, I don't think there should be any security issues.

if (typeInfo.IsClass && iThingTypeInfo.IsAssignableFrom(typeInfo))
if (typeInfo.IsClass && ThingInterfaceTypeInfo.IsAssignableFrom(typeInfo))
{
BuiltInThingTypeLookup.Add(type.Name, type);
}
Expand Down Expand Up @@ -365,8 +362,28 @@ private static bool TryGetConcreteType(string typeName, out Type type)
}
else
{
type = Type.GetType($"{NamespacePrefix}{typeName}", false);
return !(type is null);
try
{
var localType = Type.GetType(typeName, false);
if (ThingInterfaceTypeInfo.IsAssignableFrom(localType.GetTypeInfo()))
{
type = localType;
return !(type is null);
}
else
{
type = null;
return false;
}
}
#pragma warning disable CA1031 // Do not catch general exception types
catch (Exception ex)
{
Debug.WriteLine(ex.Message);
type = null;
return false;
}
#pragma warning restore CA1031 // Do not catch general exception types
}
}
}
Expand Down
9 changes: 5 additions & 4 deletions Tests/Schema.NET.Test/ValuesJsonConverterTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,27 +458,28 @@ public void ReadJson_ExplicitExternalTypes_AllowSharedNamespace()
}

[Fact]
public void ReadJson_ImplicitExternalTypes_DenyCustomNamespace()
public void ReadJson_ImplicitExternalTypes_AllowCustomNamespace()
{
var json = "{\"Property\":[" +
"{" +
"\"@type\":\"ExternalSchemaModelCustomNamespace, Schema.NET.Test\"," +
"\"@type\":\"SomeCustomNamespace.ExternalSchemaModelCustomNamespace, Schema.NET.Test\"," +
"\"name\":\"Property from Thing\"," +
"\"myCustomProperty\":\"My Test String\"" +
"}" +
"]}";
var result = this.DeserializeObject<Values<string, IThing>>(json);
var actual = Assert.Single(result.Value2);
Assert.IsNotType<SomeCustomNamespace.ExternalSchemaModelCustomNamespace>(actual);
Assert.IsType<SomeCustomNamespace.ExternalSchemaModelCustomNamespace>(actual);
Assert.Equal(new[] { "Property from Thing" }, actual.Name);
Assert.Equal(new[] { "My Test String" }, ((SomeCustomNamespace.ExternalSchemaModelCustomNamespace)actual).MyCustomProperty);
}

[Fact]
public void ReadJson_ImplicitExternalTypes_AllowSharedNamespace()
{
var json = "{\"Property\":[" +
"{" +
"\"@type\":\"ExternalSchemaModelSharedNamespace, Schema.NET.Test\"," +
"\"@type\":\"Schema.NET.ExternalSchemaModelSharedNamespace, Schema.NET.Test\"," +
"\"name\":\"Property from Thing\"," +
"\"myCustomProperty\":\"My Test String\"" +
"}" +
Expand Down