Merge lp:~jtv/launchpad/bug-876348 into lp:launchpad

[Jeroen T. Vermeulen]

Turn archive-uploader InvalidEmailAddress errors into UploadErrors.

One possible solution for bug 876348: make DSCFile.parseAddress do what the docstring says it will in this case — raise UploadError if the email address is malformed. This particular type of exception should be handled a bit more robustly.

[Julian Edwards (julian-edwards)]

You need to raise EarlyReturnUploadError and it should work. UploadError indicates a non-replyable catastrophic error.

[Jeroen T. Vermeulen (jtv)]

From IRC (times in UTC):

(13:36:37) bigjools: jtv: actually EarlyReturn is not ideal thinking about it
(13:36:37) bigjools: we just need to add a rejection and let processing continue so it collects all the errors
(13:37:19) bigjools: I really detest the upload processor
(13:47:51) jtv: bigjools: I guess it's two problems: one, “malformed email address and policy that says to create the user” does something very different from “unknown email address and policy that says not to create the user” or “malformed email address and policy that says not to create the user.” Two, it'd be nice to have more robust handling of these error conditions.
(13:47:59) jtv: Does that make sense?

[Jeroen T. Vermeulen (jtv)]

(13:51:12) bigjools: jtv: no
(13:51:21) bigjools: but then I am trying to do 2 things at once here
(13:52:04) jtv: OK — it's late here anyway, so we can leave this to ferment in the hindbrain for now.

=== modified file 'lib/lp/archiveuploader/dscfile.py'
--- lib/lp/archiveuploader/dscfile.py	2011-05-20 13:36:11 +0000
+++ lib/lp/archiveuploader/dscfile.py	2011-10-20 12:11:38 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2010 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).
 
 """ DSCFile and related.
@@ -29,6 +29,7 @@
 from debian.deb822 import Deb822Dict
 from zope.component import getUtility
 
+from canonical.launchpad.interfaces.emailaddress import InvalidEmailAddress
 from canonical.launchpad.interfaces.gpghandler import (
     GPGVerificationError,
     IGPGHandler,
@@ -203,18 +204,25 @@
             package = self._dict['Source']
             version = self._dict['Version']
             if self.policy.distroseries and self.policy.pocket:
-                policy_suite = ('%s/%s' % (self.policy.distroseries.name,
-                                           self.policy.pocket.name))
+                policy_suite = '/'.join([
+                    self.policy.distroseries.name,
+                    self.policy.pocket.name,
+                    ])
             else:
                 policy_suite = '(unknown)'
-            person = getUtility(IPersonSet).ensurePerson(
-                email, name, PersonCreationRationale.SOURCEPACKAGEUPLOAD,
-                comment=('when the %s_%s package was uploaded to %s'
-                         % (package, version, policy_suite)))
+            comment = "when the %s_%s package was uploaded to %s" % (
+                package, version, policy_suite)
+            try:
+                person = getUtility(IPersonSet).ensurePerson(
+                    email, name, PersonCreationRationale.SOURCEPACKAGEUPLOAD,
+                    comment=comment)
+            except InvalidEmailAddress:
+                self.logger.info("Invalid email address: '%s'", email)
+                person = None
 
         if person is None:
-            raise UploadError("Unable to identify '%s':<%s> in launchpad"
-                              % (name, email))
+            raise UploadError(
+                "Unable to identify '%s':<%s> in launchpad" % (name, email))
 
         return {
             "rfc822": rfc822,
@@ -234,7 +242,8 @@
         "Binary",
         "Maintainer",
         "Architecture",
-        "Files"])
+        "Files",
+        ])
 
     known_fields = mandatory_fields.union(set([
         "Build-Depends",
@@ -288,7 +297,6 @@
             raise EarlyReturnUploadError(
                 "Unsupported source format: %s" % self._dict['Format'])
 
-
     #
     # Useful properties.
     #

=== modified file 'lib/lp/archiveuploader/tests/test_dscfile.py'
--- lib/lp/archiveuploader/tests/test_dscfile.py	2011-03-21 12:55:50 +0000
+++ lib/lp/archiveuploader/tests/test_dscfile.py	2011-10-20 12:11:38 +0000
@@ -5,21 +5,24 @@
 
 __metaclass__ = type
 
+from collections import namedtuple
 import os
 
-from canonical.testing.layers import LaunchpadZopelessLayer
+from canonical.testing.layers import LaunchpadZopelessLayer, ZopelessDatabaseLayer
 from lp.archiveuploader.dscfile import (
     cleanup_unpacked_dir,
     DSCFile,
     find_changelog,
     find_copyright,
     format_to_file_checker_map,
+    SignableTagFile,
     unpack_source,
     )
 from lp.archiveuploader.nascentuploadfile import UploadError
 from lp.archiveuploader.tests import datadir
 from lp.archiveuploader.uploadpolicy import BuildDaemonUploadPolicy
 from lp.registry.interfaces.sourcepackage import SourcePackageFileType
+from lp.registry.model.person import Person
 from lp.services.log.logger import BufferLogger, DevNullLogger
 from lp.soyuz.enums import SourcePackageFormat
 from lp.testing import (
@@ -316,3 +319,40 @@
         os.chmod(unpacked_dir, 0600)
         cleanup_unpacked_dir(unpacked_dir)
         self.assertFalse(os.path.exists(unpacked_dir))
+
+
+class TestSignableTagFile(TestCaseWithFactory):
+    """Test `SignableTagFile`, a helper mixin."""
+
+    layer = ZopelessDatabaseLayer
+
+    def makeSignableTagFile(self):
+        """Create a minimal `SignableTagFile` object."""
+        FakePolicy = namedtuple(
+            'FakePolicy',
+            ['pocket', 'distroseries', 'create_people'])
+        tagfile = SignableTagFile()
+        tagfile.logger = DevNullLogger()
+        tagfile.policy = FakePolicy(None, None, create_people=True)
+        tagfile._dict = {
+            'Source': 'arbitrary-source-package-name',
+            'Version': '1.0',
+            }
+        return tagfile
+
+    def test_parseAddress_finds_addressee(self):
+        tagfile = self.makeSignableTagFile()
+        email = self.factory.getUniqueEmailAddress()
+        person = self.factory.makePerson(email=email)
+        self.assertEqual(person, tagfile.parseAddress(email)['person'])
+
+    def test_parseAddress_creates_addressee_for_unknown_address(self):
+        unknown_email = self.factory.getUniqueEmailAddress()
+        results = self.makeSignableTagFile().parseAddress(unknown_email)
+        self.assertEqual(unknown_email, results['email'])
+        self.assertIsInstance(results['person'], Person)
+
+    def test_parseAddress_raises_UploadError_if_address_is_malformed(self):
+        self.assertRaises(
+            UploadError,
+            self.makeSignableTagFile().parseAddress, "invalid@bad-address")