[types]: ID type instead of serde_json::RawValue by niklasad1 · Pull Request #325 · paritytech/jsonrpsee

niklasad1 · 2021-05-18T15:01:50Z

Similar should be done on Params and JsonRpcNotifParams to work properly but this PR is large enough as it is.

niklasad1 · 2021-05-18T15:29:34Z

@@ -157,16 +159,17 @@ impl From<SubscriptionId> for JsonValue {
 #[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)]


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?!

dvdplm

Good cleanup here, and good riddance of those *Alloc types. 👍

dvdplm · 2021-05-18T18:44:26Z

 		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) {


So these clones are cheap then you say?

niklasad1 · 2021-05-18T19:09:31Z

-				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()));


Note: I changed the error type to a String in Error::Request to get rid of the alloc type.

Yeah I noticed that. I don't think there much else we can do right?

niklasad1 · 2021-05-18T19:10:52Z

 						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()));


send empty response to notification,

currently we don't care if the method is registered or not (that API is not exposed)

niklasad1 · 2021-05-18T19:11:08Z

+								send_error(Id::Null, &tx, JsonRpcErrorCode::InvalidRequest.into());
 							}
+						} else if let Ok(_batch) = serde_json::from_slice::<Vec<JsonRpcNotification>>(&body) {
+							return Ok::<_, HyperError>(response::ok_response("".into()));


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

niklasad1 · 2021-05-18T19:20:11Z

+	/// Method
+	pub method: &'a str,
+	/// Params.
+	pub params: JsonRpcNotificationParams<'a>,


In another PR; merge JsonRpcNotificationParams and JsonRpcNotificationParamsNotification

niklasad1 · 2021-05-18T19:21:58Z

 	/// Parameter values of the request.
-	pub params: JsonRpcNotificationParams<'a>,
+	#[serde(borrow)]
+	pub params: Option<&'a RawValue>,


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

TarikGul · 2021-05-19T13:46:22Z

 	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.


What does ack mean here?

ACK - acknowledge, maybe it's a bad term ^^

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.

TarikGul

LGTM, After a brief explanation from @niklasad1 I was able to follow along the type changes, and understand the direction of the PR. 👍

niklasad1 added 3 commits May 18, 2021 16:52

get started

653cf78

add additional test

00e7636

fix nits

d03e092

niklasad1 commented May 18, 2021

View reviewed changes

Comment thread types/src/v2/params.rs Outdated

niklasad1 changed the title ~~[types]: typed ID instead of serde_json::RawValue~~ [types]: ID type instead of serde_json::RawValue May 18, 2021

niklasad1 commented May 18, 2021

View reviewed changes

Comment thread types/src/v2/params.rs Outdated

cargo fmt

3affd41

dvdplm approved these changes May 18, 2021

View reviewed changes

niklasad1 added 4 commits May 18, 2021 21:03

[types]: write some tests.

eb07a01

[http server]: send empty response on notifs

e5e9312

[http server]: fix tests

3c7d20f

[rpc module]: send subscription response

ace5cb2

niklasad1 commented May 18, 2021

View reviewed changes

Comment thread types/src/v2/error.rs Outdated

niklasad1 added 4 commits May 18, 2021 21:12

Update types/src/v2/error.rs

9eddc0c

fix nits

78fec4f

Merge branch 'na-jsonrpc-types-id-and-params' of github.com:paritytec…

e95b6c3

…h/jsonrpsee into na-jsonrpc-types-id-and-params

cargo fmt

659ce7d

niklasad1 commented May 18, 2021

View reviewed changes

Comment thread types/src/v2/params.rs Outdated

Update types/src/v2/params.rs

d5c9da9

niklasad1 commented May 18, 2021

View reviewed changes

niklasad1 marked this pull request as ready for review May 18, 2021 19:22

niklasad1 added 3 commits May 19, 2021 10:22

remove needless clone

f81eb77

Merge branch 'na-jsonrpc-types-id-and-params' of github.com:paritytec…

8097e33

…h/jsonrpsee into na-jsonrpc-types-id-and-params

remove dead code

26d3031

niklasad1 commented May 19, 2021

View reviewed changes

Comment thread types/src/v2/error.rs

[types]: impl PartialEq for JsonErrorObject + test

a2264bb

niklasad1 requested a review from TarikGul May 19, 2021 12:25

TarikGul reviewed May 19, 2021

View reviewed changes

use beef::Cow

84c00cd

niklasad1 commented May 19, 2021

View reviewed changes

Comment thread http-server/src/tests.rs Outdated

Update http-server/src/tests.rs

7f901b4

niklasad1 mentioned this pull request May 19, 2021

Types (de)serialization tests #326

Merged

TarikGul approved these changes May 19, 2021

View reviewed changes

niklasad1 merged commit 4e95f43 into master May 19, 2021

niklasad1 deleted the na-jsonrpc-types-id-and-params branch May 19, 2021 15:20

niklasad1 mentioned this pull request May 20, 2021

[tracking issue]: release jsonrpsee 0.2 #251

Closed

13 tasks

dvdplm mentioned this pull request Jun 15, 2021

[tracking] Release jsonrpsee 0.3 #376

Closed

13 tasks

		@@ -157,16 +159,17 @@ impl From<SubscriptionId> for JsonValue {
		#[derive(Debug, PartialEq, Clone, Hash, Eq, Deserialize, Serialize)]

Conversation

niklasad1 commented May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

niklasad1 May 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

dvdplm left a comment

Choose a reason for hiding this comment

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!

Choose a reason for hiding this comment

Uh oh!

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!

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!

TarikGul left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

niklasad1 commented May 18, 2021 •

edited

Loading

niklasad1 May 18, 2021 •

edited

Loading