diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 05550dfb..1a016321 100644 --- a/CHANGELOG.rst +++ b/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 diff --git a/anymail/backends/base.py b/anymail/backends/base.py index 87bbf1e2..35ed3897 100644 --- a/anymail/backends/base.py +++ b/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 diff --git a/anymail/utils.py b/anymail/utils.py index 464152bf..da804f8a 100644 --- a/anymail/utils.py +++ b/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 diff --git a/anymail/webhooks/mailgun.py b/anymail/webhooks/mailgun.py index 4d54e35e..a9f4949d 100644 --- a/anymail/webhooks/mailgun.py +++ b/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 diff --git a/tests/test_general_backend.py b/tests/test_general_backend.py index d0d55d64..119a5bc4 100644 --- a/tests/test_general_backend.py +++ b/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={ diff --git a/tests/test_utils.py b/tests/test_utils.py index a3e8a6fd..bbae407b 100644 --- a/tests/test_utils.py +++ b/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 @@ 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 @@ def test_unset_has_useful_repr(self): 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)