ohadravid / wmi-rs

WMI crate for rust
Apache License 2.0
80 stars 27 forks source link

Enum support #90

Closed omer54463 closed 6 months ago

omer54463 commented 7 months ago

I would like to represent string fields in a WMI object as enums, but the following doesn't currently work:

#![allow(dead_code)]

#[derive(serde::Deserialize, Debug)]
enum Status {
    OK,
}

#[derive(serde::Deserialize, Debug)]
#[serde(rename = "Win32_OperatingSystem")]
#[serde(rename_all = "PascalCase")]
struct OperatingSystem {
    status: Status,
}

fn main() -> Result<(), anyhow::Error> {
    let com_con = wmi::COMLibrary::new()?;
    let wmi_con = wmi::WMIConnection::new(com_con.into())?;
    let results: Vec<OperatingSystem> = wmi_con.query()?;
    println!("{:#?}", &results[0]);
    Ok(())
}

It fails with the error:

Error: invalid type: string "OK", expected enum Status

Is this intended? I assume it isn't.

I tried to tackle the issue myself, but it seems I don't know serde good enough to do so.

omer54463 commented 7 months ago

After looking into it for a while, it seems that the problem is at Variant::deserialize_enum:

    fn deserialize_enum<V>(
        self,
        name: &'static str,
        fields: &'static [&'static str],
        visitor: V,
    ) -> Result<V::Value, Self::Error>
    where
        V: de::Visitor<'de>,
    {
        match self {
            Variant::Object(o) => {
                Deserializer::from_wbem_class_obj(o).deserialize_enum(name, fields, visitor)
            }
            _ => self.deserialize_any(visitor), // This line here
        }
    }

visitor is the __Visitor created by serde for my enum. self is a Variant::String, which means we get deserialize_any -> visitor.visit_string - but serde doesn't implement that for enums. Therefore, we should not relay the call to deserialize_any in the first place - we should relay to visit_enum.

Another evidence for that is the same piece of code at serde_json:

    fn deserialize_enum<V>(
        self,
        _name: &str,
        _variants: &'static [&'static str],
        visitor: V,
    ) -> Result<V::Value, Error>
    where
        V: Visitor<'de>,
    {
        let (variant, value) = match self {
            Value::Object(value) => {
                let mut iter = value.into_iter();
                let (variant, value) = match iter.next() {
                    Some(v) => v,
                    None => {
                        return Err(serde::de::Error::invalid_value(
                            Unexpected::Map,
                            &"map with a single key",
                        ));
                    }
                };
                // enums are encoded in json as maps with a single key:value pair
                if iter.next().is_some() {
                    return Err(serde::de::Error::invalid_value(
                        Unexpected::Map,
                        &"map with a single key",
                    ));
                }
                (variant, Some(value))
            }
            Value::String(variant) => (variant, None),
            other => {
                return Err(serde::de::Error::invalid_type(
                    other.unexpected(),
                    &"string or map",
                ));
            }
        };

        visitor.visit_enum(EnumDeserializer { variant, value }) // This line here
    }

This is implemented for their Value enum, which is equivalent to our Variant enum.

ohadravid commented 7 months ago

Hi @omer54463 ,

I think you are right - there should be an equivalent Variant::String to support this.

Based on your comments, I added an initial impl at #91, but I want to add another test with more enum options (as well as failed-to-desr test).

I might have time to continue next weekend, but if you want to work on it, I'll be happy to accept a more complete PR 🥳

omer54463 commented 6 months ago

Just did on #92 I admit that I could do better on that desr test in terms of literally finding WMI fields with different values, but I think the current test achieves it's goal.