Merge lp:~deryck/launchpad/preload-tags-for-buglistings-901122 into lp:launchpad

[Deryck Hodge]

[r=lifeless][bug=901122][incr] Load tags in a single query for custom buglistings.

This is the first branch of refactorings to load new data that was added in new buglistings without adding more queries. This first bit here is to get tags loaded as a set for a given buglisting batch, rather than accessing them in a loop for each item in the buglist.

I followed the pattern used for bug_badge_properties, which calls out to a method on IBugTaskSet to get bug badge data. In my case, I added tags_for_batch which calls out to IBugTaskSet.getBugTaskTags. getBugTaskTags is where the query happens, and then _getListingItem uses tags_for_batch, which is cached, to get the tags for the listing item.

There were a few test updates required, since the required arguments changed for BugTaskListingItem. I also updated doc/bugtask.txt to test the new BugTaskSet method. Otherwise, it's a pretty straight forward change.

[Robert Collins (lifeless)]

This will certainly do the job. I have some things I would have done differently that you may wish to do, or not.

Firstly, using tags='' to indicate no-tags is horribly confusing when tags is a list the rest of the time. It works (because '' is iterable) but its confusing. I suggest using () instead.

Secondly, load_referencing or load_related would quite likely let you bring back all the tags with less code.
something like:
bug_to_task = dict((task.bugID, task.id) for task in tasks)
tags = dict((bug_to_task[bugtag.bugID], bugtag.tag for bugtag in
    load_referencing(BugTag, tasks, ["bugID"]))

Third, tags_for_batch is only used once and isn't directly tested. It seems a little ugly but I can see why you have it. I'd consider just passing the result down the call stack rather than caching it for the lifetime of the view.

I'd like to see a scaling test for this. Unless this is the entire remaining lazy load it won't be constant, but it will make the scaling clear and documented.

review: Approve

[Robert Collins (lifeless)]

Oh, and as you've added a helper with the tags, consider moving the url processing stuff into that, perhaps.

[Deryck Hodge (deryck)]

Thanks for the review, Robert. I definitely updated to use () rather than '' for empty tags. Thanks for the catch. Not sure what I was thinking there.

In terms of using load_referencing, unless I'm misunderstanding how that works, it won't work because BugTag is not directly related to tasks. I need to join on Bug, hence the direct use of store.find.

And for the other suggestions, I'd like to keep what I have since it follows that pattern of what is already there and makes less change to existing code, while still reducing the query count. I don't think the existing patterns are so off here that it merits diverging partially.

[Robert Collins (lifeless)]

On Thu, Mar 1, 2012 at 8:42 AM, Deryck Hodge <email address hidden> wrote:
> Thanks for the review, Robert. I definitely updated to use () rather than '' for empty tags.  Thanks for the catch.  Not sure what I was thinking there.
>
> In terms of using load_referencing, unless I'm misunderstanding how that works, it won't work because BugTag is not directly related to tasks.  I need to join on Bug, hence the direct use of store.find.

You need to query on the bug id, which task has. So yes, you're
misunderstanding how it works :)

-Rob

[Deryck Hodge (deryck)]

I feel dumb, sorry. Help me understand what I'm not groking. Here's my train of thought...

Your example above didn't work for me:
load_referencing(BugTag, tasks, ["bugID"])

To get it to work, I had to pass in bugs rather than tasks, which made sense to me, since it's bugtag to bug where the reference lies:
load_referencing(BugTag, bugs, ["bugID"])

Which required something like set([bugtask.bug for bugtask in bugtasks]), and, of course, being a loop is bad for query count. At which point I would have had to issue a store.find to get all bugs in a single query, and then load_referencing would issue another query. Which seems like 2 queries, whereas my original code did the join and got back tags in one query.

We can also chat on IRC about this, if you need more bandwidth to undumb me. :)

