Custom errors #977
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -28,7 +28,7 @@ use crate::helpers::generate_where_clause; | |
| use crate::rpc_macro::{RpcDescription, RpcMethod, RpcSubscription}; | ||
| use proc_macro2::TokenStream as TokenStream2; | ||
| use quote::quote; | ||
| use syn::{AngleBracketedGenericArguments, FnArg, GenericArgument, Pat, PatIdent, PatType, PathArguments, TypeParam}; | ||
|
|
||
| impl RpcDescription { | ||
| pub(super) fn render_client(&self) -> Result<TokenStream2, syn::Error> { | ||
|
|
@@ -68,6 +68,36 @@ impl RpcDescription { | |
| Ok(trait_impl) | ||
| } | ||
|
|
||
| fn patch_result_error(&self, ty: &syn::Type) -> syn::Type { | ||
|
There was a problem hiding this comment. I had a play and wonder whether this would be a clearer approach to take: fn return_result_type(&self, ty: &syn::Type) -> TokenStream2 {
// We expect a valid type path.
let syn::Type::Path(type_path) = ty else {
return quote_spanned!(ty.span() => compile_error!("Expecting something like 'Result<Foo, Err>' here. (1)"));
};
// The path (eg std::result::Result) should have a final segment like 'Result'.
let Some(type_name) = type_path.path.segments.last() else {
return quote_spanned!(ty.span() => compile_error!("Expecting this path to end in something like 'Result<Foo, Err>'"));
};
// Get the generic args eg the <T, E> in Result<T, E>.
let PathArguments::AngleBracketed(AngleBracketedGenericArguments { args, .. }) = &type_name.arguments else {
return quote_spanned!(ty.span() => compile_error!("Expecting something like 'Result<Foo, Err>' here, but got no generic args (eg no '<Foo,Err>')."));
};
if type_name.ident == "Result"{
// Result<T, E> should have 2 generic args.
if args.len() != 2 {
return quote_spanned!(args.span() => compile_error!("Expecting two generic args here."));
}
} else if type_name.ident == "RpcResult" {
// RpcResult<T> (an alias we export) should have 1 generic arg.
if args.len() != 1 {
return quote_spanned!(args.span() => compile_error!("Expecting two generic args here."));
}
} else {
// Any other type name isn't allowed.
return quote_spanned!(type_name.span() => compile_error!("The response type should be Result or RpcResult"));
}
// The first generic arg is our custom return type.
// Return a new Result with that custom type and jsonrpsee::core::Error:
let custom_return_type = args.first().unwrap();
let error_type = self.jrps_client_item(quote! { core::Error });
quote!(std::result::Result<#custom_return_type, #error_type>)
}Aside from being documented, it checks for both Result and RpcResult (and verified the number of generics), and returns nicer compile errors which highlight the offending return type when it's wrong. Instead of modifying the type I just looked for the generic param and then return a new Result type which uses it and the standard error. Needs testing properly though but it compiles ok! There was a problem hiding this comment. Yeah, it is a different approach; when choosing between the two I figured I'd rather impose as few restrictions as possible, and thus tried to surgically target the There's also a concern with supporting multiple paths to [ There was a problem hiding this comment. Tried it! One unexpected artifact is this UI warning: This comes from this code: //! Example of using proc macro to generate working client and server.
use jsonrpsee::{core::RpcResult, proc_macros::rpc};
#[rpc(client)]
pub trait Rpc {
#[method(name = "foo")]
async fn async_method(&self, param_a: u8, param_b: String) -> RpcResult<u16>;
#[method(name = "bar")]
fn sync_method(&self) -> RpcResult<u16>;
#[subscription(name = "subscribe", item = String)]
fn sub(&self);
}
fn main() {}If we rewrite the code to use We can easily handle it by passing through the There was a problem hiding this comment. What if user has a custom There was a problem hiding this comment. I think I have a version that covers all concerns now, please take a look. It does error rewrite again to be more permissive but provides those nice compile errors (for which I need to add tests actually!). There was a problem hiding this comment.
I think that is the user has some custom type that isn't something we expect (ie a Result or an RpcResult) then offhand the safest option is to complain, since in the general case we can't possibly know what the structure of said custom type is. @niklasad1 do you have any opinions on this? There was a problem hiding this comment. FYI I had a look at your latest version; it doesn't look any more permissive to me since it still requires the name to be Result or RpcResult (and the Result type still requires 2 args, so it's highly unlikely somebody will be importing a "Result" type alias of that kind anyway). So I still prefer my approach (and avoiding mutating things for clarity) but I'm not super bothered. There was a problem hiding this comment.
Yeah, I agree I think it's better to throw a compile-time error if the error type isn't Result/RpcResult. There was a problem hiding this comment. The case where this solution is more permissive is when the user provides a type called |
||
| let mut path = match ty { | ||
| syn::Type::Path(path) => path.clone(), | ||
| _ => panic!("Client only supports bare or Result values: {:?}", ty), | ||
| }; | ||
|
|
||
| let Some(first_segment) = path.path.segments.first_mut() else { | ||
| return syn::Type::Path(path); | ||
| }; | ||
|
|
||
| if first_segment.ident != "Result" { | ||
MOZGIII marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return syn::Type::Path(path); | ||
| } | ||
|
|
||
| let args = match first_segment.arguments { | ||
| PathArguments::AngleBracketed(AngleBracketedGenericArguments { ref mut args, .. }) => args, | ||
| _ => unreachable!("Unexpected Result structure"), | ||
| }; | ||
|
|
||
| let error = args.last_mut().unwrap(); | ||
MOZGIII marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let error_type = match error { | ||
| GenericArgument::Type(error_type) => error_type, | ||
| _ => unreachable!("Unexpected Result structure"), | ||
| }; | ||
|
|
||
| *error_type = syn::Type::Verbatim(self.jrps_client_item(quote! { core::Error })); | ||
|
|
||
| syn::Type::Path(path) | ||
| } | ||
|
|
||
| fn render_method(&self, method: &RpcMethod) -> Result<TokenStream2, syn::Error> { | ||
| // `jsonrpsee::Error` | ||
| let jrps_error = self.jrps_client_item(quote! { core::Error }); | ||
|
|
@@ -83,6 +113,7 @@ impl RpcDescription { | |
| // `returns` represent the return type of the *rust method* (`Result< <..>, jsonrpsee::core::Error`). | ||
| let (called_method, returns) = if let Some(returns) = &method.returns { | ||
| let called_method = quote::format_ident!("request"); | ||
| let returns = self.patch_result_error(returns); | ||
MOZGIII marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| let returns = quote! { #returns }; | ||
|
|
||
| (called_method, returns) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,88 @@ | ||
| //! Example of using custom errors. | ||
|
|
||
| use std::net::SocketAddr; | ||
|
|
||
| use jsonrpsee::core::async_trait; | ||
| use jsonrpsee::proc_macros::rpc; | ||
| use jsonrpsee::server::ServerBuilder; | ||
| use jsonrpsee::ws_client::*; | ||
|
|
||
| pub enum CustomError { | ||
| One, | ||
| Two { custom_data: u32 }, | ||
| } | ||
|
|
||
| impl From<CustomError> for jsonrpsee::core::Error { | ||
| fn from(err: CustomError) -> Self { | ||
| let code = match &err { | ||
| CustomError::One => 101, | ||
| CustomError::Two { .. } => 102, | ||
| }; | ||
| let data = match &err { | ||
| CustomError::One => None, | ||
| CustomError::Two { custom_data } => Some(serde_json::json!({ "customData": custom_data })), | ||
| }; | ||
|
|
||
| let data = data.map(|val| serde_json::value::to_raw_value(&val).unwrap()); | ||
|
|
||
| let error_object = jsonrpsee::types::ErrorObjectOwned::owned(code, "custom_error", data); | ||
|
|
||
| Self::Call(jsonrpsee::types::error::CallError::Custom(error_object)) | ||
| } | ||
| } | ||
|
|
||
| #[rpc(client, server, namespace = "foo")] | ||
| pub trait Rpc { | ||
| #[method(name = "method1")] | ||
| async fn method1(&self) -> Result<u16, CustomError>; | ||
|
|
||
| #[method(name = "method2")] | ||
| async fn method2(&self) -> Result<u16, CustomError>; | ||
| } | ||
|
|
||
| pub struct RpcServerImpl; | ||
|
|
||
| #[async_trait] | ||
| impl RpcServer for RpcServerImpl { | ||
| async fn method1(&self) -> Result<u16, CustomError> { | ||
| Err(CustomError::One) | ||
| } | ||
|
|
||
| async fn method2(&self) -> Result<u16, CustomError> { | ||
| Err(CustomError::Two { custom_data: 123 }) | ||
| } | ||
| } | ||
|
|
||
| pub async fn server() -> SocketAddr { | ||
| let server = ServerBuilder::default().build("127.0.0.1:0").await.unwrap(); | ||
| let addr = server.local_addr().unwrap(); | ||
| let server_handle = server.start(RpcServerImpl.into_rpc()).unwrap(); | ||
|
|
||
| tokio::spawn(server_handle.stopped()); | ||
|
|
||
| addr | ||
| } | ||
|
|
||
| #[tokio::main] | ||
| async fn main() { | ||
| let server_addr = server().await; | ||
| let server_url = format!("ws://{}", server_addr); | ||
| let client = WsClientBuilder::default().build(&server_url).await.unwrap(); | ||
|
|
||
| let get_error_object = |err| match err { | ||
| jsonrpsee::core::Error::Call(jsonrpsee::types::error::CallError::Custom(object)) => object, | ||
| _ => panic!("wrong error kind: {:?}", err), | ||
| }; | ||
|
|
||
| let error = client.method1().await.unwrap_err(); | ||
| let error_object = get_error_object(error); | ||
| assert_eq!(error_object.code(), 101); | ||
| assert_eq!(error_object.message(), "custom_error"); | ||
| assert!(error_object.data().is_none()); | ||
|
|
||
| let error = client.method2().await.unwrap_err(); | ||
| let error_object = get_error_object(error); | ||
| assert_eq!(error_object.code(), 102); | ||
| assert_eq!(error_object.message(), "custom_error"); | ||
| assert_eq!(error_object.data().unwrap().get(), r#"{"customData":123}"#); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether we'd gain a bit more flexibility ultimately by copying Axum a bit and having a trait which is something like
This could then be impl'd for
Result<V, E> where V: Serialize, E: Into<Error>for instance:fns like this
register_methodwould expect a callback which was likeFn(Params, &Context) -> R(whereR: IntoResponse) and would callinto_response()on the result (perhaps passing in any specific bits they need to as args to that).Then we could impl things like
From<Infallible> for Errorto allowResult<V, Infallible>responses for instance, and perhaps have more room then to consider supporting other response types than Result in the future.Just thinking out loud really; maybe for now it's fine just to support custom errors but still expect a
Resultback..There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I considered it, but ultimately decided against doing huge changes in a single PR. This is something that, I think, can be added later iteratively - and, also, maybe someone else would like to tackle that instead of me. :D I'm fine with looking into it, but I'm also happy to share the fun.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose that we split the error layers into jsonrpsee's and application level, and, further, split the jsonrpsee's error from one single struct into separate
ClientErrorandServerErroras there are very distinctly different in handling. I think it would significantly improve the ergonomics, and unlock more explicit and sensible error customization for the client side, as this PR only addresses the server side really.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you like the direction I proposed I'll create an issue for splitting the errors and mark this discussion as resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@niklasad1 whatcha reckon? I think you were less keen on splitting the errors?
But in any case, I think for now I'm fine with just allowing the custom error and not being more general; perhaps in some future iteration we can add the above sort of thing if it makes sense :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it will be weird to keep the proc macro API i.e, to share the same definition with different error types for the client and server. It's still possible to do it in the proc macro code as you have changed it this PR.
Example both client and server
Example only client
I mostly care about having the client and server to share the API definition which I like to avoid a bunch of boiler plate and so on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking about it more like this:
And the client would have a
ClientErrororClientError<CustomError>type.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that idea is interesting but it complicates things in other crates such as
corebut I would prefer to keep it out of this PR.That alone would cause plenty of changes
The benefit of splitting it up is that is easier to reason about the error scenarios but forces users to import a few different error types (fine though)