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

Consider serializing map integer keys as strings #45

Closed
erickt opened this issue Feb 27, 2016 · 16 comments
Closed

Consider serializing map integer keys as strings #45

erickt opened this issue Feb 27, 2016 · 16 comments

Comments

@erickt
Copy link
Member

erickt commented Feb 27, 2016

Right now serde_json rejects serializing maps with integer keys because semantically speaking, JSON only supports maps with string keys. There are workarounds in serde 0.7 with the new #[serde(serialize_with="...", deserialize_with="...)] (see this gist), but it still can be annoying if this keeps causing problems.

Is there any real value about erroring out on non-key values?

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

I stumbled into the "key must be a string" error while using the newtype pattern (https://aturon.github.io/features/types/newtype.html).

pub struct Kind (pub String);
BTreeMap<Kind, Json>

Also I often use the small string optimization (around https://github.com/fitzgen/inlinable_string) to avoid the heap allocations. I gather Serde won't let me do BTreeMap<InlString, Json> because it thinks that InlString is not a string.

@oli-obk
Copy link
Member

oli-obk commented Jun 7, 2016

@ArtemGr it shouldn't be a problem, you just need to make sure that your Serialize impl calls serialize_str. InlString is probably serialized as a struct.

Edit: oh InlinableString doesn't implement Serialize at all... Well then you obviously can't use it. You can make a PR to the crate to conditionally add serde, or you can use the serialize_with attribute.

wrt newtypes, I'm not sure if it's a good idea to make the default serialization work in this case. You created a newtype out of a reason (type safety), so I'm not sure if serde should break that as the default. But I'm open to be convinced otherwise ;)

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

Oh, thanks for the hint @oli-obk. I thought the problem is the string check but it looks like it's the lack of newtype support.

I sure would like for Serde to support the newtype pattern, maybe with some kind of new tag.

@oli-obk
Copy link
Member

oli-obk commented Jun 7, 2016

serde does support newtypes. It's just a question of whether it would be a good default to serialize newtypes exactly like their inner type. Right now that's not the default.

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

Yeah, what I mean is the transparent newtype support.

When we write BTreeMap<String, u32> we add type information that can't be represented in JSON: that the value is an unsigned integer and only 32 bits long. That's all well and good, because Rust is a more statically typed language than JavaScript and because serializing the type information is costly and unnecessary.

But with newtype pattern it suddenly works differently, Serde wants to actually write down in JSON that struct Foo (String) is a Foo string, not just any string.

whether it would be a good default

I guess defaulting transparent newtype support would be a breaking change. It would be much easier to just stick a new tag on it.

@oli-obk
Copy link
Member

oli-obk commented Jun 7, 2016

It would be much easier to just stick a new tag on it.

What do you mean by that? You can already add #[serde(serialize_with(...))] and you can implement Serialize manually.

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

What do you mean by that?

This: "to serialize newtypes exactly like their inner type".

You can already add #[serde(serialize_with(...))] and you can implement Serialize manually.

That's what I'm trying to avoid.

@oli-obk
Copy link
Member

oli-obk commented Jun 7, 2016

I don't understand where you want to add a new tag to prevent a breaking change (since you obviously dislike serialize_with, I don't see any other places where you can add some annotation). Can you give a hypothetical code example of what you want?

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

#[derive(Serialize, Deserialize)]
#[serde(transparent_newtype)]
pub struct Foo (pub String)

#[derive(Serialize, Deserialize)]
#[serde(transparent_newtype)]
pub struct Bar (pub BTreeMap<Foo, i32>)

or

#[derive(Serialize, Deserialize)]
#[serde(transparent_newtype)]
pub struct Foo (pub InlString)

#[derive(Serialize, Deserialize)]
#[serde(transparent_newtype)]
pub struct Bar (pub BTreeMap<Foo, i32>)

Where InlString should be treated as a string because it implements

