Launchpad itself

Merge lp:~edwin-grubbs/launchpad/bug-682727-bug-count-timeout into lp:launchpad

  1. bug-682727-bug-count-timeout
  2. Merge into devel
Proposed by Edwin Grubbs
Status: Merged
Approved by: Graham Binns
Approved revision: no longer in the source branch.
Merged at revision: 12158
Proposed branch: lp:~edwin-grubbs/launchpad/bug-682727-bug-count-timeout
Merge into: lp:launchpad
Diff against target: 200 lines (+125/-9)
3 files modified
lib/lp/bugs/model/bugtask.py (+7/-3)
lib/lp/bugs/tests/test_bugtaskset.py (+91/-0)
lib/lp/testing/factory.py (+27/-6)
To merge this branch: bzr merge lp:~edwin-grubbs/launchpad/bug-682727-bug-count-timeout
Reviewer Review Type Date Requested Status
Graham Binns (community) code Approve
Review via email: mp+45141@code.launchpad.net

Commit message

[r=gmb][ui=none][bug=682727] Fix timeout on $project/+series page by simplifying bug count query.

Description of the change

Summary
-------

This branch fixes a timeout on the $project/+series page.

I simplified the permission query for bug counts on milestones, since
the query which determines which private bugs a user can view is costly,
and displaying a count including all private bugs doesn't reveal any
sensitive information.

Implementation details
----------------------

configs/testrunner/launchpad.conf
lib/lp/bugs/model/bugtask.py
lib/lp/bugs/tests/test_bugtaskset.py
lib/lp/testing/factory.py

Tests
-----

./bin/test -vvp lp.bugs.tests.test_bugtaskset

Demo and Q/A
------------

* Open https://code.qastaging.launchpad.net/obsolete-junk/+series
  * It should not timeout.

Lint
----

No lint.

To post a comment you must log in.
Revision history for this message
Graham Binns (gmb) wrote :

Hi Edwin,

Couple of minor nitpicks here, but otherwise this is r=me.

60 + def get_counts(self, user):
61 + counts = self.bugtask_set.getStatusCountsForProductSeries(
62 + user, self.series)
63 + return [(BugTaskStatus.items[status_id], count)
64 + for status_id, count in counts]

The indentation for these statements is a bit weird. I think that it
should look like this:

    counts = self.bugtask_set.getStatusCountsForProductSeries(
        user, self.series)
    return [
        (BugTaskStatus.items[status_id], count)
        for status_id, count in counts]

186 + [bugtask] = bug.bugtasks

I know that the context makes it unlikely that we'll have > 1 bugtask on
the bug at this point, but for safety's (and readability's) sake I'd
prefer this to be:

    bugtask = bug.default_bugtask

review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py 2010-12-23 11:35:12 +0000
+++ lib/lp/bugs/model/bugtask.py 2011-01-04 17:02:31 +0000
@@ -2485,9 +2485,13 @@
24852485
2486 def getStatusCountsForProductSeries(self, user, product_series):2486 def getStatusCountsForProductSeries(self, user, product_series):
2487 """See `IBugTaskSet`."""2487 """See `IBugTaskSet`."""
2488 bug_privacy_filter = get_bug_privacy_filter(user)2488 if user is None:
2489 if bug_privacy_filter != "":2489 bug_privacy_filter = 'AND Bug.private = FALSE'
2490 bug_privacy_filter = 'AND ' + bug_privacy_filter2490 else:
2491 # Since the count won't reveal sensitive information, and
2492 # since the get_bug_privacy_filter() check for non-admins is
2493 # costly, don't filter those bugs at all.
2494 bug_privacy_filter = ''
2491 cur = cursor()2495 cur = cursor()
24922496
2493 # The union is actually much faster than a LEFT JOIN with the2497 # The union is actually much faster than a LEFT JOIN with the
24942498
=== added file 'lib/lp/bugs/tests/test_bugtaskset.py'
--- lib/lp/bugs/tests/test_bugtaskset.py 1970-01-01 00:00:00 +0000
+++ lib/lp/bugs/tests/test_bugtaskset.py 2011-01-04 17:02:31 +0000
@@ -0,0 +1,91 @@
1# Copyright 2011 Canonical Ltd. This software is licensed under the
2# GNU Affero General Public License version 3 (see the file LICENSE).
3
4"""Tests for BugTaskSet."""
5
6__metaclass__ = type
7
8from zope.component import getUtility
9
10from canonical.testing.layers import DatabaseFunctionalLayer
11from lp.bugs.interfaces.bugtask import (
12 BugTaskStatus,
13 IBugTaskSet,
14 )
15from lp.testing import (
16 login_person,
17 TestCaseWithFactory,
18 )
19
20
21class TestStatusCountsForProductSeries(TestCaseWithFactory):
22 """Test BugTaskSet.getStatusCountsForProductSeries()."""
23
24 layer = DatabaseFunctionalLayer
25
26 def setUp(self):
27 super(TestStatusCountsForProductSeries, self).setUp()
28 self.bugtask_set = getUtility(IBugTaskSet)
29 self.owner = self.factory.makePerson()
30 login_person(self.owner)
31 self.product = self.factory.makeProduct(owner=self.owner)
32 self.series = self.factory.makeProductSeries(product=self.product)
33 self.milestone = self.factory.makeMilestone(productseries=self.series)
34
35 def get_counts(self, user):
36 counts = self.bugtask_set.getStatusCountsForProductSeries(
37 user, self.series)
38 return [
39 (BugTaskStatus.items[status_id], count)
40 for status_id, count in counts]
41
42 def test_privacy_and_counts_for_unauthenticated_user(self):
43 # An unauthenticated user should see bug counts for each status
44 # that do not include private bugs.
45 self.factory.makeBug(milestone=self.milestone)
46 self.factory.makeBug(milestone=self.milestone, private=True)
47 self.factory.makeBug(series=self.series)
48 self.factory.makeBug(series=self.series, private=True)
49 self.assertEqual(
50 [(BugTaskStatus.NEW, 2)],
51 self.get_counts(None))
52
53 def test_privacy_and_counts_for_owner(self):
54 # The owner should see bug counts for each status that do
55 # include all private bugs.
56 self.factory.makeBug(milestone=self.milestone)
57 self.factory.makeBug(milestone=self.milestone, private=True)
58 self.factory.makeBug(series=self.series)
59 self.factory.makeBug(series=self.series, private=True)
60 self.assertEqual(
61 [(BugTaskStatus.NEW, 4)],
62 self.get_counts(self.owner))
63
64 def test_privacy_and_counts_for_other_user(self):
65 # A random authenticated user should see bug counts for each
66 # status that do include all private bugs, since it is costly to
67 # query just the private bugs that the user has access to view,
68 # and this query may be run many times on a single page.
69 self.factory.makeBug(milestone=self.milestone)
70 self.factory.makeBug(milestone=self.milestone, private=True)
71 self.factory.makeBug(series=self.series)
72 self.factory.makeBug(series=self.series, private=True)
73 other = self.factory.makePerson()
74 self.assertEqual(
75 [(BugTaskStatus.NEW, 4)],
76 self.get_counts(other))
77
78 def test_multiple_statuses(self):
79 # Test that separate counts are provided for each status that
80 # bugs are found in.
81 for status in (BugTaskStatus.INVALID, BugTaskStatus.OPINION):
82 self.factory.makeBug(milestone=self.milestone, status=status)
83 self.factory.makeBug(series=self.series, status=status)
84 for i in range(3):
85 self.factory.makeBug(series=self.series)
86 self.assertEqual(
87 [(BugTaskStatus.INVALID, 2),
88 (BugTaskStatus.OPINION, 2),
89 (BugTaskStatus.NEW, 3),
90 ],
91 self.get_counts(None))
092
=== modified file 'lib/lp/testing/factory.py'
--- lib/lp/testing/factory.py 2010-12-24 10:03:15 +0000
+++ lib/lp/testing/factory.py 2011-01-04 17:02:31 +0000
@@ -7,7 +7,6 @@
77
8This module should not contain tests (but it should be tested).8This module should not contain tests (but it should be tested).
9"""9"""
10from lp.code.model.recipebuild import RecipeBuildRecord
1110
12__metaclass__ = type11__metaclass__ = type
13__all__ = [12__all__ = [
@@ -161,6 +160,7 @@
161 PreviewDiff,160 PreviewDiff,
162 StaticDiff,161 StaticDiff,
163 )162 )
163from lp.code.model.recipebuild import RecipeBuildRecord
164from lp.codehosting.codeimport.worker import CodeImportSourceDetails164from lp.codehosting.codeimport.worker import CodeImportSourceDetails
165from lp.hardwaredb.interfaces.hwdb import (165from lp.hardwaredb.interfaces.hwdb import (
166 HWSubmissionFormat,166 HWSubmissionFormat,
@@ -1371,26 +1371,41 @@
1371 def makeBug(self, product=None, owner=None, bug_watch_url=None,1371 def makeBug(self, product=None, owner=None, bug_watch_url=None,
1372 private=False, date_closed=None, title=None,1372 private=False, date_closed=None, title=None,
1373 date_created=None, description=None, comment=None,1373 date_created=None, description=None, comment=None,
1374 status=None, distribution=None):1374 status=None, distribution=None, milestone=None, series=None):
1375 """Create and return a new, arbitrary Bug.1375 """Create and return a new, arbitrary Bug.
13761376
1377 The bug returned uses default values where possible. See1377 The bug returned uses default values where possible. See
1378 `IBugSet.new` for more information.1378 `IBugSet.new` for more information.
13791379
1380 :param product: If the product is not set, and if the parameter1380 :param product: If the product is not set, and if the parameter
1381 distribution is not set, a product is created and this is1381 distribution, milestone, and series are not set, a product
1382 used as the primary bug target.1382 is created and this is used as the primary bug target.
1383 :param owner: The reporter of the bug. If not set, one is created.1383 :param owner: The reporter of the bug. If not set, one is created.
1384 :param bug_watch_url: If specified, create a bug watch pointing1384 :param bug_watch_url: If specified, create a bug watch pointing
1385 to this URL.1385 to this URL.
1386 :param distribution: If set, the distribution is used as the1386 :param distribution: If set, the distribution is used as the
1387 default bug target.1387 default bug target.
1388 :param milestone: If set, the milestone.target must match the product
1389 or distribution parameters, or the those parameters must be None.
1390 :param series: If set, the series.product must match the product
1391 parameter, or the series.distribution must match the distribution
1392 parameter, or the those parameters must be None.
13881393
1389 At least one of the parameters distribution and product must be1394 At least one of the parameters distribution and product must be
1390 None, otherwise, an assertion error will be raised.1395 None, otherwise, an assertion error will be raised.
1391 """1396 """
1392 if product is None and distribution is None:1397 if product is None and distribution is None:
1393 product = self.makeProduct()1398 if milestone is not None:
1399 # One of these will be None.
1400 product = milestone.product
1401 distribution = milestone.distribution
1402 elif series is not None:
1403 if IProductSeries.providedBy(series):
1404 product = series.product
1405 else:
1406 distribution = series.distribution
1407 else:
1408 product = self.makeProduct()
1394 if owner is None:1409 if owner is None:
1395 owner = self.makePerson()1410 owner = self.makePerson()
1396 if title is None:1411 if title is None:
@@ -1407,10 +1422,16 @@
1407 if bug_watch_url is not None:1422 if bug_watch_url is not None:
1408 # fromText() creates a bug watch associated with the bug.1423 # fromText() creates a bug watch associated with the bug.
1409 getUtility(IBugWatchSet).fromText(bug_watch_url, bug, owner)1424 getUtility(IBugWatchSet).fromText(bug_watch_url, bug, owner)
1425 bugtask = bug.default_bugtask
1410 if date_closed is not None:1426 if date_closed is not None:
1411 [bugtask] = bug.bugtasks
1412 bugtask.transitionToStatus(1427 bugtask.transitionToStatus(
1413 BugTaskStatus.FIXRELEASED, owner, when=date_closed)1428 BugTaskStatus.FIXRELEASED, owner, when=date_closed)
1429 if milestone is not None:
1430 bugtask.transitionToMilestone(milestone, milestone.target.owner)
1431 else:
1432 task = bug.addTask(owner, series)
1433 task.transitionToStatus(status, owner)
1434
1414 return bug1435 return bug
14151436
1416 def makeBugTask(self, bug=None, target=None, owner=None):1437 def makeBugTask(self, bug=None, target=None, owner=None):