WIP: feat: constraint params to valid JSONRPC params #4

icorderi · 2017-04-01T00:24:56Z

This is a work in progress

Motivation

Not any json is valid in the params field.

4.2 Parameter Structures

If present, parameters for the rpc call MUST be provided as a Structured value. Either by-position > through an Array or by-name through an Object.

by-position: params MUST be an Array, containing the values in the Server expected order.
by-name: params MUST be an Object, with member names that match the Server expected parameter names. The absence of expected names MAY result in an error being generated. The names MUST match exactly, including case, to the method's expected parameters.

source: jsonrpc2.0 specification

The parsing of "params": null seems to be left to the readers discreation, given that not passing the field is valid, the PR assumes params being present but null is the the same as not being present and will be deserialized as None.

Pending:

~~fix jsonrpc_params~~ ~> parse_params!
turn foobar_anonymous_structs into proper tests
tokio-jsonrpc-derive for auto parse_params!'ish on existing structs
add #[derive(Params)] support for tagged enums

icorderi · 2017-04-01T00:29:05Z

@vorner I wanted to show you this before I finished it, there are still a few XXX and TODO's left, but it should convey the idea.

You will also see a comment on the id field, according to the spec, id is not any json either, it is restricted to a String, Number or null. I'll prepare a separate PR with this change.

vorner · 2017-04-01T15:02:55Z

I know not everything is valid there. I'm not sure however if enforcing the restriction is worth the added code and complexity. Yes, it prevents sending or receiving some invalid RPCs, but then, if someone sends them by a mistake, they get an error anyway (this is especially true on the receiver side). Also, what error do you generate if the received message has the wrong type of parameters? My guess (without any attempt to verify it) is that it would be syntax error.

Furthermore, I don't like the panicking at runtime if the wrong type of parameters is passed. I think it could be enforced by the macro itself (params!(pos 1, 2, 3) instead of params!(pos [1, 2, 3])).

That being said, I'm not completely against the idea, I'm just not convinced.

As for the id, I'm reluctant to enforce it on the receiver side and the library never generates anything but strings (I'd have to think of a way to create a message manually with something else than the string, it might be possible, but I don't think it is easy). So the benefit is probably even lower.

Also, please run the code through rustfmt (or look at the travis output). Currently, the used version of rustfmt is 0.8.0 ‒ I'll probably migrate to the newest one soon, though. I know rustfmt sometimes has suboptimal ideas about what is the best formatting, but it at least enforces some consistency through the code.

vorner · 2017-04-01T14:49:46Z

src/message.rs

