Merge pull request #267 from marhkb/master

rmp-serde/CHANGELOG.md

@@ -26,6 +26,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 - Fix error decoding `Some(enum)` (#185)
 - Fix error decoding unit structs which were encoded as `[]` (#181)
 - Fix `Display` implementations for errors not including all relevant information (#199)
+- Fix deserialization of nested `Option`s (#245)
 
 ## 0.13.7 - 2017-09-13
 ### Changed:

rmp-serde/src/decode.rs

@@ -153,6 +153,15 @@ pub struct Deserializer<R, C = DefaultConfig> {
     depth: usize,
 }
 
+impl<R: Read, C> Deserializer<R, C> {
+    #[inline]
+    fn take_or_read_marker(&mut self) -> Result<Marker, MarkerReadError> {
+        self.marker
+            .take()
+            .map_or_else(|| rmp::decode::read_marker(&mut self.rd), Ok)
+    }
+}
+
 impl<R: Read> Deserializer<ReadReader<R>, DefaultConfig> {
     #[doc(hidden)]
     #[deprecated(note="use `Deserializer::new` instead")]
@@ -321,10 +330,7 @@ impl<'de, R: ReadSlice<'de>, C: SerializerConfig> Deserializer<R, C> {
     fn read_128(&mut self) -> Result<[u8; 16], Error> {
         use std::convert::TryInto;
 
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
+        let marker = self.take_or_read_marker()?;
 
         if marker != Marker::Bin8 {
             return Err(Error::TypeMismatch(marker));
@@ -462,12 +468,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
     fn deserialize_any<V>(self, visitor: V) -> Result<V::Value, Self::Error>
         where V: Visitor<'de>
     {
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
-
-        match marker {
+        match self.take_or_read_marker()? {
             Marker::Null => visitor.visit_unit(),
             Marker::True => visitor.visit_bool(true),
             Marker::False => visitor.visit_bool(false),
@@ -571,11 +572,25 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
     fn deserialize_option<V>(self, visitor: V) -> Result<V::Value, Self::Error>
         where V: Visitor<'de>
     {
-        let marker = rmp::decode::read_marker(&mut self.rd)?;
+        // # Important
+        //
+        // If a nested Option `o ∈ { Option<Opion<t>>, Option<Option<Option<t>>>, ..., Option<Option<...Option<t>...> }`
+        // is visited for the first time, the marker (read from the underlying Reader) will determine
+        // `o`'s innermost type `t`.
+        // For subsequent visits of `o` the marker will not be re-read again but kept until type `t`
+        // is visited.
+        //
+        // # Note
+        //
+        // Round trips of Options where `Option<t> = None` such as `Some(None)` will fail because
+        // they are just seriialized as `nil`. The serialization format has probably to be changed
+        // to solve this. But as serde_json behaves the same, I think it's not worth doing this.
+        let marker = self.take_or_read_marker()?;
 
         if marker == Marker::Null {
             visitor.visit_none()
         } else {
+            // Keep the marker until `o`'s innermost type `t` is visited.
             self.marker = Some(marker);
             visitor.visit_some(self)
         }
@@ -584,11 +599,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
     fn deserialize_enum<V>(self, _name: &str, _variants: &[&str], visitor: V) -> Result<V::Value, Error>
         where V: Visitor<'de>
     {
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
-
+        let marker = self.take_or_read_marker()?;
         match rmp::decode::marker_to_len(&mut self.rd, marker)? {
             1 => visitor.visit_enum(VariantAccess::new(self)),
             n => Err(Error::LengthMismatch(n as u32)),
@@ -599,11 +610,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
         where V: Visitor<'de>
     {
         if name == MSGPACK_EXT_STRUCT_NAME {
-            let marker = match self.marker.take() {
-                Some(marker) => marker,
-                None => rmp::decode::read_marker(&mut self.rd)?,
-            };
-            let len = match marker {
+            let len = match self.take_or_read_marker()? {
                 Marker::FixExt1 => {
                     1 as u32
                 }
@@ -628,7 +635,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
                 Marker::Ext32 => {
                     read_u32(&mut self.rd)? as u32
                 }
-                _ => {
+                marker => {
                     return Err(Error::TypeMismatch(marker))
                 }
             };
@@ -646,11 +653,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
         // We need to special case this so that [] is treated as a unit struct when asked for,
         // but as a sequence otherwise. This is because we serialize unit structs as [] rather
         // than as 'nil'.
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
-        match marker {
+        match self.take_or_read_marker()? {
             Marker::Null | Marker::FixArray(0) => visitor.visit_unit(),
             marker => {
                 self.marker = Some(marker);
@@ -685,11 +688,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
         //
         // This allows interoperability with msgpack-lite, which performs an optimization of
         // storing rounded floats as integers.
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
-        match marker {
+        match self.take_or_read_marker()? {
             Marker::U8 => visitor.visit_f32(rmp::decode::read_data_u8(&mut self.rd)?.into()),
             Marker::U16 => visitor.visit_f32(rmp::decode::read_data_u16(&mut self.rd)?.into()),
             Marker::I8 => visitor.visit_f32(rmp::decode::read_data_i8(&mut self.rd)?.into()),
@@ -707,11 +706,7 @@ impl<'de, 'a, R: ReadSlice<'de>, C: SerializerConfig> serde::Deserializer<'de> f
     {
         // This special case allows us to decode some integer types as floats when safe, and
         // asked for. This is here to be consistent with 'f32'.
-        let marker = match self.marker.take() {
-            Some(marker) => marker,
-            None => rmp::decode::read_marker(&mut self.rd)?,
-        };
-        match marker {
+        match self.take_or_read_marker()? {
             Marker::U8 => visitor.visit_f64(rmp::decode::read_data_u8(&mut self.rd)?.into()),
             Marker::U16 => visitor.visit_f64(rmp::decode::read_data_u16(&mut self.rd)?.into()),
             Marker::U32 => visitor.visit_f64(rmp::decode::read_data_u32(&mut self.rd)?.into()),

rmp-serde/tests/decode.rs

@@ -273,6 +273,24 @@ fn pass_option_none() {
     assert_eq!(None, actual);
 }
 
+#[test]
+fn pass_nested_option_some() {
+    let buf = [0x1f];
+
+    let mut de = Deserializer::new(&buf[..]);
+    let actual: Option<Option<u8>> = Deserialize::deserialize(&mut de).unwrap();
+    assert_eq!(Some(Some(31)), actual);
+}
+
+#[test]
+fn pass_nested_option_none() {
+    let buf = [0xc0];
+
+    let mut de = Deserializer::new(&buf[..]);
+    let actual: Option<Option<u8>> = Deserialize::deserialize(&mut de).unwrap();
+    assert_eq!(None, actual);
+}
+
 #[test]
 fn fail_option_u8_from_reserved() {
     let buf = [0xc1];

rmp-serde/tests/decode_derive.rs

@@ -383,6 +383,25 @@ fn pass_enum_with_one_arg() {
     assert_eq!(buf.len() as u64, de.get_ref().position())
 }
 
+#[test]
+fn pass_struct_with_nested_options() {
+    // The encoded bytearray is: [null, 13].
+    let buf = [0x92, 0xc0, 0x0D];
+    let cur = Cursor::new(&buf[..]);
+
+    #[derive(Debug, PartialEq, Deserialize)]
+    struct Struct {
+        f1: Option<Option<u32>>,
+        f2: Option<Option<u32>>,
+    }
+
+    let mut de = Deserializer::new(cur);
+    let actual: Struct = Deserialize::deserialize(&mut de).unwrap();
+
+    assert_eq!(Struct { f1: None, f2: Some(Some(13)) }, actual);
+    assert_eq!(buf.len() as u64, de.get_ref().position());
+}
+
 #[test]
 fn pass_struct_with_flattened_map_field() {
     use std::collections::BTreeMap;

rmp-serde/tests/round.rs

@@ -26,6 +26,27 @@ fn round_trip_option() {
     assert_eq!(expected, Deserialize::deserialize(&mut de).unwrap());
 }
 
+#[test]
+fn round_trip_nested_option() {
+    #[derive(Debug, PartialEq, Serialize, Deserialize)]
+    struct Struct {
+        f1: Option<Option<u32>>,
+        f2: Option<Option<u32>>,
+    }
+
+    let expected = Struct {
+        f1: Some(Some(13)),
+        f2: None
+    };
+
+    let mut buf = Vec::new();
+    expected.serialize(&mut Serializer::new(&mut buf)).unwrap();
+
+    let mut de = Deserializer::new(Cursor::new(&buf[..]));
+
+    assert_eq!(expected, Deserialize::deserialize(&mut de).unwrap());
+}
+
 #[test]
 fn round_trip_optional_enum() {
     #[derive(Serialize, Deserialize, Debug, PartialEq)]