impl serde::Deserialize for InlString {
  fn deserialize<D: serde::Deserializer> (deserializer: &mut D) -> Result<InlString, D::Error> {
    struct InlStrVisitor; impl serde::de::Visitor for InlStrVisitor {type Value = InlString;
      fn visit_str<E: serde::de::Error> (&mut self, s: &str) -> Result<InlString, E> {
        Ok (InlString (InlinableString::from (s)))}}
    deserializer.deserialize (InlStrVisitor)}}
impl serde::Serialize for InlString {
  fn serialize<S: serde::Serializer> (&self, serializer: &mut S) -> Result<(), S::Error> {
    serializer.serialize_str (self.0.as_ref())}}

What doesn't work currently:

a) pub struct Bar (pub BTreeMap<Foo, i32>) puts the map inside a JSON array, e.g. "[{"foo": 123}]". For newtype pattern to work transparently it must serialize to inner type, "{"foo": 123}".

b) Using pub struct Foo (pub String) as the map key gives the "key must be a string" error.

@erickt
Copy link
Member Author

erickt commented Jun 7, 2016

I'd be fine with newtype structs being serialized without serializing the wrapper struct, that's why I added support for it. Frankly I thought I already implemented this for JSON. This would need a major version bump though since it'd change the serialization format.

@dtolnay
Copy link
Member

dtolnay commented Jun 7, 2016

a) pub struct Bar (pub BTreeMap<Foo, i32>) puts the map inside a JSON array, e.g. [{"foo": 123}]. For newtype pattern to work transparently it must serialize to inner type, {"foo": 123}.

That behavior would be very surprising to me and I was not able to reproduce it. Can you share your code? Here is mine.

#![feature(custom_derive, plugin)]
#![plugin(serde_macros)]

use std::collections::btree_map::BTreeMap;

extern crate serde_json;

pub type Foo = String;

#[derive(Serialize, Deserialize)]
pub struct Bar(pub BTreeMap<Foo, i32>);

fn main() {
    let mut b = Bar(BTreeMap::new());
    b.0.insert("foo".to_string(), 123);

    // prints {"foo":123}
    println!("{}", serde_json::to_string(&b).unwrap());
}

@dtolnay
Copy link
Member

dtolnay commented Jun 7, 2016

b) Using pub struct Foo (pub String) as the map key gives the "key must be a string" error.

Ditto Erick, I would have expected this to work already. This is a bug that we need to fix.

@ArtemGr
Copy link

ArtemGr commented Jun 7, 2016

That behavior would be very surprising to me and I was not able to reproduce it. Can you share your code?

Turns out it's kind of a corner case.

I'm not serializing to a String, instead I have an untyped JSON structure (a part of technical debt) that I work with as simply BTreeMap<String, Json> and I'm using typed structs for parts of this bigger untyped structure. In other words, I'm using from_value and to_value.

Here's some code:

use serde_json as json;
use serde_json::Value as Json;
use std::collections::BTreeMap;

#[derive(Serialize, Deserialize)]
pub struct Bar (pub BTreeMap<String, i32>);

fn main() {
  let mut map = BTreeMap::new();
  map.insert (String::from ("foo"), 123);
  let bar = Bar (map);
  println! ("json: {}", json::to_string (&bar) .unwrap());
  let mut outer: BTreeMap<String, Json> = BTreeMap::new();
  outer.insert (String::from ("bar"), json::to_value (&bar));
  println! ("outer: {}", json::to_string (&outer) .unwrap());

With Serde 0.7.0 it prints

json: {"foo":123}
outer: {"bar":[{"foo":123}]}

I'm glad to see that the first line printed is indeed a transparent newtype.

My use case is the second one though and here Serde seems to wrap the inner type in array.

@dtolnay
Copy link
Member

dtolnay commented Jun 7, 2016

Seems like a bug in Value. I filed #77 to look into it.

@dtolnay
Copy link
Member

dtolnay commented Dec 24, 2016

Another data point: Go allows integer keys:

package main

import (
	"fmt"
	"encoding/json"
)

func main() {
	m := map[int]string{1: "s"}
	j, err := json.Marshal(m)
	if err != nil {
		panic(err)
	}
	fmt.Println(string(j)) // {"1":"s"}
}

@arthurprs
Copy link

I think we can close this now 😃

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

No branches or pull requests

5 participants