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

Cannot deserialize with Struct without fields #461

Open
zhongzc opened this issue Sep 27, 2023 · 8 comments
Open

Cannot deserialize with Struct without fields #461

zhongzc opened this issue Sep 27, 2023 · 8 comments

Comments

@zhongzc
Copy link

zhongzc commented Sep 27, 2023

#[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
Copy link
Collaborator

How would you specify that in a configuration file?

@zhongzc
Copy link
Author

zhongzc commented Sep 27, 2023

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::<A>(&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
Copy link
Collaborator

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

@zhongzc
Copy link
Author

zhongzc commented Sep 28, 2023

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

[b]

@matthiasbeyer
Copy link
Collaborator

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

matthiasbeyer added a commit to matthiasbeyer/config-rs that referenced this issue Sep 28, 2023
Adds a test for an empty inner object as reported in mehcode#461.

Reported-by: Zhenchi <zhongzc_arch@outlook.com>
Signed-off-by: Matthias Beyer <mail@beyermatthias.de>
@matthiasbeyer
Copy link
Collaborator

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
Copy link
Author

zhongzc commented Sep 28, 2023

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
Copy link
Collaborator

polarathene commented Oct 20, 2023

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 #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 {} will fail with try_deserialize() due to error: value: missing field 'b'. Does not happen for struct B; (successful with struct).
  • toml::to_string(a) will fail for struct B; with: Error { inner: UnsupportedType(Some("B")) }. This also happens with the same error when wrapped in an Option<>.

struct B; would hit this route:

config-rs/src/ser.rs

Lines 165 to 182 in 6946069

fn serialize_none(self) -> Result<Self::Ok> {
self.serialize_unit()
}
fn serialize_some<T>(self, value: &T) -> Result<Self::Ok>
where
T: ?Sized + ser::Serialize,
{
value.serialize(self)
}
fn serialize_unit(self) -> Result<Self::Ok> {
self.serialize_primitive(Value::from(ValueKind::Nil))
}
fn serialize_unit_struct(self, _name: &'static str) -> Result<Self::Ok> {
self.serialize_unit()
}

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():

config-rs/src/ser.rs

Lines 243 to 249 in 6946069

fn serialize_map(self, _len: Option<usize>) -> Result<Self::SerializeMap> {
Ok(self)
}
fn serialize_struct(self, _name: &'static str, len: usize) -> Result<Self::SerializeStruct> {
self.serialize_map(Some(len))
}

  • The _name is the struct B, and len is 0.
  • The returned result for both may look like different types but they are type aliases to Self?:

config-rs/src/ser.rs

Lines 85 to 94 in 6946069

impl<'a> ser::Serializer for &'a mut ConfigSerializer {
type Ok = ();
type Error = ConfigError;
type SerializeSeq = Self;
type SerializeTuple = Self;
type SerializeTupleStruct = Self;
type SerializeTupleVariant = Self;
type SerializeMap = Self;
type SerializeStruct = Self;
type SerializeStructVariant = Self;

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()):

config-rs/src/ser.rs

Lines 15 to 22 in 6946069

impl ConfigSerializer {
fn serialize_primitive<T>(&mut self, value: T) -> Result<()>
where
T: Into<Value> + Display,
{
let key = match self.last_key_index_pair() {
Some((key, Some(index))) => format!("{}[{}]", key, index),
Some((key, None)) => key.to_string(),

config-rs/src/ser.rs

Lines 31 to 32 in 6946069

#[allow(deprecated)]
self.output.set(&key, value.into())?;

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):

config-rs/src/de.rs

Lines 132 to 142 in 6946069

#[inline]
fn deserialize_option<V>(self, visitor: V) -> Result<V::Value>
where
V: de::Visitor<'de>,
{
// Match an explicit nil as None and everything else as Some
match self.kind {
ValueKind::Nil => visitor.visit_none(),
_ => visitor.visit_some(self),
}
}


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()):

config-rs/src/de.rs

Lines 167 to 171 in 6946069

serde::forward_to_deserialize_any! {
char seq
bytes byte_buf map struct unit
identifier ignored_any unit_struct tuple_struct tuple
}

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 😅

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