launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/more-private-elimination into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/more-private-elimination/+merge/105618

This branch removes the last few non-compat references to Bug.private and Bug.security_related in the DB. They were discovered by throwing a column drop at ec2test.

I rewrote the private team LimitedView adapter's bug subquery to be a bit less insane and substantially faster, and changed get_bug_privacy_filter to require non-deprecated flag values. If this passes ec2 I'll drop the flags entirely.

A few tests used Bug.private directly. I fixed them.
-- 
https://code.launchpad.net/~wgrant/launchpad/more-private-elimination/+merge/105618
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/more-private-elimination into lp:launchpad.

=== modified file 'lib/lp/bugs/doc/bugtask-search.txt'
--- lib/lp/bugs/doc/bugtask-search.txt	2012-04-27 03:34:48 +0000
+++ lib/lp/bugs/doc/bugtask-search.txt	2012-05-14 07:44:19 +0000
@@ -14,7 +14,7 @@
 
     >>> from lp.bugs.model.bugtask import BugTask
     >>> all_public_bugtasks = BugTask.select(
-    ...     "BugTask.bug = Bug.id AND Bug.private = false",
+    ...     "BugTask.bug = Bug.id AND Bug.information_type IN (1, 2)",
     ...     clauseTables=['Bug'])
     >>> found_bugtasks.count() == all_public_bugtasks.count()
     True

=== modified file 'lib/lp/bugs/model/bugtasksearch.py'
--- lib/lp/bugs/model/bugtasksearch.py	2012-05-10 01:58:10 +0000
+++ lib/lp/bugs/model/bugtasksearch.py	2012-05-14 07:44:19 +0000
@@ -1405,18 +1405,15 @@
     """Return a SQL filter to limit returned bug tasks.
 
     :param user: The user whose visible bugs will be filtered.
-    :param private_only: If a user is specified, this parameter determines
-        whether only private bugs will be filtered. If True, the returned
-        filter omits the "Bug.private IS FALSE" clause.
     :return: A SQL filter, a decorator to cache visibility in a resultset that
         returns BugTask objects.
     """
-    if use_flat:
-        public_bug_filter = (
-            'BugTaskFlat.information_type IN %s'
-            % sqlvalues(PUBLIC_INFORMATION_TYPES))
-    else:
-        public_bug_filter = 'Bug.private IS FALSE'
+    if private_only or not use_flat:
+        raise AssertionError("Only public+private flat mode is supported.")
+
+    public_bug_filter = (
+        'BugTaskFlat.information_type IN %s'
+        % sqlvalues(PUBLIC_INFORMATION_TYPES))
 
     if user is None:
         return public_bug_filter, _nocache_bug_decorator
@@ -1425,38 +1422,21 @@
     if user.inTeam(admin_team):
         return "", _nocache_bug_decorator
 
-    if use_flat:
-        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)
-    else:
-        # A subselect is used here because joining through
-        # TeamParticipation is only relevant to the "user-aware"
-        # part of the WHERE condition (i.e. the bit below.) The
-        # other half of this condition (see code above) does not
-        # use TeamParticipation at all.
-        query = ("""
-            EXISTS (
-                WITH teams AS (
-                    SELECT team from TeamParticipation
-                    WHERE person = %d
-                )
-                SELECT BugSubscription.bug
-                FROM BugSubscription
-                WHERE BugSubscription.person IN (SELECT team FROM teams) AND
-                    BugSubscription.bug = Bug.id
-                )
-            """ % user.id)
+    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)
     if not private_only:
         query = '%s OR %s' % (public_bug_filter, query)
-    return '(%s)' % query, _make_cache_user_can_view_bug(user)
+    return (
+        '(%s OR %s)' % (public_bug_filter, query),
+        _make_cache_user_can_view_bug(user))

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2012-05-03 01:14:29 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2012-05-14 07:44:19 +0000
@@ -928,25 +928,6 @@
         """Make an arbitrary bug target with no tasks on it."""
         return IBugTarget(self.factory.makeProduct())
 
-    def test_bug_privacy_filter_private_only_param_with_no_user(self):
-        # The bug privacy filter expression always has the "private is false"
-        # clause if the specified user is None, regardless of the value of the
-        # private_only parameter.
-        filter = get_bug_privacy_filter(None)
-        self.assertIn('Bug.private IS FALSE', filter)
-        filter = get_bug_privacy_filter(None, private_only=True)
-        self.assertIn('Bug.private IS FALSE', filter)
-
-    def test_bug_privacy_filter_private_only_param_with_user(self):
-        # The bug privacy filter expression omits has the "private is false"
-        # clause if the private_only parameter is True, provided a user is
-        # specified.
-        any_user = self.factory.makePerson()
-        filter = get_bug_privacy_filter(any_user)
-        self.assertIn('Bug.private IS FALSE', filter)
-        filter = get_bug_privacy_filter(any_user, private_only=True)
-        self.assertNotIn('Bug.private IS FALSE', filter)
-
     def test_no_tasks(self):
         # A brand new bug target has no tasks.
         target = self.makeBugTarget()

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2012-05-14 01:29:38 +0000
+++ lib/lp/registry/model/distribution.py	2012-05-14 07:44:19 +0000
@@ -96,7 +96,10 @@
 from lp.code.interfaces.seriessourcepackagebranch import (
     IFindOfficialBranchLinks,
     )
