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 #1272

Merged
merged 21 commits into from
Aug 22, 2022
Merged

Only allow last extractor to mutate the request #1272

merged 21 commits into from
Aug 22, 2022

Conversation

davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented Aug 17, 2022 •
edited
Loading

Fixing #1115 requires touching pretty much every file (see #1121). So rather than implementing everything at once I think we should split the work into smaller PRs and then merge them into this branch. That way we don't have to review the whole thing at once.

Fixes #1115

TODO

Things to fix before we can merge this

  • docs for axum-extra
  • docs for axum-macros
  • On FromRequestParts document having to implement FromRequest if you need the body. To help with https://github.com/tokio-rs/axum/pull/1283/files#r950838603
  • changelog
  • document how to write extractors that wrap other extractors (needs to implement both traits manually). See https://github.com/tokio-rs/axum/pull/1288/files#r950879490
  • Can we remove any rejections for Path, like the one for the extension missing? No probably not. Someone might run Path from anywhere without running the request through Router and that shouldn't panic.
  • FromRequest and FromRequestParts for tuples (see tuple.rs)
  • Update Handler::or in axum-extra. Note this depends on the tuple impls
  • Update #[derive(FromRequest)]
  • Do we want #[derive(FromRequestParts)]? If so, that can be done after merging this. File issue if we want it.
  • Decide on final names for Mut and Once.

@davidpdrsn davidpdrsn added this to the 0.6 milestone Aug 17, 2022
@davidpdrsn davidpdrsn mentioned this pull request Aug 18, 2022
Merged
davidpdrsn and others added 3 commits August 18, 2022 22:16
@davidpdrsn @jplatte
Change FromRequest and add FromRequestParts trait (#1275)
10d0460 
* Add `Once`/`Mut` type parameter for `FromRequest` and `RequestParts`

* 🪄

* split traits

* `FromRequest` for tuples

* Remove `BodyAlreadyExtracted`

* don't need fully qualified path

* don't export `Once` and `Mut`

* remove temp tests

* depend on axum again

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@davidpdrsn
Port Handler and most extractors (#1277)
3cdf77e 
* Port `Handler` and most extractors

* Put `M` inside `Handler` impls, not trait itself

* comment out tuples for now
@davidpdrsn
fix lints
1a9f5c8
@davidpdrsn
Copy link
Member Author

@jplatte I took at stab at the tuple impls but couldn't quite make it work, even with adding another possible value of M. Do you wanna give it a shot?

@davidpdrsn
Reorder arguments to Handler (#1281)
75cdd09 
I think `Request<B>, Arc<S>` is better since its consistent with
`FromRequest` and `FromRequestParts`.
@jplatte
Copy link
Member

jplatte commented Aug 19, 2022

I'll do that, yeah. Won't be in front of a capable computer until Sunday though.

davidpdrsn and others added 6 commits August 19, 2022 23:17
@davidpdrsn
Port most things in axum-extra (#1282)
69e7164
@davidpdrsn
Merge branch 'main' into from-request-mut-once
0b8bfb0
@davidpdrsn
Port #[derive(TypedPath)] and #[debug_handler] (#1283)
bf5675d 
* port #[derive(TypedPath)]

* wip: #[debug_handler]

* fix #[debug_handler]

* don't need itertools

* also require `Send`

* update expected error

* support fully qualified `self`
@jplatte
Implement FromRequest[Parts] for tuples (#1286)
a764757
@davidpdrsn
Port docs for axum and axum-core (#1285)
80dd045
@davidpdrsn @jplatte
Port axum-extra (#1287)
f8408e1 
* Port axum-extra

* Update axum-core/Cargo.toml

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

* remove `impl FromRequest for Either*`

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@davidpdrsn davidpdrsn mentioned this pull request Aug 21, 2022
Merged
@jplatte
New FromRequest[Parts] trait cleanup (#1288)
5e7e2f7 
* Make private module truly private again

* Simplify tuple FromRequest implementation
@jplatte
Copy link
Member

jplatte commented Aug 21, 2022

Open question from #1288: What should the final names for Mut and Once be (they will appear in the docs, even if they don't have doc pages of their own)?

  • Keep Mut and Once
  • ViaParts and something?
    • ViaRequest
    • Something like Regular / Default / Direct

@jplatte jplatte mentioned this pull request Aug 21, 2022
Closed
@davidpdrsn
Copy link
Member Author

Re Mut/Once naming I like

  • Mut becomes ViaParts
  • Once becomes ViaRequest

impl<S, B, T> FromRequest<S, B, ViaParts> for T does kinda make sense since we're extracting a T "via the request parts".

trait FromRequest<S, B, M = ViaRequest> is repetitive but at least it matches the other.

davidpdrsn and others added 2 commits August 22, 2022 10:30
@davidpdrsn
axum-extra doesn't need to depend on axum-core (#1294)
49d469f 
Missed this in #1287
@davidpdrsn @jplatte
Add FromRequest changes to changelogs (#1293)
3cf5aaa 
* Update changelog

* Remove default type for `S` in `Handler`

* Clarify which types have default types for `S`

* Apply suggestions from code review

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>

Co-authored-by: Jonas Platte <jplatte+git@posteo.de>
@jplatte
Copy link
Member

jplatte commented Aug 22, 2022

Okay, let's go with that then.

@davidpdrsn davidpdrsn marked this pull request as ready for review August 22, 2022 10:14
@davidpdrsn
Copy link
Member Author

I guess this is ready for review now 😊 All the todos are done and CI is green.

jplatte
jplatte approved these changes Aug 22, 2022
View reviewed changes
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

I already reviewed everything individually, is more review needed?

@davidpdrsn
Copy link
Member Author

Don't think so! :shipit:

@davidpdrsn davidpdrsn merged commit be62430 into main Aug 22, 2022
@davidpdrsn davidpdrsn deleted the from-request-mut-once branch August 22, 2022 10:23
@davidpdrsn davidpdrsn mentioned this pull request Aug 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplatte jplatte jplatte approved these changes

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)
2 participants
@davidpdrsn @jplatte