Merge lp:~stevenk/launchpad/distroseries-spec-preload into lp:launchpad

[Steve Kowalik]

Rewrite preloading for searching specifications, allowing callsites to specify which of people, branches and workitems they need preloaded; rename IHasSpecification._valid_specifications has been renamed to valid_specifications, and create a api_valid_specifications property to ensure API compatibility.

Rewrite preloading for searching specifications, allowing callsites to specify which of people, branches and workitems they need preloaded, modeled off IPersonSet.getPrecachedPersonsFromIDs().

IHasSpecification._valid_specifications has been renamed to valid_specifications and is no longer a property, and to ensure API compatability, api_valid_specifications is a property which calls into valid_specifications.

I've also re-enabled a disabled test that now works due to the refactoring work that went into search_specifications().

I have more than enough LoC credit to take the +50 hit for this branch.

[William Grant (wgrant)]

125 - for work_item in self.work_items:
126 + for work_item in self._work_items:
127 if (work_item.title not in title_counts or

Can't this use the cached version?

267 + for specid in work_items_by_spec.keys():
268 + work_items_by_spec[specid].sort(key=lambda wi: wi.sequence)
269 + for workitem in work_items_by_spec[specid]:
270 + get_property_cache(workitem.specification).work_items.append(
271 + workitem)

This seems needlessly circuitous. get_property_cache(spec).work_items = list(sorted(work_items_by_spec[specid], key=lambda wi: wi.sequence))?

review: Approve (code)

=== modified file 'lib/lp/_schema_circular_imports.py'
--- lib/lp/_schema_circular_imports.py	2013-05-01 18:14:22 +0000
+++ lib/lp/_schema_circular_imports.py	2013-08-20 07:26:08 +0000
@@ -737,7 +737,7 @@
 patch_collection_property(
     IHasSpecifications, 'visible_specifications', ISpecification)
 patch_collection_property(
-    IHasSpecifications, '_valid_specifications', ISpecification)
+    IHasSpecifications, 'api_valid_specifications', ISpecification)
 
 
 ###

=== modified file 'lib/lp/blueprints/browser/specificationtarget.py'
--- lib/lp/blueprints/browser/specificationtarget.py	2013-04-10 09:36:25 +0000
+++ lib/lp/blueprints/browser/specificationtarget.py	2013-08-20 07:26:08 +0000
@@ -406,7 +406,7 @@
         """
         return self.context.specifications(self.user,
             sort=SpecificationSort.DATE, quantity=quantity,
-            prejoin_people=False)
+            need_people=False, need_branches=False, need_workitems=False)
 
 
 class SpecificationAssignmentsView(HasSpecificationsView):

=== modified file 'lib/lp/blueprints/interfaces/specificationtarget.py'
--- lib/lp/blueprints/interfaces/specificationtarget.py	2013-04-03 03:09:04 +0000
+++ lib/lp/blueprints/interfaces/specificationtarget.py	2013-08-20 07:26:08 +0000
@@ -47,7 +47,7 @@
                 'approval or completion, for this object.'))),
         exported_as="all_specifications", as_of="devel")
 
-    _valid_specifications = exported(doNotSnapshot(
+    api_valid_specifications = exported(doNotSnapshot(
         CollectionField(
             title=_("Valid specifications"),
             value_type=Reference(schema=Interface),  # ISpecification, really.
@@ -59,7 +59,8 @@
         exported_as="valid_specifications", as_of="devel")
 
     def specifications(user, quantity=None, sort=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """Specifications for this target.
 
         The user specifies which user to use for calculation of visibility.
@@ -73,9 +74,15 @@
         it will show all specs, in others, all approved specs, and in
         others, all incomplete specs.
 
-        If prejoin_people=False is specified, then the assignee, drafter
-        and approver will not be prejoined. This can be used in
-        situations in which these are not rendered.
+        If need_people is True, then the assignee, drafter and approver will
+        be preloaded, if need_branches is True, linked_branches will be
+        preloaded, and if need_workitems is True, work_items will be preloaded.
+        """
+
+    def valid_specifications(**kwargs):
+        """Valid specifications for this target.
+
+        Any kwargs are passed to specifications.
         """
 
 

=== modified file 'lib/lp/blueprints/mail/notifications.py'
--- lib/lp/blueprints/mail/notifications.py	2013-03-05 08:24:25 +0000
+++ lib/lp/blueprints/mail/notifications.py	2013-08-20 07:26:08 +0000
@@ -92,7 +92,6 @@
                 workitems_delta['old'], workitems_delta['new'], 72)
             info_lines.append('Work items changed:')
             info_lines.append(workitems_diff)
-
     if not info_lines:
         # The specification was modified, but we don't yet support
         # sending notification for the change.

=== 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-20 07:26:08 +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")
@@ -395,7 +399,7 @@
         """
         title_counts = self._list_to_dict_of_frequency(titles)
 
-        for work_item in self.work_items:
+        for work_item in self._work_items:
             if (work_item.title not in title_counts or
                 title_counts[work_item.title] == 0):
                 work_item.deleted = True
@@ -418,9 +422,7 @@
         # deleted.
         self._deleteWorkItemsNotMatching(
             [wi['title'] for wi in new_work_items])
-        work_items = Store.of(self).find(
-            SpecificationWorkItem, specification=self, deleted=False)
-        work_items = list(work_items.order_by("sequence"))
+        work_items = self._work_items
         # At this point the list of new_work_items is necessarily the same
         # size (or longer) than the list of existing ones, so we can just
         # iterate over it updating the existing items and creating any new
@@ -456,6 +458,7 @@
         for sequence, item in to_insert:
             self.newWorkItem(item['title'], sequence, item['status'],
                              item['assignee'], item['milestone'])
+        del get_property_cache(self).work_items
 
     def setTarget(self, target):
         """See ISpecification."""
@@ -683,9 +686,7 @@
         delta.recordNewAndOld(("name", "priority", "definition_status",
                                "target", "approver", "assignee", "drafter",
                                "whiteboard", "workitems_text"))
-        delta.recordListAddedAndRemoved("bugs",
-                                        "bugs_linked",
-                                        "bugs_unlinked")
+        delta.recordListAddedAndRemoved("bugs", "bugs_linked", "bugs_unlinked")
 
         if delta.changes:
             changes = delta.changes
