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(tonic): add Request and Response extensions #642

Merged
merged 2 commits into from
May 13, 2021
Merged

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented May 13, 2021

Adds tonic::Extensions which is a newtype around http::Extensions.

Request extensions can be set by interceptors with
Request::extensions_mut and retrieved from RPCs with
Request::extensions. Extensions can also be set in tower middleware
and will be carried through to the RPC.

Since response extensions cannot be set by interceptors the main use
case is to set them in RPCs and retrieve them in tower middlewares.
Figured that might be useful.

Fixes #255

@davidpdrsn davidpdrsn added C-enhancement Category: New feature or request A-tonic labels May 13, 2021
@davidpdrsn davidpdrsn added this to the 0.5 milestone May 13, 2021
@davidpdrsn davidpdrsn changed the title feat(tonic): add Request extensions feat(tonic): add Request and Response extensions May 13, 2021
Adds `tonic::Extensions` which is a newtype around `http::Extensions`.

Request extensions can be set by interceptors with
`Request::extensions_mut` and retrieved from RPCs with
`Request::extensions`. Extensions can also be set in tower middleware
and will be carried through to the RPC.

Since response extensions cannot be set by interceptors the main use
case is to set them in RPCs and retrieve them in tower middlewares.
Figured that might be useful.

Fixes #255

match client.unary_call(Input {}).await {
Ok(_) => {}
Err(status) => panic!("{}", status.message()),
Copy link
Member

Choose a reason for hiding this comment

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

wouldn't an unwrap here work?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah true. I copied some other test that didn't have it. Will fix it.

inner: S,
}

impl<S> Service<HyperRequest<Body>> for InterceptedService<S>
Copy link
Member

Choose a reason for hiding this comment

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

Its crazy how complex this is :(

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah I really wanna try and find a way to make this easier. Having to implement NamedService also complicates things a bit because you cannot take a middleware from tower and use it with tonics router.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, though I don't have a proper solution either :(

///
/// [`Interceptor`]: crate::Interceptor
/// [`Request`]: crate::Request
pub struct Extensions(http::Extensions);
Copy link
Member

Choose a reason for hiding this comment

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

nit (not blocking): Should we make this a proper struct with an inner? Idk what style I like better honestly but I tend to do that over a tuple struct 🤷 up to you doesn't matter too much.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't really have a preference. I'll change it.

@@ -62,6 +62,7 @@
//! [`transport`]: transport/index.html

#![recursion_limit = "256"]
#![allow(clippy::inconsistent_struct_constructor)]
Copy link
Member

Choose a reason for hiding this comment

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

the heck is this lint lmao

Copy link
Member Author

Choose a reason for hiding this comment

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

The most pedantic lint ever https://rust-lang.github.io/rust-clippy/master/#inconsistent_struct_constructor. rust-analyzer was complaining about it.

Copy link
Member

Choose a reason for hiding this comment

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

lmaooooo

@davidpdrsn davidpdrsn merged commit 352b0f5 into master May 13, 2021
@davidpdrsn davidpdrsn deleted the david/extensions branch May 13, 2021 13:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tonic C-enhancement Category: New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Arbitrary "Context" access for store metadata
2 participants