Map serde support

[CyberHoward]

Attempt to add serde support to map types.
Changes based on serde-json-core

closes #41

[webmaster128]

Thank you!

Could you add serialization tests for maps that show the JSON output directly? Like those tests: https://github.com/CosmWasm/serde-json-wasm/blob/v0.4.1/src/ser/mod.rs#L877-L980

[webmaster128]

Beautiful. Diff LGTM. Could you add a CHANGELOG entry to the unreleased section above 0.4.1 for this?

[ethanfrey]

Thank you for your contribution and it looks promising.

I still see a number of unreachable!() (which should really be todo!(), right?) and I would love a few more tests to show what cases it can handle.

[ethanfrey]

Okay, so this test will show parse(serialize(x)) == x right?

And does work with u16 as tree value, which is promising.

Maybe other pieces already work. Can you add test cases for other types?

Also, this only works when the map has all the same type of value, right? I guess we need some more work for flatten, but we can leave that for another PR.

[webmaster128]

I pushed a HashMap test here. This shows it works and has non-deterministic ordering (as expected).

[webmaster128]

I tested serializing non string keys with

        // non-string keys
        let mut map = BTreeMap::new();
        map.insert(123, "abc");
        assert_eq!(to_string(&map).unwrap(), r#"{123:"abc"}"#);

Turns out this works in the current implementation, but is wrong since JSON can only accept string keys. So we should restrict it to keys that are string or char.

serde_json handles this by creating a MapKeySerializer that only accepts string types as keys: https://github.com/serde-rs/json/blob/993e7a6eeaa89b39329d245e63879d7913ed1a41/src/ser.rs#L774-L1103. It is a wrapper around ser: &'a mut Serializer<W, F>, that either fails or passes the serialization to ser. It is constructed temporarily in serialize_key: key.serialize(MapKeySerializer { ser: *ser }).

I think we should have this too to ensure users do not accidentally produce invalid JSON.

[hashedone]

LGTM, like tests - seems to be working. The HashMap tests are not very interesting from the CW POV (because random state hashing not being useful in CW I think), but may be helpful in another non-fp application.

[webmaster128]

The HashMap tests are not very interesting from the CW POV (because random state hashing not being useful in CW I think)

Actually in Wasm the random state hashing is disabled. The point here is that the serialization is as deterministic or undeterministic as the map's iteration order. The serializer DOES NOT sort keys.

[webmaster128]

@CyberHoward do you want to work on the MapKeySerializer issue brought up in #45 (review)? Or should we merge here and I do it myself in a separate PR? Both ways are fine with me.

[hashedone]

Actually in Wasm the random state hashing is disabled

Exactly that was what I meant by that, but I haven't this tweet anywhere in my bookmarks, so I didn't want to make too strong claims not to be wrong.

[CyberHoward]

@CyberHoward do you want to work on the MapKeySerializer issue brought up in #45 (review)? Or should we merge here and I do it myself in a separate PR? Both ways are fine with me.

Sure, I'll try to do it!

[CyberHoward]

The implementation in serde_json parses number types (i32, u64, etc.) into string types through a Formatter while the current implementation refuses numbers as keys. Is this the behavior we want or should I add parsing logic to serialize them into strings?

Also FYI new type Structs and unit Enum variants are considered valid keys. See the added tests.

[webmaster128]

Great stuff. I'd go with best possible serde_json compatibility, such that map created with our lib can be decoded using serde_json and vice versa. But we only need to support a subset of serde_json. Serializing the number types to string does no harm IMO and might be useful for some users.

[webmaster128]

Great stuff.

I'd like to spend an hour this week to add a bunch of additional tests but other than that, this looks complete to me.

[CyberHoward]

Deserialization into number key types is missing so this test fails:

#[test]
    fn numbered_keys() {
        let mut ranking: BTreeMap<u64, String> = BTreeMap::new();
        ranking.insert(1, "Elon".to_string());
        ranking.insert(2, "Bazos".to_string());

        assert_eq!(
            from_str::<BTreeMap<u64, String>>(&to_string(&ranking).unwrap()).unwrap(),
            ranking
        );
    }

Also the Deserializer trait of serde does not support 128-bit values so maybe we should not allow serialization of those values?

[webmaster128]

Deserialization into number key types is missing so this test fails:

Could you add that too?

Also the Deserializer trait of serde does not support 128-bit values so maybe we should not allow serialization of those values?

This is just the default implementation which deserializer implementations then override. It is added there for compatibility reasons because the first version of the trait did not have 128 bit. I think we can just implement it and then test deserializing with serde_json in a test.

[CyberHoward]

I think the sanity check is failing due to the outdated Rust version.

[webmaster128]

Right, see #47. Thanks for the hint.

[ethanfrey]

Nice updates @CyberHoward !

[webmaster128]

I just added a bunch of additional tests. This looks great.

Will merge manually now to resolve the conflicts. Thanks a lot for the contribution!