@@ -952,9 +953,10 @@
     """
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """See IHasSpecifications."""
-        # this should be implemented by the actual context class
+        # This should be implemented by the actual context class.
         raise NotImplementedError
 
     def _specification_sort(self, sort):
@@ -976,11 +978,16 @@
         user = getUtility(ILaunchBag).user
         return self.specifications(user, filter=[SpecificationFilter.ALL])
 
-    @property
-    def _valid_specifications(self):
+    def valid_specifications(self, **kwargs):
         """See IHasSpecifications."""
         user = getUtility(ILaunchBag).user
-        return self.specifications(user, filter=[SpecificationFilter.VALID])
+        return self.specifications(
+            user, filter=[SpecificationFilter.VALID], **kwargs)
+
+    @property
+    def api_valid_specifications(self):
+        return self.valid_specifications(
+            need_people=True, need_branches=True, need_workitems=True)
 
     def specificationCount(self, user):
         """See IHasSpecifications."""
@@ -1017,11 +1024,13 @@
         return cur.fetchall()
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         from lp.blueprints.model.specificationsearch import (
             search_specifications)
         return search_specifications(
-            self, [], user, sort, quantity, filter, prejoin_people)
+            self, [], user, sort, quantity, filter, need_people=need_people,
+            need_branches=need_branches, need_workitems=need_workitems)
 
     def getByURL(self, url):
         """See ISpecificationSet."""

=== 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-20 07:26:08 +0000
@@ -11,6 +11,8 @@
     'search_specifications',
     ]
 
+from collections import defaultdict
+
 from storm.expr import (
     And,
     Coalesce,
@@ -36,6 +38,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
@@ -56,8 +59,9 @@
 
 
 def search_specifications(context, base_clauses, user, sort=None,
-                          quantity=None, spec_filter=None, prejoin_people=True,
-                          tables=[], default_acceptance=False):
+                          quantity=None, spec_filter=None, tables=[],
+                          default_acceptance=False, need_people=True,
+                          need_branches=True, need_workitems=False):
     store = IStore(Specification)
     if not default_acceptance:
         default = SpecificationFilter.INCOMPLETE
@@ -106,12 +110,39 @@
         order = [sort]
     # Set the _known_viewers property for each specification, as well as
     # preloading the objects involved, if asked.
+    def preload_hook(rows):
+        person_ids = set()
+        work_items_by_spec = defaultdict(list)
+        for spec in rows:
+            if need_people: 
+                person_ids |= set(
+                    [spec._assigneeID, spec._approverID, spec._drafterID])
+            if need_branches:
+                get_property_cache(spec).linked_branches = []
+        if need_workitems:
+            work_items = load_referencing(
+                SpecificationWorkItem, rows, ['specification_id'])
+            for workitem in work_items:
+                person_ids.add(workitem.assignee_id)
+                work_items_by_spec[workitem.specification_id].append(workitem)
+        person_ids -= set([None])
+        if need_people:
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                person_ids, need_validity=True))
+        if need_workitems:
+            for spec in rows:
+                get_property_cache(spec).work_items = sorted(
+                    work_items_by_spec[spec.id], key=lambda wi: wi.sequence)
+        if need_branches:
+            spec_branches = load_referencing(
+                SpecificationBranch, rows, ['specificationID'])
+            for sbranch in spec_branches:
+                spec_cache = get_property_cache(sbranch.specification)
+                spec_cache.linked_branches.append(sbranch)
+
     decorators = []
-    preload_hook = None
     if user is not None and not IPersonRoles(user).in_admin:
         decorators.append(_make_cache_user_can_view_spec(user))
-    if prejoin_people:
-        preload_hook = _preload_specifications_related_objects
     results = store.using(*tables).find(
         Specification, *clauses).order_by(*order).config(limit=quantity)
     return DecoratedResultSet(
@@ -227,21 +258,6 @@
     return cache_user_can_view_spec
 
 
-def _preload_specifications_related_objects(rows):
-    person_ids = set()
-    for spec in rows:
-        person_ids |= set(
-            [spec._assigneeID, spec._approverID, spec._drafterID])
-        get_property_cache(spec).linked_branches = []
-    person_ids -= set([None])
-    list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
-        person_ids, need_validity=True))
-    spec_branches = load_referencing(
-        SpecificationBranch, rows, ['specificationID'])
-    for sbranch in spec_branches:
-        get_property_cache(sbranch.specification).linked_branches.append(
-            sbranch)
-
 
 def get_specification_started_clause():
     return Or(Not(Specification.implementation_status.is_in([

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2013-01-22 05:07:31 +0000
+++ lib/lp/blueprints/model/sprint.py	2013-08-20 07:26:08 +0000
@@ -163,18 +163,17 @@
         return self.specifications(user, filter=[SpecificationFilter.ALL])
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=False):
+                       need_people=False, need_branches=False,
+                       need_workitems=False):
         """See IHasSpecifications."""
-        # prejoin_people  is provided only for interface compatibility and
-        # prejoin_people=True is not implemented.
-        assert not prejoin_people
+        # need_* is provided only for interface compatibility and
+        # need_*=True is not implemented.
         if filter is None:
             filter = set([SpecificationFilter.ACCEPTED])
         tables, query = self.spec_filter_clause(user, filter)
         # import here to avoid circular deps
         from lp.blueprints.model.specification import Specification
-        store = Store.of(self)
-        results = store.using(*tables).find(Specification, *query)
+        results = Store.of(self).using(*tables).find(Specification, *query)
         if sort == SpecificationSort.DATE:
             order = (Desc(SprintSpecification.date_created), Specification.id)
             distinct = [SprintSpecification.date_created, Specification.id]

=== modified file 'lib/lp/blueprints/model/tests/test_specification.py'
--- lib/lp/blueprints/model/tests/test_specification.py	2013-01-25 03:30:08 +0000
+++ lib/lp/blueprints/model/tests/test_specification.py	2013-08-20 07:26:08 +0000
@@ -33,6 +33,7 @@
     )
 from lp.registry.errors import CannotChangeInformationType
 from lp.registry.model.milestone import Milestone
+from lp.services.propertycache import get_property_cache
 from lp.services.mail import stub
 from lp.services.webapp import canonical_url
 from lp.testing import (
@@ -222,10 +223,9 @@
         stub.test_emails = []
         spec = self.factory.makeSpecification()
         old_spec = Snapshot(spec, providing=providedBy(spec))
-        status = SpecificationWorkItemStatus.TODO
         new_work_item = {
             'title': u'A work item',
-            'status': status,
+            'status': SpecificationWorkItemStatus.TODO,
             'assignee': None,
             'milestone': None,
             'sequence': 0
@@ -233,10 +233,9 @@
 
         login_person(spec.owner)
         spec.updateWorkItems([new_work_item])
-        # In production this notification is fired by lazr.restful for changes
-        # in the specification form and notify(ObjectModifiedEvent(...)) for
-        # changes in the +workitems form. We need to do it ourselves in this
-        # test.
+        # For API requests, lazr.restful does the notify() call, for this test
+        # we need to call ourselves.
+        transaction.commit()
         notify(ObjectModifiedEvent(
             spec, old_spec, edited_fields=['workitems_text']))
         transaction.commit()
@@ -500,7 +499,7 @@
         spec.updateWorkItems([work_item1_data, work_item2_data])
 
         # And after calling updateWorkItems() we have 2 work items.
-        self.assertEqual(2, spec.work_items.count())
+        self.assertEqual(2, len(spec.work_items))
 
         # The data dicts we pass to updateWorkItems() have no sequence because
         # that's taken from their position on the list, so we update our data
@@ -523,7 +522,7 @@
         # Create two work-items in our database.
         wi1_data = self._createWorkItemAndReturnDataDict(spec)
         wi2_data = self._createWorkItemAndReturnDataDict(spec)
-        self.assertEqual(2, spec.work_items.count())
+        self.assertEqual(2, len(spec.work_items))
 
         # These are the work items we'll be inserting.
         new_wi1_data = dict(
@@ -546,7 +545,7 @@
         new_wi2_data['sequence'] = 2
         wi2_data['sequence'] = 3
 
-        self.assertEqual(4, spec.work_items.count())
+        self.assertEqual(4, len(spec.work_items))
         for data, obj in zip(work_items, list(spec.work_items)):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
@@ -571,8 +570,8 @@
         new_wi2_data['sequence'] = 2
         wi2_data['sequence'] = 3
 
-        self.assertEqual(4, spec.work_items.count())
-        for data, obj in zip(work_items, list(spec.work_items)):
+        self.assertEqual(4, len(spec.work_items))
+        for data, obj in zip(work_items, spec.work_items):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
         return spec, work_items
@@ -586,7 +585,7 @@
         work_items.append(new_wi3_data)
         spec.updateWorkItems(work_items)
 
-        self.assertEqual(5, spec.work_items.count())
+        self.assertEqual(5, len(spec.work_items))
         for data, obj in zip(work_items, list(spec.work_items)):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
@@ -597,7 +596,7 @@
         work_items.pop()
         spec.updateWorkItems(work_items)
 
-        self.assertEqual(3, spec.work_items.count())
+        self.assertEqual(3, len(spec.work_items))
         for data, obj in zip(work_items, list(spec.work_items)):
             self.assertThat(obj, MatchesStructure.byEquality(**data))
 
@@ -606,7 +605,7 @@
         login_person(spec.owner)
         # Create a work-item in our database.
         wi_data = self._createWorkItemAndReturnDataDict(spec)
-        self.assertEqual(1, spec.work_items.count())
+        self.assertEqual(1, len(spec.work_items))
 
         # This time we're only changing the existing work item; we'll change
         # its assignee and status.
@@ -614,29 +613,29 @@
                             assignee=spec.owner))
         spec.updateWorkItems([wi_data])
 
-        self.assertEqual(1, spec.work_items.count())
+        self.assertEqual(1, len(spec.work_items))
         self.assertThat(
             spec.work_items[0], MatchesStructure.byEquality(**wi_data))
 
     def test_updateWorkItems_deletes_all_if_given_empty_list(self):
         work_item = self.factory.makeSpecificationWorkItem()
         spec = work_item.specification
-        self.assertEqual(1, spec.work_items.count())
+        self.assertEqual(1, len(spec.work_items))
         spec.updateWorkItems([])
-        self.assertEqual(0, spec.work_items.count())
+        self.assertEqual(0, len(spec.work_items))
 
     def test_updateWorkItems_marks_removed_ones_as_deleted(self):
         spec = self.factory.makeSpecification()
         self._createWorkItemAndReturnDataDict(spec)
         wi2_data = self._createWorkItemAndReturnDataDict(spec)
-        self.assertEqual(2, spec.work_items.count())
+        self.assertEqual(2, len(spec.work_items))
         login_person(spec.owner)
 
         # We have two work items in the DB but now we want to update them to
         # keep just the second one. The first will be deleted and the sequence
         # of the second will be changed.
         spec.updateWorkItems([wi2_data])
-        self.assertEqual(1, spec.work_items.count())
+        self.assertEqual(1, len(spec.work_items))
         wi2_data['sequence'] = 0
         self.assertThat(
             spec.work_items[0], MatchesStructure.byEquality(**wi2_data))
@@ -648,12 +647,14 @@
         Return a dict with the title, status, assignee, milestone and sequence
         attributes of the spec.
         """
