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

Enum support #90

Closed
omer54463 opened this issue Feb 10, 2024 · 3 comments · Fixed by #91
Closed

Enum support #90

omer54463 opened this issue Feb 10, 2024 · 3 comments · Fixed by #91

Comments

@omer54463
Copy link
Contributor

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

omer54463 commented Feb 10, 2024

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

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

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.

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

Successfully merging a pull request may close this issue.

2 participants