FromRequest for Option<T> should fail if from_request(...) T fails, not be None

[jespersm]

API Docs to Existing Functionality

https://api.rocket.rs/master/rocket/request/trait.FromRequest

Problems with Existing Functionality

When using the FromRequest guards/extractors with an Option, there's no differentiation between a good and a bad request.

Imagine the following struct:

pub struct SessionId {
    session_id: u64,
}

#[rocket::async_trait]
impl<'r> FromRequest<'r> for SessionId {
    type Error = ParseIntError;

    async fn from_request(request: &'r Request<'_>) -> Outcome<Self, (Status, Error), Status> {
        request.headers().get("Session-Id").next()
            .or_forward(Status::BadRequest)
            .and_then(|v| v.parse()
            .map(|id| SessionId { session_id: id })
            .or_error(Status::BadRequest))
    }
}

Let's imagine we have a handler which can handle the 'Session-Id' header if it is sent in, but can function without:

#[get("/optional")]
fn get_data_with_opt_header(opt_header: Option<SessionId>) -> String {
    if let Some(id) = opt_header {
        format!("GET for session {:}", id.session_id)
    } else {
        format!("GET for new session")
    }
}

In the current semantics for Option where T: FromRequest, the request will succeed, even with a malformed header, passed in as None. If I were a client of that API, I'd find it highly surprising.

Suggested Changes

I would suggest that the Option guard semantics were changed to fail if Outcome::Error, mapping Forward to None. In other words, this would be how I'd implement it:

#[crate::async_trait]
impl<'r, T: FromRequest<'r>> FromRequest<'r> for Option<T> {
    type Error = T::Error;

    async fn from_request(request: &'r Request<'_>) -> Outcome<Self, Self::Error> {
        match T::from_request(request).await {
            Success(val) => Success(Some(val)),
            Forward(_) => Success(None),
            Error((status, error)) => Error((status, error)),
        }
    }
}

That way, we could keep the parse errors and whatnot as errors (and not silently suppress them). This would compose well with catchers.

Alternatives Considered

In the current solution, the only alternative for a handler would be to use the an Outcome guard, like this:

#[get("/optional2")]
fn get_data_with_opt_header2(header_outcome: Outcome<SessionId, ParseIntError>) -> (Status, String) {
    match header_outcome {
        Outcome::Success(id) => (Status::Ok, format!("GET for session {:}", id.session_id)),
        Outcome::Error((_, e)) => (Status::BadRequest, format!("GET could not get session because: {:}", e)),
        Outcome::Forward(_) => (Status::Ok, format!("GET for new session")),
    }
}

This adds extra complexity and forces boilerplate error handling code as well as Rocket-specific type use into the handlers.

Additional Context

In #2867 I've implemented typed headers (see #1067). This would fit well with these headers, like Option, etc.

I have a patch ready.

System Checks

• I do not believe that this suggestion can or should be implemented outside of Rocket.

• I was unable to find a previous suggestion for this change.

[SergioBenitez]

Isn't this #2543?

[jespersm]

Isn't this #2543?

Close, but #2543 is for FromData. This is analogous, but I guess the motivaion is very similar.

[SergioBenitez]

Got it. I intended for #2532 to be a general change the behavior of the Option<T> guard issue, so let's track this there.