Nancy.Serialization.JsonNet icon indicating copy to clipboard operation
Nancy.Serialization.JsonNet copied to clipboard

Updated JsonNetBodyDeserializer.cs to support array types

Open logiclrd opened this issue 7 years ago • 4 comments

After encountering errors doing model binding to array types, I investigated the root cause and decided to fix it. It looks like JsonNetBodyDeserializer.cs creates a separate instance of the target type from that created during deserialization, and then manually copies properties over in order to support blacklisting of properties (a Nancy feature not directly integrated with Newtonsoft.Json). As part of this, JsonNetBodyDeserializer.cs assumes that it can use Activator.CreateInstance on the destination type. This does not work for arrays, as they do not have a parameterless constructor. The approach I took, since the deserialization is going to require the use of a List<T> anyway, is to leverage the existing List<T> deserialization, and then if the destination type is actually T[], call List<T>'s ToArray method before returning the final value.

Prerequisites

  • [x] I have written a descriptive pull-request title
  • [x] I have verified that there are no overlapping pull-requests open
  • [x] I have verified that I am following the Nancy code style guidelines
  • [x] I have provided test coverage for my change (where applicable)

Description

  • To avoid performance issues that might be associated with using late binding to dynamically call ToArray, I generate dynamic method specializations to perform the call to List<T>.ToArray, and I set up a simple cache of delegates referring to these methods.
  • If CreateObjectWithBlacklistExcluded detects an array type T[], a new method ConvertArray is called which retrieves this converter from the cache (constructing it if necessary) and applies it to the result of ConvertCollection on List<T>.
  • The order of operations in CreateObjectWithBlacklistExcluded is altered so that Activator.CreateInstance is not called on the destination type in this circumstance.
  • Unit tests verify the new functionality with arrays of various element types.

logiclrd avatar Dec 11 '17 19:12 logiclrd

So... I did fill in the CLA, I thought @dnfclas was supposed to replace the cla-required label with cla-signed? I went through the "working on behalf of my employer" flow, though, so maybe that requires manual attention...

logiclrd avatar Dec 11 '17 20:12 logiclrd

All CLA requirements met now. Check out the statuses. We are currently migrating to new CLA system which uses status instead of labels. :) image

MichaelTsengLZ avatar Dec 11 '17 22:12 MichaelTsengLZ

Ahh, okay :-) Cool beans, then.

logiclrd avatar Dec 11 '17 23:12 logiclrd

Ping!

logiclrd avatar May 23 '19 20:05 logiclrd