rbbz: Do all Bugzilla attachment actions together (bug 1249403). r?tif draft
authorMark Cote <mcote@mozilla.com>
Wed, 17 Feb 2016 17:34:03 -0500
changeset 7279 c806ab060d82cb734a945dada15e0aa5ddc63b12
parent 7278 1e3a215aa76dcc12c42f32cfcd105b8a49ae0c12
push id646
push usermcote@mozilla.com
push dateThu, 18 Feb 2016 21:04:33 +0000
reviewerstif
bugs1249403
rbbz: Do all Bugzilla attachment actions together (bug 1249403). r?tif This is another step toward switching to a single Bugzilla API call for attachment actions. We now prep all the attachment adds and updates and then execute them sequentially. This also gives a slight perf boost for large commit sets since we only grab a bug's attachments once per set, instead of once per commit. MozReview-Commit-ID: FONViwwJx8l
pylib/mozreview/mozreview/bugzilla/attachments.py
pylib/mozreview/mozreview/bugzilla/client.py
pylib/mozreview/mozreview/signal_handlers.py
--- a/pylib/mozreview/mozreview/bugzilla/attachments.py
+++ b/pylib/mozreview/mozreview/bugzilla/attachments.py
@@ -1,27 +1,33 @@
 from __future__ import unicode_literals
 
 from mozautomation.commitparser import (
     strip_commit_metadata,
 )
 
+from mozreview.bugzilla.client import (
+    BugzillaAttachmentUpdates,
+)
 from mozreview.models import (
     BugzillaUserMap,
     get_or_create_bugzilla_users,
 )
 from mozreview.rb_utils import (
     get_obj_url,
 )
 
 
 def update_bugzilla_attachments(bugzilla, bug_id, children_to_post,
                                 children_to_obsolete):
+    attachment_updates = BugzillaAttachmentUpdates(bugzilla)
+
     for child in children_to_obsolete:
-        bugzilla.obsolete_review_attachments(bug_id, get_obj_url(child))
+        attachment_updates.obsolete_review_attachments(bug_id,
+                                                       get_obj_url(child))
 
     # We publish attachments for each commit/child to Bugzilla so that
     # reviewers can easily track their requests.
 
     # The review request exposes a list of usernames for reviewers. We need
     # to convert these to Bugzilla emails in order to make the request into
     # Bugzilla.
     #
@@ -103,14 +109,17 @@ def update_bugzilla_attachments(bugzilla
                         rr_url
                     )
             else:
                 comment = ('Review request updated; see interdiff: '
                            '%sdiff/%d-%d/\n' % (rr_url,
                                                 diffset_count,
                                                 diffset_count + 1))
 
-        bugzilla.post_rb_url(bug_id,
-                             review_request.id,
-                             review_request_draft.summary,
-                             comment,
-                             diff_url,
-                             reviewers)
+        attachment_updates.create_or_update_attachment(
+            bug_id,
+            review_request.id,
+            review_request_draft.summary,
+            comment,
+            diff_url,
+            reviewers)
+
+    attachment_updates.do_updates()
--- a/pylib/mozreview/mozreview/bugzilla/client.py
+++ b/pylib/mozreview/mozreview/bugzilla/client.py
@@ -44,16 +44,162 @@ def xmlrpc_to_bugzilla_errors(func):
 
             if e.args:
                 msg = e.args[0]
 
             raise BugzillaError('IOError: %s' % msg)
     return _transform_errors
 
 
