Client call with named params by willemolding · Pull Request #541 · paritytech/jsonrpc

willemolding · 2020-02-18T01:16:05Z

Add named_params option to RPC derive macro

Closes #540

While the server generated by the derive macro doesn't support calling functions with named params some other systems do (some exclusively). It would still be nice to use the clean derive macro syntax to generate a client in this case.

This PR adds a flag named_params to the rpc method attribute. Example:

#[rpc(name = "add", named_params)]
fn add(&self, a: u32, b: u32) -> Result<String>;

In this case the json-rpc call object produced by calling the generated method on a client will have a params object that takes the key names from the function parameter names.

{
   "jsonrpc": "2.0",
   "params": {"a": 4, "b": 2},
   ...
}

As the server doesn't support this it is important that this cannot be used when deriving server RPC methods. The rpc derive macro will throw a compile time error if the named_params switch is used in any case other than #[rpc(client)]. See (derive/tests/ui/attr-named-params-on-server.rs)

… error string

parity-cla-bot · 2020-02-18T01:16:08Z

It looks like @willemolding hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

willemolding · 2020-02-18T01:20:23Z

[clabot:check]

parity-cla-bot · 2020-02-18T01:20:24Z

It looks like @willemolding signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

ascjones · 2020-02-18T11:12:34Z

