Merge pull request #80 from sam-berning/datastore_expect_json

Update datastore serializer to expect JSON and correctly handle null values

Author: sam-berning

sources/Cargo.lock

@@ -136,7 +136,7 @@ where
     let os = BottlerocketRelease::new().context(error::ReleaseDataSnafu)?;
 
     // Turn into a serde Value we can manipulate.
-    let val = serde_json::to_value(os).expect("struct to value can't fail");
+    let val = serde_json::to_value(os).context(error::SettingsToJsonSnafu)?;
 
     // Structs are Objects in serde_json, which have a map of field -> value inside.  We
     // destructure to get it by value, instead of as_object() which gives references.
@@ -366,7 +366,8 @@ pub(crate) fn set_settings<D: DataStore>(
     transaction: &str,
 ) -> Result<()> {
     trace!("Serializing Settings to write to data store");
-    let pairs = to_pairs_with_prefix("settings", settings)
+    let settings_json = serde_json::to_value(settings).expect("struct to value can't fail");
+    let pairs = to_pairs_with_prefix("settings", &settings_json)
         .context(error::DataStoreSerializationSnafu { given: "Settings" })?;
     let pending = Committed::Pending {
         tx: transaction.into(),

sources/api/apiserver/src/server/controller.rs

@@ -94,6 +94,9 @@ pub enum Error {
     #[snafu(display("Unable to serialize data: {}", source))]
     Serialize { source: serde_json::Error },
 
+    #[snafu(display("Error serializing settings to JSON: {}", source))]
+    SettingsToJson { source: serde_json::Error },
+
     #[snafu(display("Error serializing {}: {} ", given, source))]
     DataStoreSerialization {
         given: String,

sources/api/apiserver/src/server/error.rs

@@ -792,6 +792,7 @@ impl ResponseError for error::Error {
             SystemdNotifyStatus {} => StatusCode::INTERNAL_SERVER_ERROR,
             SetPermissions { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             SetGroup { .. } => StatusCode::INTERNAL_SERVER_ERROR,
+            SettingsToJson { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             ReleaseData { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             Shutdown { .. } => StatusCode::INTERNAL_SERVER_ERROR,
             Reboot { .. } => StatusCode::INTERNAL_SERVER_ERROR,

sources/api/apiserver/src/server/mod.rs

@@ -21,7 +21,7 @@ use crate::{serialize_scalar, Key, KeyType, ScalarError};
 ///    Settings -> DockerSettings -> bridge_ip = u64
 /// would turn into a key of "settings.docker-settings.bridge-ip" and a serialized String
 /// representing the u64 data.
-pub fn to_pairs<T: Serialize>(value: &T) -> Result<HashMap<Key, String>> {
+pub fn to_pairs(value: &serde_json::Value) -> Result<HashMap<Key, String>> {
     let mut output = HashMap::new();
     let serializer = Serializer::new(&mut output, None);
     value.serialize(serializer)?;
@@ -30,10 +30,9 @@ pub fn to_pairs<T: Serialize>(value: &T) -> Result<HashMap<Key, String>> {
 
 /// Like to_pairs, but lets you add an arbitrary prefix to the resulting keys.  A separator will
 /// automatically be added after the prefix.
-pub fn to_pairs_with_prefix<S, T>(prefix: S, value: &T) -> Result<HashMap<Key, String>>
+pub fn to_pairs_with_prefix<S>(prefix: S, value: &serde_json::Value) -> Result<HashMap<Key, String>>
 where
     S: AsRef<str>,
-    T: Serialize,
 {
     let prefix = prefix.as_ref();
     let prefix_key = Key::new(KeyType::Data, prefix).map_err(|e| {
@@ -230,9 +229,12 @@ impl<'a> ser::Serializer for Serializer<'a> {
         bad_type("bytes")
     }
 
-    // We just don't expect to need these, and we doesn't have a great way to represent them.
+    // serde_json::Value::Null is the only case where we should see this, so we can essentially
+    // consider this to be serialize_null(). Since we should only see null values in a settings
+    // input when the settings structure has an Option<T>, it should be safe to omit the key/value
+    // pair from the serialization output if the value is null.
     fn serialize_unit(self) -> Result<()> {
-        bad_type("unit")
+        Ok(())
     }
 
     fn serialize_unit_struct(self, _name: &'static str) -> Result<()> {
@@ -490,6 +492,7 @@ mod test {
     use crate::{Key, KeyType};
     use maplit::hashmap;
     use serde::Serialize;
+    use serde_json::json;
 
     // Helper macro for making a data Key for testing whose name we know is valid.
     macro_rules! key {
@@ -516,20 +519,22 @@ mod test {
             list: vec![3, 4, 5],
             boolean: true,
         };
-        let keys = to_pairs(&b).unwrap();
+        let j = serde_json::to_value(b).unwrap();
+        let keys = to_pairs(&j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
-                key!("B.list") => "[3,4,5]".to_string(),
-                key!("B.boolean") => "true".to_string(),
+                key!("list") => "[3,4,5]".to_string(),
+                key!("boolean") => "true".to_string(),
             )
         );
     }
 
     #[test]
     fn empty_value() {
         let val: toml::Value = toml::from_str("").unwrap();
-        let keys = to_pairs(&val).unwrap();
+        let json = serde_json::to_value(val).unwrap();
+        let keys = to_pairs(&json).unwrap();
         assert_eq!(keys, hashmap!())
     }
 
@@ -540,13 +545,14 @@ mod test {
             boolean: true,
         };
         let a = A { id: 42, b: Some(b) };
-        let keys = to_pairs(&a).unwrap();
+        let j = serde_json::to_value(a).unwrap();
+        let keys = to_pairs(&j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
-                key!("A.b.list") => "[5,6,7]".to_string(),
-                key!("A.b.boolean") => "true".to_string(),
-                key!("A.id") => "42".to_string(),
+                key!("b.list") => "[5,6,7]".to_string(),
+                key!("b.boolean") => "true".to_string(),
+                key!("id") => "42".to_string(),
             )
         );
     }
@@ -559,7 +565,8 @@ mod test {
                 key!("ie") => 43,
             ),
         );
-        let keys = to_pairs_with_prefix("map", &m).unwrap();
+        let j = serde_json::to_value(m).unwrap();
+        let keys = to_pairs_with_prefix("map", &j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
@@ -577,7 +584,8 @@ mod test {
                 key!("ie") => 43,
             ),
         );
-        let keys = to_pairs(&m).unwrap();
+        let j = serde_json::to_value(m).unwrap();
+        let keys = to_pairs(&j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
@@ -590,7 +598,8 @@ mod test {
     #[test]
     fn concrete_fails() {
         let i = 42;
-        to_pairs(&i).unwrap_err();
+        let j = serde_json::to_value(i).unwrap();
+        to_pairs(&j).unwrap_err();
     }
 
     #[test]
@@ -601,7 +610,8 @@ mod test {
                 key!("ie") => "oranges",
             ),
         );
-        let keys = to_pairs(&m).unwrap();
+        let j = serde_json::to_value(m).unwrap();
+        let keys = to_pairs(&j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
@@ -626,7 +636,8 @@ mod test {
                 key!("ie") => TestEnum::Beta,
             ),
         );
-        let keys = to_pairs(&m).unwrap();
+        let j = serde_json::to_value(m).unwrap();
+        let keys = to_pairs(&j).unwrap();
         assert_eq!(
             keys,
             hashmap!(
@@ -635,4 +646,131 @@ mod test {
             )
         );
     }
+
+    #[test]
+    fn json_null() {
+        let j = json!(null);
+        let keys = to_pairs_with_prefix("null", &j).unwrap();
+        // the null value and its key should be skipped in serialization, resulting in an empty
+        // hashmap
+        assert_eq!(keys, hashmap!());
+    }
+
+    #[test]
+    fn json_bool() {
+        let j = json!(true);
+        let keys = to_pairs_with_prefix("bool", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("bool") => "true".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_number() {
+        let j = json!(42);
+        let keys = to_pairs_with_prefix("number", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("number") => "42".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_number_float() {
+        let j = json!(4.2);
+        let keys = to_pairs_with_prefix("number", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("number") => "4.2".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_string() {
+        let j = json!("hello");
+        let keys = to_pairs_with_prefix("string", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("string") => "\"hello\"".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_array() {
+        let j = json!(["foo", true, 42]);
+        let keys = to_pairs_with_prefix("array", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("array") => "[\"foo\",true,42]".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_array_with_null() {
+        let j = json!(["foo", null, true, 42]);
+        let keys = to_pairs_with_prefix("array", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("array") => "[\"foo\",null,true,42]".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_object() {
+        let j = json!({
+            "bool": true,
+            "number": 42,
+            "string": "foo",
+            "array": ["foo", true, null, 42],
+            "object": {"number": 4.2}
+        });
+        let keys = to_pairs_with_prefix("object", &j).unwrap();
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("object.bool") => "true".to_string(),
+                key!("object.number") => "42".to_string(),
+                key!("object.string") => "\"foo\"".to_string(),
+                key!("object.array") => "[\"foo\",true,null,42]".to_string(),
+                key!("object.object.number") => "4.2".to_string(),
+            )
+        );
+    }
+
+    #[test]
+    fn json_object_with_null() {
+        let j = json!({
+            "null": null,
+            "bool": true,
+            "number": 42,
+            "string": "foo",
+            "array": ["foo", true, null, 42],
+            "object": {"number": 4.2}
+        });
+        let keys = to_pairs_with_prefix("object", &j).unwrap();
+        // the null value and its key should be skipped in serialization
+        assert_eq!(
+            keys,
+            hashmap!(
+                key!("object.bool") => "true".to_string(),
+                key!("object.number") => "42".to_string(),
+                key!("object.string") => "\"foo\"".to_string(),
+                key!("object.array") => "[\"foo\",true,null,42]".to_string(),
+                key!("object.object.number") => "4.2".to_string(),
+            )
+        );
+    }
 }

sources/api/datastore/src/serialization/pairs.rs

@@ -17,6 +17,7 @@ log.workspace = true
 models.workspace = true
 rand = { workspace = true, features = ["std", "std_rng"] }
 semver.workspace = true
+serde_json.workspace = true
 simplelog.workspace = true
 snafu.workspace = true
 toml.workspace = true

sources/api/storewolf/Cargo.toml

@@ -129,6 +129,9 @@ mod error {
 
         #[snafu(display("Could not read defaults from '{}': {}", path.display(), source))]
         ReadDefaults { path: PathBuf, source: io::Error },
+
+        #[snafu(display("Could not convert toml value to JSON: {}", source))]
+        JsonConversion { source: serde_json::Error },
     }
 }
 
@@ -288,16 +291,21 @@ fn populate_default_datastore<P: AsRef<Path>>(
     // services run, which will create config files for default keys that require them.
     if let Some(def_settings_val) = maybe_settings_val {
         debug!("Serializing default settings and writing new ones to datastore");
-        let def_settings_table = def_settings_val
-            .as_table()
-            .context(error::DefaultSettingsNotTableSnafu)?;
+
+        ensure!(
+            def_settings_val.is_table(),
+            error::DefaultSettingsNotTableSnafu
+        );
+
+        let settings_json =
+            serde_json::to_value(def_settings_val).context(error::JsonConversionSnafu)?;
 
         // The default settings were removed from the "settings" key of the
         // defaults table above. We still need them under a "settings" key
         // before serializing so we have full dotted keys like
         // "settings.foo.bar" and not just "foo.bar". We use a HashMap
         // to rebuild the nested structure.
-        let def_settings = to_pairs_with_prefix("settings", &def_settings_table).context(
+        let def_settings = to_pairs_with_prefix("settings", &settings_json).context(
             error::SerializationSnafu {
                 given: "default settings",
             },
@@ -385,7 +393,8 @@ fn populate_default_datastore<P: AsRef<Path>>(
     // If any other defaults remain (configuration files, services, etc),
     // write them to the datastore in Live state
     debug!("Serializing other defaults and writing new ones to datastore");
-    let defaults = to_pairs(&defaults_val).context(error::SerializationSnafu {
+    let defaults_json = serde_json::to_value(defaults_val).context(error::JsonConversionSnafu)?;
+    let defaults = to_pairs(&defaults_json).context(error::SerializationSnafu {
         given: "other defaults",
     })?;