Merge lp:~lifeless/launchpad/bug-724033 into lp:launchpad

[Robert Collins]

[r=thumper,wgrant][ui=none][bug=724033]

Reduce more scaling issues with BugTask:+index. No tests for this because we're still batshit insanely far from having a flat profile (which is needed to write a clean test. Tests will come when we're flat, which will be soon.

[William Grant (wgrant)]

Gratuitous ILaunchBag import in lib/lp/bugs/model/bug.py. Also, I'm not a fan of "IDs" as a variable name.

Your bugtask target preloading serves the same role as something I did in lib/lp/bugs/scripts/bugtasktargetnamecaches.py, but rather differently. We probably want a well-known pattern for this sort of thing.

review: Approve (code*)

[Tim Penhey (thumper)]

I agree with William in that I don't like the "IDs" as a variable name. Even "ids" is better to me.

review: Approve (code)

[Robert Collins (lifeless)]

Will change to ids.

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2011-02-22 08:42:07 +0000
+++ lib/lp/bugs/browser/bugtask.py	2011-03-01 03:41:39 +0000
@@ -509,7 +509,7 @@
         # anonymous user is presented with a login screen at the correct URL,
         # rather than making it look as though this task was "not found",
         # because it was filtered out by privacy-aware code.
-        for bugtask in list(bug.bugtasks):
+        for bugtask in bug.bugtasks:
             if bugtask.target == context:
                 # Security proxy this object on the way out.
                 return getUtility(IBugTaskSet).get(bugtask.id)
@@ -650,6 +650,8 @@
             self.context = getUtility(ILaunchBag).bugtask
         else:
             self.context = context
+        list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+            [self.context.bug.ownerID], need_validity=True))
 
     @property
     def page_title(self):
@@ -3246,6 +3248,13 @@
         # nominations here, so we can pass it to getNominations() later
         # on.
         nominations = list(bug.getNominations())
+        # Eager load validity for all the persons we know of that will be
+        # displayed.
+        ids = set(map(attrgetter('ownerID'), nominations))
+        ids.discard(None)
+        if ids:
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                ids, need_validity=True))
 
         # Build a cache we can pass on to getConjoinedMaster(), so that
         # it doesn't have to iterate over all the bug tasks in each loop
@@ -3271,16 +3280,6 @@
                 for nomination in target_nominations
                 if nomination.status != BugNominationStatus.APPROVED)
 
-        # Fill the ValidPersonOrTeamCache cache (using getValidPersons()),
-        # so that checking person.is_valid_person, when rendering the
-        # link, won't issue a DB query.
-        assignees = set(
-            bugtask.assignee for bugtask in all_bugtasks
-            if bugtask.assignee is not None)
-        reporters = set(
-            bugtask.owner for bugtask in all_bugtasks)
-        getUtility(IPersonSet).getValidPersons(assignees.union(reporters))
-
         return bugtask_and_nomination_views
 
     @property

=== modified file 'lib/lp/bugs/configure.zcml'
--- lib/lp/bugs/configure.zcml	2011-02-21 04:23:02 +0000
+++ lib/lp/bugs/configure.zcml	2011-03-01 03:41:39 +0000
@@ -902,6 +902,7 @@
                     productseries
                     bug
                     owner
+                    ownerID
                     decider
                     target
                     status

=== modified file 'lib/lp/bugs/interfaces/bugnomination.py'
--- lib/lp/bugs/interfaces/bugnomination.py	2011-01-31 17:08:35 +0000
+++ lib/lp/bugs/interfaces/bugnomination.py	2011-03-01 03:41:39 +0000
@@ -33,7 +33,10 @@
     Reference,
     ReferenceChoice,
     )
-from zope.interface import Interface
+from zope.interface import (
+    Attribute,
+    Interface,
+    )
 from zope.schema import (
     Choice,
     Datetime,
@@ -130,6 +133,7 @@
     owner = exported(PublicPersonChoice(
         title=_('Submitter'), required=True, readonly=True,
         vocabulary='ValidPersonOrTeam'))
+    ownerID = Attribute('The db id of the owner.')
     decider = exported(PublicPersonChoice(
         title=_('Decided By'), required=False, readonly=True,
         vocabulary='ValidPersonOrTeam'))

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2011-02-23 23:46:00 +0000
+++ lib/lp/bugs/model/bug.py	2011-03-01 03:41:39 +0000
@@ -143,6 +143,7 @@
     )
 from lp.bugs.interfaces.bugnotification import IBugNotificationSet
 from lp.bugs.interfaces.bugtask import (
+    BugTaskSearchParams,
     BugTaskStatus,
     IBugTaskSet,
     UNRESOLVED_BUGTASK_STATUSES,
@@ -175,7 +176,10 @@
     IDistributionSourcePackage,
     )
 from lp.registry.interfaces.distroseries import IDistroSeries
-from lp.registry.interfaces.person import validate_public_person
+from lp.registry.interfaces.person import (
+    IPersonSet,
+    validate_public_person,
+    )
 from lp.registry.interfaces.product import IProduct
 from lp.registry.interfaces.productseries import IProductSeries
 from lp.registry.interfaces.series import SeriesStatus
@@ -549,15 +553,39 @@
     @cachedproperty
     def bugtasks(self):
         """See `IBug`."""
-        result = BugTask.select('BugTask.bug = %s' % sqlvalues(self.id))
-        result = result.prejoin(
-            ["assignee", "product", "sourcepackagename",
-             "owner", "bugwatch"])
-        # Do not use the default orderBy as the prejoins cause ambiguities
-        # across the tables.
-        result = result.orderBy("id")
-        result = sorted(result, key=bugtask_sort_key)
-        return result
+        # \o/ circular imports.
+        from lp.bugs.model.bugwatch import BugWatch
+        from lp.registry.model.distribution import Distribution
+        from lp.registry.model.distroseries import DistroSeries
+        from lp.registry.model.product import Product
+        from lp.registry.model.productseries import ProductSeries
+        from lp.registry.model.sourcepackagename import SourcePackageName
+        store = Store.of(self)
+        tasks = list(store.find(BugTask, BugTask.bugID == self.id))
+        # The bugtasks attribute is iterated in the API and web services, so it
+        # needs to preload all related data otherwise late evaluation is
+        # triggered in both places. Separately, bugtask_sort_key requires
+        # the related products, series, distros, distroseries and source
+        # package names to be loaded.
+        ids = set(map(operator.attrgetter('assigneeID'), tasks))
+        ids.update(map(operator.attrgetter('ownerID'), tasks))
+        ids.discard(None)
+        if ids:
+            list(getUtility(IPersonSet).getPrecachedPersonsFromIDs(
+                ids, need_validity=True))
+        def load_something(attrname, klass):
+            ids = set(map(operator.attrgetter(attrname), tasks))
+            ids.discard(None)
+            if not ids:
+                return
+            list(store.find(klass, klass.id.is_in(ids)))
+        load_something('productID', Product)
+        load_something('productseriesID', ProductSeries)
+        load_something('distributionID', Distribution)
+        load_something('distroseriesID', DistroSeries)
+        load_something('sourcepackagenameID', SourcePackageName)
+        list(store.find(BugWatch, BugWatch.bugID == self.id))
+        return sorted(tasks, key=bugtask_sort_key)
 
     @property
     def default_bugtask(self):

=== modified file 'lib/lp/registry/model/distribution.py'
--- lib/lp/registry/model/distribution.py	2011-02-24 15:57:05 +0000
+++ lib/lp/registry/model/distribution.py	2011-03-01 03:41:39 +0000
@@ -282,7 +282,7 @@
         SQLBase._init(self, *args, **kw)
         # Add a marker interface to set permissions for this kind
         # of distribution.
-        if self == getUtility(ILaunchpadCelebrities).ubuntu:
+        if self.name == 'ubuntu':
             alsoProvides(self, IBaseDistribution)
         else:
             alsoProvides(self, IDerivativeDistribution)