Parsing optional field seems to not work for numbers when not filled

[cedeber]

Hello, I currently try to parse a form but I have a Parse error for a specific case:

struct TestForm {
    text: Option<String>,
    number: Option<u32>
} 

1. Whether I filled the text input field or not, it ends up being `Some("whatever") or 'None'.
2. When I fill the number input field (a html text input), It parses to Some(123).
3. But when I don't fill the number field, it fails and return a Parse error (a UrlencodedError::Parse error apparently)

Same behaviour for f32 so far I've tested.

• Rust Version: 1.48.0
• Actix Web Version: 3.3.2

Thanks a lot for the help.

[robjtede]

Can you see if you see the same behavior using serde_urlencoded without actix-web?

[cedeber]

Good catch, from:

use serde::Deserialize;

#[derive(Debug, Deserialize)]
struct FormData {
    text: Option<String>,
    number: Option<u32>,
}

fn main() {
    println!(
        "{:#?}",
        serde_urlencoded::from_str::<FormData>("text=baguette&number")
    );
}

it fails with

Err(
    Error {
        err: "cannot parse integer from empty string",
    },
)

It also looks like an empty string, so text&number=32 for instance, prints

Ok(
    FormData {
        text: Some(
            "",
        ),
        number: Some(
            32,
        ),
    },
)

instead of

Ok(
    FormData {
        text: None,
        number: Some(
            32,
        ),
    },
)

which I believe is better.

So it doesn't support the Option at all which is weird because optional fields on form are usual.

But, regarding the answer https://github.com/nox/serde_urlencoded/issues/36#issuecomment-450348356 (from 2018) I believe one should switch the parsing lib... Not supporting optional inputs is weird to me.

[cedeber]

I very quickly checked, and Rocket seems to use https://crates.io/crates/form_urlencoded. Few weeks ago I used Rocket (master, so v0.5) instead of Actix with the exact same forms and it worked well.

[Martichou]

We might want to switch to https://docs.rs/serde_qs/0.8.1/serde_qs/ ? Which does return the correct value from the string.

Exemple:

#[derive(Debug, serde::Deserialize)]
struct FormData {
    text: Option<String>,
    number: Option<u32>,
}

fn main() {
    println!(
        "{:#?}",
        serde_urlencoded::from_str::<FormData>("text&number=32")
    );
    println!(
        "{:#?}",
        serde_qs::from_str::<FormData>("text&number=32")
    );
}

Output:

Ok(
    FormData {
        text: Some(
            "",
        ),
        number: Some(
            32,
        ),
    },
)
Ok(
    FormData {
        text: None,
        number: Some(
            32,
        ),
    },
)

I've submitted a PR to fix this issue :)

[robjtede]

I'm certain this has come up before. The problem here is that serde_qs is not behaving in a spec compliant way here. text&number=32 should not produce text: None.

[cedeber]

I understand and agree that this behaviour is correct from a query parser point of view: text&number=32 should not produce text: None. Maybe the lib implementation is correct if you don't consider what sends the request. But well, this is the title: x-www-form-urlencoded meets Serde

Here we are talking about web::Form which is a Form deserializer utility and imho it should behave like a form behaves.

So, regarding browsers behaviour, a required input field is not valid until the text is not empty. From MDN:

The Boolean required attribute which, if present, indicates that the user must specify a value for the input before the owning form can be submitted

Conversely, an empty string for an optional input (default) has no value, it is None then. A web form sends all data, filled or not. I think we can't even discuss if an empty string is valuable or not, the web standard decided for us.

Unfortunately, in the current implementation state, it makes the deserialization completely useless for optional inputs as we have to treat everything as String and do the job ourselves anyway.

[robjtede]

Found a reasonable workaround for now.

/// Swallows de-serialization errors.
#[derive(Debug)]
struct Opt<T>(Option<T>);

impl<T> Opt<T> {
    pub fn into_inner(self) -> Option<T> {
        self.0
    }
}

impl<'de, T: de::Deserialize<'de>> de::Deserialize<'de> for Opt<T> {
    fn deserialize<D: de::Deserializer<'de>>(d: D) -> Result<Self, D::Error> {
        Ok(Opt(T::deserialize(d).ok()))
    }
}

which can be used as such....

#[derive(Debug, Deserialize)]
struct FormData {
    text: Option<String>,
    number: Opt<u32>,
}

fn main() {
    let pls = &["text=abc&number=123", "text=abc&number", "text=abc"];

    for pl in pls {
        println!("");
        println!("{}", pl);

        println!(
            "serde_urlencoded: {:?}",
            serde_urlencoded::from_str::<FormData>(pl)
        );

        println!("serde_qs:         {:?}", serde_qs::from_str::<FormData>(pl));
    }
}

producing...

serde_urlencoded: Ok(FormData { text: Some("abc"), number: Opt(Some(123)) })
serde_qs:         Ok(FormData { text: Some("abc"), number: Opt(Some(123)) })

text=abc&number
serde_urlencoded: Ok(FormData { text: Some("abc"), number: Opt(None) })
serde_qs:         Ok(FormData { text: Some("abc"), number: Opt(None) })

text=abc
serde_urlencoded: Ok(FormData { text: Some("abc"), number: Opt(None) })
serde_qs:         Ok(FormData { text: Some("abc"), number: Opt(None) })

[robjtede]

Lemme have a think about this. There's a definite limitation in serde_urlencoded here; but I don't think simply replacing it with serde_qs a good solution.

[Martichou]

I think your solution is pretty neat but do you think this kind of problem should be handled by the user/developer using actix-web instead of having actix-web handle it for them?

If actix-web were to manage it for them, maybe a rewrite of serde_urlencoded with some specific changes for actix-web could be useful?

[robjtede]

I think your solution is pretty neat but do you think this kind of problem should be handled by the user/developer using actix-web instead of having actix-web handle it for them?

Absolutely not. Framework should handle this.

If actix-web were to manage it for them, maybe a rewrite of serde_urlencoded with some specific changes for actix-web could be useful?

As this is the only open issue regarding problems with serde_urlencoded, I'm considering it a tracking issue for a remedy.

[robjtede]

Slightly less involved workaround:

pub fn deserialize_option_ignore_error<'de, T, D>(d: D) -> Result<Option<T>, D::Error>
where
    T: de::Deserialize<'de>,
    D: de::Deserializer<'de>,
{
    Ok(T::deserialize(d).ok())
}

#[derive(Debug, Deserialize)]
struct FormData {
    text: Option<String>,

    #[serde(default, deserialize_with = "deserialize_option_ignore_error")]
    number: Option<u32>,
}

[cedeber]

Cool, happy to see that moving forward. And thanks for the workaround, I'll try that.

[cedeber]

Hey. Ok, looks like it helps me continue. Thanks. How can we help here by the way?

[kevlarr]

@robjtede That workaround is incredibly helpful, thank you!

In my situation, that helped me to have more graceful failures when query params aren't right (either empty like someNumber= which is easy to obtain through a form with a GET action, or someone URL hacking and setting someNumber=asdf in which case I'd prefer for it to just be ignored and treated as None rather than seeing an error page).

[jplatte]

Cleaner workaround that doesn't swallow errors with an unrelated cause: https://github.com/ruma/ruma/blob/56801780b659be609bbcb9ce3701ed2676130304/crates/ruma-serde/src/strings.rs#L10-L32