add `IntoResponse` trait for method calls #1057

niklasad1 · 2023-03-27T09:03:35Z

This PR adds a similar trait as we introduced for RPC subscriptions but for RPC method calls.

/// Something that can be converted into a JSON-RPC method call response.
pub trait IntoResponse {
	type Output: serde::Serialize + ToOwned<Owned = Output>;

	/// Something that can be converted into a JSON-RPC method call response.
	fn into_response(self) -> ResponsePayload<'static, Output>;
}

impl<T: serde::Serialize, E: IntoErrorObject> IntoResponse for Result<T, E> {
	type Output = T;

	fn into_response(self) -> PartialResponse<'static, T> {
		match self {
			Ok(val) => PartialResponse::Result(val),
			Err(e) => PartialResponse::Error(e.into_error_object()),
		}
	}
}

The benefit is that the return types are more flexible and any type that implements IntoResponse can be defined in the RPC proc macro API after this change but it only works for the `proc macro API for the server-only (see #1067 for futher information)

tests/tests/proc_macros.rs

niklasad1 · 2023-03-27T09:10:03Z

tests/tests/helpers.rs

-	module.register_method("say_hello", |_, _| Ok("hello")).unwrap();
-	module.register_method("notif", |_, _| Ok("")).unwrap();
+	module.register_method("say_hello", |_, _| "hello").unwrap();
+	module.register_method("notif", |_, _| "").unwrap();


so yeah, no need to return Result here anymore

niklasad1 · 2023-03-27T09:12:29Z

core/src/server/mod.rs

+///
+/// If the value couldn't be serialized/encoded, jsonrpsee will sent out an error
+/// to the client `response could not be serialized`.
+pub trait IntoResponse {


this is the essential part of this PR

niklasad1 · 2023-03-27T09:14:22Z

core/src/server/mod.rs

+	type Output: serde::Serialize;
+
+	/// Something that can be converted into a JSON-RPC method call response.
+	fn into_response(self) -> PartialResponse<Self::Output>;


We may want to return Result here and serialize the response in into_response but I don't recall why I did it this way. Might be better to have the users do this explicitly

Currently the entire Response is serialized in a helper function which will emit a log and sent out an error to the method call.

I guess I'm wondering ; can we justify the differences between this and

pub trait IntoSubscriptionCloseResponse { fn into_response(self) -> SubscriptionCloseResponse; }

Right now IntoResponse is really quite different (more generic, for instance with its associated param, and so on). Should these traits be aiming to be as consistent with eachother as makes sense, or are there good reasons for the differences that we can explain?

I would say that they are fundamentally different mainly because IntoResponse is something that converts a response to a JSON-RPC response or JSON-RPC error object (a response with the result or error entry). IntoSubscriptionClose is really just something that is converted into a subscription notification.

In practice, users would not need to implement IntoResponse except some rare edge-cases for custom types that are not Result<T, E>.

For example you could return:

enum MyResponse { Success(String), Error1, Error2 }

where one would need to implement this trait

Thus, is in practice the only trait that folks needs to implement is IntoErrorObject where some state/error is converted into a JSON-RPC error code, message and similar. As long folks returns Result<T, IntoErrorObject or primitive types IntoResponse should already be implemented (perhaps we should add it for sequences and tuple as well)

I would say that they are fundamentally different mainly because IntoResponse is something that converts a response to a JSON-RPC response or JSON-RPC error object (a response with the result or error entry). IntoSubscriptionClose is really just something that is converted into a subscription notification.

I guess what I meant was that they both basically take some type and return some part of a JSON response, but this one comes with an associated type and is more generic, and this isn't. It's not a huge deal for me; just wondering whether there is anything that should be made more consistent between these traits, but happy if not :)

we want to enforce these formats:

{"jsonrpc": "2.0", "result": -19, "id": 2} {"jsonrpc": "2.0", "error": {"code": -32601, "message": "Method not found"}, "id": "1"}

and be flexible enough for folks to use so we could serialize the response in the IntoResponse impl then remove the generic stuff which probably is better

I guess efficiency is the main thing; if we can be less generic ie have something like:

pub trait IntoResponse { fn into_response(self) -> Response; // returns some hardcoded Response type }

Where into_response is serializing into some non-generic type, and if that doesn't lose out in efficiency (because we have to serializie stuff anyway or whatever) then I geuss it'd be easier to udnerstand and more consistent with the subscription version.

But if having the generic PartialResponse<Self::Output> type thing makes stuff more efficient then I get it :)

core/src/client/async_client/mod.rs

…-response

jsdw · 2023-04-04T13:49:55Z

benches/helpers.rs

-		.register_async_method(ASYNC_FAST_CALL, |_, _| async { Result::<_, jsonrpsee::core::Error>::Ok("lo") })
-		.unwrap();
+	module.register_method(SYNC_FAST_CALL, |_, _| "lo").unwrap();
+	module.register_async_method(ASYNC_FAST_CALL, |_, _| async { "lo" }).unwrap();


I'm surprised but happy that you can remove the type annotations!

it's cheating I implemented the IntoResponse trait for the primitive types so that can go away :P

types/src/response.rs

Bumps [actions/checkout](https://github.com/actions/checkout) from 3.4.0 to 3.5.0. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v3.4.0...v3.5.0) --- updated-dependencies: - dependency-name: actions/checkout dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [baptiste0928/cargo-install](https://github.com/baptiste0928/cargo-install) from 1 to 2. - [Release notes](https://github.com/baptiste0928/cargo-install/releases) - [Changelog](https://github.com/baptiste0928/cargo-install/blob/main/CHANGELOG.md) - [Commits](baptiste0928/cargo-install@v1...v2) --- updated-dependencies: - dependency-name: baptiste0928/cargo-install dependency-type: direct:production update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* fix: tokio v1.27 * Update server/src/transport/ws.rs * fix rustdoc * Update server/src/transport/ws.rs * Update server/src/transport/ws.rs * no more futuredriver for incoming conns * add comment for unclear code

niklasad1 · 2023-04-05T11:46:19Z

core/src/server/mod.rs

+	}
+}
+
+impl_into_response!(


we could remove these because it doesn't work to implement these for [T], Vec<T> and other sequence types because of the result impl above

I guess these are just convenience wrappers for when people want to return these basic types, so I don't see any issue keeping them around personally :) Makes it easier to write examples and such anyway!

Possible an impl for Option<T> where T: Serialize would be nice, as much as anything to let people write Some(arbitrary_thing_that_can_be_serialized) and not need to provide a type for E, but I guess mostly people will be returning Results.

types/src/response.rs

jsdw · 2023-04-05T15:57:07Z

types/src/response.rs

+	}
+}
+
+impl<'a> ResponsePayload<'a, ()> {


Nit: Maybe it doesn't matter but couldn't () be T instead and these methods part of the above impl block? (T is never used, so downside is you may need to specify it, upside is that you can create an error_borrowed() etc that would be compatible with any other ResponsePayload<T> is that ever mattered)

I did it like that because specifying the () all over the place for error stuff is annoying

jsdw · 2023-04-05T16:11:43Z

core/src/server/mod.rs

+/// to the client `response could not be serialized`.
+pub trait IntoResponse {
+	/// Output.
+	type Output: serde::Serialize + ToOwned<Owned = Self::Output>;


Aah I see; it's a shame about the ToOwned but I see why!

ToOwned is impl for all T where T: Clone, so I wonder whether there type Output: serde::Serialize + Clone would be possible also and maybe a touch cleaner looking?. Hmm but the current approach is more "correct" though. Will have to ponder this, but yeah maybe it's best as it is here :)

I don't know so I was strict and went with ToOwned and I don't really care.

Seems like it's equal to Clone in practice anyway.

jsdw · 2023-04-05T16:17:24Z

core/src/server/rpc_module.rs

+		// TODO: hack around the lifetime on the `SubscriptionId` by deserialize first to serde_json::Value.
+		let as_success: ResponseSuccess<serde_json::Value> = serde_json::from_str::<Response<_>>(&resp.result)?
+			.try_into()
+			.map_err(|e| Error::Call(CallError::Custom(e)))?;


Ah so ResponseSuccess<SubscriptionId> wouldn't work? SubscriptionId looks like it would be able to borrow the string OK from the input (and then you could into_owned() on it to make it 'static); I wonder what didn't work here basically :)

so if I deserialize to to SubscriptionId<'a> then later calls sub_id.into_owned() then serde complains about static lifetime issues

jsdw · 2023-04-05T16:19:57Z

proc-macros/src/helpers.rs

@@ -87,7 +87,7 @@ fn find_jsonrpsee_crate(crate_names: &[&str]) -> Result<proc_macro2::TokenStream
 ///  use jsonrpsee::core::{RpcResult, SubscriptionResult};
 ///
 ///  #[rpc(client, server)]
-///  pub trait RpcTrait<A, B, C> {
+///  pub trait RpcTrait<A, B: Clone, C> {


Out of interest, why did this need adding?

(some question for some other Clone's. Is it to do with the ToOwned bound on the response? Will this make using the macro more painful?)

Every response type for the server needs to implement Clone/ToOwned

such as

fn call() -> Result<T, Error>;

T will be used in IntoResponse trait and thus must implement Clone.

Yes, it will be more annoying to use but Alex added these custom bounds on the traits which is okay I guess.

it's a trade-off all these annoyance can away with separate serialize/deserialize types :)

That's why I skipped these complicated stuff in the first place but reduces some technical debt with duplicate types and hard to read.

proc-macros/src/helpers.rs

IntoResponse trait for rpc calls

69726c9

niklasad1 commented Mar 27, 2023

View reviewed changes

tests/tests/proc_macros.rs Outdated Show resolved Hide resolved

niklasad1 commented Mar 27, 2023

View reviewed changes

niklasad1 force-pushed the na-method-call-into-response branch from 221da19 to 4f9567d Compare March 27, 2023 09:11

niklasad1 commented Mar 27, 2023

View reviewed changes

remove ErrorResponse

4f9567d

niklasad1 commented Mar 27, 2023

View reviewed changes

niklasad1 changed the title ~~WIP: add IntroResponse trait for method calls~~ WIP: add IntoResponse trait for method calls Mar 27, 2023

jsdw reviewed Mar 27, 2023

View reviewed changes

core/src/client/async_client/mod.rs Outdated Show resolved Hide resolved

niklasad1 added 4 commits April 3, 2023 14:19

cleanup, fix nits

725caea

Merge remote-tracking branch 'origin/master' into na-method-call-into…

5e62960

…-response

separate types from ser/deserialization

fba5dc3

fix uncommented code

f4896e6

niklasad1 changed the title ~~WIP: add IntoResponse trait for method calls~~ add IntoResponse trait for method calls Apr 4, 2023

niklasad1 added 2 commits April 4, 2023 11:33

add Success type

69ff551

add missing dev-dep

fbedd21

niklasad1 marked this pull request as ready for review April 4, 2023 09:46

jsdw reviewed Apr 4, 2023

View reviewed changes

types/src/response.rs Outdated Show resolved Hide resolved

niklasad1 mentioned this pull request Apr 4, 2023

proc macros: client API does only work Result<T, E and can't be used with custom return types #1067

Open

niklasad1 and others added 10 commits April 4, 2023 17:16

fix tests with issue link

7c074b5

fix tests

e605676

add missing file

dbef27f

PartialResponse -> ResponsePayload

cb63a47

fix: tokio v1.27 (#1062)

777449f

* fix: tokio v1.27 * Update server/src/transport/ws.rs * fix rustdoc * Update server/src/transport/ws.rs * Update server/src/transport/ws.rs * no more futuredriver for incoming conns * add comment for unclear code

ResponsePayload Cow-like

881390f

make ResponsePayload Cow-like

92a52e6

fix ugly code

287cd69

niklasad1 commented Apr 5, 2023

View reviewed changes

cleanup

a30946f

jsdw reviewed Apr 5, 2023

View reviewed changes

types/src/response.rs Outdated Show resolved Hide resolved

jsdw reviewed Apr 5, 2023

View reviewed changes

niklasad1 added 3 commits April 5, 2023 19:08

address grumbles

7aa5bc4

ToOwned -> Clone

1e736c2

compile-time tests to workaround rustc bug

6bba426

jsdw approved these changes Apr 11, 2023

View reviewed changes

lexnv approved these changes Apr 11, 2023

View reviewed changes

niklasad1 commented Apr 11, 2023

View reviewed changes

proc-macros/src/helpers.rs Outdated Show resolved Hide resolved

niklasad1 added 2 commits April 11, 2023 17:17

Update proc-macros/src/helpers.rs

dc45c7c

add missing impls for Vec and [T; N]

0b2c2f8

niklasad1 merged commit 191167a into master Apr 11, 2023

niklasad1 deleted the na-method-call-into-response branch April 11, 2023 16:49

niklasad1 mentioned this pull request Apr 13, 2023

add missing tests for ErrorObject from #1057 #1078

Merged

niklasad1 mentioned this pull request Apr 23, 2023

proc macros: IntoResponse doesn't work for parameters that isn't Result #1093

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `IntoResponse` trait for method calls #1057

add `IntoResponse` trait for method calls #1057

niklasad1 commented Mar 27, 2023 •

edited

Loading

niklasad1 Mar 27, 2023

niklasad1 Mar 27, 2023

niklasad1 Mar 27, 2023 •

edited

Loading

jsdw Mar 27, 2023

niklasad1 Mar 27, 2023 •

edited

Loading

jsdw Mar 27, 2023

niklasad1 Mar 27, 2023 •

edited

Loading

jsdw Mar 28, 2023

jsdw Apr 4, 2023

niklasad1 Apr 4, 2023

niklasad1 Apr 5, 2023

jsdw Apr 5, 2023 •

edited

Loading

jsdw Apr 5, 2023

niklasad1 Apr 5, 2023

jsdw Apr 5, 2023

niklasad1 Apr 5, 2023

jsdw Apr 5, 2023

niklasad1 Apr 5, 2023

jsdw Apr 5, 2023

jsdw Apr 5, 2023

niklasad1 Apr 5, 2023 •

edited

Loading

niklasad1 Apr 5, 2023

add IntoResponse trait for method calls #1057

add IntoResponse trait for method calls #1057

Conversation

niklasad1 commented Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

niklasad1 Apr 5, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

add `IntoResponse` trait for method calls #1057

add `IntoResponse` trait for method calls #1057

niklasad1 commented Mar 27, 2023 •

edited

Loading

niklasad1 Mar 27, 2023 •

edited

Loading

niklasad1 Mar 27, 2023 •

edited

Loading

niklasad1 Mar 27, 2023 •

edited

Loading

jsdw Apr 5, 2023 •

edited

Loading

niklasad1 Apr 5, 2023 •

edited

Loading