Fix global SEND_DEFAULTS merging

Replace generic `combine` with
specific `merge_dicts_deep`,
`merge_dicts_shallow`,
`merge_dicts_one_level` or
`concat_lists`, depending on
appropriate behavior for each
message attribute.

Fixes merging global `SEND_DEFAULTS`
with message `esp_extra` for ESP APIs
that use nested payload structures.
And clarifies intent for other properties.

CHANGELOG.rst

@@ -30,6 +30,14 @@ vNext
 
 *unreleased changes*
 
+Fixes
+~~~~~
+
+* Correctly merge global ``SEND_DEFAULTS`` with message ``esp_extra``
+  for ESP APIs that use a nested structure (including Mandrill and SparkPost).
+  Clarify intent of global defaults merging code for other message properties.
+  (Thanks to `@mounirmesselmeni`_ for reporting the issue.)
+
 Other
 ~~~~~
 
@@ -1571,6 +1579,7 @@ Features
 .. _@mark-mishyn: https://github.com/mark-mishyn
 .. _@martinezleoml: https://github.com/martinezleoml
 .. _@mbk-ok: https://github.com/mbk-ok
+.. _@mounirmesselmeni: https://github.com/mounirmesselmeni
 .. _@mwheels: https://github.com/mwheels
 .. _@nuschk: https://github.com/nuschk
 .. _@puru02: https://github.com/puru02

anymail/backends/base.py

@@ -18,13 +18,16 @@
 from ..utils import (
     UNSET,
     Attachment,
-    combine,
+    concat_lists,
     force_non_lazy,
     force_non_lazy_dict,
     force_non_lazy_list,
     get_anymail_setting,
     is_lazy,
     last,
+    merge_dicts_deep,
+    merge_dicts_one_level,
+    merge_dicts_shallow,
     parse_address_list,
     parse_single_address,
 )
@@ -253,8 +256,7 @@ class BasePayload:
     #   attr: the property name
     #   combiner: optional function(default_value, value) -> value
     #     to combine settings defaults with the EmailMessage property value
-    #     (usually `combine` to merge, or `last` for message value to override default;
-    #     use `None` if settings defaults aren't supported)
+    #     (use `None` if settings defaults aren't supported)
     #   converter: optional function(value) -> value transformation
     #     (can be a callable or the string name of a Payload method, or `None`)
     #     The converter must force any Django lazy translation strings to text.
@@ -263,29 +265,29 @@ class BasePayload:
     base_message_attrs = (
         # Standard EmailMessage/EmailMultiAlternatives props
         ("from_email", last, parse_address_list),  # multiple from_emails are allowed
-        ("to", combine, parse_address_list),
-        ("cc", combine, parse_address_list),
-        ("bcc", combine, parse_address_list),
+        ("to", concat_lists, parse_address_list),
+        ("cc", concat_lists, parse_address_list),
+        ("bcc", concat_lists, parse_address_list),
         ("subject", last, force_non_lazy),
-        ("reply_to", combine, parse_address_list),
-        ("extra_headers", combine, force_non_lazy_dict),
+        ("reply_to", concat_lists, parse_address_list),
+        ("extra_headers", merge_dicts_shallow, force_non_lazy_dict),
         ("body", last, force_non_lazy),  # set_body handles content_subtype
-        ("alternatives", combine, "prepped_alternatives"),
-        ("attachments", combine, "prepped_attachments"),
+        ("alternatives", concat_lists, "prepped_alternatives"),
+        ("attachments", concat_lists, "prepped_attachments"),
     )
     anymail_message_attrs = (
         # Anymail expando-props
         ("envelope_sender", last, parse_single_address),
-        ("metadata", combine, force_non_lazy_dict),
+        ("metadata", merge_dicts_shallow, force_non_lazy_dict),
         ("send_at", last, "aware_datetime"),
-        ("tags", combine, force_non_lazy_list),
+        ("tags", concat_lists, force_non_lazy_list),
         ("track_clicks", last, None),
         ("track_opens", last, None),
         ("template_id", last, force_non_lazy),
-        ("merge_data", combine, force_non_lazy_dict),
-        ("merge_global_data", combine, force_non_lazy_dict),
-        ("merge_metadata", combine, force_non_lazy_dict),
-        ("esp_extra", combine, force_non_lazy_dict),
+        ("merge_data", merge_dicts_one_level, force_non_lazy_dict),
+        ("merge_global_data", merge_dicts_shallow, force_non_lazy_dict),
+        ("merge_metadata", merge_dicts_one_level, force_non_lazy_dict),
+        ("esp_extra", merge_dicts_deep, force_non_lazy_dict),
     )
     esp_message_attrs = ()  # subclasses can override
 

