Merge pull request #44 from 3Hren/pr/fix-serde-enums

Several serde deserialization fixes

Author: 3Hren

CHANGELOG.md

@@ -3,8 +3,6 @@ All notable changes to this project will be documented in this file.
 This project adheres to [Semantic Versioning](http://semver.org/).
 
 ## [Unreleased][unreleased]
-### Added
-- Serde serializer can now be extended with custom struct encoding policy.
 
 ## 0.7.0 - 2015-08-24
 ### Changed

rmp-serde/CHANGELOG.md

@@ -0,0 +1,21 @@
+# Change Log
+All notable changes to this project will be documented in this file.
+This project adheres to [Semantic Versioning](http://semver.org/).
+
+## [Unreleased][unreleased]
+### Changed
+- Serializer can now be extended with custom struct encoding policy.
+- Improved error types and its messages for serialization part.
+    - New error type introduced - UnknownLength. Returned on attempt to serialize struct, map or serquence with unknown
+    length (Serde allows this).
+    - The new type is returned if necessary.
+
+### Fixed
+- Deserializer now properly works with enums.
+- Options with default values (that can be initialized using unit marker) deserialization.
+  This fix also forbids the following Option deserialization cases:
+    - Option<()>.
+    - Option<Option<...>>.
+  It's impossible to properly deserialize the listed cases without explicit option marker in protocol.
+- Serializer now properly serializes unit structs.
+  Previously it was serialized as a unit (nil), now there is just an empty array ([]).

rmp-serde/src/decode.rs

@@ -12,6 +12,7 @@ use rmp::decode::{
     MarkerReadError,
     ReadError,
     ValueReadError,
+    read_array_size,
     read_numeric_data,
     read_str_data,
     read_marker,
@@ -75,7 +76,7 @@ impl serde::de::Error for Error {
             serde::de::Type::Str => Error::TypeMismatch(Marker::Str32),
             serde::de::Type::String => Error::TypeMismatch(Marker::Str32),
             serde::de::Type::Unit => Error::TypeMismatch(Marker::Null),
-            serde::de::Type::Option => Error::TypeMismatch(Marker::True),
+            serde::de::Type::Option => Error::TypeMismatch(Marker::Null),
             serde::de::Type::Seq => Error::TypeMismatch(Marker::Array32),
             serde::de::Type::Map => Error::TypeMismatch(Marker::Map32),
             serde::de::Type::UnitStruct => Error::TypeMismatch(Marker::Null),
@@ -150,6 +151,15 @@ impl From<MarkerReadError> for Error {
     }
 }
 
+impl From<serde::de::value::Error> for Error {
+    fn from(err: serde::de::value::Error) -> Error {
+        match err {
+            serde::de::value::Error::SyntaxError => Error::Syntax("unknown".into()),
+            _ => Error::Uncategorized("unknown".into()),
+        }
+    }
+}
+
 pub type Result<T> = result::Result<T, Error>;
 
 /// # Note
@@ -160,6 +170,7 @@ pub type Result<T> = result::Result<T, Error>;
 pub struct Deserializer<R: Read> {
     rd: R,
     buf: Vec<u8>,
+    decoding_option: bool,
 }
 
 impl<R: Read> Deserializer<R> {
@@ -168,6 +179,7 @@ impl<R: Read> Deserializer<R> {
         Deserializer {
             rd: rd,
             buf: Vec::new(),
+            decoding_option: false,
         }
     }
 
@@ -237,8 +249,16 @@ impl<R: Read> serde::Deserializer for Deserializer<R> {
     fn visit<V>(&mut self, mut visitor: V) -> Result<V::Value>
         where V: serde::de::Visitor
     {
-        match try!(read_marker(&mut self.rd)) {
-            Marker::Null => visitor.visit_unit(),
+        let marker = try!(read_marker(&mut self.rd));
+
+        match marker {
+            Marker::Null => {
+                if self.decoding_option {
+                    visitor.visit_none()
+                } else {
+                    visitor.visit_unit()
+                }
+            }
             Marker::True => visitor.visit_bool(true),
             Marker::False => visitor.visit_bool(false),
             Marker::FixPos(val) => visitor.visit_u8(val),
@@ -307,14 +327,37 @@ impl<R: Read> serde::Deserializer for Deserializer<R> {
     }
 
     /// We treat Value::Null as None.
+    ///
+    /// # Note
+    ///
+    /// Without using explicit option marker it's impossible to properly deserialize the following
+    /// specific cases:
+    ///  - `Option<()>`.
+    ///  - nested optionals, like `Option<Option<...>>`.
     fn visit_option<V>(&mut self, mut visitor: V) -> Result<V::Value>
         where V: serde::de::Visitor,
     {
         // Primarily try to read optimisticly.
-        match visitor.visit_some(self) {
+        self.decoding_option = true;
+        let res = match visitor.visit_some(self) {
             Ok(val) => Ok(val),
             Err(Error::TypeMismatch(Marker::Null)) => visitor.visit_none(),
             Err(err) => Err(err)
+        };
+        self.decoding_option = false;
+
+        res
+    }
+
+    fn visit_enum<V>(&mut self, _enum: &str, _variants: &'static [&'static str], mut visitor: V)
+        -> Result<V::Value>
+        where V: serde::de::EnumVisitor
+    {
+        let len = try!(read_array_size(&mut self.rd));
+
+        match len {
+            2 => visitor.visit(VariantVisitor::new(self)),
+            n => Err(Error::LengthMismatch(n as u32)),
         }
     }
 }
@@ -385,3 +428,55 @@ impl<'a, R: Read + 'a> serde::de::MapVisitor for MapVisitor<'a, R> {
         }
     }
 }
+
+/// Default variant visitor.
+///
+/// # Note
+///
+/// We use default behaviour for new type, which decodes enums with a single value as a tuple.
+pub struct VariantVisitor<'a, R: Read + 'a> {
+    de: &'a mut Deserializer<R>,
+}
+
+impl<'a, R: Read + 'a> VariantVisitor<'a, R> {
+    pub fn new(de: &'a mut Deserializer<R>) -> VariantVisitor<'a, R> {
+        VariantVisitor {
+            de: de,
+        }
+    }
+}
+
+impl<'a, R: Read> serde::de::VariantVisitor for VariantVisitor<'a, R> {
+    type Error = Error;
+
+    // Resolves an internal variant type by integer id.
+    fn visit_variant<V>(&mut self) -> Result<V>
+        where V: serde::Deserialize
+    {
+        use serde::de::value::ValueDeserializer;
+
+        let id: u32 = try!(serde::Deserialize::deserialize(self.de));
+
+        let mut de = (id as usize).into_deserializer();
+        Ok(try!(V::deserialize(&mut de)))
+    }
+
+    fn visit_unit(&mut self) -> Result<()> {
+        use serde::de::Deserialize;
+
+        type T = ();
+        T::deserialize(self.de)
+    }
+
+    fn visit_tuple<V>(&mut self, len: usize, visitor: V) -> Result<V::Value>
+        where V: serde::de::Visitor,
+    {
+        serde::de::Deserializer::visit_tuple(self.de, len, visitor)
+    }
+
+    fn visit_struct<V>(&mut self, fields: &'static [&'static str], visitor: V) -> Result<V::Value>
+        where V: serde::de::Visitor,
+    {
+        serde::de::Deserializer::visit_tuple(self.de, fields.len(), visitor)
+    }
+}

rmp-serde/src/encode.rs

@@ -25,15 +25,25 @@ pub enum Error {
     /// Failed to write MessagePack'ed single-byte value into the write.
     InvalidFixedValueWrite(WriteError),
     InvalidValueWrite(ValueWriteError),
+
+    /// Failed to serialize struct, sequence or map, because its length is unknown.
+    UnknownLength,
 }
 
 impl ::std::error::Error for Error {
-    fn description(&self) -> &str { "an error occurred while writing encoded value" }
+    fn description(&self) -> &str {
+        match *self {
+            Error::InvalidFixedValueWrite(..) => "invalid fixed value write",
+            Error::InvalidValueWrite(..) => "invalid value write",
+            Error::UnknownLength => "attempt to serialize struct, sequence or map with unknown length",
+        }
+    }
 
     fn cause(&self) -> Option<&::std::error::Error> {
         match *self {
             Error::InvalidFixedValueWrite(ref err) => Some(err),
             Error::InvalidValueWrite(ref err) => Some(err),
+            Error::UnknownLength => None,
         }
     }
 }
@@ -229,7 +239,7 @@ impl<'a, W: VariantWriter> serde::Serializer for Serializer<'a, W> {
 
         let len = match visitor.len() {
             Some(len) => len,
-            None => panic!("do not know how to serialize a sequence with no length"),
+            None => return Err(Error::UnknownLength),
         };
 
         // ... and its arguments length.
@@ -266,7 +276,7 @@ impl<'a, W: VariantWriter> serde::Serializer for Serializer<'a, W> {
     {
         let len = match visitor.len() {
             Some(len) => len,
-            None => panic!("do not know how to serialize a sequence with no length"),
+            None => return Err(Error::UnknownLength),
         };
 
         try!(write_array_len(&mut self.wr, len as u32));
@@ -287,7 +297,7 @@ impl<'a, W: VariantWriter> serde::Serializer for Serializer<'a, W> {
     {
         let len = match visitor.len() {
             Some(len) => len,
-            None => panic!("do not know how to serialize a map with no length"),
+            None => return Err(Error::UnknownLength),
         };
 
         try!(write_map_len(&mut self.wr, len as u32));
@@ -305,12 +315,18 @@ impl<'a, W: VariantWriter> serde::Serializer for Serializer<'a, W> {
         value.serialize(self)
     }
 
+    fn visit_unit_struct(&mut self, _name: &'static str) -> Result<(), Error> {
+        try!(write_array_len(&mut self.wr, 0));
+
+        Ok(())
+    }
+
     fn visit_struct<V>(&mut self, _name: &str, mut visitor: V) -> Result<(), Error>
         where V: serde::ser::MapVisitor,
     {
         let len = match visitor.len() {
             Some(len) => len,
-            None => panic!("do not know how to serialize a sequence with no length"),
+            None => return Err(Error::UnknownLength),
         };
 
         try!(self.vw.write_struct_len(&mut self.wr, len as u32));