=== modified file 'lib/lp/bugs/browser/bugtask.py'
--- lib/lp/bugs/browser/bugtask.py	2012-02-23 22:27:32 +0000
+++ lib/lp/bugs/browser/bugtask.py	2012-02-29 19:30:31 +0000
@@ -2161,13 +2161,14 @@
     delegates(IBugTask, 'bugtask')
 
     def __init__(self, bugtask, has_bug_branch,
-                 has_specification, has_patch, request=None,
+                 has_specification, has_patch, tags, request=None,
                  target_context=None):
         self.bugtask = bugtask
         self.review_action_widget = None
         self.has_bug_branch = has_bug_branch
         self.has_specification = has_specification
         self.has_patch = has_patch
+        self.tags = tags
         self.request = request
         self.target_context = target_context
 
@@ -2230,7 +2231,7 @@
             'status': self.status.title,
             'status_class': 'status' + self.status.name,
             'tags': [{'url': base_tag_url + urllib.quote(tag), 'tag': tag}
-                for tag in self.bug.tags],
+                for tag in self.tags],
             'title': self.bug.title,
             }
 
@@ -2275,6 +2276,11 @@
         return getUtility(IBugTaskSet).getBugTaskBadgeProperties(
             self.currentBatch())
 
+    @cachedproperty
+    def tags_for_batch(self):
+        """Return a dict matching bugtask to it's tags."""
+        return getUtility(IBugTaskSet).getBugTaskTags(self.currentBatch())
+
     def getCookieName(self):
         """Return the cookie name used in bug listings js code."""
         cookie_name_template = '%s-buglist-fields'
@@ -2310,6 +2316,7 @@
     def _getListingItem(self, bugtask):
         """Return a decorated bugtask for the bug listing."""
         badge_property = self.bug_badge_properties[bugtask]
+        tags = self.tags_for_batch.get(bugtask.id, ())
         if (IMaloneApplication.providedBy(self.target_context) or
             IPerson.providedBy(self.target_context)):
             # XXX Tom Berger bug=529846
@@ -2323,6 +2330,7 @@
             badge_property['has_branch'],
             badge_property['has_specification'],
             badge_property['has_patch'],
+            tags,
             request=self.request,
             target_context=target_context)
 

=== modified file 'lib/lp/bugs/browser/cvereport.py'
--- lib/lp/bugs/browser/cvereport.py	2012-01-24 15:15:18 +0000
+++ lib/lp/bugs/browser/cvereport.py	2012-02-29 19:30:31 +0000
@@ -91,7 +91,8 @@
                 bugtask,
                 has_bug_branch=badges['has_branch'],
                 has_specification=badges['has_specification'],
-                has_patch=badges['has_patch'])
+                has_patch=badges['has_patch'],
+                tags=())
             if bugtask.status in RESOLVED_BUGTASK_STATUSES:
                 current_bugtaskcves = resolved_bugtaskcves
             else:

=== modified file 'lib/lp/bugs/browser/tests/test_bugtask.py'
--- lib/lp/bugs/browser/tests/test_bugtask.py	2012-02-24 04:00:24 +0000
+++ lib/lp/bugs/browser/tests/test_bugtask.py	2012-02-29 19:30:31 +0000
@@ -1793,7 +1793,7 @@
             batched_view.activity_and_comments)
 
 
-def make_bug_task_listing_item(factory):
+def make_bug_task_listing_item(factory, tags=()):
     owner = factory.makePerson()
     bug = factory.makeBug(
         owner=owner, private=True, security_related=True)
@@ -1808,6 +1808,7 @@
         badge_property['has_branch'],
         badge_property['has_specification'],
         badge_property['has_patch'],
+        tags,
         target_context=bugtask.target)
 
 
@@ -2339,9 +2340,9 @@
 
     def test_model_tags(self):
         """Model contains bug tags."""
-        owner, item = make_bug_task_listing_item(self.factory)
+        tags = ['tag1', 'tag2']
+        owner, item = make_bug_task_listing_item(self.factory, tags=tags)
         with person_logged_in(owner):
-            item.bug.tags = ['tag1', 'tag2']
             self.assertEqual(2, len(item.model['tags']))
             self.assertTrue('tag' in item.model['tags'][0].keys())
             self.assertTrue('url' in item.model['tags'][0].keys())

