launchpad-reviewers team mailing list archive

[Ian Booth <ian.booth@xxxxxxxxxxxxx>]

Ian Booth has proposed merging lp:~wallyworld/launchpad/bug-privacy-filter-storm-expressions into lp:launchpad with lp:~wallyworld/launchpad/RemoveGranteeSubscriptionsJob-admins-1009283 as a prerequisite.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wallyworld/launchpad/bug-privacy-filter-storm-expressions/+merge/109990

== Implementation ==

This branch ports the code used to construct the bugtask search SQL from using strings to using Storm expressions. Existing callsites which used to need to wrap the resulting string in a SQL() expr have been updated. The one place in a security adaptor which used the string was also ported to Storm expressions.

This work is needed so that the bug subscription removal jobs can be implemented without duplicating code. This branch will not be landed separately - subsequent work will land the branch containing the jobs.

== Tests ==

Internal method change - run existing bug search tests.

== Lint ==

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/security.py
  lib/lp/bugs/model/bugbranch.py
  lib/lp/bugs/model/bugtask.py
  lib/lp/bugs/model/bugtasksearch.py
-- 
https://code.launchpad.net/~wallyworld/launchpad/bug-privacy-filter-storm-expressions/+merge/109990
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wallyworld/launchpad/bug-privacy-filter-storm-expressions into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugbranch.py'
--- lib/lp/bugs/model/bugbranch.py	2012-05-14 23:59:33 +0000
+++ lib/lp/bugs/model/bugbranch.py	2012-06-13 06:58:26 +0000
@@ -77,7 +77,7 @@
         if not branch_ids:
             return []
 
-        visible = get_bug_privacy_filter(user) or True
+        visible = get_bug_privacy_filter(user)
         return IStore(BugBranch).find(
             BugBranch.branchID,
             BugBranch.branch_id.is_in(branch_ids),

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-06-13 06:58:25 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-06-13 06:58:26 +0000
@@ -1794,8 +1794,7 @@
         # The janitor needs access to all bugs.
         if user != getUtility(ILaunchpadCelebrities).janitor:
             bug_privacy_filter = get_bug_privacy_filter(user)
-            if bug_privacy_filter != '':
-                conds.append(bug_privacy_filter)
+            conds.append(bug_privacy_filter)
 
         ids = origin.find(BugTaskFlat.bugtask_id, conds)
         ids = ids.order_by(BugTaskFlat.date_last_updated)
@@ -1893,7 +1892,7 @@
             BugTaskFlat.status.is_in(DB_UNRESOLVED_BUGTASK_STATUSES),
             BugTaskFlat.duplicateof == None,
             BugTaskFlat.product_id.is_in(product.id for product in products),
-            SQL(get_bug_privacy_filter(user) or True)
+            get_bug_privacy_filter(user),
             ).group_by(BugTaskFlat.product_id)
         # The result will return a list of product ids and counts,
         # which will be converted into key-value pairs in the dictionary.
@@ -1944,7 +1943,7 @@
             BugTaskFlat.sourcepackagename_id.is_in(package_name_ids),
             BugTaskFlat.distribution == distribution,
             BugTaskFlat.duplicateof == None,
