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.

Sign up for GitHub

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

Jump to bottom

Only allow last extractor to mutate the request #1121

Closed
wants to merge 22 commits into from
Closed

Only allow last extractor to mutate the request #1121

wants to merge 22 commits into from

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Jun 26, 2022 •
edited
Loading

Don't worry too much about the large diff. The vast majority of the changes is from code that previously was a macro_rules macro now being expanded and committed.

Fixes #1115

This is an implementation of the design I suggested here.

All these changes are to address the long standing issue that this fails at
runtime:

async fn handle(
    req: Request<Body>,
    ext: Extension<Foo>,
) {}

It fails because Request<Body> removes all the extensions so Extension<Foo>
fails. The solution is to swap the lines so Request<Body> runs last.

With this change that becomes a compile error:

error[E0277]: the trait bound `fn(Request<Body>, Extension<Foo>) -> impl Future<Output = ()> {handler}: Handler<_, _>` is not satisfied
   --> hello-world/src/main.rs:13:44
    |
13  |     let app = Router::new().route("/", get(handler));
    |                                        --- ^^^^^^^ the trait `Handler<_, _>` is not implemented for `fn(Request<Body>, Extension<Foo>) -> impl Future<Output = ()> {handler}`
    |                                        |
    |                                        required by a bound introduced by this call
    |

#[debug_handler] will give a more precise error.

It also makes multiple body extractors a compile error:

async fn handle(
    _: Json<User>,
    _: String,
) {}

It works by changing FromRequest to

// new type paramter `R`!
pub trait FromRequest<R, B> {
    type Rejection: IntoResponse;

    async fn from_request(req: &mut RequestParts<R, B>) -> Result<Self, Self::Rejection>;
}

In practice R is either Mut or Once which are zero sized marker types. Not stoked
about the names 🤷

If you have a RequestParts<Mut, _> you cannot remove all the extensions, all
headers, or the body. I.e. the extractor is safe to run multiple times. If you
have a RequestParts<Once, _> then you can consume all the headers, extensions,
or the body.

So if your extractor needs to consume the body you implement FromRequest<Once, _>
otherwise you implement FromRequest<R, _>.

Then the Handler impls all now require the last extractor to implement
FromRequest<Once, _> and all the first to implement FromRequest<Mut, _>.
Thus if you have multiple extractors that consume the body that function wont
implement Handler.

What to focus on when reviewing

Most changes are very mechanical. Just adding an R type param to extractors
that don't care about the body and Once if they do.

So reviewing should be focused on axum-core/src/extract/mod.rs and
specifically which methods RequestParts<Mut, _> and RequestParts<Once, _>
gives you access to. All other changes fall from that.

Why is the diff so big?!

The diff is so large because we couldn't use a declarative macro to make all the
Handler impls, since we have to generate trait bounds in the macro. So I wrote
a little internal script that generates the code with quote and writes it to a
file.

#[derive(FromRequest)]

I removed #[derive(FromRequest)] for now since making it work with the new
setup is a lot of work.

For example how should this

#[derive(FromRequest)]
struct Foo {
    one: HeaderMap,
    two: String,
}

implement FromRequest? To work it must implement FromRequest<Once, _>
because otherwise it cannot extract String. But how can the macro know that?

I suppose we could do

#[derive(FromRequest)]
#[from_request(mut/once)]
struct Foo {
    one: HeaderMap,
    two: String,
}

but we'd have to be careful not to allow

#[derive(FromRequest)]
#[from_request(once)]
struct Foo {
    one: Bytes,
    two: String,
}

We could do that in a similar way by requiring only the last field to implement
FromRequest<Once, _>.

All this is a lot of work that I didn't wanna do in this PR. I might work on it
after this is merged but I don't think it should block the release.

TODO

  • Update the changelog. It should describe how to update from the old setup
    to the new.

@davidpdrsn davidpdrsn mentioned this pull request Jun 26, 2022
Closed
davidpdrsn
davidpdrsn commented Jun 26, 2022
View reviewed changes
axum-core/src/extract/mod.rs Outdated
/// See [`FromRequest`] for more details.
// TODO(david): naming
#[derive(Debug, Clone, Copy)]
pub struct Mut(Infallible);
Copy link
Member Author

Choose a reason for hiding this comment

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

Not a fan of this name. Should come up with a better one.

@davidpdrsn davidpdrsn marked this pull request as ready for review June 26, 2022 19:12
@davidpdrsn davidpdrsn requested a review from jplatte June 26, 2022 19:12
@davidpdrsn davidpdrsn added this to the 0.6 milestone Jun 27, 2022
@davidpdrsn
Prepare axum-next branch
aa6557c
@davidpdrsn
Remove deprecated extractor_middleware function (#1077)
83ec57b
@davidpdrsn
Allow Error: Into<Infallible> for Route::{layer, route_layer} (#948)
6b33e46 
* Allow `Error: Into<Infallible>` for `Route::{layer, route_layer}` (#924)

* Allow `Error: Into<Infallible>` for `Route::{layer, route_layer}`

Fixes #922

* changelog

* fixup changelog
@davidpdrsn
Panic on overlapping routes in MethodRouter (#1102)
263f964 
* Panic on overlapping routes in `MethodRouter`

* changelog link

* add test to ensure `head` and `get` don't overlap
@davidpdrsn
Fix changelog
e53b8f7
@davidpdrsn davidpdrsn force-pushed the limit-extractor-mutation branch from c57b5e4 to 8f5c055 Compare June 27, 2022 19:57
@davidpdrsn davidpdrsn force-pushed the axum-next branch 2 times, most recently from f6dbabe to f6bab0f Compare June 28, 2022 19:35
Base automatically changed from axum-next to main June 28, 2022 20:07
@davidpdrsn davidpdrsn mentioned this pull request Jul 11, 2022
Closed
2 tasks
@davidpdrsn davidpdrsn removed the breaking change A PR that makes a breaking change. label Jul 13, 2022
@davidpdrsn davidpdrsn mentioned this pull request Jul 17, 2022
Closed
@davidpdrsn davidpdrsn mentioned this pull request Jul 26, 2022
Merged
@davidpdrsn
Copy link
Member Author

Gonna mark this as a draft until #1086 is merged.

@davidpdrsn davidpdrsn marked this pull request as draft July 26, 2022 17:56
@davidpdrsn
Copy link
Member Author

I think given how hard it was to review #1155 I'll close this and break it up into smaller PRs. Those individual PRs wont compile but I think it'll be easier to review and discuss if the changes are smaller. Then once we're happy with the lower level changes we can fix all the downstream errors.

@davidpdrsn davidpdrsn closed this Aug 17, 2022
@davidpdrsn davidpdrsn deleted the limit-extractor-mutation branch August 17, 2022 20:22
@davidpdrsn davidpdrsn mentioned this pull request Aug 17, 2022
Merged
11 tasks
@davidpdrsn davidpdrsn restored the limit-extractor-mutation branch August 18, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplatte jplatte Awaiting requested review from jplatte

Assignees
No one assigned
Labels
A-axum A-axum-core A-axum-extra A-axum-macros
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

Idea: Split FromRequest trait into three (like the Fn traits)
1 participant
@davidpdrsn