[ClientRuntime] Fixed polymorphic deserialization

[olydis]

Issue 1 (see 1st commit for repro): Multi-level inheritance

Happens when <static type of property> != <base type> && <static type of property> != <leaf type> && <dynamic type of property> == <leaf type>
Then <dynamic type of property> != <leaf type> after round trip.

Description

Polymorphic deserialization has broken cases. Consider the following scenario:

class A {}
class B : A {}
class C : B {}

An API method returning an A will call JsonConvert.DeserializeObject<A>(...response body...). The PolymorphicDeserializeJsonConverter we have will successfully deserialize objects of dynamic type A, B and C using the discriminator.
Now, there may be another API method returning B. AutoRest will (reasonably) emit JsonConvert.DeserializeObject<B>(...response body...). Note that the service may return an object of dynamic type B or C. However, PolymorphicDeserializeJsonConverter will not be aware that it still applies so we will always deserialize into B!

Summary

Current behavior: PolymorphicDeserializeJsonConverter only works correctly for JsonConvert.DeserializeObject<T> where T = <root type> (the root type is the type that the discriminator was declared on)
Expected behavior: PolymorphicDeserializeJsonConverter works correctly for JsonConvert.DeserializeObject<T> where T : <root type>, i.e. T is any type in the inheritance hierarchy.

Note that this problem is only exposed for hierarchies of depth at least 2 since otherwise you are dealing with either the root type (which the converter works fine for) or a leaf type (which works fine natively).

Related issue: Azure/autorest#2430

Issue 2 (see 2nd commit for repro): Polymorphic property on base class of polymorphic hierarchy

Happens when <static type of property's declaring class> == <base type> && <dynamic type of property's declaring class> == <base type> && <dynamic type of property> != <static type of property>
Then <dynamic type of property> == <static type of property> after round trip.

Description

Consider the following scenario:

class A { public A Friend { get; set; } }
class B : A {}

Then

new A { Friend = new A() } // round trips
new A { Friend = new B() } // fails round trip ("Friend" will have dyn. type A!)
new B { Friend = new A() } // round trips
new B { Friend = new B() } // round trips

Summary

When polymorphic types are deserialized and the base type is detected (above: A), the object is deserialized with the default deserializer, i.e. without passing along the polymorphic deserializer.
As a result, any properties (transitively) of the object will be naively deserialized into their static types.

---

This checklist is used to make sure that common guidelines for a pull request are followed.

• I have read the contribution guidelines.
• The pull request does not introduce breaking changes.

General Guidelines

• Title of the pull request is clear and informative.
• There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

• Pull request includes test coverage for the included changes.

[olydis]

@shahabhijeet any clue why CI no longer terminates? It actually is hanging up on the CR tests, so it must be my change. Could it be the fact that I'm using reflection? WTF.

[shahabhijeet]

@olydis looking at the logs it is not clear at all as to why the test process was terminated.

[shahabhijeet]

@olydis do we have any negative tests, where it fails to convert if we reverse the order of inheritance hierarchy? If not we should add those.

[olydis]

will add

[olydis]

@shahabhijeet well, it says timeout, but I don't get how my test times out on both Linux and Windows... will try to repro locally, but it definitely ran through on my machine before 😞

[olydis]

@shahabhijeet investigating the non-termination I exposed an even more fundamental issue... my fix will have to be more sophisticated 😉

[olydis]

@markcowl @shahabhijeet Note that I had to essentially rewrite the deserializer from scratch, it was very fundamentally broken, I'll gladly share details.

In short: Calling ToObject reevaluates JSON converters. We called that from inside a JSON converter. That quickly leads to infinite recursion and an SO unless you "trick around" a bit to prevent recusrsion.
Trick 1: CanConvert only returns true if the type asked for is the root of the hierarchy. After one iteration of the converter, the type will be more narrow. => termination since CanConvert now returns false (but this causes issue 1)
Trick 2: If the discriminator actually points to the root type of the hierarchy, the object is deserialized without further application of any converters to the entire object tree. => termination since... no more converters (but this causes issue 2)

My current approach explicitly traverses down into the properties before calling out to Newtonsoft.JSON again.

[olydis]

@shahabhijeet MachineLearning tests with the weird boolean toggle problem (that I really couldn't connect to polymorphic deserialization) pass now! All I did was rebase onto your most recent changes in psSdkJson6 and it worked out of the box. I haven't committed my version bumps, that may simplify combining our changes for the 2.4.0 release.