launchpad-reviewers team mailing list archive

[Steve Kowalik <stevenk@xxxxxxxxxx>]

Steve Kowalik has proposed merging lp:~stevenk/launchpad/cache-known-viewers-specification into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1100617 in Launchpad itself: "Blueprint searches don't precache permission checks"
  https://bugs.launchpad.net/launchpad/+bug/1100617

For more details, see:
https://code.launchpad.net/~stevenk/launchpad/cache-known-viewers-specification/+merge/145299

Set known viewers for each specification by way of the property cache during search_specifications. This also involved tearing out _preload_specifications_people(), which was effectively trying to say IPersonSet.getPrecachedPersonsFromIDs() in 45 lines.

I have also rewritten the last chunk of ISpecification.userCanView() to back onto Specification.access_{grants,policy}, and cleaned up a little bit of lint in the tests.
-- 
https://code.launchpad.net/~stevenk/launchpad/cache-known-viewers-specification/+merge/145299
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/cache-known-viewers-specification into lp:launchpad.

=== modified file 'lib/lp/blueprints/browser/tests/test_views.py'
--- lib/lp/blueprints/browser/tests/test_views.py	2012-10-09 10:28:02 +0000
+++ lib/lp/blueprints/browser/tests/test_views.py	2013-01-29 03:13:29 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2011 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).
 
 """
@@ -85,18 +85,15 @@
         logout()
         self.invalidate_and_render(browser, target, url)
         self.assertThat(
-            collector,
-            HasQueryCount(LessThan(no_assignees_count + 5)))
+            collector, HasQueryCount(LessThan(no_assignees_count + 5)))
 
     def test_product_query_counts_scale_below_unique_people(self):
         self.check_query_counts_scaling_with_unique_people(
-            self.factory.makeProduct(),
-            'product')
+            self.factory.makeProduct(), 'product')
 
     def test_distro_query_counts_scale_below_unique_people(self):
         self.check_query_counts_scaling_with_unique_people(
-            self.factory.makeDistribution(),
-            'distribution')
+            self.factory.makeDistribution(), 'distribution')
 
 
 def test_suite():
@@ -115,8 +112,7 @@
         one_test = LayeredDocFileSuite(
             path, setUp=setUp, tearDown=tearDown,
             layer=DatabaseFunctionalLayer,
-            stdout_logging_level=logging.WARNING
-            )
+            stdout_logging_level=logging.WARNING)
         suite.addTest(one_test)
 
     return suite

=== modified file 'lib/lp/blueprints/model/specification.py'
--- lib/lp/blueprints/model/specification.py	2013-01-25 03:30:08 +0000
+++ lib/lp/blueprints/model/specification.py	2013-01-29 03:13:29 +0000
@@ -24,13 +24,7 @@
     SQLRelatedJoin,
     StringCol,
     )
-from storm.expr import (
-    And,
-    In,
-    Join,
-    Or,
-    Select,
-    )
+from storm.expr import Join
 from storm.locals import (
     Desc,
     SQL,
@@ -91,7 +85,6 @@
 from lp.registry.interfaces.person import validate_public_person
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
-from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.constants import (
     DEFAULT,
     UTC_NOW,
@@ -925,11 +918,8 @@
     def userCanView(self, user):
         """See `ISpecification`."""
         # Avoid circular imports.
-        from lp.registry.model.accesspolicy import (
-            AccessArtifact,
-            AccessPolicy,
-            AccessPolicyGrantFlat,
-            )
+        from lp.blueprints.model.specificationsearch import (
+            get_specification_privacy_filter)
         if self.information_type in PUBLIC_INFORMATION_TYPES:
             return True
         if user is None:
@@ -937,42 +927,12 @@
         if user.id in self._known_viewers:
             return True
 
-        # Check if access has been granted to the user for either
-        # the pillar of this specification or the specification
-        # itself.
-        #
-        # A DB constraint ensures that either Specification.product or
-        # Specification.distribution is not null.
-        if self.product is not None:
-            pillar_clause = AccessPolicy.product == self.productID
-        else:
-            pillar_clause = AccessPolicy.distribution == self.distributionID
-        tables = (
-            AccessPolicyGrantFlat,
-            Join(
-                AccessPolicy,
-                AccessPolicyGrantFlat.policy_id == AccessPolicy.id),
-            Join(
-                TeamParticipation,
-                AccessPolicyGrantFlat.grantee == TeamParticipation.teamID)
-            )
-        grants = Store.of(self).using(*tables).find(
-            AccessPolicyGrantFlat,
-            pillar_clause,
-            Or(
-                And(
-                    AccessPolicyGrantFlat.abstract_artifact == None,
-                    AccessPolicy.type == self.information_type),
-                In(
-                    AccessPolicyGrantFlat.abstract_artifact_id,
-                    Select(
-                        AccessArtifact.id,
-                        AccessArtifact.specification_id == self.id))),
-            TeamParticipation.personID == user.id)
-        if grants.is_empty():
-            return False
-        self._known_viewers.add(user.id)
-        return True
+        if not Store.of(self).find(
+            Specification, Specification.id == self.id,
+            *get_specification_privacy_filter(user)).is_empty():
+            self._known_viewers.add(user.id)
+            return True
+        return False
 
 
 class HasSpecificationsMixin:

=== modified file 'lib/lp/blueprints/model/specificationsearch.py'
--- lib/lp/blueprints/model/specificationsearch.py	2013-01-22 06:42:25 +0000
+++ lib/lp/blueprints/model/specificationsearch.py	2013-01-29 03:13:29 +0000
@@ -24,6 +24,7 @@
     Desc,
     SQL,
     )
+from zope.component import getUtility
 
 from lp.app.enums import PUBLIC_INFORMATION_TYPES
 from lp.blueprints.enums import (
@@ -36,8 +37,10 @@
 from lp.blueprints.model.specification import Specification
 from lp.registry.interfaces.distribution import IDistribution
 from lp.registry.interfaces.distroseries import IDistroSeries
+from lp.registry.interfaces.person import IPersonSet
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.interfaces.role import IPersonRoles
 from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
@@ -47,6 +50,7 @@
     ArrayIntersects,
     fti_search,
     )
+from lp.services.propertycache import get_property_cache
 
 
 def search_specifications(context, base_clauses, user, sort=None,
@@ -98,11 +102,18 @@
             order.extend([Desc(Specification.datecreated), Specification.id])
     else:
         order = [sort]
+    # Set the _known_viewers property for each specification, as well as
+    # preloading the people involved, if asked.
+    decorators = []
+    if user is not None and not IPersonRoles(user).in_admin:
+        decorators.append(_make_cache_user_can_view_spec(user))
     if prejoin_people:
-        results = _preload_specifications_people(tables, clauses)
-    else:
-        results = store.using(*tables).find(Specification, *clauses)
-    return results.order_by(*order).config(limit=quantity)
+        decorators.append(_preload_specification_people())
+    results = store.using(*tables).find(
+        Specification, *clauses).order_by(*order).config(limit=quantity)
+    return DecoratedResultSet(
+        results,
+        lambda row: reduce(lambda task, dec: dec(task), decorators, row))
 
 
 def get_specification_active_product_filter(context):
@@ -125,6 +136,8 @@
 
     if user is None:
         return [public_spec_filter]
+    elif IPersonRoles.providedBy(user):
+        user = user.person
 
     artifact_grant_query = Coalesce(
         ArrayIntersects(
@@ -201,52 +214,23 @@
     return clauses
 
 
-def _preload_specifications_people(tables, clauses):
-    """Perform eager loading of people and their validity for query.
-
-    :param query: a string query generated in the 'specifications'
-        method.
-    :return: A DecoratedResultSet with Person precaching setup.
-    """
-    if isinstance(clauses, basestring):
-        clauses = [SQL(clauses)]
-
-    def cache_people(rows):
-        """DecoratedResultSet pre_iter_hook to eager load Person
-         attributes.
-        """
-        from lp.registry.model.person import Person
-        # Find the people we need:
-        person_ids = set()
-        for spec in rows:
-            person_ids.add(spec._assigneeID)
-            person_ids.add(spec._approverID)
-            person_ids.add(spec._drafterID)
-        person_ids.discard(None)
-        if not person_ids:
-            return
-        # Query those people
-        origin = [Person]
-        columns = [Person]
-        validity_info = Person._validity_queries()
-        origin.extend(validity_info["joins"])
-        columns.extend(validity_info["tables"])
-        decorators = validity_info["decorators"]
-        personset = IStore(Specification).using(*origin).find(
-            tuple(columns),
-            Person.id.is_in(person_ids),
-            )
-        for row in personset:
-            person = row[0]
-            index = 1
-            for decorator in decorators:
-                column = row[index]
-                index += 1
-                decorator(person, column)
-
-    results = IStore(Specification).using(*tables).find(
-        Specification, *clauses)
-    return DecoratedResultSet(results, pre_iter_hook=cache_people)
+def _make_cache_user_can_view_spec(user):
+    userid = user.id
+
+    def cache_user_can_view_spec(spec):
+        get_property_cache(spec)._known_viewers = set([userid])
+        return spec
+    return cache_user_can_view_spec
+
+
+def _preload_specification_people():
+
+    def preload_specification_related_people(spec):
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            [spec._assigneeID, spec._approverID, spec._drafterID],
+            need_validity=True))
+        return spec
+    return preload_specification_related_people
 
 
 def get_specification_started_clause():

=== modified file 'lib/lp/blueprints/tests/test_specification.py'
--- lib/lp/blueprints/tests/test_specification.py	2013-01-25 03:30:08 +0000
+++ lib/lp/blueprints/tests/test_specification.py	2013-01-29 03:13:29 +0000
@@ -716,36 +716,33 @@
             information_type=InformationType.PROPRIETARY)
         self.assertNotIn(private_spec, list(context.specifications(None)))
 
+    def _assertInSpecifications(self, spec, grant):
+        self.assertIn(
+            spec,
+            list(getUtility(ISpecificationSet).specifications(grant.grantee)))
+        self.assertIn(
+            grant.grantee.id, removeSecurityProxy(spec)._known_viewers)
+       
     def test_proprietary_listed_for_artifact_grant(self):
         # Proprietary blueprints are listed for users with an artifact grant.
-        context = getUtility(ISpecificationSet)
-        blueprint1 = self.makeSpec(
-            information_type=InformationType.PROPRIETARY)
-        grant = self.factory.makeAccessArtifactGrant(
-            concrete_artifact=blueprint1)
-        self.assertIn(
-            blueprint1,
-            list(context.specifications(grant.grantee)))
+        spec = self.makeSpec(information_type=InformationType.PROPRIETARY)
+        grant = self.factory.makeAccessArtifactGrant(concrete_artifact=spec)
+        self._assertInSpecifications(spec, grant)
 
     def test_proprietary_listed_for_policy_grant(self):
         # Proprietary blueprints are listed for users with a policy grant.
-        context = getUtility(ISpecificationSet)
-        blueprint1 = self.makeSpec(
-            information_type=InformationType.PROPRIETARY)
-        policy_source = getUtility(IAccessPolicySource)
-        (policy,) = policy_source.find(
-            [(blueprint1.product, InformationType.PROPRIETARY)])
+        spec = self.makeSpec(information_type=InformationType.PROPRIETARY)
+        (policy,) = getUtility(IAccessPolicySource).find(
+            [(spec.product, InformationType.PROPRIETARY)])
         grant = self.factory.makeAccessPolicyGrant(policy)
-        self.assertIn(
-            blueprint1,
-            list(context.specifications(user=grant.grantee)))
+        self._assertInSpecifications(spec, grant)
 
     def run_test_setting_special_role_subscribes(self, role_name):
         # If a user becomes the assignee, drafter or approver of a
         # proprietary specification, they are automatically subscribed,
         # if they do not have yet been granted access to the specification.
         specification_sharing_policy = (
-                SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC)
+            SpecificationSharingPolicy.PROPRIETARY_OR_PUBLIC)
         product = self.factory.makeProduct(
             specification_sharing_policy=specification_sharing_policy)
         blueprint = self.makeSpec(