python / cpython

The Python programming language
https://www.python.org
Other
62.34k stars 29.94k forks source link

[smtplib] collect response data for all recipients #73725

Open 2b967bd1-2db1-4873-8b2b-6c63ad684fee opened 7 years ago

2b967bd1-2db1-4873-8b2b-6c63ad684fee commented 7 years ago
BPO 29539
Nosy @warsaw, @bitdancer, @FirefighterBlu3, @Windsooon, @sls89
PRs
  • python/cpython#12148
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields: ```python assignee = None closed_at = None created_at = labels = ['type-feature', '3.8', 'expert-email'] title = '[smtplib] collect response data for all recipients' updated_at = user = 'https://github.com/FirefighterBlu3' ``` bugs.python.org fields: ```python activity = actor = 'sls' assignee = 'none' closed = False closed_date = None closer = None components = ['email'] creation = creator = 'David Ford (FirefighterBlu3)' dependencies = [] files = [] hgrepos = [] issue_num = 29539 keywords = ['patch'] message_count = 9.0 messages = ['287662', '318518', '318706', '318723', '336876', '336886', '336894', '336960', '337040'] nosy_count = 5.0 nosy_names = ['barry', 'r.david.murray', 'David Ford (FirefighterBlu3)', 'Windson Yang', 'sls'] pr_nums = ['12148'] priority = 'normal' resolution = None stage = 'patch review' status = 'open' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue29539' versions = ['Python 3.8'] ```

    2b967bd1-2db1-4873-8b2b-6c63ad684fee commented 7 years ago

    Feature request; collect SMTP response results for all recipients so data likely including the queue ID or SMTP delay expectation can be collected

    I propose the keyword "keep_results=False" be added to smtplib.sendmail() to accomplish this. The default value of False maintains the legacy functionality of prior versions. No other changes have been done to smtplib.send_message() pending discussion of the following.

    @@ -785,7 +785,7 @@
             return (resp, reply)
    
         def sendmail(self, from_addr, to_addrs, msg, mail_options=[],
    -                 rcpt_options=[]):
    +                 rcpt_options=[], keep_results=False):
             """This command performs an entire mail transaction.
    
             The arguments are:
    @@ -797,6 +797,8 @@
                                  mail command.
                 - rcpt_options : List of ESMTP options (such as DSN commands) for
                                  all the rcpt commands.
    +            - keep_results : If True, return a dictionary of recipients and the
    +                             SMTP result for each.
    
             msg may be a string containing characters in the ASCII range, or a byte
             string.  A string is encoded to bytes using the ascii codec, and lone
    @@ -807,17 +809,20 @@
             and each of the specified options will be passed to it.  If EHLO
             fails, HELO will be tried and ESMTP options suppressed.
    
    -        This method will return normally if the mail is accepted for at least
    -        one recipient.  It returns a dictionary, with one entry for each
    -        recipient that was refused.  Each entry contains a tuple of the SMTP
    -        error code and the accompanying error message sent by the server.
    +        If keep_results is False, this method will return normally if the mail
    +        is accepted for at least one recipient.  It returns a dictionary, with
    +        one entry for each recipient that was refused.  Each entry contains a
    +        tuple of the SMTP error code and the accompanying error message sent by
    +        the server.  If keep_results is True, this method returns a dictionary
    +        of all recipients and the SMTP result whether refused or accepted
    +        unless all are refused then the normal exception is raised.
    
             This method may raise the following exceptions:
    
              SMTPHeloError          The server didn't reply properly to
                                     the helo greeting.
    -         SMTPRecipientsRefused  The server rejected ALL recipients
    -                                (no mail was sent).
    +         SMTPRecipientsRefused  The server rejected ALL recipients (no mail
    +                                was sent).
              SMTPSenderRefused      The server didn't accept the from_addr.
              SMTPDataError          The server replied with an unexpected
                                     error code (other than a refusal of
    @@ -879,6 +884,10 @@
                 self._rset()
                 raise SMTPRecipientsRefused(senderrs)
             (code, resp) = self.data(msg)
    +        if keep_results:
    +            for each in to_addrs:
    +                if not each in senderrs:
    +                    senderrs[each]=(code, resp)
             if code != 250:
                 if code == 421:
                     self.close()
    2b967bd1-2db1-4873-8b2b-6c63ad684fee commented 6 years ago

    3.7 release candidates are in the queue, any thoughts on this simple enhancement?

    warsaw commented 6 years ago

    It's too late for 3.7, but something like this could be an interesting enhancement for 3.8. I'm not so sure about the name of the suggested parameter since it seems more about recording successful deliveries in addition to the normally failed deliveries. But we can quibble about that later.

    Would you be willing and able to turn your patch into a pull request against our GitHub repo? You would need to sign the CLA. We're also going to want a test, some additional documentation, and a news entry.

    See devguide.python.org for details.

    2b967bd1-2db1-4873-8b2b-6c63ad684fee commented 6 years ago

    Yes, it is distinctly intended to collect the results for every recipient as in modern MTAs, there's more than just success/fail results. This is to collect data such as queue ID and the MTA's programmatic response for each individual recipient. I have several needs of knowing if the message was immediately relayed, queued for later because of a remote issue, queued because of MTA issue, graylisted or blocked because of milter reasons, or ... any of a list of failure reasons.

    This data can be collected if there is only one recipient per message, but that is considerably resource expensive.

    Without this patch, when sending to say 100 recipients, the only response returned to the caller is the very last (100th) result. A 250 then assumes that all 100 were transmitted successfully when in truth, the first 99 could have failed.

    Yes, I'll make a PR, do the CLA, and add some tests.

    70c5da61-99e4-4e03-8993-245df88cdc07 commented 5 years ago

    Did you make a PR ? I didn't notice this issue and did a quick fix: https://github.com/python/cpython/pull/12104

    I was hoping this or similar fixes would be implemented in 3.7.x

    Anyways. I'd suggest to return both success and errors for each recipient. sendmail returns a dict already. Why not adding each status?

    Maybe using something like "mta_result" as variable instead of senderrs, which is in my opinion more clear to understand. Also, this would have the advantage to parse the return value easily and you're able to access more verbose information like mta msg id and so on.

    3db4c488-648f-4df6-97a9-da1ac9fc355c commented 5 years ago

    sls, are you working on this feature now?

    2b967bd1-2db1-4873-8b2b-6c63ad684fee commented 5 years ago

    i have a fully built patch and personally tested (i use it 24/7) but haven't done test_* yet as was requested

    On Thu, Feb 28, 2019 at 9:16 PM Windson Yang \report@bugs.python.org\ wrote:

    Windson Yang \wiwindson@outlook.com\ added the comment:

    sls, are you working on this feature now?

    ---------- nosy: +Windson Yang


    Python tracker \report@bugs.python.org\ \https://bugs.python.org/issue29539\


    70c5da61-99e4-4e03-8993-245df88cdc07 commented 5 years ago

    I closed my PR. I'd just rename "senderrs" to "mta_status" or so as aforementioned change means not just storing errors.

    FirefighterBlu3, do you open a PR for this? I'd like to see this moving into 3.8 as we don't compile Python from source but using it from deb repos.

    70c5da61-99e4-4e03-8993-245df88cdc07 commented 5 years ago

    I opened a new PR.

    I picked up some of FirefighterBlu3's suggestions and added some unittests and refactoring to assist. Hope this helps.