-
Notifications
You must be signed in to change notification settings - Fork 176
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: expose ConnectionGuard as request extension #1443
Conversation
Thanks Can you just create a simple test for this? Similar to https://github.com/paritytech/jsonrpsee/blob/master/tests/tests/integration_tests.rs#L210-#L225 but you need to add an new handler that actually fetches the ConnGuard from the extension. |
request.extensions_mut().insert::<ConnectionId>(conn.conn_id.into()); | ||
let req_ext = request.extensions_mut(); | ||
req_ext.insert::<ConnectionGuard>(conn_guard.clone()); | ||
req_ext.insert::<ConnectionId>(conn.conn_id.into()); |
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.
Cool, we could probably merge the ConnectionID and ConnectionGuard as some point to another type e.g. ConnectionDetails or something
@@ -225,6 +225,29 @@ async fn extensions_with_different_ws_clients() { | |||
assert_ne!(connection_id, second_connection_id); | |||
} | |||
|
|||
#[tokio::test] | |||
async fn connection_guard_extension_with_different_ws_clients() { |
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 sufficient with one client but this does hurt, thanks :)
@niklasad1 I tried to put the new test together with the one you pointed, but I think it would become too complex, so I created a new test and renamed the other one to make it clear which extension each one is testing. |
Yeah, you made the right call by creating another test. All good, just run |
that's nice the PR is already created <3 @niklasad1 could you suggest another reviewer with write access for this PR? |
Fixes #1438
Since ConnectionGuard keeps the internal information inside an Arc, it should be cheap to clone it for each request.