Merge lp:~abentley/launchpad/fix-bp-timeouts into lp:launchpad

[Aaron Bentley]

Fix timeouts checking blueprint visiblity

= Summary =
Fix the following bugs:
Bug #1075569: Timeout loading milestone because of blueprints
Bug #1077980: viewing project page leads to timeout when private blueprints exists
Bug #1078239: loading team blueprints leads to timeout Person:+index
Bug #1079390: Person:+upcomingwork timeout querying specifiations

== Proposed fix ==
Switch from get_specification_privacy_filter to visible_specification_query everywhere.

== Pre-implementation notes ==
None

== LOC Rationale ==
Part of private products.

== Implementation details ==
visible_specification_query does not guarantee distinct results, so added config(distinct=True) as needed.

Since get_specification_privacy_filter is no longer used, it is deleted, and its tests migrated to visible_specification_query.

TestPersonUpcomingWork.test_container_progressbar assumed that WorkItem containers were in a specific order, based on the id of their initial WorkItem. This work changed the ordering, but a consistent ordering makes sense, so I've made the ordering explicit.

All the call sites of get_specification_filters now use visible_specification_query, so its assume_product_active behaviour can now become mandatory.

_preload_specifications_people is used by both Product and Distribution, so tweaked Distribution to supply tables.

In sharingjob, replaced get_specification_privacy_filter by introducing a sub-select, because otherwise, negating visible_specification_query would be very tricky.

Fixed lint in SpecificationWorkItem.__init__.

Fixed long line by assigning LatestPersonSourcePackageReleaseCache.upload_distroseries_id to a local variable.

== Tests ==
bin/test -v

== Demo and Q/A ==
Go to the staging versions of all pages linked in the bugs and see whether they consistently time out.

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/blueprints/model/specificationworkitem.py
  lib/lp/registry/model/sharingjob.py
  lib/lp/blueprints/tests/test_specification.py
  lib/lp/blueprints/model/specification.py
  lib/lp/blueprints/model/sprint.py
  lib/lp/registry/model/product.py
  lib/lp/registry/model/person.py
  lib/lp/registry/model/milestone.py
  lib/lp/registry/model/distribution.py

[j.c.sackett (jcsackett)]

Looks good, thanks.

