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 ..signals import post_send, pre_send
 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),
+        ("to", concat_lists, parse_address_list),
-        ("cc", combine, parse_address_list),
+        ("cc", concat_lists, parse_address_list),
-        ("bcc", combine, parse_address_list),
+        ("bcc", concat_lists, parse_address_list),
         ("subject", last, force_non_lazy),
-        ("reply_to", combine, parse_address_list),
+        ("reply_to", concat_lists, parse_address_list),
-        ("extra_headers", combine, force_non_lazy_dict),
+        ("extra_headers", merge_dicts_shallow, force_non_lazy_dict),
         ("body", last, force_non_lazy),  # set_body handles content_subtype
-        ("alternatives", combine, "prepped_alternatives"),
+        ("alternatives", concat_lists, "prepped_alternatives"),
-        ("attachments", combine, "prepped_attachments"),
+        ("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_data", merge_dicts_one_level, force_non_lazy_dict),
-        ("merge_global_data", combine, force_non_lazy_dict),
+        ("merge_global_data", merge_dicts_shallow, force_non_lazy_dict),
-        ("merge_metadata", combine, force_non_lazy_dict),
+        ("merge_metadata", merge_dicts_one_level, force_non_lazy_dict),
-        ("esp_extra", combine, 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 base64
 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,16 @@ BASIC_NUMERIC_TYPES = (int, float)
 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)