-            SQL(get_bug_privacy_filter(user) or True)
+            get_bug_privacy_filter(user),
             ).group_by(
                 BugTaskFlat.distribution_id, BugTaskFlat.sourcepackagename_id)
 

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-05-24 22:25:01 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-06-13 06:58:26 +0000
@@ -14,6 +14,7 @@
 from storm.expr import (
     Alias,
     And,
+    Coalesce,
     Count,
     Desc,
     Exists,
@@ -67,17 +68,21 @@
 from lp.registry.interfaces.milestone import IProjectGroupMilestone
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
+from lp.registry.model.accesspolicy import AccessPolicyGrant
 from lp.registry.model.distribution import Distribution
 from lp.registry.model.milestone import Milestone
 from lp.registry.model.milestonetag import MilestoneTag
 from lp.registry.model.person import Person
 from lp.registry.model.product import Product
+from lp.registry.model.teammembership import TeamParticipation
 from lp.services.database.bulk import load
 from lp.services.database.decoratedresultset import DecoratedResultSet
 from lp.services.database.lpstorm import IStore
 from lp.services.database.sqlbase import sqlvalues
 from lp.services.database.stormexpr import (
     Array,
+    ArrayAgg,
+    ArrayIntersects,
     get_where_for_reference,
     NullCount,
     )
@@ -686,7 +691,7 @@
 
     clause, decorator = _get_bug_privacy_filter_with_decorator(params.user)
     if clause:
-        extra_clauses.append(SQL(clause))
+        extra_clauses.append(clause)
         decorators.append(decorator)
 
     hw_clause = _build_hardware_related_clause(params)
@@ -1373,30 +1378,47 @@
     :return: A SQL filter, a decorator to cache visibility in a resultset that
         returns BugTask objects.
     """
+    bug_filter_terms = _get_bug_privacy_filter_terms(user)
+    if not bug_filter_terms:
+        return True, _nocache_bug_decorator
+    if len(bug_filter_terms) == 1:
+        return bug_filter_terms[0], _nocache_bug_decorator
+
+    expr = Or(*bug_filter_terms)
+    return expr, _make_cache_user_can_view_bug(user)
+
+
+def _get_bug_privacy_filter_terms(user):
     public_bug_filter = (
-        'BugTaskFlat.information_type IN %s'
-        % sqlvalues(PUBLIC_INFORMATION_TYPES))
+        BugTaskFlat.information_type.is_in(PUBLIC_INFORMATION_TYPES))
 
     if user is None:
-        return public_bug_filter, _nocache_bug_decorator
+        return [public_bug_filter]
 
     admin_team = getUtility(ILaunchpadCelebrities).admin
-    if user.inTeam(admin_team):
-        return "", _nocache_bug_decorator
-
-    artifact_grant_query = ("""
-        BugTaskFlat.access_grants &&
-        (SELECT array_agg(team) FROM teamparticipation WHERE person = %d)
-        """ % user.id)
-    policy_grant_query = ("""
-        BugTaskFlat.access_policies &&
-        (SELECT array_agg(policy) FROM
-            accesspolicygrant
-            JOIN teamparticipation
-                ON teamparticipation.team = accesspolicygrant.grantee
-            WHERE person = %d)
-        """ % user.id)
-    query = "%s OR %s" % (artifact_grant_query, policy_grant_query)
-    return (
-        '(%s OR %s)' % (public_bug_filter, query),
-        _make_cache_user_can_view_bug(user))
+    if removeSecurityProxy(user).inTeam(admin_team):
+        return []
+
+    artifact_grant_query = Coalesce(
+            ArrayIntersects(SQL('BugTaskFlat.access_grants'),
+            Select(
+                ArrayAgg(TeamParticipation.teamID),
+                tables=TeamParticipation,
+                where=(TeamParticipation.personID ==
+                       user.id)
+            )), False)
+
+    policy_grant_query = Coalesce(
+            ArrayIntersects(SQL('BugTaskFlat.access_policies'),
+            Select(
+                ArrayAgg(AccessPolicyGrant.policy_id),
+                tables=(AccessPolicyGrant,
+                        Join(TeamParticipation,
+                            TeamParticipation.teamID ==
+                            AccessPolicyGrant.grantee_id)),
+                where=(
+                    TeamParticipation.personID ==
+                    user.id)
+            )), False)
+
+    return [public_bug_filter, artifact_grant_query, policy_grant_query]

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-05-21 17:29:19 +0000
+++ lib/lp/security.py	2012-06-13 06:58:26 +0000
@@ -17,6 +17,16 @@
     )
 from zope.interface import Interface
 
+from storm.expr import (
+    Exists,
+    And,
+    Or,
+    Select,
+    SQL,
+    Union,
+    With,
+    )
+
 from lp.answers.interfaces.faq import IFAQ
 from lp.answers.interfaces.faqtarget import IFAQTarget
 from lp.answers.interfaces.question import IQuestion
@@ -41,9 +51,14 @@
     )
 from lp.blueprints.interfaces.sprint import ISprint
 from lp.blueprints.interfaces.sprintspecification import ISprintSpecification
+from lp.blueprints.model.specificationsubscription import (
+    SpecificationSubscription,
+    )
 from lp.bugs.interfaces.bugtarget import IOfficialBugTagTargetRestricted
 from lp.bugs.interfaces.structuralsubscription import IStructuralSubscription
+from lp.bugs.model.bugsubscription import BugSubscription
 from lp.bugs.model.bugtasksearch import get_bug_privacy_filter
+from lp.bugs.model.bugtaskflat import BugTaskFlat
 from lp.buildmaster.interfaces.builder import (
     IBuilder,
     IBuilderSet,
@@ -159,9 +174,9 @@
     )
 from lp.registry.interfaces.wikiname import IWikiName
 from lp.registry.model.person import Person
+from lp.registry.model.teammembership import TeamParticipation
 from lp.services.config import config
 from lp.services.database.lpstorm import IStore
-from lp.services.database.sqlbase import quote
 from lp.services.identity.interfaces.account import IAccount
 from lp.services.identity.interfaces.emailaddress import IEmailAddress
 from lp.services.librarian.interfaces import ILibraryFileAliasWithParent
@@ -960,40 +975,34 @@
 
             store = IStore(Person)
             user_bugs_visible_filter = get_bug_privacy_filter(user.person)
-
-            # 1 = PUBLIC, 2 = UNEMBARGOEDSECURITY
-            query = """
-                SELECT TRUE WHERE
-                EXISTS (
-                    WITH teams AS (
-                        SELECT team from TeamParticipation
-                        WHERE person = %(personid)s
-                    )
-                    -- The team blueprint subscriptions
-                    SELECT 1
-                    FROM SpecificationSubscription
-                    WHERE SpecificationSubscription.person IN
-                        (SELECT team FROM teams)
-                    UNION ALL
-                    -- Find the bugs associated with the team and filter by
-                    -- those that are visible to the user.
-                    SELECT 1
-                    FROM BugTaskFlat
-                    WHERE
-                        %(user_bug_filter)s
-                        AND (
-                            bug IN (
-                                SELECT bug FROM bugsubscription
-                                WHERE person IN (SELECT team FROM teams))
-                            OR assignee IN (SELECT team FROM teams)
-                            )
+            teams_select = Select(SQL('team'), tables="teams")
+            blueprint_subscription_sql = Select(
+                1,
+                tables=SpecificationSubscription,
+                where=SpecificationSubscription.personID.is_in(teams_select))
+            visible_bug_sql = Select(
+                1,
+                tables=(BugTaskFlat,),
+                where=And(
+                    user_bugs_visible_filter,
+                    BugTaskFlat.bug_id.is_in(
+                        Select(
+                            BugSubscription.bug_id,
+                            where=Or(
+                                BugSubscription.person_id.is_in(teams_select),
+                                BugTaskFlat.assignee_id.is_in(teams_select)))))
                 )
-                """ % dict(
-                        personid=quote(self.obj.id),
-                        user_bug_filter=user_bugs_visible_filter)
+            bugs = Union(blueprint_subscription_sql, visible_bug_sql, all=True)
+            with_teams = With('teams',
+                    Select(
+                        TeamParticipation.teamID,
+                        where=TeamParticipation.personID == self.obj.id)),
 
-            rs = store.execute(query)
-            if rs.rowcount > 0:
+            rs = store.with_(with_teams).using(Person).find(
+                SQL("1"),
+                Exists(bugs)
+            )
+            if rs.any():
                 return True
         return False