+
+	#[rpc(client)]
+	pub trait Rpc {
+		#[rpc(name = "call_with_named", named_params)]


Would it make sense to specify named_params in the trait level attribute instead? On the one hand it's good to have the fine grained control - but in practice wouldn't all methods be either named or positional?

We already have raw_params on the method level. I think this is a good idea to have it here, but an additional attribute on the trait level would be cool tool, especially in the future if we implement server side as well.

tomusdrw

Nice! Looks good to me. Please let us know what you think about @ascjones suggestion about the trait-level attribute.

tomusdrw · 2020-02-18T11:34:03Z

+
+	#[rpc(client)]
+	pub trait Rpc {
+		#[rpc(name = "call_with_named", named_params)]


We already have raw_params on the method level. I think this is a good idea to have it here, but an additional attribute on the trait level would be cool tool, especially in the future if we implement server side as well.

willemolding · 2020-02-20T23:25:32Z

I like the idea of having it at the trait level to set the default, which can then be overridden at the method level. That way we have the fined grained control as well. I'm picturing something like

#[rpc(client, params = "named")] // sets the default to named params
pub trait Rpc {
	#[rpc(name = "call_with_named")] // uses the trait default
        fn call_with_named() -> {
        ...
        #[rpc(name = "call_positional", params = "positional")] // overrides trait default
        fn call_positional() -> {
}

tomusdrw · 2020-02-21T08:10:14Z

That looks really nice, are you willing to change how raw_params work as well? I imagine that params would take any of "raw"|"positional"|"named".

I guess we would need to keep raw_params for one version and mark it deprecated (i.e produce a warning if used).

…mpatible params type

willemolding · 2020-02-24T03:26:21Z

Ok getting pretty close.

There are now three options for params: ["named", "positional", "raw"]. You can also use the old raw_params flag which overrides the setting but I couldn't figure out how to trigger a compiler warning.

This can be set at the trait level (which then becomes the default for all trait methods without a params attribute key. It can also be used at the method level which overrides the default from the trait so both calling type can be used together.

I'm done for today but I would appreciate suggestions for any more tests this might need to be sure it is solid

tomusdrw

Looks good, few minor things.

tomusdrw · 2020-02-25T13:39:39Z

 								let raw_params =
 									get_meta_list(meta).map_or(false, |ml| has_meta_word(RAW_PARAMS_META_WORD, ml));
+								let params_style = match raw_params {
+									true => Ok(Some(ParamStyle::Raw)),


Can we print a deprecation warning here?

Unfortunately I can't figure out a way to emit a warning without using the proc_macro::Diagnostic feature which is unstable

Not sure if we have any other ideas for this or if it is ok to just add a depreciation note to the docs but not the macro

I think in the past we simply used to println the warning. @ascjones do you recall?

Yes we did use a simple println IIRC

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

willemolding · 2020-03-03T05:09:58Z

Thanks for the review @tomusdrw. The only thing lacking is a deprecation warning for using the now-deprecated raw_params flag. Not sure if anyone knows a trick for this or are we happy to just have the updated docs showing the new way?

tomusdrw

Looks good to me! I think deprecation warning (via println if works) would be nice, but is not critical.

tomusdrw · 2020-03-03T12:25:44Z

 								let raw_params =
 									get_meta_list(meta).map_or(false, |ml| has_meta_word(RAW_PARAMS_META_WORD, ml));
+								let params_style = match raw_params {
+									true => Ok(Some(ParamStyle::Raw)),


I think in the past we simply used to println the warning. @ascjones do you recall?

ascjones

LGTM

ascjones · 2020-03-03T14:56:49Z

 								let raw_params =
 									get_meta_list(meta).map_or(false, |ml| has_meta_word(RAW_PARAMS_META_WORD, ml));
+								let params_style = match raw_params {
+									true => Ok(Some(ParamStyle::Raw)),


Yes we did use a simple println IIRC

willemolding · 2020-03-05T03:21:26Z

It looks like using println! won't make a compile time error/warning. It would be a matter of making the generated code contain a runtime println! which seems wrong to me.

I left a placeholder warning so if/when the proc-macro diagnostics becomes stable it will be simple to add it in.

tomusdrw

Fair enough, looks good to merge then.

Could you take a look at the CI failure?

tomusdrw · 2020-03-05T09:29:53Z

Apparently it seems to be upstream issue, could you fix the quote version like here? paritytech/substrate#5135

willemolding · 2020-03-15T00:18:11Z

Apparently it seems to be upstream issue, could you fix the quote version like here? paritytech/substrate#5135

Hmm this doesn't seem to have fixed it. It sure is a pain not being able to see the CI output

tomusdrw · 2020-03-16T10:32:00Z

Hmm this doesn't seem to have fixed it. It sure is a pain not being able to see the CI output

Yeah, sorry about that. Seems that 1.0.2 is yanked. Could you roll back to =1.0.1? I tested that locally and it should work.

tomusdrw · 2020-03-23T16:58:05Z

Thanks!

willemolding added 7 commits February 18, 2020 09:39

add support for json objects in TypedClient.call_method

4bf380d

adds named params switch to #[rpc] method annotation

c65efae

add generating of map style params in macro

187078a

prevent creating a server when using the named_params switch

167612f

add test for attempt to derive with named_params on server. refactors…

4cfa00b

… error string

adds test that correct params object is generated

f158a7f

apply cargo fmt

b0fefbc

willemolding requested review from ascjones and tomusdrw February 18, 2020 01:18

ascjones reviewed Feb 18, 2020

View reviewed changes

tomusdrw approved these changes Feb 18, 2020

View reviewed changes

willemolding added 9 commits February 24, 2020 10:16

adds ParamStyle enum and conversions. Parses this from meta

2e266c7

updates existing tests

7baf3be

adds client side support for raw params

097276c

adds compiler error on invalid params value

66488b2

add client tests

95a945c

adds global switch to trait attribute

2d20978

add logic to override code generation with default from top level

510048f

adds error on changing trait default when generating server with inco…

a6850ae

…mpatible params type

adds tests for preventing server generation with named params

d750f40

willemolding commented Feb 24, 2020

View reviewed changes

Comment thread derive/src/rpc_attr.rs Outdated

Update derive/src/rpc_attr.rs

36bab4e

tomusdrw reviewed Feb 25, 2020

View reviewed changes

Update derive/src/rpc_trait.rs

4721ce2

Co-Authored-By: Tomasz Drwięga <tomusdrw@users.noreply.github.com>

willemolding added 5 commits March 3, 2020 15:39

remove unwrap and throw useful compile time error

e7abc09

fixes error, notifies that server also supports raw params

38bb7f0

add placeholder for future support for named params server side

0d4bb3c

remove raw_params from examples and replace with params = raw

82257ea

run cargo fmt

1933993

tomusdrw approved these changes Mar 3, 2020

View reviewed changes

ascjones approved these changes Mar 3, 2020

View reviewed changes

add deprecation message placeholder

e7e0cd5

tomusdrw approved these changes Mar 5, 2020

View reviewed changes

bump and lock quote version

2c69f44

rollback quote to 1.0.1

15a5b8b

tomusdrw merged commit c64a48a into paritytech:master Mar 23, 2020

ascjones mentioned this pull request Apr 14, 2020

14.1.0 bump #545

Merged

Conversation

willemolding commented Feb 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add named_params option to RPC derive macro

Uh oh!

parity-cla-bot commented Feb 18, 2020

Uh oh!

willemolding commented Feb 18, 2020

Uh oh!

parity-cla-bot commented Feb 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willemolding commented Feb 20, 2020

Uh oh!

tomusdrw commented Feb 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willemolding commented Feb 24, 2020

Uh oh!

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

willemolding commented Mar 3, 2020

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ascjones left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willemolding commented Mar 5, 2020

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

tomusdrw commented Mar 5, 2020

Uh oh!

willemolding commented Mar 15, 2020

Uh oh!

tomusdrw commented Mar 16, 2020

Uh oh!

tomusdrw commented Mar 23, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willemolding commented Feb 18, 2020 •

edited

Loading

tomusdrw commented Feb 21, 2020 •

edited

Loading