Repository: pyfisch/cbor
Pull request #67 (2018-06-07 09:36:56)

Adds the option to encode enums as 1-key maps.
Since enums are part of the Rust language but not part of the CBOR or JSON spec, each serializer has to make a choice how to encode them. 

serde_cbor chose `["Variant", items..]` while serde_json chose `{"Variant": item}` or `{"Variant": [items..]}`. Although this is immaterial when passing data between Rust programs, it becomes visible if `Values` are used, or when data is passed between Rust and other languages.

This turned out to be a problem for me when I tried to migrate a communicating pair of Python and Rust programs from JSON to CBOR, as assumptions about the encoding of enums had already become buried throughout the Python code.

This pull request adds the option (default false) to encode enums in the serde_json way and adds support for automatically decoding enums from either encoding.


pyfisch: Sorry. I missed this. I will review this week.
pyfisch: I think if we add this the serialization should match the one used by serde-json.
ogoodman: Tests added. I've also added a `to_vec` function to `SerializerOptions` and implemented `Default` on the latter. That makes it possible for people to code in such a way that adding new options won't be a breaking change (although that requires that they add `..Default::default()`). The alternative is having private fields and a bunch of builder methods, but I don't love that either.
kostko: @ogoodman thanks for taking the time to update this, I just got back from vacation and didn't have time to push my changes before. I did a cursory look over the new changes and it seems ok.

Regarding option defaults, either the `Default` way or the builder seem fine to me with a slight preference for `Default` due to requiring less code, especially for these simple flags that are currently implemented.
ogoodman: @kostko thanks for your comments. Sorry if I caused you to waste time working on this. I didn't think I'd have time to work on it for a while, but then I unexpectedly did.

Patch:
diff --git a/src/de.rs b/src/de.rs
index f96e7ca..044b39a 100644
--- a/src/de.rs
+++ b/src/de.rs
@@ -383,6 +383,24 @@ where
         })
     }
 
+    fn parse_enum_map<V>(&mut self, visitor: V) -> Result<V::Value>
+    where
+        V: de::Visitor<'de>,
+    {
+        self.recursion_checked(|de| {
+            let mut len = 1;
+            let value = visitor.visit_enum(VariantAccessMap {
+                map: MapAccess { de, len: &mut len },
+            })?;
+
+            if len != 0 {
+                Err(de.error(ErrorCode::TrailingData))
+            } else {
+                Ok(value)
+            }
+        })
+    }
+
     fn parse_indefinite_enum<V>(&mut self, visitor: V) -> Result<V::Value>
     where
         V: de::Visitor<'de>,
@@ -693,6 +711,10 @@ where
                     _ => unreachable!(),
                 }
             }
+            Some(0xa1) => {
+                self.consume();
+                self.parse_enum_map(visitor)
+            }
             None => Err(self.error(ErrorCode::EofWhileParsingValue)),
             _ => visitor.visit_enum(UnitVariantAccess { de: self }),
         }
@@ -822,6 +844,15 @@ where
     }
 }
 
+impl<'de, 'a, R> MakeError for MapAccess<'a, R>
+where
+    R: Read<'de>,
+{
+    fn error(&self, code: ErrorCode) -> Error {
+        self.de.error(code)
+    }
+}
+
 struct IndefiniteMapAccess<'a, R: 'a> {
     de: &'a mut Deserializer<R>,
 }
@@ -1049,3 +1080,65 @@ where
         }
     }
 }
