FromForm Option<Result> with custom type

[cazgp]

Rocket Version

0.5.1

Operating System

Linux

Rust Toolchain Version

rustc 1.79.0-nightly (0d8b3346a 2024-04-14)

What happened?

Create a form and derive FromForm with a field Option<Result<T>> where T is a custom type. Submit the form with nothing for that field, and the field is parsed as Some(Missing error) instead of None.

Test Case

#[macro_use] extern crate rocket;
use rocket::form::FromFormField;

#[derive(Debug)]
pub struct Csl<T: std::fmt::Display>(pub Vec<T>);

#[rocket::async_trait]
impl<'r, T> FromFormField<'r> for Csl<T>
where
    T: std::str::FromStr,
    T: std::fmt::Display,
    T: std::marker::Send,
{
    fn from_value(field: rocket::form::ValueField<'r>) -> rocket::form::Result<'r, Self> {
        let value = field.value;
        let mut vec: Vec<T> = vec![];
        if value.is_empty() {
            return Ok(Csl(vec));
        }

        for i in value.split(',') {
            let parsed = i.parse::<T>().map_err(|_| {
                rocket::form::Errors::from(rocket::form::error::ErrorKind::Validation(
                    format!("{} '{}' is incorrect type", field.name, i).into(),
                ))
            })?;
            vec.push(parsed);
        }

        Ok(Csl(vec))
    }

    async fn from_data(_: rocket::form::DataField<'r, '_>) -> rocket::form::Result<'r, Self> {
        todo!("This isn't implemented")
    }
}

#[cfg(test)]
mod tests {
    use rocket::form::Form;

    use super::Csl;

    #[derive(Debug, rocket::FromForm)]
    pub struct Test2<'a> {
        pub list: Option<rocket::form::Result<'a, Csl<i32>>>,
    }

    #[rocket::post("/test2", data = "<data>")]
    pub fn test2(data: Form<Test2>) -> String {
        dbg!(&data);
        match &data.list {
            None => "List is empty".to_string(),
            Some(x) => match x {
                Ok(y) => format!("List is: {}", y),
                Err(e) => format!("List failed: {}", e[0]),
            },
        }
    }

    fn client() -> rocket::local::blocking::Client {
        rocket::local::blocking::Client::untracked(
            rocket::build().mount("/", rocket::routes![test2]),
        )
        .unwrap()
    }

    #[test]
    fn it_should_handle_optional_values() {
        let response = client()
            .post("/test2")
            .header(rocket::http::ContentType::Form)
            .dispatch()
            .into_string()
            .unwrap();
        assert!(response, "List is empty");
    }


}

Log Output

🚀 Rocket has launched locally
POST /test2 application/x-www-form-urlencoded:
   >> Matched: (test2) POST /test2
[crudite/src/csl.rs:146:9] &data = Form(
    Test2 {
        list: Some(
            Err(
                Errors(
                    [
                        Error {
                            name: None,
                            value: None,
                            kind: Missing,
                            entity: Field,
                        },
                    ],
                ),
            ),
        ),
    },
)
   >> Outcome: Success(200 OK)
thread 'csl::tests::it_should_handle_optional_values' panicked at crudite/src/csl.rs:219:9:
assertion `left == right` failed
  left: "List failed: missing"
 right: "List is empty"

Additional Context

I don't think this is a runtime bug so haven't provided complete logs... it seems like it's a failure in parsing that no debug logs could help with.

System Checks

• My bug report relates to functionality.
• I have tested against the latest Rocket release or a recent git commit.
• I have tested against the latest stable rustc toolchain.
• I was unable to find this issue previously reported.

[SergioBenitez]

Create a form and derive FromForm with a field Option<Result> where T is a custom type. Submit the form with nothing for that field, and the field is parsed as Some(Missing error) instead of None.

This is exactly what's expected. Maybe you want Result<Option<T>>?

[the10thWiz]

Actually, Result<Option<T>> likely would not produce the desired behavior. The following example shows why:

#[get("/login?<p>")]
pub async fn login(p: Result<'_, Option<usize>>) -> String {
    format!("{p:?}")
}

#[launch]
fn rocket() -> _ {
    rocket::build()
        .mount("/", routes![login])
}

#[test]
fn test_options() {
    let client = Client::tracked(rocket()).unwrap();
    let response = client.get("/login?p=a").dispatch();
    let res = response.into_string().unwrap();
    assert!(res.starts_with("Err"), "Value: {res}");
}

When run, the form deserializes to Ok(None) (or Ok(Some(...)) if successful), since Option always ignores any inner error. In fact, right now, you should never use Option<Result<T>> or Result<Option<T>> because the inner type will always succeed. I think more appropriate behavior here would be for Option to only catch Missing errors, and propagate other errors.

[SergioBenitez]

That's fair. It's something I've considered changing as well. I believe this is the way that serde handles this, for example. The primary question is how to do this without making it a potentially costly silent breaking change.

[SergioBenitez]

Note that #2543 exists already for the same issue. Let's manage this there.