Merge lp:~adeuring/launchpad/bug-739065-2 into lp:launchpad

[Abel Deuring]

[r=bac][bug=739065] pre-join BinaryPackageName in the SQL query in DistroSeriesSourcePackageRelease.binaries; pre-join DistroSeriesPackageCache in the SQL query in DistributionSourcePackageRelease.sample_binary_packages

This branch is a second step to fix bug 739065: SourcePackage:+index timeouts.

The OOPS reports mentioned in the bug report show that two queries are repeated very often. The repetition of one of these queries is removed in the prerequiste branch ~adeuring/launchpad/bug-739065. This branch removes the other repetition.

I added some _very_ basic units for the affected property, DistroSeriesSourcePackageRelease.binaries, and I replaced the ancient SQLObject query with a Storm query.

test: ./bin/test soyuz -vvt test_distroseriessourcepackagerelease

no lint

[Brad Crittenden (bac)]

Abel this branch looks good. On IRC we discussed not forcing the use of the main store. Other than that it looks great. Thanks for the nice branch.

review: Approve (code)

=== modified file 'lib/lp/soyuz/model/distroseriessourcepackagerelease.py'
--- lib/lp/soyuz/model/distroseriessourcepackagerelease.py	2011-04-18 12:53:54 +0000
+++ lib/lp/soyuz/model/distroseriessourcepackagerelease.py	2011-05-20 14:16:58 +0000
@@ -12,18 +12,33 @@
     ]
 
 from lazr.delegates import delegates
+from operator import itemgetter
+from storm.expr import (
+    And,
+    Desc,
+    Join,
+    )
+from storm.store import Store
 from zope.interface import implements
 from zope.security.proxy import removeSecurityProxy
 
 from canonical.database.sqlbase import sqlvalues
+from canonical.launchpad.components.decoratedresultset import (
+    DecoratedResultSet,
+    )
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.soyuz.enums import PackagePublishingStatus
 from lp.soyuz.interfaces.distroseriessourcepackagerelease import (
     IDistroSeriesSourcePackageRelease,
     )
 from lp.soyuz.interfaces.sourcepackagerelease import ISourcePackageRelease
+from lp.soyuz.model.binarypackagebuild import BinaryPackageBuild
+from lp.soyuz.model.binarypackagename import BinaryPackageName
 from lp.soyuz.model.binarypackagerelease import BinaryPackageRelease
-from lp.soyuz.model.publishing import SourcePackagePublishingHistory
+from lp.soyuz.model.publishing import (
+    BinaryPackagePublishingHistory,
+    SourcePackagePublishingHistory,
+    )
 
 
 class DistroSeriesSourcePackageRelease:
@@ -112,7 +127,7 @@
 
         return (
             [build for build in distro_builds
-                if build.distro_arch_series.distroseries == self.distroseries])
+             if build.distro_arch_series.distroseries == self.distroseries])
 
     @property
     def files(self):
@@ -122,30 +137,40 @@
     @property
     def binaries(self):
         """See `IDistroSeriesSourcePackageRelease`."""
-        clauseTables = [
-            'BinaryPackageRelease',
-            'DistroArchSeries',
-            'BinaryPackageBuild',
-            'BinaryPackagePublishingHistory'
-        ]
-
-        query = """
-        BinaryPackageRelease.build=BinaryPackageBuild.id AND
-        DistroArchSeries.id =
-            BinaryPackagePublishingHistory.distroarchseries AND
-        BinaryPackagePublishingHistory.binarypackagerelease=
-            BinaryPackageRelease.id AND
-        DistroArchSeries.distroseries=%s AND
-        BinaryPackagePublishingHistory.archive IN %s AND
-        BinaryPackageBuild.source_package_release=%s
-        """ % sqlvalues(self.distroseries,
-                        self.distroseries.distribution.all_distro_archive_ids,
-                        self.sourcepackagerelease)
-
-        return BinaryPackageRelease.select(
-                query, prejoinClauseTables=['BinaryPackageBuild'],
-                orderBy=['-id'], clauseTables=clauseTables,
-                distinct=True)
+        # Avoid circular imports.
+        from lp.soyuz.model.distroarchseries import DistroArchSeries
+        store = Store.of(self.distroseries)
+        result_row = (
+            BinaryPackageRelease, BinaryPackageBuild, BinaryPackageName)
+
+        tables = (
+            BinaryPackageRelease,
+            Join(
+                BinaryPackageBuild,
+                BinaryPackageBuild.id == BinaryPackageRelease.buildID),
+            Join(
+                BinaryPackagePublishingHistory,
+                BinaryPackageRelease.id ==
+                BinaryPackagePublishingHistory.binarypackagereleaseID),
+            Join(
+                DistroArchSeries,
+                DistroArchSeries.id ==
+                BinaryPackagePublishingHistory.distroarchseriesID),
+            Join(
+                BinaryPackageName,
+                BinaryPackageName.id ==
+                BinaryPackageRelease.binarypackagenameID))
+        archive_ids = list(
+            self.distroseries.distribution.all_distro_archive_ids)
+        binaries = store.using(*tables).find(
+            result_row,
+            And(
+                DistroArchSeries.distroseriesID == self.distroseries.id,
+                BinaryPackagePublishingHistory.archiveID.is_in(archive_ids),
+                BinaryPackageBuild.source_package_release ==
+                self.sourcepackagerelease))
+        binaries.order_by(Desc(BinaryPackageRelease.id)).config(distinct=True)
+        return DecoratedResultSet(binaries, itemgetter(0))
 
     @property
     def changesfile(self):

