-
Notifications
You must be signed in to change notification settings - Fork 170
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(server): JSON-RPC specific middleware #1215
Changes from 29 commits
a2858ff
0bdbfd9
679543c
1c2a8a6
264944c
ed2ea57
461fed9
013f045
a8eb284
5c003a0
7432768
cb627ae
0fc11a3
5b94aa8
6e9e308
1ac49a1
d1dbce3
2b10662
be0103d
4376602
9462220
c182c1f
cad5ffc
84c2787
938ff4f
1f47102
6455325
a5251df
1c0eb53
5bd5cc5
c207d61
c1122b7
abb104b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,32 @@ pub enum MethodCallback { | |
Unsubscription(UnsubscriptionMethod), | ||
} | ||
|
||
/// The kind of the JSON-RPC method call, it can be a subscription, method call or unknown. | ||
#[derive(Debug, Copy, Clone)] | ||
pub enum MethodKind { | ||
/// Subscription Call. | ||
Subscription, | ||
/// Unsubscription Call. | ||
Unsubscription, | ||
/// Method call. | ||
MethodCall, | ||
/// Unknown method. | ||
Unknown, | ||
} | ||
|
||
impl std::fmt::Display for MethodKind { | ||
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
let s = match self { | ||
Self::Subscription => "subscription", | ||
Self::MethodCall => "method call", | ||
Self::Unknown => "unknown", | ||
Self::Unsubscription => "unsubscription", | ||
}; | ||
|
||
write!(f, "{s}") | ||
} | ||
} | ||
|
||
/// Result of a method, either direct value or a future of one. | ||
pub enum MethodResult<T> { | ||
/// Result by value | ||
|
@@ -219,6 +245,16 @@ impl Methods { | |
self.callbacks.get_key_value(method_name).map(|(k, v)| (*k, v)) | ||
} | ||
|
||
/// Returns the kind of the method callback, | ||
pub fn method_kind(&self, method_name: &str) -> MethodKind { | ||
match self.method(method_name) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah, this code is not used so I removed it but I guess your comment still applies. Lemme fix it |
||
None => MethodKind::Unknown, | ||
Some(MethodCallback::Async(_)) | Some(MethodCallback::Sync(_)) => MethodKind::MethodCall, | ||
Some(MethodCallback::Subscription(_)) => MethodKind::Subscription, | ||
Some(MethodCallback::Unsubscription(_)) => MethodKind::Unsubscription, | ||
} | ||
} | ||
|
||
/// Helper to call a method on the `RPC module` without having to spin up a server. | ||
/// | ||
/// The params must be serializable as JSON array, see [`ToRpcParams`] for further documentation. | ||
|
@@ -307,7 +343,7 @@ impl Methods { | |
) -> RawRpcResponse { | ||
let (tx, mut rx) = mpsc::channel(buf_size); | ||
let id = req.id.clone(); | ||
let params = Params::new(req.params.map(|params| params.get())); | ||
let params = Params::new(req.params.as_ref().map(|params| params.as_ref().get())); | ||
|
||
let response = match self.method(&req.method) { | ||
None => MethodResponse::error(req.id, ErrorObject::from(ErrorCode::MethodNotFound)), | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -258,7 +258,7 @@ impl PendingSubscriptionSink { | |
/// the return value is simply ignored because no further notification are propagated | ||
/// once reject has been called. | ||
pub async fn reject(self, err: impl Into<ErrorObjectOwned>) { | ||
let err = MethodResponse::error(self.id, err.into()); | ||
let err = MethodResponse::subscription_error(self.id, err.into()); | ||
_ = self.inner.send(err.result.clone()).await; | ||
_ = self.subscribe.send(err); | ||
} | ||
|
@@ -269,7 +269,7 @@ impl PendingSubscriptionSink { | |
/// | ||
/// Panics if the subscription response exceeded the `max_response_size`. | ||
pub async fn accept(self) -> Result<SubscriptionSink, PendingSubscriptionAcceptError> { | ||
let response = MethodResponse::response( | ||
let response = MethodResponse::subscription_response( | ||
self.id, | ||
ResponsePayload::result_borrowed(&self.uniq_sub.sub_id), | ||
self.inner.max_response_size() as usize, | ||
|
@@ -438,6 +438,9 @@ impl Subscription { | |
let raw = self.rx.recv().await?; | ||
|
||
tracing::debug!("[Subscription::next]: rx {}", raw); | ||
|
||
// clippy complains about this but it doesn't compile without the extra res binding. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ooh weird; what's the error?! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. lifetime doesn't live long enough, it may be some issue with my own serde impl I did a long time ago. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange though that nothing changes at all whether the variable is there or not! I wouldn't expect any difference since you haven't given any extra type info on the variable or whatever :) |
||
#[allow(clippy::let_and_return)] | ||
let res = match serde_json::from_str::<SubscriptionResponse<T>>(&raw) { | ||
Ok(r) => Some(Ok((r.params.result, r.params.subscription.into_owned()))), | ||
Err(e) => match serde_json::from_str::<SubscriptionError<serde_json::Value>>(&raw) { | ||
|
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.
Nit: I wonder whether it's worth renaming this to
method_response
to try and make it feel like "not the default one to use", and/or maybe just a tweak to docs like "If the response is from a subscription, usesubscription_response
", orr ditchingsubscription_response
and instead adding another arg toresponse
(ieis_subscription: bool
) so that every call has to be explicit.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, it makes sense.
I just added
subscription_response
and never thought about it. I was just happy that it worked :PThere 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 not too fussed, having looked at the rest of the code (maybe just add the doc comment?), so whatever you prefer :)
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.
yeah, a subscription response is basically just a method response but in some scenarios in the server it may be useful to know and in middleware doing grafana metrics etc.
I think I want to keep response but lemme adjust the docs a little bit better.