launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/distroseries-spec-preload into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #681432 in Launchpad itself: "DistroSeries.valid_specifications includes OBSOLETE ones"
  https://bugs.launchpad.net/launchpad/+bug/681432
  Bug #1202664 in Launchpad itself: "Timeouts while fetching blueprints through the api"
  https://bugs.launchpad.net/launchpad/+bug/1202664

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/distroseries-spec-preload/+merge/180046

Preload workitems as well during search_specifications. Due to the number of callsites, this will hopefully reduce specification related timeouts across the board, but we'll see.

Also due to search_specifications work, re-enable a disabled test that now works.
-- 
https://code.launchpad.net/~stevenk/launchpad/distroseries-spec-preload/+merge/180046
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/distroseries-spec-preload into lp:launchpad.

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2013-06-27 15:11:38 +0000
+++ lib/lp/blueprints/model/specification.py	2013-08-14 06:58:36 +0000
@@ -335,7 +335,7 @@
             else:
                 return "Work items for %s:" % milestone.name
 
-        if self.work_items.count() == 0:
+        if len(self.work_items) == 0:
             return ''
         milestone = self.work_items[0].milestone
         # Start by appending a header for the milestone of the first work
@@ -353,9 +353,9 @@
             else:
                 assignee_part = ""
             # work_items are ordered by sequence
-            workitems_lines.append("%s%s: %s" % (assignee_part,
-                                                 work_item.title,
-                                                 work_item.status.name))
+            workitems_lines.append(
+                "%s%s: %s" % (
+                    assignee_part, work_item.title, work_item.status.name))
         return "\n".join(workitems_lines)
 
     @property
@@ -377,9 +377,13 @@
             title=title, status=status, specification=self, assignee=assignee,
             milestone=milestone, sequence=sequence)
 
-    @property
+    @cachedproperty
     def work_items(self):
         """See ISpecification."""
+        return list(self._work_items)
+
+    @property
+    def _work_items(self):
         return Store.of(self).find(
             SpecificationWorkItem, specification=self,
             deleted=False).order_by("sequence")

=== modified file 'lib/lp/blueprints/model/specificationsearch.py'
--- lib/lp/blueprints/model/specificationsearch.py	2013-06-20 05:50:00 +0000
+++ lib/lp/blueprints/model/specificationsearch.py	2013-08-14 06:58:36 +0000
@@ -36,6 +36,7 @@
     )
 from lp.blueprints.model.specification import Specification
 from lp.blueprints.model.specificationbranch import SpecificationBranch
+from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
 from lp.registry.interfaces.person import IPersonSet
@@ -233,6 +234,12 @@
         person_ids |= set(
             [spec._assigneeID, spec._approverID, spec._drafterID])
         get_property_cache(spec).linked_branches = []
+        get_property_cache(spec).work_items = []
+    work_items = load_referencing(
+        SpecificationWorkItem, rows, ['specification_id'])
+    for workitem in work_items:
+        person_ids.add(workitem.assignee_id)
+        get_property_cache(workitem.specification).work_items.append(workitem)
     person_ids -= set([None])
     list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
         person_ids, need_validity=True))

=== modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
--- lib/lp/blueprints/tests/test_hasspecifications.py	2013-04-03 03:09:04 +0000
+++ lib/lp/blueprints/tests/test_hasspecifications.py	2013-08-14 06:58:36 +0000
@@ -38,17 +38,14 @@
 
     def test_distribution_all_specifications(self):
         distribution = self.factory.makeDistribution()
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec1")
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec2")
+        self.factory.makeSpecification(distribution=distribution, name="spec1")
+        self.factory.makeSpecification(distribution=distribution, name="spec2")
         self.assertNamesOfSpecificationsAre(
             ["spec1", "spec2"], distribution.visible_specifications)
 
     def test_distribution_valid_specifications(self):
         distribution = self.factory.makeDistribution()
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec1")
+        self.factory.makeSpecification(distribution=distribution, name="spec1")
         self.factory.makeSpecification(
             distribution=distribution, name="spec2",
             status=SpecificationDefinitionStatus.OBSOLETE)
@@ -59,36 +56,26 @@
         distroseries = self.factory.makeDistroSeries(name='maudlin')
         distribution = distroseries.distribution
         self.factory.makeSpecification(
-            distribution=distribution, name="spec1",
-            goal=distroseries)
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec2",
-            goal=distroseries)
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec3")
+            distribution=distribution, name="spec1", goal=distroseries)
+        self.factory.makeSpecification(
+            distribution=distribution, name="spec2", goal=distroseries)
+        self.factory.makeSpecification(distribution=distribution, name="spec3")
         self.assertNamesOfSpecificationsAre(
             ["spec1", "spec2"], distroseries.visible_specifications)
 
-    # XXX: salgado, 2010-11-25, bug=681432: Test disabled because
-    # DistroSeries._valid_specifications is broken.
-    def disabled_test_distroseries_valid_specifications(self):
+    def test_distroseries_valid_specifications(self):
         distroseries = self.factory.makeDistroSeries(name='maudlin')
         distribution = distroseries.distribution
         self.factory.makeSpecification(
-            distribution=distribution, name="spec1",
-            goal=distroseries)
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec2",
-            goal=distroseries)
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec3",
-            goal=distroseries,
+            distribution=distribution, name="spec1", goal=distroseries)
+        self.factory.makeSpecification(
+            distribution=distribution, name="spec2", goal=distroseries)
+        self.factory.makeSpecification(
+            distribution=distribution, name="spec3", goal=distroseries,
             status=SpecificationDefinitionStatus.OBSOLETE)
-        self.factory.makeSpecification(
-            distribution=distribution, name="spec4")
+        self.factory.makeSpecification(distribution=distribution, name="spec4")
         self.assertNamesOfSpecificationsAre(
-            ["spec1", "spec2"],
-            distroseries._valid_specifications)
+            ["spec1", "spec2"], distroseries._valid_specifications)
 
     def test_productseries_all_specifications(self):
         product = self.factory.makeProduct()
@@ -123,13 +110,11 @@
         product1 = self.factory.makeProduct(project=projectgroup)
         product2 = self.factory.makeProduct(project=projectgroup)
         product3 = self.factory.makeProduct(project=other_projectgroup)
-        self.factory.makeSpecification(
-            product=product1, name="spec1")
+        self.factory.makeSpecification(product=product1, name="spec1")
         self.factory.makeSpecification(
             product=product2, name="spec2",
             status=SpecificationDefinitionStatus.OBSOLETE)
-        self.factory.makeSpecification(
-            product=product3, name="spec3")
+        self.factory.makeSpecification(product=product3, name="spec3")
         self.assertNamesOfSpecificationsAre(
             ["spec1", "spec2"], projectgroup.visible_specifications)
 
@@ -155,8 +140,7 @@
         self.factory.makeSpecification(
             product=product, name="spec2", approver=person,
             status=SpecificationDefinitionStatus.OBSOLETE)
-        self.factory.makeSpecification(
-            product=product, name="spec3")
+        self.factory.makeSpecification(product=product, name="spec3")
         self.assertNamesOfSpecificationsAre(
             ["spec1", "spec2"], person.visible_specifications)
 
@@ -168,8 +152,7 @@
         self.factory.makeSpecification(
             product=product, name="spec2", approver=person,
             status=SpecificationDefinitionStatus.OBSOLETE)
-        self.factory.makeSpecification(
-            product=product, name="spec3")
+        self.factory.makeSpecification(product=product, name="spec3")
         self.assertNamesOfSpecificationsAre(
             ["spec1"], person._valid_specifications)
 
@@ -181,10 +164,7 @@
 
     def check_skipped(self, target):
         """Asserts that fields marked doNotSnapshot are skipped."""
-        skipped = [
-            'all_specifications',
-            'valid_specifications',
-            ]
+        skipped = ['all_specifications', 'valid_specifications']
         self.assertThat(target, DoesNotSnapshot(skipped, IHasSpecifications))
 
     def test_product(self):

=== modified file 'lib/lp/registry/tests/test_distroseries.py'
--- lib/lp/registry/tests/test_distroseries.py	2012-09-26 04:14:01 +0000
+++ lib/lp/registry/tests/test_distroseries.py	2013-08-14 06:58:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Tests for distroseries."""
@@ -11,6 +11,7 @@
 
 from logging import getLogger
 
+from testtools.matchers import Equals
 import transaction
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
@@ -19,6 +20,7 @@
 from lp.registry.interfaces.distroseries import IDistroSeriesSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
 from lp.registry.interfaces.series import SeriesStatus
+from lp.services.database.interfaces import IStore
 from lp.soyuz.enums import (
     ArchivePurpose,
     PackagePublishingStatus,
@@ -38,6 +40,7 @@
     ANONYMOUS,
     login,
     person_logged_in,
+    StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     )
@@ -45,6 +48,7 @@
     DatabaseFunctionalLayer,
     LaunchpadFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 from lp.translations.interfaces.translations import (
     TranslationsBranchImportMode,
     )
@@ -324,6 +328,23 @@
         self.assertTrue(self.checkLegalPocket(
             SeriesStatus.CURRENT, PackagePublishingPocket.UPDATES))
 
+    def test_valid_specifications_query_count(self):
+        distroseries = self.factory.makeDistroSeries()
+        distribution = distroseries.distribution
+        spec1 = self.factory.makeSpecification(
+            distribution=distribution, goal=distroseries)
+        spec2 = self.factory.makeSpecification(
+            distribution=distribution, goal=distroseries)
+        for i in range(5):
+            self.factory.makeSpecificationWorkItem(specification=spec1)
+            self.factory.makeSpecificationWorkItem(specification=spec2)
+        IStore(spec1.__class__).flush()
+        IStore(spec1.__class__).invalidate()
+        with StormStatementRecorder() as recorder:
+            for spec in distroseries._valid_specifications:
+                spec.workitems_text
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
 
 class TestDistroSeriesPackaging(TestCaseWithFactory):
 

=== modified file 'lib/lp/soyuz/browser/tests/test_archive_webservice.py'
--- lib/lp/soyuz/browser/tests/test_archive_webservice.py	2013-05-10 05:30:11 +0000
+++ lib/lp/soyuz/browser/tests/test_archive_webservice.py	2013-08-14 06:58:36 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 __metaclass__ = type
@@ -37,14 +37,13 @@
     )
 from lp.testing.layers import DatabaseFunctionalLayer
 from lp.testing.matchers import HasQueryCount
-from lp.testing.pages import LaunchpadWebServiceCaller
 
 
 class TestArchiveWebservice(TestCaseWithFactory):
     layer = DatabaseFunctionalLayer
 
     def setUp(self):
-        TestCaseWithFactory.setUp(self)
+        super(TestArchiveWebservice, self).setUp()
         with celebrity_logged_in('admin'):
             self.archive = self.factory.makeArchive(
                 purpose=ArchivePurpose.PRIMARY)
@@ -55,9 +54,6 @@
             distroseries_name = distroseries.name
             person_name = person.name
 
-        self.webservice = LaunchpadWebServiceCaller(
-            'launchpad-library', 'salgado-change-anything')
-
         self.launchpad = launchpadlib_for(
             "archive test", "salgado", "WRITE_PUBLIC")
         self.distribution = self.launchpad.distributions[distro_name]