+    /// Wether or not there are any params
+    pub fn is_empty(&self) -> bool {
+        match *self {
+            Params::Positional(ref params) => params.len() == 0,


Doesn't vec have is_empty as well? Also, what about an empty map?

Pilot error, fixed

vorner · 2017-04-01T14:50:12Z

src/message.rs

+}
+
+// Helper fuction for skipping the params field serialization when there are no params
+fn params_empty(x: &Option<Params>) -> bool {


This'll need to be public if used from outside in the macro.

icorderi · 2017-04-01T18:30:48Z

Yes, it prevents sending or receiving some invalid RPCs, but then, if someone sends them by a mistake, they get an error anyway (this is especially true on the receiver side)

I tend the prefer preventing bad things in the earliest stage possible.

If we could prevent the sender from creating malformed messages, then sending something we could have known was wrong, have the server try to process it, fail and send an error back, seems unnecessary runtime work.

It's a compile error vs a runtime error.
On the other hand, you are right, it is "added code and complexity".
You already put a lot of work on the server side to parse what comes in as params with jsonrpc_params! so this might not be adding much value.

Also, what error do you generate if the received message has the wrong type of parameters? My guess (without any attempt to verify it) is that it would be syntax error.

We chose to deny it and our current non-tokio jsonrpc returns:

Invalid Request (code: -32600, data: "params field must be an array, object or null").

You can choose to warn or allow in the server side and interpret "params": 8 in positional form as "params": [8] or as an object and try to deserialize it into whatever type the server was expecting.

I don't like the panicking at runtime if the wrong type of parameters is passed

We are in complete agreement, I was looking on how to get something equivalent to json! itself that only parses objects or arrays as top structures so we can error at compile time. That section is still a big WIP.

Let me know if you have changed your mind about this or you think its best to scratch it.

vorner · 2017-04-02T12:13:23Z

I put a lot of effort into the parsing mostly for comfort of use and because I need to do some validation no matter what the client side of my library does. The server-side validation is mandatory (security and sanity wise), the client side is nice, but kind of optional.

Anyway, about changing mind and such. Let me put it differently. I was a bit worried if this was because you wanted to have some code to hack on, or if you have a specific need that this solves. If there's a need and it is worth your time to write it, I'm not really against that. I'll leave the decision up to you, I just wanted to make sure you thought about the other side (the cost).

Especially because I think this might be a bigger task than it looks. For example, it is possible to receive the whole json object as one structure if the structure implements Deserialize (and I actually use that in one of my projects). It does so by calling from_value on the serde-json's Value. If you provide your own type, then you probably need to write your own replacement for from_value as well. And it would be less than elegant to first convert the Params type to Value to then convert it to the struct. And I think, if there's a companion macro for constructing the parameters, such struct as all the parameters feature would be nice too ‒ but again, it might be harder with having own type for parameters.

So, if you want to spend the time on this feature, I won't stop you and I'm OK with including the changes, but there might be some (even yet unknown and unexpected) problems like the above.

icorderi · 2017-04-02T20:25:11Z

It's not a personal hack, it is for production use.
I was about to move our non-tokio jsonrpc libraries to tokio and run into your project.
It looks very clean so I figured I contribute to it instead of creating yet-another-jsonrpc-library.

For example, it is possible to receive the whole json object as one structure if the structure implements Deserialize (and I actually use that in one of my projects). It does so by calling from_value on the serde-json's Value.

I wasn't sure how you were consuming the API, if through raw json or with structs through Deserialize.
We went down the Deserialize road, unfortunatly #[derive(Deserialize)] doesn't work for the following reason:

Take a struct with two fields, Foo { x: usize, y: String }, the serde_derive expansion will only parse the following json "params": { "x": 2, "y": "Bar" } and not "params": [2, "Bar"].

If you provide your own type, then you probably need to write your own replacement for from_value as well. And it would be less than elegant to first convert the Params type to Value to then convert it to the struct.

You can't convert a Params to a Value to then use from_value. That only works on the Params::Named case. The RPC's can come both in positional or named form, it's not up to the server. If you want to be able to consume either you will always need to write a custom from_value to parse the params correctly in both positional and named form.

The boilerplate can be taken care of:

Custom trait + macro,
Deserialize + macro,
macros 1.1 with a tokio-jsonrpc-derive giving us #[derive(FromParams)]

In our case, we didn't have macros 1.1 and we originally had to build this on top of rustc_serialize knowing we were gonna be moving to serde down the line.
So what we did was pair a Command trait with a command! macro that lets you build rpc commands like this:

command!{
    name = "test";
    response = ();

    #[derive(Debug, PartialEq)]
    struct TestCommand {
        pub field1: u8,
        pub field2: Option<String>,
    }
}

The macro implements the Command trait for the struct and also a FromParams trait that allows us to parse it both from Params::Positional and Params::Named.

Wether the macro deserializes from Value or from Params doesn't really make that much difference to me. This PR and the Params is just for compliance with the specification so we reject naked strings and numbers as params.

I'm not attached to the code we have or the way we did things.
At the end of the day from the API consumer perspective what I need is for all of the following cases to parse to a TestCommand with as minimum work on top of just defining the struct as possible.

jsonrpc-client -s "localhost:1234" test 8 "Hello world"
jsonrpc-client -s "localhost:1234" test 8
jsonrpc-client -s "localhost:1234" test { "field1": 8 }
jsonrpc-client -s "localhost:1234" test { "field1": 8, "field2": null }
jsonrpc-client -s "localhost:1234" test { "field1": 8, "field2": "foo" }

icorderi · 2017-04-03T20:34:06Z

@vorner I rebased and cleaned up the params! macro.

params!([1,2,3]);
params!({"x": 7, "y": 8});

I also added support for top level literals:

params!(true); // this will create `"params": [true]`
params!(8); // this will create `"params": [8]`

Additionally you can use Serialize objects:

#[derive(Serialize)]
struct TestParam {
    x: usize,
    y: usize,
}
// by default they get serialized by name
params!(TestParam {x: 1, y:2}); // this will create `"params": {"x": 1, "y": 2}`
// we can serialize structs by position too
params!(pos TestParam {x: 1, y:2}); // this will create `"params": [1, 2]`

You will still find an .unwrap() when dealing with serializable objects, unfortunately that unwrap is all over json_internal! too.

If you want to get rid of it, we can write a try_json_internal! that returns Result<Value, Error>, maybe we can try to push it upstream to serde-json with a try_json!(), there is probably several people that might want to handle errors while using the handy macro.

json! is just a pretty name, it invokes json_internal!

I still need to fix jsonrpc_params! before this can go in.

vorner · 2017-04-04T11:12:31Z

Just to let you know, I don't know when I'll have the time to look through this. I'm not ignoring it, it's in the TODO list, but it may take few days before I get to it.

icorderi · 2017-04-04T21:05:40Z

Yea, that's totally understandable.
Don't feel like I'm pushing, this is not a high priority.

- new `parse_params!` macro - new `try_parse_params!` macro

vorner · 2017-04-13T07:06:33Z

src/message.rs

+    pub fn into_positional(self) -> Self {
+        match self {
+            Params::Named(x) => {
+                let values = x.into_iter()


I don't think serde_json::Map guarantees preserving the order. Therefore, turning the map into array may result in arbitrary order. What is the use case for this method?

Damn... I figured thats why they had a Map instead of using HashMap. I'll have to find another solution for it.

The use is for sending the rpc params in positional form instead of named.

There's a feature that forces the Map to preserve order (I don't remember the name), but enabling it would enable it for whatever other use the program might have for the serde_json crate. Otherwise, Map sorts them (that is why your tests with {x: …, y: …} didn't catch the problem, if you reverse them to {y: …, x: …}, then you might).

However, serde itself has two ways to decode structs, one is by a name and another is by the order, so it may be possible to place some kind of intermediate layer that converts it somehow. I don't know the details, only that some formats serde can decode/encode don't provide names, only the order (I think CBOR is one such format).

vorner · 2017-04-13T07:12:06Z

src/message.rs

+
+    ([ $($tt:tt)+ ]) => {
+        {
+            match $crate::macro_exports::Value::Array(json_internal!(@array [] $($tt)+)) {


Wouldn't match json!([ $($tt)+ ] { be easier? Or just directly $crate::Params::Positional(json_internal!(...))? This construct array and then deconstruct it right away seems a bit strange.

Good catch, fixed

vorner · 2017-04-13T07:16:37Z

src/message.rs

+                let mut xs = Vec::with_capacity(x.len());
+                for (_,v) in x {
+                    xs.push(v);
+                }


Personally, I prefer something like x.map(|(_, v)| v).collect(). I even believe this might be more idiomatic. But maybe that's just a personal preference, so just pointing out the possibility.

Fixed, x.into_iter().map(|(_, v)| v).collect())

vorner · 2017-04-13T07:20:54Z

src/message.rs

+            // if the object serializes to `Null` map it to `None`
+            $crate::macro_exports::Value::Null => None,
+            // encode any other value as an array of 1 item
+            value => Some($crate::Params::Positional(vec![value])),


I'm not sure about the semantics here, how this will work out. What if I want to send single positional vec? So, the result should be [[42]].

Maybe if I see some examples, it'll be clearer, but currently I'm afraid about this heuristic being too surprising in some cases.

You use params!([[7]]), I added it to the test cases.

vorner · 2017-04-13T07:28:32Z

src/server.rs

+                                $crate::message::RpcError::parse_error(format!("{}", err))))
+                    }
+                    None => {
+                        // Review: should we try to parse an object where all fields are missing?


What do you mean? Being able to parse unit structs, for example, from empty objects? I think that is legal. But parsing it from no params is probably not...

That comment is from before the derive. It's referring to this code:

// Try to parse it as if we received no arguments, if we fail DO NOT error let value = ::tokio_jsonrpc::macro_exports::Value::Null; match ::tokio_jsonrpc::macro_exports::from_value(value) { Ok(x) => Some(Ok(x)), Err(_) => None, }

That code will work on the macro too for unit structs but unfortunatly we can't ask if T: Default on the macro to pull exactly same trick I have on the derive.

We could add an option for it, or just tell people to define the struct and use #[derive(Params) for complex use cases.

vorner · 2017-04-13T07:33:19Z

tests/derive.rs

+//!
+//! A server listening on localhost:2345. It reponds to the „now“ method, returning the current
+//! unix timestamp (number of seconds since 1.1. 1970). You can also subscribe to periodic time
+//! updates.


Copy-paste? O:-)

Hmm.... xD removed

vorner · 2017-04-13T08:20:27Z

I know this is still work in progress (so I just skipped commenting on some parts that are yet obviously in flux), but I wanted to give it a quick read rather sooner than later. Generally, I like your interface more than that of the monstrous jsonrpc_params! macro of mine. There are few small nitpicks above. However, there are some points I'd like to discuss.

The macro names are bit too generic to my liking (#[derive(Params)] or params!). While usual identifiers are qualified by the crate and modules, don't clash unless imported, and can be renamed on importing, macro names are global (and I'm not sure if it's possible to rename them), so I believe it would be better to use something like jsonrpc_params!.
I've seen some example to call something like from_params!(params.clone()). Would it be reasonably possible without the cloning? It's not a big problem if not, just that it would be more elegant.
The use of json_internal! seems a bit abusive. The name „internal“ seems to hint that it should not be used outside of its crate. Is it guaranteed (by the serde-json guys) that this is part of their public API and won't change in future versions (there's the problem with macros that if a public macro calls another macro, the other one needs to be public too)? Wouldn't it be a bit better to call json! instead (if possible)?
I sometimes find a bit of formatting that seems alien or inconsistent to me. I know these things are a personal preference and I haven't written any kind of style guide for the crate (and rustfmt changes its tastes so often its a bit unusable for that now), but would you mind if I reformatted some things before merging it?
Please annotate tests with doc-comments. I know these don't render in the docs (as well as docs for private members and methods), but I still like to have them documented what they do (or what the intention is) and the doc comments seem like a reasonable feature to distinguish it from just any ordinary comment.
I believe the json macros are now used not only in tests, so this might be wrong.
I only skimmed the derive subcrate and I'll have to read a lot of docs before properly understanding what it does and how. But I'd like to ask few questions about it.
- It seems rather long. Is that actually needed, or could we abuse serde (or some other crate) to do some of the heavy lifting for us?
- What is the relation between Params and Deserialize traits now? Is the Deserialize used internally?
- I saw it peeks at some of the serdes attributes on enums (I believe). Is it some kind of exception and the rest is handled by serde, or does it need to know any existing serde attribute? There are quite few already (like fancy renaming, mass-renaming, deserialize-with, etc) and I expect more to appear in future. I wouldn't like to be missing some of them (because then it would be basically missing some features and ignoring the attributes, which would be quite surprising and possibly a source of hard to track down bugs) or have to keep up with newly introduced ones.

I know this is a long comment, but there was a lot of code to comment on. As I said, I like the direction it is taking and I hope this'll be just somewhere on the level of fine-tuning it and synchronizing different views of the ideas.

icorderi · 2017-04-15T18:32:48Z

The macro names are bit too generic to my liking

I didn't want to be assuming and replace your jsonrpc_params!. But since you like the params! proposal, we can rename params! to jsonrpc_params! it and move forward from there.

I've seen some example to call something like from_params!(params.clone()). Would it be reasonably possible without the cloning?

I have a branch waiting on this PR that gets rid of that snaffu. from_params! needs to take ownership because of the serde::from_value. My proposal is to have Server::rpc(..) be:

fn rpc(&self, _ctl: &ServerCtl, _method: &str, _params: Option<Params>)

That way we avoid unnecessary cloning.

The use of json_internal! seems a bit abusive...Wouldn't it be a bit better to call json! instead (if possible)?

I will look into it

would you mind if I reformatted some things before merging it?

Go ahead

Please annotate tests with doc-comments.

Will do

I believe the json macros are now used not only in tests, so this might be wrong.

Good catch

derive magic

It seems rather long. Is that actually needed, or could we abuse serde (or some other crate) to do some of the heavy lifting for us?

I am using serde for the heavy lifting.

What is the relation between Params and Deserialize traits now? Is the Deserialize used internally?

Params have be Deserialize too.

Marking an object as Params gives you the ability to deserialize structs and enums from json objects or arrays.

Serde will only let you do one of the two depending on what it is you are deserializing. All the derive magic is doing is to map whatever json we have into something serde will deserialize correctly.

I saw it peeks at some of the serdes attributes on enums (I believe). Is it some kind of exception and the rest is handled by serde, or does it need to know any existing serde attribute?

We peek at two places for two very different reasons:

#[serde(default)] tells serde to use Default::default() if the value is missing. We piggy back on this so that if get a missing or null params instead of returning None we will return Some(whatever-serde-gave-us). If the type does not have default then we return None.
#[serde(untagged)] is for enums, it controls wether the enum variant is part of the json. Currently we don't support untagged, simply because I didn't get around to code it, but I left a TODO for it with an idea on how to get it to work.

I wouldn't like to be missing some of them (because then it would be basically missing some features and ignoring the attributes, which would be quite surprising and possibly a source of hard to track down bugs) or have to keep up with newly introduced ones.

I understand your concern, but there is no need for it. We are not adding/removing any attribute or messing with them in any way. Whatever attributes were used by the user will remain there and serde itself is the one processing them not us.

Once I code support for tagged enums, our only dependency on the attributes will be default which is unlikely to get removed from serde. We can always code our own #[jsonrpc(default)] if that ever happens.

vorner · 2017-04-16T13:50:24Z

I have a branch waiting on this PR that gets rid of that snaffu. from_params! needs to take ownership because of the serde::from_value. My proposal is to have Server::rpc(..) be:
fn rpc(&self, _ctl: &ServerCtl, _method: &str, _params: Option<Params>)
That way we avoid unnecessary cloning.

I was thinking about that. But this is a bit in a conflict with another feature, composability of the server implementations. If one returns None (meaning „I don't know this method“), it is possible to try another in a chain, in a cheap way.

But that could probably be handled in some way. Like refusing to handle a method not by None, but by Either<Future<Result, RpcError>, Params> to send the params back if they are not consumed. But that's only the first idea, maybe there are better ways.

vorner · 2017-06-25T10:59:23Z

This went somehow silent... do you expect to continue work on this, or did you postpone it? Or, do you wait for an answer about something from me? (if so, I haven't noticed the question)

Thanks

icorderi · 2017-06-28T16:25:31Z

@vorner yea, I expect to finish it. A few higher priority things showed up. I'm about a week or so away from being able to focus again on the transition to tokio.

vorner reviewed Apr 1, 2017

View reviewed changes

icorderi force-pushed the topic/params branch from 3f1c559 to f59c647 Compare April 1, 2017 19:28

icorderi force-pushed the topic/params branch from f59c647 to 9981b30 Compare April 3, 2017 20:16

icorderi force-pushed the topic/params branch from 73f9b72 to 4c10ad0 Compare April 5, 2017 18:41

icorderi added 2 commits April 7, 2017 12:45

feat: constraint params to valid JSONRPC params

d699c37

feat: add macros using new Params

f1c50e6

- new `parse_params!` macro - new `try_parse_params!` macro

icorderi force-pushed the topic/params branch 2 times, most recently from a97dc7b to 6b815a8 Compare April 7, 2017 20:34

icorderi added 3 commits April 10, 2017 16:49

feat: Macros 1.1 implementation of #[derive(Params)]

4e6b035

feat: #[derive(Params)] support for enums

4c1c3ef

feat: update server example to use #[derive(Params)]

f1d641a

icorderi force-pushed the topic/params branch from d5f8b12 to f1d641a Compare April 10, 2017 23:49

feat: add Params::into_positional

8d91e3e

vorner reviewed Apr 13, 2017

View reviewed changes

WIP: feat: constraint params to valid JSONRPC params #4

Are you sure you want to change the base?

WIP: feat: constraint params to valid JSONRPC params #4

Uh oh!

Conversation

icorderi commented Apr 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

4.2 Parameter Structures

Uh oh!

icorderi commented Apr 1, 2017

Uh oh!

vorner commented Apr 1, 2017

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!

icorderi commented Apr 1, 2017

Uh oh!

vorner commented Apr 2, 2017

Uh oh!

icorderi commented Apr 2, 2017

Uh oh!

icorderi commented Apr 3, 2017

Uh oh!

vorner commented Apr 4, 2017

Uh oh!

icorderi commented Apr 4, 2017

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

vorner commented Apr 13, 2017

Uh oh!

icorderi commented Apr 15, 2017

derive magic

Uh oh!

vorner commented Apr 16, 2017

Uh oh!

vorner commented Jun 25, 2017

Uh oh!

icorderi commented Jun 28, 2017

Uh oh!

Uh oh!

icorderi commented Apr 1, 2017 •

edited

Loading