mozreview: Do all Bugzilla attachment actions together (
bug 1237491). r?smacleod
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: 8RXnMzz5r7r
--- a/pylib/mozreview/mozreview/bugzilla/attachments.py
+++ b/pylib/mozreview/mozreview/bugzilla/attachments.py
@@ -1,30 +1,35 @@
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,
)
from mozreview.review_helpers import (
gen_latest_reviews
)
def update_bugzilla_attachments(bugzilla, bug_id, children_to_post,
children_to_obsolete):
+ attachment_updates = BugzillaAttachmentUpdates(bugzilla, bug_id)
+
for child in children_to_obsolete:
- bugzilla.obsolete_review_attachments(bug_id, get_obj_url(child))
+ attachment_updates.obsolete_review_attachments(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.
#
@@ -95,14 +100,16 @@ 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(
+ 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,167 @@ def xmlrpc_to_bugzilla_errors(func):
if e.args:
msg = e.args[0]
raise BugzillaError('IOError: %s' % msg)
return _transform_errors
+class BugzillaAttachmentUpdates(object):
+ """Create and update attachments.
+
+ This class provides methods to queue up a series of operations on one
+ or more attachments and then execute them all together. It caches
+ attachment information to avoid repeatedly polling Bugzilla.
+
+ All attachments to be created or updated must belong to the same bug.
+ """
+
+ def __init__(self, bugzilla, bug_id):
+ self.bugzilla = bugzilla
+ self.bug_id = bug_id
+ self.attachments = []
+ self.updates = []
+ self.creates = []
+
+ def create_or_update_attachment(self, 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, self.bug_id))
+ # Copy because we modify it.
+ reviewers = reviewers.copy()
+ params = {}
+ flags = []
+ rb_attachment = None
+
+ self._update_attachments()
+
+ # Find the associated attachment, then go through the review flags.
+ for a in self.attachments:
+
+ # 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'] = [self.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)
+
+ def obsolete_review_attachments(self, 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()
+
+ for a in self.attachments:
+ 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' % (
+ self.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))
+ self.creates = self.creates[1:]
+
+ for params in self.updates:
+ self.bugzilla.proxy.Bug.update_attachment(
+ self.bugzilla._auth_params(params))
+ self.updates = self.updates[1:]
+
+ def _update_attachments(self):
+ if not self.attachments:
+ self.attachments = self.bugzilla.get_rb_attachments(self.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 +317,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 +429,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, bug)
+ attachment_updates.obsolete_review_attachments(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)