Merge lp:~stevenk/launchpad/deal-with-badly-formed-section into lp:launchpad

[Steve Kowalik]

Deal with an inability to parse the file lines from the changes file, and reject the upload.

Deal with an inability to parse the file lines from the changes file. This also means we can't give any context to the user since it's far too early. Given the fields present it is only going to be component, section that are going to trip this up.

dpkg-genchanges also has a bug due to this due to accepting a Section that has a space in it.

I have tried to get this branch loc negative, but will take the ten line hit, I've clawed some of it back.

[William Grant (wgrant)]

14 + yield UploadError("Unable to parse file line, check section.")

That's pretty vague, misleading, and looks bad. Perhaps "Wrong number of fields in Files line in .changes."?

I'd also really prefer that we stopped creating malformed uploads for this. Unit tests are good.

review: Approve (code)

=== modified file 'lib/lp/archiveuploader/changesfile.py'
--- lib/lp/archiveuploader/changesfile.py	2012-06-29 08:40:05 +0000
+++ lib/lp/archiveuploader/changesfile.py	2012-09-24 01:48:20 +0000
@@ -177,8 +177,13 @@
         for fileline in self._dict['Files'].strip().split("\n"):
             # files lines from a changes file are always of the form:
             # CHECKSUM SIZE [COMPONENT/]SECTION PRIORITY FILENAME
-            digest, size, component_and_section, priority_name, filename = (
-                fileline.strip().split())
+            try:
+                digest, size, component_and_section, priority_name, filename = (
+                    fileline.strip().split())
+            except ValueError as e:
+                yield UploadError(
+                    "Wrong number of fields in Files line in .changes.")
+                continue
             filepath = os.path.join(self.dirname, filename)
             try:
                 if self.isCustom(component_and_section):

=== modified file 'lib/lp/archiveuploader/nascentuploadfile.py'
--- lib/lp/archiveuploader/nascentuploadfile.py	2012-07-24 06:39:54 +0000
+++ lib/lp/archiveuploader/nascentuploadfile.py	2012-09-24 01:48:20 +0000
@@ -335,9 +335,9 @@
         SourcePackageRelease creation, so component and section need to exist.
         Even if they might be overridden in the future.
         """
-        NascentUploadFile.__init__(
-            self, filepath, digest, size, component_and_section,
-            priority_name, policy, logger)
+        super(PackageUploadFile, self).__init__(
+            filepath, digest, size, component_and_section, priority_name,
+            policy, logger)
         self.package = package
         self.version = version
         self.changes = changes

=== added directory 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section'
=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.diff.gz'
Binary files lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.diff.gz	1970-01-01 00:00:00 +0000 and lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.diff.gz	2012-09-24 01:48:20 +0000 differ
=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.dsc'
--- lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.dsc	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1.dsc	2012-09-24 01:48:20 +0000
@@ -0,0 +1,21 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA1
+
+Format: 1.0
+Source: bar
+Version: 1.0-1
+Binary: bar
+Maintainer: Launchpad team <[e-mail]>
+Architecture: any
+Standards-Version: 3.6.2
+Files: 
+ fc1464e5985b962a042d5354452f361d 164 bar_1.0.orig.tar.gz
+ 1e35b810764f140af9616de8274e6e73 537 bar_1.0-1.diff.gz
+
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.3 (GNU/Linux)
+
+iD8DBQFFt7Cojn63CGxkqMURAo6FAJ9ZUagUNtYpmZrqFwL6LXDKOUSOPwCdFqPa
+BdrMeT+0Hg+yMS69uO+qJRI=
+=mjFU
+-----END PGP SIGNATURE-----

=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1_source.changes'
--- lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1_source.changes	1970-01-01 00:00:00 +0000
+++ lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0-1_source.changes	2012-09-24 01:48:20 +0000
@@ -0,0 +1,30 @@
+-----BEGIN PGP SIGNED MESSAGE-----
+Hash: SHA1
+
+Format: 1.7
+Date: Thu, 16 Feb 2006 15:34:09 +0000
+Source: bar
+Binary: bar
+Architecture: source
+Version: 1.0-1
+Distribution: breezy
+Urgency: low
+Maintainer: Launchpad team <[e-mail]>
+Changed-By: Daniel Silverstone <[e-mail]>
+Description: 
+ bar        - Stuff for testing
+Changes: 
+ bar (1.0-1) breezy; urgency=low
+ .
+   * Initial version
+Files: 
+ 5d533778b698edc1a122098a98c8490e 512 universe/utils, lol optional bar_1.0-1.dsc
+ fc1464e5985b962a042d5354452f361d 164 universe/utils, lol optional bar_1.0.orig.tar.gz
+ 1e35b810764f140af9616de8274e6e73 537 universe/utils, lol optional bar_1.0-1.diff.gz
+-----BEGIN PGP SIGNATURE-----
+Version: GnuPG v1.4.11 (GNU/Linux)
+
+iEYEARECAAYFAlBfq5YACgkQjn63CGxkqMWHHQCfejRqEMreaVmYyK54nnABF1mI
++40AoJq+sgIHCMR53dtQH1+axuqyHqjn
+=M3hQ
+-----END PGP SIGNATURE-----

=== added file 'lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0.orig.tar.gz'
Binary files lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0.orig.tar.gz	1970-01-01 00:00:00 +0000 and lib/lp/archiveuploader/tests/data/suite/bar_1.0-1_malformed_section/bar_1.0.orig.tar.gz	2012-09-24 01:48:20 +0000 differ
=== modified file 'lib/lp/archiveuploader/tests/test_uploadprocessor.py'
--- lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-07-03 08:04:35 +0000
+++ lib/lp/archiveuploader/tests/test_uploadprocessor.py	2012-09-24 01:48:20 +0000
@@ -221,9 +221,9 @@
     def assertLogContains(self, line):
         """Assert if a given line is present in the log messages."""
         log_lines = self.log.getLogBuffer()
-        self.assertTrue(line in log_lines,
-                        "'%s' is not in logged output\n\n%s"
-                        % (line, log_lines))
+        self.assertTrue(
+            line in log_lines, "'%s' is not in logged output\n\n%s" % (
+                line, log_lines))
 
     def assertRaisesAndReturnError(self, excClass, callableObj, *args,
                                    **kwargs):
@@ -1264,29 +1264,40 @@
         self.layer.txn.commit()
         self._uploadPartnerToNonReleasePocketAndCheckFail()
 
+    def assertRejectionMessage(self, uploadprocessor, msg, with_file=True):
+        expected = ''
+        for part in ('-1.dsc', '.orig.tar.gz', '-1.diff.gz'):
+            if with_file:
+                expected += 'bar_1.0%s: %s\n' % (part, msg)
+            else:
+                expected += '%s\n' % msg
+        expected += ('Further error processing not possible because of a '
+            'critical previous error.')
+        self.assertEqual(
+            expected, uploadprocessor.last_processed_upload.rejection_message)
+
     def testUploadWithUnknownSectionIsRejected(self):
         uploadprocessor = self.setupBreezyAndGetUploadProcessor()
         upload_dir = self.queueUpload("bar_1.0-1_bad_section")
         self.processUpload(uploadprocessor, upload_dir)
-        self.assertEqual(
-            uploadprocessor.last_processed_upload.rejection_message,
-            "bar_1.0-1.dsc: Unknown section 'badsection'\n"
-            "bar_1.0.orig.tar.gz: Unknown section 'badsection'\n"
-            "bar_1.0-1.diff.gz: Unknown section 'badsection'\n"
-            "Further error processing not possible because of a "
-            "critical previous error.")
+        self.assertRejectionMessage(
+            uploadprocessor, "Unknown section 'badsection'")
+
+    def testUploadWithMalformedSectionIsRejected(self):
+        uploadprocessor = self.setupBreezyAndGetUploadProcessor()
+        upload_dir = self.queueUpload("bar_1.0-1_malformed_section")
+        self.processUpload(uploadprocessor, upload_dir)
+        self.assertRejectionMessage(
+            uploadprocessor,
+            'Wrong number of fields in Files line in .changes.',
+            with_file=False)
 
     def testUploadWithUnknownComponentIsRejected(self):
         uploadprocessor = self.setupBreezyAndGetUploadProcessor()
         upload_dir = self.queueUpload("bar_1.0-1_contrib_component")
         self.processUpload(uploadprocessor, upload_dir)
-        self.assertEqual(
-            uploadprocessor.last_processed_upload.rejection_message,
-            "bar_1.0-1.dsc: Unknown component 'contrib'\n"
-            "bar_1.0.orig.tar.gz: Unknown component 'contrib'\n"
-            "bar_1.0-1.diff.gz: Unknown component 'contrib'\n"
-            "Further error processing not possible because of a "
-            "critical previous error.")
+        self.assertRejectionMessage(
+            uploadprocessor, "Unknown component 'contrib'")
 
     def testSourceUploadToBuilddPath(self):
         """Source uploads to buildd upload paths are not permitted."""
@@ -1340,8 +1351,6 @@
         # The component contrib does not exist in the sample data, so
         # add it here.
         Component(name='contrib')
-
-        # Test it.
         self.checkComponentOverride(
             "bar_1.0-1_contrib_component", "multiverse")
 
@@ -1350,8 +1359,6 @@
         # The component non-free does not exist in the sample data, so
         # add it here.
         Component(name='non-free')
-
-        # Test it.
         self.checkComponentOverride(
             "bar_1.0-1_nonfree_component", "multiverse")
 
@@ -1902,7 +1909,7 @@
         should both have the PGP signature removed.
         """
         uploadprocessor = self.setupBreezyAndGetUploadProcessor(
-        policy='insecure')
+            policy='insecure')
         upload_dir = self.queueUpload("bar_1.0-1")
         self.processUpload(uploadprocessor, upload_dir)
         # ACCEPT the upload