-
Notifications
You must be signed in to change notification settings - Fork 185
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: client rpc middleware #1521
Conversation
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.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
Overall I'm good with this change. The pros are basically allowing middlewares to be shared between client and server, but need to make sure that we don't have to mangle one side or the other to fit this trait into both places. As we said, possible making the response type generic as well as the error type would make that easier, but you'd know best :) |
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.
Pull Request Overview
This PR unifies the RPC service API to be used by both clients and servers, introducing a common trait (RpcServiceT) that supports method calls, notifications, and batch requests along with middleware support. Key changes include:
- Implementing a unified RpcServiceT trait with batch and notification methods.
- Extending the HttpClientBuilder with generic middleware parameters and integrating RPC middleware.
- Refactoring middleware layers and associated components (e.g., logger, either) to support the new API.
Reviewed Changes
Copilot reviewed 44 out of 44 changed files in this pull request and generated no comments.
Show a summary per file
File | Description |
---|---|
client/http-client/src/rpc_service.rs | Introduces RpcService implementation for bridging client requests. |
core/src/middleware/mod.rs | Adds middleware types for batch requests and notifications. |
client/http-client/src/client.rs | Updates HttpClientBuilder with additional middleware and RPC layers. |
core/src/server/method_response.rs | Refactors method response handling and logging improvements. |
client/http-client/src/transport.rs | Integrates request timeout into transport calls. |
examples/examples/http.rs | Provides example usage with a custom Logger middleware. |
examples/examples/jsonrpsee_as_service.rs | Updates the AuthorizationMiddleware with stricter type bounds. |
... | Additional dependency and module refinements across the codebase. |
Comments suppressed due to low confidence (1)
client/http-client/src/client.rs:359
- The current implementation of request_timeout uses todo!(), which will panic if called at runtime. Replace this with a proper implementation that returns the configured request timeout.
pub fn request_timeout(&self) -> Duration { todo!(); }
{ | ||
type Future = ResponseFuture<S::Future>; | ||
type Error = S::Error; | ||
type Response = S::Response; |
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.
This could be a followup thing to try out but: I wonder whether it's worth now having a separate response type for each call, so that there is no enum?
eg
``rust
type CallResponse = ...
type BatchResponse = ...
type NotificationResponse = ...
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.
(though maybe this prevents the middleware from doing certain things, ie returning a MethodResponse::Error in response to a call or whatever?)
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.
Let's file an issue for this, it's not trivial to break up the method response into different types in the server
&self, | ||
n: Notification<'a>, | ||
) -> impl Future<Output = Result<Self::Response, Self::Error>> + Send + 'a { | ||
self.inner.notification(n) |
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: we have logic to filter the notification in the batch case; can we do the same here or are we forced to send it 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.
Ok(MethodResponse::notification())
?
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.
it's doesn't matter that much but better the call the inner service if extensions are provided in the notification..
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.
Nice, LGTM! left a few comments/nits that are worth looking at before merging but nothing too major :)
where | ||
for<'a> S: RpcServiceT<'a> + Send, | ||
S: RpcServiceT<Response = MethodResponse, Error = Infallible> + Send, |
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 guess the point of Error = Infallible
here is that any "error" should be returned in the form of a MethodResponse to a client, and so it enforces that all errors are converted into proper JSON-RPC errors or whatever before they are emitted. Am I understanding that right?
(A bit like how in HTTP servers like Axum, any errors need to be converted to HTTP responses)
I geuss the reason to keep the Error
associated type at all is just for internal middleware that might want to signal something went wrong, and then some other middleware that wraps it can take that error and decide what sort of response to turn it into? I think this makes sense, but I wonder if we have any examples that show this is useful? (Or maybe I am missing something more obvious!)
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.
Copilot reviewed 64 out of 64 changed files in this pull request and generated 1 comment.
Close #1503
This PR unifies the RPC service API to be used by both clients and servers, introducing a common trait (RpcServiceT) that supports method calls, notifications, and batch requests along with middleware support. Key changes include:
Old trait:
Proposed change:
The reason is that the underlying client service will serialize the request which won't support Batches unless we add with a dedicate API i.e, we would need to send them one-by-one.