launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/bug-1051002 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1051002 in Launchpad itself: "timeout iterating over a user's super_teams"
  https://bugs.launchpad.net/launchpad/+bug/1051002

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/bug-1051002/+merge/124849

This branch fixes the slow bug+spec query in IPerson's launchpad.LimitedView security adapter. The old query was fairly large and encompassed both bugs and specs, and took 1-2 seconds to run. It's replaced by two new queries, each taking a couple of milliseconds in the normal case. The key to it is that PostgreSQL apparently won't optimise "baz IN (foo) OR baz IN (bar)" down to "baz IN (foo UNION ALL bar)"; http://paste.ubuntu.com/1212288/ has representative parts of the new and old queries.

I also removed the super-teams bit of those queries, as it doesn't make sense for them. If private team sub is a member of private team super, and super is subscribed to a public bug, I should get to see that super exists, but not get to see its member list -- there's no reason I need to see sub.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-1051002/+merge/124849
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-1051002 into lp:launchpad.

=== modified file 'lib/lp/security.py'
--- lib/lp/security.py	2012-09-07 20:33:10 +0000
+++ lib/lp/security.py	2012-09-18 07:23:21 +0000
@@ -996,49 +996,28 @@
             if len(intersection_teams) > 0:
                 return True
 
-            # There are a number of other conditions under which a private
-            # team may be visible. These are:
-            #  - All blueprints are public, so if the team is subscribed to
-            #    any blueprints, they are in a public role and hence visible.
-            #  - If the team is directly subscribed or assigned to any bugs
-            #    the user can see, the team should be visible.
-            #
-            # 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.
-
-            store = IStore(Person)
-            user_bugs_visible_filter = get_bug_privacy_filter(user.person)
-            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,
-                    Or(
-                        BugTaskFlat.bug_id.is_in(
-                            Select(
-                                BugSubscription.bug_id,
-                                tables=(BugSubscription,),
-                                where=BugSubscription.person_id.is_in(
-                                    teams_select))),
-                        BugTaskFlat.assignee_id.is_in(teams_select))))
-            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.with_(with_teams).using(Person).find(
-                SQL("1"),
-                Exists(bugs)
-            )
-            if rs.any():
+            # Teams subscribed to blueprints are visible. This needs to
+            # be taught about privacy eventually.
+            specsubs = store.find(SpecificationSubscription, person=self.obj)
+
+            # Teams subscribed or assigned to bugs that the user can see
+            # are visible.
+            bugs = store.find(
+                BugTaskFlat,
+                get_bug_privacy_filter(user.person),
+                BugTaskFlat.bug_id.is_in(
+                    Union(
+                        Select(
+                            BugSubscription.bug_id,
+                            tables=(BugSubscription,),
+                            where=BugSubscription.person == self.obj),
+                        Select(
+                            BugTaskFlat.bug_id,
+                            tables=(BugTaskFlat,),
+                            where=BugTaskFlat.assignee == self.obj),
+                        all=True)))
+
+            if not specsubs.is_empty() or not bugs.is_empty():
                 return True
         return False