Add propagate_extensions middleware#425
Conversation
jlizen
left a comment
There was a problem hiding this comment.
@bassmanitram - Big ++ for this being an important feature, thanks for cutting this PR. Sorry it took a while to get to. I'm on board with the general approach (minus the removal semantics) though have some feedback about specifics.
Let me know if you no longer have time to work on this, no problem, I'm glad to follow up with a revision on top.
| CompressionBody::new(BodyInner::zstd(WrapBody::new(body, self.quality))) | ||
| } | ||
| #[cfg(feature = "fs")] | ||
| #[allow(unreachable_patterns)] |
There was a problem hiding this comment.
Let's pull this out of this PR, no real relationship. Happy to review a separate one if still needed.
| use tower_layer::Layer; | ||
| use tower_service::Service; | ||
|
|
||
| #[allow(unused_imports)] |
There was a problem hiding this comment.
Let's only include the one we need rather than allowing unused (looks like only debug!)?
Let's also gate tracing imports/usage behind the trace feature and mention that you can enable it to get the event emissions.
| } | ||
|
|
||
| fn call(&mut self, mut req: Request<ReqBody>) -> Self::Future { | ||
| let extension: Option<X> = req.extensions_mut().remove(); |
There was a problem hiding this comment.
Let's clone it rather than remove it, and document it as such. (If you think it is useful to have an optional flag to remove instead of clone, I'm fine with it, but I think it is the wrong default). That will bring us in line with propagate_headers and be less footgun-prone.
Extensions anyway require Clone so it's not a meaningful change in bounds, though it would take tweaking some bounds specified in this change.
| } | ||
| } | ||
|
|
||
| #[cfg(test)] |
There was a problem hiding this comment.
Also need a test for the case where the targeted extension is missing
| /// and this middleware will ensure that it is available to the post service mappers via the Response extensions. | ||
| /// | ||
| /// See the [module docs](crate::propagate_extension) for more details. | ||
| #[derive(Clone, Debug)] |
There was a problem hiding this comment.
We should derive Default as well
| //! use tower_http::add_extension::AddExtensionLayer; | ||
| //! use tower_http::propagate_extension::PropagateExtensionLayer; | ||
| //! use tower_http::ServiceBuilderExt; | ||
| //! use hyper::Body; |
There was a problem hiding this comment.
We should move to http-body-util and Full<Bytes>, the rest of the codebase has shifted since you opened this.
| use tower::{Service, ServiceExt, ServiceBuilder}; | ||
| use crate::add_extension::AddExtensionLayer; | ||
| use crate::builder::ServiceBuilderExt; | ||
| use hyper::Body; |
There was a problem hiding this comment.
We should move to http-body-util and Full<Bytes>, the rest of the codebase has shifted since you opened this.
| //! # } | ||
| //! ``` | ||
|
|
||
| use futures_util::ready; |
There was a problem hiding this comment.
We have std::task::ready available under our MSRV, let's use that.
| let request = Request::builder().body(Body::empty()).expect("Expected an empty body"); | ||
|
|
||
| // Call the service. | ||
| let ready = futures::executor::block_on(svc.ready()).expect("Expected the service to be ready"); |
There was a problem hiding this comment.
let's use #[tokio::test] in the tests instead of futures::executor::block_on for consistency with the rest of the codebase
|
Absolutely onboard - my company has me working on COBOL -> cloud
replatforming - I thought I'd gotten rid of COBOL 30 years ago!!!! "Just
when I thought I was out ..." etc!
So, yeah, I'll refresh my memory on the PR and your suggestions and
actually do something _other_ than telling Claude "This a COBOL ecosystem.
Design a way to run it on <chosen cloud provider> that allows the customer
to think they have something new and fresh and wizz-bang cloud native,
while keeping things semantically EXACTLY the same for their tunnelvisioned
developers (and, despite their inisitence on requiring direct adhoc access
to production, make the damn thing airtight security-wise)"
…On Fri, May 22, 2026 at 9:59 AM Jess Izen ***@***.***> wrote:
***@***.**** requested changes on this pull request.
@bassmanitram <https://github.com/bassmanitram> - Big ++ for this being
an important feature, thanks for cutting this PR. Sorry it took a while to
get to. I'm on board with the general approach (minus the removal
semantics) though have some feedback about specifics.
Let me know if you no longer have time to work on this, no problem, I'm
glad to follow up with a revision on top.
------------------------------
In tower-http/src/compression/future.rs
<#425 (comment)>:
> @@ -73,6 +73,7 @@ where
CompressionBody::new(BodyInner::zstd(WrapBody::new(body, self.quality)))
}
#[cfg(feature = "fs")]
+ #[allow(unreachable_patterns)]
Let's pull this out of this PR, no real relationship. Happy to review a
separate one if still needed.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> +//! # }
+//! ```
+
+use futures_util::ready;
+use http::{Request, Response};
+use pin_project_lite::pin_project;
+use std::future::Future;
+use std::{
+ pin::Pin,
+ task::{Context, Poll},
+ marker::PhantomData,
+};
+use tower_layer::Layer;
+use tower_service::Service;
+
+#[allow(unused_imports)]
Let's only include the one we need rather than allowing unused (looks like
only debug!)?
Let's also gate tracing imports/usage behind the trace feature and
mention that you can enable it to get the event emissions.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> +impl<ReqBody, ResBody, S, X> Service<Request<ReqBody>> for PropagateExtension<S,X>
+where
+ X: Sync + Send + 'static,
+ S: Service<Request<ReqBody>, Response = Response<ResBody>>,
+{
+ type Response = S::Response;
+ type Error = S::Error;
+ type Future = ResponseFuture<S::Future,X>;
+
+ #[inline]
+ fn poll_ready(&mut self, cx: &mut Context<'_>) -> Poll<Result<(), Self::Error>> {
+ self.inner.poll_ready(cx)
+ }
+
+ fn call(&mut self, mut req: Request<ReqBody>) -> Self::Future {
+ let extension: Option<X> = req.extensions_mut().remove();
Let's clone it rather than remove it, and document it as such. (If you
think it is useful to have an optional flag to remove instead of clone, I'm
fine with it, but I think it is the wrong default). That will bring us in
line with propagate_headers and be less footgun-prone.
Extensions anyway require Clone so it's not a meaningful change in
bounds, though it would take tweaking some bounds specified in this change.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> + fn poll(self: Pin<&mut Self>, cx: &mut Context<'_>) -> Poll<Self::Output> {
+ let this = self.project();
+ let mut res = ready!(this.future.poll(cx)?);
+
+ if let Some(extension) = this.extension.take() {
+ debug!("Inserting state into response extensions");
+ res.extensions_mut().insert(extension);
+ } else {
+ debug!("No state to insert into response");
+ }
+
+ Poll::Ready(Ok(res))
+ }
+}
+
+#[cfg(test)]
Also need a test for the case where the targeted extension is missing
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> + debug,
+ info,
+ warn,
+ error,
+};
+
+/// Layer that applies [`PropagateExtension`] which propagates an extension from the request to the response.
+///
+/// This middleware is intended to wrap a Request->Response service handler that is _unaware_ of the
+/// extension. Consequently it _removes_ the extension from the request before forwarding the request, and then
+/// inserts it into the response when the response is ready. As a usage example, if you have pre-service mappers
+/// that need to share state with post-service mappers, you can store the state in the Request extensions,
+/// and this middleware will ensure that it is available to the post service mappers via the Response extensions.
+///
+/// See the [module docs](crate::propagate_extension) for more details.
+#[derive(Clone, Debug)]
We should derive Default as well
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> +//! This middleware is intended to wrap a Request->Response service handler that is _unaware_ of the
+//! extension. Consequently it _removes_ the extension from the request before forwarding the request, and then
+//! inserts it into the response when the response is ready. As a usage example, if you have pre-service mappers
+//! that need to share state with post-service mappers, you can store the state in the Request extensions,
+//! and this middleware will ensure that it is available to the post service mappers via the Response extensions.
+//!
+//! # Example
+//!
+//! ```rust
+//! use http::{Request, Response};
+//! use std::convert::Infallible;
+//! use tower::{Service, ServiceExt, ServiceBuilder, service_fn};
+//! use tower_http::add_extension::AddExtensionLayer;
+//! use tower_http::propagate_extension::PropagateExtensionLayer;
+//! use tower_http::ServiceBuilderExt;
+//! use hyper::Body;
We should move to http-body-util and Full<Bytes>, the rest of the
codebase has shifted since you opened this.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> + }
+
+ Poll::Ready(Ok(res))
+ }
+}
+
+#[cfg(test)]
+mod tests {
+ use super::*;
+
+ use http::{Request, Response};
+ use std::convert::Infallible;
+ use tower::{Service, ServiceExt, ServiceBuilder};
+ use crate::add_extension::AddExtensionLayer;
+ use crate::builder::ServiceBuilderExt;
+ use hyper::Body;
We should move to http-body-util and Full<Bytes>, the rest of the
codebase has shifted since you opened this.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> +//! .propagate_extension::<MyState>()
+//! .service_fn(handle);
+//!
+//! // Call the service.
+//! let request = Request::builder()
+//! .body(Body::empty())?;
+//!
+//! let response = svc.ready().await?.call(request).await?;
+//!
+//! assert_eq!(response.extensions().get::<MyState>().unwrap().state_message, "propagated state");
+//! #
+//! # Ok(())
+//! # }
+//! ```
+
+use futures_util::ready;
We have std::task::ready available under our MSRV, let's use that.
------------------------------
In tower-http/src/propagate_extension.rs
<#425 (comment)>:
> + }
+
+ #[test]
+ fn basic_test() {
+
+ let my_state = MyState { state_message: "propagated state".to_string() };
+
+ let mut svc = ServiceBuilder::new()
+ .layer(AddExtensionLayer::new(my_state)) // any other way of adding the extension to the request is OK too
+ .layer(PropagateExtensionLayer::<MyState>::new())
+ .service_fn(handle);
+
+ let request = Request::builder().body(Body::empty()).expect("Expected an empty body");
+
+ // Call the service.
+ let ready = futures::executor::block_on(svc.ready()).expect("Expected the service to be ready");
let's use #[tokio::test] in the tests instead of
futures::executor::block_on for consistency with the rest of the codebase
—
Reply to this email directly, view it on GitHub
<#425 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADAKLWR2SEESRT47OEGRNUT44ACGTAVCNFSM6AAAAACZI33E7CVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHM2DGNBTGYYDEMJWGQ>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
(See issue #423 )
This middleware is intended to be used as a way of sharing state between "pre service" layers/filters etc and those that run "post service", particularly when the service itself is unaware of that state.
This is in fact, almost a verbatim copy of propagate_header, replacing the Header semantics with those required by http::Extensions.
For ease of implementation it removes the state from the request before the inner service is invoked, and inserts the state into the response when the response is ready.
It does not concern itself with the initial insert of the state into the request since that is covered by an existing middleware, and may also be achieved by other means in one of the "pre service" components.
Because Extensions are managed by type, both the layer and the underlying service have to be generic over the type of the user's state object - and that type has to be cloneable as well as Send and Sync. In order to keep the compiler happy, both the service and the layer have a phantom data field. I am not 100% sure that this is necessary, but it does seem to be one of the few safe code instances where it probably is.