feat: set multiple request header#677
Conversation
… layer multiple header middleware Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
jlizen
changed the title
|
|
||
| /// Metadata describing a request header to be set by [`SetMultipleRequestHeadersLayer`] or [`SetMultipleRequestHeader`]. | ||
| #[derive(Clone, Debug)] | ||
| pub struct HeaderMetadata<T> { |
There was a problem hiding this comment.
We are going to the trouble of lifting BoxedMakeHeaderValue and friends into a shared module, can we not lift out HeaderMetadata and HeaderInsertionConfig as well?
They have effectively identical structures, usages, and doc comments.
It also reduces the surface area where we need to have parallel tests for everything (and also the number of places we can have copy/paste bugs ^^)
There was a problem hiding this comment.
The big win here would be to have just a single:
tower_http::set_header::HeaderMetadata
Rather than two different types with the same name, that behave identically.
I imagine the colliding name is part of why only response is re-exporting that type...
| @@ -0,0 +1,422 @@ | |||
| //! Set multiple headers on the request. | |||
There was a problem hiding this comment.
This is multiple_headers.rs but response is multiple_header.rs. I prefer multiple_headers. We can safely rename the response file name without breakage, it looks like?
There was a problem hiding this comment.
Yes, I realised late and wasn't sure if it was fine to fix in this PR
|
|
||
| #[doc(inline)] | ||
| pub use self::{ | ||
| request::{SetRequestHeader, SetRequestHeaderLayer}, |
There was a problem hiding this comment.
we should be re-exporting consistently across both (or ideally HeaderMetadata is unified anyway)
| headers: Vec<HeaderInsertionConfig<T>>, | ||
| } | ||
|
|
||
| impl<T> fmt::Debug for SetMultipleRequestHeadersLayer<T> { |
There was a problem hiding this comment.
We should implement Clone for this where T: Clone...
That lets you do, eg:
let layer = ...;
svc1.layer(layer.clone()); svc2.layer(layer.clone())
Refer to SetRequestHeaderLayer for an example.
I missed this in the response PR, we need it there too. I'd be fine to have it in this PR.
| let static_meta = (header::CONTENT_TYPE, HeaderValue::from_static("text/html")).into(); | ||
|
|
||
| // Wrap the closure | ||
| let closure_meta = (header::X_FRAME_OPTIONS, |_res: &Response<Body>| { |
There was a problem hiding this comment.
This is passing a response body closure into a layer that expects a request body. It only works because there is no actual usage of this service. Let's add a oneshot.await() to make sure we flush it out.
We already are doing this properly on the response side.
Signed-off-by: Aminu Oluwaseun Joshua <seun.aminujoshua@gmail.com>
2 participants
Motivation
Fixes #676
Adds native support for inserting multiple header middleware per layer
Solution
For better code organisation, the request.rs file, which handles the request, is now a module and split into two files. One for setting a single header and the other for multiple, while still maintaining the current API structure.
The functions handling setting headers take a dynamic variable that implements HeaderMetadataGenerator. The newly created HeaderMetadata and a tuple of (HeaderName, M), currently implemented in
M, can be HeaderValue (A test case covers this). This also helps with maintainability.It also includes tests
The PR also moved some reused code into the root module for easy access by both request and response headers