Add a separate trait for optional extractors #2475
Conversation
jplatte
force-pushed
the
jplatte/optional-from-request
branch
2 times, most recently
from
d988573
to
91fabd2
Compare
… and deprecate OptionalPath.
… and deprecate OptionalQuery.
jplatte
force-pushed
the
jplatte/optional-from-request
branch
from
91fabd2
to
b8f499e
Compare
jplatte
added
C-cleanup
|
@davidpdrsn Have you had time to take a look? The main changes are done, and I don't want to do the remaining docs / changelog writing and cleanup if it's not yet clear whether this will be merged. |
There was a problem hiding this comment.
One thing that bugs me is that the optional variants use the same rejections so for example if someone uses the optional typed header there still is valid representation in the rejection. for the missing case.
That said the rejections are non exhaustive so they cannot handle explicitly everything so they may omit this one as well. It might just be confusing.
I will read it more thoroughly tomorrow or over the weekend, but it makes sense as a whole.
I want to think on other ways to make the option work but on a first sight this may be the best solution.
| @@ -152,9 +174,11 @@ impl std::error::Error for QueryRejection { | |||
| /// | |||
| /// [example]: https://github.com/tokio-rs/axum/blob/main/examples/query-params-with-empty-strings/src/main.rs | |||
| #[cfg_attr(docsrs, doc(cfg(feature = "query")))] | |||
| #[deprecated = "Use Option<Path<_>> instead"] | |||
There was a problem hiding this comment.
The code looks good to me.
The drawback I see here is having two distinct traits so it might be harder for users to discover how to properly use Options. That said in hindsight, it might be better to force users to use Result instead of Option if they really mean to just discard any errors.
I am still unsure about whether it is better to use the same rejections for optional and normal extractors if the missing case is already handled in the normal rejection. I think we could use some kind of enum (probably very similar to Option) for extractors that have optional variants. But each extractor would have to define IntoResponse for its missing case.
I also wonder if we couldn't get away with just one FromRequestParts trait where the extracting method would return an enum with three cases like MyResult<T, T::Missing, T::Rejection>. Then we could maybe create the extractors with correct semantics without the need for multiple traits.
But if that's not possible or having the two traits is a better tradeoff than whatever issues that potential solution has, the implementation as is looks ok. I think we should just go over the implementations of FromRequest and FromRequstParts and add a few implementations for the optional conterparts.
| #[derive(Debug, Clone, Copy, Default)] | ||
| pub struct OptionalQuery<T>(pub Option<T>); | ||
|
|
||
| #[allow(deprecated)] |
There was a problem hiding this comment.
OptionalPath became this:
parts
.extract::<Option<Path<T>>>()
.await
.map(|opt| Self(opt.map(|Path(x)| x)))
I think that a similar thing can be done here.
| use axum::{extract::rejection::PathRejection, http::StatusCode}; | ||
| use axum_extra::routing::{RouterExt, TypedPath}; |
There was a problem hiding this comment.
Nit, this change seems that the uses were just reordered.
| async fn option_handler(_: Option<UsersShow>) {} | ||
|
|
There was a problem hiding this comment.
I think this should also be derived and usable.
| where | ||
| S: Send + Sync, | ||
| { | ||
| type Rejection = MatchedPathRejection; |
There was a problem hiding this comment.
I think this should be Infallible as missing is the only failure case.
And just to be sure, it seems to me like the only way MatchedPath would be missing is if it is accessed inside a fallback handler? If that's so, I think it would be better not to add this impl and have users just use Result if they want to guard against using it there.
| @@ -82,13 +82,11 @@ pub struct Pagination { | |||
| } | |||
|
|
|||
| async fn todos_index( | |||
| pagination: Option<Query<Pagination>>, | |||
| Query(pagination): Query<Pagination>, | |||
There was a problem hiding this comment.
This should not change I think.
There was a problem hiding this comment.
Ah, this was intentional but I guess it should really be a separate PR with its own description.
Motivation
Closes #2298.
Solution
OptionalFromRequest,OptionalFromRequestPartsOptionalFooextractors