[Feature Request] [L] Stop using reflection to deserialize JSON

[bgavrilMS]

Reflection is causing tons of problems on the more exotic runtimes such as ARM and Unity. It's also a perf impact, because deserializing JSON via reflection is slow (it discovers the JSON 2 obj mappings first and then it deserialize it).

We should explore eliminating the reflection step. It should be possible to read the JSON in memory and extract each field individually. Each object that supports deserialization would need to implement a simple interface

[jmprieur]

Would it make sense to move to System.Text.JSon?

[bgavrilMS]

We can't move to System.Text because:

• it doesn't support some of our targets
• it doesn't allow for JSON modifications, which is our current strategy for deserializing the token cache and keeping it future proof (i.e. if new fields are encountered, leave them be)

[bgavrilMS]

@pmaytak - are you interested in taking a look at this? It has a perf component to it, so it would be great to understand if we will make some improvement (or at least not regress perf).

[pmaytak]

Related issue #2231 (so it's easier to find).

[jmprieur]

Remaining work;

• For each of the 10 classes (9 remaining), use the methods that serialize and serialize by property (it's manual) => 4 to 8 hours
• Unit testing (same serialization and deserialization as before) - we do already have many tests but there are subtleties like capitalization
• Full test pass of all the scenarios (including with/without broker)
• Check that this works on HoloLens and UWP (or have partners/customers validate)

[pmaytak]

As discussed with @bgavrilMS, removing code that uses reflection is very impractical. The recommendation then is to use Link XML as a solution to enable MSAL to work in Unity projects. This is one of the solutions to this known behavior that is described in Unity Managed code stripping docs. I also updated our wiki with more information.

PR #2403 shows the attempt at serializing without reflection and the complexities that arise. Changes to MsalTokenResponse show the complexity well.

• There's a ton of added boilerplate code, which is more error prone.
• Some things like HttpResponse and its inner .NET classes are complex to serialize.
• This work is not future-proof. It's complex to know all the places where reflection is used. So we might run into the same issue later.

[jmprieur]

@pmaytak : there was already an FAQ for this issue: https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/wiki/Troubleshooting-unity
can we please combine?

[charlesroddie]

There's a ton of added boilerplate code, which is more error prone.

The only thing that looks difficult and error prone from the PR code is knowing whether the conversions need CurrentCulture or InvariantCulture but if that is an important issue then surely it's best to be explicit about it rather than relying on a machine default. If it's not an important issue, then helper methods will trim down this code.

In general making implicit code explicit will reduce bugs in the medium term as it gives more precise specification.

Some things like HttpResponse and its inner .NET classes are complex to serialize.

What is the difficulty of HttpResponse?

This work is not future-proof. It's complex to know all the places where reflection is used.

Replacing serialization code should be fine for now (this issue). In the future linker (currently living in mono/linker) and AOT analysis (currently living in dotnet/runtimelab) will identify reflection.