[types]: ID type instead of serde_json::RawValue #325
Conversation
| @@ -157,16 +159,17 @@ impl From<SubscriptionId> for JsonValue { | |||
| #[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)] | |||
There was a problem hiding this comment.
Clone should be cheap as deserialize just borrows the String AFAIU.
Then in the client code we just use Id::Number for now.
Thus, should never allocate in server at least?!
niklasad1
changed the title
| @@ -141,12 +141,12 @@ async fn background_task( | |||
| let execute = move |tx: &MethodSink, req: JsonRpcRequest| { | |||
| if let Some(method) = methods.get(&*req.method) { | |||
| let params = RpcParams::new(req.params.map(|params| params.get())); | |||
| if let Err(err) = (method)(req.id, params, &tx, conn_id) { | |||
| if let Err(err) = (method)(req.id.clone(), params, &tx, conn_id) { | |||
There was a problem hiding this comment.
So these clones are cheap then you say?
| let err: JsonRpcErrorAlloc = serde_json::from_slice(&body).map_err(Error::ParseError)?; | ||
| return Err(Error::Request(err)); | ||
| let err: JsonRpcError = serde_json::from_slice(&body).map_err(Error::ParseError)?; | ||
| return Err(Error::Request(err.to_string())); |
There was a problem hiding this comment.
Note: I changed the error type to a String in Error::Request to get rid of the alloc type.
There was a problem hiding this comment.
Yeah I noticed that. I don't think there much else we can do right?
| @@ -211,23 +212,27 @@ impl Server { | |||
| // Our [issue](https://github.com/paritytech/jsonrpsee/issues/296). | |||
| if let Ok(req) = serde_json::from_slice::<JsonRpcRequest>(&body) { | |||
| execute(&tx, req); | |||
| } else if let Ok(_req) = serde_json::from_slice::<JsonRpcNotification>(&body) { | |||
| return Ok::<_, HyperError>(response::ok_response("".into())); | |||
There was a problem hiding this comment.
send empty response to notification,
currently we don't care if the method is registered or not (that API is not exposed)
| } | ||
| } else if let Ok(_batch) = serde_json::from_slice::<Vec<JsonRpcNotification>>(&body) { | ||
| return Ok::<_, HyperError>(response::ok_response("".into())); |
There was a problem hiding this comment.
send empty response to batch notification,
currently we don't care if the method is registered or not (that API is not exposed)
…h/jsonrpsee into na-jsonrpc-types-id-and-params
| /// Method | ||
| pub method: &'a str, | ||
| /// Params. | ||
| pub params: JsonRpcNotificationParams<'a>, |
There was a problem hiding this comment.
In another PR; merge JsonRpcNotificationParams and JsonRpcNotificationParamsNotification
| pub struct JsonRpcNotification<'a> { | ||
| /// JSON-RPC version. | ||
| pub jsonrpc: TwoPointZero, | ||
| /// Name of the method to be invoked. | ||
| pub method: &'a str, | ||
| /// Parameter values of the request. | ||
| pub params: JsonRpcNotificationParams<'a>, | ||
| #[serde(borrow)] | ||
| pub params: Option<&'a RawValue>, |
There was a problem hiding this comment.
In another PR, hopefully merge JsonRpcParams/RpcParams and replace RawValue here,
That's the reason why some de-serialize tests still fail
…h/jsonrpsee into na-jsonrpc-types-id-and-params
http-server/src/tests.rs
Outdated
| let response = http_request(req.into(), uri).await.unwrap(); | ||
| assert_eq!(response.status, StatusCode::OK); | ||
| // Note: this is *not* according to spec. Response should be the empty string, `""`. | ||
| assert_eq!(response.body, r#"[{"jsonrpc":"2.0","result":"","id":null},{"jsonrpc":"2.0","result":"","id":null}]"#); | ||
| // Note: on HTTP we ack the notification with an empty response. |
There was a problem hiding this comment.
ACK - acknowledge, maybe it's a bad term ^^
There was a problem hiding this comment.
Ahh makes sense, yea I wouldn't have been able to deduce that. But now that I know what it means I like it. For the general user, it would probably make sense to write out the whole word though in this case.
There was a problem hiding this comment.
LGTM, After a brief explanation from @niklasad1 I was able to follow along the type changes, and understand the direction of the PR. 👍
Similar should be done on
ParamsandJsonRpcNotifParamsto work properly but this PR is large enough as it is.Further info see #292 (comment)