+
+struct VariantAccessMap<T> {
+    map: T,
+}
+
+impl<'de, T> de::EnumAccess<'de> for VariantAccessMap<T>
+where
+    T: de::MapAccess<'de, Error = Error> + MakeError,
+{
+    type Error = Error;
+    type Variant = VariantAccessMap<T>;
+
+    fn variant_seed<V>(mut self, seed: V) -> Result<(V::Value, VariantAccessMap<T>)>
+    where
+        V: de::DeserializeSeed<'de>,
+    {
+        let variant = match self.map.next_key_seed(seed) {
+            Ok(Some(variant)) => variant,
+            Ok(None) => return Err(self.map.error(ErrorCode::ArrayTooShort)),
+            Err(e) => return Err(e),
+        };
+        Ok((variant, self))
+    }
+}
+
+impl<'de, T> de::VariantAccess<'de> for VariantAccessMap<T>
+where
+    T: de::MapAccess<'de, Error = Error>
+        + MakeError,
+{
+    type Error = Error;
+
+    fn unit_variant(mut self) -> Result<()> {
+        match self.map.next_value() {
+            Ok(()) => Ok(()),
+            Err(e) => Err(e),
+        }
+    }
+
+    fn newtype_variant_seed<S>(mut self, seed: S) -> Result<S::Value>
+    where
+        S: de::DeserializeSeed<'de>,
+    {
+        self.map.next_value_seed(seed)
+    }
+
+    fn tuple_variant<V>(mut self, _len: usize, visitor: V) -> Result<V::Value>
+    where
+        V: de::Visitor<'de>,
+    {
+        let seed = StructVariantSeed { visitor };
+        self.map.next_value_seed(seed)
+    }
+
+    fn struct_variant<V>(mut self, _fields: &'static [&'static str], visitor: V) -> Result<V::Value>
+    where
+        V: de::Visitor<'de>,
+    {
+        let seed = StructVariantSeed { visitor };
+        self.map.next_value_seed(seed)
+    }
+}
diff --git a/src/lib.rs b/src/lib.rs
index 837af77..88d612b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -162,6 +162,6 @@ pub mod value;
 #[doc(inline)]
 pub use de::{from_slice, from_reader, Deserializer, StreamDeserializer};
 #[doc(inline)]
-pub use ser::{to_writer, to_vec, Serializer};
+pub use ser::{to_writer, to_vec, to_vec_with_options, Serializer, SerializerOptions};
 #[doc(inline)]
 pub use value::{Value, ObjectKey, to_value, from_value};
diff --git a/src/ser.rs b/src/ser.rs
index 3329675..57b9a95 100644
--- a/src/ser.rs
+++ b/src/ser.rs
@@ -97,10 +97,83 @@ where
     Ok(vec)
 }
 
+/// Serializes a value to a vector.
+pub fn to_vec_with_options<T>(value: &T, options: &SerializerOptions) -> Result<Vec<u8>>
+where
+    T: ser::Serialize,
+{
+    let mut vec = Vec::new();
+    {
+        let mut ser = Serializer::new_with_options(&mut vec, options);
+        if options.self_describe {
+            ser.self_describe()?;
+        }
+        value.serialize(&mut ser)?;
+    }
+    Ok(vec)
+}
+
+/// Options for a CBOR serializer.
+///
+/// The `enum_as_map` option determines how enums are encoded.
+///
+/// This makes no difference when encoding and decoding enums using
+/// this crate, but it shows up when decoding to a `Value` or decoding
+/// in other languages.
+///
+/// With enum_as_map true, the encoding scheme matches the default encoding
+/// scheme used by `serde_json`.
+///
+/// # Examples
+///
+/// Given the following enum
+/// ```
+/// enum Enum {
+///     Unit,
+///     NewType(i32),
+///     Tuple(String, bool),
+///     Struct{ x: i32, y: i32 },
+/// }
+/// ```
+/// we will give the `Value` with the same encoding for each case using
+/// JSON notation.
+///
+/// ## Default encodings
+///
+/// * `Enum::Unit` encodes as `"Unit"`
+/// * `Enum::NewType(10)` encodes as `["NewType", 10]`
+/// * `Enum::Tuple("x", true)` encodes as `["Tuple", "x", true]`
+/// * `Enum::Struct{ x: 5, y: -5 }` encodes as `["Struct", {"x": 5, "y": -5}]`
+///
+/// ## Encodings with enum_as_map true
+///
+/// * `Enum::Unit` encodes as `"Unit"`
+/// * `Enum::NewType(10)` encodes as `{"NewType": 10}`
+/// * `Enum::Tuple("x", true)` encodes as `{"Tuple": ["x", true]}`
+/// * `Enum::Struct{ x: 5, y: -5 }` encodes as `{"Struct": {"x": 5, "y": -5}}`
+#[derive(Default)]
+pub struct SerializerOptions {
+    /// When set, struct fields and enum variants are identified by their numeric indices rather than names
+    /// to save space.
+    pub packed: bool,
+    /// When set, enums are encoded as maps rather than arrays.
+    pub enum_as_map: bool,
+    /// When set, `to_vec` will prepend the CBOR self-describe tag.
+    pub self_describe: bool,
+}
+
+impl SerializerOptions {
+    /// Serializes a value to a vector.
+    pub fn to_vec<T: ser::Serialize>(&self, value: &T) -> Result<Vec<u8>> {
+        to_vec_with_options(value, self)
+    }
+}
+
 /// A structure for serializing Rust values to CBOR.
 pub struct Serializer<W> {
     writer: W,
     packed: bool,
+    enum_as_map: bool,
 }
 
 impl<W> Serializer<W>
