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 RPC marshaling support for custom interfaces #777

Merged
merged 8 commits into from
Mar 18, 2022

Conversation

matteo-prosperi
Copy link
Member

@matteo-prosperi matteo-prosperi commented Mar 7, 2022

This PR addresses #774.

I added the RpcMarshalable attribute that can be applied to an interface that

  • extends IDisposable
  • doesn't have properties
  • doesn't have events

I added unit tests and docs.

Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall. It's exciting to see this come together so quickly.

src/StreamJsonRpc/RpcMarshalableAttribute.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/RpcMarshalableAttribute.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/Resources.resx Outdated Show resolved Hide resolved
src/StreamJsonRpc/JsonMessageFormatter.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/JsonMessageFormatter.cs Outdated Show resolved Hide resolved
test/StreamJsonRpc.Tests/MarshalableProxyTests.cs Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Mar 8, 2022

Codecov Report

Merging #777 (385aec3) into main (114ee4f) will increase coverage by 0.17%.
The diff coverage is 95.89%.

❗ Current head 385aec3 differs from pull request most recent head dc8016f. Consider uploading reports for the commit dc8016f to get more accurate results

@@            Coverage Diff             @@
##             main     #777      +/-   ##
==========================================
+ Coverage   91.69%   91.87%   +0.17%     
==========================================
  Files          61       61              
  Lines        5323     5342      +19     
==========================================
+ Hits         4881     4908      +27     
+ Misses        442      434       -8     
Impacted Files Coverage Δ
src/StreamJsonRpc/JsonMessageFormatter.cs 91.33% <85.71%> (-0.28%) ⬇️
src/StreamJsonRpc/MessagePackFormatter.cs 92.56% <100.00%> (+0.14%) ⬆️
...tion/MessageFormatterRpcMarshaledContextTracker.cs 97.82% <100.00%> (+0.65%) ⬆️
src/StreamJsonRpc/Resources.Designer.cs 68.67% <100.00%> (+1.17%) ⬆️
src/StreamJsonRpc/JsonRpc.cs 94.17% <0.00%> (+0.39%) ⬆️
src/StreamJsonRpc/Reflection/RpcTargetInfo.cs 93.99% <0.00%> (+1.06%) ⬆️
src/StreamJsonRpc/MessageHandlerBase.cs 96.38% <0.00%> (+1.20%) ⬆️

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 114ee4f...dc8016f. Read the comment docs.

@matteo-prosperi matteo-prosperi force-pushed the dev/maprospe/marshalable branch 2 times, most recently from 8ab5d80 to 3570d33 Compare March 10, 2022 17:56
@matteo-prosperi matteo-prosperi requested a review from AArnott March 10, 2022 17:56
@matteo-prosperi matteo-prosperi force-pushed the dev/maprospe/marshalable branch from 3570d33 to fa1f68d Compare March 10, 2022 18:01
@AArnott AArnott changed the title Adding Rpc marshaling support for custom interfaces. Add RPC marshaling support for custom interfaces Mar 10, 2022
Copy link
Member

@AArnott AArnott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good. Just a few touch-up suggestions.

doc/rpc_marshalable_objects.md Outdated Show resolved Hide resolved
src/StreamJsonRpc/MessagePackFormatter.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/JsonMessageFormatter.cs Outdated Show resolved Hide resolved
src/StreamJsonRpc/JsonMessageFormatter.cs Show resolved Hide resolved
src/StreamJsonRpc/JsonMessageFormatter.cs Outdated Show resolved Hide resolved
@AArnott AArnott linked an issue Mar 18, 2022 that may be closed by this pull request
@AArnott AArnott added this to the v2.11 milestone Mar 18, 2022
@matteo-prosperi matteo-prosperi enabled auto-merge (squash) March 18, 2022 21:21
@matteo-prosperi matteo-prosperi merged commit 381dbe8 into main Mar 18, 2022
@matteo-prosperi matteo-prosperi deleted the dev/maprospe/marshalable branch March 18, 2022 22:19
matteo-prosperi added a commit that referenced this pull request Mar 22, 2022
…ng capabilities (#783)

Addressing failures similar to
```
StreamJsonRpc.RemoteMethodNotFoundException : Unable to find method 'xxx' on {no object} for the following reasons: Deserializing JSON-RPC argument with name "observer" and position 1 to type "xxx" failed: Could not create an instance of type xxx. Type is an interface or abstract class and cannot be instantiated. Path 'params[1].__jsonrpc_marshaled'.
```
when calling APIs under the following conditions:
- using Newtonsoft.Json
- the API has marshalable (`IObserver<>`, `IDisposable` or interfaces with `RpcMarshalableAttribute`) objects as parameters or return values
- the user replaces `JsonMessageFormatter`'s `ContractResolver`.

This issue has worsened since #777 due to more behavior being delegated to the `ContractResolver`

This PR integrates the `ContractResolver` provided by the user with `MarshalContractResolver` before the first serialization or deserialization.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make support for marshaled objects available for custom types
4 participants