launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging lp:~wgrant/launchpad/faster-checkCanBeDeleted into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1028256 in Launchpad itself: "BugTask.canBeDeleted issues a query for every call"
  https://bugs.launchpad.net/launchpad/+bug/1028256

For more details, see:
https://code.launchpad.net/~wgrant/launchpad/faster-checkCanBeDeleted/+merge/116404

This branch fixes bug #1028256 by making BugTask.canBeDeleted work off the bug's cached list of tasks, rather than calculating a fresh count. This fixes the leading cause of O(tasks) queries on BugTask:+index.
-- 
https://code.launchpad.net/~wgrant/launchpad/faster-checkCanBeDeleted/+merge/116404
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~wgrant/launchpad/faster-checkCanBeDeleted into lp:launchpad.

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-07-17 06:34:59 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-07-24 05:30:27 +0000
@@ -622,10 +622,9 @@
         return True
 
     def checkCanBeDeleted(self):
-        num_bugtasks = Store.of(self).find(
-            BugTask, bug=self.bug).count()
-
-        if num_bugtasks < 2:
+        # Bug.bugtasks is a cachedproperty, so this is pretty much free
+        # to call. Better than a manual count query, at any rate.
+        if len(self.bug.bugtasks) < 2:
             raise CannotDeleteBugtask(
                 "Cannot delete only bugtask affecting: %s."
                 % self.target.bugtargetdisplayname)

=== modified file 'lib/lp/bugs/model/tests/test_bugtask.py'
--- lib/lp/bugs/model/tests/test_bugtask.py	2012-07-24 04:04:07 +0000
+++ lib/lp/bugs/model/tests/test_bugtask.py	2012-07-24 05:30:27 +0000
@@ -13,6 +13,7 @@
 from lazr.lifecycle.snapshot import Snapshot
 from lazr.restfulclient.errors import Unauthorized
 from storm.store import Store
+from testtools.matchers import Equals
 from testtools.testcase import ExpectedException
 import transaction
 from zope.component import getUtility
@@ -97,6 +98,7 @@
     logout,
     person_logged_in,
     set_feature_flag,
+    StormStatementRecorder,
     TestCase,
     TestCaseWithFactory,
     ws_object,
@@ -108,6 +110,7 @@
     CeleryJobLayer,
     DatabaseFunctionalLayer,
     )
+from lp.testing.matchers import HasQueryCount
 
 
 BugData = namedtuple("BugData", ['owner', 'distro', 'distro_release',
@@ -1476,7 +1479,7 @@
         bug = self.factory.makeBug()
         login_celebrity('admin')
         self.assertTrue(
-            check_permission('launchpad.Admin', bug.default_bugtask))
+            check_permission('launchpad.Delete', bug.default_bugtask))
 
     def test_pillar_owner_can_delete(self):
         # With the feature flag on, the pillar owner can delete a bug task.
@@ -1508,6 +1511,18 @@
         login_person(bugtask.owner)
         self.assertRaises(CannotDeleteBugtask, bugtask.delete)
 
+    def test_canBeDeleted_is_free(self):
+        # BugTask.canBeDeleted uses cached data, so repeated execution
+        # on a single bug is free.
+        bug = self.factory.makeBug()
+        task1 = self.factory.makeBugTask(bug=bug)
+        task2 = self.factory.makeBugTask(bug=bug)
+        self.assertEqual(True, bug.default_bugtask.canBeDeleted())
+        with StormStatementRecorder() as recorder:
+            self.assertEqual(True, task1.canBeDeleted())
+            self.assertEqual(True, task2.canBeDeleted())
+        self.assertThat(recorder, HasQueryCount(Equals(0)))
+
     def test_delete_bugtask(self):
         # A bugtask can be deleted and after deletion, re-nominated.
         owner = self.factory.makePerson()