=== added file 'lib/lp/soyuz/tests/test_distroseriessourcepackagerelease.py'
--- lib/lp/soyuz/tests/test_distroseriessourcepackagerelease.py	1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/tests/test_distroseriessourcepackagerelease.py	2011-05-20 14:16:58 +0000
@@ -0,0 +1,93 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Tests for DistroSeriesSourcePackageRelease."""
+
+from storm.store import Store
+from testtools.matchers import Equals
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.soyuz.enums import PackagePublishingStatus
+from lp.soyuz.model.distroseriessourcepackagerelease import (
+    DistroSeriesSourcePackageRelease,
+    )
+from lp.testing import (
+    StormStatementRecorder,
+    TestCaseWithFactory,
+    )
+from lp.testing.matchers import HasQueryCount
+
+
+class TestDistroSeriesSourcePackageRelease(TestCaseWithFactory):
+    """Tests for DistroSeriesSourcePackageRelease."""
+
+    layer = DatabaseFunctionalLayer
+
+    def setUp(self):
+        super(TestDistroSeriesSourcePackageRelease, self).setUp()
+        self.sourcepackagerelease = self.factory.makeSourcePackageRelease()
+        self.distroarchseries = self.factory.makeDistroArchSeries(
+            distroseries=self.sourcepackagerelease.sourcepackage.distroseries)
+        self.dssp_release = DistroSeriesSourcePackageRelease(
+            self.distroarchseries.distroseries, self.sourcepackagerelease)
+
+    def makeBinaryPackageRelease(self, name=None):
+        if name is None:
+            name = self.factory.makeBinaryPackageName()
+        bp_build = self.factory.makeBinaryPackageBuild(
+            source_package_release=self.sourcepackagerelease,
+            distroarchseries=self.distroarchseries)
+        bp_release = self.factory.makeBinaryPackageRelease(
+            build=bp_build, binarypackagename=name)
+        sourcepackagename = self.sourcepackagerelease.sourcepackagename
+        self.factory.makeSourcePackagePublishingHistory(
+            sourcepackagename,
+            sourcepackagerelease=self.sourcepackagerelease,
+            distroseries=self.sourcepackagerelease.sourcepackage.distroseries,
+            status=PackagePublishingStatus.PUBLISHED)
+        self.factory.makeBinaryPackagePublishingHistory(
+            binarypackagerelease=bp_release,
+            distroarchseries=self.distroarchseries)
+        return bp_release
+
+    def test_binaries__no_releases(self):
+        # If no binary releases exist,
+        # DistroSeriesSourcePackageRelease.binaries returns an empty
+        # sequence.
+        self.assertEqual(0, self.dssp_release.binaries.count())
+
+    def test_binaries__one_release_for_source_package(self):
+        # If a binary release exists, it is returned by
+        # DistroSeriesSourcePackageRelease.binaries.
+        bp_release = self.makeBinaryPackageRelease()
+        self.assertEqual(
+            [bp_release], list(self.dssp_release.binaries))
+
+    def test_binaries__two_releases_for_source_package(self):
+        # If two binary releases with the sam name exist, both
+        # are returned. The more recent one is returned first.
+        name = self.factory.makeBinaryPackageName()
+        bp_release_one = self.makeBinaryPackageRelease(name)
+        bp_release_two = self.makeBinaryPackageRelease(name)
+        self.assertEqual(
+            [bp_release_two, bp_release_one],
+            list(self.dssp_release.binaries))
+
+    def test_prejoins(self):
+        # The properties BinaryPackageRelease.build and
+        # and BinaryPackageRelease.binarypackagename of the
+        # the result objects are preloaded in the query
+        # issued in DistroSeriesSourcePackageRelease.binaries.
+        self.makeBinaryPackageRelease()
+        # Both properties we want to check have been created
+        # in makeBinaryPackageRelease() and are thus already
+        # in Storm's cache. We must empty the cache, otherwise
+        # accessing bp_release.build and
+        # bp_release.binarypackagename will never cause an
+        # SQL query to be issued.
+        Store.of(self.distroarchseries).invalidate()
+        [bp_release] = list(self.dssp_release.binaries)
+        with StormStatementRecorder() as recorder:
+            bp_release.build
+            bp_release.binarypackagename
+        self.assertThat(recorder, HasQueryCount(Equals(0)))