launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-607954 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #607954 in Launchpad itself: "timeout on DistributionSourcePackage:+changelog"
  https://bugs.launchpad.net/launchpad/+bug/607954

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-607954/+merge/53567

DistributionSourcePackage:+changelog loads all relevant PackageDiffs and then goes on to show their titles. But this calculation requires both involved SPRs, their upload archives, their distributions, and the diff's library file. Only the library files are problematic at the moment, since the page already has most of the other data. This will change once derivation is in use, and we have inter-distro SPRs and diffs.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-607954/+merge/53567
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-607954 into lp:launchpad.

=== modified file 'lib/lp/registry/browser/distributionsourcepackage.py'
--- lib/lp/registry/browser/distributionsourcepackage.py	2011-01-21 08:12:29 +0000
+++ lib/lp/registry/browser/distributionsourcepackage.py	2011-03-16 04:27:31 +0000
@@ -275,7 +275,8 @@
         else:
             self._person_data = None
         # Collate diffs for relevant SourcePackageReleases
-        pkg_diffs = getUtility(IPackageDiffSet).getDiffsToReleases(sprs)
+        pkg_diffs = getUtility(IPackageDiffSet).getDiffsToReleases(
+            sprs, preload_for_display=True)
         spr_diffs = {}
         for spr, diffs in itertools.groupby(pkg_diffs,
                                             operator.attrgetter('to_source')):

=== modified file 'lib/lp/registry/browser/tests/test_distributionsourcepackage.py'
--- lib/lp/registry/browser/tests/test_distributionsourcepackage.py	2010-10-04 19:50:45 +0000
+++ lib/lp/registry/browser/tests/test_distributionsourcepackage.py	2011-03-16 04:27:31 +0000
@@ -8,8 +8,17 @@
 from zope.component import getUtility
 
 from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
-from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.testing import test_tales, TestCaseWithFactory
+from canonical.testing.layers import (
+    DatabaseFunctionalLayer,
+    LaunchpadFunctionalLayer,
+    )
+from lp.soyuz.interfaces.archive import ArchivePurpose
+from lp.testing import (
+    celebrity_logged_in,
+    test_tales,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import BrowsesWithQueryLimit
 
 
 class TestDistributionSourcePackageFormatterAPI(TestCaseWithFactory):
@@ -24,3 +33,23 @@
             u'<a href="/ubuntu/+source/mouse" class="sprite package-source">'
             u'mouse in ubuntu</a>')
         self.assertEqual(markup, test_tales('dsp/fmt:link', dsp=dsp))
+
+
+class TestDistributionSourcePackageChangelogView(TestCaseWithFactory):
+
+    layer = LaunchpadFunctionalLayer
+
+    def test_packagediff_query_count(self):
+        archive = self.factory.makeArchive(purpose=ArchivePurpose.PRIMARY)
+        spph = self.factory.makeSourcePackagePublishingHistory(
+            archive=archive)
+        dsp = archive.distribution.getSourcePackage(
+            spph.sourcepackagerelease.sourcepackagename)
+        changelog_browses_under_limit = BrowsesWithQueryLimit(
+            32, self.factory.makePerson(), '+changelog')
+        self.assertThat(dsp, changelog_browses_under_limit)
+        with celebrity_logged_in('admin'):
+            for i in range(5):
+                self.factory.makePackageDiff(
+                    to_source=spph.sourcepackagerelease)
+        self.assertThat(dsp, changelog_browses_under_limit)

=== modified file 'lib/lp/services/database/bulk.py'
--- lib/lp/services/database/bulk.py	2011-03-07 21:17:41 +0000
+++ lib/lp/services/database/bulk.py	2011-03-16 04:27:31 +0000
@@ -69,7 +69,7 @@
             "Compound primary keys are not supported: %s." %
             object_type.__name__)
     primary_key_column = primary_key[0]
-    condition = primary_key_column.is_in(primary_keys)
+    condition = primary_key_column.is_in(set(primary_keys))
     if store is None:
         store = IStore(object_type)
     query = store.find(object_type, condition)