-from lp.registry.enums import InformationType
+from lp.registry.enums import (
+    InformationType,
+    PUBLIC_INFORMATION_TYPES,
+    )
 from lp.registry.errors import NoSuchDistroSeries
 from lp.registry.interfaces.accesspolicy import IAccessPolicySource
 from lp.registry.interfaces.distribution import (
@@ -1523,7 +1526,7 @@
                 AND Bugtask.sourcepackagename = spn.id
                 AND Bugtask.distroseries IS NULL
                 AND Bugtask.status IN %(unresolved)s
-                AND Bug.private = 'F'
+                AND Bug.information_type IN %(public_types)s
                 AND Bug.duplicateof IS NULL
                 AND spn.name NOT IN %(excluded_packages)s
             GROUP BY SPN.id, SPN.name
@@ -1535,6 +1538,7 @@
                'distro': self.id,
                'unresolved': quote(DB_UNRESOLVED_BUGTASK_STATUSES),
                'excluded_packages': quote(exclude_packages),
+               'public_types': quote(PUBLIC_INFORMATION_TYPES),
                 })
         counts = cur.fetchall()
         cur.close()

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-05-01 16:09:30 +0000
+++ lib/lp/security.py	2012-05-14 07:44:19 +0000
@@ -957,27 +957,11 @@
             # For efficiency, we do not want to perform several
             # TeamParticipation joins and we only want to do the user visible
             # bug filtering once. We use a With statement for the team
-            # participation check. For the bug query, we first filter on team
-            # association (subscribed to, assigned to etc) and then on user
-            # visibility.
+            # participation check.
 
             store = IStore(Person)
-            team_bugs_visible_select = """
-                SELECT bug_id FROM (
-                    -- The direct team bug subscriptions
-                    SELECT BugSubscription.bug as bug_id
-                    FROM BugSubscription
-                    WHERE BugSubscription.person IN
-                        (SELECT team FROM teams)
-                    UNION
-                    -- The bugs assigned to the team
-                    SELECT BugTask.bug as bug_id
-                    FROM BugTask
-                    WHERE BugTask.assignee IN (SELECT team FROM teams)
-                    ) as TeamBugs
-                """
             user_private_bugs_visible_filter = get_bug_privacy_filter(
-                user.person, private_only=True)
+                user.person, use_flat=True)
 
             # 1 = PUBLIC, 2 = UNEMBARGOEDSECURITY
             query = """
@@ -995,23 +979,19 @@
                     UNION ALL
                     -- Find the bugs associated with the team and filter by
                     -- those that are visible to the user.
-                    -- Public bugs are simple to check and always visible so
-                    -- do those first.
-                    %(team_bug_select)s
-                    WHERE bug_id in (
-                        SELECT Bug.id FROM Bug WHERE Bug.information_type IN
-                        (1, 2)
-                    )
-                    UNION ALL
-                    -- Now do the private bugs the user can see.
-                    %(team_bug_select)s
-                    WHERE bug_id in (
-                        SELECT Bug.id FROM Bug WHERE %(user_bug_filter)s
-                    )
+                    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)
+                            )
                 )
                 """ % dict(
                         personid=quote(self.obj.id),
-                        team_bug_select=team_bugs_visible_select,
                         user_bug_filter=user_private_bugs_visible_filter)
 
             rs = store.execute(query)

=== modified file 'lib/lp/services/feeds/stories/xx-security.txt'
--- lib/lp/services/feeds/stories/xx-security.txt	2012-04-26 06:18:48 +0000
+++ lib/lp/services/feeds/stories/xx-security.txt	2012-05-14 07:44:19 +0000
@@ -10,8 +10,7 @@
     >>> import transaction
     >>> from lp.bugs.model.bug import Bug
     >>> from lp.registry.enums import InformationType
-    >>> IStore(Bug).find(Bug).set(
-    ...     information_type=InformationType.USERDATA, _private=True)
+    >>> IStore(Bug).find(Bug).set(information_type=InformationType.USERDATA)
     >>> transaction.commit()
 
 There should be zero entries in these feeds, since all the bugs are private.