From 5598c87e6298bf41a38e2ce720d1b3264cfd0189 Mon Sep 17 00:00:00 2001 From: medmunds Date: Wed, 11 Apr 2018 11:50:06 -0700 Subject: [PATCH] Backends: identify source of problem in AnymailInvalidAddress message Include the name of the field with the the unparsable email address in AnymailInvalidAddress error messages. Should help tracking down problems like in #98. --- anymail/backends/base.py | 12 ++++++++---- anymail/utils.py | 20 ++++++++++++++------ tests/test_general_backend.py | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/anymail/backends/base.py b/anymail/backends/base.py index d8357c4..7052b05 100644 --- a/anymail/backends/base.py +++ b/anymail/backends/base.py @@ -268,7 +268,11 @@ class BasePayload(object): if converter is not None: if not callable(converter): converter = getattr(self, converter) - value = converter(value) + if converter in (parse_address_list, parse_single_address): + # hack to include field name in error message + value = converter(value, field=attr) + else: + value = converter(value) if value is not UNSET: if attr == 'body': setter = self.set_html_body if message.content_subtype == 'html' else self.set_text_body @@ -296,7 +300,7 @@ class BasePayload(object): # message.extra_headers['Reply-To'] will override message.reply_to # (because the extra_headers attr is processed after reply_to). # This matches the behavior of Django's EmailMessage.message(). - self.set_reply_to(parse_address_list([reply_to])) + self.set_reply_to(parse_address_list([reply_to], field="extra_headers['Reply-To']")) if 'From' in headers: # If message.extra_headers['From'] is supplied, it should override message.from_email, @@ -304,8 +308,8 @@ class BasePayload(object): # - https://code.djangoproject.com/ticket/9214 # - https://github.com/django/django/blob/1.8/django/core/mail/message.py#L269 # - https://github.com/django/django/blob/1.8/django/core/mail/backends/smtp.py#L118 - header_from = parse_address_list(headers.pop('From')) - envelope_sender = parse_single_address(self.message.from_email) # must be single address + header_from = parse_address_list(headers.pop('From'), field="extra_headers['From']") + envelope_sender = parse_single_address(self.message.from_email, field="from_email") # must be single self.set_from_email_list(header_from) self.set_envelope_sender(envelope_sender) diff --git a/anymail/utils.py b/anymail/utils.py index 24f6b40..f0e2c64 100644 --- a/anymail/utils.py +++ b/anymail/utils.py @@ -116,7 +116,7 @@ def update_deep(dct, other): # (like dict.update(), no return value) -def parse_address_list(address_list): +def parse_address_list(address_list, field=None): """Returns a list of EmailAddress objects from strings in address_list. Essentially wraps :func:`email.utils.getaddresses` with better error @@ -127,6 +127,8 @@ def parse_address_list(address_list): :param list[str]|str|None|list[None] address_list: the address or addresses to parse + :param str|None field: + optional description of the source of these addresses, for error message :return list[:class:`EmailAddress`]: :raises :exc:`AnymailInvalidAddress`: """ @@ -151,8 +153,11 @@ def parse_address_list(address_list): for address in parsed: if address.username == '' or address.domain == '': # Django SMTP allows username-only emails, but they're not meaningful with an ESP - errmsg = "Invalid email address '%s' parsed from '%s'." % ( - address.addr_spec, ", ".join(address_list_strings)) + errmsg = "Invalid email address '{problem}' parsed from '{source}'{where}.".format( + problem=address.addr_spec, + source=", ".join(address_list_strings), + where=" in `%s`" % field if field else "", + ) if len(parsed) > len(address_list): errmsg += " (Maybe missing quotes around a display-name?)" raise AnymailInvalidAddress(errmsg) @@ -160,17 +165,20 @@ def parse_address_list(address_list): return parsed -def parse_single_address(address): +def parse_single_address(address, field=None): """Parses a single EmailAddress from str address, or raises AnymailInvalidAddress :param str address: the fully-formatted email str to parse + :param str|None field: optional description of the source of this address, for error message :return :class:`EmailAddress`: if address contains a single email :raises :exc:`AnymailInvalidAddress`: if address contains no or multiple emails """ - parsed = parse_address_list([address]) + parsed = parse_address_list([address], field=field) count = len(parsed) if count > 1: - raise AnymailInvalidAddress("Only one email address is allowed; found %d in %r" % (count, address)) + raise AnymailInvalidAddress( + "Only one email address is allowed; found {count} in '{address}'{where}.".format( + count=count, address=address, where=" in `%s`" % field if field else "")) else: return parsed[0] diff --git a/tests/test_general_backend.py b/tests/test_general_backend.py index 91c9707..c0f4efa 100644 --- a/tests/test_general_backend.py +++ b/tests/test_general_backend.py @@ -11,7 +11,7 @@ from django.utils.functional import Promise from django.utils.timezone import utc from django.utils.translation import ugettext_lazy -from anymail.exceptions import AnymailConfigurationError, AnymailUnsupportedFeature +from anymail.exceptions import AnymailConfigurationError, AnymailInvalidAddress, AnymailUnsupportedFeature from anymail.message import AnymailMessage from .utils import AnymailTestMixin @@ -319,6 +319,38 @@ class CatchCommonErrorsTests(TestBackendTestCase): with self.assertRaisesMessage(TypeError, '"reply_to" attribute must be a list or other iterable'): self.message.send() + def test_identifies_source_of_parsing_errors(self): + """Errors parsing email addresses should say which field had the problem""" + # Note: General email address parsing tests are in test_utils.ParseAddressListTests. + # This just checks the error includes the field name when parsing for sending a message. + self.message.from_email = '' + with self.assertRaisesMessage(AnymailInvalidAddress, + "Invalid email address '' parsed from '' in `from_email`."): + self.message.send() + self.message.from_email = 'from@example.com' + + # parse_address_list + self.message.to = ['ok@example.com', 'oops'] + with self.assertRaisesMessage(AnymailInvalidAddress, + "Invalid email address 'oops' parsed from 'ok@example.com, oops' in `to`."): + self.message.send() + self.message.to = ['test@example.com'] + + # parse_single_address + self.message.envelope_sender = 'one@example.com, two@example.com' + with self.assertRaisesMessage(AnymailInvalidAddress, + "Only one email address is allowed; found 2" + " in 'one@example.com, two@example.com' in `envelope_sender`."): + self.message.send() + delattr(self.message, 'envelope_sender') + + # process_extra_headers + self.message.extra_headers['From'] = 'Mail, Inc. ' + with self.assertRaisesMessage(AnymailInvalidAddress, + "Invalid email address 'Mail' parsed from 'Mail, Inc. '" + " in `extra_headers['From']`. (Maybe missing quotes around a display-name?)"): + self.message.send() + def flatten_emails(emails): return [str(email) for email in emails]