=== modified file 'lib/lp/soyuz/interfaces/packagediff.py'
--- lib/lp/soyuz/interfaces/packagediff.py	2010-08-24 12:05:25 +0000
+++ lib/lp/soyuz/interfaces/packagediff.py	2011-03-16 04:27:31 +0000
@@ -64,7 +64,7 @@
         title=_('Status'),
         description=_('The status of this package diff request.'),
         vocabulary='PackageDiffStatus',
-        required=False, default=PackageDiffStatus.PENDING
+        required=False, default=PackageDiffStatus.PENDING,
         )
 
     title = Attribute("The Package diff title.")
@@ -95,10 +95,12 @@
         :return a `SelectResult` ordered by id respecting the given limit.
         """
 
-    def getDiffsToReleases(self, sprs):
+    def getDiffsToReleases(self, sprs, want_files=False):
         """Return all diffs that targetting a set of source package releases.
 
         :param sprs: a sequence of `SourcePackageRelease` objects.
+        :param want_files: True if the `LibraryFileAlias` and
+            `LibraryFileContent` objects should be preloaded
 
         :return a `ResultSet` ordered by `SourcePackageRelease` ID and
         then diff request date in descending order.  If sprs is empty,

=== modified file 'lib/lp/soyuz/model/packagediff.py'
--- lib/lp/soyuz/model/packagediff.py	2011-01-14 11:02:44 +0000
+++ lib/lp/soyuz/model/packagediff.py	2011-03-16 04:27:31 +0000
@@ -8,6 +8,7 @@
     ]
 
 import gzip
+import itertools
 import os
 import shutil
 import subprocess
@@ -26,6 +27,13 @@
     SQLBase,
     sqlvalues,
     )
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
+from canonical.launchpad.database.librarian import (
+    LibraryFileAlias,
+    LibraryFileContent,
+    )
 from canonical.launchpad.interfaces.librarian import ILibraryFileAliasSet
 from canonical.launchpad.webapp.interfaces import (
     DEFAULT_FLAVOR,
@@ -33,6 +41,7 @@
     MAIN_STORE,
     )
 from canonical.librarian.utils import copy_and_close
+from lp.services.database.bulk import load
 from lp.soyuz.enums import PackageDiffStatus
 from lp.soyuz.interfaces.packagediff import (
     IPackageDiff,
@@ -271,8 +280,11 @@
         result.order_by(PackageDiff.id)
         return result.config(limit=limit)
 
-    def getDiffsToReleases(self, sprs):
+    def getDiffsToReleases(self, sprs, preload_for_display=False):
         """See `IPackageDiffSet`."""
+        from lp.registry.model.distribution import Distribution
+        from lp.soyuz.model.archive import Archive
+        from lp.soyuz.model.sourcepackagerelease import SourcePackageRelease
         if len(sprs) == 0:
             return EmptyResultSet()
         store = getUtility(IStoreSelector).get(MAIN_STORE, DEFAULT_FLAVOR)
@@ -281,4 +293,18 @@
             PackageDiff, PackageDiff.to_sourceID.is_in(spr_ids))
         result.order_by(PackageDiff.to_sourceID,
                         Desc(PackageDiff.date_requested))
-        return result
+
+        def preload_hook(rows):
+            lfas = load(LibraryFileAlias, (pd.diff_contentID for pd in rows))
+            lfcs = load(LibraryFileContent, (lfa.contentID for lfa in lfas))
+            sprs = load(
+                SourcePackageRelease,
+                itertools.chain.from_iterable(
+                    (pd.from_sourceID, pd.to_sourceID) for pd in rows))
+            archives = load(Archive, (spr.upload_archiveID for spr in sprs))
+            distros = load(Distribution, (a.distributionID for a in archives))
+
+        if preload_for_display:
+            return DecoratedResultSet(result, pre_iter_hook=preload_hook)
+        else:
+            return result