gsquire / sendgrid-rs

Unofficial Rust library for the SendGrid API
https://crates.io/crates/sendgrid
MIT License
107 stars 50 forks source link

dynamic_template_data uses SGMap instead of a more general JSON friendly type #85

Closed konradjniemiec closed 3 years ago

konradjniemiec commented 3 years ago

The official v3 documentation 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

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 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
  2. Java
  3. Python

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.

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!