review: Approve

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2012-11-12 16:40:28 +0000
+++ lib/lp/blueprints/model/specification.py	2012-11-16 20:58:25 +0000
@@ -4,7 +4,6 @@
 __metaclass__ = type
 __all__ = [
     'get_specification_filters',
-    'get_specification_privacy_filter',
     'HasSpecificationsMixin',
     'recursive_blocked_query',
     'recursive_dependent_query',
@@ -960,7 +959,7 @@
         elif sort == SpecificationSort.DATE:
             return (Desc(Specification.datecreated), Specification.id)
 
-    def _preload_specifications_people(self, clauses):
+    def _preload_specifications_people(self, tables, clauses):
         """Perform eager loading of people and their validity for query.
 
         :param query: a string query generated in the 'specifications'
@@ -1001,7 +1000,7 @@
                     index += 1
                     decorator(person, column)
 
-        results = Store.of(self).find(Specification, *clauses)
+        results = Store.of(self).using(*tables).find(Specification, *clauses)
         return DecoratedResultSet(results, pre_iter_hook=cache_people)
 
     @property
@@ -1230,58 +1229,6 @@
         return Specification.get(spec_id)
 
 
-def get_specification_privacy_filter(user):
-    """Return a Storm expression for filtering specifications by privacy.
-
-    :param user: A Person ID or a column reference.
-    :return: A Storm expression to check if a peron has access grants
-         for a specification.
-    """
-    # Avoid circular imports.
-    from lp.registry.model.accesspolicy import (
-        AccessArtifact,
-        AccessPolicy,
-        AccessPolicyGrantFlat,
-        )
-    public_specification_filter = (
-        Specification.information_type.is_in(PUBLIC_INFORMATION_TYPES))
-    if user is None:
-        return public_specification_filter
-    return Or(
-        public_specification_filter,
-        Specification.id.is_in(
-            Select(
-                Specification.id,
-                tables=(
-                    Specification,
-                    Join(
-                        AccessPolicy,
-                        And(
-                            Or(
-                                Specification.productID ==
-                                    AccessPolicy.product_id,
-                                Specification.distributionID ==
-                                    AccessPolicy.distribution_id),
-                            Specification.information_type ==
-                                AccessPolicy.type)),
-                    Join(
-                        AccessPolicyGrantFlat,
-                        AccessPolicy.id == AccessPolicyGrantFlat.policy_id),
-                    LeftJoin(
-                        AccessArtifact,
-                        AccessPolicyGrantFlat.abstract_artifact_id ==
-                            AccessArtifact.id),
-                    Join(
-                        TeamParticipation,
-                        And(
-                            TeamParticipation.team ==
-                                AccessPolicyGrantFlat.grantee_id,
-                            TeamParticipation.person == user))),
-                where=Or(
-                    AccessPolicyGrantFlat.abstract_artifact_id == None,
-                    AccessArtifact.specification_id == Specification.id))))
-
-
 def visible_specification_query(user):
     """Return a Storm expression and list of tables for filtering
     specifications by privacy.
@@ -1324,21 +1271,13 @@
     return tables, clauses
 
 
-def get_specification_filters(filter, assume_product_active=False):
+def get_specification_filters(filter):
     """Return a list of Storm expressions for filtering Specifications.
 
     :param filters: A collection of SpecificationFilter and/or strings.
         Strings are used for text searches.
-    :param assume_product_active: If True, assume the Product is active,
-        instead of ensuring it is active.
     """
-    from lp.registry.model.product import Product
     clauses = []
-    # If Product is used, it must be active.
-    if not assume_product_active:
-        clauses.extend([Or(Specification.product == None,
-                        Not(Specification.productID.is_in(Select(Product.id,
-                        Product.active == False))))])
     # ALL is the trump card.
     if SpecificationFilter.ALL in filter:
         return clauses

=== modified file 'lib/lp/blueprints/model/specificationworkitem.py'
--- lib/lp/blueprints/model/specificationworkitem.py	2012-04-05 13:05:04 +0000
+++ lib/lp/blueprints/model/specificationworkitem.py	2012-11-16 20:58:25 +0000
@@ -29,6 +29,7 @@
     implements(ISpecificationWorkItem)
 
     __storm_table__ = 'SpecificationWorkItem'
+    __storm_order__ = 'id'
 
     id = Int(primary=True)
     title = Unicode(allow_none=False)
@@ -53,12 +54,12 @@
 
     def __init__(self, title, status, specification, assignee, milestone,
                  sequence):
-        self.title=title
-        self.status=status
+        self.title = title
+        self.status = status
         self.specification = specification
-        self.assignee=assignee
-        self.milestone=milestone
-        self.sequence=sequence
+        self.assignee = assignee
+        self.milestone = milestone
+        self.sequence = sequence
 
     @property
     def is_complete(self):

=== modified file 'lib/lp/blueprints/model/sprint.py'
--- lib/lp/blueprints/model/sprint.py	2012-11-12 16:40:28 +0000
+++ lib/lp/blueprints/model/sprint.py	2012-11-16 20:58:25 +0000
@@ -155,8 +155,7 @@
         if len(statuses) > 0:
             query.append(Or(*statuses))
         # Filter for specification text
-        query.extend(
-            get_specification_filters(filter, assume_product_active=True))
+        query.extend(get_specification_filters(filter))
         return tables, query
 
     def all_specifications(self, user):

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2012-10-16 14:47:54 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2012-11-16 20:58:25 +0000
@@ -33,8 +33,8 @@
 from lp.blueprints.errors import TargetAlreadyHasSpecification
 from lp.blueprints.interfaces.specification import ISpecificationSet
 from lp.blueprints.model.specification import (
-    get_specification_privacy_filter,
     Specification,
+    visible_specification_query,
     )
 from lp.registry.enums import (
     SharingPermission,
@@ -397,8 +397,8 @@
                 specification.target.owner, specification,
                 error_expected=False, attribute='name', value='foo')
 
-    def test_get_specification_privacy_filter(self):
-        # get_specification_privacy_filter() returns a Storm expression
+    def test_visible_specification_query(self):
+        # visible_specification_query returns a Storm expression
         # that can be used to filter specifications by their visibility-
         owner = self.factory.makePerson()
         product = self.factory.makeProduct(
@@ -413,30 +413,32 @@
         all_specs = [
             public_spec, proprietary_spec_1, proprietary_spec_2]
         store = Store.of(product)
-        specs_for_anon = store.find(
-            Specification, get_specification_privacy_filter(None),
-            Specification.productID == product.id)
-        self.assertContentEqual([public_spec], specs_for_anon)
+        tables, query = visible_specification_query(None)
+        specs_for_anon = store.using(*tables).find(
+            Specification,
+            Specification.productID == product.id, *query)
+        self.assertContentEqual([public_spec],
+                                specs_for_anon.config(distinct=True))
         # Product owners havae grants on the product, the privacy
         # filter returns thus all specifications for them.
-        specs_for_owner = store.find(
-            Specification, get_specification_privacy_filter(owner.id),
-            Specification.productID == product.id)
+        tables, query = visible_specification_query(owner.id)
+        specs_for_owner = store.using(*tables).find(
+            Specification, Specification.productID == product.id, *query)
         self.assertContentEqual(all_specs, specs_for_owner)
         # The filter returns only public specs for ordinary users.
         user = self.factory.makePerson()
-        specs_for_other_user = store.find(
-            Specification, get_specification_privacy_filter(user.id),
-            Specification.productID == product.id)
+        tables, query = visible_specification_query(user.id)
+        specs_for_other_user = store.using(*tables).find(
+            Specification, Specification.productID == product.id, *query)
         self.assertContentEqual([public_spec], specs_for_other_user)
         # If the user has a grant for a specification, the filter returns
         # this specification too.
         with person_logged_in(owner):
             getUtility(IService, 'sharing').ensureAccessGrants(
                 [user], owner, specifications=[proprietary_spec_1])
-        specs_for_other_user = store.find(
-            Specification, get_specification_privacy_filter(user.id),
-            Specification.productID == product.id)
+        tables, query = visible_specification_query(user.id)
+        specs_for_other_user = store.using(*tables).find(
+            Specification, Specification.productID == product.id, *query)
         self.assertContentEqual(
             [public_spec, proprietary_spec_1], specs_for_other_user)
 

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-11-02 00:53:58 +0000
+++ lib/lp/registry/model/distribution.py	2012-11-16 20:58:25 +0000
@@ -952,7 +952,8 @@
                     constraint)
 
         if prejoin_people:
-            results = self._preload_specifications_people(query)
+            results = self._preload_specifications_people([Specification],
+                                                          query)
         else:
             results = Store.of(self).find(
                 Specification,

=== modified file 'lib/lp/registry/model/milestone.py'
--- lib/lp/registry/model/milestone.py	2012-10-26 08:59:24 +0000
+++ lib/lp/registry/model/milestone.py	2012-11-16 20:58:25 +0000
@@ -39,8 +39,8 @@
 
 from lp.app.errors import NotFoundError
 from lp.blueprints.model.specification import (
-    get_specification_privacy_filter,
     Specification,
+    visible_specification_query
     )
 from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
@@ -155,10 +155,10 @@
         """See `IMilestoneData`"""
         from lp.registry.model.person import Person
         store = Store.of(self.target)
-        origin = [
-            Specification,
+        origin, clauses = visible_specification_query(user)
+        origin.extend([
             LeftJoin(Person, Specification.assigneeID == Person.id),
-            ]
+            ])
         milestones = self._milestone_ids_expr(user)
 
         results = store.using(*origin).find(
@@ -176,12 +176,12 @@
                                 milestones),
                             SpecificationWorkItem.deleted == False)),
                     all=True)),
