RReverser / serde-xml-rs

xml-rs based deserializer for Serde (compatible with 1.0+)
https://crates.io/crates/serde-xml-rs
MIT License
273 stars 92 forks source link

serde(with = ...) not supported on optional fields? #114

Open ilyvion opened 5 years ago

ilyvion commented 5 years ago

I have a field that is sometimes missing, and otherwise it has to be parsed to bool from either "1" or "0". This particular requirement isn't natively supported by serde, so I had to implement my own deserialize function. The field looks like this:

#[serde(rename = "usingmatchkey", with = "serializers::zero_one_bool_option")]
pub using_match_key: Option<bool>,

I believe what I'm reporting to be a bug with serde-xml-rs and not serde itself, because the behavior that I'm trying for here is supported by serde-json, as evidenced by the discussion here. My own variant of this is:

pub fn deserialize<'de, D>(deserializer: D) -> std::result::Result<Option<bool>, D::Error>
where
    D: Deserializer<'de>,
{
    #[derive(Deserialize)]
    struct Wrapper(#[serde(with = "super::zero_one_bool")] bool);

    println!("Called!")
    let v = Option::deserialize(deserializer)?;
    Ok(v.map(|Wrapper(a)| a))
}

However, in the case of a missing usingmatchkey attribute, we don't even get called (as evidenced by no printout of "Called!". Instead, I get this unhelpful error:

custom: 'missing field usingmatchkey'

Right. It can be missing. That's why it's marked with Option in the first place...

If I leave off serde's with attribute, the Option works as expected, but the bool gets parsed incorrectly, because serde doesn't automatically support 1/0 bools.

ilyvion commented 5 years ago

By happenstance, I noticed that support for this particular idea had been added in one of the latest commits, but had not made it to a release on crates.io, so I just set my dependency to this git repo for now. Still, I think this is an issue that should be resolved in general, because I imagine it'll be a problem not just with custom deserializing for Option<bool>, but for any Option<T>.

ilyvion commented 5 years ago

And one more update, because the built-in implementation is bugged. This attribute: autoEnter="0" when marked as bool is read in as true. So I had to switch back to my zero_one_bool deserializer to get it to work right... but that still leaves me with my zero_one_bool_option being broken due to the problem reported originally. sigh

punkstarman commented 5 years ago

Thank you for submitting this.

PR #111 solves the bug you mention with parsing booleans in attributes. I am trying to find the time to review it. The commit you referred to only handled an element's contents.

I agree that this issue is more about fixing the general case, which should be done.