From ed6a3c9882fbc43eae9313ab1801610e49af863f Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 4 Oct 2023 16:37:49 +0100 Subject: [PATCH 1/4] ser: sequences: Centralise Have all the various versions of sequences (arrays and various forms of tuple) all go via ser::SerializeSeq. This reduces some duplication. And, we're about to change the implementation. Signed-off-by: Ian Jackson --- src/ser.rs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/ser.rs b/src/ser.rs index 2d09e1c8..f8e60843 100644 --- a/src/ser.rs +++ b/src/ser.rs @@ -234,10 +234,10 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer { _name: &'static str, _variant_index: u32, variant: &'static str, - _len: usize, + len: usize, ) -> Result { self.push_key(variant); - Ok(self) + self.serialize_seq(Some(len)) } fn serialize_map(self, _len: Option) -> Result { @@ -286,13 +286,11 @@ impl<'a> ser::SerializeTuple for &'a mut ConfigSerializer { where T: ?Sized + ser::Serialize, { - self.inc_last_key_index()?; - value.serialize(&mut **self)?; - Ok(()) + ser::SerializeSeq::serialize_element(self, value) } fn end(self) -> Result { - Ok(()) + ser::SerializeSeq::end(self) } } @@ -304,13 +302,11 @@ impl<'a> ser::SerializeTupleStruct for &'a mut ConfigSerializer { where T: ?Sized + ser::Serialize, { - self.inc_last_key_index()?; - value.serialize(&mut **self)?; - Ok(()) + ser::SerializeSeq::serialize_element(self, value) } fn end(self) -> Result { - Ok(()) + ser::SerializeSeq::end(self) } } @@ -322,12 +318,11 @@ impl<'a> ser::SerializeTupleVariant for &'a mut ConfigSerializer { where T: ?Sized + ser::Serialize, { - self.inc_last_key_index()?; - value.serialize(&mut **self)?; - Ok(()) + ser::SerializeSeq::serialize_element(self, value) } fn end(self) -> Result { + ser::SerializeSeq::end(&mut *self)?; self.pop_key(); Ok(()) } From 147e6c7275b65b6a74eaec9c05b317673e61084e Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 4 Oct 2023 16:55:10 +0100 Subject: [PATCH 2/4] ser: sequences: Introduce SeqSerializer newtype We're going to want to do something more complicated. In particular, to handle nested arrays properly, we need to do some work at the start and end of each array. The `new` and (inherent) `end` methods of this newtype is where that work will be done. Signed-off-by: Ian Jackson --- src/ser.rs | 42 ++++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 14 deletions(-) diff --git a/src/ser.rs b/src/ser.rs index f8e60843..9a86ca13 100644 --- a/src/ser.rs +++ b/src/ser.rs @@ -12,6 +12,8 @@ pub struct ConfigSerializer { pub output: Config, } +pub struct SeqSerializer<'a>(&'a mut ConfigSerializer); + impl ConfigSerializer { fn serialize_primitive(&mut self, value: T) -> Result<()> where @@ -85,10 +87,10 @@ impl ConfigSerializer { impl<'a> ser::Serializer for &'a mut ConfigSerializer { type Ok = (); type Error = ConfigError; - type SerializeSeq = Self; - type SerializeTuple = Self; - type SerializeTupleStruct = Self; - type SerializeTupleVariant = Self; + type SerializeSeq = SeqSerializer<'a>; + type SerializeTuple = SeqSerializer<'a>; + type SerializeTupleStruct = SeqSerializer<'a>; + type SerializeTupleVariant = SeqSerializer<'a>; type SerializeMap = Self; type SerializeStruct = Self; type SerializeStructVariant = Self; @@ -159,7 +161,8 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer { for byte in v { seq.serialize_element(byte)?; } - seq.end() + seq.end(); + Ok(()) } fn serialize_none(self) -> Result { @@ -214,7 +217,7 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer { } fn serialize_seq(self, _len: Option) -> Result { - Ok(self) + SeqSerializer::new(self) } fn serialize_tuple(self, len: usize) -> Result { @@ -260,7 +263,17 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer { } } -impl<'a> ser::SerializeSeq for &'a mut ConfigSerializer { +impl<'a> SeqSerializer<'a> { + fn new(inner: &'a mut ConfigSerializer) -> Result { + Ok(SeqSerializer(inner)) + } + + fn end(self) -> &'a mut ConfigSerializer { + self.0 + } +} + +impl<'a> ser::SerializeSeq for SeqSerializer<'a> { type Ok = (); type Error = ConfigError; @@ -268,17 +281,18 @@ impl<'a> ser::SerializeSeq for &'a mut ConfigSerializer { where T: ?Sized + ser::Serialize, { - self.inc_last_key_index()?; - value.serialize(&mut **self)?; + self.0.inc_last_key_index()?; + value.serialize(&mut *(self.0))?; Ok(()) } fn end(self) -> Result { + self.end(); Ok(()) } } -impl<'a> ser::SerializeTuple for &'a mut ConfigSerializer { +impl<'a> ser::SerializeTuple for SeqSerializer<'a> { type Ok = (); type Error = ConfigError; @@ -294,7 +308,7 @@ impl<'a> ser::SerializeTuple for &'a mut ConfigSerializer { } } -impl<'a> ser::SerializeTupleStruct for &'a mut ConfigSerializer { +impl<'a> ser::SerializeTupleStruct for SeqSerializer<'a> { type Ok = (); type Error = ConfigError; @@ -310,7 +324,7 @@ impl<'a> ser::SerializeTupleStruct for &'a mut ConfigSerializer { } } -impl<'a> ser::SerializeTupleVariant for &'a mut ConfigSerializer { +impl<'a> ser::SerializeTupleVariant for SeqSerializer<'a> { type Ok = (); type Error = ConfigError; @@ -322,8 +336,8 @@ impl<'a> ser::SerializeTupleVariant for &'a mut ConfigSerializer { } fn end(self) -> Result { - ser::SerializeSeq::end(&mut *self)?; - self.pop_key(); + let inner = self.end(); + inner.pop_key(); Ok(()) } } From 831102fe0ffd5c7fe475efe5f379c710d201f165 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 4 Oct 2023 17:38:14 +0100 Subject: [PATCH 3/4] ser: sequences: Rework, fixing handling of nested arrays Change the representation of our "current location". Previously it was a list of increasingly-long full paths, but excepting the putative final array index. Now we just record the individual elements. and assemble the whole path just before calling .set(). This saves a little memory on the whole since the entries in keys are now a bit shorter. It is much less confusing. (There are perhaps still further opportunities to use Rust's type system to better advantage to eliminuate opportunities for bugs.) Arrays that appear other than at the top level are now handled correctly. This includes nested arrays, and arrays containing structs, etc. Signed-off-by: Ian Jackson --- src/ser.rs | 104 ++++++++++++++++++++++++++++------------------------- 1 file changed, 55 insertions(+), 49 deletions(-) diff --git a/src/ser.rs b/src/ser.rs index 9a86ca13..f87bea2b 100644 --- a/src/ser.rs +++ b/src/ser.rs @@ -1,4 +1,5 @@ use std::fmt::Display; +use std::fmt::Write as _; use serde::ser; @@ -8,10 +9,25 @@ use crate::Config; #[derive(Default, Debug)] pub struct ConfigSerializer { - keys: Vec<(String, Option)>, + keys: Vec, pub output: Config, } +#[derive(Debug)] +enum SerKey { + Named(String), + Seq(usize), +} + +/// Serializer for numbered sequences +/// +/// This wrapper is present when we are outputting a sequence (numbered indices). +/// Making this a separate type centralises the handling of sequences +/// and ensures we don't have any call sites for `ser::SerializeSeq::serialize_element` +/// that don't do the necessary work of `SeqSerializer::new`. +/// +/// Existence of this wrapper implies that `.0.keys.last()` is +/// `Some(SerKey::Seq(next_index))`. pub struct SeqSerializer<'a>(&'a mut ConfigSerializer); impl ConfigSerializer { @@ -19,68 +35,47 @@ impl ConfigSerializer { where T: Into + Display, { - let key = match self.last_key_index_pair() { - Some((key, Some(index))) => format!("{}[{}]", key, index), - Some((key, None)) => key.to_string(), - None => { - return Err(ConfigError::Message(format!( - "key is not found for value {}", - value - ))) - } - }; + // At some future point we could perhaps retain a cursor into the output `Config`, + // rather than reifying the whole thing into a single string with `make_full_key` + // and passing that whole path to the `set` method. + // + // That would be marginally more performant, but more fiddly. + let key = self.make_full_key()?; #[allow(deprecated)] self.output.set(&key, value.into())?; Ok(()) } - fn last_key_index_pair(&self) -> Option<(&str, Option)> { - let len = self.keys.len(); - if len > 0 { - self.keys - .get(len - 1) - .map(|&(ref key, opt)| (key.as_str(), opt)) - } else { - None - } - } + fn make_full_key(&self) -> Result { + let mut keys = self.keys.iter(); - fn inc_last_key_index(&mut self) -> Result<()> { - let len = self.keys.len(); - if len > 0 { - self.keys - .get_mut(len - 1) - .map(|pair| pair.1 = pair.1.map(|i| i + 1).or(Some(0))) - .ok_or_else(|| { - ConfigError::Message(format!("last key is not found in {} keys", len)) - }) - } else { - Err(ConfigError::Message("keys is empty".to_string())) - } - } + let mut whole = match keys.next() { + Some(SerKey::Named(s)) => s.clone(), + _ => { + return Err(ConfigError::Message( + "top level is not a struct".to_string(), + )) + } + }; - fn make_full_key(&self, key: &str) -> String { - let len = self.keys.len(); - if len > 0 { - if let Some(&(ref prev_key, index)) = self.keys.get(len - 1) { - return if let Some(index) = index { - format!("{}[{}].{}", prev_key, index, key) - } else { - format!("{}.{}", prev_key, key) - }; + for k in keys { + match k { + SerKey::Named(s) => write!(whole, ".{}", s), + SerKey::Seq(i) => write!(whole, "[{}]", i), } + .expect("write! to a string failed"); } - key.to_string() + + Ok(whole) } fn push_key(&mut self, key: &str) { - let full_key = self.make_full_key(key); - self.keys.push((full_key, None)); + self.keys.push(SerKey::Named(key.to_string())); } - fn pop_key(&mut self) -> Option<(String, Option)> { - self.keys.pop() + fn pop_key(&mut self) { + self.keys.pop(); } } @@ -265,10 +260,14 @@ impl<'a> ser::Serializer for &'a mut ConfigSerializer { impl<'a> SeqSerializer<'a> { fn new(inner: &'a mut ConfigSerializer) -> Result { + inner.keys.push(SerKey::Seq(0)); + Ok(SeqSerializer(inner)) } fn end(self) -> &'a mut ConfigSerializer { + // This ought to be Some(SerKey::Seq(..)) but we don't want to panic if we are buggy + let _: Option = self.0.keys.pop(); self.0 } } @@ -281,8 +280,15 @@ impl<'a> ser::SerializeSeq for SeqSerializer<'a> { where T: ?Sized + ser::Serialize, { - self.0.inc_last_key_index()?; value.serialize(&mut *(self.0))?; + match self.0.keys.last_mut() { + Some(SerKey::Seq(i)) => *i += 1, + _ => { + return Err(ConfigError::Message( + "config-rs internal error (ser._element but last not Seq!".to_string(), + )) + } + }; Ok(()) } From aa63d2dbbcc13fbdfa846185d54d87d7822e2509 Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Wed, 4 Oct 2023 18:06:24 +0100 Subject: [PATCH 4/4] ser: sequences: Test a more comprehensive round-trip Signed-off-by: Ian Jackson --- src/ser.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/ser.rs b/src/ser.rs index f87bea2b..b9e35681 100644 --- a/src/ser.rs +++ b/src/ser.rs @@ -732,4 +732,28 @@ mod test { let actual: Test = config.try_deserialize().unwrap(); assert_eq!(test, actual); } + + #[test] + fn test_nest() { + let val = serde_json::json! { { + "top": { + "num": 1, + "array": [2], + "nested": [[3,4]], + "deep": [{ + "yes": true, + }], + "mixed": [ + { "boolish": false, }, + 42, + ["hi"], + { "inner": 66 }, + 23, + ], + } + } }; + let config = Config::try_from(&val).unwrap(); + let output: serde_json::Value = config.try_deserialize().unwrap(); + assert_eq!(val, output); + } }