sklarsa / django-sendgrid-v5

An implementation of Django's EmailBackend compatible with sendgrid-python v5+
MIT License
318 stars 54 forks source link

Improved logging when Sendgrid returns bad request #81

Closed MattFanto closed 3 years ago

MattFanto commented 3 years ago

While using this library I faced an issue with Sendgrid I had some duplicates across to, cc and bcc.

E.g. to = ["test@test.com"] cc = ["test@test.com"] bcc = ["test@test.com"]

Unfortunately, the printed log is a non-helpful "HTTP 400 Bad request" which made debugging really difficult while the response body from Sendgrid was really helpful to understand the issue

exc.body: {"errors":[{"message":"Each email address in the personalization block should be unique between to, cc, and bcc. We found the first duplicate instance of [test@test.com] in the personalizations.0.cc field.","field":"personalizations.0","help":"http://sendgrid.com/docs/API_Reference/Web_API_v3/Mail/errors.html#message.recipient-errors"}]}

Compare to sendgrid django doesn't raise an issue for this kind of scenario, if it could be helpful I can add as part of this PR few lines of code to remove duplicates across bcc cc and to, e.g.:

_bcc = list(set(bcc) - set(cc) - set(to))
_cc = list(set(cc) - set(to))
if len(bcc) != len(_bcc) or len(cc) != len(_cc):
    logger.warning("Not unique address between to, cc and bcc (%s, %s, %s)" % (to, cc, bcc))
    bcc = _bcc
    cc = _cc

But I'm not sure if this is can be helpful

sklarsa commented 3 years ago

Why a warning and not an error? I'm always a fan of clearer logging, and this seems like something that's easy to check. We know we're going to raise a 400 when we try to deliver the email so why not just raise an error first?

MattFanto commented 3 years ago

You throwing an error? Well I think that in that case the error from SendGrid would more self-explanatory and easier to maintain. If you mean instead logger.error sure probably more appropriate, but still not sure if this pre-check is really useful

sklarsa commented 3 years ago

Thanks for your contribution! I'm making a release now