-
Notifications
You must be signed in to change notification settings - Fork 173
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
Custom errors #977
Merged
Merged
Custom errors #977
Changes from 8 commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
0b23192
Implement support for custom errors
MOZGIII 8a6091d
Remove unneded for<'a> from E bound
MOZGIII fd642eb
Fix doctest
MOZGIII 1b368fd
Handle the case where there are not exactly two arguments
MOZGIII ea833bc
Support for other Result paths
MOZGIII 648438b
Rewrite with a more explicit rewriting logic
MOZGIII fa3d48c
Back to rewriting the error argument
MOZGIII 0b334a5
Add UI error for non-result
MOZGIII 03cb912
Apply suggestions from code review
MOZGIII 6faf51c
Fix a typo
MOZGIII b7f24c0
Fix errors in the rest of the targets
MOZGIII File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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}"#); | ||
} |
9 changes: 9 additions & 0 deletions
9
proc-macros/tests/ui/incorrect/method/method_non_result_return_type.rs
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
use jsonrpsee::proc_macros::rpc; | ||
|
||
#[rpc(client)] | ||
pub trait NonResultReturnType { | ||
#[method(name = "a")] | ||
async fn a(&self) -> u16; | ||
} | ||
|
||
fn main() {} |
5 changes: 5 additions & 0 deletions
5
proc-macros/tests/ui/incorrect/method/method_non_result_return_type.stderr
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
error: Expecting something like 'Result<Foo, Err>' here, but got no generic args (eg no '<Foo,Err>'). | ||
--> tests/ui/incorrect/method/method_non_result_return_type.rs:6:23 | ||
| | ||
6 | async fn a(&self) -> u16; | ||
| ^^^ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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_method
would 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 Error
to 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
Result
back..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
ClientError
andServerError
as 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
ClientError
orClientError<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
core
but 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)