+class BugzillaAttachmentUpdates(object):
+
+    def __init__(self, bugzilla):
+        self.bugzilla = bugzilla
+        self.attachments = {}
+        self.updates = []
+        self.creates = []
+
+    @xmlrpc_to_bugzilla_errors
+    def create_or_update_attachment(self, bug_id, review_request_id, summary,
+                                    comment, url, reviewers):
+        """Creates or updates an attachment containing a review-request URL.
+
+        The reviewers argument should be a dictionary mapping reviewer email
+        to a boolean indicating if that reviewer has given an r+ on the
+        attachment in the past that should be left untouched.
+        """
+
+        logger.info('Posting review request %s to bug %d.' %
+                    (review_request_id, bug_id))
+        # Copy because we modify it.
+        reviewers = reviewers.copy()
+        params = {}
+        flags = []
+        rb_attachment = None
+
+        self._update_attachments(bug_id)
+
+        # Find the associated attachment, then go through the review flags.
+        for a in self.attachments[bug_id]:
+
+            # Make sure we check for old-style URLs as well.
+            if not self.bugzilla._rb_attach_url_matches(a['data'], url):
+                continue
+
+            rb_attachment = a
+
+            for f in a.get('flags', []):
+                if f['name'] not in ['review', 'feedback']:
+                    # We only care about review and feedback flags.
+                    continue
+                elif f['name'] == 'feedback':
+                    # We always clear feedback flags.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['status'] == '+' and f['setter'] not in reviewers:
+                    # This r+ flag was set manually on bugzilla rather
+                    # then through a review on Review Board. Always
+                    # clear these flags.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['status'] == '+':
+                    if not reviewers[f['setter']]:
+                        # We should not carry this r+ forward so
+                        # re-request review.
+                        flags.append({
+                            'id': f['id'],
+                            'name': 'review',
+                            'status': '?',
+                            'requestee': f['setter']
+                        })
+
+                    reviewers.pop(f['setter'])
+                elif 'requestee' not in f or f['requestee'] not in reviewers:
+                    # We clear review flags where the requestee is not
+                    # a reviewer or someone has manually set r- on the
+                    # attachment.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['requestee'] in reviewers:
+                    # We're already waiting for a review from this user
+                    # so don't touch the flag.
+                    reviewers.pop(f['requestee'])
+
+            break
+
+        # Add flags for new reviewers.
+
+        # Sorted so behavior is deterministic (this mucks with test output
+        # otherwise).
+        for r in sorted(reviewers.keys()):
+            flags.append({
+                'name': 'review',
+                'status': '?',
+                'requestee': r,
+                'new': True
+            })
+
+        if rb_attachment:
+            params['ids'] = [rb_attachment['id']]
+
+            if rb_attachment['is_obsolete']:
+                params['is_obsolete'] = False
+        else:
+            params['ids'] = [bug_id]
+            params['data'] = url
+            params['content_type'] = 'text/x-review-board-request'
+
+        params['file_name'] = 'reviewboard-%d-url.txt' % review_request_id
+        params['summary'] = "MozReview Request: %s" % summary
+        params['comment'] = comment
+        if flags:
+            params['flags'] = flags
+
+        if rb_attachment:
+            self.updates.append(params)
+        else:
+            self.creates.append(params)
+
+    @xmlrpc_to_bugzilla_errors
+    def obsolete_review_attachments(self, bug_id, rb_url):
+        """Mark any attachments for a given bug and review request as obsolete.
+
+        This is called when review requests are discarded or deleted. We don't
+        want to leave any lingering references in Bugzilla.
+        """
+        params = {'ids': [], 'is_obsolete': True}
+
+        self._update_attachments(bug_id)
+
+        for a in self.attachments[bug_id]:
+            if (self.bugzilla._rb_attach_url_matches(a.get('data'), rb_url) and
+                not a.get('is_obsolete')):
+                params['ids'].append(a['id'])
+
+        if params['ids']:
+            logger.info('Obsoleting attachments on bug %d: %s' % (
+                        bug_id, params['ids']))
+            self.updates.append(params)
+
+    @xmlrpc_to_bugzilla_errors
+    def do_updates(self):
+        for params in self.creates:
+            self.bugzilla.proxy.Bug.add_attachment(
+                self.bugzilla._auth_params(params))
+
+        for params in self.updates:
+            self.bugzilla.proxy.Bug.update_attachment(
+                self.bugzilla._auth_params(params))
+
+        self.creates = []
+        self.updates = []
+
+    @xmlrpc_to_bugzilla_errors
+    def _update_attachments(self, bug_id):
+        if bug_id not in self.attachments:
+            self.attachments[bug_id] = self.bugzilla.get_rb_attachments(bug_id)
+
+
 class Bugzilla(object):
     """
     Interface to a Bugzilla system.
 
     At the moment this uses the XMLRPC API.  It should probably be converted
     to REST at some point.
 
     General FIXME: try to get more specific errors out of xmlrpclib.Fault
@@ -166,113 +312,16 @@ class Bugzilla(object):
                 logger.info('Bug %d is confidential.' % bug_id)
                 return True
             raise
 
         logger.info('Bug %d confidential: %s.' % (bug_id, bool(groups)))
         return bool(groups)
 
     @xmlrpc_to_bugzilla_errors
-    def post_rb_url(self, bug_id, review_request_id, summary, comment, url,
-                    reviewers):
-        """Creates or updates an attachment containing a review-request URL.
-
-        The reviewers argument should be a dictionary mapping reviewer email
-        to a boolean indicating if that reviewer has given an r+ on the
-        attachment in the past that should be left untouched.
-        """
-
-        logger.info('Posting review request %s to bug %d.' %
-                    (review_request_id, bug_id))
-        # Copy because we modify it.
-        reviewers = reviewers.copy()
-        params = self._auth_params({})
-        flags = []
-        rb_attachment = None
-        attachments = self.get_rb_attachments(bug_id)
-
-        # Find the associated attachment, then go through the review flags.
-        for a in attachments:
-
-            # Make sure we check for old-style URLs as well.
-            if not self._rb_attach_url_matches(a['data'], url):
-                continue
-
-            rb_attachment = a
-
-            for f in a.get('flags', []):
-                if f['name'] not in ['review', 'feedback']:
-                    # We only care about review and feedback flags.
-                    continue
-                elif f['name'] == 'feedback':
-                    # We always clear feedback flags.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['status'] == '+' and f['setter'] not in reviewers:
-                    # This r+ flag was set manually on bugzilla rather
-                    # then through a review on Review Board. Always
-                    # clear these flags.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['status'] == '+':
-                    if not reviewers[f['setter']]:
-                        # We should not carry this r+ forward so
-                        # re-request review.
-                        flags.append({
-                            'id': f['id'],
-                            'name': 'review',
-                            'status': '?',
-                            'requestee': f['setter']
-                        })
-
-                    reviewers.pop(f['setter'])
-                elif 'requestee' not in f or f['requestee'] not in reviewers:
-                    # We clear review flags where the requestee is not
-                    # a reviewer or someone has manually set r- on the
-                    # attachment.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['requestee'] in reviewers:
-                    # We're already waiting for a review from this user
-                    # so don't touch the flag.
-                    reviewers.pop(f['requestee'])
-
-            break
-
-        # Add flags for new reviewers.
-
-        # Sorted so behavior is deterministic (this mucks with test output
-        # otherwise).
-        for r in sorted(reviewers.keys()):
-            flags.append({
-                'name': 'review',
-                'status': '?',
-                'requestee': r,
-                'new': True
-            })
-
-        if rb_attachment:
-            params['ids'] = [rb_attachment['id']]
-
-            if rb_attachment['is_obsolete']:
-                params['is_obsolete'] = False
-        else:
-            params['ids'] = [bug_id]
-            params['data'] = url
-            params['content_type'] = 'text/x-review-board-request'
-
-        params['file_name'] = 'reviewboard-%d-url.txt' % review_request_id
-        params['summary'] = "MozReview Request: %s" % summary
-        params['comment'] = comment
-        if flags:
-            params['flags'] = flags
-
-        if rb_attachment:
-            self.proxy.Bug.update_attachment(params)
-        else:
-            self.proxy.Bug.add_attachment(params)
-
-    @xmlrpc_to_bugzilla_errors
     def get_rb_attachments(self, bug_id):
         """Get all attachments that contain review-request URLs."""
 
         params = self._auth_params({
             'ids': [bug_id],
             'include_fields': ['id', 'data', 'content_type', 'is_obsolete',
                                'flags']
         })
@@ -375,38 +424,16 @@ class Bugzilla(object):
 
         if comment:
             params['comment'] = comment
 
         self.proxy.Bug.update_attachment(params)
         return True
 
     @xmlrpc_to_bugzilla_errors
-    def obsolete_review_attachments(self, bug_id, rb_url):
-        """Mark any attachments for a given bug and review request as obsolete.
-
-        This is called when review requests are discarded or deleted. We don't
-        want to leave any lingering references in Bugzilla.
-        """
-        params = self._auth_params({
-            'ids': [],
-            'is_obsolete': True,
-        })
-
-        for a in self.get_rb_attachments(bug_id):
-            if (self._rb_attach_url_matches(a.get('data'), rb_url) and
-                not a.get('is_obsolete')):
-                params['ids'].append(a['id'])
-
-        if params['ids']:
-            logger.info('Obsoleting attachments on bug %d: %s' % (
-                        bug_id, params['ids']))
-            self.proxy.Bug.update_attachment(params)
-
-    @xmlrpc_to_bugzilla_errors
     def valid_api_key(self, username, api_key):
         try:
             return self.proxy.User.valid_login({
                 'login': username,
                 'api_key': api_key,
             })
         except xmlrpclib.Fault as e:
             # Invalid API-key formats (e.g. not 40 characters long) or expired
--- a/pylib/mozreview/mozreview/signal_handlers.py
+++ b/pylib/mozreview/mozreview/signal_handlers.py
@@ -28,16 +28,17 @@ from reviewboard.reviews.signals import 
     review_request_reopened,
 )
 
 from mozreview.bugzilla.attachments import (
     update_bugzilla_attachments,
 )
 from mozreview.bugzilla.client import (
     Bugzilla,
+    BugzillaAttachmentUpdates,
 )
 from mozreview.bugzilla.errors import (
     BugzillaError,
     bugzilla_to_publish_errors,
 )
 from mozreview.errors import (
     CommitPublishProhibited,
     ConfidentialBugError,
@@ -491,17 +492,19 @@ def on_review_request_closed_discarded(u
                                      AUTO_CLOSE_DESCRIPTION,
                                      commit_data=commit_data)
     else:
         # TODO: Remove this once we properly prevent users from closing
         # commit review requests.
         b = Bugzilla(get_bugzilla_api_key(user))
         bug = int(review_request.get_bug_list()[0])
         diff_url = '%sdiff/#index_header' % get_obj_url(review_request)
-        b.obsolete_review_attachments(bug, diff_url)
+        attachment_updates = BugzillaAttachmentUpdates(b)
+        attachment_updates.obsolete_review_attachments(bug, diff_url)
+        attachment_updates.do_updates()
 
 
 def on_review_request_closed_submitted(user, review_request, type, **kwargs):
     if type != ReviewRequest.SUBMITTED:
         return
 
     commit_data = fetch_commit_data(review_request)