launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/private-structsubs-redux-redux into lp:launchpad with lp:~wgrant/launchpad/private-structsubs-redux-redux-resurrection as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/private-structsubs-redux-redux/+merge/118509

This branch resurrects StevenK's private structural subscription work, and rewrites the notification queries to be a little more performant. The old version timed out terribly on private bugs on production, while the new one runs snappily even on dogfood.

The core of the issue was that all the other in-app bug access checks dealt with one user and lots of bugs, so get_bug_privacy_filter_terms returned a query fragment optimised for the single-user case. I've adjusted the notification queries to use a new query fragment, which I've designed to sort of work in the opposite direction. It causes these multi-user single-bug queries to execute in 2ms rather than 500-1500ms, by calculating the set of viewers once per bug with an efficient join, not once per subscriber with a slow hash join.

The old function, get_bug_privacy_filter_terms, evolved over several years and doesn't have any really direct tests. So I added a new test base class to run over both implementations.

The resurrection of the reviewed and reverted code is large, so I've set a branch containing just that as a prereq. This diff is just the performance-related fixes.
-- 
https://code.launchpad.net/~wgrant/launchpad/private-structsubs-redux-redux/+merge/118509
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/private-structsubs-redux-redux into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-08-07 08:35:45 +0000
+++ lib/lp/bugs/model/bug.py	2012-08-07 08:35:46 +0000
@@ -2431,12 +2431,12 @@
 
     def visible_recipients_filter(self, column):
         # Circular fail :(
-        from lp.bugs.model.bugtaskflat import BugTaskFlat
-        from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
+        from lp.bugs.model.bugtasksearch import (
+            get_bug_bulk_privacy_filter_terms,
+            )
 
         if self.bug.private:
-            return [Or(*get_bug_privacy_filter_terms(column)),
-                BugTaskFlat.bug_id == self.bug.id]
+            return get_bug_bulk_privacy_filter_terms(column, self.bug.id)
         else:
             return True
 

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-08-07 02:31:56 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-08-07 08:35:46 +0000
@@ -6,6 +6,7 @@
 __all__ = [
     'get_bug_privacy_filter',
     'get_bug_privacy_filter_terms',
+    'get_bug_bulk_privacy_filter_terms',
     'orderby_expression',
     'search_bugs',
     ]
@@ -27,6 +28,7 @@
     Row,
     Select,
     SQL,
+    Union,
     )
 from storm.info import ClassAlias
 from storm.references import Reference
@@ -87,6 +89,7 @@
     ArrayIntersects,
     get_where_for_reference,
     NullCount,
+    Unnest
     )
 from lp.services.propertycache import get_property_cache
 from lp.services.searchbuilder import (
@@ -1412,6 +1415,23 @@
 
 
 def get_bug_privacy_filter_terms(user, check_admin=True):
+    """Return Storm terms for filtering bugs by visibility.
+
+    The same rules as get_bug_bulk_privacy_filter_terms, except designed
+    and optimised for cases like bug searches, where we have a small
+    number of users and a lot of bugs.
+
+    Also unlike get_bug_bulk_privacy_filter_terms, this constrains
+    BugTaskFlat to user, rather than user to a bug column. It's up to
+    callsites to work with BugTaskFlat.
+
+    :param user: a Person ID value or column reference.
+    :param check_admin: add an admin role check. This is probably only
+        necessary when checking multiple users; if you're only checking a
+        specific one, you can do the admin check beforehand.
+    :return: a Storm expression relating person to bug, where bug is visible
+        to person.
+    """
     public_bug_filter = (
         BugTaskFlat.information_type.is_in(PUBLIC_INFORMATION_TYPES))
 
@@ -1449,3 +1469,51 @@
                         getUtility(ILaunchpadCelebrities).admin))))
 
     return filters
