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: add Extensions to Request/MethodResponse #1306

Merged
merged 9 commits into from
May 28, 2024
Merged

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented Feb 29, 2024

Fixes #1305

In summary this PR adds http::Extensions to Request and Responses this may be used to share state between middlewares and similar.

Most of the changes are chore but I had to add this to all RPC handlers and the proc macro API.
Thus, the most important things to review is the changes to Request and MethodResponse.

@niklasad1 niklasad1 requested a review from a team as a code owner February 29, 2024 15:57
@niklasad1 niklasad1 added the breaking change Breaking change in the public APIs label Feb 29, 2024
types/src/request.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Once we merge this, I think we could replace the connection_id we expose to raw methods to Params::Extensions. And then guarantee by our middleware that we add a unique type for connection_id.

And we'll have a way to easily pass data not only between middlewares, but also middleware -> methods

But that could happen later :D

@niklasad1 niklasad1 removed the breaking change Breaking change in the public APIs label May 21, 2024
@niklasad1 niklasad1 requested a review from lexnv May 22, 2024 13:30
@jsdw
Copy link
Collaborator

jsdw commented May 24, 2024

Do we have an example of ths Extensions stuff being used? Would be great to see it in practice and offhand I can't see one :)


let mut module = RpcModule::new(());
module.register_method("say_hello", |_, _, _| "hello").unwrap();
module.register_method("raw_method", |_, _, ext| *ext.get::<u32>().unwrap()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I wonder whether we should rename this to eg "get_connection_id" to make it clearer what it's doing/testing?


let server_handle = server.start(module);
let methods2 = methods.clone();
tokio::spawn(async move {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I geuss this is all added so that we can inject connection details!

I wonder whether there's something wecan do in the future to simplify this "low level" initialising

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could probably inject the connection id by default but it's quite weird to explain/document it so I didn't do it.

What does @lexnv reckon? :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, substrate could then inject the connection ID in a similar way

Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great! IMO it would benefit from an example showing extensions being used to eg share details between middlewares to the request method or whatever :)

Getting details in I guess requires the "low level" interface as in the test stuff, and I wonder if we can simplify injecting data on that side of things!

@niklasad1
Copy link
Member Author

Do we have an example of ths Extensions stuff being used? Would be great to see it in practice and offhand I can't see one :)

Yes, https://github.com/paritytech/jsonrpsee/pull/1306/files#diff-aa58ad08343facdd5487fab413ac599701293922cbcd160d00c7022a7577fc4cR50-R60

@jsdw
Copy link
Collaborator

jsdw commented May 24, 2024

Do we have an example of ths Extensions stuff being used? Would be great to see it in practice and offhand I can't see one :)

Yes, https://github.com/paritytech/jsonrpsee/pull/1306/files#diff-aa58ad08343facdd5487fab413ac599701293922cbcd160d00c7022a7577fc4cR50-R60

Ah awesome; I missed that!

Copy link
Contributor

@lexnv lexnv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@niklasad1 niklasad1 merged commit 23e2d72 into master May 28, 2024
11 checks passed
@niklasad1 niklasad1 deleted the na-fix-1305 branch May 28, 2024 12:38
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.

add type map http::Extensions to Request and MethodResponse types
3 participants