Suppress transmission of $type property in named arguments objects

[rruizGit]

Fix for issue #630: CancellationToken no longer works on v2.6.121 when setting TypeNameHandling.Objects.

When enabling TypeNameHandling.Objects for the JsonSerializer, invocations with simple object dictionaries would end with the dictionary's type in the "params".

{
  "jsonrpc": "2.0",
  "method": "$/cancelRequest",
  "params": {
    "$type": "System.Collections.Generic.Dictionary`2[[System.String, mscorlib],[System.Object, mscorlib]], mscorlib",
    "id": 2
  }
}

With this type of message, the JsonMessageFormatter would convert the properties into a IReadOnlyDictionary<string, object>.

return args.Properties().ToDictionary(p => p.Name, p => (object)p.Value);

Unfortunately, this would treat the "type" JToken as a separate parameter which would then cause later failures because the number of parameters would be incorrect. The proposed solution is to first allow the JToken to convert itself to the right type of dictionary which uses and filters out the "type" and then convert it to the IReadOnlyDictionary<string, object>.

return args.ToObject<Dictionary<string, JToken>>().ToDictionary(k => k.Key, k => (object)k.Value);

This solves all the issues discussed in #630.

[AArnott]

A surprisingly simple solution. Thanks for sharing.

However we lack two parts to the fix:

1. We are sending invalid JSON-RPC requests still by transmitting $type as a property. We need to suppress that.
2. We need a regression test for this.

Are you interested in pursuing these items?

[rruizGit]

Andrew, sorry for delay, I was on vacation. I can look into your request. I did think about "filtering out" the type handling for the parameters dictionary while I was working on this solution. But I came across a few problems with that. I will look into it again and get back to you.

First questions related to this topic,

• Suppressing type handling when serializing the "parameters dictionary" also means that no custom type handling is allowed when invoking methods that way. Is that Ok? Or do you mean suppress the $type only for the dictionary but allow it for the parameters in the dictionary? When using dynamic interface proxies this is not an issue because it employs a totally different serialization/deserialization code path.
• In term of unit tests, I don't think the codebase actually has any test with custom type handling. Is that your assessment as well? If so, are you asking me to create unit tests around these scenarios?

[AArnott]

Or do you mean suppress the $type only for the dictionary but allow it for the parameters in the dictionary

Generally the goal is to control what the protocol dictates, and allow the app to do anything within that boundary. So in this case that means the named parameters object has one property for every parameter, and nothing more. But within each argument value, the app can do whatever it wants including having $type properties.

In term of unit tests, I don't think the codebase actually has any test with custom type handling. Is that your assessment as well? If so, are you asking me to create unit tests around these scenarios?

You are probably correct about the test gap. And yes, I'm suggesting we need to fill that gap and then verify that the client never emits a $type property as if there were a $type parameter on the method. This particular test should probably go in JsonRpcClient20InteropTests where we have full insight as an arbitrary server to inspect the message.

[codecov-commenter]

Codecov Report

Merging #651 (09fba5a) into main (3afa0d9) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #651      +/-   ##
==========================================
- Coverage   90.30%   90.22%   -0.08%     
==========================================
  Files          59       59              
  Lines        4950     4952       +2     
==========================================
- Hits         4470     4468       -2     
- Misses        480      484       +4     

Impacted Files	Coverage Δ
src/StreamJsonRpc/JsonMessageFormatter.cs	91.49% <100.00%> (+0.03%)	⬆️
src/StreamJsonRpc/JsonRpc.cs	91.93% <0.00%> (-0.40%)	⬇️
src/StreamJsonRpc/Reflection/RpcTargetInfo.cs	92.01% <0.00%> (-0.39%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3afa0d9...09fba5a. Read the comment docs.

[rruizGit]

@AArnott, here is another attempt at the fix. It preserves compatibility with the Json RPC specification. I also added unit tests mostly by inheriting from some of the existing unit tests and re-executing them with TypeHandling set to "Objects". If you take the fix out you will notice that a lot of those unit tests fail. I ran the fix through our own unit tests which are heavily dependent on TypeHandling and it seems to work well.

Please take a look. Also, I'm thinking it might be worth my time to add some true TypeHandling dependent unit tests were interfaces and sub-types are being serialized/deserialized. Thoughts?

[AArnott]

Thank you for the fix to the product and tests.

I revised the product fix so we were less vulnerable to issues like this in the future, and to add a missing feature (declared types) that was lacking anyway.