anymail/utils.py

@@ -2,6 +2,7 @@
 import mimetypes
 from base64 import b64encode
 from collections.abc import Mapping, MutableMapping
+from copy import copy, deepcopy
 from email.mime.base import MIMEBase
 from email.utils import formatdate, getaddresses, parsedate_to_datetime, unquote
 from urllib.parse import urlsplit, urlunsplit
@@ -20,17 +21,44 @@
 UNSET = type("UNSET", (object,), {})  # Used as non-None default value
 
 
-def combine(*args):
+def concat_lists(*args):
     """
-    Combines all non-UNSET args, by shallow merging mappings and concatenating sequences
+    Combines all non-UNSET args, by concatenating lists (or sequence-like types).
+    Does not modify any args.
 
-    >>> combine({'a': 1, 'b': 2}, UNSET, {'b': 3, 'c': 4}, UNSET)
-    {'a': 1, 'b': 3, 'c': 4}
-    >>> combine([1, 2], UNSET, [3, 4], UNSET)
+    >>> concat_lists([1, 2], UNSET, [3, 4], UNSET)
     [1, 2, 3, 4]
-    >>> combine({'a': 1}, None, {'b': 2})  # None suppresses earlier args
+    >>> concat_lists([1, 2], None, [3, 4])  # None suppresses earlier args
+    [3, 4]
+    >>> concat_lists()
+    UNSET
+
+    """
+    result = UNSET
+    for value in args:
+        if value is None:
+            # None is a request to suppress any earlier values
+            result = UNSET
+        elif value is not UNSET:
+            if result is UNSET:
+                result = list(value)
+            else:
+                result = result + list(value)  # concatenate sequence-like
+    return result
+
+
+def merge_dicts_shallow(*args):
+    """
+    Shallow-merges all non-UNSET args.
+    Does not modify any args.
+
+    >>> merge_dicts_shallow({'a': 1, 'b': 2}, UNSET, {'b': 3, 'c': 4}, UNSET)
+    {'a': 1, 'b': 3, 'c': 4}
+    >>> merge_dicts_shallow({'a': {'a1': 1, 'a2': 2}}, {'a': {'a1': 11, 'a3': 33}})
+    {'a': {'a1': 11, 'a3': 33}}
+    >>> merge_dicts_shallow({'a': 1}, None, {'b': 2})  # None suppresses earlier args
     {'b': 2}
-    >>> combine()
+    >>> merge_dicts_shallow()
     UNSET
 
     """
@@ -41,23 +69,65 @@ def combine(*args):
             result = UNSET
         elif value is not UNSET:
             if result is UNSET:
-                try:
-                    result = value.copy()  # will shallow merge if dict-like
-                except AttributeError:
-                    result = value  # will concatenate if sequence-like
+                result = copy(value)
             else:
-                try:
-                    result.update(value)  # shallow merge if dict-like
-                except AttributeError:
-                    result = result + value  # concatenate if sequence-like
+                result.update(value)
+    return result
+
+
+def merge_dicts_deep(*args):
+    """
+    Deep-merges all non-UNSET args.
+    Does not modify any args.
+
+    >>> merge_dicts_deep({'a': 1, 'b': 2}, UNSET, {'b': 3, 'c': 4}, UNSET)
+    {'a': 1, 'b': 3, 'c': 4}
+    >>> merge_dicts_deep({'a': {'a1': 1, 'a2': 2}}, {'a': {'a1': 11, 'a3': 33}})
+    {'a': {'a1': 11, 'a2': 2, 'a3': 33}}
+    >>> merge_dicts_deep({'a': 1}, None, {'b': 2})  # None suppresses earlier args
+    {'b': 2}
+    >>> merge_dicts_deep()
+    UNSET
+
+    """
+    result = UNSET
+    for value in args:
+        if value is None:
+            # None is a request to suppress any earlier values
+            result = UNSET
+        elif value is not UNSET:
+            if result is UNSET:
+                result = deepcopy(value)
+            else:
+                update_deep(result, value)
+    return result
+
+
+def merge_dicts_one_level(*args):
+    """
+    Mixture of merge_dicts_deep and merge_dicts_shallow:
+    Deep merges first level, shallow merges second level.
+    Does not modify any args.
+
+    (Useful for {"email": {options...}, ...} style dicts,
+    like merge_data: shallow merges the options for each email.)
+    """
+    result = UNSET
+    for value in args:
+        if value is None:
+            # None is a request to suppress any earlier values
+            result = UNSET
+        elif value is not UNSET:
+            if result is UNSET:
+                result = {}
+            for k, v in value.items():
+                result.setdefault(k, {}).update(v)
     return result
 
 
 def last(*args):
     """Returns the last of its args which is not UNSET.
 
-    (Essentially `combine` without the merge behavior)
-
     >>> last(1, 2, UNSET, 3, UNSET, UNSET)
     3
     >>> last(1, 2, None, UNSET)  # None suppresses earlier args

anymail/webhooks/mailgun.py

@@ -21,8 +21,8 @@
 )
 from ..utils import (
     UNSET,
-    combine,
     get_anymail_setting,
+    merge_dicts_shallow,
     parse_single_address,
     querydict_getfirst,
 )
@@ -341,7 +341,9 @@ def _extract_legacy_metadata(self, esp_event):
             if len(variables) >= 1:
                 # Each X-Mailgun-Variables value is JSON. Parse and merge them all into
                 # single dict:
-                metadata = combine(*[json.loads(value) for value in variables])
+                metadata = merge_dicts_shallow(
+                    *[json.loads(value) for value in variables]
+                )
 
         elif event_type in self._known_legacy_event_fields:
             # For other events, we must extract from the POST fields, ignoring known

tests/test_general_backend.py

@@ -209,7 +209,10 @@ def test_esp_send_defaults(self):
                 "tags": ["globaltag"],
                 "track_clicks": True,
                 "track_opens": False,
-                "esp_extra": {"globalextra": "globalsetting"},
+                "esp_extra": {
+                    "globalextra": "globalsetting",
+                    "deepextra": {"deep1": "globaldeep1", "deep2": "globaldeep2"},
+                },
             }
         }
     )
@@ -218,7 +221,10 @@ def test_send_defaults_combine_with_message(self):
         self.message.metadata = {"message": "messagevalue", "other": "override"}
         self.message.tags = ["messagetag"]
         self.message.track_clicks = False
-        self.message.esp_extra = {"messageextra": "messagesetting"}
+        self.message.esp_extra = {
+            "messageextra": "messagesetting",
+            "deepextra": {"deep2": "messagedeep2", "deep3": "messagedeep3"},
+        }
 
         self.message.send()
         params = self.get_send_params()
@@ -234,8 +240,13 @@ def test_send_defaults_combine_with_message(self):
         self.assertEqual(params["tags"], ["globaltag", "messagetag"])
         self.assertEqual(params["track_clicks"], False)  # message overrides
         self.assertEqual(params["track_opens"], False)  # (no message setting)
+        # esp_extra is deep merged:
         self.assertEqual(params["globalextra"], "globalsetting")
         self.assertEqual(params["messageextra"], "messagesetting")
+        self.assertEqual(
+            params["deepextra"],
+            {"deep1": "globaldeep1", "deep2": "messagedeep2", "deep3": "messagedeep3"},
+        )
 
         # Send another message to make sure original SEND_DEFAULTS unchanged
         send_mail("subject", "body", "from@example.com", ["to@example.com"])
@@ -247,6 +258,9 @@ def test_send_defaults_combine_with_message(self):
         self.assertEqual(params["track_clicks"], True)
         self.assertEqual(params["track_opens"], False)
         self.assertEqual(params["globalextra"], "globalsetting")
+        self.assertEqual(
+            params["deepextra"], {"deep1": "globaldeep1", "deep2": "globaldeep2"}
+        )
 
     @override_settings(
         ANYMAIL={