=== modified file 'lib/lp/bugs/doc/bugtask.txt'
--- lib/lp/bugs/doc/bugtask.txt	2012-02-08 10:33:31 +0000
+++ lib/lp/bugs/doc/bugtask.txt	2012-02-29 19:30:31 +0000
@@ -1153,6 +1153,21 @@
      has_specification: False
 
 
+Bugtask Tags
+------------
+
+List of bugtasks often need to display related tags.  Since tags are
+related to bugtasks via bugs, BugTaskSet has a method getBugTaskTags
+that can calculate the tags in one query.
+
+    >>> some_bugtask.bug.tags = [u'foo', u'bar']
+    >>> another_bugtask.bug.tags = [u'baz', u'bop']
+    >>> tags_by_task = getUtility(IBugTaskSet).getBugTaskTags([
+    ...     some_bugtask, another_bugtask])
+    >>> print tags_by_task
+    {3: [u'foo', u'bar'], 6: [u'bop', u'baz']}
+
+
 Similar bugs
 ------------
 

=== modified file 'lib/lp/bugs/interfaces/bugtask.py'
--- lib/lp/bugs/interfaces/bugtask.py	2012-02-28 21:35:06 +0000
+++ lib/lp/bugs/interfaces/bugtask.py	2012-02-29 19:30:31 +0000
@@ -1475,6 +1475,12 @@
         :return: A dictionary mapping the bugs to their bugtasks.
         """
 
+    def getBugTaskTags(bugtasks):
+        """Return a set of bugtasks bug tags
+
+        Return a dict mapping from bugtask to tag.
+        """
+
     def getBugTaskBadgeProperties(bugtasks):
         """Return whether the bugtasks should have badges.
 

=== modified file 'lib/lp/bugs/model/bugtask.py'
--- lib/lp/bugs/model/bugtask.py	2012-02-15 08:13:51 +0000
+++ lib/lp/bugs/model/bugtask.py	2012-02-29 19:30:31 +0000
@@ -21,6 +21,7 @@
     ]
 
 
+from collections import defaultdict
 import datetime
 from itertools import chain
 from operator import attrgetter
@@ -1380,6 +1381,23 @@
             bugs_and_tasks[bug].append(task)
         return bugs_and_tasks
 
+    def getBugTaskTags(self, bugtasks):
+        """See `IBugTaskSet`"""
+        # Import locally to avoid circular imports.
+        from lp.bugs.model.bug import Bug, BugTag
+        bugtask_ids = set(bugtask.id for bugtask in bugtasks)
+        bug_ids = set(bugtask.bugID for bugtask in bugtasks)
+        tags = IStore(BugTag).find(
+            (BugTag.tag, BugTask.id),
+            BugTask.bug == Bug.id,
+            BugTag.bug == Bug.id,
+            BugTag.bugID.is_in(bug_ids),
+            BugTask.id.is_in(bugtask_ids))
+        tags_by_bugtask = defaultdict(list)
+        for tag_name, bugtask_id in tags:
+            tags_by_bugtask[bugtask_id].append(tag_name)
+        return dict(tags_by_bugtask)
+
     def getBugTaskBadgeProperties(self, bugtasks):
         """See `IBugTaskSet`."""
         # Import locally to avoid circular imports.

=== modified file 'lib/lp/registry/browser/milestone.py'
--- lib/lp/registry/browser/milestone.py	2012-02-21 22:46:28 +0000
+++ lib/lp/registry/browser/milestone.py	2012-02-29 19:30:31 +0000
@@ -254,11 +254,13 @@
     def _getListingItem(self, bugtask):
         """Return a decorated bugtask for the bug listing."""
         badge_property = self._bug_badge_properties[bugtask]
+        tags = ()
         return BugTaskListingItem(
             bugtask,
             badge_property['has_branch'],
             badge_property['has_specification'],
-            badge_property['has_patch'])
+            badge_property['has_patch'],
+            tags)
 
     @cachedproperty
     def bugtasks(self):