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

auto convert problem with serde flatten #6

Open
Larusso opened this issue Nov 7, 2018 · 2 comments
Open

auto convert problem with serde flatten #6

Larusso opened this issue Nov 7, 2018 · 2 comments

Comments

@Larusso
Copy link

Larusso commented Nov 7, 2018

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

Originally posted by @Larusso in #5 (comment)

@arcnmx
Copy link
Owner

arcnmx commented Nov 7, 2018

Okay so, the issue here is that the ini data is untyped, all values are strings. This deserializer tries to use FromStr when needed as a convenience, so that deserialize_with isn't necessary to use for anything that's not a string type. However, serde's flatten collects all extra fields until the end of deserializing (as typed strings), then uses its own Deserializer to convert that to the HashMap<Keys, FieldData> which does not use the same conversion magic - this results in the inconsistency.

Perhaps @dtolnay could chime in on how best to approach this. Removing the conversions and requiring deserialize_with="from_str" on everything would be more consistent and explicit. This seems like an annoying and unnecessary burden for the majority of use cases, but perhaps the conversions are too magic to be something a deserializer should be doing?

@dtolnay
Copy link

dtolnay commented Nov 8, 2018

This is the same underlying limitation as serde-rs/serde#1183 and serde-rs/json#497. We will be able to handle this better in Serde once associated type defaults are available (rust-lang/rust#29661).

For now I would recommend keeping the type conversions in serde_ini but using a #[serde(with = "...")] attribute on non-string fields that get flattened, in this case size. The implementation in serde_with::rust::display_fromstr should do the trick.

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