Repository: gsquire/sendgrid-rs (language: rust)

Issue:
dynamic_template_data uses SGMap instead of a more general JSON friendly type
The [official v3 documentation](https://sendgrid.com/docs/api-reference/) lists the definition of `dynamic_template_data` as 
> This field is a collection of key/value pairs following the pattern "variable_name":"value to insert".

This is a bit mischaracterized, since technically any json object of any nesting level is supported: [official example](https://sendgrid.com/docs/ui/sending-email/how-to-send-an-email-with-dynamic-transactional-templates/)

`SGMap` fits the spec definition exactly, but is very limiting, because json serializable objects with any nesting can't be easily transformed into this type. You would need to serialize every member object to a string but only at a depth of 1. In order to do programatically, a complicated [declarative macro](https://doc.rust-lang.org/book/ch19-06-macros.html#declarative-macros-with-macro_rules-for-general-metaprogramming) would have to be written that iterates over each top level field that is serde serializable.

However all official client libraries use some variation of `Map<String, Object>` to define `dynamic_template_data`:

1. [Go](https://github.com/sendgrid/sendgrid-go/blob/2b5d9d2e1a70ad2c4abdd2bbaa78ca8a93249a81/helpers/mail/mail_v3.go#L40)
2. [Java](https://github.com/sendgrid/sendgrid-java/blob/main/src/main/java/com/sendgrid/helpers/mail/objects/Personalization.java#L37)
3. [Python](https://github.com/sendgrid/sendgrid-python/blob/main/sendgrid/helpers/mail/dynamic_template_data.py#L33)

I propose to:
1. change `dynamic_template_data` to `Option<serde_json::value::Value>`, and modify `add_dynamic_template_data` to use `serde_json::to_value`. `HashMap<String, String>` is `serde::Serialize` and should not panic, so we can safely unwrap that `to_value` call, not needing to change the signature.
2. add a new method to `Personalization`, `add_dynamic_template_data_json` that instead of a `SGMap` takes a `&T where <T: serde::Serialize+?Sized>`, and again calls `serde_json::to_value` on this and stores the resulting owned `serde_json::value::Value`. Since this feasibly could error with improper input, we may want to return a `Result` or `Option<Error>` here, happy to iterate on the proper error propagation.

reqwest does [something similar for their json body implementation](https://docs.rs/reqwest/0.11.1/src/reqwest/async_impl/request.rs.html#358).

Since `dynamic_template_data` is not `pub`, and `add_dynamic_template_data` signature doesn't change, this will not be a breaking change. 

One alternative here is to avoid storing `dynamic_template_data` as a `serde_json::value::Value` since it is a concrete type from another library, and instead use a `Box<dyn serde::Serialize + ?Sized>`, however this is arguably less clean and will store the object on the heap.

Another edge case to consider is we would no longer be enforcing the format of a json object, theoretically someone could pass a random string that is not in the format of key:value into `add_dynamic_template_data_json`, however we would expect sendgrid to handle this for us. I could even do some testing here to see if its a major issue or not.

Let me know if this makes sense and I'll start on a PR!


Discussion / hints:

Fix patch:
diff --git a/src/error.rs b/src/error.rs
index c918a3f..7e2e4b2 100644
--- a/src/error.rs
+++ b/src/error.rs
@@ -53,6 +53,10 @@ pub enum SendgridError {
     #[error("could not UTF-8 decode this filename")]
     InvalidFilename,
 
+    /// The failure was due to a non-object being used as template data.
+    #[error("dynamic template data must be a serializable object")]
+    InvalidTemplateValue,
+
     /// SendGrid returned an unsuccessful HTTP status code.
     #[error("Request failed: `{0}`")]
     RequestNotSuccessful(#[from] RequestNotSuccessful),
diff --git a/src/v3.rs b/src/v3.rs
index bac0625..ce9d328 100644
--- a/src/v3.rs
+++ b/src/v3.rs
@@ -6,13 +6,14 @@ use std::collections::HashMap;
 use data_encoding::BASE64;
 use reqwest::header::{self, HeaderMap, HeaderValue, InvalidHeaderValue};
 use serde::Serialize;
+use serde_json::{to_value, value::Value, value::Value::Object, Map};
 
 #[cfg(not(feature = "async"))]
 use reqwest::blocking::{Client, Response};
 #[cfg(feature = "async")]
 use reqwest::{Client, Response};
 
-use crate::error::{RequestNotSuccessful, SendgridResult};
+use crate::error::{RequestNotSuccessful, SendgridError, SendgridResult};
 
 const V3_API_URL: &str = "https://api.sendgrid.com/v3/mail/send";
 
@@ -86,7 +87,7 @@ pub struct Personalization {
     custom_args: Option<SGMap>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
-    dynamic_template_data: Option<SGMap>,
+    dynamic_template_data: Option<Map<String, Value>>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     send_at: Option<u64>,
@@ -339,11 +340,32 @@ impl Personalization {
 
     /// Add a dynamic template data field.
     pub fn add_dynamic_template_data(mut self, dynamic_template_data: SGMap) -> Personalization {
+        // We can safely unwrap & unreachable here since SGMap will always serialize
+        // to a JSON object.
+        let new_vals = match to_value(dynamic_template_data).unwrap() {
+            Object(map) => map,
+            _ => unreachable!(),
+        };
         self.dynamic_template_data
-            .get_or_insert_with(|| SGMap::with_capacity(dynamic_template_data.len()))
-            .extend(dynamic_template_data);
+            .get_or_insert_with(|| Map::with_capacity(new_vals.len()))
+            .extend(new_vals);
         self
     }
+
+    /// Add a dynamic template data fields from a json object.
+    pub fn add_dynamic_template_data_json<T: Serialize + ?Sized>(
+        mut self,
+        json_object: &T,
+    ) -> SendgridResult<Personalization> {
+        let new_vals = match to_value(json_object)? {
+            Object(map) => map,
+            _ => return Err(SendgridError::InvalidTemplateValue),
+        };
+        self.dynamic_template_data
+            .get_or_insert_with(|| Map::with_capacity(new_vals.len()))
+            .extend(new_vals);
+        Ok(self)
+    }
 }
 
 impl Attachment {
@@ -389,3 +411,67 @@ impl Attachment {
         self
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::v3::{Email, Message, Personalization};
+    use serde::Serialize;
+
+    #[derive(Serialize)]
+    struct OuterModel {
+        inners: Vec<InnerModel>,
+    }
+
+    #[derive(Serialize)]
+    struct InnerModel {
+        x: String,
+        y: String,
+        z: String,
+    }
+
+    #[test]
+    fn dynamic_template_data_sgmap() {
+        let json_str = Message::new(Email::new("from_email@test.com"))
+            .add_personalization(
+                Personalization::new(Email::new("to_email@test.com")).add_dynamic_template_data(
+                    [
+                        ("Norway".to_string(), "100".to_string()),
+                        ("Denmark".to_string(), "50".to_string()),
+                        ("Iceland".to_string(), "10".to_string()),
+                    ]
+                    .iter()
+                    .cloned()
+                    .collect(),
+                ),
+            )
+            .gen_json();
+        let expected = r#"{"from":{"email":"from_email@test.com"},"subject":"","personalizations":[{"to":[{"email":"to_email@test.com"}],"dynamic_template_data":{"Denmark":"50","Iceland":"10","Norway":"100"}}]}"#;
+        assert_eq!(json_str, expected);
+    }
+
+    #[test]
+    fn dynamic_template_data_json() {
+        let json_str = Message::new(Email::new("from_email@test.com"))
+            .add_personalization(
+                Personalization::new(Email::new("to_email@test.com"))
+                    .add_dynamic_template_data_json(&OuterModel {
+                        inners: vec![
+                            InnerModel {
+                                x: "1".to_string(),
+                                y: "2".to_string(),
+                                z: "3".to_string(),
+                            },
+                            InnerModel {
+                                x: "1".to_string(),
+                                y: "2".to_string(),
+                                z: "3".to_string(),
+                            },
+                        ],
+                    })
+                    .unwrap(),
+            )
+            .gen_json();
+        let expected = r#"{"from":{"email":"from_email@test.com"},"subject":"","personalizations":[{"to":[{"email":"to_email@test.com"}],"dynamic_template_data":{"inners":[{"x":"1","y":"2","z":"3"},{"x":"1","y":"2","z":"3"}]}}]}"#;
+        assert_eq!(json_str, expected);
+    }
+}


Test patch: