Change String to &str

[omid]

Basically, it doesn't that much make sense to accept owned String in the structs.
So I changed all of them in the public API to &str and just in one case change it to Cow.

It's a breaking change, but very useful IMO, because this library is just a middleware to convert a set of input params to a request and doesn't need the ownership.

It can happen a lot to send multiple emails/request for one set of params. So it makes sense to get them as reference as much as possible.

[gsquire]

Thanks for the PR. I will likely review it this weekend.

[omid]

@gsquire I tried to integrate/test my changes in one of our apps.

The values in SGMap can be Cow to cover more cases.

SGMap<'a> = HashMap<&'a str, Cow<'a, str>>

Even SGMap<'a> = HashMap<&'a str, &'a str> also works, but Cow covers more scenarios easier.

I'm also thinking to change:

#[serde(skip_serializing_if = "Option::is_none")]
    headers: Option<SGMap<'a>>,

    #[serde(skip_serializing_if = "Option::is_none")]
    substitutions: Option<SGMap<'a>>,

    #[serde(skip_serializing_if = "Option::is_none")]
    custom_args: Option<SGMap<'a>>,

to:

#[serde(skip_serializing_if = "Vec::is_empty", serialize_with = "flat_vec")]
    headers: Vec<&'a SGMap>,

    #[serde(skip_serializing_if = "Vec::is_empty", serialize_with = "flat_vec")]
    substitutions: Vec<&'a SGMap>,

    #[serde(skip_serializing_if = "Vec::is_empty", serialize_with = "flat_vec")]
    custom_args: Vec<&'a SGMap>,

and something like:

fn flat_vec<S>(maps: &Vec<&SGMap>, serializer: S) -> Result<S::Ok, S::Error>
where
    S: serde::Serializer,
{
    maps.iter()
        .copied()
        .flatten()
        .map(|(k, v)| (k.as_str(), v.as_str()))
        .collect::<HashMap<_, _>>()
        .serialize(serializer)
}

and the merge will happen lazily in the flat_vec function.
It may help us to simplify SGMap to its original format: SGMap = HashMap<String, String>.
It doesn't remove all possible clones, but removes most of them.

If we choose the latest method, something like this can also happen to add_dynamic_template_data, but in that case, we will have some problem with add_dynamic_template_data_json and we need to either remove it or handle it with a new property.

[gsquire]

Hi, @omid. The changes LGTM overall.

Can you share more about why you want to change from Option<SGMap<'a>> to Vec<&'a SGMap>?

The values in SGMap can be Cow to cover more cases.

So long as Cow isn't exposed publicly, I think this is fine too. It may be confusing to newcomers otherwise.

[omid]

Thanks for checking the pr @gsquire

SGMap is used as an argument in 3-4 functions. In any of the scenarios, they should be references.

Excluding the change above, I kinda proposed 3 ways to solve the cloning issue in SGMap.
(I'm on mobile, so I'll write like pseudo code, sorry for that)

1. The one I implemented, which is: <&str, &str>.
2. The one which uses Cow in SGMap (which is public anyway): <&str, Cow>
3. To stick to <String, String>, but store references to SGMap in the Personalization struct.

I implemented all these method in one of my projects, the first solution had the least clone, then (most probably) 2nd, and then 3rd.
But on the other hand, 1st solution is harder to implement, then 2nd and then 3rd.

If you are perfectionist 😁 we can continue with the existing PR. If you are ignorant 😅 we can go with the last one. And if you are kinda both, 2nd is the way. 😬

I hope it's clearer now 🙏🏼

[gsquire]

I think the change in this PR is fine for now. If you want to experiment some more and send a subsequent patch, I'll happily review it again. I'll merge this.

[omid]

Thanks, I'm also a perfectionist 😁 so I prefer this PR.

[gsquire]

Thanks again! I published the new version just now.