+
+
+def get_bug_bulk_privacy_filter_terms(person, bug):
+    """Return Storm terms for filtering people by bug visibility.
+
+    The same rules as get_bug_privacy_filter_terms, except that it's
+    designed and optimised for cases like bug notifications, where we
+    have a small number of bug and need to check which people can see
+    them.
+
+    :param person: a Person ID value or column reference.
+    :param bug: a Bug ID value or column reference.
+    :return: a Storm expression relating person to bug, where bug is visible
+        to person.
+    """
+    # This whole query is a bit ugly what with the repeated BugTaskFlat
+    # SELECTs and all that, but it's an order of magnitude faster than
+    # joining at a higher level, and a thousand times faster than
+    # get_bug_privacy_filter_terms for some cases. Test carefully
+    # (particularly with the structural subscription queries) before
+    # touching.
+
+    select_btf = (
+        lambda select, *conds: Select(
+            select, tables=[BugTaskFlat],
+            where=And(BugTaskFlat.bug == bug, *conds)))
+    # Admins, artifact grantees, and policy grantees can all see private
+    # bugs.
+    teams = Union(
+        Select(getUtility(ILaunchpadCelebrities).admin.id),
+        select_btf(Unnest(BugTaskFlat.access_grants)),
+        Select(
+            AccessPolicyGrant.grantee_id,
+            tables=[AccessPolicyGrant],
+            where=In(
+                AccessPolicyGrant.policy_id,
+                select_btf(
+                    Unnest(BugTaskFlat.access_policies)))))
+    # And we need to expand team memberships.
+    participants = Select(
+        TeamParticipation.personID,
+        tables=[TeamParticipation],
+        where=In(TeamParticipation.teamID, teams))
+    # The bug must public, or the user must satisfy the above criteria.
+    return Or(
+        Exists(select_btf(
+            1, BugTaskFlat.information_type.is_in(PUBLIC_INFORMATION_TYPES))),
+        In(person, participants))

