rpc module: refactor subscriptions to return impl IntoSubscriptionResponse
#1034
Conversation
8062295 to
3140865
Compare
Option<Result<>>Option<Result<>>
This changes the subscription API again to return `Result<(), Option<SubscriptionMessage>>` to work a little smoother with the combinators and it introduces an extention trait to make convert it `Result<T, Error>` to `Result<T, Option<SubscriptionMessage>>` This trait is implemented for types where it is possible and for custom types and tricky situations the user has to implement themselves. For instance it's not possible to implement on `TrySendError` because that depends on the use-case i.e. whether the channel is full should be regarded as an error or not.
Option<Result<>>Result<(), Option>
Result<(), Option>impl IntoSubscriptionResponse
Co-authored-by: James Wilson <james@jsdw.me>
| @@ -502,3 +495,36 @@ async fn bounded_subscription_works() { | |||
| assert_eq!(item, exp); | |||
| } | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
| async fn serialize_sub_error_adds_extra_string_quotes() { | |||
There was a problem hiding this comment.
This is a test to illustrate that StringError will be serialized with extra string quotes such as "error":"{...}" even if the value is JSON
We could actually parse the string before adding the "<string>" but I came to the solution that is too opinionated and likely better for folks to implement their own type then.
There was a problem hiding this comment.
Ok so basically fi the string returned is actually JSON already we'll always be assumign it's a string (I think that's fair; we are expecting "standard" errors to be returned here and not errors that are actually also valid JSON strings
There was a problem hiding this comment.
Yep, that does make sense! In the end, we will be wrapping the message (may it be a string or already a valid json) with "{ msg }". I'm guessing this is not the common behavior and we don't need to provide a sort of wrapped error to ensure we don't add the extra quotes.
I do wonder, do we have in substrate subscriptions that return directly a valid json for the error? I'd expect that to be unlikely
There was a problem hiding this comment.
I do wonder, do we have in substrate subscriptions that return directly a valid json for the error? I'd expect that to be unlikely
No, substrate will likely use Option<Msg> or a custom message type because all substrate subscriptions has defined states then it doesn't make sense with Result with this new pattern.
Thus, in practice Result should only by used when one doesn't have defined messages when something fails in the subscription callback such as parsing messages and some other custom logic to be indicated to the "connected client"
The goal with this PR is to simplify the subscription API from the server-side of things.
Previously the subscriptions were changed to return
Resultfor ergonomics but the errors were never used and only logged after the latest refactoring.One problem with the current API is that "result error bailing" is hard to get right and might cause the error not to sent the client.
This PR adds a new trait
IntoSubscriptionCloseResponsewhich can be implemented for customized types and it is implemented forResult<(), StringError> and ()by jsonrpseeResult
Once an error occurs it's sent out a subscription error notification
module .register_subscription("my_sub", "my_sub", "my_unsub", |_, pending, _| async move { let mut sink = pending.accept().await?; sink.send("foo").await?; Ok(()) }) .unwrap();The outcome is that the proc macro APIs can't assume that the return type will be
Result<(), Error>anymore instead it could be any type that implementIntoSubscriptionResponse