launchpad-reviewers team mailing list archive

[Aaron Bentley <aaron@xxxxxxxxxxxxx>]

Aaron Bentley has proposed merging lp:~abentley/launchpad/duplicate-msgid into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #595166 in Launchpad itself: "IntegrityError raised filing a bug using the email interface"
  https://bugs.launchpad.net/launchpad/+bug/595166

For more details, see:
https://code.launchpad.net/~abentley/launchpad/duplicate-msgid/+merge/62697

= Summary =
Fix bug #595166: IntegrityError raised filing a bug using the email interface

== Proposed fix ==
Skip all incoming messages whose message-ids duplicate messages already in the database.

== Pre-implementation notes ==
Discussed with sinzui

== Implementation details ==
The IntegrityError comes from trying to link the same message to two different bugs.  This can happen if a message with identical contents is received twice.  While there may be times we want to permit duplicate message-ids, but not when processing incoming commands.

== Tests ==
bin/test -t duplicate_message_id test_incoming

== Demo and Q/A ==
Send an email to launchpad to create a bug.  Then re-send that message with the same message-id.  The second message should be ignored.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/mail/incoming.py
  lib/lp/services/mail/tests/test_incoming.py
  lib/lp/services/mail/tests/__init__.py
-- 
https://code.launchpad.net/~abentley/launchpad/duplicate-msgid/+merge/62697
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~abentley/launchpad/duplicate-msgid into lp:launchpad.

=== modified file 'lib/lp/services/mail/incoming.py'
--- lib/lp/services/mail/incoming.py	2011-05-09 20:04:32 +0000
+++ lib/lp/services/mail/incoming.py	2011-05-27 15:33:28 +0000
@@ -50,11 +50,13 @@
     )
 from canonical.launchpad.webapp.interfaces import IPlacelessAuthUtility
 from canonical.librarian.interfaces import UploadFailed
+from lp.app.errors import NotFoundError
 from lp.registry.interfaces.person import IPerson
 from lp.services.features import getFeatureFlag
 from lp.services.mail.handlers import mail_handlers
 from lp.services.mail.sendmail import do_paranoid_envelope_to_validation
 from lp.services.mail.signedmessage import signed_message_from_string
+from lp.services.messages.interfaces.message import IMessageSet
 
 # Match '\n' and '\r' line endings. That is, all '\r' that are not
 # followed by a '\n', and all '\n' that are not preceded by a '\r'.
@@ -424,6 +426,16 @@
 
     log.debug('processing mail from %r message-id %r' %
         (mail['from'], mail['message-id']))
+    msg_set = getUtility(IMessageSet)
+    try:
+        msg_set.get(mail['message-id'])
+    except NotFoundError:
+        pass
+    else:
+        log.warning(
+            'Message with id %s already stored.  Skipping.',
+            mail['message-id'])
+        return None
 
     # If the Return-Path header is '<>', it probably means
     # that it's a bounce from a message we sent.

=== modified file 'lib/lp/services/mail/tests/__init__.py'
--- lib/lp/services/mail/tests/__init__.py	2009-05-11 04:14:52 +0000
+++ lib/lp/services/mail/tests/__init__.py	2011-05-27 15:33:28 +0000
@@ -0,0 +1,32 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for mail services."""
+
+__metaclass__ = type
+__all__ = ['ListHandler']
+
+
+from contextlib import contextmanager
+
+from lp.services.mail.handlers import mail_handlers
+
+
+class ListHandler:
+    """A mail handler that simply adds the message to a list."""
+    def __init__(self):
+        self.processed = []
+
+    def process(self, signed_msg, to_addr, filealias=None, log=None):
+        self.processed.append(signed_msg)
+        return True
+
+
+@contextmanager
+def active_handler(domain, handler):
+    """In the context, the specified handler is registered for the domain."""
+    mail_handlers.add(domain, handler)
+    try:
+        yield handler
+    finally:
+        del mail_handlers._handlers[domain]

=== modified file 'lib/lp/services/mail/tests/test_incoming.py'
--- lib/lp/services/mail/tests/test_incoming.py	2011-05-07 15:35:16 +0000
+++ lib/lp/services/mail/tests/test_incoming.py	2011-05-27 15:33:28 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009, 2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 from doctest import DocTestSuite
@@ -6,7 +6,11 @@
 import os
 import unittest
 
-from testtools.matchers import Equals, Is
+from testtools.matchers import (
+    Equals,
+    Is,
+    MatchesRegex,
+    )
 import transaction
 from zope.security.management import setSecurityPolicy
 
@@ -24,11 +28,17 @@
     authenticateEmail,
     extract_addresses,
     handleMail,
+    handle_one_mail,
     MailErrorUtility,
     ORIGINAL_TO_HEADER,
     )
 from lp.services.mail.sendmail import MailController
 from lp.services.mail.stub import TestMailer
+from lp.services.messages.model.message import MessageSet
+from lp.services.mail.tests import (
+    active_handler,
+    ListHandler,
+    )
 from lp.testing import TestCaseWithFactory
 from lp.testing.factory import GPGSigningContext
 from lp.testing.mail_helpers import pop_notifications
@@ -122,6 +132,25 @@
         mail = self.factory.makeSignedMessage(email_address=email)
         self.assertThat(authenticateEmail(mail), Is(None))
 
+    def test_handle_one_mail_duplicate_message_id(self):
+        # An incoming mail with the message-id of an already-processed mail is
+        # skipped.
+        orig_message = self.factory.makeMessage()
+        handler_calls = []
+        self.assertEqual([], handler_calls)
+        duplicate_mail = self.factory.makeSignedMessage(
+            msgid=orig_message.rfc822msgid,
+            to_address='new@xxxxxxxxxxxxxxxxxx')
+        log = BufferLogger()
+        with active_handler('random.example.com', ListHandler()) as handler:
+            handle_one_mail(log, duplicate_mail, None, None, None)
+        self.assertEqual([], handler.processed)
+        messages = MessageSet().get(orig_message.rfc822msgid)
+        self.assertEqual(1, len(messages))
+        matches = MatchesRegex(
+            '(.|\n)*WARNING Message with id .* already stored.  Skipping.')
+        self.assertThat(log.getLogBuffer(), matches)
+
 
 class TestExtractAddresses(TestCaseWithFactory):