launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/sane-buildable-distroseries into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/sane-buildable-distroseries/+merge/122791

Destroy get_buildable_distroseries_set and buildable_distroseries_vocabulary
and birth BuildableDistroSeries from their ashes.

buildable_distroseries_vocabulary had the problem that as a SimpleVocabulary it would execute six queries per recipe when attempting to fetch recipes over the API due to ISourcePackageRecipe.distroseries having a vocab of BuildableDistrosSeries, and the LAZR code marshalling the vocab. Now we use an IHugeVocabulary which mitigates that problem.

I have also added a webservice test for this case -- fetching a single recipe from the API results in 28 queries rather 34.
-- 
https://code.launchpad.net/~stevenk/launchpad/sane-buildable-distroseries/+merge/122791
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/sane-buildable-distroseries into lp:launchpad.

=== modified file 'lib/lp/code/browser/sourcepackagerecipe.py'
--- lib/lp/code/browser/sourcepackagerecipe.py	2012-08-15 01:04:53 +0000
+++ lib/lp/code/browser/sourcepackagerecipe.py	2012-09-05 05:18:20 +0000
@@ -99,7 +99,7 @@
     MINIMAL_RECIPE_TEXT,
     )
 from lp.code.model.branchtarget import PersonBranchTarget
-from lp.code.model.sourcepackagerecipe import get_buildable_distroseries_set
+from lp.code.vocabularies.sourcepackagerecipe import BuildableDistroSeries
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.fields import PersonChoice
 from lp.services.propertycache import cachedproperty
@@ -787,7 +787,7 @@
 
     @property
     def initial_values(self):
-        distroseries = get_buildable_distroseries_set(self.user)
+        distroseries = BuildableDistroSeries.findSeries(self.user)
         series = [series for series in distroseries if series.status in (
                 SeriesStatus.CURRENT, SeriesStatus.DEVELOPMENT)]
         return {

=== modified file 'lib/lp/code/model/sourcepackagerecipe.py'
--- lib/lp/code/model/sourcepackagerecipe.py	2012-08-15 01:04:53 +0000
+++ lib/lp/code/model/sourcepackagerecipe.py	2012-09-05 05:18:20 +0000
@@ -1,13 +1,10 @@
 # Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
-# pylint: disable-msg=F0401,W1001
-
 """Implementation of the `SourcePackageRecipe` content type."""
 
 __metaclass__ = type
 __all__ = [
-    'get_buildable_distroseries_set',
     'SourcePackageRecipe',
     ]
 
@@ -39,7 +36,6 @@
     implements,
     )
 
-from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.buildmaster.enums import BuildStatus
 from lp.buildmaster.model.buildfarmjob import BuildFarmJob
 from lp.buildmaster.model.packagebuild import PackageBuild
@@ -59,7 +55,7 @@
 from lp.code.model.branch import Branch
 from lp.code.model.sourcepackagerecipebuild import SourcePackageRecipeBuild
 from lp.code.model.sourcepackagerecipedata import SourcePackageRecipeData
-from lp.registry.interfaces.distroseries import IDistroSeriesSet
+from lp.code.vocabularies.sourcepackagerecipe import BuildableDistroSeries
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.model.distroseries import DistroSeries
 from lp.services.database.bulk import (
@@ -80,24 +76,9 @@
     cachedproperty,
     get_property_cache,
     )
-from lp.soyuz.interfaces.archive import IArchiveSet
 from lp.soyuz.model.archive import Archive
 
 
-def get_buildable_distroseries_set(user):
-    ppas = getUtility(IArchiveSet).getPPAsForUser(user)
-    supported_distros = set([ppa.distribution for ppa in ppas])
-    # Now add in Ubuntu.
-    supported_distros.add(getUtility(ILaunchpadCelebrities).ubuntu)
-    distros = getUtility(IDistroSeriesSet).search()
-
-    buildables = []
-    for distro in distros:
-        if distro.active and distro.distribution in supported_distros:
-            buildables.append(distro)
-    return buildables
-
-
 def recipe_modified(recipe, event):
     """Update the date_last_modified property when a recipe is modified.
 
@@ -282,7 +263,8 @@
             requester, self, distroseries).count() >= 5
 
     def containsUnbuildableSeries(self, archive):
-        buildable_distros = set(get_buildable_distroseries_set(archive.owner))
+        buildable_distros = set(
+            BuildableDistroSeries.findSeries(archive.owner))
         return len(set(self.distroseries).difference(buildable_distros)) >= 1
 
     def requestBuild(self, archive, requester, distroseries,
@@ -292,7 +274,7 @@
         if not archive.is_ppa:
             raise NonPPABuildRequest
 
-        buildable_distros = get_buildable_distroseries_set(archive.owner)
+        buildable_distros = BuildableDistroSeries.findSeries(archive.owner)
         if distroseries not in buildable_distros:
             raise BuildNotAllowedForDistro(self, distroseries)
 
@@ -325,7 +307,7 @@
         """See `ISourcePackageRecipe`."""
         builds = []
         self.is_stale = False
-        buildable_distros = set(get_buildable_distroseries_set(
+        buildable_distros = set(BuildableDistroSeries.findSeries(
             self.daily_build_archive.owner))
         build_for = set(self.distroseries).intersection(buildable_distros)
         for distroseries in build_for:

=== modified file 'lib/lp/code/model/tests/test_sourcepackagerecipe.py'
--- lib/lp/code/model/tests/test_sourcepackagerecipe.py	2012-08-15 01:04:53 +0000
+++ lib/lp/code/model/tests/test_sourcepackagerecipe.py	2012-09-05 05:18:20 +0000
@@ -15,6 +15,7 @@
 from lazr.lifecycle.event import ObjectModifiedEvent
 from pytz import UTC
 from storm.locals import Store
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 from zope.event import notify
@@ -61,6 +62,7 @@
     )
 from lp.services.propertycache import clear_property_cache
 from lp.services.webapp.authorization import check_permission
+from lp.services.webapp.publisher import canonical_url
 from lp.services.webapp.testing import verifyObject
 from lp.soyuz.enums import ArchivePurpose
 from lp.soyuz.interfaces.archive import (
@@ -74,6 +76,7 @@
     login,
     login_person,
     person_logged_in,
+    StormStatementRecorder,
     TestCaseWithFactory,
     ws_object,
     )
@@ -81,7 +84,11 @@
     AppServerLayer,
     DatabaseFunctionalLayer,
     )
-from lp.testing.matchers import DoesNotSnapshot
+from lp.testing.matchers import (
+    DoesNotSnapshot,
+    HasQueryCount,
+    )
+from lp.testing.pages import webservice_for_person
 
 
 class TestSourcePackageRecipe(TestCaseWithFactory):
@@ -1196,3 +1203,16 @@
                 "archive": '%s/%s' %
                            (archive.owner.name, archive.name)})
         self.assertEqual(build_info, list(recipe.getPendingBuildInfo()))
+
+    def test_query_count_of_webservice_recipe(self):
+        owner = self.factory.makePerson()
+        recipe = self.factory.makeSourcePackageRecipe(owner=owner)
+        webservice = webservice_for_person(owner)
+        with person_logged_in(owner):
+            url = canonical_url(recipe, force_local_path=True)
+        store = Store.of(recipe)
+        store.flush()
+        store.invalidate()
+        with StormStatementRecorder() as recorder:
+            webservice.get(url)
+        self.assertThat(recorder, HasQueryCount(Equals(28)))

=== modified file 'lib/lp/code/vocabularies/configure.zcml'
--- lib/lp/code/vocabularies/configure.zcml	2011-12-24 17:49:30 +0000
+++ lib/lp/code/vocabularies/configure.zcml	2012-09-05 05:18:20 +0000
@@ -1,4 +1,4 @@
-<!-- Copyright 2010 Canonical Ltd.  This software is licensed under the
+<!-- Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
      GNU Affero General Public License version 3 (see the file LICENSE).
 -->
 
@@ -6,11 +6,15 @@
 
   <securedutility
      name="BuildableDistroSeries"
-     component=".sourcepackagerecipe.buildable_distroseries_vocabulary"
+     component=".sourcepackagerecipe.BuildableDistroSeries"
      provides="zope.schema.interfaces.IVocabularyFactory">
     <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
   </securedutility>
 
+  <class class=".sourcepackagerecipe.BuildableDistroSeries">
+    <allow interface="lp.services.webapp.vocabulary.IHugeVocabulary"/>
+  </class>
+
   <securedutility
      name="TargetPPAs"
      component=".sourcepackagerecipe.target_ppas_vocabulary"

=== modified file 'lib/lp/code/vocabularies/sourcepackagerecipe.py'
--- lib/lp/code/vocabularies/sourcepackagerecipe.py	2012-01-01 02:58:52 +0000
+++ lib/lp/code/vocabularies/sourcepackagerecipe.py	2012-09-05 05:18:20 +0000
@@ -1,36 +1,60 @@
-# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Source Package Recipe vocabularies used in the lp/code modules."""
+
 __metaclass__ = type
 __all__ = [
-    'buildable_distroseries_vocabulary',
+    'BuildableDistroSeries',
     'target_ppas_vocabulary',
     ]
 
 from zope.component import getUtility
-from zope.schema.vocabulary import (
-    SimpleTerm,
-    SimpleVocabulary,
-    )
+from zope.interface import implements
+from zope.schema.vocabulary import SimpleTerm
 
-from lp.code.model.sourcepackagerecipe import get_buildable_distroseries_set
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.registry.interfaces.distroseries import IDistroSeriesSet
+from lp.registry.model.distroseries import DistroSeries
 from lp.services.webapp.authorization import check_permission
 from lp.services.webapp.interfaces import ILaunchBag
 from lp.services.webapp.sorting import sorted_dotted_numbers
+from lp.services.webapp.vocabulary import (
+    IHugeVocabulary,
+    SQLObjectVocabularyBase,
+    )
 from lp.soyuz.browser.archive import make_archive_vocabulary
 from lp.soyuz.interfaces.archive import IArchiveSet
 
 
-def buildable_distroseries_vocabulary(context):
-    """Return a vocabulary of buildable distroseries."""
-    distros = get_buildable_distroseries_set(getUtility(ILaunchBag).user)
-    terms = sorted_dotted_numbers(
-        [SimpleTerm(distro, distro.id, distro.displayname)
-         for distro in distros],
-        key=lambda term: term.value.version)
-    terms.reverse()
-    return SimpleVocabulary(terms)
+class BuildableDistroSeries(SQLObjectVocabularyBase):
+    implements(IHugeVocabulary)
+
+    _table = DistroSeries
+
+    def toTerm(self, obj):
+        """See `IVocabulary`."""
+        return SimpleTerm(obj, obj.id, obj.displayname)
+
+    @classmethod
+    def findSeries(self, user):
+        ppas = getUtility(IArchiveSet).getPPAsForUser(user)
+        supported_distros = set([ppa.distribution for ppa in ppas])
+        # Now add in Ubuntu.
+        supported_distros.add(getUtility(ILaunchpadCelebrities).ubuntu)
+        all_series = getUtility(IDistroSeriesSet).search()
+
+        return [
+            series for series in all_series
+            if series.active and series.distribution in supported_distros]
+
+    def __iter__(self):
+        distroseries = self.findSeries(getUtility(ILaunchBag).user)
+        series = sorted_dotted_numbers(
+            [self.toTerm(s) for s in distroseries],
+            key=lambda term: term.value.version)
+        series.reverse()
+        return iter(series)
 
 
 def target_ppas_vocabulary(context):