@@ -113,6 +186,7 @@ where
         Serializer {
             writer,
             packed: false,
+            enum_as_map: false,
         }
     }
 
@@ -125,6 +199,17 @@ where
         Serializer {
             writer,
             packed: true,
+            enum_as_map: false,
+        }
+    }
+
+    /// Creates a new CBOR serializer with the specified options.
+    #[inline]
+    pub fn new_with_options(writer: W, options: &SerializerOptions) -> Serializer<W> {
+        Serializer {
+            writer,
+            packed: options.packed,
+            enum_as_map: options.enum_as_map,
         }
     }
 
@@ -133,6 +218,7 @@ where
         let mut s = Serializer {
             writer: buf,
             packed: self.packed,
+            enum_as_map: self.enum_as_map,
         };
         v.serialize(&mut s)?;
         Ok(s.writer)
@@ -406,8 +492,13 @@ where
     where
         T: ?Sized + ser::Serialize,
     {
-        self.writer.write_all(&[4 << 5 | 2]).map_err(Error::io)?;
-        self.serialize_unit_variant(name, variant_index, variant)?;
+        if self.enum_as_map {
+            self.write_u64(5, 1u64)?;
+            variant.serialize(&mut *self)?;
+        } else {
+            self.writer.write_all(&[4 << 5 | 2]).map_err(Error::io)?;
+            self.serialize_unit_variant(name, variant_index, variant)?;
+        }
         value.serialize(self)
     }
 
@@ -439,9 +530,15 @@ where
         variant: &'static str,
         len: usize,
     ) -> Result<&'a mut Serializer<W>> {
-        self.write_u64(4, (len + 1) as u64)?;
-        self.serialize_unit_variant(name, variant_index, variant)?;
-        Ok(self)
+        if self.enum_as_map {
+            self.write_u64(5, 1u64)?;
+            variant.serialize(&mut *self)?;
+            self.serialize_tuple(len)
+        } else {
+            self.write_u64(4, (len + 1) as u64)?;
+            self.serialize_unit_variant(name, variant_index, variant)?;
+            Ok(self)
+        }
     }
 
     #[inline]
@@ -492,7 +589,11 @@ where
         variant: &'static str,
         len: usize,
     ) -> Result<StructSerializer<'a, W>> {
-        self.writer.write_all(&[4 << 5 | 2]).map_err(Error::io)?;
+        if self.enum_as_map {
+            self.write_u64(5, 1u64)?;
+        } else {
+            self.writer.write_all(&[4 << 5 | 2]).map_err(Error::io)?;
+        }
         self.serialize_unit_variant(name, variant_index, variant)?;
         self.serialize_struct(name, len)
     }


Test patch:
diff --git a/tests/enum.rs b/tests/enum.rs
index 52eff8f..8bd6540 100644
--- a/tests/enum.rs
+++ b/tests/enum.rs
@@ -3,7 +3,9 @@ extern crate serde_cbor;
 #[macro_use]
 extern crate serde_derive;
 
