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: expose ConnectionGuard as request extension #1443

Merged
merged 3 commits into from
Aug 19, 2024

Conversation

dinhani
Copy link
Contributor

@dinhani dinhani commented Aug 16, 2024

Fixes #1438

Since ConnectionGuard keeps the internal information inside an Arc, it should be cheap to clone it for each request.

@dinhani dinhani requested a review from a team as a code owner August 16, 2024 15:37
@niklasad1
Copy link
Member

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());
Copy link
Member

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() {
Copy link
Member

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 :)

@dinhani
Copy link
Contributor Author

dinhani commented Aug 16, 2024

@niklasad1
I did not notice code was being tested via JSON-RPC interface.

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.

@niklasad1
Copy link
Member

niklasad1 commented Aug 16, 2024

I did not notice code was being tested via JSON-RPC interface.

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 $ cargo +stable fmt --all on your PR which will make the CI happy

@mayconamaro
Copy link

that's nice the PR is already created <3
thank you @dinhani!

@niklasad1 could you suggest another reviewer with write access for this PR?

@niklasad1 niklasad1 requested a review from a team August 16, 2024 19:38
@niklasad1 niklasad1 merged commit 9b66e9c into paritytech:master Aug 19, 2024
10 checks passed
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.

Expose current connections count as request extension
4 participants