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

Numbers and &str deserialization fails #5

Open
spease opened this issue Feb 17, 2018 · 7 comments
Open

Numbers and &str deserialization fails #5

spease opened this issue Feb 17, 2018 · 7 comments

Comments

@spease
Copy link

spease commented Feb 17, 2018

I was trying to deserialize a u64 and noticed it was failing, even though strings and other objects were working fine. There didn't seem to be any problem with the text being deserialized:

id=61

So I tried making a workaround to investigate:

fn u64_workaround<'de, D>(deserializer: D) -> std::result::Result<u64, D::Error>
where
    D: serde::de::Deserializer<'de>,
{
    use serde::Deserialize;
    <&str>::deserialize(deserializer)?.parse().map_err(serde::de::Error::custom)
}

However this failed, so I eventually tried using a full String

fn u64_workaround<'de, D>(deserializer: D) -> std::result::Result<u64, D::Error>
where
    D: serde::de::Deserializer<'de>,
{
    use serde::Deserialize;
    String::deserialize(deserializer)?.parse().map_err(serde::de::Error::custom)
}

This works, but obviously involves a heap allocation that would seem to be unnecessary.

Regardless, without this workaround, the typical behavior is that integers and real numbers of all types seem to fail.

@spease spease changed the title &str deserialization fails Numbers and &str deserialization fails Feb 17, 2018
@arcnmx
Copy link
Owner

arcnmx commented Feb 18, 2018

Could you provide an isolated test case I could compile/run that fails? Note that deserializing to &str is expected to fail because the current implementation does not support zero-copy deserialization/borrows, but workarounds to deserialize into numeric types like that should not be necessary.

See also the smoke test for an example usage that as far as I know does deserialize and serialize a u32 properly, that might give us something to compare against.

@spease
Copy link
Author

spease commented Feb 26, 2018

I don't have it easily accessible right now, but IIRC I was triyng to parse the output of wpa_supplicant's status command (sudo wpa_cli status on Linux if you're using wifi)

@arcnmx
Copy link
Owner

arcnmx commented Feb 26, 2018

By "it failed" are you getting a particular error? Not sure what's going on, what you have there should be equivalent to what's done internally... I'm not able to run that command, could you attach some example output?

@Larusso
Copy link

Larusso commented Nov 7, 2018

I ran into the same issue and can provide a simple test setup.

https://github.com/Larusso/serde-ini-fail-example/blob/master/src/lib.rs

This example is derived from some experimentation I do at the moment.
It creates three different struct layouts and tries to deserialize the same test data into it.

If you run the tests you will see that the test data_3 (which deserilized into Data3 struct) fails because of the number deserialization issue.

For a better visibility here are the three structs:

#[derive(Deserialize, Debug)]
#[serde(rename_all = "PascalCase")]
pub struct Data {
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_1: Option<Box<FieldData>>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_2: Option<Box<FieldData>>,
    #[serde(default, skip_serializing_if = "Option::is_none")]
    section_3: Option<Box<FieldData>>,
}

#[derive(Deserialize, Debug)]
#[serde(transparent)]
pub struct Data2(HashMap<Keys,FieldData>);

#[derive(Deserialize, Debug)]
#[serde(rename_all = "PascalCase")]
pub struct Data3 {
    section_1: FieldData,

    #[serde(flatten)]
    components: HashMap<Keys, FieldData>,
}

All three tests in this sample setup use the same struct: FieldData for the fields and the deserializer has no issues with it in the data_1 and data_2 tests.

I hope this helps you

@arcnmx
Copy link
Owner

arcnmx commented Nov 7, 2018

Hm, that looks like an issue with the new flatten attribute because it's treated by serde as if the struct has a map subfield, and subsections aren't supported by the deserializer.

Given that flatten support in serde looks newer than this issue, it's maybe not the same issue? Or perhaps they were trying to use subsections somehow.

@Larusso
Copy link

Larusso commented Nov 7, 2018

Hmm maybe. I am still somewhat new to Rust and the cargo ecosystem and have no idea what feature is new and which crate is older etc :)
Shall I open a new issue with some more version information etc?

@arcnmx
Copy link
Owner

arcnmx commented Nov 7, 2018

Hm yeah, no need to add more information but you can just use the "Open New Issue" button next to your comment. There's an issue with how flatten works that doesn't allow the automatic conversions to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants