-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
axum: allow body types other than axum::body::Body
in Service
s passed to serve
#3205
base: main
Are you sure you want to change the base?
Conversation
axum/src/serve/mod.rs
Outdated
|
||
let app = ServiceBuilder::new() | ||
.layer_fn(|_| tower::service_fn(|_| std::future::ready(Ok(Response::new(CustomBody))))) | ||
.service(Router::<()>::new()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we add some routes, which type will be the body? The axum body type? Or CustomBody
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CustomBody
. The layer returns a service which always returns that without even using the Router
in this test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we add an handle to check if this works? Or are our handlers tight to our body implementation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that (in a few days).
The handler (and Router
) would still return axum::body::Body
, it's just the outer layer which changes that to something else.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added a route there.
Note though that the whole Router
is completely ignored by the outer layer. I could even write this instead and it would work the same way even without the router.
let app = tower::service_fn(|_| std::future::ready(Ok(Response::new(CustomBody))));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. What would be a better example?
A middleware changing the body type depending of the request?
(I have issues understanding the value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#3202 is good example I think. Doing stuff like compression. I just wanted to add a test that stuff like this compiles going forward. I don't think we can use a layer like that here though because we don't want to depend on these layers from here.
We can add a real example or extend the one showing middlewares wrapping the whole router (assuming we already have one).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's this example I wrote: https://github.com/tokio-rs/axum/blob/main/examples/compression/src/main.rs
(but I guess there's no need to layer outside the router for this)
I'll approve this PR code-wise. Feel free to enhance it with examples / tests showing its value as I cannot find any myself.
This is not reverting anything from #1751. That was about the router being generic over the body. |
a5e3bc8
to
cb1b0a0
Compare
Can you add a changelog entry? |
cb1b0a0
to
2aef075
Compare
I've added the changelog entry. Just to make sure, adding a generic parameter like this isn't considered breaking? I don't think this should break anyone calling I assume we're not merging breaking changes yet. |
Motivation
A user in #3202 tried to use middlewares around the whole
Router
so that their layers run before routing happens. They ran into problems caused by their layers changing theBody
type.So far I've seen only the
NormalizePathLayer
used in this way which does not change theBody
type so there were never issues. But I don't see a reason not to support this for other layers that do change the body, such asCompressionLayer
.Solution
This PR adds another generic parameter to
Serve
to keep track of theResponse::Body
type returned by the service provided.There are no breaking changes as far as I can tell as inference knows the body type. This could break someone who for some reason names the type parameters explicitly. There is no rush with this and we may want to wait with this and release it in the next breaking release.
Alternatives
We may also choose not to support this. Users can just map the response body with
.map_response_body(axum::body::Body::new)
before callignserve
. The price is just one box (and maybe a bit of ergonomics) as far as I can tell.