Skip to content
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.

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

Add support for MessagePack [Union] types as parameter and return types #490

Merged
merged 8 commits into from
Jul 16, 2020

Conversation

AArnott
Copy link
Member

@AArnott AArnott commented Jul 15, 2020

  • Add several tests to demonstrate the expected use cases.
  • Update the library to take declared argument types, and propagate these and return types to the formatter.
  • Update the MessagePackFormatter to use this type info to serialize using the declared type so that union data wraps the msgpack data when appropriate.
  • Optimize proxies to never allocate 0-length arrays.

Original problem

MessagePack requires knowing the declared type as well as the object to actually serialize/deserialize. The declared type might be BaseType while the value itself might be an instance of DerivedType. When the declared type equals the value's actual runtime type, MessagePack simply serializes the value. But when the declared type is a base type of the value's type, MessagePack looks for a [Union] attribute on the declared type so it knows how to annotate the serialized value so that the runtime type can be recovered on the deserialized side.

It is very important that the serializing and deserializing sides agree on the declared type so that they agree on what the type annotation (if any) should be, otherwise there is a deserialization failure.
The deserializing side always knows the declared type, as this is required to deserialize it in the first place. But while the serializing side always knows the runtime type (since it has the value and can call GetType() on it), the serializer does not always know the declared type. Thus, our MessagePack serialization code does not annotate values with the sometimes necessary Union type data, leading to a failure on the deserializing side.

Design

We can divide the problem into two parts: parameters and return types.
Parameters are sent from the RPC client to the RPC server while return values go the opposite direction.
Because the problem is always on the serializing side, that means on the RPC server side we only need to worry about the declared type of the return value and on the RPC client side we only need to worry about the declared type of the arguments.

Return types

JsonRpc can already discover the declared return types of RPC methods on the server side because JsonRpc invoked the server method after all, so it has direct access to MethodInfo.ReturnType.
JsonRpc needs to pass this declared return type to the formatter and will do this by way of a new property set on JsonRpcResult. This property will not itself be serialized, and will only be set on the server.

Parameter types

Parameter handling breaks down into two groups: positional and named.

Positional arguments

On the client side where arguments are serialized, JsonRpc only has an IReadOnlyList<object?> of arguments. So we have runtime types, but no declared types, where the declared type for a given argument would be the parameter type on the RPC method on the server. JsonRpc.Invoke* methods will need to take a new IReadOnlyList<Type> parameter so the client can list the declared types of each argument as the server expects it.
JsonRpc needs to pass this IReadOnlyList<Type> to the formatter so it can use it while serializing arguments. It does this via a new property on JsonRpcRequest. This property is not serialized, but the formatter will be able to use it on the client side and correlate each declared parameter type with the actual argument value.

Dynamically generated proxies will lazily create a static array for each RPC method with the declared parameter types and pass it to JsonRpc.

Named arguments

Named arguments are provided by a single object. Typically this object has properties to represent each parameter on the RPC method. In this case each argument is provided in its own property. When reflecting over this type and its properties to get the values, we have the PropertyInfo for each and can get the declared type from the PropertyInfo.PropertyType property. We therefore don't need the client to provide any additional type information regarding these arguments.
The MessagePackFormatter captures these property types just before serializing the single parameter object and uses them as necessary.

In another scenario, the named arguments are passed as a name=value dictionary instead of a specialized type with a property for each named parameter.
In this case the MessagePackFormatter has no way to obtain the declared parameter types.
JsonRpc needs to collect this data from the caller and pass it along to the formatter.
Unlike positional arguments that will use IReadOnlyList<Type> to represent each parameter type, named parameters will require an IReadOnlyDictionary<string, Type> so each parameter's type can be looked up by name.
It does this through a new property on JsonRpcRequest. This property is not serialized, but the formatter will be able to use it on the client side and correlate each declared parameter type with the actual argument value.

Dynamically generated proxies support passing named arguments, but always with a specially generated parameter type, so they do not need to create a dictionary of property types, as the MessagePackFormatter will do it while reflecting over the special parameters type the proxy created.

Closes #460

@AArnott AArnott added this to the v2.6 milestone Jul 15, 2020
@AArnott AArnott self-assigned this Jul 15, 2020
@AArnott
Copy link
Member Author

AArnott commented Jul 15, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@codecov-commenter
Copy link

codecov-commenter commented Jul 15, 2020

Codecov Report

Merging #490 into master will increase coverage by 0.02%.
The diff coverage is 97.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #490      +/-   ##
==========================================
+ Coverage   90.33%   90.36%   +0.02%     
==========================================
  Files          50       50              
  Lines        3799     3891      +92     
==========================================
+ Hits         3432     3516      +84     
- Misses        367      375       +8     
Impacted Files Coverage Δ
src/StreamJsonRpc/MessagePackFormatter.cs 92.85% <91.17%> (+0.02%) ⬆️
src/StreamJsonRpc/JsonRpc.cs 92.41% <100.00%> (+0.12%) ⬆️
src/StreamJsonRpc/Protocol/JsonRpcRequest.cs 87.75% <100.00%> (+0.52%) ⬆️
src/StreamJsonRpc/Protocol/JsonRpcResult.cs 66.66% <100.00%> (+3.03%) ⬆️
src/StreamJsonRpc/ProxyGeneration.cs 99.73% <100.00%> (+0.02%) ⬆️
src/StreamJsonRpc/Resources.Designer.cs 69.86% <100.00%> (+0.41%) ⬆️
src/StreamJsonRpc/WebSocketMessageHandler.cs 87.03% <0.00%> (-10.34%) ⬇️

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 fe3d4de...126cf3a. Read the comment docs.

@AArnott AArnott merged commit d108f17 into microsoft:master Jul 16, 2020
@AArnott AArnott deleted the fix460 branch July 16, 2020 17:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MessagePackFormatter fails to deserialize Union types
3 participants