Skip to content
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

feat: customizable JSON-RPC error codes via new enum variant on CallErrror #394

Merged
merged 6 commits into from
Jun 29, 2021

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Jun 25, 2021

The motivation behind this change is that we need to provide functionality for users to configure their own error messages; that means error code, error message and data, https://www.jsonrpc.org/specification#error-object

Superseeds #393

Closes #299

This commit introduces a new trait for defining user customizable error codes and messages
// and need to be included in the error.
fn message(&self) -> String;
/// A Primitive or Structured value that contains additional information about the error.
fn data(&self) -> Option<&serde_json::value::RawValue>;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be annoying serde_json::RawValue here but at least we rexport it from types

@dvdplm
Copy link
Contributor

dvdplm commented Jun 28, 2021

cc @maciejhirsz Thoughts on this?
For context: it seems like substrate only adheres to the spec (which mandates a full "Error Object") some of the time? Possibly because it's verbose and annoying.

@niklasad1
Copy link
Member Author

cc @maciejhirsz Thoughts on this?
For context: it seems like substrate only adheres to the spec (which mandates a full "Error Object") some of the time? Possibly because it's verbose and annoying.

example of this can be found here for context but I think these error codes is still quite useful in the context of substrate so we should really support it.

But I was thinking of two variants of register_method:

  1. like old one, just require StdError + Send + Sync and just let jsonrpsee assign the error code
  2. register_method_with_custom_error, require the error to impl RpcErrorTrait

@niklasad1 niklasad1 marked this pull request as ready for review June 28, 2021 11:36
Copy link
Contributor

@maciejhirsz maciejhirsz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I'm wondering if trait is the right approach. I think it might be better to have an enum like:

enum JsonRpcError {
    User(Box<dyn std::error::Error>),
    ParseError,
    InvalidError,
    // ...
}

With a From<Box<dyn std::error::Error>> impl, so we can auto cast all user errors into the User variant and give it some default error code, while also having the option to set codes for things like params parsing fail etc.

http-server/src/tests.rs Outdated Show resolved Hide resolved
@niklasad1
Copy link
Member Author

With a From<Box<dyn std::error::Error>> impl, so we can auto cast all user errors into the User variant and give it some default error code, while also having the option to set codes for things like params parsing fail etc.

Right, that would be nice but the custom error messages won't work but if we could do:

enum JsonRpcError {
    // provide default error code
    User(Box<dyn std::error::Error>),
    // user provides error code and similar
    CustomUser { code: i32, message: String, data: JsonValue },
    // some specific error code
    ParseError,
    // some specific error code
    InvalidError,
    // ...
}

that would be quite flexible and better, what do you think @maciejhirsz? :)

@maciejhirsz
Copy link
Contributor

Ye, that works I think.

@@ -19,12 +20,22 @@ impl<T: fmt::Display> fmt::Display for Mismatch<T> {
/// Error that occurs when a call failed.
#[derive(Debug, thiserror::Error)]
pub enum CallError {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to put non_exhaustive on this but a little weird to just add some error code for non defined enum variant :P

/// Short description of the error.
message: String,
/// A primitive or structured value that contains additional information about the error.
data: Option<Box<RawValue>>,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this might save allocations of the value is already a String

@niklasad1 niklasad1 changed the title feat: customizable error via RpcError trait feat: customizable JSON-RPC error codes via new enum variant on CallErrror Jun 29, 2021
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should work.

@niklasad1 niklasad1 merged commit 8b65edf into master Jun 29, 2021
@niklasad1 niklasad1 deleted the na-jsonrpsee-error-trait branch June 29, 2021 13:28
dvdplm added a commit that referenced this pull request Jul 6, 2021
* master: (21 commits)
  New proc macro (#387)
  Streaming RpcParams parsing (#401)
  Set allowed Host header values (#399)
  Synchronization-less async connections in ws-server (#388)
  [ws server]: terminate already established connection(s) when the server is stopped (#396)
  feat: customizable JSON-RPC error codes via new enum variant on `CallErrror` (#394)
  [ci]: test each individual crate's manifest (#392)
  Add a way to stop servers (#386)
  [jsonrpsee types]: unify a couple of types + more tests (#389)
  Update roadmap link in readme (#390)
  Cross-origin protection (#375)
  Method aliases + RpcModule: Clone (#383)
  Use criterion's async bencher (#385)
  Async/subscription benches (#372)
  send text (#374)
  Fix link to ws server in README.md (#373)
  Concat -> simple push (#370)
  Add missing `rt` feature (#369)
  Release prep for v0.2 (#368)
  chore(scripts): publish script (#354)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[server]: error types when register methods
3 participants