-            get_specification_privacy_filter(user))
-        results.config(distinct=True)
+            *clauses)
         ordered_results = results.order_by(Desc(Specification.priority),
                                            Specification.definition_status,
                                            Specification.implementation_status,
                                            Specification.title)
+        ordered_results.config(distinct=True)
         mapper = lambda row: row[0]
         return DecoratedResultSet(ordered_results, mapper)
 

=== modified file 'lib/lp/registry/model/person.py'
--- lib/lp/registry/model/person.py	2012-11-14 02:03:32 +0000
+++ lib/lp/registry/model/person.py	2012-11-16 20:58:25 +0000
@@ -130,10 +130,10 @@
     )
 from lp.blueprints.model.specification import (
     get_specification_filters,
-    get_specification_privacy_filter,
     HasSpecificationsMixin,
     Specification,
     spec_started_clause,
+    visible_specification_query,
     )
 from lp.blueprints.model.specificationworkitem import SpecificationWorkItem
 from lp.bugs.interfaces.bugtarget import IBugTarget
@@ -857,7 +857,8 @@
                     Select(SpecificationSubscription.specificationID,
                         [SpecificationSubscription.person == self]
                     )))
-        clauses = [Or(*role_clauses), get_specification_privacy_filter(user)]
+        tables, clauses = visible_specification_query(user)
+        clauses.append(Or(*role_clauses))
         # Defaults for completeness: if nothing is said about completeness
         # then we want to show INCOMPLETE.
         if SpecificationFilter.COMPLETE not in filter:
