Stricter optional extractors

[arlyon]

• I have looked for existing issues (including closed) about this

Feature Request

Right now, an optional extractor in axum will maps to the following:

Valid X -> Some(X)
Invalid X -> None
Missing -> None

A result type maps to the following:

Valid X -> Ok(X)
Invalid X -> Err(X::Error)
Missing -> Implementation Dependent, but often some 'missing' error variant

Whereas, I would like the complement / dual of this, which is

Valid X -> Some(X)
Invalid X -> Rejected by extractor
Missing -> None

Motivation

It seems to be a common operation (ex: when handling cookies or login details) to want to get either valid data, or no data, but to reject invalid data, and it seems that axum doesn't really have the nuance to describe that relationship.

Proposal

This is a significant breaking change, but I think it is important to have a distinction at the extractor level between completely missing data, invalid data, and valid data. I think that in the case of the Option type, by default, the extractor's validation should be respected rather than returning None. Additionally, we could support Option<Result<E, E::Error>> as a middleground for those who need to be able to distinguish between valid, invalid, and missing data in their handlers.

Alternatives

I could wrap all libraries that are programmed to axum's APIs to give limited information, and try to intercept to infer that data myself on a case-by-case basis. I would rather not do that.

[jplatte]

You can already use Result<Extractor, ExtractorRejection> and inspect ExtractorRejection whether it is invalid or missing data that caused Extractor to fail. That is, if Extractor has these different failure modes at all.

[arlyon]

Yes that is the alternative, but I think it would be useful to push that information into the type system since we have the capability to do so with axum's excellent handlers. Saves having to dive into the implementation of the extractor itself to understand how it works, and, of course, repeat the same boilerplate checks across a number of handlers.

In the case that the extractor can't have missing values (is that possible in practice?) or can't fail parsing, then adding Option or Result to the type signature can simply always hit the Ok or Some case.

Also it would be cool if axum exported a result type similar to:

type Result<T: FromRequestParts> = Result<T, T::Rejection>;

But that is a separate idea :)

[davidpdrsn]

I like the spirit of the idea but I'm not sure it's feasible. Supporting something like Option<Result<E, E::Error>> would require changing FromRequest and FromRequestParts to probably return Option<Result<_, _>> as well, so we can distinguish between "no data" and "error". That would also require a way to convert None into a response.

Sorta feels to me like Result<T, T::Rejection> is okay.

[davidpdrsn]

repeat the same boilerplate checks across a number of handlers

No boilerplate needed. You can customize rejections like shown in this example

[arlyon]

repeat the same boilerplate checks across a number of handlers

No boilerplate needed. You can customize rejections like shown in this example

Thank you for directing me towards WithRejection, I believe that will be enough to cover my usecase. I quite like the additive nature of option and result in my proposal, however I realise we are quite far down the line and changing these two traits is probably not reasonable.

[davidpdrsn]

Consider JsonRejection:

pub enum JsonRejection {
    JsonDataError(JsonDataError),
    JsonSyntaxError(JsonSyntaxError),
    MissingJsonContentType(MissingJsonContentType),
    BytesRejection(BytesRejection),
}

when should that return None vs Err(_)?

• If there is no data in the request body? The type you deserialize into might be okay with that and ignore the data all together.
• If we don't get content-type: application/json? I suppose this is the only option but that's also surprising and you'd need to look up in the docs exactly what None means.
• If we can't buffer the request body? That's an error so you don't want to loose that fact.

I worry that we'd hit similar questions with other extractors as well. By just using an enum we avoid all that.

Thank you for directing me towards WithRejection, I believe that will be enough to cover my usecase.

Sounds good! I think I'll close this for now but feel free to ask follow up questions.

[arlyon]

So I had some more time today, and I don't think that any of the three mechanisms for customizing rejections will work for me on its own so I am back to a match statement. Essentially what I am looking for is a way to opt out of certain rejections, rather than customizing the rejection object. I want to be able to take a Result<T, T::Rejection> and map it into an Option<T> for all but one case of the T::Rejection enum. It doesn't seem that either WithRejection or the derive macro allows you to express that. I guess I could implement a newtype and write out a custom extractor I guess, but it feels odd. I am working with jwt cookies, so here is an example:

async fn handler(result: Result<Jwt, JwtRejection>) -> String {
  match result {
    Ok(jwt) => Ok(Some(jwt)),
    Err(JwtRejection::Missing) => Ok(None),
    rest => rest
  }
}

Json I agree is more complicated (though an error variant for 'no data at all' would be nice), an opt-in trait to express optional pieces of data (such as query strings, cookies) would be useful.

trait FromRequestPartsOptional<S>: FromRequestParts<S> {
    fn option_reject(result: Result<Self, Self::Rejection>) -> Result<Option<Self>, Self::Rejection>;
}

