mehcode / config-rs

⚙️ Layered configuration system for Rust applications (with strong support for 12-factor applications).
Apache License 2.0
2.57k stars 213 forks source link

Cannot deserialize with Struct without fields #461

Open zhongzc opened 1 year ago

zhongzc commented 1 year ago
#[derive(Debug, Serialize, Deserialize)]
struct A {
    b: Option<B>,
}
#[derive(Debug, Serialize, Deserialize)]
struct B {}

let a = A { b: Some(B {}) };
let de: A = Config::try_from(&a).unwrap().try_deserialize().unwrap();
assert!(de.b.is_some()); // Failed
matthiasbeyer commented 1 year ago

How would you specify that in a configuration file?

zhongzc commented 1 year ago

How would you specify that in a configuration file?


#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct A {
b: Option<B>,
}
#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct B {}

impl Default for A { fn default() -> Self { Self { b: Some(B {}) } } }

let a = A::default(); let toml_str = toml::to_string(&a).unwrap(); println!("toml str: {}", toml_str);

// let de_from_toml = toml::from_str::(&toml_str).unwrap(); // assert_eq!(a, de_from_toml); // Passed

let de_from_toml: A = Config::builder() .add_source(File::from_str(&toml_str, FileFormat::Toml)) .build() .unwrap() .try_deserialize() .unwrap(); assert_eq!(a, de_from_toml); // Passed

let de_from_default_object: A = Config::builder() .add_source(Config::try_from(&a).unwrap()) .build() .unwrap() .try_deserialize() .unwrap(); assert_eq!(a, de_from_default_object); // Failed


Output:

toml str: [b]

assertion failed: (left == right) left: A { b: Some(B) }, right: A { b: None }

matthiasbeyer commented 1 year ago

And again I ask: How would you specify a instance of A { b: Some(B) } in a configuration file?

zhongzc commented 1 year ago

Just like the previous example, for toml file, I specify it like this:

[b]
matthiasbeyer commented 1 year ago

Ah, I missed that bit. Hnm, indeed that's an issue. Thanks for reporting!

matthiasbeyer commented 1 year ago

I opened #463 with a testcase for this issue. If you have an idea solving that problem, you're of course very much welcome!

zhongzc commented 1 year ago

The scenarios that cause B to be lost are related to iterators. An example that better reflects the situation I found:

#[derive(Debug, Serialize, Deserialize, PartialEq)]
struct A {
    b_option: Option<B>,
    b_vec: Vec<B>,
    b_set: HashSet<B>,
    b_map: HashMap<String, B>,
}
#[derive(Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
struct B {}

let a = A {
    b_option: Some(B {}),
    b_vec: vec![B {}],
    b_set: HashSet::from([B {}]),
    b_map: HashMap::from([("".to_owned(), B {})]),
};

// Serialize that `a` with `ConfigSerializer` will produce an empty config
let config = Config::try_from(&a).unwrap();
polarathene commented 11 months ago

TL;DR: Looks like this can be resolved easily enough with the below bolded fixes.

NOTE: Depending on when this is acted on, the referenced snippets below may change due to https://github.com/mehcode/config-rs/pull/465 (suggested fixes remain compatible, other referenced snippets may be refactored)


Note that there is two ways to define a unit-like struct (technically more?):

// Officially documented way:
// https://doc.rust-lang.org/book/ch05-01-defining-structs.html#unit-like-structs-without-any-fields
// https://learning-rust.github.io/docs/structs/
struct B;

// RFC:
// https://rust-lang.github.io/rfcs/0218-empty-struct-with-braces.html
// Previously not supported: https://github.com/rust-lang/rfcs/pull/147#issuecomment-47589168
// Accepted 2015: https://github.com/rust-lang/rfcs/pull/218#issuecomment-91558974
// Stabilized 2016: https://github.com/rust-lang/rust/issues/29720#issuecomment-189523437
struct B {}

These are roughly the same but have some differences.

If you omit the Option<>, and always have the unit struct:


struct B; would hit this route:

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L165-L182

Even though it does reach serialize_some() you can see that it takes the same path as serialize_none(), as value.serialize(self) will resolve to the serialize_unit_struct() method, which like serialize_none() redirects to serialize_unit() which normalizes to the Nil type, aka None.

Fix: Instead of self.serialize_unit(), a unit struct is more likely to map to a Table/Map like other structs. So an empty struct seems more appropriate?:

// Call this instead:
self.serialize_primitive(Value::from(crate::value::Table::new()))

EDIT: This fails to deserialize the unit struct properly with error:

It requires adding an additional deserialize method for Value in de.rs instead of relying on unit_struct in serde::forward_to_deserialize_any!:

#[inline]
fn deserialize_unit_struct<V: de::Visitor<'de>>(self, _name: &'static str, visitor: V) -> Result<V::Value> {
    visitor.visit_unit()
}

struct B {} instead hits the same serialize_some() route to start, but value.serialize(self) recognizes it as a struct thus calls serialize_struct():

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L243-L249

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L85-L94

This is also None presumably because there is no logic involved that serializes to the config-rs internal Value type (_which serialize_unit() did for struct B; earlier by calling serialize_primitive()_):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L15-L22

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/ser.rs#L31-L32

Fix: A similar fix here is to also create a table, presumably only when len is 0. This should not replace the existing call, due to conflicting return type. Modify serialize_struct():

// Struct is empty (unit struct with empty braces):
if len == 0 {
    self.serialize_primitive(Value::from(crate::value::Table::new()))?;
}
// Keep the original call:
self.serialize_map(Some(len))

In both cases, I assume the expected serialization is what I've observed the deserialized format to Value is, an empty variant of Table (Map<String, Value>).

Applying either fix for that as suggested above now serializes the unit struct type option successfully, which will properly deserialize the option (since ValueKind::Nil would resolve to None but any other ValueKind variant like Table resolves to Some):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/de.rs#L132-L142


Deserialization isn't an issue without the Option wrapping type, since Nil seems to resolve implicitly via the unit_struct -> unit derived methods generated here (_deserialize_unit_struct() that calls self.deserialize_unit(visitor) that calls visitor.visit_unit()_):

https://github.com/mehcode/config-rs/blob/6946069755e4bc75af9b7ca678da66c055a0af16/src/de.rs#L167-L171

The value prior to calling visitor.visit_unit() is Value of ValueKind::Nil:

Value {
    origin: None,
    kind: Nil,
}

I assume directly after this call it's converted to the unit struct, but I'm not sure how that works 😅