-use serde_cbor::{from_slice, to_vec};
+use std::collections::BTreeMap;
+
+use serde_cbor::{from_slice, to_vec, Value, ObjectKey};
 
 #[derive(Debug,Serialize,Deserialize,PartialEq,Eq)]
 enum Enum {
@@ -96,3 +98,86 @@ fn test_variable_length_array() {
     let value: Vec<Foo> = from_slice(slice).unwrap();
     assert_eq!(value, [Foo::Require]);
 }
+
+#[derive(Serialize, Deserialize, PartialEq, Debug)]
+enum Bar {
+    Empty,
+    Number(i32),
+    Flag(String, bool),
+    Point{x: i32, y: i32},
+}
+
+#[test]
+fn test_enum_as_map() {
+    // unit variants serialize like bare strings
+    let empty_s = to_vec(&Bar::Empty).unwrap();
+    let empty_str_s = to_vec(&"Empty").unwrap();
+    assert_eq!(empty_s, empty_str_s);
+
+    // tuple-variants serialize like ["<variant>", values..]
+    let number_s = to_vec(&Bar::Number(42)).unwrap();
+    let number_vec = vec![Value::String("Number".to_string()), Value::I64(42)];
+    let number_vec_s = to_vec(&number_vec).unwrap();
+    assert_eq!(number_s, number_vec_s);
+
+    let flag_s = to_vec(&Bar::Flag("foo".to_string(), true)).unwrap();
+    let flag_vec = vec![Value::String("Flag".to_string()), Value::String("foo".to_string()), Value::Bool(true)];
+    let flag_vec_s = to_vec(&flag_vec).unwrap();
+    assert_eq!(flag_s, flag_vec_s);
+
+    // struct-variants serialize like ["<variant>", {struct..}]
+    let point_s = to_vec(&Bar::Point{ x: 5, y: -5}).unwrap();
+    let mut struct_map = BTreeMap::new();
+    struct_map.insert(ObjectKey::String("x".to_string()), Value::I64(5));
+    struct_map.insert(ObjectKey::String("y".to_string()), Value::I64(-5));
+    let point_vec = vec![Value::String("Point".to_string()), Value::Object(struct_map.clone())];
+    let point_vec_s = to_vec(&point_vec).unwrap();
+    assert_eq!(point_s, point_vec_s);
+
+    // enum_as_map matches serde_json's default serialization for enums.
+    let opts = serde_cbor::SerializerOptions{ enum_as_map: true, ..Default::default() };
+
+    // unit variants still serialize like bare strings
+    let empty_s = opts.to_vec(&Bar::Empty).unwrap();
+    assert_eq!(empty_s, empty_str_s);
+
+    // 1-element tuple variants serialize like {"<variant>": value}
+    let number_s = opts.to_vec(&Bar::Number(42)).unwrap();
+    let mut number_map = BTreeMap::new();
+    number_map.insert("Number", 42);
+    let number_map_s = to_vec(&number_map).unwrap();
+    assert_eq!(number_s, number_map_s);
+
+    // multi-element tuple variants serialize like {"<variant>": [values..]}
+    let flag_s = opts.to_vec(&Bar::Flag("foo".to_string(), true)).unwrap();
+    let mut flag_map = BTreeMap::new();
+    flag_map.insert("Flag", vec![Value::String("foo".to_string()), Value::Bool(true)]);
+    let flag_map_s = to_vec(&flag_map).unwrap();
+    assert_eq!(flag_s, flag_map_s);
+
+    // struct-variants serialize like {"<variant>", {struct..}}
+    let point_s = opts.to_vec(&Bar::Point{ x: 5, y: -5}).unwrap();
+    let mut point_map = BTreeMap::new();
+    point_map.insert("Point", Value::Object(struct_map));
+    let point_map_s = to_vec(&point_map).unwrap();
+    assert_eq!(point_s, point_map_s);
+
+    // deserialization of all encodings should just work
+    let empty_str_ds = from_slice(&empty_str_s).unwrap();
+    assert_eq!(Bar::Empty, empty_str_ds);
+
+    let number_vec_ds = from_slice(&number_vec_s).unwrap();
+    assert_eq!(Bar::Number(42), number_vec_ds);
+    let number_map_ds = from_slice(&number_map_s).unwrap();
+    assert_eq!(Bar::Number(42), number_map_ds);
+
+    let flag_vec_ds = from_slice(&flag_vec_s).unwrap();
+    assert_eq!(Bar::Flag("foo".to_string(), true), flag_vec_ds);
+    let flag_map_ds = from_slice(&flag_map_s).unwrap();
+    assert_eq!(Bar::Flag("foo".to_string(), true), flag_map_ds);
+
+    let point_vec_ds = from_slice(&point_vec_s).unwrap();
+    assert_eq!(Bar::Point{ x: 5, y: -5}, point_vec_ds);
+    let point_map_ds = from_slice(&point_map_s).unwrap();
+    assert_eq!(Bar::Point{ x: 5, y: -5}, point_map_ds);
+}
diff --git a/tests/ser.rs b/tests/ser.rs
index aba7cca..3a23f42 100644
--- a/tests/ser.rs
+++ b/tests/ser.rs
@@ -135,6 +135,10 @@ fn test_self_describing() {
         serializer.serialize_u64(9).unwrap();
     }
     assert_eq!(vec, b"\xd9\xd9\xf7\x09");
+
+    let sd = ser::SerializerOptions{ self_describe: true, ..Default::default() };
+    let vec = sd.to_vec(&9).unwrap();
+    assert_eq!(vec, b"\xd9\xd9\xf7\x09");
 }
 
 #[test]