impl<T, S> FromRequestPartsOptional<S> for JwtClaims<T>
where
    T: DeserializeOwned + Send + Sync + Clone + 'static,
    S: Send + Sync,
{
    fn option_reject(result: Result<Self, Self::Rejection>) -> Result<Option<Self>, Self::Rejection> {
        match result {
            Ok(claims) => Ok(Some(claims)),
            Err(AuthError::MissingToken) => Ok(None),
            Err(e) => Err(e),
        }
    }
}

For cases where it is ambiguous, then the trait is not implemented, and Result<Option<T>, T::Rejection> is not supported in the handler. I feel like its a fairly common case to require, for example, having a valid cookie or none at all, having a valid set of query params or not, having a valid Basic auth, or none at all, bearer, etc.

[davidpdrsn]

I guess I could implement a newtype and write out a custom extractor I guess, but it feels odd

For something like what you're describing I think a wrapper is the best approach. It gives you full control.

[arlyon]

I have opted to write a crate for this (tentatively called 'axum-option') which solves the N + M problem (N implementors, M extractors) and provides an alternative extractor to Option along with associated traits that can be implemented for extractors that have genuine 'missing' statuses.

#[axum::async_trait]
#[cfg_attr(
    nightly_error_messages,
    rustc_on_unimplemented(
        note = "Function argument is not a valid optional extractor. \nSee `https://docs.rs/axum-option/latest/axum-option` for details",
    )
)]
pub trait FromRequestPartsOptional<S>: FromRequestParts<S> {
    async fn option_reject(
        result: Result<Self, Self::Rejection>,
    ) -> Result<Option<Self>, Self::Rejection>;
}

#[axum::async_trait]
#[cfg_attr(
    nightly_error_messages,
    rustc_on_unimplemented(
        note = "Function argument is not a valid optional extractor. \nSee `https://docs.rs/axum-option/latest/axum-option` for details",
    )
)]
pub trait FromRequestOptional<S, B>: FromRequest<S, B> {
    async fn option_reject(
        result: Result<Self, Self::Rejection>,
    ) -> Result<Option<Self>, Self::Rejection>;
}

It will need some PRs to properly expose missing data in axum before it is ready for publishing. I am currently using it with a fork. Would you be open to a PR that improves the detail in some extractors (such as a totally missing query string, for example)?

I will note that this would also support #1884 under the same machinery, which is why I was interested in having this. Under axum-option, the Path could just implement the trait, and OptionalPath<T> = ValidOption<Path<T>>, rejecting paths with invalid T types, but accepting paths with no T.

[jplatte]

Hm, I'm not convinced ValidOption(opt): ValidatOption<Path<T>> will be easier to use than OptionalPath(opt): OptionalPath<T>. What's the use case for a trait / another level of genericism here? Is there really a place where you want to abstract over different ValidOption compatible extractors?

[arlyon]

Rather than have the end user remember a bunch of additional types (one for each extractor they use), and have the implementer of those extractors write / maintain a newtype that contains the logic or worse have each end user have to write their own version, everyone in the ecosystem can use just use / implement the ValidOption trait instead of their own OptionalPath, OptionalQuery, OptionalTypedHeader, OptionalCookie, OptionalBytes, and of course, all the types that could be built on top of those extractors such as OptionalBearer, OptionalBasicAuth, OptionalJwt, OptionalJson, OptionalMessagePack, OptionalSession, or headers with custom validation (etc etc). There are any number of possible headers, cookies, or request bodies that could all be expressed via a single trait. I am currently using it a handful of locations for at least cookies, jwts, headers, and queries. I would also like to use it for paths.

Less code to maintain, fewer types to remember, and better ergonomics for both library authors and consumers. If you want to create a custom type, you can of course as mentioned just re-expose a type alias.

[jplatte]

I think I kind of get what you mean, but I don't really see a need for all of these extractors. I can kind of see use cases for a OptionalTypedHeader and OptionalJwt (as a specialized variation of the former), but Bytes already permits empty bodies and CookieJar allows there to be no cookies as well (I don't know of a Cookie extractor).

[davidpdrsn]

Would you be open to a PR that improves the detail in some extractors (such as a totally missing query string, for example)?

Sure, as long as we don't have to rewrite the two extractor traits and just make the rejection enums more informative. This generally seems like a better approach to me than writing new Optional* extractors.

[jplatte]

such as a totally missing query string, for example

Actually, I think this is a bad example. If you intend to change QueryRejection, please open a feature request about that first since I think any time put into it might be wasted.

[jplatte]

I've come around to this. In a significant number of cases, using Option<_> as an extractor seems like a good pattern when it actually isn't because essentially users only wanted to have axum call the handler with None in specific error cases, others were not thought about.

I opened issue #2298 with a more detailed explanation of where I see the problem(s) and PR #2475 for an (as of yet incomplete) implementation.