@@ -867,7 +868,7 @@
             filter.add(SpecificationFilter.INCOMPLETE)
 
         clauses.extend(get_specification_filters(filter))
-        results = Store.of(self).find(Specification, *clauses)
+        results = Store.of(self).using(*tables).find(Specification, *clauses)
         # The default sort is priority descending, so only explictly sort for
         # DATE.
         if sort == SpecificationSort.DATE:
@@ -876,6 +877,7 @@
             sort = None
         if sort is not None:
             results = results.order_by(sort)
+        results.config(distinct=True)
         if quantity is not None:
             results = results[:quantity]
         return results
@@ -1466,23 +1468,23 @@
         from lp.registry.model.distribution import Distribution
         store = Store.of(self)
         WorkItem = SpecificationWorkItem
-        origin = [
-            WorkItem,
-            Join(Specification, WorkItem.specification == Specification.id),
+        origin, query = visible_specification_query(user)
+        origin.extend([
+            Join(WorkItem, WorkItem.specification == Specification.id),
             # WorkItems may not have a milestone and in that case they inherit
             # the one from the spec.
             Join(Milestone,
                  Coalesce(WorkItem.milestone_id,
                           Specification.milestoneID) == Milestone.id),
-            ]
+            ])
         today = datetime.today().date()
-        query = And(
-            get_specification_privacy_filter(user),
+        query.extend([
             Milestone.dateexpected <= date, Milestone.dateexpected >= today,
             WorkItem.deleted == False,
             OR(WorkItem.assignee_id.is_in(self.participant_ids),
-               Specification.assigneeID.is_in(self.participant_ids)))
-        result = store.using(*origin).find(WorkItem, query)
+               Specification.assigneeID.is_in(self.participant_ids))])
+        result = store.using(*origin).find(WorkItem, *query)
+        result.config(distinct=True)
 
         def eager_load(workitems):
             specs = bulk.load_related(
@@ -2844,6 +2846,8 @@
             pass
         elif uploader_only:
             lpspr = ClassAlias(LatestPersonSourcePackageReleaseCache, 'lpspr')
+            upload_distroseries_id = (
+                LatestPersonSourcePackageReleaseCache.upload_distroseries_id)
             clauses.append(Not(Exists(Select(1,
             where=And(
                 lpspr.sourcepackagename_id ==
@@ -2851,7 +2855,7 @@
                 lpspr.upload_archive_id ==
                     LatestPersonSourcePackageReleaseCache.upload_archive_id,
                 lpspr.upload_distroseries_id ==
-                    LatestPersonSourcePackageReleaseCache.upload_distroseries_id,
+                    upload_distroseries_id,
                 lpspr.archive_purpose != ArchivePurpose.PPA,
                 lpspr.maintainer_id == self.id),
             tables=lpspr))))

=== modified file 'lib/lp/registry/model/product.py'
--- lib/lp/registry/model/product.py	2012-11-15 23:05:42 +0000
+++ lib/lp/registry/model/product.py	2012-11-16 20:58:25 +0000
@@ -97,11 +97,11 @@
     )
 from lp.blueprints.model.specification import (
     get_specification_filters,
-    get_specification_privacy_filter,
     HasSpecificationsMixin,
     Specification,
     SPECIFICATION_POLICY_ALLOWED_TYPES,
     SPECIFICATION_POLICY_DEFAULT_TYPES,
+    visible_specification_query,
     )
 from lp.blueprints.model.sprint import HasSprintsMixin
 from lp.bugs.interfaces.bugsummary import IBugSummaryDimension