=== modified file 'lib/lp/bugs/model/structuralsubscription.py'
--- lib/lp/bugs/model/structuralsubscription.py	2012-08-07 08:35:45 +0000
+++ lib/lp/bugs/model/structuralsubscription.py	2012-08-07 08:35:46 +0000
@@ -702,8 +702,7 @@
                                directly subscribed to the bug.
     """
     # Circular. :-(
-    from lp.bugs.model.bugtaskflat import BugTaskFlat
-    from lp.bugs.model.bugtasksearch import get_bug_privacy_filter_terms
+    from lp.bugs.model.bugtasksearch import get_bug_bulk_privacy_filter_terms
     # We get the ids because we need to use group by in order to
     # look at the filters' tags in aggregate.  Once we have the ids,
     # we can get the full set of what we need in subsuming or
@@ -741,10 +740,9 @@
                    Select(BugSubscription.person_id,
                           BugSubscription.bug == bug))))
     if bug.private:
-        filters.extend([
-            Or(*get_bug_privacy_filter_terms(
-                StructuralSubscription.subscriberID)),
-            BugTaskFlat.bug == bug])
+        filters.append(
+            get_bug_bulk_privacy_filter_terms(
+                StructuralSubscription.subscriberID, bug))
     candidates = list(_get_structural_subscriptions(
         StructuralSubscription.id, query_arguments, *filters))
     if not candidates:
@@ -763,7 +761,7 @@
     conditions.append(_calculate_bugtask_condition(query_arguments))
     # Handle filtering by information type.
     conditions.append(Or(
-        BugSubscriptionFilterInformationType.information_type == 
+        BugSubscriptionFilterInformationType.information_type ==
             bug.information_type,
         BugSubscriptionFilterInformationType.information_type == None))
     # Now we handle tags.  This actually assembles the query, because it

=== modified file 'lib/lp/bugs/model/tests/test_bugtasksearch.py'
--- lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-08-07 02:31:56 +0000
+++ lib/lp/bugs/model/tests/test_bugtasksearch.py	2012-08-07 08:35:46 +0000
@@ -7,14 +7,18 @@
     datetime,
     timedelta,
     )
+from operator import attrgetter
 import unittest
 
 import pytz
+from storm.expr import Or
 from testtools.matchers import Equals
 from testtools.testcase import ExpectedException
 from zope.component import getUtility
 from zope.security.proxy import removeSecurityProxy
 
+from lp.app.interfaces.launchpad import ILaunchpadCelebrities
+from lp.app.interfaces.services import IService
 from lp.bugs.interfaces.bugattachment import BugAttachmentType
 from lp.bugs.interfaces.bugtarget import IBugTarget
 from lp.bugs.interfaces.bugtask import (
@@ -24,19 +28,29 @@
     BugTaskStatus,
     IBugTaskSet,
     )
+<<<<<<< TREE
 from lp.bugs.interfaces.bugtasksearch import BugTaskSearchParams
+=======
+from lp.bugs.model.bug import Bug
+>>>>>>> MERGE-SOURCE
 from lp.bugs.model.bugsummary import BugSummary
 from lp.bugs.model.bugtask import BugTask
+from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.bugs.model.bugtasksearch import (
     _build_status_clause,
     _build_tag_search_clause,
     _process_order_by,
+    get_bug_bulk_privacy_filter_terms,
+    get_bug_privacy_filter_terms,
     )
 from lp.hardwaredb.interfaces.hwdb import (
     HWBus,
     IHWDeviceSet,
     )
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    SharingPermission,
+    )
 from lp.registry.interfaces.distribution import (
     IDistribution,
     IDistributionSet,
@@ -51,7 +65,10 @@
     )
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.sourcepackage import ISourcePackage
+from lp.registry.model.person import Person
+from lp.services.database.lpstorm import IStore
 from lp.services.database.sqlbase import convert_storm_clause_to_string
+from lp.services.features.testing import FeatureFixture
 from lp.services.searchbuilder import (
     all,
     any,
@@ -62,6 +79,7 @@
 from lp.soyuz.interfaces.component import IComponentSet
 from lp.soyuz.interfaces.publishing import PackagePublishingStatus
 from lp.testing import (
+    admin_logged_in,
     login_person,
     logout,
     normalize_whitespace,
@@ -2349,6 +2367,97 @@
         self.assertContentEqual(bug1.bugtasks, tasks)
 
 
+class BaseGetBugPrivacyFilterTermsTests:
+
+    layer = DatabaseFunctionalLayer
+
+    def test_public(self):
+        bug = self.factory.makeBug()
+        people = [bug.owner, self.factory.makePerson()]
+        self.assertContentEqual(people, self.getVisiblePeople(bug, people))
+
+    def makePrivacyScenario(self):
+        self.owner = self.factory.makePerson()
+        login_person(self.owner)
+        self.bug = self.factory.makeBug(
+            owner=self.owner, information_type=InformationType.USERDATA)
+        self.grantee_member = self.factory.makePerson()
+        self.grantee_team = self.factory.makeTeam(
+            members=[self.grantee_member])
+        self.grantee_person = self.factory.makePerson()
+        self.other_person = self.factory.makePerson()
+
+        self.people = [
+            self.owner, self.grantee_team, self.grantee_member,
+            self.grantee_person, self.other_person]
+        self.expected_people = [
+            self.owner, self.grantee_team, self.grantee_member,
+            self.grantee_person]
+
+    def assertPrivacyRespected(self):
+        self.assertContentEqual(
+            [], self.getVisiblePeople(self.bug, [self.other_person]))
+        self.assertContentEqual(
+            self.expected_people, self.getVisiblePeople(self.bug, self.people))
+
+    def test_artifact_grant(self):
+        # People and teams with AccessArtifactGrants can see the bug.
+        self.makePrivacyScenario()
+
+        getUtility(IService, 'sharing').ensureAccessGrants(
+            [self.grantee_team, self.grantee_person], self.owner,
+            bugs=[self.bug], ignore_permissions=True)
+
+        self.assertPrivacyRespected()
+
+    def test_policy_grant(self):
+        # People and teams with AccessPolicyGrants can see the bug.
+        self.makePrivacyScenario()
+
+        self.useFixture(FeatureFixture(
+            {'disclosure.enhanced_sharing.writable': 'true'}))
+        with admin_logged_in():
+            for princ in (self.grantee_team, self.grantee_person):
+                getUtility(IService, 'sharing').sharePillarInformation(
+                    self.bug.default_bugtask.target, princ, self.owner,
+                    {InformationType.USERDATA: SharingPermission.ALL})
+
+        self.assertPrivacyRespected()
+
+    def test_admin(self):
+        # People and teams in the admin team can see the bug.
+        self.makePrivacyScenario()
+
+        admins = getUtility(ILaunchpadCelebrities).admin
+        with admin_logged_in():
+            for princ in (self.grantee_team, self.grantee_person):
+                admins.addMember(princ, admins)
+            self.grantee_team.acceptInvitationToBeMemberOf(admins, None)
+
+        self.assertPrivacyRespected()
+
+
+class TestGetBugPrivacyFilterTerms(BaseGetBugPrivacyFilterTermsTests,
+                                   TestCaseWithFactory):
+
+    def getVisiblePeople(self, bug, people):
+        return IStore(Bug).find(
+            Person,
+            Person.id.is_in(map(attrgetter('id'), people)),
+            BugTaskFlat.bug_id == bug.id,
+            Or(*get_bug_privacy_filter_terms(Person.id)))
+
+
+class TestGetBugBulkPrivacyFilterTerms(BaseGetBugPrivacyFilterTermsTests,
+                                       TestCaseWithFactory):
+
+    def getVisiblePeople(self, bug, people):
+        return IStore(Bug).find(
+            Person,
+            Person.id.is_in(map(attrgetter('id'), people)),
+            get_bug_bulk_privacy_filter_terms(Person.id, bug.id))
+
+
 def test_suite():
     suite = unittest.TestSuite()
     loader = unittest.TestLoader()