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

Propagate cause of InvalidParams #463

Merged
merged 48 commits into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from 45 commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
00658e3
Add a test illustrating how to use the `call` convenience method
dvdplm Aug 26, 2021
ff3c0fb
Extend test + review feedback
dvdplm Sep 6, 2021
4902527
Merge branch 'master' into dp-add-a-test
dvdplm Sep 7, 2021
6d35197
log
dvdplm Sep 7, 2021
befb314
log
dvdplm Sep 7, 2021
ea50b0d
log
dvdplm Sep 7, 2021
bbd6b62
log
dvdplm Sep 7, 2021
b665e34
log
dvdplm Sep 7, 2021
ebf33fa
log
dvdplm Sep 7, 2021
5ef08a5
log
dvdplm Sep 7, 2021
02343c2
log
dvdplm Sep 7, 2021
e568c6c
log
dvdplm Sep 7, 2021
2ffe324
log
dvdplm Sep 7, 2021
ac2c36a
log
dvdplm Sep 7, 2021
5183cea
log
dvdplm Sep 7, 2021
5ec442a
log
dvdplm Sep 8, 2021
4c60d08
log
dvdplm Sep 8, 2021
e62a0de
log
dvdplm Sep 8, 2021
51fd476
Add Methods::test_subscription
dvdplm Sep 10, 2021
d2e5742
Add call_with test helper (ty @niklas!) + cleanup
dvdplm Sep 10, 2021
c2c78d4
Remove todo (part of https://github.com/paritytech/jsonrpsee/issues/457)
dvdplm Sep 10, 2021
677dde0
fmt
dvdplm Sep 10, 2021
b3615b5
Let `test_subscription` be called from other crates
dvdplm Sep 10, 2021
cbd5630
Manually fix indentation
dvdplm Sep 11, 2021
b7a25d4
Merge branch 'master' into dp-debug-substrate-tests
dvdplm Sep 11, 2021
fb68340
fmt
dvdplm Sep 11, 2021
1e74565
SSself-review grumbles
dvdplm Sep 11, 2021
c82592c
Merge branch 'dp-invalid-params-carry-context' into dp-invalid-params…
dvdplm Sep 11, 2021
e7ba44e
CallError::InvalidParams carries an anyhow::Error
dvdplm Sep 11, 2021
9a8dc4e
fmt
dvdplm Sep 11, 2021
8101f1c
Merge branch 'master' into dp-debug-substrate-tests
dvdplm Sep 13, 2021
a91f72a
Tweak docs
dvdplm Sep 13, 2021
0350b5d
Update utils/src/server/rpc_module.rs
dvdplm Sep 13, 2021
27ca5e3
Merge branch 'dp-debug-substrate-tests' of github.com:paritytech/json…
dvdplm Sep 13, 2021
ac56f98
review grumble
dvdplm Sep 13, 2021
6edbe90
Fix todos
dvdplm Sep 13, 2021
ab64bb1
fmt
dvdplm Sep 13, 2021
3121789
Merge branch 'master' into dp-debug-substrate-tests
dvdplm Sep 13, 2021
34ac99d
Merge branch 'dp-debug-substrate-tests' into dp-invalid-params-carrie…
dvdplm Sep 13, 2021
5dd7f4e
Fixup error messages
dvdplm Sep 13, 2021
0fec7c2
Include source in the error message for `CallError`
dvdplm Sep 13, 2021
dc10e9f
fmt
dvdplm Sep 13, 2021
82e2ca5
Update proc-macros/src/render_server.rs
dvdplm Sep 14, 2021
3098acf
Mention needing jsonrpsee crate in scope
dvdplm Sep 14, 2021
7eb7d0f
Resolve todo
dvdplm Sep 14, 2021
3e827a1
Impl ToRpcParams for 0-sized array
dvdplm Sep 14, 2021
e53a571
Merge branch 'dp-invalid-params-carries-error' of github.com:parityte…
dvdplm Sep 14, 2021
5b2d486
optimized logging
dvdplm Sep 14, 2021
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions http-server/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,12 @@ async fn single_method_call_with_multiple_params_of_different_types() {
async fn single_method_call_with_faulty_params_returns_err() {
let addr = server().with_default_timeout().await.unwrap();
let uri = to_http_uri(addr);
let expected = r#"{"jsonrpc":"2.0","error":{"code":-32602,"message":"invalid type: string \"this should be a number\", expected u64 at line 1 column 26"},"id":1}"#;

let req = r#"{"jsonrpc":"2.0","method":"add", "params":["Invalid"],"id":1}"#;
let req = r#"{"jsonrpc":"2.0","method":"add", "params":["this should be a number"],"id":1}"#;
let response = http_request(req.into(), uri).with_default_timeout().await.unwrap().unwrap();
assert_eq!(response.status, StatusCode::OK);
assert_eq!(response.body, invalid_params(Id::Num(1)));
assert_eq!(response.body, expected);
}

#[tokio::test]
Expand Down
1 change: 1 addition & 0 deletions proc-macros/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ quote = "1.0"
syn = { version = "1.0", default-features = false, features = ["extra-traits", "full", "visit", "parsing"] }
proc-macro-crate = "1"
bae = "0.1.6"
log = "0.4"

[dev-dependencies]
jsonrpsee = { path = "../jsonrpsee", features = ["full"] }
Expand Down
2 changes: 2 additions & 0 deletions proc-macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,8 @@ pub(crate) mod visitor;
/// To use the `FooClient`, just import it in the context. To use the server, the `FooServer` trait must be implemented
/// on your type first.
///
/// Note: you need to import the `jsonrpsee` façade crate in your code for the macro to work properly.
///
/// ## Prerequisites
///
/// - Implementors of the server trait must be `Sync`, `Send`, `Sized` and `'static`. If you want to implement this
Expand Down
19 changes: 16 additions & 3 deletions proc-macros/src/render_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,8 @@ impl RpcDescription {
let rpc_method_name = self.rpc_identifier(&method.name);
// `parsing` is the code associated with parsing structure from the
// provided `RpcParams` object.
// `params_seq` is the comma-delimited sequence of parameters.
// `params_seq` is the comma-delimited sequence of parameters we're passing to the rust function
// called..
let (parsing, params_seq) = self.render_params_decoding(&method.params);

check_name(&rpc_method_name, rust_method_name.span());
Expand Down Expand Up @@ -272,11 +273,23 @@ impl RpcDescription {
let decode_fields = params.iter().map(|(name, ty)| {
if is_option(ty) {
quote! {
let #name: #ty = seq.optional_next()?;
let #name: #ty = match seq.optional_next() {
Ok(v) => v,
Err(e) => {
log::error!("Error parsing optional {:?} as {:?}: {:?}", stringify!(#name), stringify!(#ty), e);
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
return Err(e.into())
}
};
}
} else {
quote! {
let #name: #ty = seq.next()?;
let #name: #ty = match seq.next() {
Ok(v) => v,
Err(e) => {
log::error!("Error parsing {:?} as {:?}: {:?}", stringify!(#name), stringify!(#ty), e);
dvdplm marked this conversation as resolved.
Show resolved Hide resolved
return Err(e.into())
}
};
}
}
});
Expand Down
3 changes: 2 additions & 1 deletion tests/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,4 +13,5 @@ env_logger = "0.9"
futures-channel = { version = "0.3.14", default-features = false }
jsonrpsee = { path = "../jsonrpsee", features = ["full"] }
tokio = { version = "1", features = ["full"] }
serde_json = "1"
serde_json = "1"
log = "0.4"
3 changes: 2 additions & 1 deletion tests/tests/proc_macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,8 @@ async fn macro_optional_param_parsing() {
// Named params using a map
let params = RawValue::from_string(r#"{"a": 22, "c": 50}"#.into()).ok();
let result = module.call("foo_optional_params", params).await.unwrap();
assert_eq!(result, r#"{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params"},"id":0}"#);
let expected = r#"{"jsonrpc":"2.0","error":{"code":-32602,"message":"Invalid params. Expected one of '[', ']' or ',' but found \"{\\\"a\\\": 22, \\\"c\\\": 50}\""},"id":0}"#;
assert_eq!(result, expected);
}

#[tokio::test]
Expand Down
6 changes: 3 additions & 3 deletions types/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ impl<T: fmt::Display> fmt::Display for Mismatch<T> {
#[derive(Debug, thiserror::Error)]
pub enum CallError {
/// Invalid params in the call.
#[error("Invalid params in the call")]
InvalidParams,
#[error("Invalid params in the call: {0}")]
InvalidParams(#[source] anyhow::Error),
Copy link
Member

Choose a reason for hiding this comment

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

This looks good

/// The call failed (let jsonrpsee assign default error code and error message).
#[error("RPC Call failed: {0}")]
Failed(#[from] anyhow::Error),
Expand Down Expand Up @@ -162,7 +162,7 @@ impl Error {
/// Error type with a special `subscription_closed` field to detect that
/// a subscription has been closed to distinguish valid items produced
/// by the server on the subscription stream from an error.
#[derive(Deserialize, Serialize, Debug)]
#[derive(Deserialize, Serialize, Debug, PartialEq)]
pub struct SubscriptionClosedError {
subscription_closed: String,
}
Expand Down
8 changes: 4 additions & 4 deletions types/src/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -88,10 +88,10 @@ pub trait SubscriptionClient: Client {

/// Marker trait for types that can be serialized as JSON array/sequence.
///
/// If your type isn't a sequence such as `String`, `usize` or similar.
/// You could insert it in a tuple, slice, array or Vec for it to work.
/// If your type isn't a sequence, for example `String`, `usize` or similar
/// you must insert it in a tuple, slice, array or Vec for it to work.
pub trait ToRpcParams: Serialize {
/// Serialized the type as a JSON array.
/// Serialize the type as a JSON array.
Copy link
Member

Choose a reason for hiding this comment

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

👍

fn to_rpc_params(&self) -> Result<Box<RawValue>, serde_json::Error> {
serde_json::to_string(&self).map(|json| RawValue::from_string(json).expect("JSON String; qed"))
}
Expand All @@ -110,7 +110,7 @@ macro_rules! array_impls {
}

array_impls! {
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16
Copy link
Contributor

@maciejhirsz maciejhirsz Sep 14, 2021

Choose a reason for hiding this comment

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

This is kind of a tangent, but we could just use const generics now that they are available instead of the macro:

impl<P: Serialize, const N: usize> ToRpcParams for [P; N] {}

Edit: not sure if that works if serde isn't using const generics for arrays yet (and it looks like they don't?), might need an extra bound where [P; N]: Serialize.

Copy link
Member

@niklasad1 niklasad1 Sep 14, 2021

Choose a reason for hiding this comment

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

The problem I think is that serde impls Serialize for [T; 0] .... [T; 32] and impl<P: Serialize, const N: usize> ToRpcParams for [P; N] for 0...usize::MAX?

but yeah I think your snippet should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@niklasad1 tried this and reported back that it doesn't work.

Copy link
Member

Choose a reason for hiding this comment

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

ok, maybe we should open a PR to serde then?!

Copy link
Contributor

Choose a reason for hiding this comment

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

impl<P, const N: usize> ToRpcParams for [P; N] where [P; N]: Serialize {}

Works for me

}

macro_rules! tuple_impls {
Expand Down
24 changes: 18 additions & 6 deletions types/src/v2/params.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

use crate::error::CallError;
use alloc::collections::BTreeMap;
use anyhow::anyhow;
use beef::Cow;
use serde::de::{self, Deserializer, Unexpected, Visitor};
use serde::ser::Serializer;
Expand Down Expand Up @@ -128,7 +129,7 @@ impl<'a> RpcParams<'a> {
{
// NOTE(niklasad1): Option::None is serialized as `null` so we provide that here.
let params = self.0.as_ref().map(AsRef::as_ref).unwrap_or("null");
serde_json::from_str(params).map_err(|_| CallError::InvalidParams)
serde_json::from_str(params).map_err(|e| CallError::InvalidParams(e.into()))
}

/// Attempt to parse parameters as an array of a single value of type `T`, and returns that value.
Expand Down Expand Up @@ -163,15 +164,20 @@ impl<'a> RpcParamsSequence<'a> {
T: Deserialize<'a>,
{
let mut json = self.0;

log::trace!("[next_inner] Params JSON: {:?}", json);
match json.as_bytes().get(0)? {
b']' => {
self.0 = "";

log::trace!("[next_inner] Reached end of sequence.");
return None;
}
b'[' | b',' => json = &json[1..],
_ => return Some(Err(CallError::InvalidParams)),
_ => {
let errmsg = format!("Invalid params. Expected one of '[', ']' or ',' but found {:?}", json);
log::error!("[next_inner] {}", errmsg);
return Some(Err(CallError::InvalidParams(anyhow!(errmsg))));
}
}

let mut iter = serde_json::Deserializer::from_str(json).into_iter::<T>();
Expand All @@ -182,10 +188,16 @@ impl<'a> RpcParamsSequence<'a> {

Some(Ok(value))
}
Err(_) => {
Err(e) => {
log::error!(
"[next_inner] Deserialization to {:?} failed. Error: {:?}, input JSON: {:?}",
std::any::type_name::<T>(),
e,
json
);
self.0 = "";

Some(Err(CallError::InvalidParams))
Some(Err(CallError::InvalidParams(e.into())))
}
}
}
Expand All @@ -211,7 +223,7 @@ impl<'a> RpcParamsSequence<'a> {
{
match self.next_inner() {
Some(result) => result,
None => Err(CallError::InvalidParams),
None => Err(CallError::InvalidParams(anyhow!("No more params"))),
}
}

Expand Down
4 changes: 3 additions & 1 deletion utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,6 @@ server = [

[dev-dependencies]
serde_json = "1.0"
tokio = { version = "1", features = ["macros", "rt"] }
env_logger = "0.9"
tokio = { version = "1", features = ["macros", "rt"] }
jsonrpsee = { path = "../jsonrpsee" }
2 changes: 1 addition & 1 deletion utils/src/server/helpers.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ pub fn send_error(id: Id, tx: &MethodSink, error: JsonRpcErrorObject) {
};

if let Err(err) = tx.unbounded_send(json) {
log::error!("Error sending response to the client: {:?}", err)
log::error!("Could not send error response to the client: {:?}", err)
}
}

Expand Down
Loading