@@ -1408,14 +1408,15 @@
         #  - completeness.
         #  - informational.
         #
-        clauses = [Specification.product == self,
-                   get_specification_privacy_filter(user)]
+        tables, clauses = visible_specification_query(user)
+        clauses.append(Specification.product == self)
         clauses.extend(get_specification_filters(filter))
         if prejoin_people:
-            results = self._preload_specifications_people(clauses)
+            results = self._preload_specifications_people(tables, clauses)
         else:
-            results = Store.of(self).find(Specification, *clauses)
-        results.order_by(order)
+            tableset = Store.of(self).using(*tables)
+            results = tableset.find(Specification, *clauses)
+        results.order_by(order).config(distinct=True)
         if quantity is not None:
             results = results[:quantity]
         return results

=== modified file 'lib/lp/registry/model/sharingjob.py'
--- lib/lp/registry/model/sharingjob.py	2012-09-28 06:15:58 +0000
+++ lib/lp/registry/model/sharingjob.py	2012-11-16 20:58:25 +0000
@@ -44,8 +44,8 @@
 from lp.app.enums import InformationType
 from lp.blueprints.interfaces.specification import ISpecification
 from lp.blueprints.model.specification import (
-    get_specification_privacy_filter,
     Specification,
+    visible_specification_query,
     )
 from lp.blueprints.model.specificationsubscription import (
     SpecificationSubscription,
@@ -440,8 +440,7 @@
                     sub.person, self.requestor, ignore_permissions=True)
         if specification_filters:
             specification_filters.append(
-                Not(get_specification_privacy_filter(
-                    SpecificationSubscription.personID)))
+                spec_not_visible(SpecificationSubscription.personID))
             tables = (
                 SpecificationSubscription,
                 Join(
@@ -455,3 +454,10 @@
             for sub in specifications_subscriptions:
                 sub.specification.unsubscribe(
                     sub.person, self.requestor, ignore_permissions=True)
+
+
+def spec_not_visible(person_id):
+    """Return an expression for finding specs not visible to the person."""
+    tables, clauses = visible_specification_query(person_id)
+    subselect = Select(Specification.id, tables=tables, where=And(clauses))
+    return Not(Specification.id.is_in(subselect))