+    >>> concat_lists([1, 2], UNSET, [3, 4], UNSET)
-    {'a': 1, 'b': 3, 'c': 4}
-    >>> combine([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
-    {'b': 2}
+    [3, 4]
-    >>> combine()
+    >>> concat_lists()
     UNSET
 
     """
@@ -41,23 +41,93 @@ def combine(*args):
             result = UNSET
         elif value is not UNSET:
             if result is UNSET:
-                try:
+                result = list(value)
-                    result = value.copy()  # will shallow merge if dict-like
-                except AttributeError:
-                    result = value  # will concatenate if sequence-like
             else:
-                try:
+                result = result + list(value)  # concatenate sequence-like
-                    result.update(value)  # shallow merge if dict-like
+    return result
-                except AttributeError:
+
-                    result = result + value  # concatenate if sequence-like
+
+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}
+    >>> merge_dicts_shallow()
+    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 = copy(value)
+            else:
+                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 ..signals import (
 )
 from ..utils import (
     UNSET,
-    combine,
     get_anymail_setting,
+    merge_dicts_shallow,
     parse_single_address,
     querydict_getfirst,
 )
@@ -341,7 +341,9 @@ class MailgunTrackingWebhookView(MailgunBaseWebhookView):
             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 @@ class SendDefaultsTests(TestBackendTestCase):
                 "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 @@ class SendDefaultsTests(TestBackendTestCase):
         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 @@ class SendDefaultsTests(TestBackendTestCase):
         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 @@ class SendDefaultsTests(TestBackendTestCase):
         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={

tests/test_utils.py

@@ -1,5 +1,5 @@
 # Tests for the anymail/utils.py module
-# (not to be confused with utilities for testing found in in tests/utils.py)
+# (not to be confused with utilities for testing found in tests/utils.py)
 import base64
 import copy
 import pickle
@@ -17,12 +17,17 @@ from anymail.utils import (
     Attachment,
     CaseInsensitiveCasePreservingDict,
     EmailAddress,
+    concat_lists,
     force_non_lazy,
     force_non_lazy_dict,
     force_non_lazy_list,
     get_request_basic_auth,
     get_request_uri,
     is_lazy,
+    last,
+    merge_dicts_deep,
+    merge_dicts_one_level,
+    merge_dicts_shallow,
     parse_address_list,
     parse_rfc2822date,
     parse_single_address,
@@ -559,3 +564,133 @@ class UnsetValueTests(SimpleTestCase):
     def test_equality(self):
         # `is UNSET` is preferred to `== UNSET`, but both should work
         self.assertEqual(UNSET, UNSET)
+
+
+class CombinerTests(SimpleTestCase):
+    def test_concat_lists(self):
+        for args, expected in [
+            (([1, 2], [3, 4]), [1, 2, 3, 4]),
+            # Does not flatten:
+            (([1, [11, 12]], [2]), [1, [11, 12], 2]),
+            # UNSET args ignored:
+            ((UNSET, [1, 2], UNSET, [3, 4], UNSET), [1, 2, 3, 4]),
+            # None clears previous:
+            (([1, 2], None, [3, 4]), [3, 4]),
+            # Works with other sequence-like types:
+            (([1], (2, 3), {4}), [1, 2, 3, 4]),
+            # Degenerate cases:
+            ((), UNSET),
+            ((UNSET,), UNSET),
+            ((None,), UNSET),
+            (([], None), UNSET),
+        ]:
+            with self.subTest(repr(args)):
+                original_args = copy.deepcopy(args)
+                merged = concat_lists(*args)
+                self.assertEqual(merged, expected)
+                # Verify args were not modified:
+                self.assertEqual(args, original_args)
+
+    def test_merge_dicts_shallow(self):
+        for args, expected in [
+            (({"a": 1}, {"b": 2}), {"a": 1, "b": 2}),
+            (
+                ({"a": 1, "b": 2}, {"a": 11, "c": 33}, {"c": 3}),
+                {"a": 11, "b": 2, "c": 3},
+            ),
+            # shallow merge:
+            (({"a": {"a1": 1}, "b": 2}, {"a": {"a2": 2}}), {"a": {"a2": 2}, "b": 2}),
+            # UNSET args ignored:
+            ((UNSET, {"a": 1}, UNSET, {"b": 2}, UNSET), {"a": 1, "b": 2}),
+            # None clears previous:
+            (({"a": 1}, None, {"b": 2}), {"b": 2}),
+            # Degenerate cases:
+            ((), UNSET),
+            ((UNSET,), UNSET),
+            ((None,), UNSET),
+            (({}, None), UNSET),
+        ]:
+            with self.subTest(repr(args)):
+                original_args = copy.deepcopy(args)
+                merged = merge_dicts_shallow(*args)
+                self.assertEqual(merged, expected)
+                # Verify args were not modified:
+                self.assertEqual(args, original_args)
+
+    def test_merge_dicts_deep(self):
+        for args, expected in [
+            (({"a": 1}, {"b": 2}), {"a": 1, "b": 2}),
+            (
+                ({"a": 1, "b": 2}, {"a": 11, "c": 33}, {"c": 3}),
+                {"a": 11, "b": 2, "c": 3},
+            ),
+            # deep merge:
+            (
+                (
+                    {"a": {"a1": 1, "a3": {"a3a": 31}}},
+                    {"a": {"a2": 2, "a3": {"a3b": 32}}},
+                ),
+                {"a": {"a1": 1, "a2": 2, "a3": {"a3a": 31, "a3b": 32}}},
+            ),
+            # UNSET (top-level) args ignored:
+            ((UNSET, {"a": 1}, UNSET, {"b": 2}, UNSET), {"a": 1, "b": 2}),
+            # None clears previous:
+            (({"a": 1}, None, {"b": 2}), {"b": 2}),
+            # Degenerate cases:
+            ((), UNSET),
+            ((UNSET,), UNSET),
+            ((None,), UNSET),
+            (({}, None), UNSET),
+        ]:
+            with self.subTest(repr(args)):
+                original_args = copy.deepcopy(args)
+                merged = merge_dicts_deep(*args)
+                self.assertEqual(merged, expected)
+                # Verify args were not modified:
+                self.assertEqual(args, original_args)
+
+    def test_merge_dicts_one_level(self):
+        for args, expected in [
+            # one-level merge:
+            (
+                (
+                    {"a": {"a1": 1, "a3": {"a3a": 31}}},
+                    {"a": {"a2": 2, "a3": {"a3b": 32}}},
+                ),
+                {"a": {"a1": 1, "a2": 2, "a3": {"a3b": 32}}},  # but not a3a
+            ),
+            # UNSET (top-level) args ignored:
+            ((UNSET, {"a": {}}, UNSET, {"b": {}}, UNSET), {"a": {}, "b": {}}),
+            # None clears previous:
+            (({"a": {}}, None, {"b": {}}), {"b": {}}),
+            # Degenerate cases:
+            ((), UNSET),
+            ((UNSET,), UNSET),
+            ((None,), UNSET),
+            (({}, None), UNSET),
+        ]:
+            with self.subTest(repr(args)):
+                original_args = copy.deepcopy(args)
+                merged = merge_dicts_one_level(*args)
+                self.assertEqual(merged, expected)
+                # Verify args were not modified:
+                self.assertEqual(args, original_args)
+
+    def test_last(self):
+        for args, expected in [
+            ((1, 2, 3), 3),
+            # UNSET args ignored:
+            ((UNSET, 1, UNSET, 2, UNSET), 2),
+            # None clears previous:
+            ((1, 2, None), UNSET),
+            # Degenerate cases:
+            ((), UNSET),
+            ((UNSET,), UNSET),
+            ((None,), UNSET),
+        ]:
+            with self.subTest(repr(args)):
+                original_args = copy.deepcopy(args)
+                merged = last(*args)
+                self.assertEqual(merged, expected)
+                # Verify args were not modified:
+                self.assertEqual(args, original_args)