-        if spec.work_items.count() == 0:
+        del get_property_cache(spec).work_items
+        if len(spec.work_items) == 0:
             sequence = 0
         else:
             sequence = max(wi.sequence for wi in spec.work_items) + 1
         wi = self.factory.makeSpecificationWorkItem(
             specification=spec, sequence=sequence)
+        del get_property_cache(spec).work_items
         return dict(
             title=wi.title, status=wi.status, assignee=wi.assignee,
             milestone=wi.milestone, sequence=sequence)

=== 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-20 07:26:08 +0000
@@ -34,61 +34,48 @@
             product=product, name="spec2",
             status=SpecificationDefinitionStatus.OBSOLETE)
         self.assertNamesOfSpecificationsAre(
-            ["spec1"], product._valid_specifications)
+            ["spec1"], product.valid_specifications())
 
     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)
         self.assertNamesOfSpecificationsAre(
-            ["spec1"], distribution._valid_specifications)
+            ["spec1"], distribution.valid_specifications())
 
     def test_distroseries_all_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")
+            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()
@@ -115,7 +102,7 @@
             status=SpecificationDefinitionStatus.OBSOLETE)
         self.factory.makeSpecification(product=product, name="spec4")
         self.assertNamesOfSpecificationsAre(
-            ["spec1", "spec2"], productseries._valid_specifications)
+            ["spec1", "spec2"], productseries.valid_specifications())
 
     def test_projectgroup_all_specifications(self):
         projectgroup = self.factory.makeProject()
@@ -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)
 
@@ -145,7 +130,7 @@
             status=SpecificationDefinitionStatus.OBSOLETE)
         self.factory.makeSpecification(product=product3, name="spec3")
         self.assertNamesOfSpecificationsAre(
-            ["spec1"], projectgroup._valid_specifications)
+            ["spec1"], projectgroup.valid_specifications())
 
     def test_person_all_specifications(self):
         person = self.factory.makePerson(name="james-w")
@@ -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,10 +152,9 @@
         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)
+            ["spec1"], person.valid_specifications())
 
 
 class HasSpecificationsSnapshotTestCase(TestCaseWithFactory):
@@ -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/configure.zcml'
--- lib/lp/registry/configure.zcml	2013-04-03 03:09:04 +0000
+++ lib/lp/registry/configure.zcml	2013-08-20 07:26:08 +0000
@@ -1460,7 +1460,8 @@
             permission="launchpad.View"
             attributes="
                 visible_specifications
-                _valid_specifications
+                valid_specifications
+                api_valid_specifications
                 getAllowedSpecificationInformationTypes
                 getDefaultSpecificationInformationType
                 specifications" />

=== modified file 'lib/lp/registry/doc/distribution.txt'
--- lib/lp/registry/doc/distribution.txt	2013-05-01 21:23:16 +0000
+++ lib/lp/registry/doc/distribution.txt	2013-08-20 07:26:08 +0000
@@ -562,7 +562,7 @@
     ...         spec.definition_status = (
     ...             SpecificationDefinitionStatus.SUPERSEDED)
     ...     shim = spec.updateLifecycleStatus(owner)
-    >>> for spec in kubuntu._valid_specifications:
+    >>> for spec in kubuntu.valid_specifications():
     ...     print spec.name
     kde-desktopfile-langpacks
 

=== modified file 'lib/lp/registry/doc/projectgroup.txt'
--- lib/lp/registry/doc/projectgroup.txt	2013-05-01 21:23:16 +0000
+++ lib/lp/registry/doc/projectgroup.txt	2013-08-20 07:26:08 +0000
@@ -201,7 +201,7 @@
     >>> flush_database_updates()
 
 We can get all the specifications via the visible_specifications property,
-and all valid specifications via the _valid_specifications property:
+and all valid specifications via the valid_specifications method:
 
     >>> for spec in mozilla.visible_specifications:
     ...    print spec.name
@@ -211,7 +211,7 @@
     mergewin
     e4x
 
-    >>> for spec in mozilla._valid_specifications:
+    >>> for spec in mozilla.valid_specifications():
     ...    print spec.name
     svg-support
     canvas
@@ -313,7 +313,7 @@
     >>> firefox.active = True
 
 We can get all the specifications via the visible_specifications property,
-and all valid specifications via the _valid_specifications property:
+and all valid specifications via the valid_specifications method:
 
     >>> for spec in mozilla_series_1_0.visible_specifications:
     ...    print spec.name
@@ -323,7 +323,7 @@
     mergewin
     e4x
 
-    >>> for spec in mozilla_series_1_0._valid_specifications:
+    >>> for spec in mozilla_series_1_0.valid_specifications():
     ...    print spec.name
     svg-support
     canvas

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2013-07-11 06:12:20 +0000
+++ lib/lp/registry/model/distribution.py	2013-08-20 07:26:08 +0000
@@ -868,7 +868,8 @@
             {self: source_package_names})
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """See `IHasSpecifications`.
 
         In the case of distributions, there are two kinds of filtering,
@@ -880,7 +881,9 @@
         """
         base_clauses = [Specification.distributionID == self.id]
         return search_specifications(
-            self, base_clauses, user, sort, quantity, filter, prejoin_people)
+            self, base_clauses, user, sort, quantity, filter,
+            need_people=need_people, need_branches=need_branches,
+            need_workitems=need_workitems)
 
     def getSpecification(self, name):
         """See `ISpecificationTarget`."""

=== modified file 'lib/lp/registry/model/distroseries.py'
--- lib/lp/registry/model/distroseries.py	2013-07-11 06:12:20 +0000
+++ lib/lp/registry/model/distroseries.py	2013-08-20 07:26:08 +0000
@@ -766,7 +766,8 @@
         return self.distribution.official_bug_tags
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """See IHasSpecifications.
 
         In this case the rules for the default behaviour cover three things:
@@ -778,8 +779,9 @@
         """
         base_clauses = [Specification.distroseriesID == self.id]
         return search_specifications(
-            self, base_clauses, user, sort, quantity, filter, prejoin_people,
-            default_acceptance=True)
+            self, base_clauses, user, sort, quantity, filter,
+            default_acceptance=True, need_people=need_people,
+            need_branches=need_branches, need_workitems=need_workitems)
 
     def getDistroSeriesLanguage(self, language):
         """See `IDistroSeries`."""

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2013-08-08 04:11:59 +0000
+++ lib/lp/registry/model/person.py	2013-08-20 07:26:08 +0000
@@ -823,7 +823,8 @@
         return "%s (%s)" % (self.displayname, self.name)
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True, in_progress=False):
+                       in_progress=False, need_people=True, need_branches=True,
+                       need_workitems=False):
         """See `IHasSpecifications`."""
         from lp.blueprints.model.specificationsubscription import (
             SpecificationSubscription,
@@ -874,7 +875,9 @@
                     SpecificationFilter.STARTED])
 
         return search_specifications(
-            self, clauses, user, sort, quantity, list(filter), prejoin_people)
+            self, clauses, user, sort, quantity, list(filter),
+            need_people=need_people, need_branches=need_branches,
+            need_workitems=need_workitems)
 
     # XXX: Tom Berger 2008-04-14 bug=191799:
     # The implementation of these functions

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2013-07-11 06:12:20 +0000
+++ lib/lp/registry/model/product.py	2013-08-20 07:26:08 +0000
@@ -1416,12 +1416,14 @@
         return True
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """See `IHasSpecifications`."""
-
         base_clauses = [Specification.productID == self.id]
         return search_specifications(
-            self, base_clauses, user, sort, quantity, filter, prejoin_people)
+            self, base_clauses, user, sort, quantity, filter,
+            need_people=need_people, need_branches=need_branches,
+            need_workitems=need_workitems)
 
     def getSpecification(self, name):
         """See `ISpecificationTarget`."""

=== modified file 'lib/lp/registry/model/productseries.py'
--- lib/lp/registry/model/productseries.py	2013-04-03 03:09:04 +0000
+++ lib/lp/registry/model/productseries.py	2013-08-20 07:26:08 +0000
@@ -316,7 +316,8 @@
         return self == self.product.development_focus
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         """See IHasSpecifications.
 
         The rules for filtering are that there are three areas where you can
@@ -329,8 +330,9 @@
         """
         base_clauses = [Specification.productseriesID == self.id]
         return search_specifications(
-            self, base_clauses, user, sort, quantity, filter, prejoin_people,
-            default_acceptance=True)
+            self, base_clauses, user, sort, quantity, filter,
+            default_acceptance=True, need_people=need_people,
+            need_branches=need_branches, need_workitems=need_workitems)
 
     @property
     def all_specifications(self):

=== modified file 'lib/lp/registry/model/projectgroup.py'
--- lib/lp/registry/model/projectgroup.py	2013-07-12 06:14:22 +0000
+++ lib/lp/registry/model/projectgroup.py	2013-08-20 07:26:08 +0000
@@ -244,7 +244,8 @@
         return query, ['Product', 'Specification', 'SprintSpecification']
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       series=None, prejoin_people=True):
+                       series=None, need_people=True, need_branches=True,
+                       need_workitems=False):
         """See `IHasSpecifications`."""
         base_clauses = [
             Specification.productID == Product.id,
@@ -256,8 +257,9 @@
                 Join(ProductSeries,
                 Specification.productseriesID == ProductSeries.id))
         return search_specifications(
-            self, base_clauses, user, sort, quantity, filter, prejoin_people,
-            tables=tables)
+            self, base_clauses, user, sort, quantity, filter, tables=tables,
+            need_people=need_people, need_branches=need_branches,
+            need_workitems=need_workitems)
 
     def _customizeSearchParams(self, search_params):
         """Customize `search_params` for this milestone."""
@@ -617,10 +619,11 @@
         self.name = name
 
     def specifications(self, user, sort=None, quantity=None, filter=None,
-                       prejoin_people=True):
+                       need_people=True, need_branches=True,
+                       need_workitems=False):
         return self.project.specifications(
-            user, sort, quantity, filter, self.name,
-            prejoin_people=prejoin_people)
+            user, sort, quantity, filter, self.name, need_people=need_people,
+            need_branches=need_branches, need_workitems=need_workitems)
 
     @property
     def title(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-20 07:26:08 +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.api_valid_specifications:
+                spec.workitems_text
+        self.assertThat(recorder, HasQueryCount(Equals(4)))
+
 
 class TestDistroSeriesPackaging(TestCaseWithFactory):
 

=== modified file 'lib/lp/registry/tests/test_product.py'
--- lib/lp/registry/tests/test_product.py	2013-06-20 05:50:00 +0000
+++ lib/lp/registry/tests/test_product.py	2013-08-20 07:26:08 +0000
@@ -813,8 +813,8 @@
             'owner', 'parent_subscription_target', 'project', 'title', )),
         'launchpad.View': set((
             '_getOfficialTagClause', 'visible_specifications',
-            '_valid_specifications', 'active_or_packaged_series',
-            'aliases', 'all_milestones',
+            'valid_specifications', 'api_valid_specifications',
+            'active_or_packaged_series', 'aliases', 'all_milestones',
             'allowsTranslationEdits', 'allowsTranslationSuggestions',
             'announce', 'answer_contacts', 'answers_usage', 'autoupdate',
             'blueprints_usage', 'branch_sharing_policy',

=== modified file 'lib/lp/registry/tests/test_productseries.py'
--- lib/lp/registry/tests/test_productseries.py	2013-06-20 05:50:00 +0000
+++ lib/lp/registry/tests/test_productseries.py	2013-08-20 07:26:08 +0000
@@ -627,13 +627,13 @@
             'parent_subscription_target', 'product', 'productID', 'series')),
         'launchpad.View': set((
             'visible_specifications', '_getOfficialTagClause',
-            '_valid_specifications', 'active', 'all_milestones',
-            'answers_usage', 'blueprints_usage', 'branch',
-            'bug_reported_acknowledgement', 'bug_reporting_guidelines',
-            'bug_subscriptions', 'bug_supervisor', 'bug_tracking_usage',
-            'bugtargetname',
-            'codehosting_usage', 'createBug', 'datecreated', 'displayname',
-            'driver', 'drivers', 'enable_bugfiling_duplicate_search',
+            'valid_specifications', 'api_valid_specifications',
+            'active', 'all_milestones', 'answers_usage', 'blueprints_usage',
+            'branch', 'bug_reported_acknowledgement',
+            'bug_reporting_guidelines', 'bug_subscriptions', 'bug_supervisor',
+            'bug_tracking_usage', 'bugtargetname', 'codehosting_usage',
+            'createBug', 'datecreated', 'displayname', 'driver', 'drivers',
+            'enable_bugfiling_duplicate_search',
             'getAllowedSpecificationInformationTypes',
             'getBugSummaryContextWhereClause',
             'getBugTaskWeightFunction', 'getCachedReleases',

=== 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-20 07:26:08 +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]