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

Deserializing a map with integer keys doesn't work when wrapped in an enum #560

Closed
chanks opened this issue Aug 19, 2019 · 5 comments
Closed

Comments

@chanks
Copy link

chanks commented Aug 19, 2019

Not sure what's causing this behavior, but I have a minimal reproduction:

use serde::{Deserialize, Serialize};
use serde_json;
use std::collections::BTreeMap;

#[derive(Serialize, Deserialize)]
#[serde(tag = "_")]
pub enum ItemTypes {
    #[serde(rename = "r")]
    Resource(Resource),
}

#[derive(Serialize, Deserialize)]
pub struct Resource {
    #[serde(rename = "s")]
    pub strings: BTreeMap<i32, String>,
}

fn main() {
    // This works:
    let resource_json_string = r#"{"s":{"2":"a","3":"b"}}"#;
    let _: Resource = serde_json::from_str(resource_json_string).unwrap();
    println!("Resource deserialized!");

    // But wrap the struct in an enum and it fails:
    let items_json_string = r#"{"_":"r","s":{"2":"a","3":"b"}}"#;
    let _: ItemTypes = serde_json::from_str(items_json_string).unwrap();
    
    // Error("invalid type: string \"2\", expected i32", line: 0, column: 0)
}

Try it out at: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=dbf570c164d60ccf734c375e05b79aa0

@tyranron
Copy link

tyranron commented Sep 13, 2019

ping @dtolnay
Would you be so kind at least to point at the place which possible causes the problem?

We're hitting this in our codebase to and this makes us to omit using HashMap in our API, which is quite undesirable. So, we can try to fix it at least.

cc @evdokimovs

@dtolnay
Copy link
Member

dtolnay commented Sep 16, 2019

This is a duplicate of serde-rs/serde#1183. I'll close so that we keep the discussion in one place.

We will be better positioned to fix this after there's been progress on associated type defaults (rust-lang/rust#29661). Without that, this is an extremely complicated issue.

@dtolnay dtolnay closed this as completed Sep 16, 2019
@chanks
Copy link
Author

chanks commented Sep 17, 2019

Thanks @dtolnay! In the meantime, does anyone have a workaround for this issue? I'm experimenting with providing a deserialize_with option for the map field, but I'm struggling with how to implement the function it'll point to. It'd be nice if I could get serde to handle deserializing to BTreeMap<String, String>, for example, and then it'd be simple enough for me to convert that to a BTreeMap<i32, String> myself.

@dtolnay
Copy link
Member

dtolnay commented Sep 17, 2019

Your deserialize_with function can directly call BTreeMap<String, String>'s Deserialize impl.

use serde::{de, Deserialize, Deserializer};
use std::collections::HashMap;
use std::fmt::Display;
use std::hash::Hash;
use std::str::FromStr;

fn de_int_key<'de, D, K, V>(deserializer: D) -> Result<HashMap<K, V>, D::Error>
where
    D: Deserializer<'de>,
    K: Eq + Hash + FromStr,
    K::Err: Display,
    V: Deserialize<'de>,
{
    let string_map = <HashMap<String, V>>::deserialize(deserializer)?;
    let mut map = HashMap::with_capacity(string_map.len());
    for (s, v) in string_map {
        let k = K::from_str(&s).map_err(de::Error::custom)?;
        map.insert(k, v);
    }
    Ok(map)
}

If you need to optimize for performance or memory usage, the following will likely do better by not allocating the string keys or the string to string map.

use serde::de::{self, Deserialize, DeserializeSeed, Deserializer, MapAccess, Visitor};
use std::collections::HashMap;
use std::fmt::{self, Display};
use std::hash::Hash;
use std::marker::PhantomData;
use std::str::FromStr;

fn de_int_key<'de, D, K, V>(deserializer: D) -> Result<HashMap<K, V>, D::Error>
where
    D: Deserializer<'de>,
    K: Eq + Hash + FromStr,
    K::Err: Display,
    V: Deserialize<'de>,
{
    struct KeySeed<K> {
        k: PhantomData<K>,
    }

    impl<'de, K> DeserializeSeed<'de> for KeySeed<K>
    where
        K: FromStr,
        K::Err: Display,
    {
        type Value = K;

        fn deserialize<D>(self, deserializer: D) -> Result<Self::Value, D::Error>
        where
            D: Deserializer<'de>,
        {
            deserializer.deserialize_str(self)
        }
    }

    impl<'de, K> Visitor<'de> for KeySeed<K>
    where
        K: FromStr,
        K::Err: Display,
    {
        type Value = K;

        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
            formatter.write_str("a string")
        }

        fn visit_str<E>(self, string: &str) -> Result<Self::Value, E>
        where
            E: de::Error,
        {
            K::from_str(string).map_err(de::Error::custom)
        }
    }

    struct MapVisitor<K, V> {
        k: PhantomData<K>,
        v: PhantomData<V>,
    }

    impl<'de, K, V> Visitor<'de> for MapVisitor<K, V>
    where
        K: Eq + Hash + FromStr,
        K::Err: Display,
        V: Deserialize<'de>,
    {
        type Value = HashMap<K, V>;

        fn expecting(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
            formatter.write_str("a map")
        }

        fn visit_map<A>(self, mut input: A) -> Result<Self::Value, A::Error>
        where
            A: MapAccess<'de>,
        {
            let mut map = HashMap::new();
            while let Some((k, v)) =
                input.next_entry_seed(KeySeed { k: PhantomData }, PhantomData)?
            {
                map.insert(k, v);
            }
            Ok(map)
        }
    }

    deserializer.deserialize_map(MapVisitor {
        k: PhantomData,
        v: PhantomData,
    })
}

@chanks
Copy link
Author

chanks commented Sep 17, 2019

Wow, that's super helpful @dtolnay, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants