launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/more-log-parser-fixes into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #705469 PPA log parser needs to handle URL-quoted paths
  https://bugs.launchpad.net/bugs/705469
  #705489 PPA log parser needs to normalise paths
  https://bugs.launchpad.net/bugs/705489
  #705564 Archive.getBinaryPackageReleaseByFileName is slow
  https://bugs.launchpad.net/bugs/705564

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/more-log-parser-fixes/+merge/46951

This branch fixes a couple of bugs found while QAing the PPA log parser on dogfood:

 - Bug #705469: It did not handle URL-quoted paths, so binary versions containing tildes were ignored. Paths are now URL-unquoted.
 - Bug #705489: It did not normalise slashes in paths, so requests containing double slashes were ignored. Slashes are now normalised.
 - Bug #705564: Archive.getBinaryPackageReleaseByFileName is slow, as it attempts to avoid adding download counts where the target file is ambiguous (something that is now impossible, but some historical cases remain). It now just uses the latest available file, avoiding a COUNT.
-- 
https://code.launchpad.net/~wgrant/launchpad/more-log-parser-fixes/+merge/46951
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/more-log-parser-fixes into lp:launchpad.

=== modified file 'lib/lp/soyuz/model/archive.py'
--- lib/lp/soyuz/model/archive.py	2011-01-15 06:32:40 +0000
+++ lib/lp/soyuz/model/archive.py	2011-01-20 18:50:31 +0000
@@ -1373,7 +1373,7 @@
 
     def getBinaryPackageReleaseByFileName(self, filename):
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
-        results = store.find(
+        return store.find(
             BinaryPackageRelease,
             BinaryPackageFile.binarypackagereleaseID ==
                 BinaryPackageRelease.id,
@@ -1381,10 +1381,8 @@
             LibraryFileAlias.filename == filename,
             BinaryPackagePublishingHistory.archive == self,
             BinaryPackagePublishingHistory.binarypackagereleaseID ==
-                BinaryPackageRelease.id).config(distinct=True)
-        if results.count() > 1:
-            return None
-        return results.one()
+                BinaryPackageRelease.id,
+            ).order_by(Desc(BinaryPackagePublishingHistory.id)).first()
 
     def requestPackageCopy(self, target_location, requestor, suite=None,
         copy_binaries=False, reason=None):

=== modified file 'lib/lp/soyuz/scripts/ppa_apache_log_parser.py'
--- lib/lp/soyuz/scripts/ppa_apache_log_parser.py	2011-01-05 04:54:50 +0000
+++ lib/lp/soyuz/scripts/ppa_apache_log_parser.py	2011-01-20 18:50:31 +0000
@@ -3,6 +3,9 @@
 
 __all__ = ['DBUSER', 'get_ppa_file_key']
 
+import os.path
+import urllib
+
 from lp.archiveuploader.utils import re_isadeb
 
 
@@ -10,7 +13,7 @@
 
 
 def get_ppa_file_key(path):
-    split_path = path.split('/')
+    split_path = os.path.normpath(urllib.unquote(path)).split('/')
     if len(split_path) != 9:
         return None
 

=== modified file 'lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py'
--- lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py	2011-01-05 04:55:14 +0000
+++ lib/lp/soyuz/scripts/tests/test_ppa_apache_log_parser.py	2011-01-20 18:50:31 +0000
@@ -34,7 +34,6 @@
         # distribution and file names.
         archive_owner, archive_name, distro_name, filename = get_ppa_file_key(
             '/cprov/ppa/ubuntu/pool/main/f/foo/foo_1.2.3-4_i386.deb')
-
         self.assertEqual(archive_owner, 'cprov')
         self.assertEqual(archive_name, 'ppa')
         self.assertEqual(distro_name, 'ubuntu')
@@ -53,6 +52,22 @@
         self.assertIs(None, get_ppa_file_key(
             '/cprov/ppa/ubuntu/pool/main/f/foo/foo_1.2.3-4.dsc'))
 
+    def test_get_ppa_file_key_unquotes_path(self):
+        archive_owner, archive_name, distro_name, filename = get_ppa_file_key(
+            '/cprov/ppa/ubuntu/pool/main/f/foo/foo_1.2.3%7E4_i386.deb')
+        self.assertEqual(archive_owner, 'cprov')
+        self.assertEqual(archive_name, 'ppa')
+        self.assertEqual(distro_name, 'ubuntu')
+        self.assertEqual(filename, 'foo_1.2.3~4_i386.deb')
+
+    def test_get_ppa_file_key_normalises_path(self):
+        archive_owner, archive_name, distro_name, filename = get_ppa_file_key(
+            '/cprov/ppa/ubuntu/pool//main/f///foo/foo_1.2.3-4_i386.deb')
+        self.assertEqual(archive_owner, 'cprov')
+        self.assertEqual(archive_name, 'ppa')
+        self.assertEqual(distro_name, 'ubuntu')
+        self.assertEqual(filename, 'foo_1.2.3-4_i386.deb')
+
 
 class TestScriptRunning(TestCaseWithFactory):
     """Run parse-ppa-apache-access-logs.py and test its outcome."""

=== modified file 'lib/lp/soyuz/tests/test_archive.py'
--- lib/lp/soyuz/tests/test_archive.py	2011-01-14 03:28:56 +0000
+++ lib/lp/soyuz/tests/test_archive.py	2011-01-20 18:50:31 +0000
@@ -1157,19 +1157,19 @@
             self.archive.getBinaryPackageReleaseByFileName(
                 "this-is-not-real_1.2.3-4_all.deb"))
 
-    def test_returns_none_for_duplicate_file(self):
+    def test_returns_latest_for_duplicate_file(self):
         # In the unlikely case of multiple BPRs in this archive with the same
         # name (hopefully impossible, but it still happens occasionally due
-        # to bugs), None is returned.
+        # to bugs), the latest is returned.
 
         # Publish the same binaries again. Evil.
-        self.publisher.getPubBinaries(
+        new_pubs = self.publisher.getPubBinaries(
             version="1.2.3-4", archive=self.archive, binaryname="foo-bin",
             status=PackagePublishingStatus.PUBLISHED,
             architecturespecific=True)
 
-        self.assertIs(
-            None,
+        self.assertEquals(
+            new_pubs[0].binarypackagerelease,
             self.archive.getBinaryPackageReleaseByFileName(
                 "foo-bin_1.2.3-4_i386.deb"))