launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

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

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #869631 in Launchpad itself: "BugTask:+editstatus timeout closing bug 353126 due to bug subscription lookups"
  https://bugs.launchpad.net/launchpad/+bug/869631

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

This branch fixes a bug subscription query to be correct and a thousand times faster. Bug.duplicate_subscriptions has a BugMute subquery to attempt to filter out muted subscriptions, but the query ends up uncorrelated, so a single subscription to any bug erroneously causes any of the user's dupe subscriptions to be muted. This also causes seq scans over a few tables, making things terribly slow.

Adding tables=[BugMute] changes it to the intended correlated subquery -- fast and correct. I added a test for this behaviour, and changed an existing test which relied on the broken one.
-- 
https://code.launchpad.net/~wgrant/launchpad/bug-869631/+merge/96280
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/bug-869631 into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-02-20 22:21:36 +0000
+++ lib/lp/bugs/model/bug.py	2012-03-07 02:08:26 +0000
@@ -2613,8 +2613,11 @@
                 BugSubscription.bug_notification_level >= self.level,
                 BugSubscription.bug_id == Bug.id,
                 Bug.duplicateof == self.bug,
-                Not(In(BugSubscription.person_id,
-                       Select(BugMute.person_id, BugMute.bug_id == Bug.id))))
+                Not(In(
+                    BugSubscription.person_id,
+                    Select(
+                        BugMute.person_id, BugMute.bug_id == Bug.id,
+                        tables=[BugMute]))))
 
     @property
     def duplicate_subscribers(self):

=== modified file 'lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py'
--- lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-01-04 17:21:01 +0000
+++ lib/lp/bugs/model/tests/test_bugsubscriptioninfo.py	2012-03-07 02:08:26 +0000
@@ -258,10 +258,21 @@
         duplicate_bug, duplicate_bug_subscription = (
             self._create_duplicate_subscription())
         with person_logged_in(duplicate_bug.owner):
-            self.bug.mute(duplicate_bug.owner, duplicate_bug.owner)
+            duplicate_bug.mute(duplicate_bug.owner, duplicate_bug.owner)
         found_subscriptions = self.getInfo().duplicate_subscriptions
         self.assertContentEqual([], found_subscriptions)
 
+    def test_duplicate_other_mute(self):
+        # If some other bug is muted, the dupe is still listed.
+        duplicate_bug, duplicate_bug_subscription = (
+            self._create_duplicate_subscription())
+        with person_logged_in(duplicate_bug.owner):
+            self.factory.makeBug().mute(
+                duplicate_bug.owner, duplicate_bug.owner)
+        found_subscriptions = self.getInfo().duplicate_subscriptions
+        self.assertContentEqual(
+            [duplicate_bug_subscription], found_subscriptions)
+
     def test_duplicate_for_private_bug(self):
         # The set of subscribers from duplicate bugs is always empty when the
         # master bug is private.