jplatte / serde_html_form

Rust crate for (de-)serialization of `application/x-www-form-urlencoded` data.
MIT License
23 stars 3 forks source link

Can't handle missing fields when deserializing into Vec #2

Closed samhclark closed 9 months ago

samhclark commented 1 year ago

Problem description

When deserializing application/x-www-form-urlencoded values from a form where multiple inputs have the same name into a single Vec, it can't handle when none of the values are present. (Like when all checkboxes are optional)

Example

To give a concrete example, I'll rip off the <form> example from WHATWG. Say you have a form like this:

<form method="post" action="submit">
    <fieldset>
    <legend> Pizza Toppings </legend>
        <p><label> <input type=checkbox name="topping" value="bacon"> Bacon </label></p>
        <p><label> <input type=checkbox name="topping" value="cheese"> Extra Cheese </label></p>
        <p><label> <input type=checkbox name="topping" value="onion"> Onion </label></p>
        <p><label> <input type=checkbox name="topping" value="mushroom"> Mushroom </label></p>
    </fieldset>
    <p><button>Submit order</button></p>
</form>

You can correctly deserialize it into the following struct if at least one of the checkboxes are checked when the form is submitted:

struct PizzaOrder {
    topping: Vec<String>,
}

But if you submit the form with none of the boxes checked, then we get this error:

Failed to deserialize form: missing field topping

If you change the struct to the following:

struct PizzaOrder {
    topping: Option<Vec<String>>,
}

Then you can deserialize it when the field is missing, but now you get this error when you select one item:

Failed to deserialize form: invalid type: string "bacon", expected a sequence

and this error when you select multiple items:

Failed to deserialize form: unsupported value

Minimal reproduction

use axum::{
    response::{Html, IntoResponse},
    routing::{get, post},
    Router,
};
use axum_extra::extract::Form;

#[tokio::main]
async fn main() {
    let app = Router::new()
        .route("/", get(form))
        .route("/submit", post(submit));

    axum::Server::bind(&"0.0.0.0:3000".parse().unwrap())
        .serve(app.into_make_service())
        .await
        .unwrap();
}

async fn form() -> Html<&'static str> {
    Html(
        r#"
        <form method="post" action="submit">
            <fieldset>
                <legend> Pizza Toppings </legend>
                <p><label> <input type=checkbox name="topping" value="bacon"> Bacon </label></p>
                <p><label> <input type=checkbox name="topping" value="cheese"> Extra Cheese </label></p>
                <p><label> <input type=checkbox name="topping" value="onion"> Onion </label></p>
                <p><label> <input type=checkbox name="topping" value="mushroom"> Mushroom </label></p>
            </fieldset>
            <p><button>Submit order</button></p>
        </form>
    "#,
    )
}

#[derive(serde::Deserialize, Debug)]
struct PizzaOrder {
    topping: Vec<String>,
}

async fn submit(body: Form<PizzaOrder>) -> impl IntoResponse {
    dbg!(body);
}

Solution?

I really wish I was good enough with Rust to figure out how to contribute here. I would have loved to send a PR along with this, but I've been struggling. In my head, the "right answer" is to set topping to an empty Vec when the field is missing. It's what I tried to do first, and I don't see the benefit in wrapping it in an Option if that means replacing the "empty list" state with None.

Related

https://github.com/tokio-rs/axum/issues/1029 https://github.com/tokio-rs/axum/pull/1031

Posting here based on the last comment on that first issue. I can move this if another repo needs to handle this.

samhclark commented 1 year ago

Damn, I feel really silly, I didn't consider this option until I was digging into your implementation some more:

#[derive(serde::Deserialize, Debug)]
struct PizzaOrder {
    #[serde(default = "empty_list")]
    topping: Vec<String>,
}

fn empty_list() -> Vec<String> {
    vec![]
}

That works

Edit:

Or actually just

#[derive(serde::Deserialize, Debug)]
struct PizzaOrder {
    #[serde(default)]
    topping: Vec<String>,
}
jplatte commented 1 year ago

This has gotten some renewed attention as a pattern people expect to work. I'm skeptical whether it's possible to make it work without regressing other desireable properties of the parser, but I'll reopen this for now while that's not clear. I guess it should at least be documented that it doesn't work, if it's not fixable.

flo-at commented 1 year ago

I just had the same problem and was lucky to find this issue. Option<Vec<_>> behaves really strangely. I switched from the axum Query extractor (which requires the Option<_> part) to the one from axumextra and it took me some time to realize that I don't need the `Option<>` at all. Maybe just a warning in the documentation about this would help.

Thanks a lot for sharing the crate!

jplatte commented 1 year ago

Maybe just a warning in the documentation about this would help.

Want to send a PR? :)

flo-at commented 1 year ago

I guess this happens for the combination of deserialize_option and deserialize_seq. I'm unsure what would be a good place to add a note about this. Probably deserialize_option is the better place. If you agree I'll make a PR.

jplatte commented 1 year ago

I thought we were talking about the toplevel documentation of the crate. I think it would make sense for that to contain a note.

boggye commented 12 months ago

I had the same issue - I am glad I found this. Adding #[serde(default)] worked for me.

jplatte commented 9 months ago

Closing in favor of #11 which has a much clearer description.