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

middleware::map_request[_with_state[_arc]] and map_response[_with_state[_arc]] #1394

Closed
jplatte opened this issue Sep 19, 2022 · 8 comments · Fixed by #1408
Closed

middleware::map_request[_with_state[_arc]] and map_response[_with_state[_arc]] #1394

jplatte opened this issue Sep 19, 2022 · 8 comments · Fixed by #1408
Assignees
Labels
A-axum C-feature-request Category: A feature request, i.e: not implemented / a PR. E-medium Call for participation: Experience needed to fix: Medium / intermediate

Comments

@jplatte
Copy link
Member

jplatte commented Sep 19, 2022

Feature Request

Motivation

axums from_fn[_with_state[_arc]] can be confusing to newcomers who haven't read about / fully understood tower's "onion principle" of layers yet. Its generality also makes for a little more boilerplate than necessary when one wants to write a middleware that only concerns itself with either the request or response.

Proposal

Add map_request[_with_state[_arc]] and map_response[_with_state[_arc]] to the middleware module. In the case of map_response* it is notable that async functions passed to these functions will never have to be generic.

Alternatives

Do nothing.

@davidpdrsn davidpdrsn added A-axum C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Sep 19, 2022
@davidpdrsn
Copy link
Member

I think that makes sense to support. I'm a bit worried about duplicating stuff that is in tower but I think these two are probably fine.

@davidpdrsn davidpdrsn changed the title middleware::map_request[_with_state[_arc]] and map_response[_with_state[_arc]] middleware::map_request[_with_state[_arc]] and map_response[_with_state[_arc]] Sep 19, 2022
@davidpdrsn davidpdrsn added E-help-wanted Call for participation: Help is requested to fix this issue. E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Sep 19, 2022
@jplatte
Copy link
Member Author

jplatte commented Sep 19, 2022

Well, ours are going to be more ergonomic by supporting extractors.

@davidpdrsn
Copy link
Member

Oh yeah that's right!

@jplatte
Copy link
Member Author

jplatte commented Sep 23, 2022

I've been discussing this a bit with @genusistimelord, one question that came up is what the function signature should look like, in particular for map_request*. For some use cases, you may want to return early inside the closure, so a Result<Request<B>, impl IntoResponse> return type could make sense. But then people who have an infallible closure have to write Ok::<_, Infallible>(req) because of the generic E type on the Result.

The easy way out of this is providing both map_request* and try_map_request*. For map_response, none of this is needed because that can just return any type that implements IntoResponse.

@davidpdrsn
Copy link
Member

@jplatte
Copy link
Member Author

jplatte commented Sep 23, 2022

Can you elaborate? That's pretty much just a different way of spelling the same thing (w/ some different rules for ?) and still leaves the question of whether there should be separate try_ variants or not, no?

@genusistimelord
Copy link
Contributor

genusistimelord commented Sep 23, 2022

I think a map_request* and try_map_request* make the most sense here as we can dictate that map_request never fails and try Can fail returning a Result<Request<B>, IntoResponse> or a ControlFlow<IntoResponse> and make req passable via &mut req?

@davidpdrsn
Copy link
Member

I was thinking something like #1408

@davidpdrsn davidpdrsn self-assigned this Sep 25, 2022
@davidpdrsn davidpdrsn removed the E-help-wanted Call for participation: Help is requested to fix this issue. label Sep 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-axum C-feature-request Category: A feature request, i.e: not implemented / a PR. E-medium Call for participation: Experience needed to fix: Medium / intermediate
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants