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.

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

FromForm Option<Result> with custom type #2801

Closed
4 tasks done
cazgp opened this issue May 30, 2024 · 4 comments
Closed
4 tasks done

FromForm Option<Result> with custom type #2801

cazgp opened this issue May 30, 2024 · 4 comments
Labels
triage A bug report being investigated

Comments

@cazgp
Copy link

cazgp commented May 30, 2024

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.
@cazgp cazgp added the triage A bug report being investigated label May 30, 2024
@SergioBenitez
Copy link
Member

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>>?

@SergioBenitez SergioBenitez closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2024
@the10thWiz
Copy link
Collaborator

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.

@the10thWiz the10thWiz reopened this Jul 20, 2024
@SergioBenitez
Copy link
Member

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
Copy link
Member

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

@SergioBenitez SergioBenitez closed this as not planned Won't fix, can't repro, duplicate, stale Jul 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage A bug report being investigated
Projects
None yet
Development

No branches or pull requests

3 participants